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

Enable fused optimizer for DP #98270

Closed
wants to merge 6 commits into from

Conversation

rohan-varma
Copy link
Member

@rohan-varma rohan-varma commented Apr 4, 2023

Stack from ghstack (oldest at bottom):

Differential Revision: D42714482

NOTE FOR REVIEWERS: This PR has internal Meta-specific changes or comments, please review them on Phabricator!

Differential Revision: [D42714482](https://our.internmc.facebook.com/intern/diff/D42714482/)

**NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D42714482/)!

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Apr 4, 2023

🔗 Helpful Links

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

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

✅ No Failures

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

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

@rohan-varma rohan-varma requested a review from d4l3k as a code owner April 4, 2023 02:48
@pytorch-bot pytorch-bot bot added the release notes: distributed (c10d) release notes category label Apr 4, 2023
rohan-varma added a commit that referenced this pull request Apr 4, 2023
Differential Revision: [D42714482](https://our.internmc.facebook.com/intern/diff/D42714482/)

**NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D42714482/)!

ghstack-source-id: 184956702
Pull Request resolved: #98270
Copy link
Contributor

@awgu awgu left a comment

Choose a reason for hiding this comment

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

LGTM. There is one real lint error.

@@ -1464,9 +1464,9 @@ void Reducer::finalize_bucket_dense(Bucket& bucket) {
}

if (!gradient_as_bucket_view_) {
if (set_grads_to_none_) {
if (optim_in_backward_) {
// Return early has optimizer has already run.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Maybe typo?

Suggested change
// Return early has optimizer has already run.
// Return early since optimizer has already run.

if (set_grads_to_none_) {
grad.reset();
if (optim_in_backward_) {
// Return early has optimizer has already run.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above: maybe typo?

Differential Revision: [D42714482](https://our.internmc.facebook.com/intern/diff/D42714482/)

**NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D42714482/)!

[ghstack-poisoned]
rohan-varma added a commit that referenced this pull request Apr 11, 2023
Pull Request resolved: #98270

Enable DDP optimizer overlap for HPC 10x CMF, providing ~7% QPS gain.


ghstack-source-id: 185743906

Differential Revision: [D42714482](https://our.internmc.facebook.com/intern/diff/D42714482/)

**NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D42714482/)!
Differential Revision: [D42714482](https://our.internmc.facebook.com/intern/diff/D42714482/)

**NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D42714482/)!

[ghstack-poisoned]
rohan-varma added a commit that referenced this pull request Apr 11, 2023
Pull Request resolved: #98270

Enable DDP optimizer overlap for HPC 10x CMF, providing ~7% QPS gain.


ghstack-source-id: 185745827

Differential Revision: [D42714482](https://our.internmc.facebook.com/intern/diff/D42714482/)

**NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D42714482/)!
Differential Revision: [D42714482](https://our.internmc.facebook.com/intern/diff/D42714482/)

**NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D42714482/)!

[ghstack-poisoned]
rohan-varma added a commit that referenced this pull request Apr 12, 2023
Pull Request resolved: #98270

Enable DDP optimizer overlap for HPC 10x CMF, providing ~7% QPS gain.


ghstack-source-id: 185811936

Differential Revision: [D42714482](https://our.internmc.facebook.com/intern/diff/D42714482/)

**NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D42714482/)!
Differential Revision: [D42714482](https://our.internmc.facebook.com/intern/diff/D42714482/)

**NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D42714482/)!

[ghstack-poisoned]
rohan-varma added a commit that referenced this pull request Apr 12, 2023
Pull Request resolved: #98270

Enable DDP optimizer overlap for HPC 10x CMF, providing ~7% QPS gain.


ghstack-source-id: 185866613

Differential Revision: [D42714482](https://our.internmc.facebook.com/intern/diff/D42714482/)

**NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D42714482/)!
Differential Revision: [D42714482](https://our.internmc.facebook.com/intern/diff/D42714482/)

**NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D42714482/)!

[ghstack-poisoned]
rohan-varma added a commit that referenced this pull request Apr 12, 2023
Pull Request resolved: #98270

Enable DDP optimizer overlap for HPC 10x CMF, providing ~7% QPS gain.


ghstack-source-id: 185879423

Differential Revision: [D42714482](https://our.internmc.facebook.com/intern/diff/D42714482/)

**NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D42714482/)!
@facebook-github-bot
Copy link
Contributor

@pytorchbot merge

(Initiating merge automatically since Phabricator Diff has merged)

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Apr 13, 2023
@facebook-github-bot
Copy link
Contributor

@pytorchbot merge

(Initiating merge automatically since Phabricator Diff has merged)

@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

ZainRizvi pushed a commit that referenced this pull request Apr 19, 2023
Differential Revision: [D42714482](https://our.internmc.facebook.com/intern/diff/D42714482/)

**NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D42714482/)!
Pull Request resolved: #98270
Approved by: https://github.com/awgu
@facebook-github-bot facebook-github-bot deleted the gh/rohan-varma/657/head branch June 8, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged merging release notes: distributed (c10d) release notes category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants