Hi Hen - we started to document the process - https://cwiki.apache.org/confluence/display/MXNET/Development+Process Contributions are welcome! Steffen
On Sun, Oct 8, 2017 at 3:08 AM, Henri Yandell <[email protected]> wrote: > A late followup on this. > > Is the "How a committer develops on MXNet" documented anywhere? > Staging/master etc? The more complex the development process, the harder it > is for a newcomer to get involved on the project. I couldn't find it on the > website/github. > > (I'd also note that the 'how to modify the website/documentation' also > needs to be documented) > > I'd suggest that the how-to-dev doc also explain why it's bad for master to > not build. One could argue that, outside of when a release is being made, > master is not important to our users. Yes it should build, but an accident > should only affect those who have put themselves on the bleeding edge. > > Hen > > On Mon, Oct 2, 2017 at 1:19 PM, Gautam <[email protected]> wrote: > > > Thanks All. > > > > I've created the JIRA to mark the protected branch for master > > https://issues.apache.org/jira/browse/INCUBATOR-205. > > We also need to add all the committers to be code owner as discussed in > the > > slack, I've opened a PR for it > > https://github.com/apache/incubator-mxnet/pull/8128. > > > > Good point Joern, I'll follow up on that. > > > > Regards, > > Gautam > > > > On Fri, Sep 29, 2017 at 2:20 AM, Joern Kottmann <[email protected]> > > wrote: > > > > > It also makes sense to block too old PRs from merging, because the > > > test results are outdated and the build might fail after it gets > > > merged. > > > > > > Jörn > > > > > > On Thu, Sep 28, 2017 at 9:14 PM, Zha, Sheng <[email protected]> > wrote: > > > > +1 on protected branch. > > > > > > > > Best regards, > > > > -sz > > > > > > > > On 9/28/17, 11:48 AM, "Kumar, Gautam" <[email protected]> wrote: > > > > > > > > Hi Guys, > > > > > > > > Let’s focus on specific issue here. > > > > > > > > Marking the master branch protected which involves “Only merge if > > > checks has passed, and yes it will run the complete build”. > > > > > > > > We can’t afford to degrade the quality and keep debugging the > build > > > failure forever. If it’s slow down the development at the cost of > > quality I > > > will vote for the quality. > > > > We can work on improving the infrastructure to improve the > overall > > > speed. If you have any specific concerns on availability of Jenkins > > please > > > point out. > > > > > > > > -Gautam > > > > > > > > > > > > On 9/28/17, 11:38 AM, "Chris Olivier" <[email protected]> > > wrote: > > > > > > > > -1000 on that. :) > > > > > > > > On Thu, Sep 28, 2017 at 11:33 AM Naveen Swamy < > > > [email protected]> wrote: > > > > > > > > > PR->Sanity test/Linux build/test->reviewer/committer > approves > > > the > > > > > change->Comment "Build Now" (Or trigger on at least one > > > approval from a > > > > > committer other than author)->*Full build-*>*passes > > > build*->Enable Merge > > > > > > > > > > Let us take this particular topic to a separate thread or > > > discuss offline > > > > > if further clarification is needed. > > > > > > > > > > On Thu, Sep 28, 2017 at 11:24 AM, Chris Olivier < > > > [email protected]> > > > > > wrote: > > > > > > > > > > > I understand the proposal. How to trigger a build in > that > > > case? > > > > > > > > > > > > > > > > > > On Thu, Sep 28, 2017 at 10:54 AM Madan Jampani < > > > [email protected]> > > > > > > wrote: > > > > > > > > > > > > > Chris, > > > > > > > I don't think Naveen is suggesting that a merge happen > > > without full > > > > > > > verification i.e. all tests across all platforms pass. > > > > > > > If a PR has some back and forth and results in multiple > > > revisions > > > > > (which > > > > > > is > > > > > > > arguably more common than a random unit test failing), > we > > > simply delay > > > > > > full > > > > > > > verification until the owner/reviewer have settled on a > > > mutually > > > > > > acceptable > > > > > > > state. > > > > > > > > > > > > > > On Thu, Sep 28, 2017 at 10:25 AM, Chris Olivier < > > > [email protected] > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > -1 for running only partial tests. Most failing unit > > > tests that get > > > > > > > > through fail only for certain > platforms/configurations. > > > I personally > > > > > > > > prefer to be assured the build and test is good > before > > > merge. Most > > > > > PR > > > > > > > > merges aren't in a huge hurry. > > > > > > > > > > > > > > > > On Thu, Sep 28, 2017 at 9:54 AM, Naveen Swamy < > > > [email protected]> > > > > > > > wrote: > > > > > > > > > > > > > > > > > +1 to make it protected. Here is what I am thinking > > > for PR builds > > > > > > > > > on a PR run Sanity Tests + build/test one > > > platform->committer > > > > > reviews > > > > > > > the > > > > > > > > > code and issues "Build Now", a full build is > > > run->Github checks > > > > > that > > > > > > > the > > > > > > > > > full build checks succeed before it can be merged. > > > > > > > > > > > > > > > > > > I agree with Madan that PR should be approved by > one > > > another > > > > > > committer. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Sep 28, 2017 at 9:37 AM, Madan Jampani < > > > > > > > [email protected]> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > > > > > > At a minimum I'd like to see the following two > > > happen: > > > > > > > > > > - Option to merge is disabled until all required > > > checks pass. > > > > > > > > > > - Code is reviewed and given +1 by at least one > > > other committer > > > > > (no > > > > > > > > self > > > > > > > > > > review). > > > > > > > > > > > > > > > > > > > > On Wed, Sep 27, 2017 at 11:15 PM, Gautam < > > > [email protected]> > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi Chris, > > > > > > > > > > > > > > > > > > > > > > Here <https://help.github.com/ > > > articles/about-protected- > > > > > > branches/ > > > > > > > > > > > > > > > > is > > > > > > > > > > > user > > > > > > > > > > > document on semantics of protected branch. > > > > > > > > > > > In short when a branch is protected following > > > applies to that > > > > > > > branch. > > > > > > > > > > > > > > > > > > > > > > - Can't be force pushed > > > > > > > > > > > - Can't be deleted > > > > > > > > > > > - Can't have changes merged into it until > > > required status > > > > > > checks > > > > > > > > > > > <https://help.github.com/ > > > articles/about-required- > > > > > > status-checks> > > > > > > > > > pass > > > > > > > > > > > - Can't have changes merged into it until > > > required reviews > > > > > are > > > > > > > > > > approved > > > > > > > > > > > <https://help.github.com/ > > > articles/approving-a-pull- > > > > > > > > > > > request-with-required-reviews> > > > > > > > > > > > - Can't be edited or have files uploaded to > it > > > from the web > > > > > > > > > > > - Can't have changes merged into it until > > > changes to files > > > > > > that > > > > > > > > > > > have a designated > > > > > > > > > > > code owner <https://help.github.com/ > > > > > > articles/about-codeowners/> > > > > > > > > > have > > > > > > > > > > > been approved by that owner > > > > > > > > > > > > > > > > > > > > > > I am sure many of us might not want to have > all > > > these but we > > > > > can > > > > > > > > > debate > > > > > > > > > > on > > > > > > > > > > > it. My main motive was to "*Can't have changes > > > merged into it > > > > > > until > > > > > > > > > > > required status checks pass*" > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -Gautam > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Sep 27, 2017 at 11:09 PM, Chris > Olivier < > > > > > > > > [email protected] > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > What does that mean? "Protected"? Protected > > from > > > what? > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Sep 27, 2017 at 11:08 PM Gautam < > > > > > [email protected]> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hi Chris, > > > > > > > > > > > > > > > > > > > > > > > > > > I mean make "master branch protected" of > > > MXNet. > > > > > > > > > > > > > > > > > > > > > > > > > > -Gautam > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Sep 27, 2017 at 11:04 PM, Chris > > > Olivier < > > > > > > > > > > [email protected] > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > What does this mean? "Mx-net branch > > > protected"? > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Sep 27, 2017 at 9:59 PM Tsuyoshi > > > OZAWA < > > > > > > > > > > > > [email protected] > > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > +1, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > While I'm checking the recent build > > > failures, and I > > > > > think > > > > > > > the > > > > > > > > > > > > decision > > > > > > > > > > > > > > > of making the mx-net branch protected > is > > > necessary for > > > > > > > stable > > > > > > > > > > > > > > > building. > > > > > > > > > > > > > > > Thanks Kumar for resuming important > > > discussion. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best regards > > > > > > > > > > > > > > > - Tsuyoshi > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Sep 28, 2017 at 12:56 PM, > Kumar, > > > Gautam < > > > > > > > > > > [email protected]> > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > Reviving the discussion. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > At this point of time we have couple > of > > > stable builds > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://builds.apache.org/view/Incubator%20Projects/job/ > > > > > > > > > > > > > > incubator-mxnet/job/master/448/ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://builds.apache.org/view/Incubator%20Projects/job/ > > > > > > > > > > > > > > incubator-mxnet/job/master/449/ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Should we have a quick discussion or > > > polling on > > > > > making > > > > > > > the > > > > > > > > > > mx-net > > > > > > > > > > > > > > branch > > > > > > > > > > > > > > > protected? If you still think we > > shouldn’t > > > make it > > > > > > > protected > > > > > > > > > > please > > > > > > > > > > > > > > provide > > > > > > > > > > > > > > > a reason to support your claim. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Few of us have concern over Jenkin’s > > > stability. If I > > > > > > look > > > > > > > > two > > > > > > > > > > > weeks > > > > > > > > > > > > > > > back, after upgrading Linux slave to > > g2.8x > > > and new > > > > > > windows > > > > > > > > AMI, > > > > > > > > > > we > > > > > > > > > > > > have > > > > > > > > > > > > > > not > > > > > > > > > > > > > > > seen any case where instance died due > to > > > high memory > > > > > > usage > > > > > > > or > > > > > > > > > any > > > > > > > > > > > > > process > > > > > > > > > > > > > > > got killed due to high cpu usage or any > > > other issue > > > > > with > > > > > > > > > windows > > > > > > > > > > > > > slaves. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Going forward we are also planning > that > > > if we add any > > > > > > new > > > > > > > > > slave > > > > > > > > > > > we > > > > > > > > > > > > > will > > > > > > > > > > > > > > > not enable the main load immediately, > but > > > rather will > > > > > do > > > > > > > > ‘test > > > > > > > > > > > build’ > > > > > > > > > > > > > to > > > > > > > > > > > > > > > make sure that new slaves are not > causing > > > any > > > > > > > infrastructure > > > > > > > > > > issue > > > > > > > > > > > > and > > > > > > > > > > > > > > > capable to perform as good as existing > > > slaves. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -Gautam > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 8/31/17, 5:27 PM, "Lupesko, > Hagay" < > > > > > > [email protected] > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @madan looking into some > failures – > > > you’re right… > > > > > > > > there’s > > > > > > > > > > > > > multiple > > > > > > > > > > > > > > > issues going on, some of them > > > intermittent, and we want > > > > > > to > > > > > > > be > > > > > > > > > > able > > > > > > > > > > > to > > > > > > > > > > > > > > merge > > > > > > > > > > > > > > > fixes in. > > > > > > > > > > > > > > > > Agreed that we can wait with > > setting > > > up protected > > > > > > > mode > > > > > > > > > > until > > > > > > > > > > > > > build > > > > > > > > > > > > > > > stabilizes. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 8/31/17, 11:41, "Madan > Jampani" > > < > > > > > > > > > > [email protected]> > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @hagay: we agree on the end > > > state. I'm not > > > > > too > > > > > > > > > > particular > > > > > > > > > > > > > about > > > > > > > > > > > > > > > how we get > > > > > > > > > > > > > > > > there. If you think enabling > it > > > now and fixes > > > > > > > > > > regression > > > > > > > > > > > > > later > > > > > > > > > > > > > > > is doable, > > > > > > > > > > > > > > > > I'm fine with. I see a bit > of a > > > chicken and > > > > > egg > > > > > > > > > > problem. > > > > > > > > > > > We > > > > > > > > > > > > > > need > > > > > > > > > > > > > > > to get > > > > > > > > > > > > > > > > some fixes in even when the > > > status checks are > > > > > > > > > failing. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Aug 31, 2017 at 11:25 > > > AM, Lupesko, > > > > > > Hagay > > > > > > > < > > > > > > > > > > > > > > > [email protected]> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @madan – re: getting to a > > > stable CI first: > > > > > > > > > > > > > > > > > I’m concerned that by not > > > enabling > > > > > protected > > > > > > > > branch > > > > > > > > > > > mode > > > > > > > > > > > > > > ASAP, > > > > > > > > > > > > > > > we’re just > > > > > > > > > > > > > > > > > taking in more regressions, > > > which makes a > > > > > > > stable > > > > > > > > > > build > > > > > > > > > > > a > > > > > > > > > > > > > > > moving target for > > > > > > > > > > > > > > > > > us… > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 8/31/17, 10:49, "Zha, > > > Sheng" < > > > > > > > > > [email protected] > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Just one thing: please > > > don’t disable > > > > > more > > > > > > > > tests > > > > > > > > > > or > > > > > > > > > > > > just > > > > > > > > > > > > > > > raise the > > > > > > > > > > > > > > > > > tolerance thresholds. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > > > > > > > > -sz > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 8/31/17, 10:45 AM, > > > "Madan Jampani" < > > > > > > > > > > > > > > > [email protected]> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > > > Before we can turn > > > protected mode I > > > > > > > feel > > > > > > > > we > > > > > > > > > > > > should > > > > > > > > > > > > > > > first get to a > > > > > > > > > > > > > > > > > stable CI > > > > > > > > > > > > > > > > > pipeline. > > > > > > > > > > > > > > > > > Sandeep is chasing > > > down known > > > > > > breaking > > > > > > > > > > issues. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Aug 31, > 2017 > > > at 10:27 AM, > > > > > > Hagay > > > > > > > > > > > Lupesko < > > > > > > > > > > > > > > > [email protected]> > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Build stability > is > > a > > > major issue, > > > > > > > > builds > > > > > > > > > > have > > > > > > > > > > > > > been > > > > > > > > > > > > > > > failing left > > > > > > > > > > > > > > > > > and right > > > > > > > > > > > > > > > > > > over the last > week. > > > Some of it is > > > > > > due > > > > > > > > to > > > > > > > > > > > > Jenkins > > > > > > > > > > > > > > > slave issues, > > > > > > > > > > > > > > > > > but some are > > > > > > > > > > > > > > > > > > real regressions. > > > > > > > > > > > > > > > > > > We need to be > more > > > strict in the > > > > > > code > > > > > > > > > we're > > > > > > > > > > > > > > > committing. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I propose we > > > configure our master > > > > > > to > > > > > > > > be a > > > > > > > > > > > > > protected > > > > > > > > > > > > > > > branch ( > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://help.github.com/ > > > articles/about-protected- > > > > > > branches/ > > > > > > > ). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thoughts? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 2017-08-28 > > 22:41, > > > sandeep > > > > > > > > > krishnamurthy > > > > > > > > > > < > > > > > > > > > > > > > > > [email protected]> > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hello > Committers > > > and > > > > > > Contributors,> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Due to unstable > > > build > > > > > pipelines, > > > > > > > from > > > > > > > > > > past > > > > > > > > > > > 1 > > > > > > > > > > > > > > week, > > > > > > > > > > > > > > > PRs are > > > > > > > > > > > > > > > > > being merged> > > > > > > > > > > > > > > > > > > > after CR > ignoring > > > PR build > > > > > > status. > > > > > > > > > Build > > > > > > > > > > > > > pipeline > > > > > > > > > > > > > > > is much more > > > > > > > > > > > > > > > > > stable > > > > > > > > > > > > > > > > > > than> > > > > > > > > > > > > > > > > > > > last week and > > most > > > of the build > > > > > > > > > failures > > > > > > > > > > > you > > > > > > > > > > > > > see > > > > > > > > > > > > > > > from now on, > > > > > > > > > > > > > > > > > are likely > > > > > > > > > > > > > > > > > > to> > > > > > > > > > > > > > > > > > > > be a valid > > failure > > > and hence, > > > > > it > > > > > > is > > > > > > > > > > > > recommended > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > wait for PR > > > > > > > > > > > > > > > > > builds, > > > > > > > > > > > > > > > > > > see> > > > > > > > > > > > > > > > > > > > the root cause > of > > > any build > > > > > > > failures > > > > > > > > > > before > > > > > > > > > > > > > > > proceeding with > > > > > > > > > > > > > > > > > merges.> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > At this point > of > > > time, there > > > > > are > > > > > > 2 > > > > > > > > > > > > intermittent > > > > > > > > > > > > > > > issue yet to > > > > > > > > > > > > > > > > > be fixed -> > > > > > > > > > > > > > > > > > > > * Network error > > > leading to > > > > > GitHub > > > > > > > > > > requests > > > > > > > > > > > > > > > throwing 404> > > > > > > > > > > > > > > > > > > > * A conflict in > > > artifacts > > > > > > generated > > > > > > > > > > between > > > > > > > > > > > > > > > branches/PR - > > > > > > > > > > > > > > > > > Cause unknown > > > > > > > > > > > > > > > > > > yet.> > > > > > > > > > > > > > > > > > > > These issues > will > > > be fixed > > > > > soon.> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > > > > > > > > > Sandeep > > > Krishnamurthy> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > > > > - Tsuyoshi > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > > Best Regards, > > > > > > > > > > > > > Gautam Kumar > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > Best Regards, > > > > > > > > > > > Gautam Kumar > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > Best Regards, > > Gautam Kumar > > >
