Skip to content
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

sync AveragedModel buffers when use_buffers=False #84054

Closed
wants to merge 1 commit into from

Conversation

RangiLyu
Copy link
Contributor

Fixes #84053

As described in the issue, the AveragedModel will deep copy the model during initialization, which means that the buffers in the averaged model cannot be updated together with the model.

One solution is to make the buffers equal to the source model every time when calling update_parameters.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 25, 2022

🔗 Helpful links

✅ No Failures (18 Pending)

As of commit 8dea46b (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.

@dagitses dagitses requested a review from samdow August 29, 2022 13:28
@dagitses dagitses added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 29, 2022
Copy link
Contributor

@samdow samdow left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in this review @RangiLyu, thanks for doing this!

@facebook-github-bot
Copy link
Contributor

/easycla

As part of the transition to the PyTorch Foundation, this project now requires contributions be covered under the new CLA. See #85559 for additional details.

This comment will trigger a new check of this PR. If you are already covered, you will simply see a new "EasyCLA" check that passes. If you are not covered, a bot will leave a new comment with a link to sign.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 4, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@RangiLyu
Copy link
Contributor Author

@samdow @albanD Hi, sorry for bothering. Is there a plan to merge this PR?

@samdow
Copy link
Contributor

samdow commented Oct 21, 2022

Hi sorry--since it's approved, feel free to comment with @pytorchbot merge and it will merge it for you

@RangiLyu
Copy link
Contributor Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR is too stale; the last push date was more than 3 days ago. Please rebase and try again. You can rebase by leaving the following comment on this PR:
@pytorchbot rebase

Details for Dev Infra team Raised by workflow job

@RangiLyu
Copy link
Contributor Author

@pytorchbot rebase

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 24, 2022

You don't have permissions to rebase this PR, only people with write permissions may rebase PRs.

@samdow
Copy link
Contributor

samdow commented Oct 24, 2022

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot
Copy link
Collaborator

Successfully rebased fix_avgmodel_buffer onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout fix_avgmodel_buffer && git pull --rebase)

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 24, 2022

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/84054

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit ad4cd0e:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@samdow
Copy link
Contributor

samdow commented Oct 24, 2022

@pytorchbot merge

@samdow
Copy link
Contributor

samdow commented Oct 24, 2022

Just set up the job so I didn't forget. Sorry about that @RangiLyu and thanks for the PR

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@github-actions
Copy link

Hey @RangiLyu.
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.

sgrigory pushed a commit to sgrigory/pytorch that referenced this pull request Oct 28, 2022
Fixes pytorch#84053

As described in the issue, the AveragedModel will deep copy the model during initialization, which means that the buffers in the averaged model cannot be updated together with the model.

One solution is to make the buffers equal to the source model every time when calling `update_parameters`.
Pull Request resolved: pytorch#84054
Approved by: https://github.com/samdow
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Nov 5, 2022
Fixes pytorch#84053

As described in the issue, the AveragedModel will deep copy the model during initialization, which means that the buffers in the averaged model cannot be updated together with the model.

One solution is to make the buffers equal to the source model every time when calling `update_parameters`.
Pull Request resolved: pytorch#84054
Approved by: https://github.com/samdow
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Dec 10, 2022
Fixes pytorch#84053

As described in the issue, the AveragedModel will deep copy the model during initialization, which means that the buffers in the averaged model cannot be updated together with the model.

One solution is to make the buffers equal to the source model every time when calling `update_parameters`.
Pull Request resolved: pytorch#84054
Approved by: https://github.com/samdow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Buffers in AveragedModel are not synchronized with the source model when use_buffers=False
6 participants