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 04:39:16 GMT
Sandeep -
1. Yes, but not only. Using maintainers the community will also know who is
expert or point of contact for a specific package within the MXNet repo.
2. I suggested: 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.
Of course, open to suggestion and I assume we all know when to apply common
sense for small changes.
As we are gaining more experience with a larger community we can decide if
it make sense to use required reviews by the CODEOWNERS (could be one or
more per package), but I think this would be to restrictive at this time.

I liked the description from github
<https://opensource.guide/leadership-and-governance/> about the role of a
maintainer: "... Regardless of what they do day-to-day, a maintainer is
probably someone who feels responsibility over the direction of the project
and is committed to improving it. " I feel this does apply to the various
packages/directories in MXNet to grow the community and project.

Chris - can you please elaborate your concerns and suggest alternative? How
can we ensure certain packages will not become orphans? I do see a
maintainer as somebody with detailed knowledge who cares about an area and
certainly not as dictator or king.

Steffen

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

> 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