move unused parameters to end of bucket orders when rebuild buckets for static graph #58097
Conversation
zhaojuanmao
requested review from
H-Huang,
mingzhe09088,
mrshenli,
pritamdamania87,
rohan-varma and
wayi1
as code owners
facebook-github-bot
added
cla signed
oncall: distributed
💊 CI failures summary and remediationsAs of commit 1baa975 (more details on the Dr. CI page):
2 failures not recognized by patterns:
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
|
This pull request was exported from Phabricator. Differential Revision: D28366689 |
zhaojuanmao
changed the title
…or static graph (pytorch#58097) Summary: Pull Request resolved: pytorch#58097 move unused parameters to end of bucket orders when rebuild buckets for static graph Test Plan: unit tests Differential Revision: D28366689 fbshipit-source-id: 76bf8938ef94e248c675a5dbfd88b9c7d640d0e0
|
This pull request was exported from Phabricator. Differential Revision: D28366689 |
zhaojuanmao
force-pushed
the
export-D28366689
branch
from
97b89db to
1baa975
Compare
| // Finally mark variable for which this function was originally called. | ||
| mark_variable_ready(index); | ||
| } | ||
| } else { | ||
| if (should_rebuild_buckets()) { |
There was a problem hiding this comment.
Can we just have it before mark_variable_ready to reduce the code duplication?
There was a problem hiding this comment.
the codes need to be put in different places for static graph and non-static graph case. e.g., when grad hooks are triggered more than once in static graph, we want to push this parameter to 'rebuilt_bucket_indices' only once
| @@ -767,6 +769,11 @@ void Reducer::mark_variable_ready(VariableIndex index) { | |||
| } | |||
| // Check that all buckets were completed and had their work kicked off. | |||
| TORCH_INTERNAL_ASSERT(next_bucket_ == buckets_.size()); | |||
| if (static_graph_after_first_iteration() && should_rebuild_buckets()) { | |||
There was a problem hiding this comment.
nit: Add a comment here mentioning why unused parameters need to be pushed at the end so that it's not accidentally changed by developer later on?
Also curious, are there any workflows/use cases where this could actually negatively affect performance?
There was a problem hiding this comment.
I think this should not result in negative performance, as it is not blocking other all reduces
Codecov Report
@@ Coverage Diff @@
## master #58097 +/- ##
=======================================
Coverage 76.45% 76.46%
=======================================
Files 1992 1992
Lines 199913 199917 +4
=======================================
+ Hits 152853 152859 +6
+ Misses 47060 47058 -2 |
|
This pull request has been merged in ea0f7c4. |
Summary: move unused parameters to end of bucket orders when rebuild buckets for static graph
Test Plan: unit tests
Differential Revision: D28366689