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 <muli....@gmail.com> 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 <muli....@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.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 > > > > > > > > > >
