-
Notifications
You must be signed in to change notification settings - Fork 4.9k
zero3: defer param release during retain_graph backward #7352 #8045
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 21 commits
001f77c
b90aee5
b6da9af
bb7f64f
cbc816c
5fcc9a7
f7c5d75
18efbcc
e2ac74d
da07382
5d8875c
316b6dd
2020543
1a8694c
d6725be
a06c548
6959eb4
e88eb3e
683bd0b
b41bb4c
5c75f99
7c5f269
3cfd153
f53eaf0
563acb4
4849fd9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -552,7 +552,13 @@ def post_sub_module_backward_function(self, sub_module): | |
| for param in params_to_fetch: | ||
| param.data = param.data.t() if len(param.ds_shape) != 1 else param.data | ||
|
|
||
| self.get_param_coordinator().release_sub_module(sub_module, forward=False) | ||
| # Keep gathered params alive when the current backward retains the graph, | ||
| # so a second backward over the same forward can reuse valid saved tensors. | ||
| zero_optimizer = getattr(self, "zero_optimizer", None) | ||
| retain_graph_backward = bool(zero_optimizer is not None | ||
| and getattr(zero_optimizer, "retain_graph_on_current_backward", False)) | ||
| if not retain_graph_backward: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When users take the supported torch-style path Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, thanks. You're right that the original fix only set retain_graph_on_current_backward in engine.backward() / ZeROOptimizer.backward(), so the torch-style manual path engine.scale(loss).backward(retain_graph=True) bypassed it and ZeRO-3 still released the gathered params after the first backward. Since autograd hooks can't see the user's retain_graph argument on a direct .backward() call, I propagate it explicitly through engine.scale(): it now accepts a retain_graph argument that sets the flag on the manual path. The flag is reset in the shared _backward_epilogue(), which both the engine.backward() and manual paths reach after the gradient hooks run. Added a regression test test_two_losses_separate_manual_backward_gas1 covering the manual path for ZeRO stages 1/2/3 (two separate backwards over one forward with zero_grad() in between). Both it and the existing test_two_losses_separate_backward_gas1 pass (3 passed each). |
||
| self.get_param_coordinator().release_sub_module(sub_module, forward=False) | ||
|
|
||
| see_memory_usage( | ||
| f"After sub module backward function {sub_module.__class__.__name__} {sub_module.ds_id} after release", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -274,6 +274,7 @@ def __init__( | |
| zero_module_granularity_threshold=zero_module_granularity_threshold, | ||
| log_trace_cache_warnings=log_trace_cache_warnings, | ||
| ) | ||
| self.parameter_offload.zero_optimizer = self | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I understand the trickiness of this feature, I am concerned by the brittleness of he approach:
Can we explore a different approach for propagating
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @sfc-gh-truwase — great point, and I agree with the maintainability concern. This PR is intentionally scoped as a minimal regression fix for #7352 (separate backward with retain_graph=True), so I avoided a broader hook/state refactor here. As a follow-up, I’ll open a cleanup PR to:
If you agree, I’ll link the follow-up issue/PR in this thread.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this sounds great to me. Thanks for valuing maintenability.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @sfc-gh-truwase — I’ve updated the PR to address this. The latest revision removes the I also re-ran the regression coverage for both |
||
|
|
||
| self.persistent_parameters = self.parameter_offload.persistent_parameters | ||
| self._configure_offloading(offload_optimizer_config, offload_param_config) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You call
scale_if_losstwice?