mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sandeep krishnamurthy <sandeep.krishn...@gmail.com>
Subject Re: Module maintainers proposal
Date Mon, 15 Jan 2018 04:00:13 GMT
Just wanted to clarify my understanding.
1. We are going to use Github CODEOWNERS functionality as a feature for
getting notified.
2. This does not mean only CODEOWNERS approved code will be merged for
respective module. (We need to evolve community to self-sustain without
getting blocked on one poc)

Regards,
Sandeep

On Sun, Jan 14, 2018 at 7:43 PM, sandeep krishnamurthy <
sandeep.krishna98@gmail.com> wrote:

> +1 (binding) for suggestion around framing CODEOWNERS functionality as the
> watchlist.
> I also feel that we should enable and find/request more than 1 person per
> module to help the project.
>
> But, still, if it is something like +1 or watch button for modules rather
> than a new PR to follow a topic, it would have been great. Something for
> future :-)
>
> Regards,
> Sandeep
>
> On Sun, Jan 14, 2018 at 4:18 PM, Steffen Rochel <steffenrochel@gmail.com>
> wrote:
>
>> 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.
>> > > >> > > > > > > > >
>> > > >> > > > > > > >
>> > > >> > > > > > >
>> > > >> > > > > >
>> > > >> > > > >
>> > > >> > > >
>> > > >> > >
>> > > >> >
>> > > >>
>> > > >
>> > > >
>> > >
>> >
>>
>
>
>
> --
> Sandeep Krishnamurthy
>



-- 
Sandeep Krishnamurthy

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