Making new operators and AMP lists

[Przemys��aw Tr��dak <pt...@apache.org>]

Dear Community,

One of the recently merged features of the 1.5 release, AMP (Automatic Mixed Precision) support (PR [1], design doc [5]), introduced a requirement that every new operator added to MXNet would need to be present in 1 of the lists (in [2]). To make sure that this requirement is not broken when somebody adds a new operator and does not know about AMP's existence, a test was added to CI ([3]).

A few people reached out to me (the original author of the feature) saying this test increases a burden on a developer of new operators and should not be an actual error, but just warning (PR for that change [4]). That is why I would like to present a motivation for it and discuss with the wider audience why I feel it was necessary.

First, for people who do not know the details of what AMP is - it is a solution that tries to automatically apply best practices of training in lower precision (FP16) to user's FP32 model in order to fully utilize capabilities of modern GPUs (and potentially other hardware in the future). It does so by casting to lower precision inputs to operators benefitting from it, while casting to full precision inputs of operators that are unsafe to run in lower precision or just do not support it.

The first iteration of AMP kept 2 main lists of operators - operators that are beneficial and safe to do in fp16 and operators that need to be cast to FP32. The problem (raised in review of the PR [6], [8]) is how to make sure that the feature works as intended and is not inadvertently broken by somebody adding a new operator. The failure scenario here is adding a new operator that does not support FP16 and so should be cast to FP32, but AMP does not know about its existence and so does not do the casting. The solution proposed in the review was to implicitly treat all of the unknown operators as FP32-only and keep the list of operators that work fine in both FP16 and FP32. This solution however does not really work, because there are multiple operators (most notably optimizers) where introducing additional casting of the input to FP32 would break the operator.

That is why after discussion with a few members of the community, I decided to proceed with all lists being explicit and introducing the test that would fail when somebody added an operator without classifying it into 1 of the categories, and explain clearly how to do it [7]. It is not ideal solution, as it introduces some burden on the developers who are not aware about AMP, however in the typical case of adding at most a few operators to MXNet the inconvenience is I think pretty minor while important for the feature correctness going forward.

I would like to gather Community feedback and ideas how to handle this situation.

[1] https://github.com/apache/incubator-mxnet/pull/14173
[2] https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
[3] https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
[4] https://github.com/apache/incubator-mxnet/pull/15085
[5] https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
[6] https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
[7] https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
[8] https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341

Re: Making new operators and AMP lists

[Anirudh Subramanian <an...@gmail.com>]

Hi all,

I had discussion with Przemyslaw about this offline. There are two options
we can pursue to make developer experience better ( Since currently they
have to wait for CI to complete):

1. Obtain the current lists and check if the length of the combined lists
is same as MXListAllOpNames which gets trigger during "import mxnet". This
provides much early feedback to the developer instead of compiling, testing
and pushing code and waiting to be failed in the CI.
2. Option 1 still has the problem, that developer have to still add op name
to the lists, when adding a new alias. This pain point can be reduced by
adding additional attr for an operator (also suggested by Haibin earlier)
which will tell whether to force cast to FP32, FP16 or no casts. This way
adding a new alias wont cause additional burden on the developer and the
attr if not set can still  be caught early at the statement "import mxnet".

Thoughts?

Anirudh

On Tue, May 28, 2019 at 2:32 PM Przemysław Trędak <pt...@apache.org>
wrote:

> Dear Community,
>
> One of the recently merged features of the 1.5 release, AMP (Automatic
> Mixed Precision) support (PR [1], design doc [5]), introduced a requirement
> that every new operator added to MXNet would need to be present in 1 of the
> lists (in [2]). To make sure that this requirement is not broken when
> somebody adds a new operator and does not know about AMP's existence, a
> test was added to CI ([3]).
>
> A few people reached out to me (the original author of the feature) saying
> this test increases a burden on a developer of new operators and should not
> be an actual error, but just warning (PR for that change [4]). That is why
> I would like to present a motivation for it and discuss with the wider
> audience why I feel it was necessary.
>
> First, for people who do not know the details of what AMP is - it is a
> solution that tries to automatically apply best practices of training in
> lower precision (FP16) to user's FP32 model in order to fully utilize
> capabilities of modern GPUs (and potentially other hardware in the future).
> It does so by casting to lower precision inputs to operators benefitting
> from it, while casting to full precision inputs of operators that are
> unsafe to run in lower precision or just do not support it.
>
> The first iteration of AMP kept 2 main lists of operators - operators that
> are beneficial and safe to do in fp16 and operators that need to be cast to
> FP32. The problem (raised in review of the PR [6], [8]) is how to make sure
> that the feature works as intended and is not inadvertently broken by
> somebody adding a new operator. The failure scenario here is adding a new
> operator that does not support FP16 and so should be cast to FP32, but AMP
> does not know about its existence and so does not do the casting. The
> solution proposed in the review was to implicitly treat all of the unknown
> operators as FP32-only and keep the list of operators that work fine in
> both FP16 and FP32. This solution however does not really work, because
> there are multiple operators (most notably optimizers) where introducing
> additional casting of the input to FP32 would break the operator.
>
> That is why after discussion with a few members of the community, I
> decided to proceed with all lists being explicit and introducing the test
> that would fail when somebody added an operator without classifying it into
> 1 of the categories, and explain clearly how to do it [7]. It is not ideal
> solution, as it introduces some burden on the developers who are not aware
> about AMP, however in the typical case of adding at most a few operators to
> MXNet the inconvenience is I think pretty minor while important for the
> feature correctness going forward.
>
> I would like to gather Community feedback and ideas how to handle this
> situation.
>
> [1] https://github.com/apache/incubator-mxnet/pull/14173
> [2]
> https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> [3]
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> [4] https://github.com/apache/incubator-mxnet/pull/15085
> [5]
> https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> [6]
> https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> [7]
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> [8]
> https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
>

Re: Making new operators and AMP lists

[Anirudh Subramanian <an...@gmail.com>]

Hi,

I agree with Marco there are some easy wins to be had since many new  GPU
operators come with FP16 support.
I think we can explore the overhead to the developer and try to reduce the
feedback time for the developer, so
that cost associated with adding support for AMP feature is minimized.
Also, this will be very important once we move the feature out of contrib.

Anirudh

On Tue, May 28, 2019 at 3:52 PM Marco de Abreu <ma...@gmail.com>
wrote:

> Hi,
>
> I'm generally in favour of these kind of tests since they make developers
> aware of changes they have to make which they would usually not be aware
> of. We have a similar test for tutorials, for example. Whenever somebody
> adds a tutorial, there's a validation that assures that all contraints in
> our testing environment are met and that they are properly tied into the
> system. This AMP test fits into the same category in my opinion and we
> never heard bad feedback about these kind of checks.
>
> What seems to be bothering people is the fact that the feedback time is too
> high. Thus, I'd like to propose to move the test into the sanity-test stage
> instead of doing it as part of the unit tests which take quite a bit of
> time until they're actually executed. The sanity checks run immediately and
> give a response within about 1 minute.
>
> While I understand that this might increase the amount of work a developer
> has to do if they develop a new operator, I think that this is the right
> thing to do. Developers won't know of every single feature other people
> worked on and thus might simply miss adding the support for it. This kind
> of test on the other hand makes them aware of it. If they'd like to opt
> out, it's one single line they would have to change and then they're
> totally fine. On the other hand, this might motivate them to add the
> support since the kernel would be the last piece and everything else would
> already be implemented.
>
> Considering how often a PR gets declined because of linting errors, I'd say
> that these kind of errors are WAY more frequent that AMP telling somebody
> to add their operator to a list. Considering that this would only have to
> be done once per operator, that's work of about one minute. Add that to the
> waiting time of the sanity check and you're left with about five "wasted"
> minutes.
>
> I'm opposed towards adding a warning or treating them as float32 by default
> since the operator author wouldn't notice. What will happen is that people
> won't know about AMP and simply forget about low precision in general until
> they're actively reminded. This check will remind them actively and thus
> bring more attention to the feature. I know that the feature is still
> experimental, but we have just started with the 1.6 branch and thus there's
> enough time to make the experimental features production ready. Adding this
> test early on will allow others to add the support for AMP during the early
> stage of the 1.6 branch instead of asking them in the last few weeks before
> a release. The result would only be that stuff is rushed or forgotten.
>
> To sum it up: I think this test is good and it should be kept as error, but
> it should be moved to sanity checks.
>
> -Marco
>
> On Wed, May 29, 2019 at 12:21 AM Sheng Zha <zh...@apache.org> wrote:
>
> > Thanks for initiating the discussion.
> >
> > The premise for adding the test was to make sure that AMP feature is "not
> > broken", but that's IMO not the right view. AMP is not supposed to
> support
> > a new operator it hasn't seen before in the first place. There's no way
> for
> > it to know whether the fp32 cast should happen or not. So AMP feature
> > cannot provide the guarantee that it works for all future operators.
> Thus,
> > adding new operators to AMP list should be considered new feature instead
> > of fixing existing feature.
> >
> > The AMP test that breaks upon the addition of new operator is thus
> > equivalent to forcing developers of the new operator to add the new
> support
> > for AMP. This feels wrong. Especially given that AMP is an experimental
> > feature in contrib namespace (i.e. no semver guarantee), this practice
> > should be stopped immediately. We cannot force new developers to invest
> > into experimental feature this way.
> >
> > I'd suggest the following changes:
> > - for new operators that aren't registered in AMP, cast to float32 by
> > default and print one-time warning. People using AMP who want to avoid
> > casting can register it in the AMP's list.
> > - change the test to print warning about the operators that are not
> listed
> > so that it's easy to track the problem.
> >
> > -sz
> >
> > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org> wrote:
> > > Dear Community,
> > >
> > > One of the recently merged features of the 1.5 release, AMP (Automatic
> > Mixed Precision) support (PR [1], design doc [5]), introduced a
> requirement
> > that every new operator added to MXNet would need to be present in 1 of
> the
> > lists (in [2]). To make sure that this requirement is not broken when
> > somebody adds a new operator and does not know about AMP's existence, a
> > test was added to CI ([3]).
> > >
> > > A few people reached out to me (the original author of the feature)
> > saying this test increases a burden on a developer of new operators and
> > should not be an actual error, but just warning (PR for that change [4]).
> > That is why I would like to present a motivation for it and discuss with
> > the wider audience why I feel it was necessary.
> > >
> > > First, for people who do not know the details of what AMP is - it is a
> > solution that tries to automatically apply best practices of training in
> > lower precision (FP16) to user's FP32 model in order to fully utilize
> > capabilities of modern GPUs (and potentially other hardware in the
> future).
> > It does so by casting to lower precision inputs to operators benefitting
> > from it, while casting to full precision inputs of operators that are
> > unsafe to run in lower precision or just do not support it.
> > >
> > > The first iteration of AMP kept 2 main lists of operators - operators
> > that are beneficial and safe to do in fp16 and operators that need to be
> > cast to FP32. The problem (raised in review of the PR [6], [8]) is how to
> > make sure that the feature works as intended and is not inadvertently
> > broken by somebody adding a new operator. The failure scenario here is
> > adding a new operator that does not support FP16 and so should be cast to
> > FP32, but AMP does not know about its existence and so does not do the
> > casting. The solution proposed in the review was to implicitly treat all
> of
> > the unknown operators as FP32-only and keep the list of operators that
> work
> > fine in both FP16 and FP32. This solution however does not really work,
> > because there are multiple operators (most notably optimizers) where
> > introducing additional casting of the input to FP32 would break the
> > operator.
> > >
> > > That is why after discussion with a few members of the community, I
> > decided to proceed with all lists being explicit and introducing the test
> > that would fail when somebody added an operator without classifying it
> into
> > 1 of the categories, and explain clearly how to do it [7]. It is not
> ideal
> > solution, as it introduces some burden on the developers who are not
> aware
> > about AMP, however in the typical case of adding at most a few operators
> to
> > MXNet the inconvenience is I think pretty minor while important for the
> > feature correctness going forward.
> > >
> > > I would like to gather Community feedback and ideas how to handle this
> > situation.
> > >
> > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > [2]
> >
> https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > [3]
> >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > [5]
> >
> https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > [6]
> >
> https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > [7]
> >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > [8]
> >
> https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > >
> >
>

Re: Making new operators and AMP lists

[Sheng Zha <zh...@apache.org>]

Please don't be dismissive. For a new contributor, every time we impose a requirement to the developer, a new task would pop up out of nowhere. The more we do that, the more costly it becomes for new contributors to have their contributions accepted, and hence the less likely they will stick around. Let's avoid these unnecessary requirements from hindering the development process that could have been a lot more smooth.

-sz

On 2019/05/28 23:27:26, Marco de Abreu <ma...@gmail.com> wrote: 
> Hey Jun,
> 
> could we please quantify the amount of time and effort that is required to
> follow the actions to add an operator to the FP32_FUNCS? To me it sounds
> like we are making this a bigger deal than it actually is.
> 
> -Marco
> 
> On Wed, May 29, 2019 at 1:20 AM Jun Wu <wu...@gmail.com> wrote:
> 
> > Thanks for initiating the discussion on dev.
> >
> > I understand the dilemma from designing AMP for making the feature usable
> > and functional as well as for not breaking other developer experience.
> > However, IMO, this is not about WHEN we should let other developers know
> > they have made a mistake by not adding newly developed ops to the AMP list,
> > but rather about whether we should do it in this way and thinking about its
> > long-term impact on development.
> >
> > Asking other developers to delve into the feature they are not necessarily
> > aware of has imposed a too strong burden and will hinder others'
> > development work. FP16 is a feature nice-to-have in model training, but has
> > not become a standard so that every developer should be aware of. As we are
> > growing the community by encouraging more junior developers to contribute,
> > such error not caused by their work would actually hold them back from
> > moving forward.
> >
> > IMO, AMP is not supposed cover the operators it has not seen and should
> > treat them as those in FP32_FUNCS to be safe. We should come up with a
> > maintenance plan of promoting newly added operators that can benefit from
> > FP16 computing into the appropriate lists, instead of shifting the burden
> > away to many other developers at the moment.
> >
> > On Tue, May 28, 2019 at 4:13 PM Sheng Zha <zh...@apache.org> wrote:
> >
> > > The support for AMP should not be a burden of authors of new operators.
> > > The lint analogy doesn't apply because lint is for established and
> > accepted
> > > coding standard at MXNet and AMP is not. AMP is an experimental feature
> > > right now and it doesn't make sense to require contributors to invest in
> > > it. Keeping this as error will inevitably cause comments like "CI test
> > > failure seems unrelated to my change, please proceed and merge".
> > >
> > > -sz
> > >
> > > On 2019/05/28 22:51:30, Marco de Abreu <ma...@gmail.com> wrote:
> > > > Hi,
> > > >
> > > > I'm generally in favour of these kind of tests since they make
> > developers
> > > > aware of changes they have to make which they would usually not be
> > aware
> > > > of. We have a similar test for tutorials, for example. Whenever
> > somebody
> > > > adds a tutorial, there's a validation that assures that all contraints
> > in
> > > > our testing environment are met and that they are properly tied into
> > the
> > > > system. This AMP test fits into the same category in my opinion and we
> > > > never heard bad feedback about these kind of checks.
> > > >
> > > > What seems to be bothering people is the fact that the feedback time is
> > > too
> > > > high. Thus, I'd like to propose to move the test into the sanity-test
> > > stage
> > > > instead of doing it as part of the unit tests which take quite a bit of
> > > > time until they're actually executed. The sanity checks run immediately
> > > and
> > > > give a response within about 1 minute.
> > > >
> > > > While I understand that this might increase the amount of work a
> > > developer
> > > > has to do if they develop a new operator, I think that this is the
> > right
> > > > thing to do. Developers won't know of every single feature other people
> > > > worked on and thus might simply miss adding the support for it. This
> > kind
> > > > of test on the other hand makes them aware of it. If they'd like to opt
> > > > out, it's one single line they would have to change and then they're
> > > > totally fine. On the other hand, this might motivate them to add the
> > > > support since the kernel would be the last piece and everything else
> > > would
> > > > already be implemented.
> > > >
> > > > Considering how often a PR gets declined because of linting errors, I'd
> > > say
> > > > that these kind of errors are WAY more frequent that AMP telling
> > somebody
> > > > to add their operator to a list. Considering that this would only have
> > to
> > > > be done once per operator, that's work of about one minute. Add that to
> > > the
> > > > waiting time of the sanity check and you're left with about five
> > "wasted"
> > > > minutes.
> > > >
> > > > I'm opposed towards adding a warning or treating them as float32 by
> > > default
> > > > since the operator author wouldn't notice. What will happen is that
> > > people
> > > > won't know about AMP and simply forget about low precision in general
> > > until
> > > > they're actively reminded. This check will remind them actively and
> > thus
> > > > bring more attention to the feature. I know that the feature is still
> > > > experimental, but we have just started with the 1.6 branch and thus
> > > there's
> > > > enough time to make the experimental features production ready. Adding
> > > this
> > > > test early on will allow others to add the support for AMP during the
> > > early
> > > > stage of the 1.6 branch instead of asking them in the last few weeks
> > > before
> > > > a release. The result would only be that stuff is rushed or forgotten.
> > > >
> > > > To sum it up: I think this test is good and it should be kept as error,
> > > but
> > > > it should be moved to sanity checks.
> > > >
> > > > -Marco
> > > >
> > > > On Wed, May 29, 2019 at 12:21 AM Sheng Zha <zh...@apache.org>
> > wrote:
> > > >
> > > > > Thanks for initiating the discussion.
> > > > >
> > > > > The premise for adding the test was to make sure that AMP feature is
> > > "not
> > > > > broken", but that's IMO not the right view. AMP is not supposed to
> > > support
> > > > > a new operator it hasn't seen before in the first place. There's no
> > > way for
> > > > > it to know whether the fp32 cast should happen or not. So AMP feature
> > > > > cannot provide the guarantee that it works for all future operators.
> > > Thus,
> > > > > adding new operators to AMP list should be considered new feature
> > > instead
> > > > > of fixing existing feature.
> > > > >
> > > > > The AMP test that breaks upon the addition of new operator is thus
> > > > > equivalent to forcing developers of the new operator to add the new
> > > support
> > > > > for AMP. This feels wrong. Especially given that AMP is an
> > experimental
> > > > > feature in contrib namespace (i.e. no semver guarantee), this
> > practice
> > > > > should be stopped immediately. We cannot force new developers to
> > invest
> > > > > into experimental feature this way.
> > > > >
> > > > > I'd suggest the following changes:
> > > > > - for new operators that aren't registered in AMP, cast to float32 by
> > > > > default and print one-time warning. People using AMP who want to
> > avoid
> > > > > casting can register it in the AMP's list.
> > > > > - change the test to print warning about the operators that are not
> > > listed
> > > > > so that it's easy to track the problem.
> > > > >
> > > > > -sz
> > > > >
> > > > > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org>
> > > wrote:
> > > > > > Dear Community,
> > > > > >
> > > > > > One of the recently merged features of the 1.5 release, AMP
> > > (Automatic
> > > > > Mixed Precision) support (PR [1], design doc [5]), introduced a
> > > requirement
> > > > > that every new operator added to MXNet would need to be present in 1
> > > of the
> > > > > lists (in [2]). To make sure that this requirement is not broken when
> > > > > somebody adds a new operator and does not know about AMP's
> > existence, a
> > > > > test was added to CI ([3]).
> > > > > >
> > > > > > A few people reached out to me (the original author of the feature)
> > > > > saying this test increases a burden on a developer of new operators
> > and
> > > > > should not be an actual error, but just warning (PR for that change
> > > [4]).
> > > > > That is why I would like to present a motivation for it and discuss
> > > with
> > > > > the wider audience why I feel it was necessary.
> > > > > >
> > > > > > First, for people who do not know the details of what AMP is - it
> > is
> > > a
> > > > > solution that tries to automatically apply best practices of training
> > > in
> > > > > lower precision (FP16) to user's FP32 model in order to fully utilize
> > > > > capabilities of modern GPUs (and potentially other hardware in the
> > > future).
> > > > > It does so by casting to lower precision inputs to operators
> > > benefitting
> > > > > from it, while casting to full precision inputs of operators that are
> > > > > unsafe to run in lower precision or just do not support it.
> > > > > >
> > > > > > The first iteration of AMP kept 2 main lists of operators -
> > operators
> > > > > that are beneficial and safe to do in fp16 and operators that need to
> > > be
> > > > > cast to FP32. The problem (raised in review of the PR [6], [8]) is
> > how
> > > to
> > > > > make sure that the feature works as intended and is not inadvertently
> > > > > broken by somebody adding a new operator. The failure scenario here
> > is
> > > > > adding a new operator that does not support FP16 and so should be
> > cast
> > > to
> > > > > FP32, but AMP does not know about its existence and so does not do
> > the
> > > > > casting. The solution proposed in the review was to implicitly treat
> > > all of
> > > > > the unknown operators as FP32-only and keep the list of operators
> > that
> > > work
> > > > > fine in both FP16 and FP32. This solution however does not really
> > work,
> > > > > because there are multiple operators (most notably optimizers) where
> > > > > introducing additional casting of the input to FP32 would break the
> > > > > operator.
> > > > > >
> > > > > > That is why after discussion with a few members of the community, I
> > > > > decided to proceed with all lists being explicit and introducing the
> > > test
> > > > > that would fail when somebody added an operator without classifying
> > it
> > > into
> > > > > 1 of the categories, and explain clearly how to do it [7]. It is not
> > > ideal
> > > > > solution, as it introduces some burden on the developers who are not
> > > aware
> > > > > about AMP, however in the typical case of adding at most a few
> > > operators to
> > > > > MXNet the inconvenience is I think pretty minor while important for
> > the
> > > > > feature correctness going forward.
> > > > > >
> > > > > > I would like to gather Community feedback and ideas how to handle
> > > this
> > > > > situation.
> > > > > >
> > > > > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > > > > [2]
> > > > >
> > >
> > https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > > > > [3]
> > > > >
> > >
> > https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > > > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > > > > [5]
> > > > >
> > >
> > https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > > > > [6]
> > > > >
> > >
> > https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > > > > [7]
> > > > >
> > >
> > https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > > > > [8]
> > > > >
> > >
> > https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > > > > >
> > > > >
> > > >
> > >
> >
> 

Re: Making new operators and AMP lists

[Marco de Abreu <ma...@gmail.com>]

Hey Jun,

could we please quantify the amount of time and effort that is required to
follow the actions to add an operator to the FP32_FUNCS? To me it sounds
like we are making this a bigger deal than it actually is.

-Marco

On Wed, May 29, 2019 at 1:20 AM Jun Wu <wu...@gmail.com> wrote:

> Thanks for initiating the discussion on dev.
>
> I understand the dilemma from designing AMP for making the feature usable
> and functional as well as for not breaking other developer experience.
> However, IMO, this is not about WHEN we should let other developers know
> they have made a mistake by not adding newly developed ops to the AMP list,
> but rather about whether we should do it in this way and thinking about its
> long-term impact on development.
>
> Asking other developers to delve into the feature they are not necessarily
> aware of has imposed a too strong burden and will hinder others'
> development work. FP16 is a feature nice-to-have in model training, but has
> not become a standard so that every developer should be aware of. As we are
> growing the community by encouraging more junior developers to contribute,
> such error not caused by their work would actually hold them back from
> moving forward.
>
> IMO, AMP is not supposed cover the operators it has not seen and should
> treat them as those in FP32_FUNCS to be safe. We should come up with a
> maintenance plan of promoting newly added operators that can benefit from
> FP16 computing into the appropriate lists, instead of shifting the burden
> away to many other developers at the moment.
>
> On Tue, May 28, 2019 at 4:13 PM Sheng Zha <zh...@apache.org> wrote:
>
> > The support for AMP should not be a burden of authors of new operators.
> > The lint analogy doesn't apply because lint is for established and
> accepted
> > coding standard at MXNet and AMP is not. AMP is an experimental feature
> > right now and it doesn't make sense to require contributors to invest in
> > it. Keeping this as error will inevitably cause comments like "CI test
> > failure seems unrelated to my change, please proceed and merge".
> >
> > -sz
> >
> > On 2019/05/28 22:51:30, Marco de Abreu <ma...@gmail.com> wrote:
> > > Hi,
> > >
> > > I'm generally in favour of these kind of tests since they make
> developers
> > > aware of changes they have to make which they would usually not be
> aware
> > > of. We have a similar test for tutorials, for example. Whenever
> somebody
> > > adds a tutorial, there's a validation that assures that all contraints
> in
> > > our testing environment are met and that they are properly tied into
> the
> > > system. This AMP test fits into the same category in my opinion and we
> > > never heard bad feedback about these kind of checks.
> > >
> > > What seems to be bothering people is the fact that the feedback time is
> > too
> > > high. Thus, I'd like to propose to move the test into the sanity-test
> > stage
> > > instead of doing it as part of the unit tests which take quite a bit of
> > > time until they're actually executed. The sanity checks run immediately
> > and
> > > give a response within about 1 minute.
> > >
> > > While I understand that this might increase the amount of work a
> > developer
> > > has to do if they develop a new operator, I think that this is the
> right
> > > thing to do. Developers won't know of every single feature other people
> > > worked on and thus might simply miss adding the support for it. This
> kind
> > > of test on the other hand makes them aware of it. If they'd like to opt
> > > out, it's one single line they would have to change and then they're
> > > totally fine. On the other hand, this might motivate them to add the
> > > support since the kernel would be the last piece and everything else
> > would
> > > already be implemented.
> > >
> > > Considering how often a PR gets declined because of linting errors, I'd
> > say
> > > that these kind of errors are WAY more frequent that AMP telling
> somebody
> > > to add their operator to a list. Considering that this would only have
> to
> > > be done once per operator, that's work of about one minute. Add that to
> > the
> > > waiting time of the sanity check and you're left with about five
> "wasted"
> > > minutes.
> > >
> > > I'm opposed towards adding a warning or treating them as float32 by
> > default
> > > since the operator author wouldn't notice. What will happen is that
> > people
> > > won't know about AMP and simply forget about low precision in general
> > until
> > > they're actively reminded. This check will remind them actively and
> thus
> > > bring more attention to the feature. I know that the feature is still
> > > experimental, but we have just started with the 1.6 branch and thus
> > there's
> > > enough time to make the experimental features production ready. Adding
> > this
> > > test early on will allow others to add the support for AMP during the
> > early
> > > stage of the 1.6 branch instead of asking them in the last few weeks
> > before
> > > a release. The result would only be that stuff is rushed or forgotten.
> > >
> > > To sum it up: I think this test is good and it should be kept as error,
> > but
> > > it should be moved to sanity checks.
> > >
> > > -Marco
> > >
> > > On Wed, May 29, 2019 at 12:21 AM Sheng Zha <zh...@apache.org>
> wrote:
> > >
> > > > Thanks for initiating the discussion.
> > > >
> > > > The premise for adding the test was to make sure that AMP feature is
> > "not
> > > > broken", but that's IMO not the right view. AMP is not supposed to
> > support
> > > > a new operator it hasn't seen before in the first place. There's no
> > way for
> > > > it to know whether the fp32 cast should happen or not. So AMP feature
> > > > cannot provide the guarantee that it works for all future operators.
> > Thus,
> > > > adding new operators to AMP list should be considered new feature
> > instead
> > > > of fixing existing feature.
> > > >
> > > > The AMP test that breaks upon the addition of new operator is thus
> > > > equivalent to forcing developers of the new operator to add the new
> > support
> > > > for AMP. This feels wrong. Especially given that AMP is an
> experimental
> > > > feature in contrib namespace (i.e. no semver guarantee), this
> practice
> > > > should be stopped immediately. We cannot force new developers to
> invest
> > > > into experimental feature this way.
> > > >
> > > > I'd suggest the following changes:
> > > > - for new operators that aren't registered in AMP, cast to float32 by
> > > > default and print one-time warning. People using AMP who want to
> avoid
> > > > casting can register it in the AMP's list.
> > > > - change the test to print warning about the operators that are not
> > listed
> > > > so that it's easy to track the problem.
> > > >
> > > > -sz
> > > >
> > > > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org>
> > wrote:
> > > > > Dear Community,
> > > > >
> > > > > One of the recently merged features of the 1.5 release, AMP
> > (Automatic
> > > > Mixed Precision) support (PR [1], design doc [5]), introduced a
> > requirement
> > > > that every new operator added to MXNet would need to be present in 1
> > of the
> > > > lists (in [2]). To make sure that this requirement is not broken when
> > > > somebody adds a new operator and does not know about AMP's
> existence, a
> > > > test was added to CI ([3]).
> > > > >
> > > > > A few people reached out to me (the original author of the feature)
> > > > saying this test increases a burden on a developer of new operators
> and
> > > > should not be an actual error, but just warning (PR for that change
> > [4]).
> > > > That is why I would like to present a motivation for it and discuss
> > with
> > > > the wider audience why I feel it was necessary.
> > > > >
> > > > > First, for people who do not know the details of what AMP is - it
> is
> > a
> > > > solution that tries to automatically apply best practices of training
> > in
> > > > lower precision (FP16) to user's FP32 model in order to fully utilize
> > > > capabilities of modern GPUs (and potentially other hardware in the
> > future).
> > > > It does so by casting to lower precision inputs to operators
> > benefitting
> > > > from it, while casting to full precision inputs of operators that are
> > > > unsafe to run in lower precision or just do not support it.
> > > > >
> > > > > The first iteration of AMP kept 2 main lists of operators -
> operators
> > > > that are beneficial and safe to do in fp16 and operators that need to
> > be
> > > > cast to FP32. The problem (raised in review of the PR [6], [8]) is
> how
> > to
> > > > make sure that the feature works as intended and is not inadvertently
> > > > broken by somebody adding a new operator. The failure scenario here
> is
> > > > adding a new operator that does not support FP16 and so should be
> cast
> > to
> > > > FP32, but AMP does not know about its existence and so does not do
> the
> > > > casting. The solution proposed in the review was to implicitly treat
> > all of
> > > > the unknown operators as FP32-only and keep the list of operators
> that
> > work
> > > > fine in both FP16 and FP32. This solution however does not really
> work,
> > > > because there are multiple operators (most notably optimizers) where
> > > > introducing additional casting of the input to FP32 would break the
> > > > operator.
> > > > >
> > > > > That is why after discussion with a few members of the community, I
> > > > decided to proceed with all lists being explicit and introducing the
> > test
> > > > that would fail when somebody added an operator without classifying
> it
> > into
> > > > 1 of the categories, and explain clearly how to do it [7]. It is not
> > ideal
> > > > solution, as it introduces some burden on the developers who are not
> > aware
> > > > about AMP, however in the typical case of adding at most a few
> > operators to
> > > > MXNet the inconvenience is I think pretty minor while important for
> the
> > > > feature correctness going forward.
> > > > >
> > > > > I would like to gather Community feedback and ideas how to handle
> > this
> > > > situation.
> > > > >
> > > > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > > > [2]
> > > >
> >
> https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > > > [3]
> > > >
> >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > > > [5]
> > > >
> >
> https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > > > [6]
> > > >
> >
> https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > > > [7]
> > > >
> >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > > > [8]
> > > >
> >
> https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > > > >
> > > >
> > >
> >
>

Re: Making new operators and AMP lists

[Jun Wu <wu...@gmail.com>]

Thanks for initiating the discussion on dev.

I understand the dilemma from designing AMP for making the feature usable
and functional as well as for not breaking other developer experience.
However, IMO, this is not about WHEN we should let other developers know
they have made a mistake by not adding newly developed ops to the AMP list,
but rather about whether we should do it in this way and thinking about its
long-term impact on development.

Asking other developers to delve into the feature they are not necessarily
aware of has imposed a too strong burden and will hinder others'
development work. FP16 is a feature nice-to-have in model training, but has
not become a standard so that every developer should be aware of. As we are
growing the community by encouraging more junior developers to contribute,
such error not caused by their work would actually hold them back from
moving forward.

IMO, AMP is not supposed cover the operators it has not seen and should
treat them as those in FP32_FUNCS to be safe. We should come up with a
maintenance plan of promoting newly added operators that can benefit from
FP16 computing into the appropriate lists, instead of shifting the burden
away to many other developers at the moment.

On Tue, May 28, 2019 at 4:13 PM Sheng Zha <zh...@apache.org> wrote:

> The support for AMP should not be a burden of authors of new operators.
> The lint analogy doesn't apply because lint is for established and accepted
> coding standard at MXNet and AMP is not. AMP is an experimental feature
> right now and it doesn't make sense to require contributors to invest in
> it. Keeping this as error will inevitably cause comments like "CI test
> failure seems unrelated to my change, please proceed and merge".
>
> -sz
>
> On 2019/05/28 22:51:30, Marco de Abreu <ma...@gmail.com> wrote:
> > Hi,
> >
> > I'm generally in favour of these kind of tests since they make developers
> > aware of changes they have to make which they would usually not be aware
> > of. We have a similar test for tutorials, for example. Whenever somebody
> > adds a tutorial, there's a validation that assures that all contraints in
> > our testing environment are met and that they are properly tied into the
> > system. This AMP test fits into the same category in my opinion and we
> > never heard bad feedback about these kind of checks.
> >
> > What seems to be bothering people is the fact that the feedback time is
> too
> > high. Thus, I'd like to propose to move the test into the sanity-test
> stage
> > instead of doing it as part of the unit tests which take quite a bit of
> > time until they're actually executed. The sanity checks run immediately
> and
> > give a response within about 1 minute.
> >
> > While I understand that this might increase the amount of work a
> developer
> > has to do if they develop a new operator, I think that this is the right
> > thing to do. Developers won't know of every single feature other people
> > worked on and thus might simply miss adding the support for it. This kind
> > of test on the other hand makes them aware of it. If they'd like to opt
> > out, it's one single line they would have to change and then they're
> > totally fine. On the other hand, this might motivate them to add the
> > support since the kernel would be the last piece and everything else
> would
> > already be implemented.
> >
> > Considering how often a PR gets declined because of linting errors, I'd
> say
> > that these kind of errors are WAY more frequent that AMP telling somebody
> > to add their operator to a list. Considering that this would only have to
> > be done once per operator, that's work of about one minute. Add that to
> the
> > waiting time of the sanity check and you're left with about five "wasted"
> > minutes.
> >
> > I'm opposed towards adding a warning or treating them as float32 by
> default
> > since the operator author wouldn't notice. What will happen is that
> people
> > won't know about AMP and simply forget about low precision in general
> until
> > they're actively reminded. This check will remind them actively and thus
> > bring more attention to the feature. I know that the feature is still
> > experimental, but we have just started with the 1.6 branch and thus
> there's
> > enough time to make the experimental features production ready. Adding
> this
> > test early on will allow others to add the support for AMP during the
> early
> > stage of the 1.6 branch instead of asking them in the last few weeks
> before
> > a release. The result would only be that stuff is rushed or forgotten.
> >
> > To sum it up: I think this test is good and it should be kept as error,
> but
> > it should be moved to sanity checks.
> >
> > -Marco
> >
> > On Wed, May 29, 2019 at 12:21 AM Sheng Zha <zh...@apache.org> wrote:
> >
> > > Thanks for initiating the discussion.
> > >
> > > The premise for adding the test was to make sure that AMP feature is
> "not
> > > broken", but that's IMO not the right view. AMP is not supposed to
> support
> > > a new operator it hasn't seen before in the first place. There's no
> way for
> > > it to know whether the fp32 cast should happen or not. So AMP feature
> > > cannot provide the guarantee that it works for all future operators.
> Thus,
> > > adding new operators to AMP list should be considered new feature
> instead
> > > of fixing existing feature.
> > >
> > > The AMP test that breaks upon the addition of new operator is thus
> > > equivalent to forcing developers of the new operator to add the new
> support
> > > for AMP. This feels wrong. Especially given that AMP is an experimental
> > > feature in contrib namespace (i.e. no semver guarantee), this practice
> > > should be stopped immediately. We cannot force new developers to invest
> > > into experimental feature this way.
> > >
> > > I'd suggest the following changes:
> > > - for new operators that aren't registered in AMP, cast to float32 by
> > > default and print one-time warning. People using AMP who want to avoid
> > > casting can register it in the AMP's list.
> > > - change the test to print warning about the operators that are not
> listed
> > > so that it's easy to track the problem.
> > >
> > > -sz
> > >
> > > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org>
> wrote:
> > > > Dear Community,
> > > >
> > > > One of the recently merged features of the 1.5 release, AMP
> (Automatic
> > > Mixed Precision) support (PR [1], design doc [5]), introduced a
> requirement
> > > that every new operator added to MXNet would need to be present in 1
> of the
> > > lists (in [2]). To make sure that this requirement is not broken when
> > > somebody adds a new operator and does not know about AMP's existence, a
> > > test was added to CI ([3]).
> > > >
> > > > A few people reached out to me (the original author of the feature)
> > > saying this test increases a burden on a developer of new operators and
> > > should not be an actual error, but just warning (PR for that change
> [4]).
> > > That is why I would like to present a motivation for it and discuss
> with
> > > the wider audience why I feel it was necessary.
> > > >
> > > > First, for people who do not know the details of what AMP is - it is
> a
> > > solution that tries to automatically apply best practices of training
> in
> > > lower precision (FP16) to user's FP32 model in order to fully utilize
> > > capabilities of modern GPUs (and potentially other hardware in the
> future).
> > > It does so by casting to lower precision inputs to operators
> benefitting
> > > from it, while casting to full precision inputs of operators that are
> > > unsafe to run in lower precision or just do not support it.
> > > >
> > > > The first iteration of AMP kept 2 main lists of operators - operators
> > > that are beneficial and safe to do in fp16 and operators that need to
> be
> > > cast to FP32. The problem (raised in review of the PR [6], [8]) is how
> to
> > > make sure that the feature works as intended and is not inadvertently
> > > broken by somebody adding a new operator. The failure scenario here is
> > > adding a new operator that does not support FP16 and so should be cast
> to
> > > FP32, but AMP does not know about its existence and so does not do the
> > > casting. The solution proposed in the review was to implicitly treat
> all of
> > > the unknown operators as FP32-only and keep the list of operators that
> work
> > > fine in both FP16 and FP32. This solution however does not really work,
> > > because there are multiple operators (most notably optimizers) where
> > > introducing additional casting of the input to FP32 would break the
> > > operator.
> > > >
> > > > That is why after discussion with a few members of the community, I
> > > decided to proceed with all lists being explicit and introducing the
> test
> > > that would fail when somebody added an operator without classifying it
> into
> > > 1 of the categories, and explain clearly how to do it [7]. It is not
> ideal
> > > solution, as it introduces some burden on the developers who are not
> aware
> > > about AMP, however in the typical case of adding at most a few
> operators to
> > > MXNet the inconvenience is I think pretty minor while important for the
> > > feature correctness going forward.
> > > >
> > > > I would like to gather Community feedback and ideas how to handle
> this
> > > situation.
> > > >
> > > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > > [2]
> > >
> https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > > [3]
> > >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > > [5]
> > >
> https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > > [6]
> > >
> https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > > [7]
> > >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > > [8]
> > >
> https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > > >
> > >
> >
>

Re: Making new operators and AMP lists

[Jun Wu <wu...@gmail.com>]

Hi Marco,

As it has been stated by others, the concern is not about how much cost it
results in, but about whether it should be there from the beginning. Just
imagine how you would explain to a MKLDNN developer who just added an
operator for CPU computing that he/she must put the operator name in AMP
list, which is used for training jobs on GPUs. The requirement imposed on
every developer responsible for adding operators in the future deviates
from the philosophy of making MXNet easy to use and develop. From this
perspective, this is a big deal.

Design-wise, I agree that making the requirement as an operator attribute
in the backend makes sense. In addition to that, we need to pay attention
to the following two conditions:
1. The attribute should be optional as FInplace, because for the most
common cases, people would just use FP32 version without worrying about
overflow/underflow. It's not as essential as attributes FInferShape,
FInferType, and FCompute that must exist to run the most common cases.
2. The attribute should be device/kernel-specific as FCompute, since CPU
implementation may be totally different than GPU.

Thanks,
Jun

On Tue, May 28, 2019 at 5:08 PM Anirudh Subramanian <an...@gmail.com>
wrote:

> The assumption is the AMP requirement is something that has a steep
> learning curve. Developers may get confused by the name, but the question
> the developer has to essentially answer is (and this can be added in the
> error):
> 1. If the operator can run in FP16 and FP32 modes, put it in
> FP16_FP32_FUNCS.
> 2. If you want to always run the op in FP16 because of the benefits, put it
> in FP16.
> 3. Not sure, Don't know or dont care: FP32 list.
>
> Any operator developer who wrote InferType logic for their operator won't
> take much time to choose one of these options.
> Now it may be possible that it took a long time before developer realizes
> that he has to do this,
> thats why I suggested we need to reduce the time it takes for him to
> realize that something was missed.
>
> Anirudh
>
> On Tue, May 28, 2019 at 4:57 PM Sheng Zha <zh...@apache.org> wrote:
>
> > This is driving people away exactly because they don't know this is
> what's
> > asked of them, and why they are asked of this AMP requirement in the
> first
> > place. To someone who's already familiar with the context, this is little
> > to be worried about. It's now the process that requires everyone to
> become
> > familiarized with the AMP requirement that becomes costly. More
> > importantly, this is not a question of whether it's too much, but whether
> > it should be there in the first place. If it's merely a question of cost
> I
> > imagine you'd have no trouble stepping up and support all of the future
> > operators for AMP :)
> >
> > -sz
> >
> > On 2019/05/28 23:49:52, Marco de Abreu <ma...@gmail.com> wrote:
> > > I'm having trouble in how far adding the name of an operator in a
> single
> > > file is too much to expect from somebody and how this is driving people
> > > away.
> > >
> > > If somebody adds a tutorial, they also have to add the tutorial to a
> > > specific file. As far as I can tell, this has not resulted in people
> not
> > > wanting to write tutorials anymore or it being considered as such a big
> > > burden.
> > >
> > > So far, I'm really not following why adding a single line to a single
> > file
> > > is considered such a big deal. Considering how long this guide [1]
> > already
> > > is, what's the harm in adding this as an additional instruction? (AMP
> is
> > > not mentioned there yet, it would be great if that could be follow up)
> > >
> > > [1]
> > >
> >
> https://mxnet.incubator.apache.org/versions/master/faq/add_op_in_backend.html
> > >
> > > -Marco
> > >
> > > On Wed, May 29, 2019 at 1:42 AM Sheng Zha <zh...@apache.org> wrote:
> > >
> > > > AMP is in contrib so there's no guarantee that the API is final.
> > Adopting
> > > > the test as-is is harmful because operator authors should not be
> > required
> > > > to invest in an experimental feature that they are not aware of.
> > > >
> > > > I'm all for openness and welcoming, but think about whether you'd
> like
> > to
> > > > turn away developers who just want to write a CPU-only operator. The
> > more
> > > > you impose on the developers the less likely they will make the
> > > > contribution through.
> > > >
> > > > Having an unfamiliar operator in AMP as a warning could let everyone
> > know
> > > > what the support state is whenever that feature is used. For those
> who
> > care
> > > > about this, they would see the warning and add the support to get the
> > speed
> > > > benefit of not casting to fp32. In this case, rather than imposing it
> > to
> > > > developers who don't know about AMP, the one who actually uses AMP
> and
> > > > cares about this feature would drive the work forward.
> > > >
> > > > -sz
> > > >
> > > > On 2019/05/28 23:25:43, Marco de Abreu <ma...@gmail.com>
> > wrote:
> > > > > While AMP might be an experimental feature, I rather would like to
> > put
> > > > the
> > > > > focus on the maturity of its interfaces. If the interfaces and the
> > > > actions
> > > > > developers have to do aren't finalized yet, I'd agree with
> disabling
> > the
> > > > > test. But if the API is final and easy to use, I don't see why
> > adopting
> > > > > early on would be harmful. But from what I can see, the output of
> the
> > > > test
> > > > > is very meaningful and explicit, easily understandable and offers
> the
> > > > > developer a clear list of action items that they can follow.
> > > > >
> > > > > If people actually start commenting "CI test failure seems
> unrelated
> > to
> > > > my
> > > > > change, please proceed and merge", we should advise them to please
> > open
> > > > the
> > > > > result tab, which will directly show the clear list of action
> items.
> > > > > Committers should support these contributors who are not that
> > familiar
> > > > with
> > > > > our CI system and explain to them how they can retrieve the reasons
> > for
> > > > the
> > > > > failure. Simply ignoring the fact that the change is not compatible
> > with
> > > > > AMP doesn't seem to be the right way.
> > > > >
> > > > > I think nobody is opposed to having AMP in MXNet. As part of
> > accepting
> > > > the
> > > > > feature, we also added AMP to the established methods including the
> > > > coding
> > > > > constraints and other checks that come with it. Lets be open and
> > welcome
> > > > to
> > > > > this new feature and come back if the turnaround time is actually
> too
> > > > high.
> > > > >
> > > > > -Marco
> > > > >
> > > > >
> > > > > On Wed, May 29, 2019 at 1:13 AM Sheng Zha <zh...@apache.org>
> > wrote:
> > > > >
> > > > > > The support for AMP should not be a burden of authors of new
> > operators.
> > > > > > The lint analogy doesn't apply because lint is for established
> and
> > > > accepted
> > > > > > coding standard at MXNet and AMP is not. AMP is an experimental
> > feature
> > > > > > right now and it doesn't make sense to require contributors to
> > invest
> > > > in
> > > > > > it. Keeping this as error will inevitably cause comments like "CI
> > test
> > > > > > failure seems unrelated to my change, please proceed and merge".
> > > > > >
> > > > > > -sz
> > > > > >
> > > > > > On 2019/05/28 22:51:30, Marco de Abreu <ma...@gmail.com>
> > > > wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > I'm generally in favour of these kind of tests since they make
> > > > developers
> > > > > > > aware of changes they have to make which they would usually not
> > be
> > > > aware
> > > > > > > of. We have a similar test for tutorials, for example. Whenever
> > > > somebody
> > > > > > > adds a tutorial, there's a validation that assures that all
> > > > contraints in
> > > > > > > our testing environment are met and that they are properly tied
> > into
> > > > the
> > > > > > > system. This AMP test fits into the same category in my opinion
> > and
> > > > we
> > > > > > > never heard bad feedback about these kind of checks.
> > > > > > >
> > > > > > > What seems to be bothering people is the fact that the feedback
> > time
> > > > is
> > > > > > too
> > > > > > > high. Thus, I'd like to propose to move the test into the
> > sanity-test
> > > > > > stage
> > > > > > > instead of doing it as part of the unit tests which take quite
> a
> > bit
> > > > of
> > > > > > > time until they're actually executed. The sanity checks run
> > > > immediately
> > > > > > and
> > > > > > > give a response within about 1 minute.
> > > > > > >
> > > > > > > While I understand that this might increase the amount of work
> a
> > > > > > developer
> > > > > > > has to do if they develop a new operator, I think that this is
> > the
> > > > right
> > > > > > > thing to do. Developers won't know of every single feature
> other
> > > > people
> > > > > > > worked on and thus might simply miss adding the support for it.
> > This
> > > > kind
> > > > > > > of test on the other hand makes them aware of it. If they'd
> like
> > to
> > > > opt
> > > > > > > out, it's one single line they would have to change and then
> > they're
> > > > > > > totally fine. On the other hand, this might motivate them to
> add
> > the
> > > > > > > support since the kernel would be the last piece and everything
> > else
> > > > > > would
> > > > > > > already be implemented.
> > > > > > >
> > > > > > > Considering how often a PR gets declined because of linting
> > errors,
> > > > I'd
> > > > > > say
> > > > > > > that these kind of errors are WAY more frequent that AMP
> telling
> > > > somebody
> > > > > > > to add their operator to a list. Considering that this would
> only
> > > > have to
> > > > > > > be done once per operator, that's work of about one minute. Add
> > that
> > > > to
> > > > > > the
> > > > > > > waiting time of the sanity check and you're left with about
> five
> > > > "wasted"
> > > > > > > minutes.
> > > > > > >
> > > > > > > I'm opposed towards adding a warning or treating them as
> float32
> > by
> > > > > > default
> > > > > > > since the operator author wouldn't notice. What will happen is
> > that
> > > > > > people
> > > > > > > won't know about AMP and simply forget about low precision in
> > general
> > > > > > until
> > > > > > > they're actively reminded. This check will remind them actively
> > and
> > > > thus
> > > > > > > bring more attention to the feature. I know that the feature is
> > still
> > > > > > > experimental, but we have just started with the 1.6 branch and
> > thus
> > > > > > there's
> > > > > > > enough time to make the experimental features production ready.
> > > > Adding
> > > > > > this
> > > > > > > test early on will allow others to add the support for AMP
> > during the
> > > > > > early
> > > > > > > stage of the 1.6 branch instead of asking them in the last few
> > weeks
> > > > > > before
> > > > > > > a release. The result would only be that stuff is rushed or
> > > > forgotten.
> > > > > > >
> > > > > > > To sum it up: I think this test is good and it should be kept
> as
> > > > error,
> > > > > > but
> > > > > > > it should be moved to sanity checks.
> > > > > > >
> > > > > > > -Marco
> > > > > > >
> > > > > > > On Wed, May 29, 2019 at 12:21 AM Sheng Zha <
> zhasheng@apache.org>
> > > > wrote:
> > > > > > >
> > > > > > > > Thanks for initiating the discussion.
> > > > > > > >
> > > > > > > > The premise for adding the test was to make sure that AMP
> > feature
> > > > is
> > > > > > "not
> > > > > > > > broken", but that's IMO not the right view. AMP is not
> > supposed to
> > > > > > support
> > > > > > > > a new operator it hasn't seen before in the first place.
> > There's no
> > > > > > way for
> > > > > > > > it to know whether the fp32 cast should happen or not. So AMP
> > > > feature
> > > > > > > > cannot provide the guarantee that it works for all future
> > > > operators.
> > > > > > Thus,
> > > > > > > > adding new operators to AMP list should be considered new
> > feature
> > > > > > instead
> > > > > > > > of fixing existing feature.
> > > > > > > >
> > > > > > > > The AMP test that breaks upon the addition of new operator is
> > thus
> > > > > > > > equivalent to forcing developers of the new operator to add
> > the new
> > > > > > support
> > > > > > > > for AMP. This feels wrong. Especially given that AMP is an
> > > > experimental
> > > > > > > > feature in contrib namespace (i.e. no semver guarantee), this
> > > > practice
> > > > > > > > should be stopped immediately. We cannot force new developers
> > to
> > > > invest
> > > > > > > > into experimental feature this way.
> > > > > > > >
> > > > > > > > I'd suggest the following changes:
> > > > > > > > - for new operators that aren't registered in AMP, cast to
> > float32
> > > > by
> > > > > > > > default and print one-time warning. People using AMP who want
> > to
> > > > avoid
> > > > > > > > casting can register it in the AMP's list.
> > > > > > > > - change the test to print warning about the operators that
> > are not
> > > > > > listed
> > > > > > > > so that it's easy to track the problem.
> > > > > > > >
> > > > > > > > -sz
> > > > > > > >
> > > > > > > > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <
> > ptrendx@apache.org>
> > > > > > wrote:
> > > > > > > > > Dear Community,
> > > > > > > > >
> > > > > > > > > One of the recently merged features of the 1.5 release, AMP
> > > > > > (Automatic
> > > > > > > > Mixed Precision) support (PR [1], design doc [5]),
> introduced a
> > > > > > requirement
> > > > > > > > that every new operator added to MXNet would need to be
> > present in
> > > > 1
> > > > > > of the
> > > > > > > > lists (in [2]). To make sure that this requirement is not
> > broken
> > > > when
> > > > > > > > somebody adds a new operator and does not know about AMP's
> > > > existence, a
> > > > > > > > test was added to CI ([3]).
> > > > > > > > >
> > > > > > > > > A few people reached out to me (the original author of the
> > > > feature)
> > > > > > > > saying this test increases a burden on a developer of new
> > > > operators and
> > > > > > > > should not be an actual error, but just warning (PR for that
> > change
> > > > > > [4]).
> > > > > > > > That is why I would like to present a motivation for it and
> > discuss
> > > > > > with
> > > > > > > > the wider audience why I feel it was necessary.
> > > > > > > > >
> > > > > > > > > First, for people who do not know the details of what AMP
> is
> > -
> > > > it is
> > > > > > a
> > > > > > > > solution that tries to automatically apply best practices of
> > > > training
> > > > > > in
> > > > > > > > lower precision (FP16) to user's FP32 model in order to fully
> > > > utilize
> > > > > > > > capabilities of modern GPUs (and potentially other hardware
> in
> > the
> > > > > > future).
> > > > > > > > It does so by casting to lower precision inputs to operators
> > > > > > benefitting
> > > > > > > > from it, while casting to full precision inputs of operators
> > that
> > > > are
> > > > > > > > unsafe to run in lower precision or just do not support it.
> > > > > > > > >
> > > > > > > > > The first iteration of AMP kept 2 main lists of operators -
> > > > operators
> > > > > > > > that are beneficial and safe to do in fp16 and operators that
> > need
> > > > to
> > > > > > be
> > > > > > > > cast to FP32. The problem (raised in review of the PR [6],
> > [8]) is
> > > > how
> > > > > > to
> > > > > > > > make sure that the feature works as intended and is not
> > > > inadvertently
> > > > > > > > broken by somebody adding a new operator. The failure
> scenario
> > > > here is
> > > > > > > > adding a new operator that does not support FP16 and so
> should
> > be
> > > > cast
> > > > > > to
> > > > > > > > FP32, but AMP does not know about its existence and so does
> > not do
> > > > the
> > > > > > > > casting. The solution proposed in the review was to
> implicitly
> > > > treat
> > > > > > all of
> > > > > > > > the unknown operators as FP32-only and keep the list of
> > operators
> > > > that
> > > > > > work
> > > > > > > > fine in both FP16 and FP32. This solution however does not
> > really
> > > > work,
> > > > > > > > because there are multiple operators (most notably
> optimizers)
> > > > where
> > > > > > > > introducing additional casting of the input to FP32 would
> > break the
> > > > > > > > operator.
> > > > > > > > >
> > > > > > > > > That is why after discussion with a few members of the
> > > > community, I
> > > > > > > > decided to proceed with all lists being explicit and
> > introducing
> > > > the
> > > > > > test
> > > > > > > > that would fail when somebody added an operator without
> > > > classifying it
> > > > > > into
> > > > > > > > 1 of the categories, and explain clearly how to do it [7]. It
> > is
> > > > not
> > > > > > ideal
> > > > > > > > solution, as it introduces some burden on the developers who
> > are
> > > > not
> > > > > > aware
> > > > > > > > about AMP, however in the typical case of adding at most a
> few
> > > > > > operators to
> > > > > > > > MXNet the inconvenience is I think pretty minor while
> important
> > > > for the
> > > > > > > > feature correctness going forward.
> > > > > > > > >
> > > > > > > > > I would like to gather Community feedback and ideas how to
> > handle
> > > > > > this
> > > > > > > > situation.
> > > > > > > > >
> > > > > > > > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > > > > > > > [2]
> > > > > > > >
> > > > > >
> > > >
> >
> https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > > > > > > > [3]
> > > > > > > >
> > > > > >
> > > >
> >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > > > > > > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > > > > > > > [5]
> > > > > > > >
> > > > > >
> > > >
> >
> https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > > > > > > > [6]
> > > > > > > >
> > > > > >
> > > >
> >
> https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > > > > > > > [7]
> > > > > > > >
> > > > > >
> > > >
> >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > > > > > > > [8]
> > > > > > > >
> > > > > >
> > > >
> >
> https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Making new operators and AMP lists

[Anirudh Subramanian <an...@gmail.com>]

The assumption is the AMP requirement is something that has a steep
learning curve. Developers may get confused by the name, but the question
the developer has to essentially answer is (and this can be added in the
error):
1. If the operator can run in FP16 and FP32 modes, put it in
FP16_FP32_FUNCS.
2. If you want to always run the op in FP16 because of the benefits, put it
in FP16.
3. Not sure, Don't know or dont care: FP32 list.

Any operator developer who wrote InferType logic for their operator won't
take much time to choose one of these options.
Now it may be possible that it took a long time before developer realizes
that he has to do this,
thats why I suggested we need to reduce the time it takes for him to
realize that something was missed.

Anirudh

On Tue, May 28, 2019 at 4:57 PM Sheng Zha <zh...@apache.org> wrote:

> This is driving people away exactly because they don't know this is what's
> asked of them, and why they are asked of this AMP requirement in the first
> place. To someone who's already familiar with the context, this is little
> to be worried about. It's now the process that requires everyone to become
> familiarized with the AMP requirement that becomes costly. More
> importantly, this is not a question of whether it's too much, but whether
> it should be there in the first place. If it's merely a question of cost I
> imagine you'd have no trouble stepping up and support all of the future
> operators for AMP :)
>
> -sz
>
> On 2019/05/28 23:49:52, Marco de Abreu <ma...@gmail.com> wrote:
> > I'm having trouble in how far adding the name of an operator in a single
> > file is too much to expect from somebody and how this is driving people
> > away.
> >
> > If somebody adds a tutorial, they also have to add the tutorial to a
> > specific file. As far as I can tell, this has not resulted in people not
> > wanting to write tutorials anymore or it being considered as such a big
> > burden.
> >
> > So far, I'm really not following why adding a single line to a single
> file
> > is considered such a big deal. Considering how long this guide [1]
> already
> > is, what's the harm in adding this as an additional instruction? (AMP is
> > not mentioned there yet, it would be great if that could be follow up)
> >
> > [1]
> >
> https://mxnet.incubator.apache.org/versions/master/faq/add_op_in_backend.html
> >
> > -Marco
> >
> > On Wed, May 29, 2019 at 1:42 AM Sheng Zha <zh...@apache.org> wrote:
> >
> > > AMP is in contrib so there's no guarantee that the API is final.
> Adopting
> > > the test as-is is harmful because operator authors should not be
> required
> > > to invest in an experimental feature that they are not aware of.
> > >
> > > I'm all for openness and welcoming, but think about whether you'd like
> to
> > > turn away developers who just want to write a CPU-only operator. The
> more
> > > you impose on the developers the less likely they will make the
> > > contribution through.
> > >
> > > Having an unfamiliar operator in AMP as a warning could let everyone
> know
> > > what the support state is whenever that feature is used. For those who
> care
> > > about this, they would see the warning and add the support to get the
> speed
> > > benefit of not casting to fp32. In this case, rather than imposing it
> to
> > > developers who don't know about AMP, the one who actually uses AMP and
> > > cares about this feature would drive the work forward.
> > >
> > > -sz
> > >
> > > On 2019/05/28 23:25:43, Marco de Abreu <ma...@gmail.com>
> wrote:
> > > > While AMP might be an experimental feature, I rather would like to
> put
> > > the
> > > > focus on the maturity of its interfaces. If the interfaces and the
> > > actions
> > > > developers have to do aren't finalized yet, I'd agree with disabling
> the
> > > > test. But if the API is final and easy to use, I don't see why
> adopting
> > > > early on would be harmful. But from what I can see, the output of the
> > > test
> > > > is very meaningful and explicit, easily understandable and offers the
> > > > developer a clear list of action items that they can follow.
> > > >
> > > > If people actually start commenting "CI test failure seems unrelated
> to
> > > my
> > > > change, please proceed and merge", we should advise them to please
> open
> > > the
> > > > result tab, which will directly show the clear list of action items.
> > > > Committers should support these contributors who are not that
> familiar
> > > with
> > > > our CI system and explain to them how they can retrieve the reasons
> for
> > > the
> > > > failure. Simply ignoring the fact that the change is not compatible
> with
> > > > AMP doesn't seem to be the right way.
> > > >
> > > > I think nobody is opposed to having AMP in MXNet. As part of
> accepting
> > > the
> > > > feature, we also added AMP to the established methods including the
> > > coding
> > > > constraints and other checks that come with it. Lets be open and
> welcome
> > > to
> > > > this new feature and come back if the turnaround time is actually too
> > > high.
> > > >
> > > > -Marco
> > > >
> > > >
> > > > On Wed, May 29, 2019 at 1:13 AM Sheng Zha <zh...@apache.org>
> wrote:
> > > >
> > > > > The support for AMP should not be a burden of authors of new
> operators.
> > > > > The lint analogy doesn't apply because lint is for established and
> > > accepted
> > > > > coding standard at MXNet and AMP is not. AMP is an experimental
> feature
> > > > > right now and it doesn't make sense to require contributors to
> invest
> > > in
> > > > > it. Keeping this as error will inevitably cause comments like "CI
> test
> > > > > failure seems unrelated to my change, please proceed and merge".
> > > > >
> > > > > -sz
> > > > >
> > > > > On 2019/05/28 22:51:30, Marco de Abreu <ma...@gmail.com>
> > > wrote:
> > > > > > Hi,
> > > > > >
> > > > > > I'm generally in favour of these kind of tests since they make
> > > developers
> > > > > > aware of changes they have to make which they would usually not
> be
> > > aware
> > > > > > of. We have a similar test for tutorials, for example. Whenever
> > > somebody
> > > > > > adds a tutorial, there's a validation that assures that all
> > > contraints in
> > > > > > our testing environment are met and that they are properly tied
> into
> > > the
> > > > > > system. This AMP test fits into the same category in my opinion
> and
> > > we
> > > > > > never heard bad feedback about these kind of checks.
> > > > > >
> > > > > > What seems to be bothering people is the fact that the feedback
> time
> > > is
> > > > > too
> > > > > > high. Thus, I'd like to propose to move the test into the
> sanity-test
> > > > > stage
> > > > > > instead of doing it as part of the unit tests which take quite a
> bit
> > > of
> > > > > > time until they're actually executed. The sanity checks run
> > > immediately
> > > > > and
> > > > > > give a response within about 1 minute.
> > > > > >
> > > > > > While I understand that this might increase the amount of work a
> > > > > developer
> > > > > > has to do if they develop a new operator, I think that this is
> the
> > > right
> > > > > > thing to do. Developers won't know of every single feature other
> > > people
> > > > > > worked on and thus might simply miss adding the support for it.
> This
> > > kind
> > > > > > of test on the other hand makes them aware of it. If they'd like
> to
> > > opt
> > > > > > out, it's one single line they would have to change and then
> they're
> > > > > > totally fine. On the other hand, this might motivate them to add
> the
> > > > > > support since the kernel would be the last piece and everything
> else
> > > > > would
> > > > > > already be implemented.
> > > > > >
> > > > > > Considering how often a PR gets declined because of linting
> errors,
> > > I'd
> > > > > say
> > > > > > that these kind of errors are WAY more frequent that AMP telling
> > > somebody
> > > > > > to add their operator to a list. Considering that this would only
> > > have to
> > > > > > be done once per operator, that's work of about one minute. Add
> that
> > > to
> > > > > the
> > > > > > waiting time of the sanity check and you're left with about five
> > > "wasted"
> > > > > > minutes.
> > > > > >
> > > > > > I'm opposed towards adding a warning or treating them as float32
> by
> > > > > default
> > > > > > since the operator author wouldn't notice. What will happen is
> that
> > > > > people
> > > > > > won't know about AMP and simply forget about low precision in
> general
> > > > > until
> > > > > > they're actively reminded. This check will remind them actively
> and
> > > thus
> > > > > > bring more attention to the feature. I know that the feature is
> still
> > > > > > experimental, but we have just started with the 1.6 branch and
> thus
> > > > > there's
> > > > > > enough time to make the experimental features production ready.
> > > Adding
> > > > > this
> > > > > > test early on will allow others to add the support for AMP
> during the
> > > > > early
> > > > > > stage of the 1.6 branch instead of asking them in the last few
> weeks
> > > > > before
> > > > > > a release. The result would only be that stuff is rushed or
> > > forgotten.
> > > > > >
> > > > > > To sum it up: I think this test is good and it should be kept as
> > > error,
> > > > > but
> > > > > > it should be moved to sanity checks.
> > > > > >
> > > > > > -Marco
> > > > > >
> > > > > > On Wed, May 29, 2019 at 12:21 AM Sheng Zha <zh...@apache.org>
> > > wrote:
> > > > > >
> > > > > > > Thanks for initiating the discussion.
> > > > > > >
> > > > > > > The premise for adding the test was to make sure that AMP
> feature
> > > is
> > > > > "not
> > > > > > > broken", but that's IMO not the right view. AMP is not
> supposed to
> > > > > support
> > > > > > > a new operator it hasn't seen before in the first place.
> There's no
> > > > > way for
> > > > > > > it to know whether the fp32 cast should happen or not. So AMP
> > > feature
> > > > > > > cannot provide the guarantee that it works for all future
> > > operators.
> > > > > Thus,
> > > > > > > adding new operators to AMP list should be considered new
> feature
> > > > > instead
> > > > > > > of fixing existing feature.
> > > > > > >
> > > > > > > The AMP test that breaks upon the addition of new operator is
> thus
> > > > > > > equivalent to forcing developers of the new operator to add
> the new
> > > > > support
> > > > > > > for AMP. This feels wrong. Especially given that AMP is an
> > > experimental
> > > > > > > feature in contrib namespace (i.e. no semver guarantee), this
> > > practice
> > > > > > > should be stopped immediately. We cannot force new developers
> to
> > > invest
> > > > > > > into experimental feature this way.
> > > > > > >
> > > > > > > I'd suggest the following changes:
> > > > > > > - for new operators that aren't registered in AMP, cast to
> float32
> > > by
> > > > > > > default and print one-time warning. People using AMP who want
> to
> > > avoid
> > > > > > > casting can register it in the AMP's list.
> > > > > > > - change the test to print warning about the operators that
> are not
> > > > > listed
> > > > > > > so that it's easy to track the problem.
> > > > > > >
> > > > > > > -sz
> > > > > > >
> > > > > > > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <
> ptrendx@apache.org>
> > > > > wrote:
> > > > > > > > Dear Community,
> > > > > > > >
> > > > > > > > One of the recently merged features of the 1.5 release, AMP
> > > > > (Automatic
> > > > > > > Mixed Precision) support (PR [1], design doc [5]), introduced a
> > > > > requirement
> > > > > > > that every new operator added to MXNet would need to be
> present in
> > > 1
> > > > > of the
> > > > > > > lists (in [2]). To make sure that this requirement is not
> broken
> > > when
> > > > > > > somebody adds a new operator and does not know about AMP's
> > > existence, a
> > > > > > > test was added to CI ([3]).
> > > > > > > >
> > > > > > > > A few people reached out to me (the original author of the
> > > feature)
> > > > > > > saying this test increases a burden on a developer of new
> > > operators and
> > > > > > > should not be an actual error, but just warning (PR for that
> change
> > > > > [4]).
> > > > > > > That is why I would like to present a motivation for it and
> discuss
> > > > > with
> > > > > > > the wider audience why I feel it was necessary.
> > > > > > > >
> > > > > > > > First, for people who do not know the details of what AMP is
> -
> > > it is
> > > > > a
> > > > > > > solution that tries to automatically apply best practices of
> > > training
> > > > > in
> > > > > > > lower precision (FP16) to user's FP32 model in order to fully
> > > utilize
> > > > > > > capabilities of modern GPUs (and potentially other hardware in
> the
> > > > > future).
> > > > > > > It does so by casting to lower precision inputs to operators
> > > > > benefitting
> > > > > > > from it, while casting to full precision inputs of operators
> that
> > > are
> > > > > > > unsafe to run in lower precision or just do not support it.
> > > > > > > >
> > > > > > > > The first iteration of AMP kept 2 main lists of operators -
> > > operators
> > > > > > > that are beneficial and safe to do in fp16 and operators that
> need
> > > to
> > > > > be
> > > > > > > cast to FP32. The problem (raised in review of the PR [6],
> [8]) is
> > > how
> > > > > to
> > > > > > > make sure that the feature works as intended and is not
> > > inadvertently
> > > > > > > broken by somebody adding a new operator. The failure scenario
> > > here is
> > > > > > > adding a new operator that does not support FP16 and so should
> be
> > > cast
> > > > > to
> > > > > > > FP32, but AMP does not know about its existence and so does
> not do
> > > the
> > > > > > > casting. The solution proposed in the review was to implicitly
> > > treat
> > > > > all of
> > > > > > > the unknown operators as FP32-only and keep the list of
> operators
> > > that
> > > > > work
> > > > > > > fine in both FP16 and FP32. This solution however does not
> really
> > > work,
> > > > > > > because there are multiple operators (most notably optimizers)
> > > where
> > > > > > > introducing additional casting of the input to FP32 would
> break the
> > > > > > > operator.
> > > > > > > >
> > > > > > > > That is why after discussion with a few members of the
> > > community, I
> > > > > > > decided to proceed with all lists being explicit and
> introducing
> > > the
> > > > > test
> > > > > > > that would fail when somebody added an operator without
> > > classifying it
> > > > > into
> > > > > > > 1 of the categories, and explain clearly how to do it [7]. It
> is
> > > not
> > > > > ideal
> > > > > > > solution, as it introduces some burden on the developers who
> are
> > > not
> > > > > aware
> > > > > > > about AMP, however in the typical case of adding at most a few
> > > > > operators to
> > > > > > > MXNet the inconvenience is I think pretty minor while important
> > > for the
> > > > > > > feature correctness going forward.
> > > > > > > >
> > > > > > > > I would like to gather Community feedback and ideas how to
> handle
> > > > > this
> > > > > > > situation.
> > > > > > > >
> > > > > > > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > > > > > > [2]
> > > > > > >
> > > > >
> > >
> https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > > > > > > [3]
> > > > > > >
> > > > >
> > >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > > > > > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > > > > > > [5]
> > > > > > >
> > > > >
> > >
> https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > > > > > > [6]
> > > > > > >
> > > > >
> > >
> https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > > > > > > [7]
> > > > > > >
> > > > >
> > >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > > > > > > [8]
> > > > > > >
> > > > >
> > >
> https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Making new operators and AMP lists

[Sheng Zha <zh...@apache.org>]

This is driving people away exactly because they don't know this is what's asked of them, and why they are asked of this AMP requirement in the first place. To someone who's already familiar with the context, this is little to be worried about. It's now the process that requires everyone to become familiarized with the AMP requirement that becomes costly. More importantly, this is not a question of whether it's too much, but whether it should be there in the first place. If it's merely a question of cost I imagine you'd have no trouble stepping up and support all of the future operators for AMP :)

-sz

On 2019/05/28 23:49:52, Marco de Abreu <ma...@gmail.com> wrote: 
> I'm having trouble in how far adding the name of an operator in a single
> file is too much to expect from somebody and how this is driving people
> away.
> 
> If somebody adds a tutorial, they also have to add the tutorial to a
> specific file. As far as I can tell, this has not resulted in people not
> wanting to write tutorials anymore or it being considered as such a big
> burden.
> 
> So far, I'm really not following why adding a single line to a single file
> is considered such a big deal. Considering how long this guide [1] already
> is, what's the harm in adding this as an additional instruction? (AMP is
> not mentioned there yet, it would be great if that could be follow up)
> 
> [1]
> https://mxnet.incubator.apache.org/versions/master/faq/add_op_in_backend.html
> 
> -Marco
> 
> On Wed, May 29, 2019 at 1:42 AM Sheng Zha <zh...@apache.org> wrote:
> 
> > AMP is in contrib so there's no guarantee that the API is final. Adopting
> > the test as-is is harmful because operator authors should not be required
> > to invest in an experimental feature that they are not aware of.
> >
> > I'm all for openness and welcoming, but think about whether you'd like to
> > turn away developers who just want to write a CPU-only operator. The more
> > you impose on the developers the less likely they will make the
> > contribution through.
> >
> > Having an unfamiliar operator in AMP as a warning could let everyone know
> > what the support state is whenever that feature is used. For those who care
> > about this, they would see the warning and add the support to get the speed
> > benefit of not casting to fp32. In this case, rather than imposing it to
> > developers who don't know about AMP, the one who actually uses AMP and
> > cares about this feature would drive the work forward.
> >
> > -sz
> >
> > On 2019/05/28 23:25:43, Marco de Abreu <ma...@gmail.com> wrote:
> > > While AMP might be an experimental feature, I rather would like to put
> > the
> > > focus on the maturity of its interfaces. If the interfaces and the
> > actions
> > > developers have to do aren't finalized yet, I'd agree with disabling the
> > > test. But if the API is final and easy to use, I don't see why adopting
> > > early on would be harmful. But from what I can see, the output of the
> > test
> > > is very meaningful and explicit, easily understandable and offers the
> > > developer a clear list of action items that they can follow.
> > >
> > > If people actually start commenting "CI test failure seems unrelated to
> > my
> > > change, please proceed and merge", we should advise them to please open
> > the
> > > result tab, which will directly show the clear list of action items.
> > > Committers should support these contributors who are not that familiar
> > with
> > > our CI system and explain to them how they can retrieve the reasons for
> > the
> > > failure. Simply ignoring the fact that the change is not compatible with
> > > AMP doesn't seem to be the right way.
> > >
> > > I think nobody is opposed to having AMP in MXNet. As part of accepting
> > the
> > > feature, we also added AMP to the established methods including the
> > coding
> > > constraints and other checks that come with it. Lets be open and welcome
> > to
> > > this new feature and come back if the turnaround time is actually too
> > high.
> > >
> > > -Marco
> > >
> > >
> > > On Wed, May 29, 2019 at 1:13 AM Sheng Zha <zh...@apache.org> wrote:
> > >
> > > > The support for AMP should not be a burden of authors of new operators.
> > > > The lint analogy doesn't apply because lint is for established and
> > accepted
> > > > coding standard at MXNet and AMP is not. AMP is an experimental feature
> > > > right now and it doesn't make sense to require contributors to invest
> > in
> > > > it. Keeping this as error will inevitably cause comments like "CI test
> > > > failure seems unrelated to my change, please proceed and merge".
> > > >
> > > > -sz
> > > >
> > > > On 2019/05/28 22:51:30, Marco de Abreu <ma...@gmail.com>
> > wrote:
> > > > > Hi,
> > > > >
> > > > > I'm generally in favour of these kind of tests since they make
> > developers
> > > > > aware of changes they have to make which they would usually not be
> > aware
> > > > > of. We have a similar test for tutorials, for example. Whenever
> > somebody
> > > > > adds a tutorial, there's a validation that assures that all
> > contraints in
> > > > > our testing environment are met and that they are properly tied into
> > the
> > > > > system. This AMP test fits into the same category in my opinion and
> > we
> > > > > never heard bad feedback about these kind of checks.
> > > > >
> > > > > What seems to be bothering people is the fact that the feedback time
> > is
> > > > too
> > > > > high. Thus, I'd like to propose to move the test into the sanity-test
> > > > stage
> > > > > instead of doing it as part of the unit tests which take quite a bit
> > of
> > > > > time until they're actually executed. The sanity checks run
> > immediately
> > > > and
> > > > > give a response within about 1 minute.
> > > > >
> > > > > While I understand that this might increase the amount of work a
> > > > developer
> > > > > has to do if they develop a new operator, I think that this is the
> > right
> > > > > thing to do. Developers won't know of every single feature other
> > people
> > > > > worked on and thus might simply miss adding the support for it. This
> > kind
> > > > > of test on the other hand makes them aware of it. If they'd like to
> > opt
> > > > > out, it's one single line they would have to change and then they're
> > > > > totally fine. On the other hand, this might motivate them to add the
> > > > > support since the kernel would be the last piece and everything else
> > > > would
> > > > > already be implemented.
> > > > >
> > > > > Considering how often a PR gets declined because of linting errors,
> > I'd
> > > > say
> > > > > that these kind of errors are WAY more frequent that AMP telling
> > somebody
> > > > > to add their operator to a list. Considering that this would only
> > have to
> > > > > be done once per operator, that's work of about one minute. Add that
> > to
> > > > the
> > > > > waiting time of the sanity check and you're left with about five
> > "wasted"
> > > > > minutes.
> > > > >
> > > > > I'm opposed towards adding a warning or treating them as float32 by
> > > > default
> > > > > since the operator author wouldn't notice. What will happen is that
> > > > people
> > > > > won't know about AMP and simply forget about low precision in general
> > > > until
> > > > > they're actively reminded. This check will remind them actively and
> > thus
> > > > > bring more attention to the feature. I know that the feature is still
> > > > > experimental, but we have just started with the 1.6 branch and thus
> > > > there's
> > > > > enough time to make the experimental features production ready.
> > Adding
> > > > this
> > > > > test early on will allow others to add the support for AMP during the
> > > > early
> > > > > stage of the 1.6 branch instead of asking them in the last few weeks
> > > > before
> > > > > a release. The result would only be that stuff is rushed or
> > forgotten.
> > > > >
> > > > > To sum it up: I think this test is good and it should be kept as
> > error,
> > > > but
> > > > > it should be moved to sanity checks.
> > > > >
> > > > > -Marco
> > > > >
> > > > > On Wed, May 29, 2019 at 12:21 AM Sheng Zha <zh...@apache.org>
> > wrote:
> > > > >
> > > > > > Thanks for initiating the discussion.
> > > > > >
> > > > > > The premise for adding the test was to make sure that AMP feature
> > is
> > > > "not
> > > > > > broken", but that's IMO not the right view. AMP is not supposed to
> > > > support
> > > > > > a new operator it hasn't seen before in the first place. There's no
> > > > way for
> > > > > > it to know whether the fp32 cast should happen or not. So AMP
> > feature
> > > > > > cannot provide the guarantee that it works for all future
> > operators.
> > > > Thus,
> > > > > > adding new operators to AMP list should be considered new feature
> > > > instead
> > > > > > of fixing existing feature.
> > > > > >
> > > > > > The AMP test that breaks upon the addition of new operator is thus
> > > > > > equivalent to forcing developers of the new operator to add the new
> > > > support
> > > > > > for AMP. This feels wrong. Especially given that AMP is an
> > experimental
> > > > > > feature in contrib namespace (i.e. no semver guarantee), this
> > practice
> > > > > > should be stopped immediately. We cannot force new developers to
> > invest
> > > > > > into experimental feature this way.
> > > > > >
> > > > > > I'd suggest the following changes:
> > > > > > - for new operators that aren't registered in AMP, cast to float32
> > by
> > > > > > default and print one-time warning. People using AMP who want to
> > avoid
> > > > > > casting can register it in the AMP's list.
> > > > > > - change the test to print warning about the operators that are not
> > > > listed
> > > > > > so that it's easy to track the problem.
> > > > > >
> > > > > > -sz
> > > > > >
> > > > > > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org>
> > > > wrote:
> > > > > > > Dear Community,
> > > > > > >
> > > > > > > One of the recently merged features of the 1.5 release, AMP
> > > > (Automatic
> > > > > > Mixed Precision) support (PR [1], design doc [5]), introduced a
> > > > requirement
> > > > > > that every new operator added to MXNet would need to be present in
> > 1
> > > > of the
> > > > > > lists (in [2]). To make sure that this requirement is not broken
> > when
> > > > > > somebody adds a new operator and does not know about AMP's
> > existence, a
> > > > > > test was added to CI ([3]).
> > > > > > >
> > > > > > > A few people reached out to me (the original author of the
> > feature)
> > > > > > saying this test increases a burden on a developer of new
> > operators and
> > > > > > should not be an actual error, but just warning (PR for that change
> > > > [4]).
> > > > > > That is why I would like to present a motivation for it and discuss
> > > > with
> > > > > > the wider audience why I feel it was necessary.
> > > > > > >
> > > > > > > First, for people who do not know the details of what AMP is -
> > it is
> > > > a
> > > > > > solution that tries to automatically apply best practices of
> > training
> > > > in
> > > > > > lower precision (FP16) to user's FP32 model in order to fully
> > utilize
> > > > > > capabilities of modern GPUs (and potentially other hardware in the
> > > > future).
> > > > > > It does so by casting to lower precision inputs to operators
> > > > benefitting
> > > > > > from it, while casting to full precision inputs of operators that
> > are
> > > > > > unsafe to run in lower precision or just do not support it.
> > > > > > >
> > > > > > > The first iteration of AMP kept 2 main lists of operators -
> > operators
> > > > > > that are beneficial and safe to do in fp16 and operators that need
> > to
> > > > be
> > > > > > cast to FP32. The problem (raised in review of the PR [6], [8]) is
> > how
> > > > to
> > > > > > make sure that the feature works as intended and is not
> > inadvertently
> > > > > > broken by somebody adding a new operator. The failure scenario
> > here is
> > > > > > adding a new operator that does not support FP16 and so should be
> > cast
> > > > to
> > > > > > FP32, but AMP does not know about its existence and so does not do
> > the
> > > > > > casting. The solution proposed in the review was to implicitly
> > treat
> > > > all of
> > > > > > the unknown operators as FP32-only and keep the list of operators
> > that
> > > > work
> > > > > > fine in both FP16 and FP32. This solution however does not really
> > work,
> > > > > > because there are multiple operators (most notably optimizers)
> > where
> > > > > > introducing additional casting of the input to FP32 would break the
> > > > > > operator.
> > > > > > >
> > > > > > > That is why after discussion with a few members of the
> > community, I
> > > > > > decided to proceed with all lists being explicit and introducing
> > the
> > > > test
> > > > > > that would fail when somebody added an operator without
> > classifying it
> > > > into
> > > > > > 1 of the categories, and explain clearly how to do it [7]. It is
> > not
> > > > ideal
> > > > > > solution, as it introduces some burden on the developers who are
> > not
> > > > aware
> > > > > > about AMP, however in the typical case of adding at most a few
> > > > operators to
> > > > > > MXNet the inconvenience is I think pretty minor while important
> > for the
> > > > > > feature correctness going forward.
> > > > > > >
> > > > > > > I would like to gather Community feedback and ideas how to handle
> > > > this
> > > > > > situation.
> > > > > > >
> > > > > > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > > > > > [2]
> > > > > >
> > > >
> > https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > > > > > [3]
> > > > > >
> > > >
> > https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > > > > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > > > > > [5]
> > > > > >
> > > >
> > https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > > > > > [6]
> > > > > >
> > > >
> > https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > > > > > [7]
> > > > > >
> > > >
> > https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > > > > > [8]
> > > > > >
> > > >
> > https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> 

Re: Making new operators and AMP lists

[Marco de Abreu <ma...@gmail.com>]

I'm having trouble in how far adding the name of an operator in a single
file is too much to expect from somebody and how this is driving people
away.

If somebody adds a tutorial, they also have to add the tutorial to a
specific file. As far as I can tell, this has not resulted in people not
wanting to write tutorials anymore or it being considered as such a big
burden.

So far, I'm really not following why adding a single line to a single file
is considered such a big deal. Considering how long this guide [1] already
is, what's the harm in adding this as an additional instruction? (AMP is
not mentioned there yet, it would be great if that could be follow up)

[1]
https://mxnet.incubator.apache.org/versions/master/faq/add_op_in_backend.html

-Marco

On Wed, May 29, 2019 at 1:42 AM Sheng Zha <zh...@apache.org> wrote:

> AMP is in contrib so there's no guarantee that the API is final. Adopting
> the test as-is is harmful because operator authors should not be required
> to invest in an experimental feature that they are not aware of.
>
> I'm all for openness and welcoming, but think about whether you'd like to
> turn away developers who just want to write a CPU-only operator. The more
> you impose on the developers the less likely they will make the
> contribution through.
>
> Having an unfamiliar operator in AMP as a warning could let everyone know
> what the support state is whenever that feature is used. For those who care
> about this, they would see the warning and add the support to get the speed
> benefit of not casting to fp32. In this case, rather than imposing it to
> developers who don't know about AMP, the one who actually uses AMP and
> cares about this feature would drive the work forward.
>
> -sz
>
> On 2019/05/28 23:25:43, Marco de Abreu <ma...@gmail.com> wrote:
> > While AMP might be an experimental feature, I rather would like to put
> the
> > focus on the maturity of its interfaces. If the interfaces and the
> actions
> > developers have to do aren't finalized yet, I'd agree with disabling the
> > test. But if the API is final and easy to use, I don't see why adopting
> > early on would be harmful. But from what I can see, the output of the
> test
> > is very meaningful and explicit, easily understandable and offers the
> > developer a clear list of action items that they can follow.
> >
> > If people actually start commenting "CI test failure seems unrelated to
> my
> > change, please proceed and merge", we should advise them to please open
> the
> > result tab, which will directly show the clear list of action items.
> > Committers should support these contributors who are not that familiar
> with
> > our CI system and explain to them how they can retrieve the reasons for
> the
> > failure. Simply ignoring the fact that the change is not compatible with
> > AMP doesn't seem to be the right way.
> >
> > I think nobody is opposed to having AMP in MXNet. As part of accepting
> the
> > feature, we also added AMP to the established methods including the
> coding
> > constraints and other checks that come with it. Lets be open and welcome
> to
> > this new feature and come back if the turnaround time is actually too
> high.
> >
> > -Marco
> >
> >
> > On Wed, May 29, 2019 at 1:13 AM Sheng Zha <zh...@apache.org> wrote:
> >
> > > The support for AMP should not be a burden of authors of new operators.
> > > The lint analogy doesn't apply because lint is for established and
> accepted
> > > coding standard at MXNet and AMP is not. AMP is an experimental feature
> > > right now and it doesn't make sense to require contributors to invest
> in
> > > it. Keeping this as error will inevitably cause comments like "CI test
> > > failure seems unrelated to my change, please proceed and merge".
> > >
> > > -sz
> > >
> > > On 2019/05/28 22:51:30, Marco de Abreu <ma...@gmail.com>
> wrote:
> > > > Hi,
> > > >
> > > > I'm generally in favour of these kind of tests since they make
> developers
> > > > aware of changes they have to make which they would usually not be
> aware
> > > > of. We have a similar test for tutorials, for example. Whenever
> somebody
> > > > adds a tutorial, there's a validation that assures that all
> contraints in
> > > > our testing environment are met and that they are properly tied into
> the
> > > > system. This AMP test fits into the same category in my opinion and
> we
> > > > never heard bad feedback about these kind of checks.
> > > >
> > > > What seems to be bothering people is the fact that the feedback time
> is
> > > too
> > > > high. Thus, I'd like to propose to move the test into the sanity-test
> > > stage
> > > > instead of doing it as part of the unit tests which take quite a bit
> of
> > > > time until they're actually executed. The sanity checks run
> immediately
> > > and
> > > > give a response within about 1 minute.
> > > >
> > > > While I understand that this might increase the amount of work a
> > > developer
> > > > has to do if they develop a new operator, I think that this is the
> right
> > > > thing to do. Developers won't know of every single feature other
> people
> > > > worked on and thus might simply miss adding the support for it. This
> kind
> > > > of test on the other hand makes them aware of it. If they'd like to
> opt
> > > > out, it's one single line they would have to change and then they're
> > > > totally fine. On the other hand, this might motivate them to add the
> > > > support since the kernel would be the last piece and everything else
> > > would
> > > > already be implemented.
> > > >
> > > > Considering how often a PR gets declined because of linting errors,
> I'd
> > > say
> > > > that these kind of errors are WAY more frequent that AMP telling
> somebody
> > > > to add their operator to a list. Considering that this would only
> have to
> > > > be done once per operator, that's work of about one minute. Add that
> to
> > > the
> > > > waiting time of the sanity check and you're left with about five
> "wasted"
> > > > minutes.
> > > >
> > > > I'm opposed towards adding a warning or treating them as float32 by
> > > default
> > > > since the operator author wouldn't notice. What will happen is that
> > > people
> > > > won't know about AMP and simply forget about low precision in general
> > > until
> > > > they're actively reminded. This check will remind them actively and
> thus
> > > > bring more attention to the feature. I know that the feature is still
> > > > experimental, but we have just started with the 1.6 branch and thus
> > > there's
> > > > enough time to make the experimental features production ready.
> Adding
> > > this
> > > > test early on will allow others to add the support for AMP during the
> > > early
> > > > stage of the 1.6 branch instead of asking them in the last few weeks
> > > before
> > > > a release. The result would only be that stuff is rushed or
> forgotten.
> > > >
> > > > To sum it up: I think this test is good and it should be kept as
> error,
> > > but
> > > > it should be moved to sanity checks.
> > > >
> > > > -Marco
> > > >
> > > > On Wed, May 29, 2019 at 12:21 AM Sheng Zha <zh...@apache.org>
> wrote:
> > > >
> > > > > Thanks for initiating the discussion.
> > > > >
> > > > > The premise for adding the test was to make sure that AMP feature
> is
> > > "not
> > > > > broken", but that's IMO not the right view. AMP is not supposed to
> > > support
> > > > > a new operator it hasn't seen before in the first place. There's no
> > > way for
> > > > > it to know whether the fp32 cast should happen or not. So AMP
> feature
> > > > > cannot provide the guarantee that it works for all future
> operators.
> > > Thus,
> > > > > adding new operators to AMP list should be considered new feature
> > > instead
> > > > > of fixing existing feature.
> > > > >
> > > > > The AMP test that breaks upon the addition of new operator is thus
> > > > > equivalent to forcing developers of the new operator to add the new
> > > support
> > > > > for AMP. This feels wrong. Especially given that AMP is an
> experimental
> > > > > feature in contrib namespace (i.e. no semver guarantee), this
> practice
> > > > > should be stopped immediately. We cannot force new developers to
> invest
> > > > > into experimental feature this way.
> > > > >
> > > > > I'd suggest the following changes:
> > > > > - for new operators that aren't registered in AMP, cast to float32
> by
> > > > > default and print one-time warning. People using AMP who want to
> avoid
> > > > > casting can register it in the AMP's list.
> > > > > - change the test to print warning about the operators that are not
> > > listed
> > > > > so that it's easy to track the problem.
> > > > >
> > > > > -sz
> > > > >
> > > > > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org>
> > > wrote:
> > > > > > Dear Community,
> > > > > >
> > > > > > One of the recently merged features of the 1.5 release, AMP
> > > (Automatic
> > > > > Mixed Precision) support (PR [1], design doc [5]), introduced a
> > > requirement
> > > > > that every new operator added to MXNet would need to be present in
> 1
> > > of the
> > > > > lists (in [2]). To make sure that this requirement is not broken
> when
> > > > > somebody adds a new operator and does not know about AMP's
> existence, a
> > > > > test was added to CI ([3]).
> > > > > >
> > > > > > A few people reached out to me (the original author of the
> feature)
> > > > > saying this test increases a burden on a developer of new
> operators and
> > > > > should not be an actual error, but just warning (PR for that change
> > > [4]).
> > > > > That is why I would like to present a motivation for it and discuss
> > > with
> > > > > the wider audience why I feel it was necessary.
> > > > > >
> > > > > > First, for people who do not know the details of what AMP is -
> it is
> > > a
> > > > > solution that tries to automatically apply best practices of
> training
> > > in
> > > > > lower precision (FP16) to user's FP32 model in order to fully
> utilize
> > > > > capabilities of modern GPUs (and potentially other hardware in the
> > > future).
> > > > > It does so by casting to lower precision inputs to operators
> > > benefitting
> > > > > from it, while casting to full precision inputs of operators that
> are
> > > > > unsafe to run in lower precision or just do not support it.
> > > > > >
> > > > > > The first iteration of AMP kept 2 main lists of operators -
> operators
> > > > > that are beneficial and safe to do in fp16 and operators that need
> to
> > > be
> > > > > cast to FP32. The problem (raised in review of the PR [6], [8]) is
> how
> > > to
> > > > > make sure that the feature works as intended and is not
> inadvertently
> > > > > broken by somebody adding a new operator. The failure scenario
> here is
> > > > > adding a new operator that does not support FP16 and so should be
> cast
> > > to
> > > > > FP32, but AMP does not know about its existence and so does not do
> the
> > > > > casting. The solution proposed in the review was to implicitly
> treat
> > > all of
> > > > > the unknown operators as FP32-only and keep the list of operators
> that
> > > work
> > > > > fine in both FP16 and FP32. This solution however does not really
> work,
> > > > > because there are multiple operators (most notably optimizers)
> where
> > > > > introducing additional casting of the input to FP32 would break the
> > > > > operator.
> > > > > >
> > > > > > That is why after discussion with a few members of the
> community, I
> > > > > decided to proceed with all lists being explicit and introducing
> the
> > > test
> > > > > that would fail when somebody added an operator without
> classifying it
> > > into
> > > > > 1 of the categories, and explain clearly how to do it [7]. It is
> not
> > > ideal
> > > > > solution, as it introduces some burden on the developers who are
> not
> > > aware
> > > > > about AMP, however in the typical case of adding at most a few
> > > operators to
> > > > > MXNet the inconvenience is I think pretty minor while important
> for the
> > > > > feature correctness going forward.
> > > > > >
> > > > > > I would like to gather Community feedback and ideas how to handle
> > > this
> > > > > situation.
> > > > > >
> > > > > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > > > > [2]
> > > > >
> > >
> https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > > > > [3]
> > > > >
> > >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > > > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > > > > [5]
> > > > >
> > >
> https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > > > > [6]
> > > > >
> > >
> https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > > > > [7]
> > > > >
> > >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > > > > [8]
> > > > >
> > >
> https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Making new operators and AMP lists

[Sheng Zha <zh...@apache.org>]

AMP is in contrib so there's no guarantee that the API is final. Adopting the test as-is is harmful because operator authors should not be required to invest in an experimental feature that they are not aware of.

I'm all for openness and welcoming, but think about whether you'd like to turn away developers who just want to write a CPU-only operator. The more you impose on the developers the less likely they will make the contribution through.

Having an unfamiliar operator in AMP as a warning could let everyone know what the support state is whenever that feature is used. For those who care about this, they would see the warning and add the support to get the speed benefit of not casting to fp32. In this case, rather than imposing it to developers who don't know about AMP, the one who actually uses AMP and cares about this feature would drive the work forward.

-sz

On 2019/05/28 23:25:43, Marco de Abreu <ma...@gmail.com> wrote: 
> While AMP might be an experimental feature, I rather would like to put the
> focus on the maturity of its interfaces. If the interfaces and the actions
> developers have to do aren't finalized yet, I'd agree with disabling the
> test. But if the API is final and easy to use, I don't see why adopting
> early on would be harmful. But from what I can see, the output of the test
> is very meaningful and explicit, easily understandable and offers the
> developer a clear list of action items that they can follow.
> 
> If people actually start commenting "CI test failure seems unrelated to my
> change, please proceed and merge", we should advise them to please open the
> result tab, which will directly show the clear list of action items.
> Committers should support these contributors who are not that familiar with
> our CI system and explain to them how they can retrieve the reasons for the
> failure. Simply ignoring the fact that the change is not compatible with
> AMP doesn't seem to be the right way.
> 
> I think nobody is opposed to having AMP in MXNet. As part of accepting the
> feature, we also added AMP to the established methods including the coding
> constraints and other checks that come with it. Lets be open and welcome to
> this new feature and come back if the turnaround time is actually too high.
> 
> -Marco
> 
> 
> On Wed, May 29, 2019 at 1:13 AM Sheng Zha <zh...@apache.org> wrote:
> 
> > The support for AMP should not be a burden of authors of new operators.
> > The lint analogy doesn't apply because lint is for established and accepted
> > coding standard at MXNet and AMP is not. AMP is an experimental feature
> > right now and it doesn't make sense to require contributors to invest in
> > it. Keeping this as error will inevitably cause comments like "CI test
> > failure seems unrelated to my change, please proceed and merge".
> >
> > -sz
> >
> > On 2019/05/28 22:51:30, Marco de Abreu <ma...@gmail.com> wrote:
> > > Hi,
> > >
> > > I'm generally in favour of these kind of tests since they make developers
> > > aware of changes they have to make which they would usually not be aware
> > > of. We have a similar test for tutorials, for example. Whenever somebody
> > > adds a tutorial, there's a validation that assures that all contraints in
> > > our testing environment are met and that they are properly tied into the
> > > system. This AMP test fits into the same category in my opinion and we
> > > never heard bad feedback about these kind of checks.
> > >
> > > What seems to be bothering people is the fact that the feedback time is
> > too
> > > high. Thus, I'd like to propose to move the test into the sanity-test
> > stage
> > > instead of doing it as part of the unit tests which take quite a bit of
> > > time until they're actually executed. The sanity checks run immediately
> > and
> > > give a response within about 1 minute.
> > >
> > > While I understand that this might increase the amount of work a
> > developer
> > > has to do if they develop a new operator, I think that this is the right
> > > thing to do. Developers won't know of every single feature other people
> > > worked on and thus might simply miss adding the support for it. This kind
> > > of test on the other hand makes them aware of it. If they'd like to opt
> > > out, it's one single line they would have to change and then they're
> > > totally fine. On the other hand, this might motivate them to add the
> > > support since the kernel would be the last piece and everything else
> > would
> > > already be implemented.
> > >
> > > Considering how often a PR gets declined because of linting errors, I'd
> > say
> > > that these kind of errors are WAY more frequent that AMP telling somebody
> > > to add their operator to a list. Considering that this would only have to
> > > be done once per operator, that's work of about one minute. Add that to
> > the
> > > waiting time of the sanity check and you're left with about five "wasted"
> > > minutes.
> > >
> > > I'm opposed towards adding a warning or treating them as float32 by
> > default
> > > since the operator author wouldn't notice. What will happen is that
> > people
> > > won't know about AMP and simply forget about low precision in general
> > until
> > > they're actively reminded. This check will remind them actively and thus
> > > bring more attention to the feature. I know that the feature is still
> > > experimental, but we have just started with the 1.6 branch and thus
> > there's
> > > enough time to make the experimental features production ready. Adding
> > this
> > > test early on will allow others to add the support for AMP during the
> > early
> > > stage of the 1.6 branch instead of asking them in the last few weeks
> > before
> > > a release. The result would only be that stuff is rushed or forgotten.
> > >
> > > To sum it up: I think this test is good and it should be kept as error,
> > but
> > > it should be moved to sanity checks.
> > >
> > > -Marco
> > >
> > > On Wed, May 29, 2019 at 12:21 AM Sheng Zha <zh...@apache.org> wrote:
> > >
> > > > Thanks for initiating the discussion.
> > > >
> > > > The premise for adding the test was to make sure that AMP feature is
> > "not
> > > > broken", but that's IMO not the right view. AMP is not supposed to
> > support
> > > > a new operator it hasn't seen before in the first place. There's no
> > way for
> > > > it to know whether the fp32 cast should happen or not. So AMP feature
> > > > cannot provide the guarantee that it works for all future operators.
> > Thus,
> > > > adding new operators to AMP list should be considered new feature
> > instead
> > > > of fixing existing feature.
> > > >
> > > > The AMP test that breaks upon the addition of new operator is thus
> > > > equivalent to forcing developers of the new operator to add the new
> > support
> > > > for AMP. This feels wrong. Especially given that AMP is an experimental
> > > > feature in contrib namespace (i.e. no semver guarantee), this practice
> > > > should be stopped immediately. We cannot force new developers to invest
> > > > into experimental feature this way.
> > > >
> > > > I'd suggest the following changes:
> > > > - for new operators that aren't registered in AMP, cast to float32 by
> > > > default and print one-time warning. People using AMP who want to avoid
> > > > casting can register it in the AMP's list.
> > > > - change the test to print warning about the operators that are not
> > listed
> > > > so that it's easy to track the problem.
> > > >
> > > > -sz
> > > >
> > > > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org>
> > wrote:
> > > > > Dear Community,
> > > > >
> > > > > One of the recently merged features of the 1.5 release, AMP
> > (Automatic
> > > > Mixed Precision) support (PR [1], design doc [5]), introduced a
> > requirement
> > > > that every new operator added to MXNet would need to be present in 1
> > of the
> > > > lists (in [2]). To make sure that this requirement is not broken when
> > > > somebody adds a new operator and does not know about AMP's existence, a
> > > > test was added to CI ([3]).
> > > > >
> > > > > A few people reached out to me (the original author of the feature)
> > > > saying this test increases a burden on a developer of new operators and
> > > > should not be an actual error, but just warning (PR for that change
> > [4]).
> > > > That is why I would like to present a motivation for it and discuss
> > with
> > > > the wider audience why I feel it was necessary.
> > > > >
> > > > > First, for people who do not know the details of what AMP is - it is
> > a
> > > > solution that tries to automatically apply best practices of training
> > in
> > > > lower precision (FP16) to user's FP32 model in order to fully utilize
> > > > capabilities of modern GPUs (and potentially other hardware in the
> > future).
> > > > It does so by casting to lower precision inputs to operators
> > benefitting
> > > > from it, while casting to full precision inputs of operators that are
> > > > unsafe to run in lower precision or just do not support it.
> > > > >
> > > > > The first iteration of AMP kept 2 main lists of operators - operators
> > > > that are beneficial and safe to do in fp16 and operators that need to
> > be
> > > > cast to FP32. The problem (raised in review of the PR [6], [8]) is how
> > to
> > > > make sure that the feature works as intended and is not inadvertently
> > > > broken by somebody adding a new operator. The failure scenario here is
> > > > adding a new operator that does not support FP16 and so should be cast
> > to
> > > > FP32, but AMP does not know about its existence and so does not do the
> > > > casting. The solution proposed in the review was to implicitly treat
> > all of
> > > > the unknown operators as FP32-only and keep the list of operators that
> > work
> > > > fine in both FP16 and FP32. This solution however does not really work,
> > > > because there are multiple operators (most notably optimizers) where
> > > > introducing additional casting of the input to FP32 would break the
> > > > operator.
> > > > >
> > > > > That is why after discussion with a few members of the community, I
> > > > decided to proceed with all lists being explicit and introducing the
> > test
> > > > that would fail when somebody added an operator without classifying it
> > into
> > > > 1 of the categories, and explain clearly how to do it [7]. It is not
> > ideal
> > > > solution, as it introduces some burden on the developers who are not
> > aware
> > > > about AMP, however in the typical case of adding at most a few
> > operators to
> > > > MXNet the inconvenience is I think pretty minor while important for the
> > > > feature correctness going forward.
> > > > >
> > > > > I would like to gather Community feedback and ideas how to handle
> > this
> > > > situation.
> > > > >
> > > > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > > > [2]
> > > >
> > https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > > > [3]
> > > >
> > https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > > > [5]
> > > >
> > https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > > > [6]
> > > >
> > https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > > > [7]
> > > >
> > https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > > > [8]
> > > >
> > https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > > > >
> > > >
> > >
> >
> 

Re: Making new operators and AMP lists

[Marco de Abreu <ma...@gmail.com>]

While AMP might be an experimental feature, I rather would like to put the
focus on the maturity of its interfaces. If the interfaces and the actions
developers have to do aren't finalized yet, I'd agree with disabling the
test. But if the API is final and easy to use, I don't see why adopting
early on would be harmful. But from what I can see, the output of the test
is very meaningful and explicit, easily understandable and offers the
developer a clear list of action items that they can follow.

If people actually start commenting "CI test failure seems unrelated to my
change, please proceed and merge", we should advise them to please open the
result tab, which will directly show the clear list of action items.
Committers should support these contributors who are not that familiar with
our CI system and explain to them how they can retrieve the reasons for the
failure. Simply ignoring the fact that the change is not compatible with
AMP doesn't seem to be the right way.

I think nobody is opposed to having AMP in MXNet. As part of accepting the
feature, we also added AMP to the established methods including the coding
constraints and other checks that come with it. Lets be open and welcome to
this new feature and come back if the turnaround time is actually too high.

-Marco


On Wed, May 29, 2019 at 1:13 AM Sheng Zha <zh...@apache.org> wrote:

> The support for AMP should not be a burden of authors of new operators.
> The lint analogy doesn't apply because lint is for established and accepted
> coding standard at MXNet and AMP is not. AMP is an experimental feature
> right now and it doesn't make sense to require contributors to invest in
> it. Keeping this as error will inevitably cause comments like "CI test
> failure seems unrelated to my change, please proceed and merge".
>
> -sz
>
> On 2019/05/28 22:51:30, Marco de Abreu <ma...@gmail.com> wrote:
> > Hi,
> >
> > I'm generally in favour of these kind of tests since they make developers
> > aware of changes they have to make which they would usually not be aware
> > of. We have a similar test for tutorials, for example. Whenever somebody
> > adds a tutorial, there's a validation that assures that all contraints in
> > our testing environment are met and that they are properly tied into the
> > system. This AMP test fits into the same category in my opinion and we
> > never heard bad feedback about these kind of checks.
> >
> > What seems to be bothering people is the fact that the feedback time is
> too
> > high. Thus, I'd like to propose to move the test into the sanity-test
> stage
> > instead of doing it as part of the unit tests which take quite a bit of
> > time until they're actually executed. The sanity checks run immediately
> and
> > give a response within about 1 minute.
> >
> > While I understand that this might increase the amount of work a
> developer
> > has to do if they develop a new operator, I think that this is the right
> > thing to do. Developers won't know of every single feature other people
> > worked on and thus might simply miss adding the support for it. This kind
> > of test on the other hand makes them aware of it. If they'd like to opt
> > out, it's one single line they would have to change and then they're
> > totally fine. On the other hand, this might motivate them to add the
> > support since the kernel would be the last piece and everything else
> would
> > already be implemented.
> >
> > Considering how often a PR gets declined because of linting errors, I'd
> say
> > that these kind of errors are WAY more frequent that AMP telling somebody
> > to add their operator to a list. Considering that this would only have to
> > be done once per operator, that's work of about one minute. Add that to
> the
> > waiting time of the sanity check and you're left with about five "wasted"
> > minutes.
> >
> > I'm opposed towards adding a warning or treating them as float32 by
> default
> > since the operator author wouldn't notice. What will happen is that
> people
> > won't know about AMP and simply forget about low precision in general
> until
> > they're actively reminded. This check will remind them actively and thus
> > bring more attention to the feature. I know that the feature is still
> > experimental, but we have just started with the 1.6 branch and thus
> there's
> > enough time to make the experimental features production ready. Adding
> this
> > test early on will allow others to add the support for AMP during the
> early
> > stage of the 1.6 branch instead of asking them in the last few weeks
> before
> > a release. The result would only be that stuff is rushed or forgotten.
> >
> > To sum it up: I think this test is good and it should be kept as error,
> but
> > it should be moved to sanity checks.
> >
> > -Marco
> >
> > On Wed, May 29, 2019 at 12:21 AM Sheng Zha <zh...@apache.org> wrote:
> >
> > > Thanks for initiating the discussion.
> > >
> > > The premise for adding the test was to make sure that AMP feature is
> "not
> > > broken", but that's IMO not the right view. AMP is not supposed to
> support
> > > a new operator it hasn't seen before in the first place. There's no
> way for
> > > it to know whether the fp32 cast should happen or not. So AMP feature
> > > cannot provide the guarantee that it works for all future operators.
> Thus,
> > > adding new operators to AMP list should be considered new feature
> instead
> > > of fixing existing feature.
> > >
> > > The AMP test that breaks upon the addition of new operator is thus
> > > equivalent to forcing developers of the new operator to add the new
> support
> > > for AMP. This feels wrong. Especially given that AMP is an experimental
> > > feature in contrib namespace (i.e. no semver guarantee), this practice
> > > should be stopped immediately. We cannot force new developers to invest
> > > into experimental feature this way.
> > >
> > > I'd suggest the following changes:
> > > - for new operators that aren't registered in AMP, cast to float32 by
> > > default and print one-time warning. People using AMP who want to avoid
> > > casting can register it in the AMP's list.
> > > - change the test to print warning about the operators that are not
> listed
> > > so that it's easy to track the problem.
> > >
> > > -sz
> > >
> > > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org>
> wrote:
> > > > Dear Community,
> > > >
> > > > One of the recently merged features of the 1.5 release, AMP
> (Automatic
> > > Mixed Precision) support (PR [1], design doc [5]), introduced a
> requirement
> > > that every new operator added to MXNet would need to be present in 1
> of the
> > > lists (in [2]). To make sure that this requirement is not broken when
> > > somebody adds a new operator and does not know about AMP's existence, a
> > > test was added to CI ([3]).
> > > >
> > > > A few people reached out to me (the original author of the feature)
> > > saying this test increases a burden on a developer of new operators and
> > > should not be an actual error, but just warning (PR for that change
> [4]).
> > > That is why I would like to present a motivation for it and discuss
> with
> > > the wider audience why I feel it was necessary.
> > > >
> > > > First, for people who do not know the details of what AMP is - it is
> a
> > > solution that tries to automatically apply best practices of training
> in
> > > lower precision (FP16) to user's FP32 model in order to fully utilize
> > > capabilities of modern GPUs (and potentially other hardware in the
> future).
> > > It does so by casting to lower precision inputs to operators
> benefitting
> > > from it, while casting to full precision inputs of operators that are
> > > unsafe to run in lower precision or just do not support it.
> > > >
> > > > The first iteration of AMP kept 2 main lists of operators - operators
> > > that are beneficial and safe to do in fp16 and operators that need to
> be
> > > cast to FP32. The problem (raised in review of the PR [6], [8]) is how
> to
> > > make sure that the feature works as intended and is not inadvertently
> > > broken by somebody adding a new operator. The failure scenario here is
> > > adding a new operator that does not support FP16 and so should be cast
> to
> > > FP32, but AMP does not know about its existence and so does not do the
> > > casting. The solution proposed in the review was to implicitly treat
> all of
> > > the unknown operators as FP32-only and keep the list of operators that
> work
> > > fine in both FP16 and FP32. This solution however does not really work,
> > > because there are multiple operators (most notably optimizers) where
> > > introducing additional casting of the input to FP32 would break the
> > > operator.
> > > >
> > > > That is why after discussion with a few members of the community, I
> > > decided to proceed with all lists being explicit and introducing the
> test
> > > that would fail when somebody added an operator without classifying it
> into
> > > 1 of the categories, and explain clearly how to do it [7]. It is not
> ideal
> > > solution, as it introduces some burden on the developers who are not
> aware
> > > about AMP, however in the typical case of adding at most a few
> operators to
> > > MXNet the inconvenience is I think pretty minor while important for the
> > > feature correctness going forward.
> > > >
> > > > I would like to gather Community feedback and ideas how to handle
> this
> > > situation.
> > > >
> > > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > > [2]
> > >
> https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > > [3]
> > >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > > [5]
> > >
> https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > > [6]
> > >
> https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > > [7]
> > >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > > [8]
> > >
> https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > > >
> > >
> >
>

Re: Making new operators and AMP lists

[Sheng Zha <zh...@apache.org>]

The support for AMP should not be a burden of authors of new operators. The lint analogy doesn't apply because lint is for established and accepted coding standard at MXNet and AMP is not. AMP is an experimental feature right now and it doesn't make sense to require contributors to invest in it. Keeping this as error will inevitably cause comments like "CI test failure seems unrelated to my change, please proceed and merge".

-sz

On 2019/05/28 22:51:30, Marco de Abreu <ma...@gmail.com> wrote: 
> Hi,
> 
> I'm generally in favour of these kind of tests since they make developers
> aware of changes they have to make which they would usually not be aware
> of. We have a similar test for tutorials, for example. Whenever somebody
> adds a tutorial, there's a validation that assures that all contraints in
> our testing environment are met and that they are properly tied into the
> system. This AMP test fits into the same category in my opinion and we
> never heard bad feedback about these kind of checks.
> 
> What seems to be bothering people is the fact that the feedback time is too
> high. Thus, I'd like to propose to move the test into the sanity-test stage
> instead of doing it as part of the unit tests which take quite a bit of
> time until they're actually executed. The sanity checks run immediately and
> give a response within about 1 minute.
> 
> While I understand that this might increase the amount of work a developer
> has to do if they develop a new operator, I think that this is the right
> thing to do. Developers won't know of every single feature other people
> worked on and thus might simply miss adding the support for it. This kind
> of test on the other hand makes them aware of it. If they'd like to opt
> out, it's one single line they would have to change and then they're
> totally fine. On the other hand, this might motivate them to add the
> support since the kernel would be the last piece and everything else would
> already be implemented.
> 
> Considering how often a PR gets declined because of linting errors, I'd say
> that these kind of errors are WAY more frequent that AMP telling somebody
> to add their operator to a list. Considering that this would only have to
> be done once per operator, that's work of about one minute. Add that to the
> waiting time of the sanity check and you're left with about five "wasted"
> minutes.
> 
> I'm opposed towards adding a warning or treating them as float32 by default
> since the operator author wouldn't notice. What will happen is that people
> won't know about AMP and simply forget about low precision in general until
> they're actively reminded. This check will remind them actively and thus
> bring more attention to the feature. I know that the feature is still
> experimental, but we have just started with the 1.6 branch and thus there's
> enough time to make the experimental features production ready. Adding this
> test early on will allow others to add the support for AMP during the early
> stage of the 1.6 branch instead of asking them in the last few weeks before
> a release. The result would only be that stuff is rushed or forgotten.
> 
> To sum it up: I think this test is good and it should be kept as error, but
> it should be moved to sanity checks.
> 
> -Marco
> 
> On Wed, May 29, 2019 at 12:21 AM Sheng Zha <zh...@apache.org> wrote:
> 
> > Thanks for initiating the discussion.
> >
> > The premise for adding the test was to make sure that AMP feature is "not
> > broken", but that's IMO not the right view. AMP is not supposed to support
> > a new operator it hasn't seen before in the first place. There's no way for
> > it to know whether the fp32 cast should happen or not. So AMP feature
> > cannot provide the guarantee that it works for all future operators. Thus,
> > adding new operators to AMP list should be considered new feature instead
> > of fixing existing feature.
> >
> > The AMP test that breaks upon the addition of new operator is thus
> > equivalent to forcing developers of the new operator to add the new support
> > for AMP. This feels wrong. Especially given that AMP is an experimental
> > feature in contrib namespace (i.e. no semver guarantee), this practice
> > should be stopped immediately. We cannot force new developers to invest
> > into experimental feature this way.
> >
> > I'd suggest the following changes:
> > - for new operators that aren't registered in AMP, cast to float32 by
> > default and print one-time warning. People using AMP who want to avoid
> > casting can register it in the AMP's list.
> > - change the test to print warning about the operators that are not listed
> > so that it's easy to track the problem.
> >
> > -sz
> >
> > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org> wrote:
> > > Dear Community,
> > >
> > > One of the recently merged features of the 1.5 release, AMP (Automatic
> > Mixed Precision) support (PR [1], design doc [5]), introduced a requirement
> > that every new operator added to MXNet would need to be present in 1 of the
> > lists (in [2]). To make sure that this requirement is not broken when
> > somebody adds a new operator and does not know about AMP's existence, a
> > test was added to CI ([3]).
> > >
> > > A few people reached out to me (the original author of the feature)
> > saying this test increases a burden on a developer of new operators and
> > should not be an actual error, but just warning (PR for that change [4]).
> > That is why I would like to present a motivation for it and discuss with
> > the wider audience why I feel it was necessary.
> > >
> > > First, for people who do not know the details of what AMP is - it is a
> > solution that tries to automatically apply best practices of training in
> > lower precision (FP16) to user's FP32 model in order to fully utilize
> > capabilities of modern GPUs (and potentially other hardware in the future).
> > It does so by casting to lower precision inputs to operators benefitting
> > from it, while casting to full precision inputs of operators that are
> > unsafe to run in lower precision or just do not support it.
> > >
> > > The first iteration of AMP kept 2 main lists of operators - operators
> > that are beneficial and safe to do in fp16 and operators that need to be
> > cast to FP32. The problem (raised in review of the PR [6], [8]) is how to
> > make sure that the feature works as intended and is not inadvertently
> > broken by somebody adding a new operator. The failure scenario here is
> > adding a new operator that does not support FP16 and so should be cast to
> > FP32, but AMP does not know about its existence and so does not do the
> > casting. The solution proposed in the review was to implicitly treat all of
> > the unknown operators as FP32-only and keep the list of operators that work
> > fine in both FP16 and FP32. This solution however does not really work,
> > because there are multiple operators (most notably optimizers) where
> > introducing additional casting of the input to FP32 would break the
> > operator.
> > >
> > > That is why after discussion with a few members of the community, I
> > decided to proceed with all lists being explicit and introducing the test
> > that would fail when somebody added an operator without classifying it into
> > 1 of the categories, and explain clearly how to do it [7]. It is not ideal
> > solution, as it introduces some burden on the developers who are not aware
> > about AMP, however in the typical case of adding at most a few operators to
> > MXNet the inconvenience is I think pretty minor while important for the
> > feature correctness going forward.
> > >
> > > I would like to gather Community feedback and ideas how to handle this
> > situation.
> > >
> > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > [2]
> > https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > [3]
> > https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > [5]
> > https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > [6]
> > https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > [7]
> > https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > [8]
> > https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > >
> >
> 

Re: Making new operators and AMP lists

[Marco de Abreu <ma...@gmail.com>]

Hi,

I'm generally in favour of these kind of tests since they make developers
aware of changes they have to make which they would usually not be aware
of. We have a similar test for tutorials, for example. Whenever somebody
adds a tutorial, there's a validation that assures that all contraints in
our testing environment are met and that they are properly tied into the
system. This AMP test fits into the same category in my opinion and we
never heard bad feedback about these kind of checks.

What seems to be bothering people is the fact that the feedback time is too
high. Thus, I'd like to propose to move the test into the sanity-test stage
instead of doing it as part of the unit tests which take quite a bit of
time until they're actually executed. The sanity checks run immediately and
give a response within about 1 minute.

While I understand that this might increase the amount of work a developer
has to do if they develop a new operator, I think that this is the right
thing to do. Developers won't know of every single feature other people
worked on and thus might simply miss adding the support for it. This kind
of test on the other hand makes them aware of it. If they'd like to opt
out, it's one single line they would have to change and then they're
totally fine. On the other hand, this might motivate them to add the
support since the kernel would be the last piece and everything else would
already be implemented.

Considering how often a PR gets declined because of linting errors, I'd say
that these kind of errors are WAY more frequent that AMP telling somebody
to add their operator to a list. Considering that this would only have to
be done once per operator, that's work of about one minute. Add that to the
waiting time of the sanity check and you're left with about five "wasted"
minutes.

I'm opposed towards adding a warning or treating them as float32 by default
since the operator author wouldn't notice. What will happen is that people
won't know about AMP and simply forget about low precision in general until
they're actively reminded. This check will remind them actively and thus
bring more attention to the feature. I know that the feature is still
experimental, but we have just started with the 1.6 branch and thus there's
enough time to make the experimental features production ready. Adding this
test early on will allow others to add the support for AMP during the early
stage of the 1.6 branch instead of asking them in the last few weeks before
a release. The result would only be that stuff is rushed or forgotten.

To sum it up: I think this test is good and it should be kept as error, but
it should be moved to sanity checks.

-Marco

On Wed, May 29, 2019 at 12:21 AM Sheng Zha <zh...@apache.org> wrote:

> Thanks for initiating the discussion.
>
> The premise for adding the test was to make sure that AMP feature is "not
> broken", but that's IMO not the right view. AMP is not supposed to support
> a new operator it hasn't seen before in the first place. There's no way for
> it to know whether the fp32 cast should happen or not. So AMP feature
> cannot provide the guarantee that it works for all future operators. Thus,
> adding new operators to AMP list should be considered new feature instead
> of fixing existing feature.
>
> The AMP test that breaks upon the addition of new operator is thus
> equivalent to forcing developers of the new operator to add the new support
> for AMP. This feels wrong. Especially given that AMP is an experimental
> feature in contrib namespace (i.e. no semver guarantee), this practice
> should be stopped immediately. We cannot force new developers to invest
> into experimental feature this way.
>
> I'd suggest the following changes:
> - for new operators that aren't registered in AMP, cast to float32 by
> default and print one-time warning. People using AMP who want to avoid
> casting can register it in the AMP's list.
> - change the test to print warning about the operators that are not listed
> so that it's easy to track the problem.
>
> -sz
>
> On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org> wrote:
> > Dear Community,
> >
> > One of the recently merged features of the 1.5 release, AMP (Automatic
> Mixed Precision) support (PR [1], design doc [5]), introduced a requirement
> that every new operator added to MXNet would need to be present in 1 of the
> lists (in [2]). To make sure that this requirement is not broken when
> somebody adds a new operator and does not know about AMP's existence, a
> test was added to CI ([3]).
> >
> > A few people reached out to me (the original author of the feature)
> saying this test increases a burden on a developer of new operators and
> should not be an actual error, but just warning (PR for that change [4]).
> That is why I would like to present a motivation for it and discuss with
> the wider audience why I feel it was necessary.
> >
> > First, for people who do not know the details of what AMP is - it is a
> solution that tries to automatically apply best practices of training in
> lower precision (FP16) to user's FP32 model in order to fully utilize
> capabilities of modern GPUs (and potentially other hardware in the future).
> It does so by casting to lower precision inputs to operators benefitting
> from it, while casting to full precision inputs of operators that are
> unsafe to run in lower precision or just do not support it.
> >
> > The first iteration of AMP kept 2 main lists of operators - operators
> that are beneficial and safe to do in fp16 and operators that need to be
> cast to FP32. The problem (raised in review of the PR [6], [8]) is how to
> make sure that the feature works as intended and is not inadvertently
> broken by somebody adding a new operator. The failure scenario here is
> adding a new operator that does not support FP16 and so should be cast to
> FP32, but AMP does not know about its existence and so does not do the
> casting. The solution proposed in the review was to implicitly treat all of
> the unknown operators as FP32-only and keep the list of operators that work
> fine in both FP16 and FP32. This solution however does not really work,
> because there are multiple operators (most notably optimizers) where
> introducing additional casting of the input to FP32 would break the
> operator.
> >
> > That is why after discussion with a few members of the community, I
> decided to proceed with all lists being explicit and introducing the test
> that would fail when somebody added an operator without classifying it into
> 1 of the categories, and explain clearly how to do it [7]. It is not ideal
> solution, as it introduces some burden on the developers who are not aware
> about AMP, however in the typical case of adding at most a few operators to
> MXNet the inconvenience is I think pretty minor while important for the
> feature correctness going forward.
> >
> > I would like to gather Community feedback and ideas how to handle this
> situation.
> >
> > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > [2]
> https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > [3]
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > [5]
> https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > [6]
> https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > [7]
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > [8]
> https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> >
>

Re: Making new operators and AMP lists

[Sheng Zha <zh...@apache.org>]

Thanks for initiating the discussion. 

The premise for adding the test was to make sure that AMP feature is "not broken", but that's IMO not the right view. AMP is not supposed to support a new operator it hasn't seen before in the first place. There's no way for it to know whether the fp32 cast should happen or not. So AMP feature cannot provide the guarantee that it works for all future operators. Thus, adding new operators to AMP list should be considered new feature instead of fixing existing feature.

The AMP test that breaks upon the addition of new operator is thus equivalent to forcing developers of the new operator to add the new support for AMP. This feels wrong. Especially given that AMP is an experimental feature in contrib namespace (i.e. no semver guarantee), this practice should be stopped immediately. We cannot force new developers to invest into experimental feature this way.

I'd suggest the following changes:
- for new operators that aren't registered in AMP, cast to float32 by default and print one-time warning. People using AMP who want to avoid casting can register it in the AMP's list.
- change the test to print warning about the operators that are not listed so that it's easy to track the problem.

-sz

On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org> wrote: 
> Dear Community,
> 
> One of the recently merged features of the 1.5 release, AMP (Automatic Mixed Precision) support (PR [1], design doc [5]), introduced a requirement that every new operator added to MXNet would need to be present in 1 of the lists (in [2]). To make sure that this requirement is not broken when somebody adds a new operator and does not know about AMP's existence, a test was added to CI ([3]).
> 
> A few people reached out to me (the original author of the feature) saying this test increases a burden on a developer of new operators and should not be an actual error, but just warning (PR for that change [4]). That is why I would like to present a motivation for it and discuss with the wider audience why I feel it was necessary.
> 
> First, for people who do not know the details of what AMP is - it is a solution that tries to automatically apply best practices of training in lower precision (FP16) to user's FP32 model in order to fully utilize capabilities of modern GPUs (and potentially other hardware in the future). It does so by casting to lower precision inputs to operators benefitting from it, while casting to full precision inputs of operators that are unsafe to run in lower precision or just do not support it.
> 
> The first iteration of AMP kept 2 main lists of operators - operators that are beneficial and safe to do in fp16 and operators that need to be cast to FP32. The problem (raised in review of the PR [6], [8]) is how to make sure that the feature works as intended and is not inadvertently broken by somebody adding a new operator. The failure scenario here is adding a new operator that does not support FP16 and so should be cast to FP32, but AMP does not know about its existence and so does not do the casting. The solution proposed in the review was to implicitly treat all of the unknown operators as FP32-only and keep the list of operators that work fine in both FP16 and FP32. This solution however does not really work, because there are multiple operators (most notably optimizers) where introducing additional casting of the input to FP32 would break the operator.
> 
> That is why after discussion with a few members of the community, I decided to proceed with all lists being explicit and introducing the test that would fail when somebody added an operator without classifying it into 1 of the categories, and explain clearly how to do it [7]. It is not ideal solution, as it introduces some burden on the developers who are not aware about AMP, however in the typical case of adding at most a few operators to MXNet the inconvenience is I think pretty minor while important for the feature correctness going forward.
> 
> I would like to gather Community feedback and ideas how to handle this situation.
> 
> [1] https://github.com/apache/incubator-mxnet/pull/14173
> [2] https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> [3] https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> [4] https://github.com/apache/incubator-mxnet/pull/15085
> [5] https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> [6] https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> [7] https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> [8] https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> 

Re: Making new operators and AMP lists

[Anirudh Subramanian <an...@gmail.com>]

> HOWEVER, as I was writing this reply I realized that due to pure luck
this is not actually what happens - optimizers could in fact be in the
FP32_FUNCS list. That is because, as AMP's assumption is that the model
being changed is FP32 model at the start, all weights (and so all the
gradients just before application as well) are stored in FP32 already (and
so the casting would not actually occur). It would therefore be possible to
make the FP32 list to be implicit (although relying on this lucky
coincidence makes me slightly uncomfortable).

This is under the assumption that the customers use the default lists. If
customer use their own lists, it can happen that some weights have to be in
FP16 and they may have to do some heavy lifting.  My understanding is that
this is the use case that makes you uncomfortable ? If not can you expand ?
The common use cases will be customer using default lists, and the later
use case may be rare.

Anirudh


On Thu, May 30, 2019 at 3:25 PM Sheng Zha <zh...@apache.org> wrote:

> Given that we're swtiching from the practice of failing the AMP related
> test to warning, I intend to merge #15085 soon if no objection.
>
> -sz
>
> On 2019/05/30 19:07:07, Przemys��aw Tr��dak <pt...@apache.org> wrote:
> > Hi Sam and Zhennan,
> >
> > the problem is not how to implicitly produce a list of all operators not
> in any other list - that is easy and the code Zhennan provided would work.
> The problem is that such list would not be actually correct in all cases -
> you do NOT want optimizers to land in FP32_FUNCS list, because then the
> tensor being updated could potentially be the FP32 copy and not the actual
> tensor you care about. That is why the first bullet point in the error
> message in the test is along the lines of: "if you do an optimizer or
> anything else that does not go in the computational graph, put it in the
> FP16_FP32_FUNCS list" - that list does not do any wrapping.
> >
> > HOWEVER, as I was writing this reply I realized that due to pure luck
> this is not actually what happens - optimizers could in fact be in the
> FP32_FUNCS list. That is because, as AMP's assumption is that the model
> being changed is FP32 model at the start, all weights (and so all the
> gradients just before application as well) are stored in FP32 already (and
> so the casting would not actually occur). It would therefore be possible to
> make the FP32 list to be implicit (although relying on this lucky
> coincidence makes me slightly uncomfortable).
> >
> > Przemek
> >
> >
> >
> > On 2019/05/30 08:04:05, "Qin, Zhennan" <zh...@intel.com> wrote:
> > > How about change the below line in amp.py:164
> > >
> > > wrap_list = fp32_ops if fp32_ops is not None else
> > > lists.symbol.FP32_FUNCS
> > >
> > > to be
> > >
> > >     plist = ctypes.POINTER(ctypes.c_char_p)()
> > >     size = ctypes.c_uint()
> > >
> > >     check_call(_LIB.MXListAllOpNames(ctypes.byref(size),
> > >                                      ctypes.byref(plist)))
> > >     op_names = []
> > >     for i in range(size.value):
> > >         op_names.append(py_str(plist[i]))
> > >
> > >     wrap_list = []
> > >     fp16_op_list = lists.symbol.FP16_FUNCS +
> > > lists.symbol.WIDEST_TYPE_CASTS + lists.symbol.FP16_FP32_FUNCS +
> > > list(map(lambda x: x[0], lists.symbol.CONDITIONAL_FP32_FUNCS))
> > >     for op_name in op_names:
> > >         if not op_name.startswith("_backward_") and not
> > > op_name.startswith("_contrib_backward_") and op_name not in
> > > fp16_op_list:
> > >             wrap_list.append(op_name)
> > >     wrap_list = fp32_ops if fp32_ops is not None else wrap_list
> > >
> > >
> > > I've checked, the changed code can produce identity wrap_list as
> > > lists.symbol.FP16_FP32_FUNCS.
> > >
> > > The check code is,
> > >
> > >     print("op that in wrap_list but not in FP32_FUNCS:")
> > >     for i in wrap_list:
> > >         if i not in lists.symbol.FP32_FUNCS:
> > >             print(i)
> > >
> > >     print("op that in FP32_FUNCS but not in wrap_list:")
> > >     for i in lists.symbol.FP32_FUNCS:
> > >         if i not in wrap_list:
> > >             print(i)
> > >
> > > The output is,
> > >
> > > op that in infered_fp32_op_list but not in FP32_FUNCS:
> > > op that in FP32_FUNCS but not in infered_fp32_op_list:
> > > op that in infered_fp32_op_list but not in FP32_FUNCS:
> > > op that in FP32_FUNCS but not in infered_fp32_op_list:
> > >
> > > This change should be able to automatically set new operator as fp32
> > > only.
> > >
> > > Thanks,
> > > Zhennan
> > >
> > > On Wed, 2019-05-29 at 19:13 +0000, Skalicky, Sam wrote:
> > > > Thanks Przemek for the additional explanation, but I’m still confused
> > > > on this part. I don’t understand the explanation of the optimizer’s
> > > > interaction here.
> > > >
> > > > > > The technical reason for that is the only place from which one
> > > > > > can get MXNet operators is MXListAllOps call, which gives back
> > > > > > all operators without any way of differentiating them, including
> > > > > > for example calls to optimizers. If somebody added a new
> > > > > > optimizer and AMP inserts a FP32 cast before its input, the
> > > > > > optimizer would actually update the FP32 copy of the tensor
> > > > > > instead of the tensor itself. This is not a performance problem
> > > > > > (which, I agree, would be perfectly fine to solve later) - it is
> > > > > > a correctness problem.
> > > > > > That is why the error message in the test explicitly mentions
> > > > > > this case as one where the operator should be put in the list
> > > > > > that does not make any casts.
> > > >
> > > >
> > > > If this is third list that Anirudh mentions:
> > > >
> > > > > > 3. Not sure, Don't know or dont care: FP32 list.
> > > >
> > > >
> > > > and this list simply contains all operators that have not been
> > > > categorized, why do we need the list at all?
> > > >
> > > > Thanks,
> > > > Sam
> > > >
> > > >
> > > > > On May 29, 2019, at 11:58 AM, Sheng Zha <zh...@apache.org>
> > > > > wrote:
> > > > >
> > > > > > The second misunderstanding is that the ask from the test is to
> > > > > > somehow "add support" for AMP in the operator. That is definitely
> > > > > > not true and adding a single line in either FP32_FUNCS (cast me
> > > > > > always to FP32) or FP16_FP32_FUNCS (do not do anything with me
> > > > > > because I'm not relevant for this usecase) is perfectly fine.
> > > > >
> > > > > I respectfully disagree with this statement. In this case, the
> > > > > "support to add" is exactly to differentiate how the new operator
> > > > > should be treated. It was not there before, so it's new. And
> > > > > requiring other developers to add any line is totally not fine.
> > > > >
> > > > > > The test ensures that the list is updated at the same time as new
> > > > > > operator is introduced.
> > > > >
> > > > > It would indeed have that effect. The problem is that the test
> > > > > requires the wrong person to add such support. A developer who adds
> > > > > a new operator should be under no obligation to do anything with
> > > > > AMP.
> > > > >
> > > > > Having it as warning both in runtime and in the test has the effect
> > > > > to prompt people who actually use and care about the performance
> > > > > from AMP to take the steps and decide where a new operator fits.
> > > > > And like you said, "It is up to people like me actively working on
> > > > > better FP16 support to go through the FP32 list to introduce
> > > > > support and then move such operator to another list." This is also
> > > > > exactly how it should work with AMP.
> > > > >
> > > > > -sz
> > > > >
> > > > > On 2019/05/29 18:25:46, Przemys��aw Tr��dak <pt...@apache.org>
> > > > > wrote:
> > > > > > Thank you all for responding.
> > > > > >
> > > > > > Let me address a few misunderstandings about AMP in the
> > > > > > discussion so far:
> > > > > > - I think the main misunderstanding in the discussion so far is
> > > > > > that it would be somehow possible to implicitly cast to FP32 all
> > > > > > previously unseen operators. I would much prefer such solution
> > > > > > myself, however this is actually (unfortunately) not really
> > > > > > possible. The technical reason for that is the only place from
> > > > > > which one can get MXNet operators is MXListAllOps call, which
> > > > > > gives back all operators without any way of differentiating them,
> > > > > > including for example calls to optimizers. If somebody added a
> > > > > > new optimizer and AMP inserts a FP32 cast before its input, the
> > > > > > optimizer would actually update the FP32 copy of the tensor
> > > > > > instead of the tensor itself. This is not a performance problem
> > > > > > (which, I agree, would be perfectly fine to solve later) - it is
> > > > > > a correctness problem. That is why the error message in the test
> > > > > > explicitly mentions this case as one where the operator should be
> > > > > > put in the list that does not make any casts.
> > > > > > - The second misunderstanding is that the ask from the test is to
> > > > > > somehow "add support" for AMP in the operator. That is definitely
> > > > > > not true and adding a single line in either FP32_FUNCS (cast me
> > > > > > always to FP32) or FP16_FP32_FUNCS (do not do anything with me
> > > > > > because I'm not relevant for this usecase) is perfectly fine. It
> > > > > > is up to people like me actively working on better FP16 support
> > > > > > to go through the FP32 list to introduce support and then move
> > > > > > such operator to another list.
> > > > > > - The third (although minor) misunderstanding is that this
> > > > > > feature is completely GPU-specific. It is not, and actually the
> > > > > > review comment that prompted me to introduce the design as is
> > > > > > currently implemented comes from MKLDNN developer :-).
> > > > > >
> > > > > > About the proposal to move it from Python list into a operator
> > > > > > property - I am in favor of it and, as Anirudh mentioned, this
> > > > > > should make it much easier when just adding aliases to the
> > > > > > operators. But again, this could not be an optional parameter
> > > > > > unfortunately since automatic "put me in FP32 list" is not
> > > > > > possible.
> > > > > > About the proposal to change the error into a warning and then
> > > > > > rely on people interested in AMP to adding operators to AMP lists
> > > > > > - I do not like this proposal because of user experience that it
> > > > > > would introduce. Basically what would happen is a user who
> > > > > > downloads nightly build of MXNet (because they needed a feature
> > > > > > introduced after last official release) would likely find that
> > > > > > AMP is broken in that build because it happened between updates
> > > > > > to the AMP lists. The test ensures that the list is updated at
> > > > > > the same time as new operator is introduced. This is a question
> > > > > > really of how do we treat nightly releases - should the user
> > > > > > expect for all the features to generally work in them or not? If
> > > > > > the Community thinks that it is acceptable outcome then I am fine
> > > > > > with the proposal and regularly updating those lists myself.
> > > > > >
> > > > > > Thank you
> > > > > > Przemek
> > > > > >
> > > > > > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org>
> > > > > > wrote:
> > > > > > > Dear Community,
> > > > > > >
> > > > > > > One of the recently merged features of the 1.5 release, AMP
> > > > > > > (Automatic Mixed Precision) support (PR [1], design doc [5]),
> > > > > > > introduced a requirement that every new operator added to MXNet
> > > > > > > would need to be present in 1 of the lists (in [2]). To make
> > > > > > > sure that this requirement is not broken when somebody adds a
> > > > > > > new operator and does not know about AMP's existence, a test
> > > > > > > was added to CI ([3]).
> > > > > > >
> > > > > > > A few people reached out to me (the original author of the
> > > > > > > feature) saying this test increases a burden on a developer of
> > > > > > > new operators and should not be an actual error, but just
> > > > > > > warning (PR for that change [4]). That is why I would like to
> > > > > > > present a motivation for it and discuss with the wider audience
> > > > > > > why I feel it was necessary.
> > > > > > >
> > > > > > > First, for people who do not know the details of what AMP is -
> > > > > > > it is a solution that tries to automatically apply best
> > > > > > > practices of training in lower precision (FP16) to user's FP32
> > > > > > > model in order to fully utilize capabilities of modern GPUs
> > > > > > > (and potentially other hardware in the future). It does so by
> > > > > > > casting to lower precision inputs to operators benefitting from
> > > > > > > it, while casting to full precision inputs of operators that
> > > > > > > are unsafe to run in lower precision or just do not support it.
> > > > > > >
> > > > > > > The first iteration of AMP kept 2 main lists of operators -
> > > > > > > operators that are beneficial and safe to do in fp16 and
> > > > > > > operators that need to be cast to FP32. The problem (raised in
> > > > > > > review of the PR [6], [8]) is how to make sure that the feature
> > > > > > > works as intended and is not inadvertently broken by somebody
> > > > > > > adding a new operator. The failure scenario here is adding a
> > > > > > > new operator that does not support FP16 and so should be cast
> > > > > > > to FP32, but AMP does not know about its existence and so does
> > > > > > > not do the casting. The solution proposed in the review was to
> > > > > > > implicitly treat all of the unknown operators as FP32-only and
> > > > > > > keep the list of operators that work fine in both FP16 and
> > > > > > > FP32. This solution however does not really work, because there
> > > > > > > are multiple operators (most notably optimizers) where
> > > > > > > introducing additional casting of the input to FP32 would break
> > > > > > > the operator.
> > > > > > >
> > > > > > > That is why after discussion with a few members of the
> > > > > > > community, I decided to proceed with all lists being explicit
> > > > > > > and introducing the test that would fail when somebody added an
> > > > > > > operator without classifying it into 1 of the categories, and
> > > > > > > explain clearly how to do it [7]. It is not ideal solution, as
> > > > > > > it introduces some burden on the developers who are not aware
> > > > > > > about AMP, however in the typical case of adding at most a few
> > > > > > > operators to MXNet the inconvenience is I think pretty minor
> > > > > > > while important for the feature correctness going forward.
> > > > > > >
> > > > > > > I would like to gather Community feedback and ideas how to
> > > > > > > handle this situation.
> > > > > > >
> > > > > > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > > > > > [2]
> > > > > > >
> https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > > > > > [3]
> > > > > > >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > > > > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > > > > > [5]
> > > > > > >
> https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > > > > > [6]
> > > > > > >
> https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > > > > > [7]
> > > > > > >
> https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > > > > > [8]
> > > > > > >
> https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > > > > > >
> > > >
> > > >
> > >
> >
>

Re: Making new operators and AMP lists

[Sheng Zha <zh...@apache.org>]

Given that we're swtiching from the practice of failing the AMP related test to warning, I intend to merge #15085 soon if no objection.

-sz

On 2019/05/30 19:07:07, Przemys��aw Tr��dak <pt...@apache.org> wrote: 
> Hi Sam and Zhennan, 
> 
> the problem is not how to implicitly produce a list of all operators not in any other list - that is easy and the code Zhennan provided would work. The problem is that such list would not be actually correct in all cases - you do NOT want optimizers to land in FP32_FUNCS list, because then the tensor being updated could potentially be the FP32 copy and not the actual tensor you care about. That is why the first bullet point in the error message in the test is along the lines of: "if you do an optimizer or anything else that does not go in the computational graph, put it in the FP16_FP32_FUNCS list" - that list does not do any wrapping.
> 
> HOWEVER, as I was writing this reply I realized that due to pure luck this is not actually what happens - optimizers could in fact be in the FP32_FUNCS list. That is because, as AMP's assumption is that the model being changed is FP32 model at the start, all weights (and so all the gradients just before application as well) are stored in FP32 already (and so the casting would not actually occur). It would therefore be possible to make the FP32 list to be implicit (although relying on this lucky coincidence makes me slightly uncomfortable).
> 
> Przemek 
> 
> 
> 
> On 2019/05/30 08:04:05, "Qin, Zhennan" <zh...@intel.com> wrote: 
> > How about change the below line in amp.py:164
> > 
> > wrap_list = fp32_ops if fp32_ops is not None else
> > lists.symbol.FP32_FUNCS
> > 
> > to be
> > 
> >     plist = ctypes.POINTER(ctypes.c_char_p)()
> >     size = ctypes.c_uint()
> > 
> >     check_call(_LIB.MXListAllOpNames(ctypes.byref(size),
> >                                      ctypes.byref(plist)))
> >     op_names = []
> >     for i in range(size.value):
> >         op_names.append(py_str(plist[i]))
> > 
> >     wrap_list = []
> >     fp16_op_list = lists.symbol.FP16_FUNCS +
> > lists.symbol.WIDEST_TYPE_CASTS + lists.symbol.FP16_FP32_FUNCS +
> > list(map(lambda x: x[0], lists.symbol.CONDITIONAL_FP32_FUNCS))
> >     for op_name in op_names:
> >         if not op_name.startswith("_backward_") and not
> > op_name.startswith("_contrib_backward_") and op_name not in
> > fp16_op_list:
> >             wrap_list.append(op_name)
> >     wrap_list = fp32_ops if fp32_ops is not None else wrap_list
> > 
> > 
> > I've checked, the changed code can produce identity wrap_list as
> > lists.symbol.FP16_FP32_FUNCS.
> > 
> > The check code is,
> > 
> >     print("op that in wrap_list but not in FP32_FUNCS:")
> >     for i in wrap_list:
> >         if i not in lists.symbol.FP32_FUNCS:
> >             print(i)
> > 
> >     print("op that in FP32_FUNCS but not in wrap_list:")
> >     for i in lists.symbol.FP32_FUNCS:
> >         if i not in wrap_list:
> >             print(i)
> > 
> > The output is,
> > 
> > op that in infered_fp32_op_list but not in FP32_FUNCS:
> > op that in FP32_FUNCS but not in infered_fp32_op_list:
> > op that in infered_fp32_op_list but not in FP32_FUNCS:
> > op that in FP32_FUNCS but not in infered_fp32_op_list:
> > 
> > This change should be able to automatically set new operator as fp32
> > only.
> > 
> > Thanks,
> > Zhennan
> > 
> > On Wed, 2019-05-29 at 19:13 +0000, Skalicky, Sam wrote:
> > > Thanks Przemek for the additional explanation, but I’m still confused
> > > on this part. I don’t understand the explanation of the optimizer’s
> > > interaction here. 
> > > 
> > > > > The technical reason for that is the only place from which one
> > > > > can get MXNet operators is MXListAllOps call, which gives back
> > > > > all operators without any way of differentiating them, including
> > > > > for example calls to optimizers. If somebody added a new
> > > > > optimizer and AMP inserts a FP32 cast before its input, the
> > > > > optimizer would actually update the FP32 copy of the tensor
> > > > > instead of the tensor itself. This is not a performance problem
> > > > > (which, I agree, would be perfectly fine to solve later) - it is
> > > > > a correctness problem.
> > > > > That is why the error message in the test explicitly mentions
> > > > > this case as one where the operator should be put in the list
> > > > > that does not make any casts.
> > > 
> > > 
> > > If this is third list that Anirudh mentions:
> > > 
> > > > > 3. Not sure, Don't know or dont care: FP32 list.
> > > 
> > > 
> > > and this list simply contains all operators that have not been
> > > categorized, why do we need the list at all? 
> > > 
> > > Thanks,
> > > Sam
> > > 
> > > 
> > > > On May 29, 2019, at 11:58 AM, Sheng Zha <zh...@apache.org>
> > > > wrote:
> > > > 
> > > > > The second misunderstanding is that the ask from the test is to
> > > > > somehow "add support" for AMP in the operator. That is definitely
> > > > > not true and adding a single line in either FP32_FUNCS (cast me
> > > > > always to FP32) or FP16_FP32_FUNCS (do not do anything with me
> > > > > because I'm not relevant for this usecase) is perfectly fine.
> > > > 
> > > > I respectfully disagree with this statement. In this case, the
> > > > "support to add" is exactly to differentiate how the new operator
> > > > should be treated. It was not there before, so it's new. And
> > > > requiring other developers to add any line is totally not fine.
> > > > 
> > > > > The test ensures that the list is updated at the same time as new
> > > > > operator is introduced.
> > > > 
> > > > It would indeed have that effect. The problem is that the test
> > > > requires the wrong person to add such support. A developer who adds
> > > > a new operator should be under no obligation to do anything with
> > > > AMP.
> > > > 
> > > > Having it as warning both in runtime and in the test has the effect
> > > > to prompt people who actually use and care about the performance
> > > > from AMP to take the steps and decide where a new operator fits.
> > > > And like you said, "It is up to people like me actively working on
> > > > better FP16 support to go through the FP32 list to introduce
> > > > support and then move such operator to another list." This is also
> > > > exactly how it should work with AMP.
> > > > 
> > > > -sz
> > > > 
> > > > On 2019/05/29 18:25:46, Przemys��aw Tr��dak <pt...@apache.org>
> > > > wrote: 
> > > > > Thank you all for responding.
> > > > > 
> > > > > Let me address a few misunderstandings about AMP in the
> > > > > discussion so far:
> > > > > - I think the main misunderstanding in the discussion so far is
> > > > > that it would be somehow possible to implicitly cast to FP32 all
> > > > > previously unseen operators. I would much prefer such solution
> > > > > myself, however this is actually (unfortunately) not really
> > > > > possible. The technical reason for that is the only place from
> > > > > which one can get MXNet operators is MXListAllOps call, which
> > > > > gives back all operators without any way of differentiating them,
> > > > > including for example calls to optimizers. If somebody added a
> > > > > new optimizer and AMP inserts a FP32 cast before its input, the
> > > > > optimizer would actually update the FP32 copy of the tensor
> > > > > instead of the tensor itself. This is not a performance problem
> > > > > (which, I agree, would be perfectly fine to solve later) - it is
> > > > > a correctness problem. That is why the error message in the test
> > > > > explicitly mentions this case as one where the operator should be
> > > > > put in the list that does not make any casts.
> > > > > - The second misunderstanding is that the ask from the test is to
> > > > > somehow "add support" for AMP in the operator. That is definitely
> > > > > not true and adding a single line in either FP32_FUNCS (cast me
> > > > > always to FP32) or FP16_FP32_FUNCS (do not do anything with me
> > > > > because I'm not relevant for this usecase) is perfectly fine. It
> > > > > is up to people like me actively working on better FP16 support
> > > > > to go through the FP32 list to introduce support and then move
> > > > > such operator to another list.
> > > > > - The third (although minor) misunderstanding is that this
> > > > > feature is completely GPU-specific. It is not, and actually the
> > > > > review comment that prompted me to introduce the design as is
> > > > > currently implemented comes from MKLDNN developer :-).
> > > > > 
> > > > > About the proposal to move it from Python list into a operator
> > > > > property - I am in favor of it and, as Anirudh mentioned, this
> > > > > should make it much easier when just adding aliases to the
> > > > > operators. But again, this could not be an optional parameter
> > > > > unfortunately since automatic "put me in FP32 list" is not
> > > > > possible.
> > > > > About the proposal to change the error into a warning and then
> > > > > rely on people interested in AMP to adding operators to AMP lists
> > > > > - I do not like this proposal because of user experience that it
> > > > > would introduce. Basically what would happen is a user who
> > > > > downloads nightly build of MXNet (because they needed a feature
> > > > > introduced after last official release) would likely find that
> > > > > AMP is broken in that build because it happened between updates
> > > > > to the AMP lists. The test ensures that the list is updated at
> > > > > the same time as new operator is introduced. This is a question
> > > > > really of how do we treat nightly releases - should the user
> > > > > expect for all the features to generally work in them or not? If
> > > > > the Community thinks that it is acceptable outcome then I am fine
> > > > > with the proposal and regularly updating those lists myself.
> > > > > 
> > > > > Thank you
> > > > > Przemek
> > > > > 
> > > > > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org>
> > > > > wrote: 
> > > > > > Dear Community,
> > > > > > 
> > > > > > One of the recently merged features of the 1.5 release, AMP
> > > > > > (Automatic Mixed Precision) support (PR [1], design doc [5]),
> > > > > > introduced a requirement that every new operator added to MXNet
> > > > > > would need to be present in 1 of the lists (in [2]). To make
> > > > > > sure that this requirement is not broken when somebody adds a
> > > > > > new operator and does not know about AMP's existence, a test
> > > > > > was added to CI ([3]).
> > > > > > 
> > > > > > A few people reached out to me (the original author of the
> > > > > > feature) saying this test increases a burden on a developer of
> > > > > > new operators and should not be an actual error, but just
> > > > > > warning (PR for that change [4]). That is why I would like to
> > > > > > present a motivation for it and discuss with the wider audience
> > > > > > why I feel it was necessary.
> > > > > > 
> > > > > > First, for people who do not know the details of what AMP is -
> > > > > > it is a solution that tries to automatically apply best
> > > > > > practices of training in lower precision (FP16) to user's FP32
> > > > > > model in order to fully utilize capabilities of modern GPUs
> > > > > > (and potentially other hardware in the future). It does so by
> > > > > > casting to lower precision inputs to operators benefitting from
> > > > > > it, while casting to full precision inputs of operators that
> > > > > > are unsafe to run in lower precision or just do not support it.
> > > > > > 
> > > > > > The first iteration of AMP kept 2 main lists of operators -
> > > > > > operators that are beneficial and safe to do in fp16 and
> > > > > > operators that need to be cast to FP32. The problem (raised in
> > > > > > review of the PR [6], [8]) is how to make sure that the feature
> > > > > > works as intended and is not inadvertently broken by somebody
> > > > > > adding a new operator. The failure scenario here is adding a
> > > > > > new operator that does not support FP16 and so should be cast
> > > > > > to FP32, but AMP does not know about its existence and so does
> > > > > > not do the casting. The solution proposed in the review was to
> > > > > > implicitly treat all of the unknown operators as FP32-only and
> > > > > > keep the list of operators that work fine in both FP16 and
> > > > > > FP32. This solution however does not really work, because there
> > > > > > are multiple operators (most notably optimizers) where
> > > > > > introducing additional casting of the input to FP32 would break
> > > > > > the operator.
> > > > > > 
> > > > > > That is why after discussion with a few members of the
> > > > > > community, I decided to proceed with all lists being explicit
> > > > > > and introducing the test that would fail when somebody added an
> > > > > > operator without classifying it into 1 of the categories, and
> > > > > > explain clearly how to do it [7]. It is not ideal solution, as
> > > > > > it introduces some burden on the developers who are not aware
> > > > > > about AMP, however in the typical case of adding at most a few
> > > > > > operators to MXNet the inconvenience is I think pretty minor
> > > > > > while important for the feature correctness going forward.
> > > > > > 
> > > > > > I would like to gather Community feedback and ideas how to
> > > > > > handle this situation.
> > > > > > 
> > > > > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > > > > [2] 
> > > > > > https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > > > > [3] 
> > > > > > https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > > > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > > > > [5] 
> > > > > > https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > > > > [6] 
> > > > > > https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > > > > [7] 
> > > > > > https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > > > > [8] 
> > > > > > https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > > > > > 
> > > 
> > > 
> > 
> 

Re: Making new operators and AMP lists

[Przemys��aw Tr��dak <pt...@apache.org>]

Hi Sam and Zhennan, 

the problem is not how to implicitly produce a list of all operators not in any other list - that is easy and the code Zhennan provided would work. The problem is that such list would not be actually correct in all cases - you do NOT want optimizers to land in FP32_FUNCS list, because then the tensor being updated could potentially be the FP32 copy and not the actual tensor you care about. That is why the first bullet point in the error message in the test is along the lines of: "if you do an optimizer or anything else that does not go in the computational graph, put it in the FP16_FP32_FUNCS list" - that list does not do any wrapping.

HOWEVER, as I was writing this reply I realized that due to pure luck this is not actually what happens - optimizers could in fact be in the FP32_FUNCS list. That is because, as AMP's assumption is that the model being changed is FP32 model at the start, all weights (and so all the gradients just before application as well) are stored in FP32 already (and so the casting would not actually occur). It would therefore be possible to make the FP32 list to be implicit (although relying on this lucky coincidence makes me slightly uncomfortable).

Przemek 



On 2019/05/30 08:04:05, "Qin, Zhennan" <zh...@intel.com> wrote: 
> How about change the below line in amp.py:164
> 
> wrap_list = fp32_ops if fp32_ops is not None else
> lists.symbol.FP32_FUNCS
> 
> to be
> 
>     plist = ctypes.POINTER(ctypes.c_char_p)()
>     size = ctypes.c_uint()
> 
>     check_call(_LIB.MXListAllOpNames(ctypes.byref(size),
>                                      ctypes.byref(plist)))
>     op_names = []
>     for i in range(size.value):
>         op_names.append(py_str(plist[i]))
> 
>     wrap_list = []
>     fp16_op_list = lists.symbol.FP16_FUNCS +
> lists.symbol.WIDEST_TYPE_CASTS + lists.symbol.FP16_FP32_FUNCS +
> list(map(lambda x: x[0], lists.symbol.CONDITIONAL_FP32_FUNCS))
>     for op_name in op_names:
>         if not op_name.startswith("_backward_") and not
> op_name.startswith("_contrib_backward_") and op_name not in
> fp16_op_list:
>             wrap_list.append(op_name)
>     wrap_list = fp32_ops if fp32_ops is not None else wrap_list
> 
> 
> I've checked, the changed code can produce identity wrap_list as
> lists.symbol.FP16_FP32_FUNCS.
> 
> The check code is,
> 
>     print("op that in wrap_list but not in FP32_FUNCS:")
>     for i in wrap_list:
>         if i not in lists.symbol.FP32_FUNCS:
>             print(i)
> 
>     print("op that in FP32_FUNCS but not in wrap_list:")
>     for i in lists.symbol.FP32_FUNCS:
>         if i not in wrap_list:
>             print(i)
> 
> The output is,
> 
> op that in infered_fp32_op_list but not in FP32_FUNCS:
> op that in FP32_FUNCS but not in infered_fp32_op_list:
> op that in infered_fp32_op_list but not in FP32_FUNCS:
> op that in FP32_FUNCS but not in infered_fp32_op_list:
> 
> This change should be able to automatically set new operator as fp32
> only.
> 
> Thanks,
> Zhennan
> 
> On Wed, 2019-05-29 at 19:13 +0000, Skalicky, Sam wrote:
> > Thanks Przemek for the additional explanation, but I’m still confused
> > on this part. I don’t understand the explanation of the optimizer’s
> > interaction here. 
> > 
> > > > The technical reason for that is the only place from which one
> > > > can get MXNet operators is MXListAllOps call, which gives back
> > > > all operators without any way of differentiating them, including
> > > > for example calls to optimizers. If somebody added a new
> > > > optimizer and AMP inserts a FP32 cast before its input, the
> > > > optimizer would actually update the FP32 copy of the tensor
> > > > instead of the tensor itself. This is not a performance problem
> > > > (which, I agree, would be perfectly fine to solve later) - it is
> > > > a correctness problem.
> > > > That is why the error message in the test explicitly mentions
> > > > this case as one where the operator should be put in the list
> > > > that does not make any casts.
> > 
> > 
> > If this is third list that Anirudh mentions:
> > 
> > > > 3. Not sure, Don't know or dont care: FP32 list.
> > 
> > 
> > and this list simply contains all operators that have not been
> > categorized, why do we need the list at all? 
> > 
> > Thanks,
> > Sam
> > 
> > 
> > > On May 29, 2019, at 11:58 AM, Sheng Zha <zh...@apache.org>
> > > wrote:
> > > 
> > > > The second misunderstanding is that the ask from the test is to
> > > > somehow "add support" for AMP in the operator. That is definitely
> > > > not true and adding a single line in either FP32_FUNCS (cast me
> > > > always to FP32) or FP16_FP32_FUNCS (do not do anything with me
> > > > because I'm not relevant for this usecase) is perfectly fine.
> > > 
> > > I respectfully disagree with this statement. In this case, the
> > > "support to add" is exactly to differentiate how the new operator
> > > should be treated. It was not there before, so it's new. And
> > > requiring other developers to add any line is totally not fine.
> > > 
> > > > The test ensures that the list is updated at the same time as new
> > > > operator is introduced.
> > > 
> > > It would indeed have that effect. The problem is that the test
> > > requires the wrong person to add such support. A developer who adds
> > > a new operator should be under no obligation to do anything with
> > > AMP.
> > > 
> > > Having it as warning both in runtime and in the test has the effect
> > > to prompt people who actually use and care about the performance
> > > from AMP to take the steps and decide where a new operator fits.
> > > And like you said, "It is up to people like me actively working on
> > > better FP16 support to go through the FP32 list to introduce
> > > support and then move such operator to another list." This is also
> > > exactly how it should work with AMP.
> > > 
> > > -sz
> > > 
> > > On 2019/05/29 18:25:46, Przemys��aw Tr��dak <pt...@apache.org>
> > > wrote: 
> > > > Thank you all for responding.
> > > > 
> > > > Let me address a few misunderstandings about AMP in the
> > > > discussion so far:
> > > > - I think the main misunderstanding in the discussion so far is
> > > > that it would be somehow possible to implicitly cast to FP32 all
> > > > previously unseen operators. I would much prefer such solution
> > > > myself, however this is actually (unfortunately) not really
> > > > possible. The technical reason for that is the only place from
> > > > which one can get MXNet operators is MXListAllOps call, which
> > > > gives back all operators without any way of differentiating them,
> > > > including for example calls to optimizers. If somebody added a
> > > > new optimizer and AMP inserts a FP32 cast before its input, the
> > > > optimizer would actually update the FP32 copy of the tensor
> > > > instead of the tensor itself. This is not a performance problem
> > > > (which, I agree, would be perfectly fine to solve later) - it is
> > > > a correctness problem. That is why the error message in the test
> > > > explicitly mentions this case as one where the operator should be
> > > > put in the list that does not make any casts.
> > > > - The second misunderstanding is that the ask from the test is to
> > > > somehow "add support" for AMP in the operator. That is definitely
> > > > not true and adding a single line in either FP32_FUNCS (cast me
> > > > always to FP32) or FP16_FP32_FUNCS (do not do anything with me
> > > > because I'm not relevant for this usecase) is perfectly fine. It
> > > > is up to people like me actively working on better FP16 support
> > > > to go through the FP32 list to introduce support and then move
> > > > such operator to another list.
> > > > - The third (although minor) misunderstanding is that this
> > > > feature is completely GPU-specific. It is not, and actually the
> > > > review comment that prompted me to introduce the design as is
> > > > currently implemented comes from MKLDNN developer :-).
> > > > 
> > > > About the proposal to move it from Python list into a operator
> > > > property - I am in favor of it and, as Anirudh mentioned, this
> > > > should make it much easier when just adding aliases to the
> > > > operators. But again, this could not be an optional parameter
> > > > unfortunately since automatic "put me in FP32 list" is not
> > > > possible.
> > > > About the proposal to change the error into a warning and then
> > > > rely on people interested in AMP to adding operators to AMP lists
> > > > - I do not like this proposal because of user experience that it
> > > > would introduce. Basically what would happen is a user who
> > > > downloads nightly build of MXNet (because they needed a feature
> > > > introduced after last official release) would likely find that
> > > > AMP is broken in that build because it happened between updates
> > > > to the AMP lists. The test ensures that the list is updated at
> > > > the same time as new operator is introduced. This is a question
> > > > really of how do we treat nightly releases - should the user
> > > > expect for all the features to generally work in them or not? If
> > > > the Community thinks that it is acceptable outcome then I am fine
> > > > with the proposal and regularly updating those lists myself.
> > > > 
> > > > Thank you
> > > > Przemek
> > > > 
> > > > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org>
> > > > wrote: 
> > > > > Dear Community,
> > > > > 
> > > > > One of the recently merged features of the 1.5 release, AMP
> > > > > (Automatic Mixed Precision) support (PR [1], design doc [5]),
> > > > > introduced a requirement that every new operator added to MXNet
> > > > > would need to be present in 1 of the lists (in [2]). To make
> > > > > sure that this requirement is not broken when somebody adds a
> > > > > new operator and does not know about AMP's existence, a test
> > > > > was added to CI ([3]).
> > > > > 
> > > > > A few people reached out to me (the original author of the
> > > > > feature) saying this test increases a burden on a developer of
> > > > > new operators and should not be an actual error, but just
> > > > > warning (PR for that change [4]). That is why I would like to
> > > > > present a motivation for it and discuss with the wider audience
> > > > > why I feel it was necessary.
> > > > > 
> > > > > First, for people who do not know the details of what AMP is -
> > > > > it is a solution that tries to automatically apply best
> > > > > practices of training in lower precision (FP16) to user's FP32
> > > > > model in order to fully utilize capabilities of modern GPUs
> > > > > (and potentially other hardware in the future). It does so by
> > > > > casting to lower precision inputs to operators benefitting from
> > > > > it, while casting to full precision inputs of operators that
> > > > > are unsafe to run in lower precision or just do not support it.
> > > > > 
> > > > > The first iteration of AMP kept 2 main lists of operators -
> > > > > operators that are beneficial and safe to do in fp16 and
> > > > > operators that need to be cast to FP32. The problem (raised in
> > > > > review of the PR [6], [8]) is how to make sure that the feature
> > > > > works as intended and is not inadvertently broken by somebody
> > > > > adding a new operator. The failure scenario here is adding a
> > > > > new operator that does not support FP16 and so should be cast
> > > > > to FP32, but AMP does not know about its existence and so does
> > > > > not do the casting. The solution proposed in the review was to
> > > > > implicitly treat all of the unknown operators as FP32-only and
> > > > > keep the list of operators that work fine in both FP16 and
> > > > > FP32. This solution however does not really work, because there
> > > > > are multiple operators (most notably optimizers) where
> > > > > introducing additional casting of the input to FP32 would break
> > > > > the operator.
> > > > > 
> > > > > That is why after discussion with a few members of the
> > > > > community, I decided to proceed with all lists being explicit
> > > > > and introducing the test that would fail when somebody added an
> > > > > operator without classifying it into 1 of the categories, and
> > > > > explain clearly how to do it [7]. It is not ideal solution, as
> > > > > it introduces some burden on the developers who are not aware
> > > > > about AMP, however in the typical case of adding at most a few
> > > > > operators to MXNet the inconvenience is I think pretty minor
> > > > > while important for the feature correctness going forward.
> > > > > 
> > > > > I would like to gather Community feedback and ideas how to
> > > > > handle this situation.
> > > > > 
> > > > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > > > [2] 
> > > > > https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > > > [3] 
> > > > > https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > > > [5] 
> > > > > https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > > > [6] 
> > > > > https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > > > [7] 
> > > > > https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > > > [8] 
> > > > > https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > > > > 
> > 
> > 
> 

Re: Making new operators and AMP lists

["Qin, Zhennan" <zh...@intel.com>]

How about change the below line in amp.py:164

wrap_list = fp32_ops if fp32_ops is not None else
lists.symbol.FP32_FUNCS

to be

    plist = ctypes.POINTER(ctypes.c_char_p)()
    size = ctypes.c_uint()

    check_call(_LIB.MXListAllOpNames(ctypes.byref(size),
                                     ctypes.byref(plist)))
    op_names = []
    for i in range(size.value):
        op_names.append(py_str(plist[i]))

    wrap_list = []
    fp16_op_list = lists.symbol.FP16_FUNCS +
lists.symbol.WIDEST_TYPE_CASTS + lists.symbol.FP16_FP32_FUNCS +
list(map(lambda x: x[0], lists.symbol.CONDITIONAL_FP32_FUNCS))
    for op_name in op_names:
        if not op_name.startswith("_backward_") and not
op_name.startswith("_contrib_backward_") and op_name not in
fp16_op_list:
            wrap_list.append(op_name)
    wrap_list = fp32_ops if fp32_ops is not None else wrap_list


I've checked, the changed code can produce identity wrap_list as
lists.symbol.FP16_FP32_FUNCS.

The check code is,

    print("op that in wrap_list but not in FP32_FUNCS:")
    for i in wrap_list:
        if i not in lists.symbol.FP32_FUNCS:
            print(i)

    print("op that in FP32_FUNCS but not in wrap_list:")
    for i in lists.symbol.FP32_FUNCS:
        if i not in wrap_list:
            print(i)

The output is,

op that in infered_fp32_op_list but not in FP32_FUNCS:
op that in FP32_FUNCS but not in infered_fp32_op_list:
op that in infered_fp32_op_list but not in FP32_FUNCS:
op that in FP32_FUNCS but not in infered_fp32_op_list:

This change should be able to automatically set new operator as fp32
only.

Thanks,
Zhennan

On Wed, 2019-05-29 at 19:13 +0000, Skalicky, Sam wrote:
> Thanks Przemek for the additional explanation, but I’m still confused
> on this part. I don’t understand the explanation of the optimizer’s
> interaction here. 
> 
> > > The technical reason for that is the only place from which one
> > > can get MXNet operators is MXListAllOps call, which gives back
> > > all operators without any way of differentiating them, including
> > > for example calls to optimizers. If somebody added a new
> > > optimizer and AMP inserts a FP32 cast before its input, the
> > > optimizer would actually update the FP32 copy of the tensor
> > > instead of the tensor itself. This is not a performance problem
> > > (which, I agree, would be perfectly fine to solve later) - it is
> > > a correctness problem.
> > > That is why the error message in the test explicitly mentions
> > > this case as one where the operator should be put in the list
> > > that does not make any casts.
> 
> 
> If this is third list that Anirudh mentions:
> 
> > > 3. Not sure, Don't know or dont care: FP32 list.
> 
> 
> and this list simply contains all operators that have not been
> categorized, why do we need the list at all? 
> 
> Thanks,
> Sam
> 
> 
> > On May 29, 2019, at 11:58 AM, Sheng Zha <zh...@apache.org>
> > wrote:
> > 
> > > The second misunderstanding is that the ask from the test is to
> > > somehow "add support" for AMP in the operator. That is definitely
> > > not true and adding a single line in either FP32_FUNCS (cast me
> > > always to FP32) or FP16_FP32_FUNCS (do not do anything with me
> > > because I'm not relevant for this usecase) is perfectly fine.
> > 
> > I respectfully disagree with this statement. In this case, the
> > "support to add" is exactly to differentiate how the new operator
> > should be treated. It was not there before, so it's new. And
> > requiring other developers to add any line is totally not fine.
> > 
> > > The test ensures that the list is updated at the same time as new
> > > operator is introduced.
> > 
> > It would indeed have that effect. The problem is that the test
> > requires the wrong person to add such support. A developer who adds
> > a new operator should be under no obligation to do anything with
> > AMP.
> > 
> > Having it as warning both in runtime and in the test has the effect
> > to prompt people who actually use and care about the performance
> > from AMP to take the steps and decide where a new operator fits.
> > And like you said, "It is up to people like me actively working on
> > better FP16 support to go through the FP32 list to introduce
> > support and then move such operator to another list." This is also
> > exactly how it should work with AMP.
> > 
> > -sz
> > 
> > On 2019/05/29 18:25:46, Przemys��aw Tr��dak <pt...@apache.org>
> > wrote: 
> > > Thank you all for responding.
> > > 
> > > Let me address a few misunderstandings about AMP in the
> > > discussion so far:
> > > - I think the main misunderstanding in the discussion so far is
> > > that it would be somehow possible to implicitly cast to FP32 all
> > > previously unseen operators. I would much prefer such solution
> > > myself, however this is actually (unfortunately) not really
> > > possible. The technical reason for that is the only place from
> > > which one can get MXNet operators is MXListAllOps call, which
> > > gives back all operators without any way of differentiating them,
> > > including for example calls to optimizers. If somebody added a
> > > new optimizer and AMP inserts a FP32 cast before its input, the
> > > optimizer would actually update the FP32 copy of the tensor
> > > instead of the tensor itself. This is not a performance problem
> > > (which, I agree, would be perfectly fine to solve later) - it is
> > > a correctness problem. That is why the error message in the test
> > > explicitly mentions this case as one where the operator should be
> > > put in the list that does not make any casts.
> > > - The second misunderstanding is that the ask from the test is to
> > > somehow "add support" for AMP in the operator. That is definitely
> > > not true and adding a single line in either FP32_FUNCS (cast me
> > > always to FP32) or FP16_FP32_FUNCS (do not do anything with me
> > > because I'm not relevant for this usecase) is perfectly fine. It
> > > is up to people like me actively working on better FP16 support
> > > to go through the FP32 list to introduce support and then move
> > > such operator to another list.
> > > - The third (although minor) misunderstanding is that this
> > > feature is completely GPU-specific. It is not, and actually the
> > > review comment that prompted me to introduce the design as is
> > > currently implemented comes from MKLDNN developer :-).
> > > 
> > > About the proposal to move it from Python list into a operator
> > > property - I am in favor of it and, as Anirudh mentioned, this
> > > should make it much easier when just adding aliases to the
> > > operators. But again, this could not be an optional parameter
> > > unfortunately since automatic "put me in FP32 list" is not
> > > possible.
> > > About the proposal to change the error into a warning and then
> > > rely on people interested in AMP to adding operators to AMP lists
> > > - I do not like this proposal because of user experience that it
> > > would introduce. Basically what would happen is a user who
> > > downloads nightly build of MXNet (because they needed a feature
> > > introduced after last official release) would likely find that
> > > AMP is broken in that build because it happened between updates
> > > to the AMP lists. The test ensures that the list is updated at
> > > the same time as new operator is introduced. This is a question
> > > really of how do we treat nightly releases - should the user
> > > expect for all the features to generally work in them or not? If
> > > the Community thinks that it is acceptable outcome then I am fine
> > > with the proposal and regularly updating those lists myself.
> > > 
> > > Thank you
> > > Przemek
> > > 
> > > On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org>
> > > wrote: 
> > > > Dear Community,
> > > > 
> > > > One of the recently merged features of the 1.5 release, AMP
> > > > (Automatic Mixed Precision) support (PR [1], design doc [5]),
> > > > introduced a requirement that every new operator added to MXNet
> > > > would need to be present in 1 of the lists (in [2]). To make
> > > > sure that this requirement is not broken when somebody adds a
> > > > new operator and does not know about AMP's existence, a test
> > > > was added to CI ([3]).
> > > > 
> > > > A few people reached out to me (the original author of the
> > > > feature) saying this test increases a burden on a developer of
> > > > new operators and should not be an actual error, but just
> > > > warning (PR for that change [4]). That is why I would like to
> > > > present a motivation for it and discuss with the wider audience
> > > > why I feel it was necessary.
> > > > 
> > > > First, for people who do not know the details of what AMP is -
> > > > it is a solution that tries to automatically apply best
> > > > practices of training in lower precision (FP16) to user's FP32
> > > > model in order to fully utilize capabilities of modern GPUs
> > > > (and potentially other hardware in the future). It does so by
> > > > casting to lower precision inputs to operators benefitting from
> > > > it, while casting to full precision inputs of operators that
> > > > are unsafe to run in lower precision or just do not support it.
> > > > 
> > > > The first iteration of AMP kept 2 main lists of operators -
> > > > operators that are beneficial and safe to do in fp16 and
> > > > operators that need to be cast to FP32. The problem (raised in
> > > > review of the PR [6], [8]) is how to make sure that the feature
> > > > works as intended and is not inadvertently broken by somebody
> > > > adding a new operator. The failure scenario here is adding a
> > > > new operator that does not support FP16 and so should be cast
> > > > to FP32, but AMP does not know about its existence and so does
> > > > not do the casting. The solution proposed in the review was to
> > > > implicitly treat all of the unknown operators as FP32-only and
> > > > keep the list of operators that work fine in both FP16 and
> > > > FP32. This solution however does not really work, because there
> > > > are multiple operators (most notably optimizers) where
> > > > introducing additional casting of the input to FP32 would break
> > > > the operator.
> > > > 
> > > > That is why after discussion with a few members of the
> > > > community, I decided to proceed with all lists being explicit
> > > > and introducing the test that would fail when somebody added an
> > > > operator without classifying it into 1 of the categories, and
> > > > explain clearly how to do it [7]. It is not ideal solution, as
> > > > it introduces some burden on the developers who are not aware
> > > > about AMP, however in the typical case of adding at most a few
> > > > operators to MXNet the inconvenience is I think pretty minor
> > > > while important for the feature correctness going forward.
> > > > 
> > > > I would like to gather Community feedback and ideas how to
> > > > handle this situation.
> > > > 
> > > > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > > > [2] 
> > > > https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > > > [3] 
> > > > https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > > > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > > > [5] 
> > > > https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > > > [6] 
> > > > https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > > > [7] 
> > > > https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > > > [8] 
> > > > https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > > > 
> 
> 

Re: Making new operators and AMP lists

["Skalicky, Sam" <ss...@amazon.com.INVALID>]

Thanks Przemek for the additional explanation, but I’m still confused on this part. I don’t understand the explanation of the optimizer’s interaction here. 

>> The technical reason for that is the only place from which one can get MXNet operators is MXListAllOps call, which gives back all operators without any way of differentiating them, including for example calls to optimizers. If somebody added a new optimizer and AMP inserts a FP32 cast before its input, the optimizer would actually update the FP32 copy of the tensor instead of the tensor itself. This is not a performance problem (which, I agree, would be perfectly fine to solve later) - it is a correctness problem.

>> That is why the error message in the test explicitly mentions this case as one where the operator should be put in the list that does not make any casts.


If this is third list that Anirudh mentions:

>> 3. Not sure, Don't know or dont care: FP32 list.


and this list simply contains all operators that have not been categorized, why do we need the list at all? 

Thanks,
Sam


> On May 29, 2019, at 11:58 AM, Sheng Zha <zh...@apache.org> wrote:
> 
>> The second misunderstanding is that the ask from the test is to somehow "add support" for AMP in the operator. That is definitely not true and adding a single line in either FP32_FUNCS (cast me always to FP32) or FP16_FP32_FUNCS (do not do anything with me because I'm not relevant for this usecase) is perfectly fine.
> 
> I respectfully disagree with this statement. In this case, the "support to add" is exactly to differentiate how the new operator should be treated. It was not there before, so it's new. And requiring other developers to add any line is totally not fine.
> 
>> The test ensures that the list is updated at the same time as new operator is introduced.
> 
> It would indeed have that effect. The problem is that the test requires the wrong person to add such support. A developer who adds a new operator should be under no obligation to do anything with AMP.
> 
> Having it as warning both in runtime and in the test has the effect to prompt people who actually use and care about the performance from AMP to take the steps and decide where a new operator fits. And like you said, "It is up to people like me actively working on better FP16 support to go through the FP32 list to introduce support and then move such operator to another list." This is also exactly how it should work with AMP.
> 
> -sz
> 
> On 2019/05/29 18:25:46, Przemys��aw Tr��dak <pt...@apache.org> wrote: 
>> Thank you all for responding.
>> 
>> Let me address a few misunderstandings about AMP in the discussion so far:
>> - I think the main misunderstanding in the discussion so far is that it would be somehow possible to implicitly cast to FP32 all previously unseen operators. I would much prefer such solution myself, however this is actually (unfortunately) not really possible. The technical reason for that is the only place from which one can get MXNet operators is MXListAllOps call, which gives back all operators without any way of differentiating them, including for example calls to optimizers. If somebody added a new optimizer and AMP inserts a FP32 cast before its input, the optimizer would actually update the FP32 copy of the tensor instead of the tensor itself. This is not a performance problem (which, I agree, would be perfectly fine to solve later) - it is a correctness problem. That is why the error message in the test explicitly mentions this case as one where the operator should be put in the list that does not make any casts.
>> - The second misunderstanding is that the ask from the test is to somehow "add support" for AMP in the operator. That is definitely not true and adding a single line in either FP32_FUNCS (cast me always to FP32) or FP16_FP32_FUNCS (do not do anything with me because I'm not relevant for this usecase) is perfectly fine. It is up to people like me actively working on better FP16 support to go through the FP32 list to introduce support and then move such operator to another list.
>> - The third (although minor) misunderstanding is that this feature is completely GPU-specific. It is not, and actually the review comment that prompted me to introduce the design as is currently implemented comes from MKLDNN developer :-).
>> 
>> About the proposal to move it from Python list into a operator property - I am in favor of it and, as Anirudh mentioned, this should make it much easier when just adding aliases to the operators. But again, this could not be an optional parameter unfortunately since automatic "put me in FP32 list" is not possible.
>> About the proposal to change the error into a warning and then rely on people interested in AMP to adding operators to AMP lists - I do not like this proposal because of user experience that it would introduce. Basically what would happen is a user who downloads nightly build of MXNet (because they needed a feature introduced after last official release) would likely find that AMP is broken in that build because it happened between updates to the AMP lists. The test ensures that the list is updated at the same time as new operator is introduced. This is a question really of how do we treat nightly releases - should the user expect for all the features to generally work in them or not? If the Community thinks that it is acceptable outcome then I am fine with the proposal and regularly updating those lists myself.
>> 
>> Thank you
>> Przemek
>> 
>> On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org> wrote: 
>>> Dear Community,
>>> 
>>> One of the recently merged features of the 1.5 release, AMP (Automatic Mixed Precision) support (PR [1], design doc [5]), introduced a requirement that every new operator added to MXNet would need to be present in 1 of the lists (in [2]). To make sure that this requirement is not broken when somebody adds a new operator and does not know about AMP's existence, a test was added to CI ([3]).
>>> 
>>> A few people reached out to me (the original author of the feature) saying this test increases a burden on a developer of new operators and should not be an actual error, but just warning (PR for that change [4]). That is why I would like to present a motivation for it and discuss with the wider audience why I feel it was necessary.
>>> 
>>> First, for people who do not know the details of what AMP is - it is a solution that tries to automatically apply best practices of training in lower precision (FP16) to user's FP32 model in order to fully utilize capabilities of modern GPUs (and potentially other hardware in the future). It does so by casting to lower precision inputs to operators benefitting from it, while casting to full precision inputs of operators that are unsafe to run in lower precision or just do not support it.
>>> 
>>> The first iteration of AMP kept 2 main lists of operators - operators that are beneficial and safe to do in fp16 and operators that need to be cast to FP32. The problem (raised in review of the PR [6], [8]) is how to make sure that the feature works as intended and is not inadvertently broken by somebody adding a new operator. The failure scenario here is adding a new operator that does not support FP16 and so should be cast to FP32, but AMP does not know about its existence and so does not do the casting. The solution proposed in the review was to implicitly treat all of the unknown operators as FP32-only and keep the list of operators that work fine in both FP16 and FP32. This solution however does not really work, because there are multiple operators (most notably optimizers) where introducing additional casting of the input to FP32 would break the operator.
>>> 
>>> That is why after discussion with a few members of the community, I decided to proceed with all lists being explicit and introducing the test that would fail when somebody added an operator without classifying it into 1 of the categories, and explain clearly how to do it [7]. It is not ideal solution, as it introduces some burden on the developers who are not aware about AMP, however in the typical case of adding at most a few operators to MXNet the inconvenience is I think pretty minor while important for the feature correctness going forward.
>>> 
>>> I would like to gather Community feedback and ideas how to handle this situation.
>>> 
>>> [1] https://github.com/apache/incubator-mxnet/pull/14173
>>> [2] https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
>>> [3] https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
>>> [4] https://github.com/apache/incubator-mxnet/pull/15085
>>> [5] https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
>>> [6] https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
>>> [7] https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
>>> [8] https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
>>> 
>> 

Re: Making new operators and AMP lists

[Sheng Zha <zh...@apache.org>]

> The second misunderstanding is that the ask from the test is to somehow "add support" for AMP in the operator. That is definitely not true and adding a single line in either FP32_FUNCS (cast me always to FP32) or FP16_FP32_FUNCS (do not do anything with me because I'm not relevant for this usecase) is perfectly fine.

I respectfully disagree with this statement. In this case, the "support to add" is exactly to differentiate how the new operator should be treated. It was not there before, so it's new. And requiring other developers to add any line is totally not fine.

> The test ensures that the list is updated at the same time as new operator is introduced.

It would indeed have that effect. The problem is that the test requires the wrong person to add such support. A developer who adds a new operator should be under no obligation to do anything with AMP.

Having it as warning both in runtime and in the test has the effect to prompt people who actually use and care about the performance from AMP to take the steps and decide where a new operator fits. And like you said, "It is up to people like me actively working on better FP16 support to go through the FP32 list to introduce support and then move such operator to another list." This is also exactly how it should work with AMP.

-sz

On 2019/05/29 18:25:46, Przemys��aw Tr��dak <pt...@apache.org> wrote: 
> Thank you all for responding.
> 
> Let me address a few misunderstandings about AMP in the discussion so far:
>  - I think the main misunderstanding in the discussion so far is that it would be somehow possible to implicitly cast to FP32 all previously unseen operators. I would much prefer such solution myself, however this is actually (unfortunately) not really possible. The technical reason for that is the only place from which one can get MXNet operators is MXListAllOps call, which gives back all operators without any way of differentiating them, including for example calls to optimizers. If somebody added a new optimizer and AMP inserts a FP32 cast before its input, the optimizer would actually update the FP32 copy of the tensor instead of the tensor itself. This is not a performance problem (which, I agree, would be perfectly fine to solve later) - it is a correctness problem. That is why the error message in the test explicitly mentions this case as one where the operator should be put in the list that does not make any casts.
>  - The second misunderstanding is that the ask from the test is to somehow "add support" for AMP in the operator. That is definitely not true and adding a single line in either FP32_FUNCS (cast me always to FP32) or FP16_FP32_FUNCS (do not do anything with me because I'm not relevant for this usecase) is perfectly fine. It is up to people like me actively working on better FP16 support to go through the FP32 list to introduce support and then move such operator to another list.
>  - The third (although minor) misunderstanding is that this feature is completely GPU-specific. It is not, and actually the review comment that prompted me to introduce the design as is currently implemented comes from MKLDNN developer :-).
> 
> About the proposal to move it from Python list into a operator property - I am in favor of it and, as Anirudh mentioned, this should make it much easier when just adding aliases to the operators. But again, this could not be an optional parameter unfortunately since automatic "put me in FP32 list" is not possible.
> About the proposal to change the error into a warning and then rely on people interested in AMP to adding operators to AMP lists - I do not like this proposal because of user experience that it would introduce. Basically what would happen is a user who downloads nightly build of MXNet (because they needed a feature introduced after last official release) would likely find that AMP is broken in that build because it happened between updates to the AMP lists. The test ensures that the list is updated at the same time as new operator is introduced. This is a question really of how do we treat nightly releases - should the user expect for all the features to generally work in them or not? If the Community thinks that it is acceptable outcome then I am fine with the proposal and regularly updating those lists myself.
> 
> Thank you
> Przemek
> 
> On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org> wrote: 
> > Dear Community,
> > 
> > One of the recently merged features of the 1.5 release, AMP (Automatic Mixed Precision) support (PR [1], design doc [5]), introduced a requirement that every new operator added to MXNet would need to be present in 1 of the lists (in [2]). To make sure that this requirement is not broken when somebody adds a new operator and does not know about AMP's existence, a test was added to CI ([3]).
> > 
> > A few people reached out to me (the original author of the feature) saying this test increases a burden on a developer of new operators and should not be an actual error, but just warning (PR for that change [4]). That is why I would like to present a motivation for it and discuss with the wider audience why I feel it was necessary.
> > 
> > First, for people who do not know the details of what AMP is - it is a solution that tries to automatically apply best practices of training in lower precision (FP16) to user's FP32 model in order to fully utilize capabilities of modern GPUs (and potentially other hardware in the future). It does so by casting to lower precision inputs to operators benefitting from it, while casting to full precision inputs of operators that are unsafe to run in lower precision or just do not support it.
> > 
> > The first iteration of AMP kept 2 main lists of operators - operators that are beneficial and safe to do in fp16 and operators that need to be cast to FP32. The problem (raised in review of the PR [6], [8]) is how to make sure that the feature works as intended and is not inadvertently broken by somebody adding a new operator. The failure scenario here is adding a new operator that does not support FP16 and so should be cast to FP32, but AMP does not know about its existence and so does not do the casting. The solution proposed in the review was to implicitly treat all of the unknown operators as FP32-only and keep the list of operators that work fine in both FP16 and FP32. This solution however does not really work, because there are multiple operators (most notably optimizers) where introducing additional casting of the input to FP32 would break the operator.
> > 
> > That is why after discussion with a few members of the community, I decided to proceed with all lists being explicit and introducing the test that would fail when somebody added an operator without classifying it into 1 of the categories, and explain clearly how to do it [7]. It is not ideal solution, as it introduces some burden on the developers who are not aware about AMP, however in the typical case of adding at most a few operators to MXNet the inconvenience is I think pretty minor while important for the feature correctness going forward.
> > 
> > I would like to gather Community feedback and ideas how to handle this situation.
> > 
> > [1] https://github.com/apache/incubator-mxnet/pull/14173
> > [2] https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> > [3] https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> > [4] https://github.com/apache/incubator-mxnet/pull/15085
> > [5] https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> > [6] https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> > [7] https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> > [8] https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
> > 
> 

Re: Making new operators and AMP lists

[Przemys��aw Tr��dak <pt...@apache.org>]

Thank you all for responding.

Let me address a few misunderstandings about AMP in the discussion so far:
 - I think the main misunderstanding in the discussion so far is that it would be somehow possible to implicitly cast to FP32 all previously unseen operators. I would much prefer such solution myself, however this is actually (unfortunately) not really possible. The technical reason for that is the only place from which one can get MXNet operators is MXListAllOps call, which gives back all operators without any way of differentiating them, including for example calls to optimizers. If somebody added a new optimizer and AMP inserts a FP32 cast before its input, the optimizer would actually update the FP32 copy of the tensor instead of the tensor itself. This is not a performance problem (which, I agree, would be perfectly fine to solve later) - it is a correctness problem. That is why the error message in the test explicitly mentions this case as one where the operator should be put in the list that does not make any casts.
 - The second misunderstanding is that the ask from the test is to somehow "add support" for AMP in the operator. That is definitely not true and adding a single line in either FP32_FUNCS (cast me always to FP32) or FP16_FP32_FUNCS (do not do anything with me because I'm not relevant for this usecase) is perfectly fine. It is up to people like me actively working on better FP16 support to go through the FP32 list to introduce support and then move such operator to another list.
 - The third (although minor) misunderstanding is that this feature is completely GPU-specific. It is not, and actually the review comment that prompted me to introduce the design as is currently implemented comes from MKLDNN developer :-).

About the proposal to move it from Python list into a operator property - I am in favor of it and, as Anirudh mentioned, this should make it much easier when just adding aliases to the operators. But again, this could not be an optional parameter unfortunately since automatic "put me in FP32 list" is not possible.
About the proposal to change the error into a warning and then rely on people interested in AMP to adding operators to AMP lists - I do not like this proposal because of user experience that it would introduce. Basically what would happen is a user who downloads nightly build of MXNet (because they needed a feature introduced after last official release) would likely find that AMP is broken in that build because it happened between updates to the AMP lists. The test ensures that the list is updated at the same time as new operator is introduced. This is a question really of how do we treat nightly releases - should the user expect for all the features to generally work in them or not? If the Community thinks that it is acceptable outcome then I am fine with the proposal and regularly updating those lists myself.

Thank you
Przemek

On 2019/05/28 21:32:42, Przemys��aw Tr��dak <pt...@apache.org> wrote: 
> Dear Community,
> 
> One of the recently merged features of the 1.5 release, AMP (Automatic Mixed Precision) support (PR [1], design doc [5]), introduced a requirement that every new operator added to MXNet would need to be present in 1 of the lists (in [2]). To make sure that this requirement is not broken when somebody adds a new operator and does not know about AMP's existence, a test was added to CI ([3]).
> 
> A few people reached out to me (the original author of the feature) saying this test increases a burden on a developer of new operators and should not be an actual error, but just warning (PR for that change [4]). That is why I would like to present a motivation for it and discuss with the wider audience why I feel it was necessary.
> 
> First, for people who do not know the details of what AMP is - it is a solution that tries to automatically apply best practices of training in lower precision (FP16) to user's FP32 model in order to fully utilize capabilities of modern GPUs (and potentially other hardware in the future). It does so by casting to lower precision inputs to operators benefitting from it, while casting to full precision inputs of operators that are unsafe to run in lower precision or just do not support it.
> 
> The first iteration of AMP kept 2 main lists of operators - operators that are beneficial and safe to do in fp16 and operators that need to be cast to FP32. The problem (raised in review of the PR [6], [8]) is how to make sure that the feature works as intended and is not inadvertently broken by somebody adding a new operator. The failure scenario here is adding a new operator that does not support FP16 and so should be cast to FP32, but AMP does not know about its existence and so does not do the casting. The solution proposed in the review was to implicitly treat all of the unknown operators as FP32-only and keep the list of operators that work fine in both FP16 and FP32. This solution however does not really work, because there are multiple operators (most notably optimizers) where introducing additional casting of the input to FP32 would break the operator.
> 
> That is why after discussion with a few members of the community, I decided to proceed with all lists being explicit and introducing the test that would fail when somebody added an operator without classifying it into 1 of the categories, and explain clearly how to do it [7]. It is not ideal solution, as it introduces some burden on the developers who are not aware about AMP, however in the typical case of adding at most a few operators to MXNet the inconvenience is I think pretty minor while important for the feature correctness going forward.
> 
> I would like to gather Community feedback and ideas how to handle this situation.
> 
> [1] https://github.com/apache/incubator-mxnet/pull/14173
> [2] https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/amp/lists/symbol.py
> [3] https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py
> [4] https://github.com/apache/incubator-mxnet/pull/15085
> [5] https://docs.google.com/document/d/1sQzMoPEwux0WXSWirY07us1POD_6Y8pLYq--b9Fvd1o/edit?usp=sharing
> [6] https://github.com/apache/incubator-mxnet/pull/14173#discussion_r270728019
> [7] https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_amp.py#L62-L80
> [8] https://github.com/apache/incubator-mxnet/pull/14173#pullrequestreview-235846341
>