You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/06/30 11:06:18 UTC
[GitHub] [incubator-mxnet] anko-intel opened a new pull request #18644: Fix BatchNorm backward synchronization
anko-intel opened a new pull request #18644:
URL: https://github.com/apache/incubator-mxnet/pull/18644
## Description ##
Fix the issue #18610 - synchronization problem with running variables for backward pass of BatchNorm
## Checklist ##
### Essentials ###
- [ ] Changes are complete (i.e. I finished coding on this PR)
- [ ] All changes have test coverage:
- Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
- [ ] To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #18644: Fix BatchNorm backward synchronization
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18644:
URL: https://github.com/apache/incubator-mxnet/pull/18644#issuecomment-652372006
Undefined action detected.
Permissible actions are : run ci [all], run ci [job1, job2]
Example : @mxnet-bot run ci [all]
Example : @mxnet-bot run ci [centos-cpu, clang]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] anko-intel commented on pull request #18644: Fix BatchNorm backward synchronization
Posted by GitBox <gi...@apache.org>.
anko-intel commented on pull request #18644:
URL: https://github.com/apache/incubator-mxnet/pull/18644#issuecomment-652371982
@mxnet-bot [codecov/project]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] anko-intel commented on a change in pull request #18644: Fix BatchNorm backward synchronization
Posted by GitBox <gi...@apache.org>.
anko-intel commented on a change in pull request #18644:
URL: https://github.com/apache/incubator-mxnet/pull/18644#discussion_r448157104
##########
File path: tests/python/unittest/test_gluon.py
##########
@@ -665,6 +665,34 @@ def transpose(shape):
assert (layer(x).shape==ceil_out_shape)
+@with_seed()
+@pytest.mark.parametrize('cudnn_off', [True, False])
+@pytest.mark.parametrize('variable', ['running_var', 'running_mean'])
+def test_batchnorm_backward_synchronization(cudnn_off, variable):
+ """
+ Tests if synchronization of BatchNorm running variables is done correctly.
+ If not, the test sometimes fails - depending on the timing.
+ """
+ ctx = mx.cpu() if cudnn_off else mx.gpu()
Review comment:
Thanks for the tip @wkcn,
Is there any command line option or environment value to enforce particular context when I run pytest locally?
##########
File path: tests/python/unittest/test_gluon.py
##########
@@ -665,6 +665,34 @@ def transpose(shape):
assert (layer(x).shape==ceil_out_shape)
+@with_seed()
+@pytest.mark.parametrize('cudnn_off', [True, False])
+@pytest.mark.parametrize('variable', ['running_var', 'running_mean'])
+def test_batchnorm_backward_synchronization(cudnn_off, variable):
+ """
+ Tests if synchronization of BatchNorm running variables is done correctly.
+ If not, the test sometimes fails - depending on the timing.
+ """
+ ctx = mx.cpu() if cudnn_off else mx.gpu()
+ read_op = 'layer.' + variable + '.data().asnumpy()'
+
+ for _ in range(20):
+ layer = nn.BatchNorm()
+ layer.initialize(ctx=ctx)
+ for _ in range(3):
+ data = mx.nd.random.normal(loc=10, scale=2, shape=(1, 3, 10, 10), ctx=ctx)
+ with mx.autograd.record():
+ out = layer(data)
+ out.backward()
+
+ # check if each read give the same value
+ var1 = eval(read_op)
Review comment:
Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] wkcn commented on a change in pull request #18644: Fix BatchNorm backward synchronization
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #18644:
URL: https://github.com/apache/incubator-mxnet/pull/18644#discussion_r447749228
##########
File path: tests/python/unittest/test_gluon.py
##########
@@ -665,6 +665,34 @@ def transpose(shape):
assert (layer(x).shape==ceil_out_shape)
+@with_seed()
+@pytest.mark.parametrize('cudnn_off', [True, False])
+@pytest.mark.parametrize('variable', ['running_var', 'running_mean'])
+def test_batchnorm_backward_synchronization(cudnn_off, variable):
+ """
+ Tests if synchronization of BatchNorm running variables is done correctly.
+ If not, the test sometimes fails - depending on the timing.
+ """
+ ctx = mx.cpu() if cudnn_off else mx.gpu()
Review comment:
It should be `ctx=default_context()`.
In CI, there are tests on CPU and GPU. We don't need to specify the context.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] TaoLv merged pull request #18644: Fix BatchNorm backward synchronization
Posted by GitBox <gi...@apache.org>.
TaoLv merged pull request #18644:
URL: https://github.com/apache/incubator-mxnet/pull/18644
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] wkcn commented on a change in pull request #18644: Fix BatchNorm backward synchronization
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #18644:
URL: https://github.com/apache/incubator-mxnet/pull/18644#discussion_r447749228
##########
File path: tests/python/unittest/test_gluon.py
##########
@@ -665,6 +665,34 @@ def transpose(shape):
assert (layer(x).shape==ceil_out_shape)
+@with_seed()
+@pytest.mark.parametrize('cudnn_off', [True, False])
+@pytest.mark.parametrize('variable', ['running_var', 'running_mean'])
+def test_batchnorm_backward_synchronization(cudnn_off, variable):
+ """
+ Tests if synchronization of BatchNorm running variables is done correctly.
+ If not, the test sometimes fails - depending on the timing.
+ """
+ ctx = mx.cpu() if cudnn_off else mx.gpu()
Review comment:
It should be `ctx = mx.test_utils.default_context()`.
In CI, there are tests on CPU and GPU. We don't need to specify the context.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] TaoLv commented on pull request #18644: Fix BatchNorm backward synchronization
Posted by GitBox <gi...@apache.org>.
TaoLv commented on pull request #18644:
URL: https://github.com/apache/incubator-mxnet/pull/18644#issuecomment-652460275
Thanks for the contribution, @anko-intel. Does the problem also exist on v1.x? If yes, could you please cherry pick the fix to the branch also?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #18644: Fix BatchNorm backward synchronization
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18644:
URL: https://github.com/apache/incubator-mxnet/pull/18644#issuecomment-651723999
Hey @anko-intel , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:
- To trigger all jobs: @mxnet-bot run ci [all]
- To trigger specific jobs: @mxnet-bot run ci [job1, job2]
***
**CI supported jobs**: [clang, centos-gpu, sanity, centos-cpu, miscellaneous, windows-cpu, unix-cpu, windows-gpu, edge, website, unix-gpu]
***
_Note_:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] wkcn commented on a change in pull request #18644: Fix BatchNorm backward synchronization
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #18644:
URL: https://github.com/apache/incubator-mxnet/pull/18644#discussion_r447749228
##########
File path: tests/python/unittest/test_gluon.py
##########
@@ -665,6 +665,34 @@ def transpose(shape):
assert (layer(x).shape==ceil_out_shape)
+@with_seed()
+@pytest.mark.parametrize('cudnn_off', [True, False])
+@pytest.mark.parametrize('variable', ['running_var', 'running_mean'])
+def test_batchnorm_backward_synchronization(cudnn_off, variable):
+ """
+ Tests if synchronization of BatchNorm running variables is done correctly.
+ If not, the test sometimes fails - depending on the timing.
+ """
+ ctx = mx.cpu() if cudnn_off else mx.gpu()
Review comment:
It should be `ctx = default_context()`.
In CI, there are tests on CPU and GPU. We don't need to specify the context.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] wkcn commented on a change in pull request #18644: Fix BatchNorm backward synchronization
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #18644:
URL: https://github.com/apache/incubator-mxnet/pull/18644#discussion_r447742251
##########
File path: tests/python/unittest/test_gluon.py
##########
@@ -665,6 +665,34 @@ def transpose(shape):
assert (layer(x).shape==ceil_out_shape)
+@with_seed()
+@pytest.mark.parametrize('cudnn_off', [True, False])
+@pytest.mark.parametrize('variable', ['running_var', 'running_mean'])
+def test_batchnorm_backward_synchronization(cudnn_off, variable):
+ """
+ Tests if synchronization of BatchNorm running variables is done correctly.
+ If not, the test sometimes fails - depending on the timing.
+ """
+ ctx = mx.cpu() if cudnn_off else mx.gpu()
+ read_op = 'layer.' + variable + '.data().asnumpy()'
+
+ for _ in range(20):
+ layer = nn.BatchNorm()
+ layer.initialize(ctx=ctx)
+ for _ in range(3):
+ data = mx.nd.random.normal(loc=10, scale=2, shape=(1, 3, 10, 10), ctx=ctx)
+ with mx.autograd.record():
+ out = layer(data)
+ out.backward()
+
+ # check if each read give the same value
+ var1 = eval(read_op)
Review comment:
Thank you for the contribution!
For safety, I suggest to use `var1 = getattr(layer, variable).data().asnumpy()`.
##########
File path: tests/python/unittest/test_gluon.py
##########
@@ -665,6 +665,34 @@ def transpose(shape):
assert (layer(x).shape==ceil_out_shape)
+@with_seed()
+@pytest.mark.parametrize('cudnn_off', [True, False])
+@pytest.mark.parametrize('variable', ['running_var', 'running_mean'])
+def test_batchnorm_backward_synchronization(cudnn_off, variable):
+ """
+ Tests if synchronization of BatchNorm running variables is done correctly.
+ If not, the test sometimes fails - depending on the timing.
+ """
+ ctx = mx.cpu() if cudnn_off else mx.gpu()
+ read_op = 'layer.' + variable + '.data().asnumpy()'
+
+ for _ in range(20):
+ layer = nn.BatchNorm()
+ layer.initialize(ctx=ctx)
+ for _ in range(3):
+ data = mx.nd.random.normal(loc=10, scale=2, shape=(1, 3, 10, 10), ctx=ctx)
+ with mx.autograd.record():
+ out = layer(data)
+ out.backward()
+
+ # check if each read give the same value
+ var1 = eval(read_op)
+ for _ in range(10):
+ var2 = eval(read_op)
Review comment:
`var2 = getattr(layer, variable).data().asnumpy()`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] wkcn commented on a change in pull request #18644: Fix BatchNorm backward synchronization
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #18644:
URL: https://github.com/apache/incubator-mxnet/pull/18644#discussion_r448320053
##########
File path: tests/python/unittest/test_gluon.py
##########
@@ -665,6 +665,34 @@ def transpose(shape):
assert (layer(x).shape==ceil_out_shape)
+@with_seed()
+@pytest.mark.parametrize('cudnn_off', [True, False])
+@pytest.mark.parametrize('variable', ['running_var', 'running_mean'])
+def test_batchnorm_backward_synchronization(cudnn_off, variable):
+ """
+ Tests if synchronization of BatchNorm running variables is done correctly.
+ If not, the test sometimes fails - depending on the timing.
+ """
+ ctx = mx.cpu() if cudnn_off else mx.gpu()
Review comment:
There seems to no command line to specify the context.
The default context is CPU.
We can create a python file, like https://github.com/apache/incubator-mxnet/blob/master/tests/python/gpu/test_gluon_gpu.py
'''
import mxnet ad mx
from some_test_file import *
set_default_context(mx.gpu(0))
'''
It will switch to gpu context.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #18644: Fix BatchNorm backward synchronization
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18644:
URL: https://github.com/apache/incubator-mxnet/pull/18644#issuecomment-652375755
None of the jobs entered are supported.
Jobs entered by user: [codecov/project]
CI supported Jobs: [windows-gpu, unix-gpu, edge, sanity, miscellaneous, unix-cpu, clang, centos-gpu, website, centos-cpu, windows-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] anko-intel edited a comment on pull request #18644: Fix BatchNorm backward synchronization
Posted by GitBox <gi...@apache.org>.
anko-intel edited a comment on pull request #18644:
URL: https://github.com/apache/incubator-mxnet/pull/18644#issuecomment-652371982
@mxnet-bot run ci [codecov/project]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org