mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marco de Abreu <marco.g.ab...@googlemail.com>
Subject Re: Module maintainers proposal
Date Mon, 15 Jan 2018 21:25:32 GMT
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