mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mu Li <muli....@gmail.com>
Subject Re: Reverting pull request
Date Fri, 15 Jun 2018 21:37:06 GMT
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.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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message