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 2021/07/20 17:41:05 UTC
[GitHub] [incubator-mxnet] barry-jin opened a new pull request #20462: [BUGFIX] Fix #20293
barry-jin opened a new pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462
## Description ##
Fix #20293
Check reference count to decide the req_type if the req_type provided by user is kNullOp.
## Checklist ##
### Essentials ###
- [x] PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
- [x] Changes are complete (i.e. I finished coding on this PR)
- [ ] All changes have test coverage
- [x] Code is well-documented
## Comments ##
- If this change is a backward incompatible change, why must this change be made.
- Interesting edge cases to note 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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#issuecomment-947190641
Jenkins CI successfully triggered : [unix-cpu, centos-gpu, centos-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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha commented on pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
szha commented on pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#issuecomment-953116022
@barry-jin thanks for the fix!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha commented on pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
szha commented on pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#issuecomment-946932194
@barry-jin please add a test and resolve conflict so that we can merge the 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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#issuecomment-888553026
Jenkins CI successfully triggered : [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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] barry-jin commented on a change in pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
barry-jin commented on a change in pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#discussion_r673515352
##########
File path: src/imperative/cached_op.cc
##########
@@ -989,7 +991,12 @@ void CachedOp::StaticBackward(
auto entry = state.info.grad_graph.outputs[iter->second];
if (!idx.exist(entry.node.get())) continue;
auto eid = idx.entry_id(entry);
- state.array_reqs[eid] = reqs[iter->second];
+ // if ref count is not 0, then we should not assign req user provide
+ if (reqs[iter->second] == kNullOp && !(ref_count[eid] == 0)) {
+ state.array_reqs[eid] = kWriteTo;
Review comment:
Yes, we need to put `kWriteTo` here. Because memory plan already happens in [SetBackwardGraph](https://github.com/apache/incubator-mxnet/blob/3480ba2c6df02bb907d3a975d354efa8697c4e71/src/imperative/cached_op.cc#L259) before this. Here we get the updated ref_count from the graph and modify the req_types based on the ref_count.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha merged pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
szha merged pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] barry-jin commented on pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
barry-jin commented on pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#issuecomment-947066264
@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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] barry-jin commented on pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
barry-jin commented on pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#issuecomment-947190616
@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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] barry-jin commented on pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
barry-jin commented on pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#issuecomment-947341572
@mxnet-bot run ci [unix-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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] barry-jin commented on pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
barry-jin commented on pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#issuecomment-888552952
@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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#issuecomment-883574752
Hey @barry-jin , 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-cpu, unix-cpu, unix-gpu, windows-cpu, edge, windows-gpu, centos-gpu, sanity, miscellaneous, website, clang]
***
_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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] ptrendx commented on a change in pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
ptrendx commented on a change in pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#discussion_r673472678
##########
File path: src/imperative/cached_op.cc
##########
@@ -989,7 +991,12 @@ void CachedOp::StaticBackward(
auto entry = state.info.grad_graph.outputs[iter->second];
if (!idx.exist(entry.node.get())) continue;
auto eid = idx.entry_id(entry);
- state.array_reqs[eid] = reqs[iter->second];
+ // if ref count is not 0, then we should not assign req user provide
+ if (reqs[iter->second] == kNullOp && !(ref_count[eid] == 0)) {
+ state.array_reqs[eid] = kWriteTo;
Review comment:
A question, since I do not really know - do we need to put `kWriteTo` here? I think it should be handled by the memory pass later on, right?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] matteosal commented on pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
matteosal commented on pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#issuecomment-942457608
Is this ready to merge?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#issuecomment-947341614
Jenkins CI successfully triggered : [unix-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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] matteosal commented on pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
matteosal commented on pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#issuecomment-942457608
Is this ready to merge?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] matteosal commented on pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
matteosal commented on pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#issuecomment-951996914
Can this be merged now?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20462: [BUGFIX] Fix #20293
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20462:
URL: https://github.com/apache/incubator-mxnet/pull/20462#issuecomment-947066350
Jenkins CI successfully triggered : [website, edge, miscellaneous, windows-cpu, unix-cpu, sanity, centos-cpu, clang, centos-gpu, unix-gpu, 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.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org