[PGNCCL] Make sure we do not use split for P2P comm creation #139013

kwen2501 · 2024-10-27T18:33:07Z

Stack from ghstack (oldest at bottom):

-> [PGNCCL] Make sure we do not use split for P2P comm creation #139013

There was a split-vs-P2P bug:
When P2P comm creation invokes getNCCLComm, it may see a split_from options which is meant for the previous PG creation. Then the P2P comm creation may use ncclCommSplit and hang, because not all ranks join this call. The bug slips previously/today because there is no CI test with the following recipe: eager init + new group + P2P in that new group.

cc @H-Huang @awgu @wanchaol @fegin @fduwjj @wz337 @wconstab @d4l3k @c-p-i-o

[ghstack-poisoned]

pytorch-bot · 2024-10-27T18:33:10Z

🔗 Helpful Links

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

📄 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.

✅ You can merge normally! (1 Unrelated Failure)

As of commit 3717d2a with merge base 3a6f014 ():

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

pull / cuda12.1-py3.10-gcc9-sm75 / test (pr_time_benchmarks, 1, 1, linux.g4dn.metal.nvidia.gpu) (gh) (trunk failure)
##[error]Process completed with exit code 1.

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

ghstack-source-id: 9425dea Pull Request resolved: #139013

kwen2501 · 2024-10-28T17:55:33Z

@pytorchbot merge

pytorchmergebot · 2024-10-28T17:57:43Z

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 · 2024-10-28T21:28:53Z

@pytorchmergebot revert -c nosignal -m "Sorry but this appears to be breaking on trunk. See: distributed/_composable/test_composability/test_pp_composability.py::ComposabilityTest::test_manual_with_data_parallel_dp_type_DDP_ScheduleClass0_use_new_runtime_False GH job link HUD commit link"

pytorchmergebot · 2024-10-28T21:30:22Z

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

…139013)" This reverts commit 74878ac. Reverted #139013 on behalf of https://github.com/ZainRizvi due to Sorry but this appears to be breaking on trunk. See: distributed/_composable/test_composability/test_pp_composability.py::ComposabilityTest::test_manual_with_data_parallel_dp_type_DDP_ScheduleClass0_use_new_runtime_False [GH job link](https://github.com/pytorch/pytorch/actions/runs/11559910615/job/32177150816) [HUD commit link](https://hud.pytorch.org/pytorch/pytorch/commit/74878ac271feecfa3ff3d32f78c7d889bcac97d6) ([comment](#139013 (comment)))

pytorchmergebot · 2024-10-28T21:30:31Z

@kwen2501 your PR has been successfully reverted.

…ytorch#139013)" This reverts commit 74878ac. Reverted pytorch#139013 on behalf of https://github.com/ZainRizvi due to Sorry but this appears to be breaking on trunk. See: distributed/_composable/test_composability/test_pp_composability.py::ComposabilityTest::test_manual_with_data_parallel_dp_type_DDP_ScheduleClass0_use_new_runtime_False [GH job link](https://github.com/pytorch/pytorch/actions/runs/11559910615/job/32177150816) [HUD commit link](https://hud.pytorch.org/pytorch/pytorch/commit/74878ac271feecfa3ff3d32f78c7d889bcac97d6) ([comment](pytorch#139013 (comment)))

kwen2501 · 2024-11-02T07:40:42Z

@pytorchbot merge

pytorchmergebot · 2024-11-02T07:42:19Z

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

kwen2501 · 2024-11-02T18:14:34Z

@pytorchmergebot revert -c nosignal -m "More flavor of test_manual_with_data_parallel failed"

pytorchmergebot · 2024-11-02T18:16:05Z

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

pytorchmergebot · 2024-11-02T18:16:12Z

Reverting PR 139013 failed

Reason: list index out of range

Details for Dev Infra team

Raised by workflow job

kwen2501 · 2024-11-02T18:17:59Z

@pytorchmergebot revert -c nosignal -m "More flavor of test_manual_with_data_parallel failed"

pytorchmergebot · 2024-11-02T18:19:26Z

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

pytorchmergebot · 2024-11-02T18:19:34Z

Reverting PR 139013 failed

Reason: list index out of range

Details for Dev Infra team

Raised by workflow job

kwen2501 · 2024-11-02T18:27:36Z

@pytorchmergebot revert -c nosignal -m "More flavor of test_manual_with_data_parallel failed"

pytorchmergebot · 2024-11-02T18:29:01Z

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

pytorchmergebot · 2024-11-02T18:29:12Z

@kwen2501 your PR has been successfully reverted.

…139013)" This reverts commit 55038aa. Reverted #139013 on behalf of https://github.com/kwen2501 due to More flavor of test_manual_with_data_parallel failed ([comment](#139013 (comment)))

Resolve comment #138527 (comment) There was a split-vs-P2P bug: When P2P comm creation invokes `getNCCLComm`, it may see a `split_from` options which is meant for the previous PG creation. Then the P2P comm creation may use `ncclCommSplit` and hang, because not all ranks join this call. The bug slips previously/today because there is no CI test with the following recipe: eager init + new group + P2P in that new group. cc H-Huang awgu wanchaol fegin fduwjj wz337 wconstab d4l3k c-p-i-o [ghstack-poisoned]

Also moved test_manual_with_data_parallel to use lazy init to unblock this bug fix ghstack-source-id: 98743fb Pull Request resolved: #139013

…ytorch#139013)" This reverts commit 74878ac. Reverted pytorch#139013 on behalf of https://github.com/ZainRizvi due to Sorry but this appears to be breaking on trunk. See: distributed/_composable/test_composability/test_pp_composability.py::ComposabilityTest::test_manual_with_data_parallel_dp_type_DDP_ScheduleClass0_use_new_runtime_False [GH job link](https://github.com/pytorch/pytorch/actions/runs/11559910615/job/32177150816) [HUD commit link](https://hud.pytorch.org/pytorch/pytorch/commit/74878ac271feecfa3ff3d32f78c7d889bcac97d6) ([comment](pytorch#139013 (comment)))

…#139013) Resolve comment pytorch#138527 (comment) There was a split-vs-P2P bug: When P2P comm creation invokes `getNCCLComm`, it may see a `split_from` options which is meant for the previous PG creation. Then the P2P comm creation may use `ncclCommSplit` and hang, because not all ranks join this call. The bug slips previously/today because there is no CI test with the following recipe: eager init + new group + P2P in that new group. Pull Request resolved: pytorch#139013 Approved by: https://github.com/shuqiangzhang

…ytorch#139013)" This reverts commit 55038aa. Reverted pytorch#139013 on behalf of https://github.com/kwen2501 due to More flavor of test_manual_with_data_parallel failed ([comment](pytorch#139013 (comment)))

kwen2501 · 2024-12-09T17:48:32Z

@pytorchbot merge

pytorchmergebot · 2024-12-09T17:50:22Z

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

[PGNCCL] Make sure we do not use split for P2P comm creation

ff24ad4

[ghstack-poisoned]

pytorch-bot bot added oncall: distributed Add this issue/PR to distributed oncall triage queue release notes: distributed (c10d) release notes category labels Oct 27, 2024

kwen2501 added a commit that referenced this pull request Oct 27, 2024

[PGNCCL] Make sure we do not use split for P2P comm creation

b834536

ghstack-source-id: 9425dea Pull Request resolved: #139013

kwen2501 requested a review from shuqiangzhang October 27, 2024 18:38

kwen2501 added topic: bug fixes topic category ciflow/trunk Trigger trunk jobs on your pull request labels Oct 27, 2024

shuqiangzhang approved these changes Oct 28, 2024

View reviewed changes

pytorchmergebot added the merging label Oct 28, 2024

pytorchmergebot closed this in 74878ac Oct 28, 2024

pytorchmergebot added Merged and removed merging labels Oct 28, 2024

pytorchmergebot added the Reverted label Oct 28, 2024

pytorchmergebot reopened this Oct 28, 2024

kwen2501 mentioned this pull request Nov 2, 2024

DISABLED test_manual_with_data_parallel_dp_type_DDP_ScheduleClass0_use_new_runtime_False (__main__.ComposabilityTest) #139138

Closed

pytorchmergebot added the merging label Nov 2, 2024

pytorchmergebot closed this in 55038aa Nov 2, 2024

pytorchmergebot removed the merging label Nov 2, 2024

kwen2501 reopened this Nov 2, 2024

kwen2501 closed this Nov 2, 2024

pytorchmergebot added the ci-no-td Do not run TD on this PR label Nov 2, 2024

pytorchmergebot reopened this Nov 2, 2024

kwen2501 added the keep-going Don't stop on first failure, keep running tests until the end label Nov 2, 2024

kwen2501 added a commit that referenced this pull request Nov 2, 2024

[PGNCCL] Make sure we do not use split for P2P comm creation

10452fb

Also moved test_manual_with_data_parallel to use lazy init to unblock this bug fix ghstack-source-id: 98743fb Pull Request resolved: #139013

pytorchmergebot added the merging label Dec 9, 2024

pytorchmergebot closed this in d99c9c2 Dec 9, 2024

pytorchmergebot removed the merging label Dec 9, 2024

github-actions bot deleted the gh/kwen2501/88/head branch January 9, 2025 02:20

[PGNCCL] Make sure we do not use split for P2P comm creation #139013

[PGNCCL] Make sure we do not use split for P2P comm creation #139013

Uh oh!

Conversation

kwen2501 commented Oct 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Oct 27, 2024 • 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/139013

✅ You can merge normally! (1 Unrelated Failure)

Uh oh!

kwen2501 commented Oct 28, 2024

Uh oh!

pytorchmergebot commented Oct 28, 2024

Merge started

Uh oh!

ZainRizvi commented Oct 28, 2024

Uh oh!

pytorchmergebot commented Oct 28, 2024

Uh oh!

pytorchmergebot commented Oct 28, 2024

Uh oh!

kwen2501 commented Nov 2, 2024

Uh oh!

pytorchmergebot commented Nov 2, 2024

Merge started

Uh oh!

kwen2501 commented Nov 2, 2024

Uh oh!

pytorchmergebot commented Nov 2, 2024

Uh oh!

pytorchmergebot commented Nov 2, 2024

Reverting PR 139013 failed

Uh oh!

kwen2501 commented Nov 2, 2024

Uh oh!

pytorchmergebot commented Nov 2, 2024

Uh oh!

pytorchmergebot commented Nov 2, 2024

Reverting PR 139013 failed

Uh oh!

kwen2501 commented Nov 2, 2024

Uh oh!

pytorchmergebot commented Nov 2, 2024

Uh oh!

pytorchmergebot commented Nov 2, 2024

Uh oh!

kwen2501 commented Dec 9, 2024

Uh oh!

pytorchmergebot commented Dec 9, 2024

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kwen2501 commented Oct 27, 2024 •

edited

Loading

pytorch-bot bot commented Oct 27, 2024 •

edited

Loading