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 Tue, 16 Jan 2018 00:40:48 GMT
Thanks for all the feedback!
Proposed a simplified version in
https://github.com/apache/incubator-mxnet/pull/9448 :

# Owners of Apache MXNet
# 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
#
# Anybody can add themselves or a team as additional contributors
# to get notified about changes in a specific package.
# See https://help.github.com/articles/about-teams how to setup teams.
# Global owners
...

Hope we can adopt this approach.

Regards, Steffen


On Mon, Jan 15, 2018 at 1:26 PM Marco de Abreu <marco.g.abreu@googlemail.com>
wrote:

> Very good points, Chris! +1
>
> If the community does not want to support a specific part of MXNet but
> there's a business interest, the company can assign somebody for this task
> and if this person is doing good work, they might be added as a committer
> in the long-term, closing the loop. If there's no business- neither
> user-interest in that part and nobody else in the community wants to take
> care of it, it might as well get removed.
>
> -Marco
>
> On Mon, Jan 15, 2018 at 9:50 PM, Chris Olivier <cjolivier01@gmail.com>
> wrote:
>
> > 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