Re: Reverting pull request

[Lupesko, Hagay]

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"  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  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.ab...@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  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  > 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 

Re: Reverting pull request

[Anirudh]

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  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.ab...@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  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  > 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  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.ab...@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
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Reverting pull request

[Mu Li]

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.ab...@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  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  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  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.ab...@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
> > > > >
> > > >
> > >
> >
>

Re: Reverting pull request

[Eric Xie]

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 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  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 
> > 
> > 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  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.ab...@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
> > > > >
> > > >
> > >
> >
> 

Re: Reverting pull request

[Marco de Abreu]

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  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 
> 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  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.ab...@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
> > > >
> > >
> >
>

Re: Reverting pull request

[Mu Li]

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 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 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
> > > recommended usage of the API from an unmaintained tutorial, I don't
> think
> > > adding more reviewers will help it.
> > >
> > > Besides, I'm less sure if we can find enough reviewers to provide
> useful
> > > feedbacks for major changes.
> > >
> > > On Fri, Jun 15, 2018 at 2:21 PM, Marco de Abreu <
> > > marco.g.ab...@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  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.ab...@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
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Reverting pull request

[Naveen Swamy]

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 are considered super Hero of MXNet(no malafide
intended) and if you want to become Super hero of AI, you have to grow the
knowledge base of the community on the code base instead of thinking these
guys will not provide any valid input and asking questions in moderation is
not a bad place to start many don't participate in the PR process because
you guys are super rude and condescending like this..

I have not seen any discussion on the dev list or a design about this
feature addition? did I miss it?

I like what Tianqi said about moving forward but we cannot just make this a
norm..

Also I would like to request all committers to bring up controversial
topics on the private list and not drive away contributors outside of dmlc
and Amazon.



On Fri, Jun 15, 2018 at 11: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 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 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
> > > recommended usage of the API from an unmaintained tutorial, I don't
> think
> > > adding more reviewers will help it.
> > >
> > > Besides, I'm less sure if we can find enough reviewers to provide
> useful
> > > feedbacks for major changes.
> > >
> > > On Fri, Jun 15, 2018 at 2:21 PM, Marco de Abreu <
> > > marco.g.ab...@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  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.ab...@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
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Reverting pull request

[Chris Olivier]

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 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
> > recommended usage of the API from an unmaintained tutorial, I don't think
> > adding more reviewers will help it.
> >
> > Besides, I'm less sure if we can find enough reviewers to provide useful
> > feedbacks for major changes.
> >
> > On Fri, Jun 15, 2018 at 2:21 PM, Marco de Abreu <
> > marco.g.ab...@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  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.ab...@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
> > > > >
> > > >
> > >
> >
>

Re: Reverting pull request

[Marco de Abreu]

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. About
"but a proper way
would bring this issue up friendly": I don't know how often we already said
that this should be stopped, but there were still committers who merged
their own PRs.

This whole discussion would have been obsolete if the rules we've set up
ourselves would have been followed. The fact that this introduced a bug was
just the little push that brought this topic higher on my priority list.

I'm very sorry for any inconveniences this may cause, but next time we
should just stick to our own rules.

-Marco

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 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
> recommended usage of the API from an unmaintained tutorial, I don't think
> adding more reviewers will help it.
>
> Besides, I'm less sure if we can find enough reviewers to provide useful
> feedbacks for major changes.
>
> On Fri, Jun 15, 2018 at 2:21 PM, Marco de Abreu <
> marco.g.ab...@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  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.ab...@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
> > > >
> > >
> >
>

Re: Reverting pull request

[Haibin Lin]

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 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
> recommended usage of the API from an unmaintained tutorial, I don't think
> adding more reviewers will help it.
>
> Besides, I'm less sure if we can find enough reviewers to provide useful
> feedbacks for major changes.
>
> On Fri, Jun 15, 2018 at 2:21 PM, Marco de Abreu <
> marco.g.ab...@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  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.ab...@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
> > > >
> > >
> >
>

Re: Reverting pull request

[Eric Xie]

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  
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  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.ab...@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
> > >
> >
> 

Re: Reverting pull request

[Mu Li]

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
recommended usage of the API from an unmaintained tutorial, I don't think
adding more reviewers will help it.

Besides, I'm less sure if we can find enough reviewers to provide useful
feedbacks for major changes.

On Fri, Jun 15, 2018 at 2:21 PM, Marco de Abreu <
marco.g.ab...@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  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.ab...@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
> > >
> >
>

Re: Reverting pull request

[Thomas DELTEIL]

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
) 2dbd143


and it seems to be flaky. I have seen it failing a few times in recent
builds. Can you take a look? e.g
http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/master/1002/pipeline/

Given all the talk about quality contributions going on the dev mailing
list, I am a little unsettled by the fact this PR went undocumented (no
design docs or explanation in the PR), unreviewed (1 question ignored), the
optimization wasn't tested or benchmarked (it was actually making things
slower), and the code was self-merged.

Should we enforce that each PR , especially the ones that introduce a
significant number of changes be properly documented and reviewed before
merging?

"

My main gripe is that there is literally no description of what the PR is
achieving, no design docs to refer to.

I spent time benchmarking the different optimization because I thought I
did something wrong when I found that it was slower with the optimization.
I would have hoped that a significant change like that would have at least
been benchmarked.

In French we say "Il ne faut pas confondre vitesse et precipitation" which
loosely translates to "Haste makes waste".

I would advocate to take the time to document PRs properly, benchmark and
thoroughly test significant changes. Because down the line, other users end
up losing so much more time trying to figure out what is happening when
things go wrong (like here).

Thanks,


Thomas

2018-06-15 14:27 GMT-07:00 Marco de Abreu <
marco.g.ab...@googlemail.com.invalid>:

> 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]: https://www.apache.org/dev/committers.html#committer-responsibilities
>
> On Fri, Jun 15, 2018 at 2:23 PM Zheng, Da 
> wrote:
>
> > +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" <
> > workc...@gmail.com on behalf of tqc...@cs.washington.edu> wrote:
> >
> > +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 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 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
> > > >
> > >
> >
> >
> >
>

Re: Reverting pull request

[Tianqi Chen]

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 it.

Tianqi

On Fri, Jun 15, 2018 at 2:27 PM, Marco de Abreu <
marco.g.ab...@googlemail.com.invalid> wrote:

> 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]: https://www.apache.org/dev/committers.html#committer-responsibilities
>
> On Fri, Jun 15, 2018 at 2:23 PM Zheng, Da 
> wrote:
>
> > +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" <
> > workc...@gmail.com on behalf of tqc...@cs.washington.edu> wrote:
> >
> > +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 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 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
> > > >
> > >
> >
> >
> >
>

Re: Reverting pull request

[Marco de Abreu]

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]: https://www.apache.org/dev/committers.html#committer-responsibilities

On Fri, Jun 15, 2018 at 2:23 PM Zheng, Da  wrote:

> +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" <
> workc...@gmail.com on behalf of tqc...@cs.washington.edu> wrote:
>
> +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 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 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
> > >
> >
>
>
>

Re: Reverting pull request

[Zheng, Da]

+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   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 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 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
> >
>

Re: Reverting pull request

[Marco de Abreu]

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  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.ab...@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
> >
>

Re: Reverting pull request

[Tianqi Chen]

+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 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 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
> >
>

Re: Reverting pull request

[Mu Li]

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 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
>