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/11/06 22:54:05 UTC
[GitHub] [incubator-mxnet] mk-61 opened a new pull request #19488: Add AMP patching of npi ops in _api_internal module
mk-61 opened a new pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488
## Description ##
Apparently, some NumpyOps are registered in mxnet.ndarray.numpy._api_internal module, in addition to *._internal. This PR implements AMP patching of such ops there.
Fixes #19463.
## Checklist ##
### Essentials ###
- [x] Changes are complete (i.e. I finished coding on this PR)
@ptrendx
----------------------------------------------------------------
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] TristonC commented on pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
TristonC commented on pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#issuecomment-729993759
@sandeep-krishnamurthy Could someone help for the CI test failure? It seems the failure not related with the PR.
----------------------------------------------------------------
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] sxjscience commented on a change in pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
sxjscience commented on a change in pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#discussion_r526592545
##########
File path: tests/python/gpu/test_amp_init.py
##########
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import mxnet as mx
+from mxnet.gluon import nn
+from mxnet import amp
+import numpy as np
+import pytest
+
+
+@pytest.fixture
+def np_shape_array():
+ flags = mx.npx.is_np_shape(), mx.npx.is_np_array(), mx.npx.is_np_default_dtype()
+ mx.npx.set_np()
+ yield
+ mx.npx.set_np(*flags)
+
+
+@pytest.fixture(scope='module')
Review comment:
In fact, the amp in pytorch is implemented as a context manager.
----------------------------------------------------------------
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] sxjscience commented on a change in pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
sxjscience commented on a change in pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#discussion_r526578507
##########
File path: tests/python/gpu/test_amp_init.py
##########
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import mxnet as mx
+from mxnet.gluon import nn
+from mxnet import amp
+import numpy as np
+import pytest
+
+
+@pytest.fixture
+def np_shape_array():
+ flags = mx.npx.is_np_shape(), mx.npx.is_np_array(), mx.npx.is_np_default_dtype()
+ mx.npx.set_np()
+ yield
+ mx.npx.set_np(*flags)
+
+
+@pytest.fixture(scope='module')
Review comment:
Can we try to narrow the scope to `function`?
----------------------------------------------------------------
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] mk-61 commented on a change in pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
mk-61 commented on a change in pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#discussion_r527138082
##########
File path: tests/python/gpu/test_amp_init.py
##########
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import mxnet as mx
+from mxnet.gluon import nn
+from mxnet import amp
+import numpy as np
+import pytest
+
+
+@pytest.fixture
+def np_shape_array():
+ flags = mx.npx.is_np_shape(), mx.npx.is_np_array(), mx.npx.is_np_default_dtype()
+ mx.npx.set_np()
+ yield
+ mx.npx.set_np(*flags)
+
+
+@pytest.fixture(scope='module')
Review comment:
I agree it's a nifty API to enable/disable AMP with a context manager, but would definitely require a separate, non-trivial change.
For now, to unblock this PR, moved amp_init tests into a separate process.
----------------------------------------------------------------
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] TristonC commented on pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
TristonC commented on pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#issuecomment-730590809
Thank you guy for your help @leezu @sxjscience.
----------------------------------------------------------------
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] mk-61 commented on pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
mk-61 commented on pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#issuecomment-724397118
@mxnet-bot run ci [centos-cpu, centos-gpu, unix-cpu, unix-gpu]
----------------------------------------------------------------
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] sxjscience commented on a change in pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
sxjscience commented on a change in pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#discussion_r520184777
##########
File path: python/mxnet/amp/amp.py
##########
@@ -80,20 +80,27 @@ def _get_nd_fun_to_wrap(name, module, submodule_dict):
else:
func_name = name
cur_module = module
- return func_name, cur_module
+ return func_name, [cur_module]
def _get_np_fun_to_wrap(name, ns_prefix):
for pre, mod, subs in ((_NP_OP_PREFIX, 'numpy', _NP_OP_SUBMODULE_LIST),
(_NP_EXT_OP_PREFIX, 'numpy_extension', _NP_EXT_OP_SUBMODULE_LIST),
(_NP_INTERNAL_OP_PREFIX, 'numpy._internal', [])):
if name.startswith(pre):
- name = name[len(pre):]
+ nm = name[len(pre):]
for sub in subs:
- if name.startswith(sub):
- return name[len(sub):], sys.modules[f'{ns_prefix}.{mod}.{sub[1:-1]}']
- return name, sys.modules[f'{ns_prefix}.{mod}']
- assert False
- return None # for pylint
+ if nm.startswith(sub):
+ func, modules = nm[len(sub):], [sys.modules[f'{ns_prefix}.{mod}.{sub[1:-1]}']]
+ break
+ else:
Review comment:
I'm not sure if the indentation here is correct.
----------------------------------------------------------------
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 #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#issuecomment-725552825
Jenkins CI successfully triggered : [unix-gpu, centos-gpu]
----------------------------------------------------------------
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] leezu commented on a change in pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#discussion_r527151701
##########
File path: tests/python/gpu/test_amp_init.py
##########
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import mxnet as mx
+from mxnet.gluon import nn
+from mxnet import amp
+import numpy as np
+import pytest
+
+
+@pytest.fixture
+def np_shape_array():
+ flags = mx.npx.is_np_shape(), mx.npx.is_np_array(), mx.npx.is_np_default_dtype()
+ mx.npx.set_np()
+ yield
+ mx.npx.set_np(*flags)
+
+
+@pytest.fixture(scope='module')
Review comment:
@mk-61 would you like to help create a tracking issue for that change? It sounds like something we should do before the stable 2.0 release
----------------------------------------------------------------
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] mk-61 commented on a change in pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
mk-61 commented on a change in pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#discussion_r526583980
##########
File path: tests/python/gpu/test_amp_init.py
##########
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import mxnet as mx
+from mxnet.gluon import nn
+from mxnet import amp
+import numpy as np
+import pytest
+
+
+@pytest.fixture
+def np_shape_array():
+ flags = mx.npx.is_np_shape(), mx.npx.is_np_array(), mx.npx.is_np_default_dtype()
+ mx.npx.set_np()
+ yield
+ mx.npx.set_np(*flags)
+
+
+@pytest.fixture(scope='module')
Review comment:
It doesn't matter. The only reason I put scope='module' here is that amp.init() is called once per module. But even that doesn't matter much, since all subsequent calls would be a noop.
Regardless of what you declare in pytest amp.init() cannot be undone, at least with the current API.
----------------------------------------------------------------
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] mk-61 commented on pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
mk-61 commented on pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#issuecomment-724441935
@mxnet-bot run ci [centos-gpu, unix-gpu]
----------------------------------------------------------------
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] mk-61 commented on a change in pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
mk-61 commented on a change in pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#discussion_r520194107
##########
File path: python/mxnet/amp/amp.py
##########
@@ -80,20 +80,27 @@ def _get_nd_fun_to_wrap(name, module, submodule_dict):
else:
func_name = name
cur_module = module
- return func_name, cur_module
+ return func_name, [cur_module]
def _get_np_fun_to_wrap(name, ns_prefix):
for pre, mod, subs in ((_NP_OP_PREFIX, 'numpy', _NP_OP_SUBMODULE_LIST),
(_NP_EXT_OP_PREFIX, 'numpy_extension', _NP_EXT_OP_SUBMODULE_LIST),
(_NP_INTERNAL_OP_PREFIX, 'numpy._internal', [])):
if name.startswith(pre):
- name = name[len(pre):]
+ nm = name[len(pre):]
for sub in subs:
- if name.startswith(sub):
- return name[len(sub):], sys.modules[f'{ns_prefix}.{mod}.{sub[1:-1]}']
- return name, sys.modules[f'{ns_prefix}.{mod}']
- assert False
- return None # for pylint
+ if nm.startswith(sub):
+ func, modules = nm[len(sub):], [sys.modules[f'{ns_prefix}.{mod}.{sub[1:-1]}']]
+ break
+ else:
Review comment:
What are your doubts? `Else` clause on line 95 aligns with `for` on line 91 - it gets executed if the loop completes without a break.
----------------------------------------------------------------
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] leezu commented on pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#issuecomment-723382703
@mxnet-bot run ci [centos-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] mxnet-bot commented on pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#issuecomment-723335752
Hey @mk-61 , 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**: [unix-gpu, centos-cpu, windows-cpu, windows-gpu, centos-gpu, miscellaneous, website, clang, edge, unix-cpu, sanity]
***
_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] sxjscience commented on pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
sxjscience commented on pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#issuecomment-725552758
@mxnet-bot run ci [centos-gpu, unix-gpu]
----------------------------------------------------------------
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 #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#issuecomment-724397145
Jenkins CI successfully triggered : [unix-cpu, unix-gpu, centos-cpu, centos-gpu]
----------------------------------------------------------------
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] leezu commented on pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#issuecomment-729995639
@TristonC @mk-61 I haven't seen such error before. I think it may be introduced by this PR. Please try rebasing on master first and if the issue persists we can be sure that it is due to this PR.
----------------------------------------------------------------
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] leezu commented on a change in pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#discussion_r527195906
##########
File path: tests/python/gpu/test_amp_init.py
##########
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import mxnet as mx
+from mxnet.gluon import nn
+from mxnet import amp
+import numpy as np
+import pytest
+
+
+@pytest.fixture
+def np_shape_array():
+ flags = mx.npx.is_np_shape(), mx.npx.is_np_array(), mx.npx.is_np_default_dtype()
+ mx.npx.set_np()
+ yield
+ mx.npx.set_np(*flags)
+
+
+@pytest.fixture(scope='module')
Review comment:
> @leezu - for now, I've just added an item here - #18896 - I keep it as a TODO list for AMP, of sorts. Still, can create a separate issue if you prefer to track it like this.
That's fine. Thank you
> About adding it before the stable 2.0 release, well... To be clear - it's not just a matter of exposing something existing via a new API. Also, I'd like to clarify our plans about #18697 first, since it affects pretty much everything AMP-related.
Using a graph pass may address the current issue of global state. A potential downside is that graph pass will only work with hybrid models.
> When do we expect the stable 2.0 release? We still have some other, higher-priority items to fix.
We'd like to create an Alpha release soon. There is no date for a stable release yet.
> Finally, my hope was that if a new API is going to be a strict superset of already existing one - i.e., we'll still keep amp.init() call, but also add a way to turn it off - may be it's acceptable to add it later? Or, to put it differently, may be it's less of an issue to add a new APIs, as long as we are not removing anything?
It's fine to change the API in 2.0, if we feel it should be improved.
----------------------------------------------------------------
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] sxjscience commented on pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
sxjscience commented on pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#issuecomment-723526137
We may need to add the example in https://github.com/apache/incubator-mxnet/issues/19463 as a test case.
----------------------------------------------------------------
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] sxjscience commented on a change in pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
sxjscience commented on a change in pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#discussion_r520184777
##########
File path: python/mxnet/amp/amp.py
##########
@@ -80,20 +80,27 @@ def _get_nd_fun_to_wrap(name, module, submodule_dict):
else:
func_name = name
cur_module = module
- return func_name, cur_module
+ return func_name, [cur_module]
def _get_np_fun_to_wrap(name, ns_prefix):
for pre, mod, subs in ((_NP_OP_PREFIX, 'numpy', _NP_OP_SUBMODULE_LIST),
(_NP_EXT_OP_PREFIX, 'numpy_extension', _NP_EXT_OP_SUBMODULE_LIST),
(_NP_INTERNAL_OP_PREFIX, 'numpy._internal', [])):
if name.startswith(pre):
- name = name[len(pre):]
+ nm = name[len(pre):]
for sub in subs:
- if name.startswith(sub):
- return name[len(sub):], sys.modules[f'{ns_prefix}.{mod}.{sub[1:-1]}']
- return name, sys.modules[f'{ns_prefix}.{mod}']
- assert False
- return None # for pylint
+ if nm.startswith(sub):
+ func, modules = nm[len(sub):], [sys.modules[f'{ns_prefix}.{mod}.{sub[1:-1]}']]
+ break
+ else:
Review comment:
I'm not sure if the alignment here is correct.
----------------------------------------------------------------
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 #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#issuecomment-724441946
Jenkins CI successfully triggered : [unix-gpu, centos-gpu]
----------------------------------------------------------------
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] sxjscience commented on a change in pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
sxjscience commented on a change in pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#discussion_r520221070
##########
File path: python/mxnet/amp/amp.py
##########
@@ -80,20 +80,27 @@ def _get_nd_fun_to_wrap(name, module, submodule_dict):
else:
func_name = name
cur_module = module
- return func_name, cur_module
+ return func_name, [cur_module]
def _get_np_fun_to_wrap(name, ns_prefix):
for pre, mod, subs in ((_NP_OP_PREFIX, 'numpy', _NP_OP_SUBMODULE_LIST),
(_NP_EXT_OP_PREFIX, 'numpy_extension', _NP_EXT_OP_SUBMODULE_LIST),
(_NP_INTERNAL_OP_PREFIX, 'numpy._internal', [])):
if name.startswith(pre):
- name = name[len(pre):]
+ nm = name[len(pre):]
for sub in subs:
- if name.startswith(sub):
- return name[len(sub):], sys.modules[f'{ns_prefix}.{mod}.{sub[1:-1]}']
- return name, sys.modules[f'{ns_prefix}.{mod}']
- assert False
- return None # for pylint
+ if nm.startswith(sub):
+ func, modules = nm[len(sub):], [sys.modules[f'{ns_prefix}.{mod}.{sub[1:-1]}']]
+ break
+ else:
Review comment:
Thanks for clarifying.
----------------------------------------------------------------
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] mk-61 commented on pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
mk-61 commented on pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#issuecomment-730081600
@leezu, yes, the failures do appear to be related to this PR, yet in a way I don't fully understand. I tried running these test locally, and it appears that running test_amp_init.py changes something in a global state, that causes some other tests to fail.
It is understandable, in a way, since the test calls amp.init(), and performs some Python monkey patching - that's how AMP works. That's why I put this test into a separate module. Apparently, this is not enough, as it is not fully isolated.
So I can suggest the following ways to fix this, from most to least desirable, but for the first 2 I'll need a help from someone, more familiar with pytest and how it's integrated into our CI:
1. Call tests in test_amp_init.py in a completely separate, isolated process.
2. If we cannot run it completely isolated, run test_amp_init.py the last.
3. I can temporarily mark the new test skipped (one can still run it explicitly, I guess, and it does succeed).
The last option is not ideal, since we do expect to add more tests, which require calling amp.init().
----------------------------------------------------------------
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] mk-61 commented on a change in pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
mk-61 commented on a change in pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#discussion_r527182329
##########
File path: tests/python/gpu/test_amp_init.py
##########
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import mxnet as mx
+from mxnet.gluon import nn
+from mxnet import amp
+import numpy as np
+import pytest
+
+
+@pytest.fixture
+def np_shape_array():
+ flags = mx.npx.is_np_shape(), mx.npx.is_np_array(), mx.npx.is_np_default_dtype()
+ mx.npx.set_np()
+ yield
+ mx.npx.set_np(*flags)
+
+
+@pytest.fixture(scope='module')
Review comment:
@leezu - for now, I've just added an item here - https://github.com/apache/incubator-mxnet/issues/18896 - I keep it as a TODO list for AMP, of sorts. Still, can create a separate issue if you prefer to track it like this.
About adding it before the stable 2.0 release, well... To be clear - it's not just a matter of exposing something existing via a new API. Also, I'd like to clarify our plans about https://github.com/apache/incubator-mxnet/issues/18697 first, since it affects pretty much everything AMP-related.
When do we expect the stable 2.0 release? We still have some other, higher-priority items to fix.
Finally, my hope was that if a new API is going to be a strict superset of already existing one - i.e., we'll still keep amp.init() call, but also add a way to turn it off - may be it's acceptable to add it later? Or, to put it differently, may be it's less of an issue to add a new APIs, as long as we are not removing anything?
----------------------------------------------------------------
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] leezu merged pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
leezu merged pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488
----------------------------------------------------------------
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 #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#issuecomment-723382709
Jenkins CI successfully triggered : [centos-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] leezu commented on a change in pull request #19488: Add AMP patching of npi ops in _api_internal module
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19488:
URL: https://github.com/apache/incubator-mxnet/pull/19488#discussion_r526584389
##########
File path: tests/python/gpu/test_amp_init.py
##########
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import mxnet as mx
+from mxnet.gluon import nn
+from mxnet import amp
+import numpy as np
+import pytest
+
+
+@pytest.fixture
+def np_shape_array():
+ flags = mx.npx.is_np_shape(), mx.npx.is_np_array(), mx.npx.is_np_default_dtype()
+ mx.npx.set_np()
+ yield
+ mx.npx.set_np(*flags)
+
+
+@pytest.fixture(scope='module')
Review comment:
Should we add a utility to disable amp? If it is too much work to add quickly, you may add a separate pytest call in the `runtime_functions.sh` file for the amp test files.
----------------------------------------------------------------
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