Yay!
On 10/12/17, 11:27, "Gautam" <[email protected]> wrote:
Hi All,
The master branch is protected now. Please review your PR for merge.
Thanks to Infra team for following up on this.
Regards,
Gautam
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/artic
>> les/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/artic
>> les/about-required-
>> > > > status-checks>
>> > > > > > > pass
>> > > > > > > > > - Can't have changes merged into it until
>> required reviews
>> > > are
>> > > > > > > > approved
>> > > > > > > > > <https://help.github.com/artic
>> les/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/articl
>> es/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
>
>
>
>
>
--
Best Regards,
Gautam Kumar
