Re: Reverting pull request

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

Re: Reverting pull request

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

Re: Reverting pull request

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 <

Re: Reverting pull request

The way I see it: 1) you are just complaining and have never written code that fixes flaky tests 2) you are actively introducing bugs to the CI that causes it to fail in ways unrelated to any tests Eric On 2018/06/15 22:03:29, Marco de Abreu wrote: > CI doesn't fail for no reason but

Re: Reverting pull request

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

Re: Reverting pull request

send to *dev*-*un*subscr...@mxnet.incubator.apache.org On Fri, Jun 15, 2018 at 2:59 PM, Chris Olivier wrote: > does anyone know how to unsubscribe from this list? > > > On Fri, Jun 15, 2018 at 2:56 PM Haibin Lin > wrote: > > > Why revert the PR when we know there's a fix? > > If we keep

Re: Reverting pull request

Moving this to private, I don't want our contributors to get discouraged by our internal bickering. Mu, we have to start somewhere..your comment "find enough reviewers to provide useful feedbacks for major changes." is pretty condescending and I take objection to it. By now Eric, you and Tianqi

Re: Reverting pull request

does anyone know how to unsubscribe from this list? On Fri, Jun 15, 2018 at 2:56 PM Haibin Lin wrote: > Why revert the PR when we know there's a fix? > If we keep going backwards like this, no progress can be made. > > On Fri, Jun 15, 2018 at 2:37 PM, Mu Li wrote: > > > Agree that major

Re: Reverting pull request

We have the rule that a pull request must receive an approval from at least one committer and that they must have test coverage. Both rules have been broken multiple times. I view this situation independent of the actual bug but just from the fact that it has been self-merged without approval.

Re: Reverting pull request

Why revert the PR when we know there's a fix? If we keep going backwards like this, no progress can be made. On Fri, Jun 15, 2018 at 2:37 PM, Mu Li wrote: > Agree that major changes need more extensive reviews. But we cannot ignore > that both reviews and CI cannot catch all bugs. Reverting

Re: Reverting pull request

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

Re: Reverting pull request

Agree that major changes need more extensive reviews. But we cannot ignore that both reviews and CI cannot catch all bugs. Reverting each PR after finding a bug should be the last ways, before it, we should try to fix it first. As for the breaking change, I see it differently. It breaks a not

Re: Reverting pull request

Copying a comment I made on a flaky test introduced by this PR: https://github.com/apache/incubator-mxnet/issues/11171 " @piiswrong you introduced this test in this commit [WIP] Do Not Merge. Static memory allocation for cached_op (#10817

Re: Reverting pull request

He already sends in the fix. I agree with your point about not being self-merging, but a proper way would bring this issue up friendly and move forward with a better fix. We should not shoot every contributor for a bug they introduced due to new features as long as they take responsibility to fix

Re: Reverting pull request

If it causes issues, I'd like to invite everybody to direct their requests to Eric since he merged the PR prematurely. The committer who merges a PR is responsible and can be held liable for any negative impact being the result of their action [1]. [1]:

Re: Reverting pull request

+1 The PR has been merged a while ago, so it has been tested by many people. Other people's work now depends on this PR. Reverting it at this point can cause a lot of problems for many other people. Best, Da On 6/15/18, 2:18 PM, "workc...@gmail.com on behalf of Tianqi Chen" wrote: +1

Re: Reverting pull request

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

Re: Reverting pull request

+1 We would be stuck at local minimums if we just keep reverting the PR that brings improvements in the long term Tianqi On Fri, Jun 15, 2018 at 2:15 PM, Mu Li wrote: > Why reverting instead of fixing the bugs? Static memory aims to reduce > memory allocation, it's a key feature to bridge

Re: Reverting pull request

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.ab...@googlemail.com.invalid> wrote: > Hello, > > I'm reverting