Skip to content

[FSDP] Fix param name prefixes for ignored modules#79955

Closed
awgu wants to merge 1 commit intopytorch:masterfrom
awgu:fsdp_prefix
Closed

[FSDP] Fix param name prefixes for ignored modules#79955
awgu wants to merge 1 commit intopytorch:masterfrom
awgu:fsdp_prefix

Conversation

@awgu
Copy link
Copy Markdown
Collaborator

@awgu awgu commented Jun 21, 2022

For ignored modules' parameters, we should also clean their parameter names since they will have the FSDP-specific prefixes.

This change only affects the prefixed parameter name keys in full_optim_state_dict() (i.e. optim state dict saving). Not having this change does not actually violate the correctness of the optim state dict save-load flow because it only requires that the keys are unique and internally consistent.

Either way, this PR explicitly adds the specification now that the parameter keys in the optim state dict should match the keys of full model state dict.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Jun 21, 2022

🔗 Helpful links

✅ No Failures (0 Pending)

As of commit ec35a3b (more details on the Dr. CI page):

Expand to see more

💚 💚 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.

Click here to manually regenerate this comment.

@awgu awgu marked this pull request as ready for review June 21, 2022 18:21
@facebook-github-bot facebook-github-bot added the oncall: distributed Add this issue/PR to distributed oncall triage queue label Jun 21, 2022
@facebook-github-bot
Copy link
Copy Markdown
Contributor

@awgu has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@awgu awgu changed the title [FSDP] Fix param names for ignored modules [FSDP] Fix param name prefixes for ignored modules Jun 21, 2022
Copy link
Copy Markdown
Contributor

@rohan-varma rohan-varma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great catch, LGTM

@awgu
Copy link
Copy Markdown
Collaborator Author

awgu commented Jun 21, 2022

@pytorchbot merge

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

@pytorchbot successfully started a merge job. Check the current status here

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

@awgu your PR has been successfully merged.

@github-actions
Copy link
Copy Markdown
Contributor

Hey @awgu.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

facebook-github-bot pushed a commit that referenced this pull request Jun 22, 2022
Summary:
For ignored modules' parameters, we should also clean their parameter names since they will have the FSDP-specific prefixes.

This change only affects the prefixed parameter name keys in `full_optim_state_dict()` (i.e. optim state dict saving). Not having this change does not actually violate the correctness of the optim state dict save-load flow because it only requires that the keys are unique and internally consistent.

Either way, this PR explicitly adds the specification now that the parameter keys in the optim state dict should match the keys of full model state dict.

Pull Request resolved: #79955
Approved by: https://github.com/rohan-varma

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/0b0e65516d88b6ea5ac06598dc8247afe3c21d20

Reviewed By: rohan-varma

Differential Revision: D37320671

Pulled By: awgu

fbshipit-source-id: 576cf28f7f744fcb75f79af1fae7a3fd2c567d89
miladm pushed a commit to miladm/pytorch that referenced this pull request Jun 27, 2022
For ignored modules' parameters, we should also clean their parameter names since they will have the FSDP-specific prefixes.

This change only affects the prefixed parameter name keys in `full_optim_state_dict()` (i.e. optim state dict saving). Not having this change does not actually violate the correctness of the optim state dict save-load flow because it only requires that the keys are unique and internally consistent.

Either way, this PR explicitly adds the specification now that the parameter keys in the optim state dict should match the keys of full model state dict.
Pull Request resolved: pytorch#79955
Approved by: https://github.com/rohan-varma
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 25, 2026
For ignored modules' parameters, we should also clean their parameter names since they will have the FSDP-specific prefixes.

This change only affects the prefixed parameter name keys in `full_optim_state_dict()` (i.e. optim state dict saving). Not having this change does not actually violate the correctness of the optim state dict save-load flow because it only requires that the keys are unique and internally consistent.

Either way, this PR explicitly adds the specification now that the parameter keys in the optim state dict should match the keys of full model state dict.
Pull Request resolved: pytorch#79955
Approved by: https://github.com/rohan-varma
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla signed Merged oncall: distributed Add this issue/PR to distributed oncall triage queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants