mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lupesko, Hagay" <lupe...@gmail.com>
Subject Re: Reverting pull request
Date Fri, 15 Jun 2018 23:26:12 GMT
Hey all,

Although I am not a committer, and also have not contributed to MXNet as much as I would have
wanted, wanted to chime in.
Based on my experience doing SW dev for quite some time, I think that holding a high bar for
the code that gets merged is a very positive thing - including making sure all code that is
merged is reviewed.
Yes - it slows you down a bit, and yes, some people may know certain areas of the code best,
to the point they feel reviews are not helpful and slow them down But, getting eye balls on
your code change is always a good thing that helps everyone improve: the author of the change,
the reviewer and the code base itself. Not to mention it helps find issues and bugs that one
person may miss.

Why merge code without a review, and if that happens accidentally or what not, why not revert,
improve as needed (bugs, unit tests, etc) and then submit a new PR? 
Assuming this is not an urgent fix - I think that a revert is a better approach then leaving
bugs in and gradually merging fixes.

On another note, this discussion became very heated and emotional, and does not make the MXNet
community look good.
I suggest the folks involved to do in person video call and sort things out - everyone's in
the same boat to make MXNet awesome, right?

Happy Friday everyone,
Hagay

´╗┐On 6/15/18, 16:07, "Anirudh" <anirudh2290@gmail.com> wrote:

    Hi,
    
    We can have a separate discussion on whether this was a friendly way to
    bring this up or not,
    but I don't see why we shouldn't roll back, share design on dev, fix the
    bug and add performance benchmarking results and call for reviews on
    a new PR. This seems to be a big change which was introduced and I have
    seen Eric himself rolling back PRs
    for big changes which happen to have a regression or insufficient tests. To
    quote Eric from here
    https://github.com/apache/incubator-mxnet/pull/8010 : "Roll
    back is always better than fix forward."
    I agree with Marco that we should abide by the rules which we set for other
    contributors.
    If the argument is that users depend on the change since it has been
    introduced a few days ago, then the assumption with depending on the master
    is always that it is bleeding edge and things can break.
    
    Anirudh
    
    On Fri, Jun 15, 2018 at 3:10 PM, Mu Li <muli.cmu@gmail.com> wrote:
    
    > Hi Marco,
    >
    > You really want to bring it into Amazon internal planning meeting. I have
    > been requesting to focus on fixing bugs for several weeks, instead of
    > adding new features. But I didn't get a concrete time when it will happen.
    >
    > Best
    > Mu
    >
    > On Fri, Jun 15, 2018 at 3:03 PM, Marco de Abreu <
    > marco.g.abreu@googlemail.com.invalid> wrote:
    >
    > > CI doesn't fail for no reason but because some people prefer to push new
    > > features than to get our codebase actually stable. We currently have 51
    > [1]
    > > flaky tests and I have only seen a few people (thanks Sheng, Alex and
    > > Pedro) work on the problem. So instead of complaining, take part and help
    > > improving the situation.
    > >
    > > The CCache/EFS failure lasted for 12 hours and was an error - these
    > things
    > > happen when you run a service. This is not a blame-game.
    > >
    > > -Marco
    > >
    > > [1]:
    > > https://github.com/apache/incubator-mxnet/issues?q=is%
    > > 3Aopen+is%3Aissue+label%3AFlaky
    > >
    > > On Fri, Jun 15, 2018 at 2:55 PM Eric Xie <jxie@apache.org> wrote:
    > >
    > > > Hi Marco de Abreu,
    > > >
    > > > CI has been totally broken recently. It randomly fails for no good
    > reason
    > > > more often than it passes. For example the ccache/efs failure has been
    > > > really annoying.
    > > >
    > > > Looks like there has been many changes to Jenkins and Docker lately. Do
    > > > you think we should revert all of the recent changes to get a stable CI
    > > or
    > > > do you think someone should find a fix for the bugs?
    > > >
    > > > Thanks,
    > > > Eric
    > > >
    > > > On 2018/06/15 21:21:50, Marco de Abreu <marco.g.abreu@googlemail.com.
    > > INVALID>
    > > > wrote:
    > > > > We revert a PR because it should not have been merged in the first
    > > place.
    > > > > So far, I have been ignoring the fact that our committers are
    > > constantly
    > > > > breaking our own rules (which we expect contributors to follow). But
    > > > since
    > > > > this caused an impact twice (1.2 breaking change about model
    > > > import/export
    > > > > as well as this regression), I'm now being more strict and enforcing
    > > > them.
    > > > >
    > > > > I could've also made a script that prevents any PR from being
    > > > self-merged,
    > > > > but I thought our committers are responsible enough to follow our
own
    > > > rules
    > > > > without systems actually enforcing them. I won't waste my time
    > working
    > > on
    > > > > that script, but from now on I will revert every single PR (except
    > > > > emergency cases) that has been self-merged without approval.
    > > > >
    > > > > -Marco
    > > > >
    > > > > On Fri, Jun 15, 2018 at 2:15 PM Mu Li <muli.cmu@gmail.com> wrote:
    > > > >
    > > > > > Why reverting instead of fixing the bugs? Static memory aims
to
    > > reduce
    > > > > > memory allocation, it's a key feature to bridge the perf gap
    > between
    > > > gluon
    > > > > > and symbol.
    > > > > >
    > > > > > On Fri, Jun 15, 2018 at 2:06 PM, Marco de Abreu <
    > > > > > marco.g.abreu@googlemail.com.invalid> wrote:
    > > > > >
    > > > > > > Hello,
    > > > > > >
    > > > > > > I'm reverting https://github.com/apache/
    > incubator-mxnet/pull/10817
    > > > as of
    > > > > > > https://github.com/apache/incubator-mxnet/pull/11311 due
to
    > > > regressions
    > > > > > > described in https://github.com/apache/
    > > incubator-mxnet/issues/11171
    > > > and
    > > > > > > https://github.com/apache/incubator-mxnet/pull/10817.
    > > > > > >
    > > > > > > The pull request has been self-merged without proper review
and
    > > > > > introduced
    > > > > > > regressions. Committers should act as role models in this
project
    > > and
    > > > > > > adhere to software engineer best practices.
    > > > > > >
    > > > > > > Best regards,
    > > > > > > Marco
    > > > > > >
    > > > > >
    > > > >
    > > >
    > >
    >
    



Mime
View raw message