[2/n] Thread PG: add class _World to distributed_c10d.py (#781) #88471
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yhcharles
requested review from
H-Huang,
awgu,
kwen2501,
mrshenli,
pritamdamania87,
rohan-varma and
zhaojuanmao
as code owners
pytorch-bot
bot
added
the
release notes: distributed (c10d)
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/88471
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit a47a15c: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D40236769 |
1 similar comment
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D40236769 |
yhcharles
force-pushed
the
export-D40236769
branch
from
1d972b7 to
322f94d
Compare
H-Huang
reviewed
Nov 4, 2022
Comment on lines
+278
to
+281
Member
There was a problem hiding this comment.
Are these vars only used by class _World? Why not put them under that class. You can define them as attributes, then also you dont need to call global abc each time you access.
Contributor
Author
There was a problem hiding this comment.
They are used in some other projects. So we have to keep them. Let me ping you some examples.
Collaborator
There was a problem hiding this comment.
Hey I randomly browsed into this piece of code. I am wondering why we need to keep them? I know it's being used in other projects, but I feel since we make all those variables be private variables, shouldn't that mean we are free to change this to whatever we like without worrying about BC?
If it's internal change related, maybe we can just refactor it directly.
Member
There was a problem hiding this comment.
FYI I have a PR to refactor some of this code since this and the if statements below are basically repeated, #88351. Feel free to land yours first if you are ready though
rohan-varma
approved these changes
Nov 4, 2022
Contributor
rohan-varma
left a comment
rohan-varma
left a comment
There was a problem hiding this comment.
Stamp to unblock, but please address @H-Huang's comments
Summary: Pull Request resolved: pytorch#88471 X-link: meta-pytorch/torchrec#781 Move a bunch of globals to instance methods and replace all use to them. We move all PG related globals under World and use a singleton instance under _world. This creates an undocumented extension point to inject full control of how how c10d state behaves. One simple hack is to change _world to an implementation that uses a threadlocal and enable per-thread PGs. It almost get DDP working and the PG is missing an implementation of all_reduce. This enables notebook usage of PTD, which is a big deal for learning it: https://gist.github.com/kumpera/32cb051fa26b8cad8bdf671f968dcd68 This change ensures BC by keeping the global variables around and have the default _World wrap it. I have relinked this diff to a new github PR, so that I can update it. The original PR is > Pull Request resolved: pytorch#86348 Reviewed By: gnadathur Differential Revision: D40236769 Pulled By: yhcharles fbshipit-source-id: efeaa7990e26a58987769a93cedf7318d5cae445
yhcharles
pushed a commit
to yhcharles/torchrec
that referenced
this pull request
Nov 4, 2022
Summary: X-link: pytorch/pytorch#88471 Pull Request resolved: meta-pytorch#781 Move a bunch of globals to instance methods and replace all use to them. We move all PG related globals under World and use a singleton instance under _world. This creates an undocumented extension point to inject full control of how how c10d state behaves. One simple hack is to change _world to an implementation that uses a threadlocal and enable per-thread PGs. It almost get DDP working and the PG is missing an implementation of all_reduce. This enables notebook usage of PTD, which is a big deal for learning it: https://gist.github.com/kumpera/32cb051fa26b8cad8bdf671f968dcd68 This change ensures BC by keeping the global variables around and have the default _World wrap it. I have relinked this diff to a new github PR, so that I can update it. The original PR is > Pull Request resolved: pytorch/pytorch#86348 Reviewed By: gnadathur Differential Revision: D40236769 Pulled By: yhcharles fbshipit-source-id: ebd6080e4923da549800a048f089fa0bb69eb331
yhcharles
force-pushed
the
export-D40236769
branch
from
322f94d to
a47a15c
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D40236769 |
facebook-github-bot
pushed a commit
to meta-pytorch/torchrec
that referenced
this pull request
Nov 7, 2022
Summary: X-link: pytorch/pytorch#88471 Pull Request resolved: #781 Move a bunch of globals to instance methods and replace all use to them. We move all PG related globals under World and use a singleton instance under _world. This creates an undocumented extension point to inject full control of how how c10d state behaves. One simple hack is to change _world to an implementation that uses a threadlocal and enable per-thread PGs. It almost get DDP working and the PG is missing an implementation of all_reduce. This enables notebook usage of PTD, which is a big deal for learning it: https://gist.github.com/kumpera/32cb051fa26b8cad8bdf671f968dcd68 This change ensures BC by keeping the global variables around and have the default _World wrap it. I have relinked this diff to a new github PR, so that I can update it. The original PR is > Pull Request resolved: pytorch/pytorch#86348 Reviewed By: gnadathur Differential Revision: D40236769 Pulled By: yhcharles fbshipit-source-id: c6aecff5b0801938713f867827d0d3b4b5c906e6
Contributor
|
@pytorchbot merge (Initiating merge automatically since Phabricator Diff has merged) |
Collaborator
Merge startedYour 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 |
lequytra
pushed a commit
to lequytra/torchrec
that referenced
this pull request
Dec 6, 2022
Summary: X-link: pytorch/pytorch#88471 Pull Request resolved: meta-pytorch#781 Move a bunch of globals to instance methods and replace all use to them. We move all PG related globals under World and use a singleton instance under _world. This creates an undocumented extension point to inject full control of how how c10d state behaves. One simple hack is to change _world to an implementation that uses a threadlocal and enable per-thread PGs. It almost get DDP working and the PG is missing an implementation of all_reduce. This enables notebook usage of PTD, which is a big deal for learning it: https://gist.github.com/kumpera/32cb051fa26b8cad8bdf671f968dcd68 This change ensures BC by keeping the global variables around and have the default _World wrap it. I have relinked this diff to a new github PR, so that I can update it. The original PR is > Pull Request resolved: pytorch/pytorch#86348 Reviewed By: gnadathur Differential Revision: D40236769 Pulled By: yhcharles fbshipit-source-id: c6aecff5b0801938713f867827d0d3b4b5c906e6
kulinseth
pushed a commit
to kulinseth/pytorch
that referenced
this pull request
Dec 10, 2022
pytorch#88471) Summary: X-link: meta-pytorch/torchrec#781 Move a bunch of globals to instance methods and replace all use to them. We move all PG related globals under World and use a singleton instance under _world. This creates an undocumented extension point to inject full control of how how c10d state behaves. One simple hack is to change _world to an implementation that uses a threadlocal and enable per-thread PGs. It almost get DDP working and the PG is missing an implementation of all_reduce. This enables notebook usage of PTD, which is a big deal for learning it: https://gist.github.com/kumpera/32cb051fa26b8cad8bdf671f968dcd68 This change ensures BC by keeping the global variables around and have the default _World wrap it. I have relinked this diff to a new github PR, so that I can update it. The original PR is > Pull Request resolved: pytorch#86348 Differential Revision: D40236769 Pulled By: yhcharles Pull Request resolved: pytorch#88471 Approved by: https://github.com/gnadathur, https://github.com/rohan-varma
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
X-link: meta-pytorch/torchrec#781
Move a bunch of globals to instance methods and replace all use to them.
We move all PG related globals under World and use a singleton instance under _world.
This creates an undocumented extension point to inject full control of how how c10d
state behaves.
One simple hack is to change _world to an implementation that uses a threadlocal
and enable per-thread PGs.
It almost get DDP working and the PG is missing an implementation of all_reduce.
This enables notebook usage of PTD, which is a big deal for learning it:
https://gist.github.com/kumpera/32cb051fa26b8cad8bdf671f968dcd68
This change ensures BC by keeping the global variables around and have the default _World wrap it.
I have relinked this diff to a new github PR, so that I can update it. The original PR is
Differential Revision: D40236769
Pulled By: yhcharles