[c10d] fix a logic of using ncclCommSplit #138781
Conversation
Summary: Currently, whether split should be used depends on the size of subgroup. It's possible that default PG is not eagerly initialized yet, but split is still called. This PR fixes this issue by removing split's dependency on subgroup size Test Plan: Modified UT Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
pytorch-bot
bot
added
oncall: distributed
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/138781
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 415abc2 with merge base 8aedc64 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Summary: Currently, whether split should be used depends on the size of subgroup. It's possible that default PG is not eagerly initialized yet, but split is still called. This PR fixes this issue by removing split's dependency on subgroup size Test Plan: Modified UT Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 45e177e Pull Request resolved: #138781
There was a problem hiding this comment.
Code change LGTM.
I wonder if we could add back some tests to cover subgroups creation using split?
I am adding eager / lazy parameterization in this PR: #138644
Let me know if you'd like to stack it on top of this PR or merge it in early.
There are still/already many tests with new group creation using split, e.g. test_comm_split_subgroup, test_non_blocking_with_eager_init, etc |
|
@pytorchbot merge |
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 |
Merge failedReason: 1 jobs have failed, first few of them are: trunk / linux-focal-rocm6.2-py3.10 / build Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -f "unrelated failure" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
#138665) Summary: Currently, eager mode is applied either to all PGs or NONE of them. There are cases where we don't want to initialize the comms for default PG, but we still want to initialize the comms for sub PG. Now with a device_id passed to new group, we can achieve this case Test Plan: newly added UT Tags: Resolves #137018 Pull Request resolved: #138665 Approved by: https://github.com/kwen2501 ghstack dependencies: #138781
3 participants
Stack from ghstack (oldest at bottom):
Summary:
Currently, whether split should be used depends on the size of subgroup.
It's possible that default PG is not eagerly initialized yet, but split is still
called.
This PR fixes this issue by removing split's dependency on subgroup size
Test Plan:
Modified UT
Reviewers:
Subscribers:
Tasks:
Tags:
cc @H-Huang @awgu @kwen2501 @wanchaol @fegin @fduwjj @wz337 @wconstab @d4l3k @c-p-i-o