mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Olivier <cjolivie...@gmail.com>
Subject Re: Module maintainers proposal
Date Mon, 15 Jan 2018 20:50:18 GMT
I'm not sure I understand the "orphaned package" thing.  You mean that no
one is reviewing them?
If a corporation wishes to assign a particular portion of the code to an
employee to review regularly, then that can take care of any portions
becoming "orphaned", but it can't mean "this person we assigned is now the
last word.

If someone takes an interest in reviewing a particular part of the code,
then they'd tend to add themself to the "watch list" (this CODEOWNERS
file), but I don't believe that this file should dictate how important one
committer's reviews are  compared to another.  You don't entice people to
review by telling them that their opinion is only worth half of person
X's.  Why would they even bother?  Committers are made committers because
they are trusted to behave competently and not merge stuff they aren't
comfortable with.

People work hard to become committers, but then saying that "ok you're a
committer but really only these 5 people get to merge code unless you jump
through all of these hoops" isn't fair, IMHO, and won't help to build the
community.

In addition, so far the mentors seem to have discouraged this sort of
"ownership" role.





On Sun, Jan 14, 2018 at 8:39 PM, Steffen Rochel <steffenrochel@gmail.com>
wrote:

> 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