-
Notifications
You must be signed in to change notification settings - Fork 26.3k
[FSDP] Option to summon on rank 0 only #73903
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
Conversation
Add an option to summon full parameter on rank0_only. Note that as documented, when using this flag, writes are only guaranteed to persist for rank 0. Differential Revision: [D34706449](https://our.internmc.facebook.com/intern/diff/D34706449/) [ghstack-poisoned]
CI Flow Status⚛️ CI FlowRuleset - Version:
|
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 3943ef5 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
Add an option to summon full parameter on rank0_only. Note that as documented, when using this flag, writes are only guaranteed to persist for rank 0. Differential Revision: [D34706449](https://our.internmc.facebook.com/intern/diff/D34706449/) [ghstack-poisoned]
fegin
left a comment
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.
LGTM. Just one comment about how the combination of the arguments summon_full_param() are handled.
| materialized on only global rank 0. This means that within the | ||
| context, only rank 0 will have full parameters and the other | ||
| ranks will have sharded parameters. This further means that for | ||
| non-zero ranks, the `writeback` argument is ignored and |
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.
The comment is not complete?
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.
Instead of implicitly ignoring writeback, should we ask users to explicitly writeback to False?
zhaojuanmao
left a comment
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.
awesome, looks god!
Add an option to summon full parameter on rank0_only. Note that as documented, when using this flag, writes are only guaranteed to persist for rank 0. Differential Revision: [D34706449](https://our.internmc.facebook.com/intern/diff/D34706449/) [ghstack-poisoned]
Pull Request resolved: #73903 Add an option to summon full parameter on rank0_only. Note that as documented, when using this flag, writes are only guaranteed to persist for rank 0. ghstack-source-id: 150949236 Differential Revision: [D34706449](https://our.internmc.facebook.com/intern/diff/D34706449/)
Add an option to summon full parameter on rank0_only. writeback=True is not supported. Differential Revision: [D34706449](https://our.internmc.facebook.com/intern/diff/D34706449/) [ghstack-poisoned]
Summary: Pull Request resolved: #73903 Add an option to summon full parameter on rank0_only. Note that as documented, when using this flag, writes are only guaranteed to persist for rank 0. ghstack-source-id: 151311233 Test Plan: CI Reviewed By: zhaojuanmao Differential Revision: D34706449 fbshipit-source-id: 034d76cdae6916a225bd6fcee092db48359f8a0b
Stack from ghstack (oldest at bottom):
Add an option to summon full parameter on rank0_only. writeback=True is not supported.
Differential Revision: D34706449