mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Steffen Rochel <steffenroc...@gmail.com>
Subject Re: Module maintainers proposal
Date Mon, 15 Jan 2018 00:18:17 GMT
Thanks Chris for the great reading suggestion
<http://www.unterstein.net/su/docs/CathBaz.pdf>!

I'm suggesting that we adopt Mu's proposal to use github code owner
mechanism to identify designated maintainer for each package.
To address the concerns raised in this thread I proposed
 to add into the header of the CODEOWNERS file
https://github.com/apache/incubator-mxnet/pull/9426
(changes below).

Chris, Sebastian, Isabel - please suggest changes, but I hope I addressed
your concerns.

In the future we can also enable required reviews (see
https://help.github.com/articles/about-pull-request-reviews/), but I would
suggest to make one change at a time.

I do suggest we should explore how we can best adopt existing github
features before considering building additional CI tasks.

Steffen

# Please see documentation of use of CODEOWNERS file at
# https://help.github.com/articles/about-codeowners/ and
# https://github.com/blog/2392-introducing-code-owners
#
# The first owner listed for a package is considered the maintainer for a
package.
# Anybody can add themselves or a team (see
https://help.github.com/articles/about-teams/)
# as additional owners to get notified about changes in a specific package.
#
# By default the package maintainer should merge PR after appropriate
review.
# A PR which received 2 +1 (or LGTM) comments can be merged by any
committer.
# In the future we might consider adopting required reviews
# (see https://help.github.com/articles/about-pull-request-reviews/)


On Fri, Jan 12, 2018 at 7:22 PM Bhavin Thaker <bhavinthaker@gmail.com>
wrote:

> During the MXNet 1.0 release, there was feedback from the mentors and folks
> in general@ to clarify at the top of the CODEOWNERs file on what the
> contents of this file meant.
>
> Hi Mu,
>
> Please add the description of the file in the file header. I expect that
> this will be a requirement for the next MXNet release 1.0.1.
>
> Thanks,
> Bhavin Thaker.
>
> On Fri, Jan 12, 2018 at 5:43 PM Chris Olivier <cjolivier01@gmail.com>
> wrote:
>
> > i’d be +1 if CODEOWNERS file has a big note at the top saying basically
> > it’s just for watching code changes that you’d like to know about (to
> > review or just to follow) and that anyone can add themself.
> >
> > On Fri, Jan 12, 2018 at 1:58 PM Chris Olivier <cjolivier01@gmail.com>
> > wrote:
> >
> > > Does it have to be called "CODEOWNERS"? I would be more comfortable
> with
> > > it if it's a "watch list" where it just means you wish to watch code
> here
> > > or there in the source structure and anyone can add or remove their
> name
> > > from watching some part of the code at any time.
> > >
> > > On Fri, Jan 12, 2018 at 11:52 AM, Marco de Abreu <
> > > marco.g.abreu@googlemail.com> wrote:
> > >
> > >> I agree. How about we find another way to allow people to subscribe
> for
> > >> changes in a specific file or directory?
> > >>
> > >> -Marco
> > >>
> > >> Am 12.01.2018 8:51 nachm. schrieb "Chris Olivier" <
> > cjolivier01@gmail.com
> > >> >:
> > >>
> > >> > Have you read "The Cathedral and the Bazaar"?
> > >> >
> > >> > http://www.unterstein.net/su/docs/CathBaz.pdf
> > >> >
> > >> > One of the points I took from this is that once a project finds its
> > >> stride,
> > >> > it actually runs more efficiently without centralization than with.
> > >> >
> > >> > -Chris
> > >> >
> > >> > On Fri, Jan 12, 2018 at 11:10 AM, Marco de Abreu <
> > >> > marco.g.abreu@googlemail.com> wrote:
> > >> >
> > >> > > Hi Chris,
> > >> > >
> > >> > > you have a good point about people being afraid of reviewing
PRs
> > which
> > >> > they
> > >> > > are not assigned to and I totally agree that we should encourage
> > >> > everybody
> > >> > > to review PRs.
> > >> > >
> > >> > > One important advantage I see in this is the notification: since
> we
> > >> are
> > >> > not
> > >> > > using the feature to required an approval, this step is entirely
> for
> > >> > > information purpose. I, for example, would like to get notified
> if a
> > >> PR
> > >> > to
> > >> > > change a CI file would be created. Just as an example: over
> > >> Christmas, a
> > >> > PR
> > >> > > to update mkl has been pushed without me knowing about it.
> Somehow,
> > >> after
> > >> > > my vacation, we started to get issues with mkl test - I only
found
> > out
> > >> > > about this PR after quite a long investigation. If we would extend
> > the
> > >> > > usage of the code maintainers, we'll make sure that changes like
> > these
> > >> > will
> > >> > > notify the people who have the best knowledge about that part.
> > >> > >
> > >> > > Marco
> > >> > >
> > >> > > Am 12.01.2018 8:03 nachm. schrieb "Chris Olivier" <
> > >> cjolivier01@gmail.com
> > >> > >:
> > >> > >
> > >> > > > -1 (binding)
> > >> > > >
> > >> > > > I totally understand the motivation for this (I've definitely
> > saved
> > >> > > myself
> > >> > > > some grief by getting called out automatically for
> CMakeLists.txt
> > >> > stuff,
> > >> > > > for example), but I respectfully decline for the following
> > >> reason(s):
> > >> > > >
> > >> > > > I feel that defining code-owners has some negative effects.
> > >> > > >
> > >> > > > Other committers may be reluctant to start reviewing and
> approving
> > >> PRs
> > >> > > > since they aren't the one listed, so I feel this will in
the
> > >> long-run
> > >> > > > reduce the number of people doing code reviews.
> > >> > > >
> > >> > > > If there aren't enough people doing PR's, then people can
> complain
> > >> on
> > >> > > dev@
> > >> > > > asking for review.
> > >> > > >
> > >> > > > -Chris
> > >> > > >
> > >> > > >
> > >> > > >
> > >> > > >
> > >> > > > On Fri, Jan 12, 2018 at 10:41 AM, Haibin Lin <haibin@apache.org
> >
> > >> > wrote:
> > >> > > >
> > >> > > > > +1 (binding)
> > >> > > > >
> > >> > > > > On 2018-01-12 10:10, kellen sunderland <
> > >> kellen.sunderland@gmail.com>
> > >> > > > > wrote:
> > >> > > > > > +1 (non-binding)
> > >> > > > > >
> > >> > > > > > On Jan 12, 2018 6:32 PM, "Steffen Rochel" <
> > >> steffenrochel@gmail.com
> > >> > >
> > >> > > > > wrote:
> > >> > > > > >
> > >> > > > > > > I propose to adopt the proposal.
> > >> > > > > > > +1 (non-binding)
> > >> > > > > > >
> > >> > > > > > > Steffen
> > >> > > > > > >
> > >> > > > > > > On Wed, Jan 10, 2018 at 8:39 PM Mu Li <muli.cmu@gmail.com
> >
> > >> > wrote:
> > >> > > > > > >
> > >> > > > > > > > Hi Isabel,
> > >> > > > > > > >
> > >> > > > > > > > My apologies that not saying that clearly.
> > >> > > > > > > >
> > >> > > > > > > > The purpose of this proposal is encouraging
more
> > >> contributors
> > >> > to
> > >> > > > help
> > >> > > > > > > > review and merge PRs. And also hope
to shorten the time
> > for
> > >> a
> > >> > PR
> > >> > > to
> > >> > > > > be
> > >> > > > > > > > merged. After assigning maintainers
to modules, then PR
> > >> > > > contributors
> > >> > > > > can
> > >> > > > > > > > easily contact the reviewers. In other
words, github
> will
> > >> > > > > automatically
> > >> > > > > > > > assign the PR to the maintainer and
send a notification
> > >> email.
> > >> > > > > > > >
> > >> > > > > > > > I don't think I put the term "inbox"
in my proposal. I
> > never
> > >> > > > > discussed
> > >> > > > > > > PRs
> > >> > > > > > > > with other contributors by sending email
directly, which
> > is
> > >> > less
> > >> > > > > > > effective
> > >> > > > > > > > than just using github. I also don't
aware any other
> > >> > contributor
> > >> > > > use
> > >> > > > > the
> > >> > > > > > > > direct email way. So I didn't clarify
it on the
> proposal.
> > >> > > > > > > >
> > >> > > > > > > > On Tue, Jan 9, 2018 at 11:47 AM, Isabel
Drost-Fromm <
> > >> > > > > isabel@apache.org>
> > >> > > > > > > > wrote:
> > >> > > > > > > >
> > >> > > > > > > > >
> > >> > > > > > > > >
> > >> > > > > > > > > Am 9. Januar 2018 18:25:50 MEZ
schrieb Mu Li <
> > >> > > muli.cmu@gmail.com
> > >> > > > >:
> > >> > > > > > > > > >We should encourage to contract
a specific
> contributor
> > >> for
> > >> > > > issues
> > >> > > > > and
> > >> > > > > > > > > >PRs.
> > >> > > > > > > > >
> > >> > > > > > > > > My head translates "encourage to
contact specific
> > >> > contributor"
> > >> > > > into
> > >> > > > > > > > > "encourage to contact specific
contributors inbox".
> This
> > >> > > > translated
> > >> > > > > > > > version
> > >> > > > > > > > > is what I would highly discourage.
> > >> > > > > > > > >
> > >> > > > > > > > > See the disclaimer here for reasons
behind that:
> > >> > > > > > > > >
> > >> > > > > > > > > https://home.apache.org/~hossman/#private_q
> > >> > > > > > > > >
> > >> > > > > > > > >
> > >> > > > > > > > > Isabel
> > >> > > > > > > > > --
> > >> > > > > > > > > Diese Nachricht wurde von meinem
Android-Gerät mit K-9
> > >> Mail
> > >> > > > > gesendet.
> > >> > > > > > > > >
> > >> > > > > > > >
> > >> > > > > > >
> > >> > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message