Mailing-List: contact dev-help@mxnet.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@mxnet.incubator.apache.org
MIME-Version: 1.0
In-Reply-To: <CA+V=Wqha0iP9Dviiz8DnQSSDZMq7=cOULeUfhMJRVgPyjFq94A@mail.gmail.com>
References: <CAGegiFaz5jf_wXnfNqTHx57TzyjVxmvoXdbnZbw=_H-iaiJ=mw@mail.gmail.com>
 <CACjccmOX2JR0vTmXo+zn3tGqaEj_T-f6ba_QHUoJkMxBjABCyQ@mail.gmail.com>
 <39AE9373-C107-4D7B-8F62-5D3AC57F95E2@amazon.com> <087FD172-52DE-42F6-B889-BD767E05A795@gmail.com>
 <CACjccmMqhc4xDnWp6strvKLk8=KkdOdVd8FKELeM=wNAdfURUg@mail.gmail.com>
 <5A99919F-2BCB-45EE-8259-01BF1C980CF1@gmail.com> <ED37EEC6-1A75-4B98-8695-71020770A13F@amazon.com>
 <CAAD07OJDW=BTLCP0NFeLMsf9f7YM2ObvCtgFw9Mes8Y5P+84UA@mail.gmail.com>
 <CABgAAfcfE__g+Gaj6QPeuuTFspqx2KgV3=EKX1rMZq0JVRCa8A@mail.gmail.com>
 <CAFgdJKAOm1G0_wAjD+tVqJRfsCwA8-WhvA0O+BS8YqtA_2J5iw@mail.gmail.com>
 <CABgAAfdn1imKiy9Lm6rjk+hFFhV-GdTLHBDHq8TSLwQx7wRfwg@mail.gmail.com>
 <CAFgdJKAZ_4jdsqgjUBnENHFukTvdCUVAo8KBMO9WiM+bzcNRew@mail.gmail.com>
 <CABgAAfc78Yw44D-03gPO2DqZ7Jg5z7Gj7x3eJ-fwc5HSpmt6xw@mail.gmail.com> <CA+V=Wqha0iP9Dviiz8DnQSSDZMq7=cOULeUfhMJRVgPyjFq94A@mail.gmail.com>
From: Pedro Larroy <pedro.larroy.lists@gmail.com>
Date: Thu, 28 Sep 2017 15:57:15 +0200
Message-ID: <CAC_CU1h9obzFoC5T-+bNOyQVzjx=Ke5N0-=kO0scLX4nR=28NA@mail.gmail.com>
Subject: Re: Apache MXNet build failures are mostly valid - verify before merge
To: dev@mxnet.incubator.apache.org
Content-Type: multipart/alternative; boundary="94eb2c1cda1e362def055a404932"
archived-at: Thu, 28 Sep 2017 13:57:27 -0000

--94eb2c1cda1e362def055a404932
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Given the cost of running all the test for all the build flavors and
architectures I would propose the following:


   - Have a staging branch were PRs are merged by committers which runs all
   the integration tests with the appropriate frequency (say nightly).
   - Have automated fast forwards from the staging branch done
   automatically by Jenkins when the latest head passes all the tests for a=
ll
   platforms.


With this we would always have a stable master branch which is well tested,
while being able to adjust the tradeoff between correctness and quick
feedback for PRs.

Another improvement would be to split the feedback in stages, one would be
multi-platform / multi-flavor build which should be around 20 minutes, and
then two or more stages of quick tests and extensive tests. And as I
explained, we wouldn't need to run extensive tests on every PR, just
nightly on staging.

What do you think?

Pedro.

On Thu, Sep 28, 2017 at 2:02 PM, Joern Kottmann <kottmann@gmail.com> wrote:

> At Apache OpenNLP we just established among committers that you check
> that the status indicator is green before you merge,
> and if it wasn't the case then we would ask the committer to take
> responsibility and repair things. Works very well our build is never
> broken.
>
> We also strongly prefer if each PR gets reviewed by another committer.
>
> Overall this works quite well. We don't use any of the protections
> against merging, it is important that you can trust each of the
> committers not to break things, if there are problems it is better to
> resolve them with talking to each other, rather than enforcing green
> status checks.
>
> J=C3=B6rn
>
> On Thu, Sep 28, 2017 at 8:21 AM, Chris Olivier <cjolivier01@gmail.com>
> wrote:
> > +1 on that
> >
> > On Wed, Sep 27, 2017 at 11:15 PM Gautam <gautamnitc@gmail.com> 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> pas=
s
> >>    - 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/> hav=
e
> >>    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 <cjolivier01@gmail.com=
>
> >> wrote:
> >>
> >> > What does that mean? "Protected"? Protected from what?
> >> >
> >> > On Wed, Sep 27, 2017 at 11:08 PM Gautam <gautamnitc@gmail.com> wrote=
:
> >> >
> >> > > Hi Chris,
> >> > >
> >> > >    I mean make "master branch protected" of  MXNet.
> >> > >
> >> > > -Gautam
> >> > >
> >> > > On Wed, Sep 27, 2017 at 11:04 PM, Chris Olivier <
> cjolivier01@gmail.com
> >> >
> >> > > wrote:
> >> > >
> >> > > > What does this mean? "Mx-net branch protected"?
> >> > > >
> >> > > > On Wed, Sep 27, 2017 at 9:59 PM Tsuyoshi OZAWA <
> >> > ozawa.tsuyoshi@gmail.com
> >> > > >
> >> > > > 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 <
> gauta@amazon.com>
> >> > > > 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=E2=80=99t make it pro=
tected
> please
> >> > > > provide
> >> > > > > a reason to support your claim.
> >> > > > > >
> >> > > > > > Few of us have concern over Jenkin=E2=80=99s 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 windo=
ws
> >> > > 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 =E2=
=80=98test
> >> build=E2=80=99
> >> > > 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" <lupesko@gmail.com>
> wrote:
> >> > > > > >
> >> > > > > >     @madan looking into some failures =E2=80=93 you=E2=80=99=
re right=E2=80=A6 there=E2=80=99s
> >> > > 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" <
> madan.jampani@gmail.com>
> >> > > > 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 failin=
g.
> >> > > > > >
> >> > > > > >         On Thu, Aug 31, 2017 at 11:25 AM, Lupesko, Hagay <
> >> > > > > lupesko@gmail.com> wrote:
> >> > > > > >
> >> > > > > >         > @madan =E2=80=93 re: getting to a stable CI first:
> >> > > > > >         > I=E2=80=99m concerned that by not enabling protect=
ed branch
> >> mode
> >> > > > ASAP,
> >> > > > > we=E2=80=99re just
> >> > > > > >         > taking in more regressions, which makes a stable
> build
> >> a
> >> > > > > moving target for
> >> > > > > >         > us=E2=80=A6
> >> > > > > >         >
> >> > > > > >         > On 8/31/17, 10:49, "Zha, Sheng" <
> zhasheng@amazon.com>
> >> > > wrote:
> >> > > > > >         >
> >> > > > > >         >     Just one thing: please don=E2=80=99t disable m=
ore tests
> or
> >> > just
> >> > > > > raise the
> >> > > > > >         > tolerance thresholds.
> >> > > > > >         >
> >> > > > > >         >     Best regards,
> >> > > > > >         >     -sz
> >> > > > > >         >
> >> > > > > >         >     On 8/31/17, 10:45 AM, "Madan Jampani" <
> >> > > > > madan.jampani@gmail.com> wrote:
> >> > > > > >         >
> >> > > > > >         >         +1
> >> > > > > >         >         Before we can turn protected mode I feel w=
e
> >> > 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 <
> >> > > > > lupesko@gmail.com>
> >> > > > > >         > 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 <
> >> > > > > s...@gmail.com>
> >> > > > > >         > wrote:
> >> > > > > >         >         > > Hello Committers and Contributors,>
> >> > > > > >         >         > >
> >> > > > > >         >         > > Due to unstable build pipelines, from
> past
> >> 1
> >> > > > week,
> >> > > > > PRs are
> >> > > > > >         > being merged>
> >> > > > > >         >         > > after CR ignoring PR build status. Bui=
ld
> >> > > pipeline
> >> > > > > is much more
> >> > > > > >         > stable
> >> > > > > >         >         > than>
> >> > > > > >         >         > > last week and most of the build failur=
es
> >> 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
> >>
>

--94eb2c1cda1e362def055a404932--