+1 On Mon, Jan 15, 2018 at 4:40 PM, Steffen Rochel <[email protected]> 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 < > [email protected]> > 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 <[email protected]> > > 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 < > [email protected] > > > > > > 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 < > > > > [email protected]> 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 < > > > > > [email protected]> 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 < > > > > [email protected] > > > > > > > > > > > > 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 < > > > [email protected] > > > > > > > > > > >> 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 < > > > > [email protected]> > > > > > >> > 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 < > > > > > [email protected]> > > > > > >> > > 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 < > > > > > >> > > > [email protected]> 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" < > > > > > >> > > [email protected] > > > > > >> > > >> >: > > > > > >> > > >> > > > > > >> > > >> > 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 < > > > > > >> > > >> > [email protected]> 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" < > > > > > >> > > >> [email protected] > > > > > >> > > >> > >: > > > > > >> > > >> > > > > > > > >> > > >> > > > -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 < > > > > > >> [email protected] > > > > > >> > > > > > > > >> > > >> > wrote: > > > > > >> > > >> > > > > > > > > >> > > >> > > > > +1 (binding) > > > > > >> > > >> > > > > > > > > > >> > > >> > > > > On 2018-01-12 10:10, kellen sunderland < > > > > > >> > > >> [email protected]> > > > > > >> > > >> > > > > wrote: > > > > > >> > > >> > > > > > +1 (non-binding) > > > > > >> > > >> > > > > > > > > > > >> > > >> > > > > > On Jan 12, 2018 6:32 PM, "Steffen Rochel" < > > > > > >> > > >> [email protected] > > > > > >> > > >> > > > > > > > >> > > >> > > > > wrote: > > > > > >> > > >> > > > > > > > > > > >> > > >> > > > > > > I propose to adopt the proposal. > > > > > >> > > >> > > > > > > +1 (non-binding) > > > > > >> > > >> > > > > > > > > > > > >> > > >> > > > > > > Steffen > > > > > >> > > >> > > > > > > > > > > > >> > > >> > > > > > > On Wed, Jan 10, 2018 at 8:39 PM Mu Li < > > > > > >> [email protected] > > > > > >> > > > > > > > >> > > >> > 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 < > > > > > >> > > >> > > > > [email protected]> > > > > > >> > > >> > > > > > > > wrote: > > > > > >> > > >> > > > > > > > > > > > > >> > > >> > > > > > > > > > > > > > >> > > >> > > > > > > > > > > > > > >> > > >> > > > > > > > > Am 9. Januar 2018 18:25:50 MEZ schrieb Mu > > Li > > > < > > > > > >> > > >> > > [email protected] > > > > > >> > > >> > > > >: > > > > > >> > > >> > > > > > > > > >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 > > > > > > > > > > > > > > >
