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 > > >
