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 >
