Skip to content

[DDP] Allow await of custom buffer reduction in backward #64515

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.

Sign up for GitHub

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

Jump to bottom
Closed
rohan-varma wants to merge 12 commits into from
Closed

[DDP] Allow await of custom buffer reduction in backward #64515

rohan-varma wants to merge 12 commits into from

Conversation

@rohan-varma
Copy link
Contributor

@rohan-varma rohan-varma commented Sep 5, 2021 •
edited
Loading

Stack from ghstack:

For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.

Differential Revision: D30757761

cc @pietern @mrshenli @pritamdamania87 @zhaojuanmao @satgera @rohan-varma @gqchen @aazzolini @osalpekar @jiayisuse @agolynski @SciPioneer @H-Huang @mrzzd @cbalioglu @gcramer23

@rohan-varma
[DDP] Allow await of custom buffer reduction in backward
f1942e6 
For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.

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

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 5, 2021 •
edited
Loading

🔗 Helpful links

💊 CI failures summary and remediations

As of commit 9a08564 (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).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

This was referenced Sep 5, 2021
Closed
Closed
Closed
Closed
Closed
@facebook-github-bot facebook-github-bot added the oncall: distributed Add this issue/PR to distributed oncall triage queue label Sep 5, 2021
rohan-varma added a commit that referenced this pull request Sep 5, 2021
@rohan-varma
[DDP] Allow await of custom buffer reduction in backward
457c1b3 
For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.

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

ghstack-source-id: 137398451
Pull Request resolved: #64515
@rohan-varma
Update on "[DDP] Allow await of custom buffer reduction in backward"
0062fd7 
For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.

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

cc pietern mrshenli pritamdamania87 zhaojuanmao satgera rohan-varma gqchen aazzolini osalpekar jiayisuse agolynski @SciPioneer @H-Huang mrzzd cbalioglu gcramer23

[ghstack-poisoned]
rohan-varma added a commit that referenced this pull request Sep 7, 2021
@rohan-varma
[DDP] Allow await of custom buffer reduction in backward
aec0545 
Pull Request resolved: #64515

For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.
ghstack-source-id: 137526311

Differential Revision: [D30757761](https://our.internmc.facebook.com/intern/diff/D30757761/)
zhaojuanmao
zhaojuanmao reviewed Sep 10, 2021
View reviewed changes
torch/csrc/distributed/c10d/init.cpp Outdated
},
py::call_guard<py::gil_scoped_release>())
.def("get_backward_stats", &::c10d::Reducer::get_backward_stats)
.def("_install_futures", [](::c10d::Reducer& reducer, const std::vector<std::shared_ptr<jit::PythonFutureWrapper>>& futs) {
Copy link
Contributor

@zhaojuanmao zhaojuanmao Sep 10, 2021

Choose a reason for hiding this comment

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

nit: _install_post_backward_futures

torch/csrc/distributed/c10d/reducer.cpp
}

void Reducer::install_futures(c10::List<c10::intrusive_ptr<c10::ivalue::Future>> futs) {
installed_futures_ = std::move(futs);
Copy link
Contributor

@zhaojuanmao zhaojuanmao Sep 10, 2021

Choose a reason for hiding this comment

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

could we append futs here instead of moving? it is possible that install_futures() was called multiple times and we need to wait all these installed futures?

Copy link
Contributor Author

@rohan-varma rohan-varma Sep 17, 2021

Choose a reason for hiding this comment

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

Great idea!

torch/testing/_internal/distributed/distributed_test.py
# Note that when custom hooks return futures, this comparison is not expected to work when hook run location is pre-forward pass. This is because the hook does async communication
# and forward pass modifies the buffer without appropriate synchronization. Therefore, if returning futures from custom buffer hooks, it is advised to set hook run location to post
# forward.
if return_futures and hook_run_location == hook_post_fwd:
Copy link
Contributor

@zhaojuanmao zhaojuanmao Sep 10, 2021

Choose a reason for hiding this comment

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

hook_run_location does not have to be 'hook_post_fwd'?

Copy link
Contributor Author

@rohan-varma rohan-varma Sep 17, 2021

Choose a reason for hiding this comment

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

It actually does, if you see the comments on L8017. Basically if we run the hook before forward and return futures, then the user must perform appropriate synchronization in their forward pass for buffers to be equal. This is because the forward pass would use buffers without appropriate synchronization with the NCCL stream if we are returning futures.

For the test we can manually change it to synchronize, but I think the best bet here is to document this and let users know they must explicitly synchronize if they want to use return_futures and hook_run_location == hook_pre_fwd (this is already documented on 1289).

@rohan-varma
Update on "[DDP] Allow await of custom buffer reduction in backward"
9977105 
For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.

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

cc pietern mrshenli pritamdamania87 zhaojuanmao satgera rohan-varma gqchen aazzolini osalpekar jiayisuse agolynski @SciPioneer @H-Huang mrzzd cbalioglu gcramer23

[ghstack-poisoned]
This was referenced Sep 15, 2021
Closed
Closed
Closed
rohan-varma added a commit that referenced this pull request Sep 15, 2021
@rohan-varma
[DDP] Allow await of custom buffer reduction in backward
8787649 
Pull Request resolved: #64515

For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.
ghstack-source-id: 138095560

Differential Revision: [D30757761](https://our.internmc.facebook.com/intern/diff/D30757761/)
@rohan-varma
Update on "[DDP] Allow await of custom buffer reduction in backward"
bd30dd5 
For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.

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

cc pietern mrshenli pritamdamania87 zhaojuanmao satgera rohan-varma gqchen aazzolini osalpekar jiayisuse agolynski @SciPioneer @H-Huang mrzzd cbalioglu gcramer23

[ghstack-poisoned]
rohan-varma added a commit that referenced this pull request Sep 17, 2021
@rohan-varma
[DDP] Allow await of custom buffer reduction in backward
7635505 
Pull Request resolved: #64515

For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.
ghstack-source-id: 138326912

Differential Revision: [D30757761](https://our.internmc.facebook.com/intern/diff/D30757761/)
@rohan-varma
Update on "[DDP] Allow await of custom buffer reduction in backward"
6c3ae1a 
For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.

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

cc pietern mrshenli pritamdamania87 zhaojuanmao satgera rohan-varma gqchen aazzolini osalpekar jiayisuse agolynski @SciPioneer @H-Huang mrzzd cbalioglu gcramer23

[ghstack-poisoned]
@rohan-varma
Update on "[DDP] Allow await of custom buffer reduction in backward"
15c416a 
For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.

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

cc pietern mrshenli pritamdamania87 zhaojuanmao satgera rohan-varma gqchen aazzolini osalpekar jiayisuse agolynski @SciPioneer @H-Huang mrzzd cbalioglu gcramer23

[ghstack-poisoned]
rohan-varma added a commit that referenced this pull request Sep 17, 2021
@rohan-varma
[DDP] Allow await of custom buffer reduction in backward
8d81d83 
Pull Request resolved: #64515

For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.
ghstack-source-id: 138406398

Differential Revision: [D30757761](https://our.internmc.facebook.com/intern/diff/D30757761/)
@rohan-varma
Update on "[DDP] Allow await of custom buffer reduction in backward"
bd81a1b 
For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.

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

cc pietern mrshenli pritamdamania87 zhaojuanmao satgera rohan-varma gqchen aazzolini osalpekar jiayisuse agolynski @SciPioneer @H-Huang mrzzd cbalioglu gcramer23

[ghstack-poisoned]
rohan-varma added a commit that referenced this pull request Sep 21, 2021
@rohan-varma
[DDP] Allow await of custom buffer reduction in backward
8306743 
Pull Request resolved: #64515

For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.
ghstack-source-id: 138564329

Differential Revision: [D30757761](https://our.internmc.facebook.com/intern/diff/D30757761/)
@rohan-varma
Update on "[DDP] Allow await of custom buffer reduction in backward"
c57e172 
For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.

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

cc pietern mrshenli pritamdamania87 zhaojuanmao satgera rohan-varma gqchen aazzolini osalpekar jiayisuse agolynski @SciPioneer @H-Huang mrzzd cbalioglu gcramer23

[ghstack-poisoned]
rohan-varma added a commit that referenced this pull request Sep 21, 2021
@rohan-varma
[DDP] Allow await of custom buffer reduction in backward
76efc37 
Pull Request resolved: #64515

For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.
ghstack-source-id: 138655662

Differential Revision: [D30757761](https://our.internmc.facebook.com/intern/diff/D30757761/)
@rohan-varma
Update on "[DDP] Allow await of custom buffer reduction in backward"
89ac1f2 
For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.

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

cc pietern mrshenli pritamdamania87 zhaojuanmao satgera rohan-varma gqchen aazzolini osalpekar jiayisuse agolynski @SciPioneer @H-Huang mrzzd cbalioglu gcramer23

[ghstack-poisoned]
@rohan-varma
Update on "[DDP] Allow await of custom buffer reduction in backward"
d4e8042 
For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.

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

cc pietern mrshenli pritamdamania87 zhaojuanmao satgera rohan-varma gqchen aazzolini osalpekar jiayisuse agolynski @SciPioneer @H-Huang mrzzd cbalioglu gcramer23

[ghstack-poisoned]
rohan-varma added a commit that referenced this pull request Sep 22, 2021
@rohan-varma
[DDP] Allow await of custom buffer reduction in backward
1c6c835 
Pull Request resolved: #64515

For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.
ghstack-source-id: 138707331

Differential Revision: [D30757761](https://our.internmc.facebook.com/intern/diff/D30757761/)
@rohan-varma
Update on "[DDP] Allow await of custom buffer reduction in backward"
fa28567 
For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.

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

cc pietern mrshenli pritamdamania87 zhaojuanmao satgera rohan-varma gqchen aazzolini osalpekar jiayisuse agolynski @SciPioneer @H-Huang mrzzd cbalioglu gcramer23

[ghstack-poisoned]
@rohan-varma
Update on "[DDP] Allow await of custom buffer reduction in backward"
9a08564 
For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.

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

cc pietern mrshenli pritamdamania87 zhaojuanmao satgera rohan-varma gqchen aazzolini osalpekar jiayisuse agolynski @SciPioneer @H-Huang mrzzd cbalioglu gcramer23

[ghstack-poisoned]
rohan-varma added a commit that referenced this pull request Sep 23, 2021
@rohan-varma
[DDP] Allow await of custom buffer reduction in backward
ae41816 
Pull Request resolved: #64515

For performance reasons, we would like to ensure that we can await
user collectives as part of custom buffer reduction in parallel to other work.
As a result, add support to return futures from custom buffer hooks and await
those futures at end of backwards pass.

Also added some docs to clarify how to use these APIs.
ghstack-source-id: 138793803

Differential Revision: [D30757761](https://our.internmc.facebook.com/intern/diff/D30757761/)
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 23, 2021

This pull request has been merged in 7170434.

@facebook-github-bot facebook-github-bot deleted the gh/rohan-varma/406/head branch September 27, 2021 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhaojuanmao zhaojuanmao zhaojuanmao approved these changes

@cbalioglu cbalioglu Awaiting requested review from cbalioglu

@H-Huang H-Huang Awaiting requested review from H-Huang

@mingzhe09088 mingzhe09088 Awaiting requested review from mingzhe09088

@mrshenli mrshenli Awaiting requested review from mrshenli

@pritamdamania87 pritamdamania87 Awaiting requested review from pritamdamania87

@wayi1 wayi1 Awaiting requested review from wayi1

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rohan-varma @facebook-github-bot @zhaojuanmao