[FSDP] named_buffers fix #74517
[FSDP] named_buffers fix #74517
Conversation
Partially addresses #73890 to fix named_buffers by stripping FSDP info in summon_full_params context, similar to named_params. Differential Revision: [D35023191](https://our.internmc.facebook.com/intern/diff/D35023191/) [ghstack-poisoned]
rohan-varma
requested review from
H-Huang,
mingzhe09088,
mrshenli,
pritamdamania87 and
zhaojuanmao
as code owners
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 88fd8c8 (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. |
facebook-github-bot
added
the
oncall: distributed
Partially addresses #73890 to fix named_buffers by stripping FSDP info in summon_full_params context, similar to named_params. Differential Revision: [D35023191](https://our.internmc.facebook.com/intern/diff/D35023191/) [ghstack-poisoned]
Partially addresses #73890 to fix named_buffers by stripping FSDP info in summon_full_params context, similar to named_params. Differential Revision: [D35023191](https://our.internmc.facebook.com/intern/diff/D35023191/) [ghstack-poisoned]
Pull Request resolved: #74517 Partially addresses #73890 to fix named_buffers by stripping FSDP info in summon_full_params context, similar to named_params. ghstack-source-id: 152309646 Differential Revision: [D35023191](https://our.internmc.facebook.com/intern/diff/D35023191/)
|
CI did not run properly but it all passed on 46d93e7, landing |
| remove all occurrences of the FSDP-specific flattened buffer prefix | ||
| when inside the :meth:`summon_full_params` context manager. | ||
| """ | ||
| in_summon_full_params = getattr(self, "training_state", None) == \ |
There was a problem hiding this comment.
I'm curious on why can't we access training_state directly here.
This property is unconditionally initialized in FSDP's ctor and there's no del calls.
There was a problem hiding this comment.
It is my fault for not digging into this clearly when landing the named_parameters() override.
We use this getattr() check for named_parameters() because the constructor calls named_parameters() (twice) before setting training_state.
This should not be needed for named_buffers() since named_buffers() is not called in the constructor.
My thought is that we can either:
- move the definition of
self.training_state = TrainingState_.IDLEto before the first usage ofnamed_parameters()in the constructor, or - keep
named_parameters()as is, add a comment explaining why we usegetattr(), and remove thegetattr()fromnamed_buffers().
There was a problem hiding this comment.
Summary: Pull Request resolved: #74517 Partially addresses #73890 to fix named_buffers by stripping FSDP info in summon_full_params context, similar to named_params. ghstack-source-id: 152309646 (Note: this ignores all push blocking failures!) Test Plan: CI Reviewed By: zhaojuanmao Differential Revision: D35023191 fbshipit-source-id: 091c7afa73c595f54e303dbfc938010e08278d64
|
Hey @rohan-varma. |
Stack from ghstack (oldest at bottom):
Partially addresses #73890 to fix named_buffers by stripping FSDP info in summon_full_params context, similar to named_params.
Differential Revision: D35023191