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/09/15 18:30:56 UTC
[GitHub] [incubator-mxnet] ArmageddonKnight opened a new pull request #19154: Address Leo's comments on PR#18704
ArmageddonKnight opened a new pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154
@leezu @szha FYI, I have addressed the comments on PR#18704 . Please let me know if there are any other issues or concerns. Thank you.
----------------------------------------------------------------
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 #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#issuecomment-693542690
Jenkins CI successfully triggered : [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] ArmageddonKnight commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488974090
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -70,23 +74,20 @@ def test_gpu_memory_profiler_symbolic():
# Sample gpu_memory_profile.csv:
# "Attribute Name","Requested Size","Device","Actual Size","Reuse?"
+ # <unk>:_head_grad_0,16777216,0,16777216,0
# init:_random_uniform,33554432,0,33554432,1
# init:_random_uniform,8388608,0,8388608,1
# resource:temp_space (sample_op.h +365),8,0,4096,0
# symbol:arg_grad:unknown,8388608,0,8388608,0
# symbol:arg_grad:unknown,33554432,0,33554432,0
- # tensordot:dot,16777216,0,16777216,0
- # tensordot:dot_backward,33554432,0,33554432,0
- # tensordot:dot_backward,8388608,0,8388608,0
- # tensordot:dot_head_grad,16777216,0,16777216,0
+ # tensordot:dot0,16777216,0,16777216,0
# tensordot:in_arg:A,8388608,0,8388608,0
# tensordot:in_arg:B,33554432,0,33554432,0
+ # tensordot:node_0_backward,33554432,0,33554432,0
+ # tensordot:node_0_backward,8388608,0,8388608,0s
Review comment:
So what about this: we keep the name if the node has been named at the frontend, and name it to `node_0/1/2/...` if it has not been named.
----------------------------------------------------------------
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] ArmageddonKnight commented on pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight commented on pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#issuecomment-693704932
@leezu FYI, it has passed all CI tests.
----------------------------------------------------------------
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] ArmageddonKnight commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488995752
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -23,12 +23,16 @@
import mxnet as mx
mx.test_utils.set_default_context(mx.gpu(0))
+from mxnet.gluon.block import _block_scope
+
curr_path = os.path.dirname(os.path.abspath(os.path.expanduser(__file__)))
sys.path.insert(0, os.path.join(curr_path, '../unittest'))
-from mxnet import profiler
-from mxnet.gluon import nn
-from mxnet.gluon.block import _block_scope
-from test_profiler import enable_profiler
+# We import all tests from ../unittest/test_profiler.py
+# They will be detected by test framework, as long as the current file has a different filename
+from test_profiler import *
+# Test seen to crash pytest worker during development of
+# https://github.com/apache/incubator-mxnet/pull/18694
+del test_aggregate_duplication
Review comment:
@leezu @Zha0q1 I tried re-enable these tests and there seems to be no errors.
----------------------------------------------------------------
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 #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#issuecomment-693542659
@mxnet-bot run ci [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] mxnet-bot commented on pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#issuecomment-693175840
Jenkins CI successfully triggered : [sanity, edge, windows-cpu, clang, unix-cpu, miscellaneous, centos-gpu, website, unix-gpu, centos-cpu, windows-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 #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488973394
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -70,23 +74,20 @@ def test_gpu_memory_profiler_symbolic():
# Sample gpu_memory_profile.csv:
# "Attribute Name","Requested Size","Device","Actual Size","Reuse?"
+ # <unk>:_head_grad_0,16777216,0,16777216,0
# init:_random_uniform,33554432,0,33554432,1
# init:_random_uniform,8388608,0,8388608,1
# resource:temp_space (sample_op.h +365),8,0,4096,0
# symbol:arg_grad:unknown,8388608,0,8388608,0
# symbol:arg_grad:unknown,33554432,0,33554432,0
- # tensordot:dot,16777216,0,16777216,0
- # tensordot:dot_backward,33554432,0,33554432,0
- # tensordot:dot_backward,8388608,0,8388608,0
- # tensordot:dot_head_grad,16777216,0,16777216,0
+ # tensordot:dot0,16777216,0,16777216,0
# tensordot:in_arg:A,8388608,0,8388608,0
# tensordot:in_arg:B,33554432,0,33554432,0
+ # tensordot:node_0_backward,33554432,0,33554432,0
+ # tensordot:node_0_backward,8388608,0,8388608,0s
Review comment:
The problem is that your previous approach results in unnamed nodes in the imperative interface. It's fine to fix it later. Would you like to open a bug report about the CachedOp issue?
----------------------------------------------------------------
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 #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#issuecomment-693617762
Jenkins CI successfully triggered : [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 #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488907236
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -70,23 +74,20 @@ def test_gpu_memory_profiler_symbolic():
# Sample gpu_memory_profile.csv:
# "Attribute Name","Requested Size","Device","Actual Size","Reuse?"
+ # <unk>:_head_grad_0,16777216,0,16777216,0
# init:_random_uniform,33554432,0,33554432,1
# init:_random_uniform,8388608,0,8388608,1
# resource:temp_space (sample_op.h +365),8,0,4096,0
# symbol:arg_grad:unknown,8388608,0,8388608,0
# symbol:arg_grad:unknown,33554432,0,33554432,0
- # tensordot:dot,16777216,0,16777216,0
- # tensordot:dot_backward,33554432,0,33554432,0
- # tensordot:dot_backward,8388608,0,8388608,0
- # tensordot:dot_head_grad,16777216,0,16777216,0
+ # tensordot:dot0,16777216,0,16777216,0
# tensordot:in_arg:A,8388608,0,8388608,0
# tensordot:in_arg:B,33554432,0,33554432,0
+ # tensordot:node_0_backward,33554432,0,33554432,0
+ # tensordot:node_0_backward,8388608,0,8388608,0s
Review comment:
When you first introduced the profiler, did you have names like `tensordot:node_0_backward,8388608,0,8388608,0s` or did you have names like `tensordot:dot_backward,8388608,0,8388608,0`?
It is correct to revert the change and reintroduce `node->attrs.name = "node_" + std::to_string(node_count_++);`. My question is if we need any extra change to avoid seeing `node_0_backward`. My hypothesis is that the `node_0_backward` in symbolic API has been introduced by switching the symbolic API to use the CachedOp.
----------------------------------------------------------------
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 #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488982137
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -70,23 +74,20 @@ def test_gpu_memory_profiler_symbolic():
# Sample gpu_memory_profile.csv:
# "Attribute Name","Requested Size","Device","Actual Size","Reuse?"
+ # <unk>:_head_grad_0,16777216,0,16777216,0
# init:_random_uniform,33554432,0,33554432,1
# init:_random_uniform,8388608,0,8388608,1
# resource:temp_space (sample_op.h +365),8,0,4096,0
# symbol:arg_grad:unknown,8388608,0,8388608,0
# symbol:arg_grad:unknown,33554432,0,33554432,0
- # tensordot:dot,16777216,0,16777216,0
- # tensordot:dot_backward,33554432,0,33554432,0
- # tensordot:dot_backward,8388608,0,8388608,0
- # tensordot:dot_head_grad,16777216,0,16777216,0
+ # tensordot:dot0,16777216,0,16777216,0
# tensordot:in_arg:A,8388608,0,8388608,0
# tensordot:in_arg:B,33554432,0,33554432,0
+ # tensordot:node_0_backward,33554432,0,33554432,0
+ # tensordot:node_0_backward,8388608,0,8388608,0s
Review comment:
Did you check why `Imperative::RecordOp` or `Imperative::RecordDeferredCompute` is called here in the first place? I can't think of a reason for why it is called, but if it is correct / required to be called, yes, your workaround would be correct. Otherwise we should just ensure that `Imperative::RecordOp` or `Imperative::RecordDeferredCompute` from the symbolic 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] leezu commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488898785
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -23,12 +23,16 @@
import mxnet as mx
mx.test_utils.set_default_context(mx.gpu(0))
+from mxnet.gluon.block import _block_scope
+
curr_path = os.path.dirname(os.path.abspath(os.path.expanduser(__file__)))
sys.path.insert(0, os.path.join(curr_path, '../unittest'))
-from mxnet import profiler
-from mxnet.gluon import nn
-from mxnet.gluon.block import _block_scope
-from test_profiler import enable_profiler
+# We import all tests from ../unittest/test_profiler.py
+# They will be detected by test framework, as long as the current file has a different filename
+from test_profiler import *
+# Test seen to crash pytest worker during development of
+# https://github.com/apache/incubator-mxnet/pull/18694
+del test_aggregate_duplication
Review comment:
This was working prior to #18704? Could you try if it is really broken now, and if so, how to fix it?
```suggestion
```
----------------------------------------------------------------
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] ArmageddonKnight commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488954079
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -70,23 +74,20 @@ def test_gpu_memory_profiler_symbolic():
# Sample gpu_memory_profile.csv:
# "Attribute Name","Requested Size","Device","Actual Size","Reuse?"
+ # <unk>:_head_grad_0,16777216,0,16777216,0
# init:_random_uniform,33554432,0,33554432,1
# init:_random_uniform,8388608,0,8388608,1
# resource:temp_space (sample_op.h +365),8,0,4096,0
# symbol:arg_grad:unknown,8388608,0,8388608,0
# symbol:arg_grad:unknown,33554432,0,33554432,0
- # tensordot:dot,16777216,0,16777216,0
- # tensordot:dot_backward,33554432,0,33554432,0
- # tensordot:dot_backward,8388608,0,8388608,0
- # tensordot:dot_head_grad,16777216,0,16777216,0
+ # tensordot:dot0,16777216,0,16777216,0
# tensordot:in_arg:A,8388608,0,8388608,0
# tensordot:in_arg:B,33554432,0,33554432,0
+ # tensordot:node_0_backward,33554432,0,33554432,0
+ # tensordot:node_0_backward,8388608,0,8388608,0s
Review comment:
The previous approach is what I have in mind, but since that one is rejected I do not think I will be fixing it as part of 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] mxnet-bot commented on pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#issuecomment-693008407
Jenkins CI successfully triggered : [sanity]
----------------------------------------------------------------
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] ArmageddonKnight edited a comment on pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight edited a comment on pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#issuecomment-693008383
@mxnet-bot run ci [all]
----------------------------------------------------------------
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] ArmageddonKnight commented on pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight commented on pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#issuecomment-694463052
@leezu FYI, all CI tests have passed.
----------------------------------------------------------------
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] Zha0q1 commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488982374
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -23,12 +23,16 @@
import mxnet as mx
mx.test_utils.set_default_context(mx.gpu(0))
+from mxnet.gluon.block import _block_scope
+
curr_path = os.path.dirname(os.path.abspath(os.path.expanduser(__file__)))
sys.path.insert(0, os.path.join(curr_path, '../unittest'))
-from mxnet import profiler
-from mxnet.gluon import nn
-from mxnet.gluon.block import _block_scope
-from test_profiler import enable_profiler
+# We import all tests from ../unittest/test_profiler.py
+# They will be detected by test framework, as long as the current file has a different filename
+from test_profiler import *
+# Test seen to crash pytest worker during development of
+# https://github.com/apache/incubator-mxnet/pull/18694
+del test_aggregate_duplication
Review comment:
just this test. I can revisit this after other changes stabilize
----------------------------------------------------------------
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 #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#issuecomment-692896046
Hey @ArmageddonKnight , 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**: [centos-gpu, sanity, edge, miscellaneous, windows-cpu, unix-gpu, clang, website, centos-cpu, windows-gpu, unix-cpu]
***
_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] leezu commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488948092
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -70,23 +74,20 @@ def test_gpu_memory_profiler_symbolic():
# Sample gpu_memory_profile.csv:
# "Attribute Name","Requested Size","Device","Actual Size","Reuse?"
+ # <unk>:_head_grad_0,16777216,0,16777216,0
# init:_random_uniform,33554432,0,33554432,1
# init:_random_uniform,8388608,0,8388608,1
# resource:temp_space (sample_op.h +365),8,0,4096,0
# symbol:arg_grad:unknown,8388608,0,8388608,0
# symbol:arg_grad:unknown,33554432,0,33554432,0
- # tensordot:dot,16777216,0,16777216,0
- # tensordot:dot_backward,33554432,0,33554432,0
- # tensordot:dot_backward,8388608,0,8388608,0
- # tensordot:dot_head_grad,16777216,0,16777216,0
+ # tensordot:dot0,16777216,0,16777216,0
# tensordot:in_arg:A,8388608,0,8388608,0
# tensordot:in_arg:B,33554432,0,33554432,0
+ # tensordot:node_0_backward,33554432,0,33554432,0
+ # tensordot:node_0_backward,8388608,0,8388608,0s
Review comment:
Would you like to fix it as part of 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 #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r489798794
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -70,21 +71,20 @@ def test_gpu_memory_profiler_symbolic():
# Sample gpu_memory_profile.csv:
# "Attribute Name","Requested Size","Device","Actual Size","Reuse?"
+ # <unk>:_head_grad_0,16777216,0,16777216,0
# init:_random_uniform,33554432,0,33554432,1
# init:_random_uniform,8388608,0,8388608,1
# resource:temp_space (sample_op.h +365),8,0,4096,0
# symbol:arg_grad:unknown,8388608,0,8388608,0
# symbol:arg_grad:unknown,33554432,0,33554432,0
# tensordot:dot,16777216,0,16777216,0
- # tensordot:dot_backward,33554432,0,33554432,0
- # tensordot:dot_backward,8388608,0,8388608,0
- # tensordot:dot_head_grad,16777216,0,16777216,0
# tensordot:in_arg:A,8388608,0,8388608,0
# tensordot:in_arg:B,33554432,0,33554432,0
+ # tensordot:node_0_backward,33554432,0,33554432,0
+ # tensordot:node_0_backward,8388608,0,8388608,0s
Review comment:
After your recent change, why do we still expect `node_0_backward` here?
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -23,12 +23,13 @@
import mxnet as mx
mx.test_utils.set_default_context(mx.gpu(0))
+from mxnet.gluon.block import _block_scope
+
Review comment:
This import isn't needed?
----------------------------------------------------------------
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 #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#issuecomment-694498403
Thank you @ArmageddonKnight !
----------------------------------------------------------------
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] ArmageddonKnight edited a comment on pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight edited a comment on pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#issuecomment-693008383
@mxnet-bot run ci [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] ArmageddonKnight commented on pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight commented on pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#issuecomment-693008383
@mxnet-bot run ci [sanity]
----------------------------------------------------------------
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] Zha0q1 commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
Zha0q1 commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488973512
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -23,12 +23,16 @@
import mxnet as mx
mx.test_utils.set_default_context(mx.gpu(0))
+from mxnet.gluon.block import _block_scope
+
curr_path = os.path.dirname(os.path.abspath(os.path.expanduser(__file__)))
sys.path.insert(0, os.path.join(curr_path, '../unittest'))
-from mxnet import profiler
-from mxnet.gluon import nn
-from mxnet.gluon.block import _block_scope
-from test_profiler import enable_profiler
+# We import all tests from ../unittest/test_profiler.py
+# They will be detected by test framework, as long as the current file has a different filename
+from test_profiler import *
+# Test seen to crash pytest worker during development of
+# https://github.com/apache/incubator-mxnet/pull/18694
+del test_aggregate_duplication
Review comment:
Can you share the error message? I just copied out and ran this test on a gpu instance and it did not fail. This test was added in a pr that fixes the operator invocation count bing 2X of the actual number issue. All it does is just create a tensor, do some operations on it, and check if count 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] ArmageddonKnight commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488908070
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -70,23 +74,20 @@ def test_gpu_memory_profiler_symbolic():
# Sample gpu_memory_profile.csv:
# "Attribute Name","Requested Size","Device","Actual Size","Reuse?"
+ # <unk>:_head_grad_0,16777216,0,16777216,0
# init:_random_uniform,33554432,0,33554432,1
# init:_random_uniform,8388608,0,8388608,1
# resource:temp_space (sample_op.h +365),8,0,4096,0
# symbol:arg_grad:unknown,8388608,0,8388608,0
# symbol:arg_grad:unknown,33554432,0,33554432,0
- # tensordot:dot,16777216,0,16777216,0
- # tensordot:dot_backward,33554432,0,33554432,0
- # tensordot:dot_backward,8388608,0,8388608,0
- # tensordot:dot_head_grad,16777216,0,16777216,0
+ # tensordot:dot0,16777216,0,16777216,0
# tensordot:in_arg:A,8388608,0,8388608,0
# tensordot:in_arg:B,33554432,0,33554432,0
+ # tensordot:node_0_backward,33554432,0,33554432,0
+ # tensordot:node_0_backward,8388608,0,8388608,0s
Review comment:
Yes. Your hypothesis 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] leezu commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488935127
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -23,12 +23,16 @@
import mxnet as mx
mx.test_utils.set_default_context(mx.gpu(0))
+from mxnet.gluon.block import _block_scope
+
curr_path = os.path.dirname(os.path.abspath(os.path.expanduser(__file__)))
sys.path.insert(0, os.path.join(curr_path, '../unittest'))
-from mxnet import profiler
-from mxnet.gluon import nn
-from mxnet.gluon.block import _block_scope
-from test_profiler import enable_profiler
+# We import all tests from ../unittest/test_profiler.py
+# They will be detected by test framework, as long as the current file has a different filename
+from test_profiler import *
+# Test seen to crash pytest worker during development of
+# https://github.com/apache/incubator-mxnet/pull/18694
+del test_aggregate_duplication
Review comment:
Thank you for clarifying. @Zha0q1, you added the testcase, do you have an idea why it fails on 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 #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488900977
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -70,23 +74,20 @@ def test_gpu_memory_profiler_symbolic():
# Sample gpu_memory_profile.csv:
# "Attribute Name","Requested Size","Device","Actual Size","Reuse?"
+ # <unk>:_head_grad_0,16777216,0,16777216,0
# init:_random_uniform,33554432,0,33554432,1
# init:_random_uniform,8388608,0,8388608,1
# resource:temp_space (sample_op.h +365),8,0,4096,0
# symbol:arg_grad:unknown,8388608,0,8388608,0
# symbol:arg_grad:unknown,33554432,0,33554432,0
- # tensordot:dot,16777216,0,16777216,0
- # tensordot:dot_backward,33554432,0,33554432,0
- # tensordot:dot_backward,8388608,0,8388608,0
- # tensordot:dot_head_grad,16777216,0,16777216,0
+ # tensordot:dot0,16777216,0,16777216,0
# tensordot:in_arg:A,8388608,0,8388608,0
# tensordot:in_arg:B,33554432,0,33554432,0
+ # tensordot:node_0_backward,33554432,0,33554432,0
+ # tensordot:node_0_backward,8388608,0,8388608,0s
Review comment:
The NameManager name shouldn't be overwritten in the Symbolic API. I think this is a regression introduced in https://github.com/apache/incubator-mxnet/pull/18598
Your prior fix of deleting `node->attrs.name = "node_" + std::to_string(node_count_++);` may not be correct, as it results in unnamed nodes in the imperative API, but if like to fix the naming, you can look into why https://github.com/apache/incubator-mxnet/pull/18598 causes the names to be overwritten
----------------------------------------------------------------
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] ArmageddonKnight commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488982145
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -23,12 +23,16 @@
import mxnet as mx
mx.test_utils.set_default_context(mx.gpu(0))
+from mxnet.gluon.block import _block_scope
+
curr_path = os.path.dirname(os.path.abspath(os.path.expanduser(__file__)))
sys.path.insert(0, os.path.join(curr_path, '../unittest'))
-from mxnet import profiler
-from mxnet.gluon import nn
-from mxnet.gluon.block import _block_scope
-from test_profiler import enable_profiler
+# We import all tests from ../unittest/test_profiler.py
+# They will be detected by test framework, as long as the current file has a different filename
+from test_profiler import *
+# Test seen to crash pytest worker during development of
+# https://github.com/apache/incubator-mxnet/pull/18694
+del test_aggregate_duplication
Review comment:
It seems that this line is added by @DickJC123 . Perhaps he can explain why.
----------------------------------------------------------------
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 #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488980257
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -23,12 +23,16 @@
import mxnet as mx
mx.test_utils.set_default_context(mx.gpu(0))
+from mxnet.gluon.block import _block_scope
+
curr_path = os.path.dirname(os.path.abspath(os.path.expanduser(__file__)))
sys.path.insert(0, os.path.join(curr_path, '../unittest'))
-from mxnet import profiler
-from mxnet.gluon import nn
-from mxnet.gluon.block import _block_scope
-from test_profiler import enable_profiler
+# We import all tests from ../unittest/test_profiler.py
+# They will be detected by test framework, as long as the current file has a different filename
+from test_profiler import *
+# Test seen to crash pytest worker during development of
+# https://github.com/apache/incubator-mxnet/pull/18694
+del test_aggregate_duplication
Review comment:
@Zha0q1 it was skipped as part of https://github.com/apache/incubator-mxnet/pull/18694/commits/60dda9ba884bb35be1ac1a9bd16a2a917073e9ed
I'm not sure about the exact error. Maybe you can just re-enable the test and it will pass.
----------------------------------------------------------------
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] ArmageddonKnight commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488974090
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -70,23 +74,20 @@ def test_gpu_memory_profiler_symbolic():
# Sample gpu_memory_profile.csv:
# "Attribute Name","Requested Size","Device","Actual Size","Reuse?"
+ # <unk>:_head_grad_0,16777216,0,16777216,0
# init:_random_uniform,33554432,0,33554432,1
# init:_random_uniform,8388608,0,8388608,1
# resource:temp_space (sample_op.h +365),8,0,4096,0
# symbol:arg_grad:unknown,8388608,0,8388608,0
# symbol:arg_grad:unknown,33554432,0,33554432,0
- # tensordot:dot,16777216,0,16777216,0
- # tensordot:dot_backward,33554432,0,33554432,0
- # tensordot:dot_backward,8388608,0,8388608,0
- # tensordot:dot_head_grad,16777216,0,16777216,0
+ # tensordot:dot0,16777216,0,16777216,0
# tensordot:in_arg:A,8388608,0,8388608,0
# tensordot:in_arg:B,33554432,0,33554432,0
+ # tensordot:node_0_backward,33554432,0,33554432,0
+ # tensordot:node_0_backward,8388608,0,8388608,0s
Review comment:
So what about this: we keep the name if the node has been named at the frontend (i.e. `node->attrs.name != ""`), and name it to `node_0/1/2/...` if it has not been named.
----------------------------------------------------------------
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 #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#issuecomment-693617587
@mxnet-bot run ci [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 merged pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
leezu merged pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154
----------------------------------------------------------------
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] ArmageddonKnight commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488901945
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -23,12 +23,16 @@
import mxnet as mx
mx.test_utils.set_default_context(mx.gpu(0))
+from mxnet.gluon.block import _block_scope
+
curr_path = os.path.dirname(os.path.abspath(os.path.expanduser(__file__)))
sys.path.insert(0, os.path.join(curr_path, '../unittest'))
-from mxnet import profiler
-from mxnet.gluon import nn
-from mxnet.gluon.block import _block_scope
-from test_profiler import enable_profiler
+# We import all tests from ../unittest/test_profiler.py
+# They will be detected by test framework, as long as the current file has a different filename
+from test_profiler import *
+# Test seen to crash pytest worker during development of
+# https://github.com/apache/incubator-mxnet/pull/18694
+del test_aggregate_duplication
Review comment:
This was not working before #18704 . I just reverted the changes on that.
----------------------------------------------------------------
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 #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488980257
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -23,12 +23,16 @@
import mxnet as mx
mx.test_utils.set_default_context(mx.gpu(0))
+from mxnet.gluon.block import _block_scope
+
curr_path = os.path.dirname(os.path.abspath(os.path.expanduser(__file__)))
sys.path.insert(0, os.path.join(curr_path, '../unittest'))
-from mxnet import profiler
-from mxnet.gluon import nn
-from mxnet.gluon.block import _block_scope
-from test_profiler import enable_profiler
+# We import all tests from ../unittest/test_profiler.py
+# They will be detected by test framework, as long as the current file has a different filename
+from test_profiler import *
+# Test seen to crash pytest worker during development of
+# https://github.com/apache/incubator-mxnet/pull/18694
+del test_aggregate_duplication
Review comment:
@Zha0q1 it was skipped as part of https://github.com/apache/incubator-mxnet/pull/18694/commits/60dda9ba884bb35be1ac1a9bd16a2a917073e9ed
I'm not sure about the exact error. Maybe you can just re-enable the test and it will pass. Did you run the complete test file during your trial or only the test in question? There could be issues related to running all tests in the file.
----------------------------------------------------------------
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 #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#issuecomment-694379448
Jenkins CI successfully triggered : [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] ArmageddonKnight commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r488904532
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -70,23 +74,20 @@ def test_gpu_memory_profiler_symbolic():
# Sample gpu_memory_profile.csv:
# "Attribute Name","Requested Size","Device","Actual Size","Reuse?"
+ # <unk>:_head_grad_0,16777216,0,16777216,0
# init:_random_uniform,33554432,0,33554432,1
# init:_random_uniform,8388608,0,8388608,1
# resource:temp_space (sample_op.h +365),8,0,4096,0
# symbol:arg_grad:unknown,8388608,0,8388608,0
# symbol:arg_grad:unknown,33554432,0,33554432,0
- # tensordot:dot,16777216,0,16777216,0
- # tensordot:dot_backward,33554432,0,33554432,0
- # tensordot:dot_backward,8388608,0,8388608,0
- # tensordot:dot_head_grad,16777216,0,16777216,0
+ # tensordot:dot0,16777216,0,16777216,0
# tensordot:in_arg:A,8388608,0,8388608,0
# tensordot:in_arg:B,33554432,0,33554432,0
+ # tensordot:node_0_backward,33554432,0,33554432,0
+ # tensordot:node_0_backward,8388608,0,8388608,0s
Review comment:
Sorry I do not quite understand your comments here. I have reverted the changes on
```C++
node->attrs.name = "node_" + std::to_string(node_count_++);
```
May I know what your expectation is here?
----------------------------------------------------------------
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] ArmageddonKnight commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r489701231
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -70,23 +74,20 @@ def test_gpu_memory_profiler_symbolic():
# Sample gpu_memory_profile.csv:
# "Attribute Name","Requested Size","Device","Actual Size","Reuse?"
+ # <unk>:_head_grad_0,16777216,0,16777216,0
# init:_random_uniform,33554432,0,33554432,1
# init:_random_uniform,8388608,0,8388608,1
# resource:temp_space (sample_op.h +365),8,0,4096,0
# symbol:arg_grad:unknown,8388608,0,8388608,0
# symbol:arg_grad:unknown,33554432,0,33554432,0
- # tensordot:dot,16777216,0,16777216,0
- # tensordot:dot_backward,33554432,0,33554432,0
- # tensordot:dot_backward,8388608,0,8388608,0
- # tensordot:dot_head_grad,16777216,0,16777216,0
+ # tensordot:dot0,16777216,0,16777216,0
# tensordot:in_arg:A,8388608,0,8388608,0
# tensordot:in_arg:B,33554432,0,33554432,0
+ # tensordot:node_0_backward,33554432,0,33554432,0
+ # tensordot:node_0_backward,8388608,0,8388608,0s
Review comment:
> So what about this: we keep the name if the node has been named at the frontend (i.e. `node->attrs.name != ""`), and name it to `node_0/1/2/...` if it has not been named.
I think I will go for this solution.
----------------------------------------------------------------
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] ArmageddonKnight commented on a change in pull request #19154: Address Leo's comments on PR#18704
Posted by GitBox <gi...@apache.org>.
ArmageddonKnight commented on a change in pull request #19154:
URL: https://github.com/apache/incubator-mxnet/pull/19154#discussion_r489700659
##########
File path: tests/python/gpu/test_profiler_gpu.py
##########
@@ -70,23 +74,20 @@ def test_gpu_memory_profiler_symbolic():
# Sample gpu_memory_profile.csv:
# "Attribute Name","Requested Size","Device","Actual Size","Reuse?"
+ # <unk>:_head_grad_0,16777216,0,16777216,0
# init:_random_uniform,33554432,0,33554432,1
# init:_random_uniform,8388608,0,8388608,1
# resource:temp_space (sample_op.h +365),8,0,4096,0
# symbol:arg_grad:unknown,8388608,0,8388608,0
# symbol:arg_grad:unknown,33554432,0,33554432,0
- # tensordot:dot,16777216,0,16777216,0
- # tensordot:dot_backward,33554432,0,33554432,0
- # tensordot:dot_backward,8388608,0,8388608,0
- # tensordot:dot_head_grad,16777216,0,16777216,0
+ # tensordot:dot0,16777216,0,16777216,0
# tensordot:in_arg:A,8388608,0,8388608,0
# tensordot:in_arg:B,33554432,0,33554432,0
+ # tensordot:node_0_backward,33554432,0,33554432,0
+ # tensordot:node_0_backward,8388608,0,8388608,0s
Review comment:
The reason why they are invoked is because
```Python
# executor.py
def forward(self, is_train=False, **kwargs):
...
with autograd.record(train_mode=is_train):
self.outputs = self._cached_op(*self._args,
default_ctx=default_ctx)
```
->
```Python
# ndarray.py
def __call__(self, *args, **kwargs):
check_call(_LIB.MXInvokeCachedOp(...
```
->
```C++
int MXInvokeCachedOp(...
...
op->Forward(op_shared, ndinputs, ndoutputs, ctx);
```
->
```C++
OpStatePtr CachedOp::Forward(...
...
op_state = DynamicForward(...
```
->
```C++
OpStatePtr CachedOp::DynamicForward(...
...
RunGraph(...
```
->
```C++
void RunGraph(...
...
if (recording) {
Imperative::Get()->RecordOp(...
}
```
----------------------------------------------------------------
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