Opportunistically use `ncclCommSplit` when creating new NCCL groups #112889

chipturner · 2023-11-03T19:10:34Z

Currently ncclCommInitRankConfig is always used when creating new
communicator groups. This is wasteful as it creates non-shared pairs
of endpoint queues as well as costs time to re-establish
communication.

This change is transparent and opportunistic; when dist.new_group is
called, it will use the existing, healthy world process group to
select the right ranks to include in the process group.

pytorch-bot · 2023-11-03T19:10:39Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

✅ No Failures

As of commit d90c747 with merge base 4b7f9fa ():
💚 Looks good so far! There are no failures yet. 💚

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

linux-foundation-easycla · 2023-11-03T19:10:41Z

The committers listed above are authorized under a signed CLA.

✅ login: chipturner / name: Chip Turner (d90c747)

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

torch/csrc/distributed/c10d/ProcessGroupNCCL.hpp

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

torch/csrc/distributed/c10d/init.cpp

kwen2501 · 2023-11-06T17:14:11Z

torch/distributed/distributed_c10d.py

Synchronized between? Can the comment specify it?

whoops, we don't even need this anymore. removing.

torch/distributed/distributed_c10d.py

kwen2501 · 2023-11-06T18:06:18Z

I wonder if we would eventually need to expose the "color" via one/some of the new_group API family (new_group, new_subgroups, etc).

new_subgroups might be a nice candidate. Because its current definition of: it creates intra-machine subgroups, where each of which contains all the ranks of a machine, based on the assumption that each machine has the same number of devices is unsound. (In general, we cannot assume two ranks are on the same machine without doing detection. And detection on PyTorch level (esp python) would be too non-abstract).

kwen2501

LGTM.
I just have 3 minor comments.

torch/csrc/distributed/c10d/ProcessGroupNCCL.hpp

torch/csrc/distributed/c10d/init.cpp

torch/csrc/distributed/c10d/NCCLUtils.hpp

kwen2501 · 2023-11-20T16:13:46Z

I resolved the three last comments. I believe we are good to go. Thanks!

Currently `ncclCommInitRankConfig` is always used when creating new communicator groups. This is wasteful as it creates non-shared pairs of endpoint queues as well as costs time to re-establish communication. This change is transparent and opportunistic; when `dist.new_group` is called and the new group includes all ranks, it will use the existing, healthy world process group to select the right ranks to include in the process group.

chipturner · 2023-11-21T18:36:41Z

@pytorchmergebot merge

pytorchmergebot · 2023-11-21T18:38:41Z

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

huydhn · 2023-11-22T17:42:06Z

@pytorchbot revert -m 'Sorry for reverting you change, but it is failing ROCm distributed jobs in trunk https://hud.pytorch.org/pytorch/pytorch/commit/4d07428edee863e7f5920f0672957a9711a9f0b5' -c nosignal

The error is 'torch._C._distributed_c10d.Options' object has no attribute 'split_from'. Please take a look and reland the change. I will add ciflow/periodic to run the test on your pull request.

pytorchmergebot · 2023-11-22T17:43:46Z

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

pytorchmergebot · 2023-11-22T17:43:56Z

@chipturner your PR has been successfully reverted.

…groups (#112889)" This reverts commit 64a5372. Reverted #112889 on behalf of https://github.com/huydhn due to Sorry for reverting you change, but it is failing ROCm distributed jobs in trunk https://hud.pytorch.org/pytorch/pytorch/commit/4d07428edee863e7f5920f0672957a9711a9f0b5 ([comment](#112889 (comment)))

pytorchmergebot · 2023-11-22T17:43:58Z

Reverting PR 112889 failed

Reason: HTTP Error 422: Unprocessable Entity

Details for Dev Infra team

Raised by workflow job

…w NCCL groups (pytorch#112889) Currently `ncclCommInitRankConfig` is always used when creating new communicator groups. This is wasteful as it creates non-shared pairs of endpoint queues as well as costs time to re-establish communication. This change is transparent and opportunistic; when `dist.new_group` is called, it will use the existing, healthy world process group to select the right ranks to include in the process group. Original PR: pytorch#112889 Reverted due to ROCm incompatibility

…NCCL groups) (#114385) - [c10d] (retry) Opportunistically use `ncclCommSplit` when creating new NCCL groups (#112889) - Guard use of `split_from` with a `hasattr` check for cases when NCCL (or RCCL) lacks `ncclCommSplit` Fixes cause of revert of original PR Pull Request resolved: #114385 Approved by: https://github.com/huydhn

…114423) When reopening a reverted PR, `422: Unprocessable Entity` is returned when the head branch has been deleted, for example #112889 (comment) ``` { "message": "Validation Failed", "errors": [ { "resource": "PullRequest", "code": "custom", "field": "state", "message": "state cannot be changed. The commsplit branch has been deleted." } ], "documentation_url": "https://docs.github.com/rest/pulls/pulls#update-a-pull-request" } ``` The revert still happens though, only reopening PR fails, which is ok to ignore in this case I think instead of going the complicated route of trying to restore the deleted branch by merge bot. Pull Request resolved: #114423 Approved by: https://github.com/malfet, https://github.com/kit1980

mengpenghui · 2024-06-27T09:44:04Z

@chipturner Can you provide a pytorch test case to quantify the performance improvement brought by comm_split?

pytorch-bot bot added the release notes: distributed (c10d) release notes category label Nov 3, 2023

chipturner marked this pull request as ready for review November 3, 2023 19:22

chipturner requested review from H-Huang, LucasLLC, awgu, d4l3k, fduwjj, fegin, kiukchung, kwen2501, mrshenli, rohan-varma, wanchaol, wz337 and zhaojuanmao as code owners November 3, 2023 19:22

chipturner force-pushed the commsplit branch 2 times, most recently from 0ea2d4b to 0ceb454 Compare November 6, 2023 02:06

kwen2501 reviewed Nov 6, 2023

View reviewed changes

chipturner force-pushed the commsplit branch 7 times, most recently from c815b98 to b0adac9 Compare November 13, 2023 21:07

kwen2501 approved these changes Nov 17, 2023

View reviewed changes

torch/csrc/distributed/c10d/ProcessGroupNCCL.hpp Outdated Show resolved Hide resolved

torch/csrc/distributed/c10d/init.cpp Outdated Show resolved Hide resolved

torch/csrc/distributed/c10d/NCCLUtils.hpp Outdated Show resolved Hide resolved

chipturner force-pushed the commsplit branch 2 times, most recently from 7fcf3cd to be8a570 Compare November 20, 2023 20:26

chipturner force-pushed the commsplit branch from be8a570 to d90c747 Compare November 21, 2023 14:53

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 21, 2023

pytorchmergebot added the merging label Nov 21, 2023

pytorchmergebot added Merged and removed merging labels Nov 21, 2023

pytorchmergebot closed this in 64a5372 Nov 21, 2023

chipturner deleted the commsplit branch November 21, 2023 21:59

pytorchmergebot added the Reverted label Nov 22, 2023

chipturner mentioned this pull request Nov 22, 2023

Retry #112889 (Opportunistically use ncclCommSplit when creating new NCCL groups) #114385

Closed

huydhn mentioned this pull request Nov 22, 2023

Handle the case when opening a reverted PR with deleted head branch #114423

Closed

ajindal1 mentioned this pull request Dec 5, 2023

Undefined reference to ncclCommSplit #114884

Open

Opportunistically use ncclCommSplit when creating new NCCL groups #112889

Opportunistically use ncclCommSplit when creating new NCCL groups #112889

Uh oh!

Conversation

chipturner commented Nov 3, 2023

Uh oh!

pytorch-bot bot commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

✅ No Failures

Uh oh!

linux-foundation-easycla bot commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kwen2501 Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

chipturner Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kwen2501 commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kwen2501 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kwen2501 commented Nov 20, 2023

Uh oh!

chipturner commented Nov 21, 2023

Uh oh!

pytorchmergebot commented Nov 21, 2023

Merge started

Uh oh!

huydhn commented Nov 22, 2023

Uh oh!

pytorchmergebot commented Nov 22, 2023

Uh oh!

pytorchmergebot commented Nov 22, 2023

Uh oh!

pytorchmergebot commented Nov 22, 2023

Reverting PR 112889 failed

Uh oh!

mengpenghui commented Jun 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Opportunistically use `ncclCommSplit` when creating new NCCL groups #112889

Opportunistically use `ncclCommSplit` when creating new NCCL groups #112889

pytorch-bot bot commented Nov 3, 2023 •

edited

Loading

linux-foundation-easycla bot commented Nov 3, 2023 •

edited

Loading

kwen2501 commented Nov 6, 2023 •

edited

Loading