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 Tue, 16 Jan 2018 00:43:04 GMT
+1

On Mon, Jan 15, 2018 at 4:40 PM, Steffen Rochel <steffenrochel@gmail.com>
wrote:

> 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