You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mxnet.apache.org by Haibin Lin <ha...@gmail.com> on 2018/02/01 04:45:22 UTC
Re: Unit tests removed
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 <
marco.g.abreu@googlemail.com> 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 <ma...@googlemail.com>:
>
> > 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 <cj...@gmail.com>:
> >
> >> 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?
> >>
> >
> >
>
Re: Unit tests removed
Posted by Mu Li <mu...@gmail.com>.
Hi Hen,
I think both sides make sense to me 1) commenting out out-dated tests that
are not being tested, 2) we should not lose any test. The real issue here
is how to reach an agreement to avoid demotivating our contributors as much
as possible, especially given they are "volunteers".
It seems reasonable to me to express emotions in a *non-emotional* way, to
describe the impacts of actions on contributors. That is a good approach to
develop a community that understands each other.
On Thu, Feb 1, 2018 at 11:30 AM, Hen <ba...@apache.org> wrote:
> Doing the right thing for code is the right thing. Feelings are in how we
> communicate, not in what happens to the code.
>
> If you are saying that the unit tests should be deleted, then good, but if
> the commit was in error then rolling it back seems fine to me. Given we’re
> volunteers we can’t rely on someone adding the tests back later, that’s
> just a good intention.
>
> Hen
>
> On Thu, Feb 1, 2018 at 11:11 Mu Li <mu...@gmail.com> 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 <cj...@gmail.com>
> > 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 <pa...@intel.com>
> > > 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:cjolivier01@gmail.com]
> > > > > Sent: Thursday, February 1, 2018 1:04 PM
> > > > > To: dev@mxnet.incubator.apache.org
> > > > > 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 <
> > haibin.lin.aws@gmail.com>
> > > > > 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 <
> > > > > > marco.g.abreu@googlemail.com> 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
> > > > > > > <ma...@googlemail.com>
> > > > > > :
> > > > > > >
> > > > > > > > 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 <
> > cjolivier01@gmail.com
> > > >:
> > > > > > > >
> > > > > > > >> 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?
> > > > > > > >>
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
> > >
> >
>
Re: Unit tests removed
Posted by Hen <ba...@apache.org>.
Doing the right thing for code is the right thing. Feelings are in how we
communicate, not in what happens to the code.
If you are saying that the unit tests should be deleted, then good, but if
the commit was in error then rolling it back seems fine to me. Given we’re
volunteers we can’t rely on someone adding the tests back later, that’s
just a good intention.
Hen
On Thu, Feb 1, 2018 at 11:11 Mu Li <mu...@gmail.com> 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 <cj...@gmail.com>
> 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 <pa...@intel.com>
> > 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:cjolivier01@gmail.com]
> > > > Sent: Thursday, February 1, 2018 1:04 PM
> > > > To: dev@mxnet.incubator.apache.org
> > > > 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 <
> haibin.lin.aws@gmail.com>
> > > > 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 <
> > > > > marco.g.abreu@googlemail.com> 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
> > > > > > <ma...@googlemail.com>
> > > > > :
> > > > > >
> > > > > > > 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 <
> cjolivier01@gmail.com
> > >:
> > > > > > >
> > > > > > >> 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?
> > > > > > >>
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > >
> >
>
Re: Unit tests removed
Posted by Mu Li <mu...@gmail.com>.
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 <cj...@gmail.com>
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 <mu...@gmail.com> 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 <
> > kellen.sunderland@gmail.com> 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 <mu...@gmail.com> 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 <
> cjolivier01@gmail.com>
> > > > 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 <
> patric.zhao@intel.com
> > >
> > > > > 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:cjolivier01@gmail.com]
> > > > > > > Sent: Thursday, February 1, 2018 1:04 PM
> > > > > > > To: dev@mxnet.incubator.apache.org
> > > > > > > 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 <
> > > > haibin.lin.aws@gmail.com>
> > > > > > > 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 <
> > > > > > > > marco.g.abreu@googlemail.com> 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
> > > > > > > > > <ma...@googlemail.com>
> > > > > > > > :
> > > > > > > > >
> > > > > > > > > > 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 <
> > > > cjolivier01@gmail.com
> > > > > >:
> > > > > > > > > >
> > > > > > > > > >> 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?
> > > > > > > > > >>
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: Unit tests removed
Posted by Chris Olivier <cj...@gmail.com>.
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 <mu...@gmail.com> 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 <
> kellen.sunderland@gmail.com> 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 <mu...@gmail.com> 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 <cj...@gmail.com>
> > > 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 <patric.zhao@intel.com
> >
> > > > 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:cjolivier01@gmail.com]
> > > > > > Sent: Thursday, February 1, 2018 1:04 PM
> > > > > > To: dev@mxnet.incubator.apache.org
> > > > > > 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 <
> > > haibin.lin.aws@gmail.com>
> > > > > > 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 <
> > > > > > > marco.g.abreu@googlemail.com> 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
> > > > > > > > <ma...@googlemail.com>
> > > > > > > :
> > > > > > > >
> > > > > > > > > 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 <
> > > cjolivier01@gmail.com
> > > > >:
> > > > > > > > >
> > > > > > > > >> 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?
> > > > > > > > >>
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > >
> > > >
> > >
> >
>
Re: Unit tests removed
Posted by Mu Li <mu...@gmail.com>.
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 <
kellen.sunderland@gmail.com> 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 <mu...@gmail.com> 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 <cj...@gmail.com>
> > 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 <pa...@intel.com>
> > > 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:cjolivier01@gmail.com]
> > > > > Sent: Thursday, February 1, 2018 1:04 PM
> > > > > To: dev@mxnet.incubator.apache.org
> > > > > 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 <
> > haibin.lin.aws@gmail.com>
> > > > > 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 <
> > > > > > marco.g.abreu@googlemail.com> 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
> > > > > > > <ma...@googlemail.com>
> > > > > > :
> > > > > > >
> > > > > > > > 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 <
> > cjolivier01@gmail.com
> > > >:
> > > > > > > >
> > > > > > > >> 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?
> > > > > > > >>
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
> > >
> >
>
Re: Unit tests removed
Posted by kellen sunderland <ke...@gmail.com>.
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 <mu...@gmail.com> 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 <cj...@gmail.com>
> 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 <pa...@intel.com>
> > 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:cjolivier01@gmail.com]
> > > > Sent: Thursday, February 1, 2018 1:04 PM
> > > > To: dev@mxnet.incubator.apache.org
> > > > 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 <
> haibin.lin.aws@gmail.com>
> > > > 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 <
> > > > > marco.g.abreu@googlemail.com> 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
> > > > > > <ma...@googlemail.com>
> > > > > :
> > > > > >
> > > > > > > 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 <
> cjolivier01@gmail.com
> > >:
> > > > > > >
> > > > > > >> 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?
> > > > > > >>
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > >
> >
>
Re: Unit tests removed
Posted by Mu Li <mu...@gmail.com>.
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 <cj...@gmail.com>
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 <pa...@intel.com>
> 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:cjolivier01@gmail.com]
> > > Sent: Thursday, February 1, 2018 1:04 PM
> > > To: dev@mxnet.incubator.apache.org
> > > 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 <ha...@gmail.com>
> > > 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 <
> > > > marco.g.abreu@googlemail.com> 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
> > > > > <ma...@googlemail.com>
> > > > :
> > > > >
> > > > > > 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 <cjolivier01@gmail.com
> >:
> > > > > >
> > > > > >> 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?
> > > > > >>
> > > > > >
> > > > > >
> > > > >
> > > >
> >
>
Re: Unit tests removed
Posted by Chris Olivier <cj...@gmail.com>.
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 <pa...@intel.com> 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:cjolivier01@gmail.com]
> > Sent: Thursday, February 1, 2018 1:04 PM
> > To: dev@mxnet.incubator.apache.org
> > 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 <ha...@gmail.com>
> > 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 <
> > > marco.g.abreu@googlemail.com> 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
> > > > <ma...@googlemail.com>
> > > :
> > > >
> > > > > 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 <cj...@gmail.com>:
> > > > >
> > > > >> 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?
> > > > >>
> > > > >
> > > > >
> > > >
> > >
>
RE: Unit tests removed
Posted by "Zhao, Patric" <pa...@intel.com>.
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:cjolivier01@gmail.com]
> Sent: Thursday, February 1, 2018 1:04 PM
> To: dev@mxnet.incubator.apache.org
> 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 <ha...@gmail.com>
> 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 <
> > marco.g.abreu@googlemail.com> 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
> > > <ma...@googlemail.com>
> > :
> > >
> > > > 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 <cj...@gmail.com>:
> > > >
> > > >> 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?
> > > >>
> > > >
> > > >
> > >
> >
Re: Unit tests removed
Posted by Chris Olivier <cj...@gmail.com>.
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 <ha...@gmail.com>
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 <
> marco.g.abreu@googlemail.com> 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 <ma...@googlemail.com>
> :
> >
> > > 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 <cj...@gmail.com>:
> > >
> > >> 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?
> > >>
> > >
> > >
> >
>