I think I mean both explicit disagreement and potential ones that the contributor didn’t have a chance to express it yet. For example, your comments after #8302 have been merged, and Da even didn't have a chance to look into the revert PR, which is merged in an hour.
I’m aware that the veto rule, but it doesn’t help move things forward. On Thu, Feb 1, 2018 at 11:45 AM, Chris Olivier <[email protected]> wrote: > What do you mean by "conflicted PR"? Do you mean disagreements in what can > go in or out? If so, I believe that Apache addresses this. > > Per Apache: > > "A code-modification proposal may be stopped dead in its tracks by a -1 > vote by a qualified voter. This constitutes a veto, and it cannot be > overruled nor overridden by anyone. Vetos stand until and unless withdrawn > by their casters." > > > > On Thu, Feb 1, 2018 at 11:30 AM, Mu Li <[email protected]> wrote: > > > Hi Kellen, > > > > The CPP tests PR totally make sense to me. What I really want to figure > out > > is the process that reaching an agreement before merging conflicted PRs. > > > > Best > > Mu > > > > On Thu, Feb 1, 2018 at 11:25 AM, kellen sunderland < > > [email protected]> wrote: > > > > > Hey Mu + Da, sorry to hear that. I've been working on the CPP tests PR > > and > > > iterating on it for quite a while. I can assure you getting it merged > > had > > > nothing to do with this revert from my perceptive. It's actually a > task > > > assigned to me for Q1 internally at Amazon. > > > > > > I'm actually completely unfamiliar with what's going regarding the > > revert, > > > but I'm quite looking forward to this PR getting merged. It seemed to > > > cleanup the code a lot, and I known Asmus and I will be looking closely > > at > > > the perf numbers. (MKL in the past has given us huge speed boosts on a > > > service we're optimizing). > > > > > > -Kellen > > > > > > On Thu, Feb 1, 2018 at 8:11 PM, Mu Li <[email protected]> wrote: > > > > > > > I think simply reverting the MKL PR that Da has been working for a > half > > > > year and immediately enabling the cpp tests in the CI without > reaching > > an > > > > agreement with Da seriously hurts his feelings. > > > > > > > > I put the details at the end of > > > > https://github.com/apache/incubator-mxnet/pull/9661 > > > > > > > > > > > > On Thu, Feb 1, 2018 at 10:42 AM, Chris Olivier < > [email protected]> > > > > wrote: > > > > > > > > > I have created starter changes for converting to the > CoreOpExecutor. > > > This > > > > > code compiles the first test. > > > > > > > > > > See notes in the PR description: > > > > > > > > > > https://github.com/cjolivier01/mxnet/pull/2 > > > > > > > > > > On Wed, Jan 31, 2018 at 9:47 PM, Zhao, Patric < > [email protected] > > > > > > > > wrote: > > > > > > > > > > > Thanks, Chris, I agree the quality is the most important thing. > > > > > > > > > > > > We will try to enable these cases ASAP. > > > > > > > > > > > > ---Patric > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Chris Olivier [mailto:[email protected]] > > > > > > > Sent: Thursday, February 1, 2018 1:04 PM > > > > > > > To: [email protected] > > > > > > > Subject: Re: Unit tests removed > > > > > > > > > > > > > > C++ Unit tests test correctness of output as well as > consistency > > of > > > > > *all > > > > > > > inputs and outputs* between different implementations (mkl, > cpu, > > > gpu, > > > > > > > cudnn, etc.) > > > > > > > > > > > > > > C++ Unit tests also test that varying channel axes produce the > > > > expected > > > > > > > numeric distributions. > > > > > > > > > > > > > > In addition, *batch norm tests in test_operator.py are disabled > > due > > > > to > > > > > CI > > > > > > > issues*, so this leaves batch norm largely untested (even > though > > > > those > > > > > > tests > > > > > > > only tested numeric gradient and nothing else -- they were > > existing > > > > > tests > > > > > > > before I re-factored batch norm and they were weak to begin > > with). > > > > > > > > > > > > > > C++ unit tests also measure performance differences between the > > > > various > > > > > > > versions (cpu, mkl, gpu, cudnn, etc). > > > > > > > I've made several comments over the course of this mkl-dnn > > project > > > > that > > > > > > the > > > > > > > unit tests need to be fixed to adjust to the refactor, so this > > > > > shouldn't > > > > > > br a > > > > > > > surprise to anyone. > > > > > > > > > > > > > > Hacking out unit tests when it's inconvenient to fix them > because > > > of > > > > > your > > > > > > > code changes sets a *dangerous precedent*. What would the rule > > be? > > > > Is > > > > > it > > > > > > > "don't remove tests unless your PR is really big and it would > > take > > > > you > > > > > a > > > > > > > couple of days to fix them"? We aren't talking about 1-2 > tests, > > > > either > > > > > > -- > > > > > > > we're talking about a LOT of tests that test various aspects of > > the > > > > > > operators. > > > > > > > > > > > > > > As for Intel, they are free to use the PR branch, and even if > > they > > > > > > can't, I don't > > > > > > > think that (or anything else) justifies compromising quality. > > > > > > > > > > > > > > > > > > > > > On Wed, Jan 31, 2018 at 8:45 PM, Haibin Lin < > > > > [email protected]> > > > > > > > wrote: > > > > > > > > > > > > > > > Good catch. > > > > > > > > > > > > > > > > In general, I agree that tests are not supposed to removed, > > > > although > > > > > > > > CI is not running any of these cpp tests just yet. Usually > unit > > > > tests > > > > > > > > in python for individual operators should be sufficient to > test > > > the > > > > > > > > correctness of operators (although I don't know how/if python > > > tests > > > > > > > > can run on edge devices like iOS/Android) and it's not > feasible > > > to > > > > > > > > duplicate all operator python tests with their cpp > > counterparts. > > > > For > > > > > > > > functionalities that are not exposed to python or memory > > checks, > > > I > > > > do > > > > > > > > agree that cpp tests are necessary. > > > > > > > > > > > > > > > > I am curious what are exactly tested in those cpp tests? I > know > > > > that > > > > > > > > BatchNorm ops are tested in python already, but I don't have > a > > > full > > > > > > > > picture of all cpp tests and whether we want to re-implement > > > them. > > > > > > > > > > > > > > > > On the other hand, be aware that the Intel team have a few > > > MKL-DNN > > > > > > > > related projects depending on this PR. Reverting the PR will > > be a > > > > > > blocker for > > > > > > > them. > > > > > > > > > > > > > > > > Best, > > > > > > > > Haibin > > > > > > > > > > > > > > > > On Wed, Jan 31, 2018 at 3:30 PM, Marco de Abreu < > > > > > > > > [email protected]> wrote: > > > > > > > > > > > > > > > > > Here's a link: > > > > > > > > > https://github.com/apache/incubator-mxnet/pull/8302# > > > > > > > > discussion_r165204667 > > > > > > > > > > > > > > > > > > -Marco > > > > > > > > > > > > > > > > > > 2018-01-31 15:23 GMT-08:00 Marco de Abreu > > > > > > > > > <[email protected]> > > > > > > > > : > > > > > > > > > > > > > > > > > > > Hi Chris, > > > > > > > > > > > > > > > > > > > > considering the size of that PR, could you provide a > direct > > > > link > > > > > > > > > > to the changes you're addressing? > > > > > > > > > > > > > > > > > > > > -Marco > > > > > > > > > > > > > > > > > > > > 2018-01-31 13:39 GMT-08:00 Chris Olivier < > > > > [email protected] > > > > > >: > > > > > > > > > > > > > > > > > > > >> This PR was just merged that removed some 30 or so C++ > > unit > > > > > tests > > > > > > > > > >> for batch norm operator. > > > > > > > > > >> > > > > > > > > > >> https://github.com/apache/incubator-mxnet/pull/8302 > > > > > > > > > >> > > > > > > > > > >> Is this ok? > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
