[PGNCCL] Use non-blocking mode by default in eager init #138527

kwen2501 · 2024-10-21T23:46:31Z

Stack from ghstack (oldest at bottom):

Why use non-blocking mode in eager init?

For overlapping comm init and model init, etc.

Why can we set non-blocking as default?

If the setting is dangling -- i.e. not passed in by user nor set via env -- ProcessGroupNCCL can have some preferred logic. And torch-level API semantics does not change whether the NCCL comm is blocking or non-blocking (handled within ProcessGroupNCCL).

Why not make non-blocking default for lazy mode as well?

PR #137544 tried it.
Two reasons why that's not preferred today:

It is hard -- too big a blast.
There is no gain by doing lazy init in non-blocking mode, because the right next CPU call is a collective, and we will block there waiting for comm to be ready, so same effect as blocked init, no "opening" compared to eager mode.

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

[ghstack-poisoned]

pytorch-bot · 2024-10-21T23:46:35Z

🔗 Helpful Links

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

📄 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 821fc92 with merge base 4c91481 ():

FLAKY - The following job failed but was likely due to flakiness present on trunk:

trunk / linux-focal-rocm6.2-py3.10 / test (distributed, 1, 1, linux.rocm.gpu) (gh) (disabled by #139011)
distributed/test_c10d_nccl.py::ProcessGroupNCCLGroupTest::test_extra_cuda_context

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

ghstack-source-id: e7b5942 Pull Request resolved: #138527

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

wconstab · 2024-10-22T01:07:40Z

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

+  }
+  // 2nd priority: Respect the environment variable
+  if (nbEnv.has_value()) {
+    useNonblocking_ = nbEnv.value();


hm, do we need to check if bound device id exists here too? i thought we could not enable nonblocking without a bound id? maybe i just got confused by the bundling.

The env of non-blocking existed before we introduce bounded device.

wconstab · 2024-10-22T01:08:15Z

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

+    useNonblocking_ = true;
+    goto print_and_return;
+  }
+  // 4th priority: otherwise, nonblocking = false to preserve old behavior


lgtm, but i wonder if longer term, is this a path we'd want to add a 'deprecation warning' to start encouraging users to add device_id?

wconstab · 2024-10-22T01:09:56Z

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

+  useNonblocking_ = false;
+
+print_and_return:
+  LOG(INFO) << logPrefix()


nit: this is ok, but i think its also useful to log which of the config pathways took precedent, in case users want to debug why their nonblocking setting wasn't as expected

wconstab

overall lgtm.

shuqiangzhang · 2024-10-22T21:30:26Z

Nit: Better to add a test case with eager init but without setting the ENV?

kwen2501 · 2024-10-23T04:51:03Z

@shuqiangzhang Yeah, that's a good idea. I added a PR on top to parameterize tests in test_c10d_nccl.py to do both lazy and eager inits. See #138644.

kwen2501 · 2024-10-23T04:52:59Z

@pytorchbot merge

pytorchmergebot · 2024-10-23T04:54:37Z

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

jeanschmidt · 2024-10-23T14:48:04Z

@pytorchbot revert -m "Seems to have introduce regressions on main, pull / linux-focal-cuda11.8-py3.10-gcc9 / test (distributed, 2, 3, linux.g4dn.12xlarge.nvidia.gpu) checking if revert will do" -c nosignal

pytorchmergebot · 2024-10-23T14:49:42Z

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

)" This reverts commit 8fbf866. Reverted #138527 on behalf of https://github.com/jeanschmidt due to Seems to have introduce regressions on main, pull / linux-focal-cuda11.8-py3.10-gcc9 / test (distributed, 2, 3, linux.g4dn.12xlarge.nvidia.gpu) checking if revert will do ([comment](#138527 (comment)))

pytorchmergebot · 2024-10-26T02:09:54Z

Merge started

Your change will be merged while ignoring the following 1 checks: trunk / linux-focal-rocm6.2-py3.10 / test (distributed, 1, 1, linux.rocm.gpu)

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 · 2024-10-27T15:36:48Z

@pytorchbot revert -m 'Sorry for reverting your change, but it is failing on ROCm' -c weird

distributed/test_c10d_nccl.py::ProcessGroupNCCLGroupTest::test_extra_cuda_context GH job link HUD commit link

pytorchmergebot · 2024-10-27T15:39:24Z

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

)" This reverts commit 07e30ea. Reverted #138527 on behalf of https://github.com/huydhn due to Sorry for reverting your change, but it is failing on ROCm ([comment](#138527 (comment)))

pytorchmergebot · 2024-10-27T15:39:36Z

@kwen2501 your PR has been successfully reverted.

kwen2501 · 2024-10-27T17:37:58Z

@huydhn Thanks for the notification. It seems test_extra_cuda_context was flaky for rocm since 10/23, before my PR's landed. So I disabled that test. And relanding this PR.

kwen2501 · 2024-10-27T17:39:08Z

@pytorchbot merge -f "test_extra_cuda_context was flaky for rocm since 10/23, before my PR's landed. So I disabled that test"

pytorchmergebot · 2024-10-27T17:40:29Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

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

### Why use non-blocking mode in eager init? For overlapping comm init and model init, etc. ![image](https://github.com/user-attachments/assets/9b0bf7a9-be26-4d16-827b-dbe861f083cd) ### Why can we set non-blocking as default? If the setting is dangling -- i.e. not passed in by user nor set via env -- `ProcessGroupNCCL` can have some preferred logic. And torch-level API semantics does not change whether the NCCL comm is blocking or non-blocking (handled within `ProcessGroupNCCL`). ### Why not make non-blocking default for lazy mode as well? PR #137544 tried it. Two reasons why that's not preferred today: 1. It is hard -- too big a blast. 2. There is no gain by doing lazy init in non-blocking mode, because the right next CPU call is a collective, and we will block there waiting for comm to be ready, so same effect as blocked init, no "opening" compared to eager mode. Pull Request resolved: #138527 Approved by: https://github.com/wconstab ghstack dependencies: #137855, #138488, #138374, #138384

)" This reverts commit 8fbf866. Reverted #138527 on behalf of https://github.com/jeanschmidt due to Seems to have introduce regressions on main, pull / linux-focal-cuda11.8-py3.10-gcc9 / test (distributed, 2, 3, linux.g4dn.12xlarge.nvidia.gpu) checking if revert will do ([comment](#138527 (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. Pull Request resolved: #139013 Approved by: https://github.com/shuqiangzhang

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]

…#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

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. Pull Request resolved: #139013 Approved by: https://github.com/shuqiangzhang

[PGNCCL] Use non-blocking mode by default in eager init

6986fc4

[ghstack-poisoned]

This was referenced Oct 21, 2024

[PGNCCL] Add default value for nccl_nonblocking_timeout #138374

Closed

[PGNCCL] Ensure comm is ready before all accesses #138384

Closed

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

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

[PGNCCL] Use non-blocking mode by default in eager init

a998005

ghstack-source-id: e7b5942 Pull Request resolved: #138527

kwen2501 requested review from eqy, fduwjj, shuqiangzhang and wconstab October 22, 2024 00:04

wconstab reviewed Oct 22, 2024

View reviewed changes

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

wconstab reviewed Oct 22, 2024

View reviewed changes

wconstab approved these changes Oct 22, 2024

View reviewed changes

kwen2501 mentioned this pull request Oct 22, 2024

[Distributed] Add more tests to use eager init #138644

Closed

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 23, 2024

pytorchmergebot added the merging label Oct 23, 2024

pytorchmergebot added the Merged label Oct 23, 2024

pytorchmergebot closed this in 8fbf866 Oct 23, 2024

pytorchmergebot removed the merging label Oct 23, 2024

pytorchmergebot added the merging label Oct 26, 2024

pytorchmergebot closed this in 07e30ea Oct 26, 2024

pytorchmergebot removed the merging label Oct 26, 2024

pytorchmergebot reopened this Oct 27, 2024

pytorchmergebot added the merging label Oct 27, 2024

pytorchmergebot closed this in ee11e2d Oct 27, 2024

pytorchmergebot removed the merging label Oct 27, 2024

kwen2501 mentioned this pull request Oct 27, 2024

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

Closed

kwen2501 mentioned this pull request Nov 2, 2024

[PGNCCL] Enable non-blocking API mode by default #137544

Closed

github-actions bot deleted the gh/kwen2501/81/head branch November 28, 2024 02:13

oraluben mentioned this pull request Jan 27, 2025

Fix support for nccl < 2.17 #145719

Closed

carmocca mentioned this pull request Jan 30, 2025

[distributed] First NCCL barrier does not respect timeout #124950

Open

[PGNCCL] Use non-blocking mode by default in eager init #138527

[PGNCCL] Use non-blocking mode by default in eager init #138527

Uh oh!

Conversation

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

Uh oh!

Why use non-blocking mode in eager init?

Why can we set non-blocking as default?

Why not make non-blocking default for lazy mode as well?

Uh oh!

pytorch-bot bot commented Oct 21, 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/138527

✅ You can merge normally! (1 Unrelated Failure)

Uh oh!

Uh oh!

wconstab Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

kwen2501 Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

wconstab Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

wconstab Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

wconstab left a comment

Choose a reason for hiding this comment

Uh oh!

shuqiangzhang commented Oct 22, 2024

Uh oh!

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

Uh oh!

Uh oh!

kwen2501 commented Oct 23, 2024

Uh oh!

pytorchmergebot commented Oct 23, 2024

Merge started

Uh oh!

jeanschmidt commented Oct 23, 2024

Uh oh!

pytorchmergebot commented Oct 23, 2024

Uh oh!

pytorchmergebot commented Oct 26, 2024

Merge started

Uh oh!

huydhn commented Oct 27, 2024

Uh oh!

pytorchmergebot commented Oct 27, 2024

Uh oh!

pytorchmergebot commented Oct 27, 2024

Uh oh!

kwen2501 commented Oct 27, 2024

Uh oh!

kwen2501 commented Oct 27, 2024

Uh oh!

pytorchmergebot commented Oct 27, 2024

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kwen2501 commented Oct 21, 2024 •

edited

Loading

pytorch-bot bot commented Oct 21, 2024 •

edited

Loading

kwen2501 commented Oct 23, 2024 •

edited

Loading