[c10d] separate the codes for GPU stream synchronization and CPU thread synchronization #137295

shuqiangzhang · 2024-10-03T19:06:10Z

Stack from ghstack (oldest at bottom):

-> [c10d] separate the codes for GPU stream synchronization and CPU thread synchronization #137295

code
Summary:
This PR should not change the existing behavior of work.wait(), just
separate the stream synchronization code from the CPU busy wait code.

Also, remove the need of a private synchronization function.

In a longer term, we would like to give user the flexibility of bypassing the watchdog thread and handle the collective error by themselves.

Test Plan:
python test/distributed/test_c10d_nccl.py NcclErrorHandlingTest

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

code Summary: This PR should not change the existing behavior of work.wait(), just separate the stream synchronization code from the CPU busy wait code. Also, remove the need of a private synchronization function. Test Plan: Existing UT passed Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

pytorch-bot · 2024-10-03T19:06:14Z

🔗 Helpful Links

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

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

❌ 3 Cancelled Jobs

As of commit fd74b29 with merge base 38114ec ():

CANCELLED JOBS - The following jobs were cancelled. Please retry:

pull / linux-focal-cpu-py3.10-gcc9-bazel-test / build-and-test (default, 1, 1, lf.linux.4xlarge) (gh)
##[error]The operation was canceled.
pull / linux-focal-cuda12.1-py3.10-gcc9-bazel-test / build-and-test (default, 1, 1, lf.linux.4xlarge.nvidia.gpu) (gh)
pull / linux-focal-cuda12.4-py3.10-gcc9-bazel-test / build-and-test (default, 1, 1, lf.linux.4xlarge.nvidia.gpu) (gh)
##[error]The operation was canceled.

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

code Summary: This PR should not change the existing behavior of work.wait(), just separate the stream synchronization code from the CPU busy wait code. Also, remove the need of a private synchronization function. Test Plan: Existing UT passed Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: f089b63 Pull Request resolved: #137295

github-actions

Please commit the suggested changes from pytorch's linter.

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

…CPU blocking" code Summary: This PR should not change the existing behavior of work.wait(), just separate the stream synchronization code from the CPU busy wait code. Also, remove the need of a private synchronization function. Test Plan: Existing UT passed Reviewers: Subscribers: Tasks: Tags: cc XilunWu H-Huang awgu kwen2501 wanchaol fegin fduwjj wz337 wconstab d4l3k c-p-i-o [ghstack-poisoned]

code Summary: This PR should not change the existing behavior of work.wait(), just separate the stream synchronization code from the CPU busy wait code. Also, remove the need of a private synchronization function. Test Plan: Existing UT passed Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 34f7609 Pull Request resolved: #137295

…nd CPU thread synchronization" code Summary: This PR should not change the existing behavior of work.wait(), just separate the stream synchronization code from the CPU busy wait code. Also, remove the need of a private synchronization function. In a longer term, we would like to give user the flexibility of bypassing the watchdog thread and handle the collective error by themselves. Test Plan: python test/distributed/test_c10d_nccl.py NcclErrorHandlingTest cc XilunWu H-Huang awgu kwen2501 wanchaol fegin fduwjj wz337 wconstab d4l3k c-p-i-o [ghstack-poisoned]

code Summary: This PR should not change the existing behavior of work.wait(), just separate the stream synchronization code from the CPU busy wait code. Also, remove the need of a private synchronization function. Test Plan: Existing UT passed Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: a80cb61 Pull Request resolved: #137295

…nd CPU thread synchronization" code Summary: This PR should not change the existing behavior of work.wait(), just separate the stream synchronization code from the CPU busy wait code. Also, remove the need of a private synchronization function. In a longer term, we would like to give user the flexibility of bypassing the watchdog thread and handle the collective error by themselves. Test Plan: python test/distributed/test_c10d_nccl.py NcclErrorHandlingTest cc XilunWu H-Huang awgu kwen2501 wanchaol fegin fduwjj wz337 wconstab d4l3k c-p-i-o [ghstack-poisoned]

code Summary: This PR should not change the existing behavior of work.wait(), just separate the stream synchronization code from the CPU busy wait code. Also, remove the need of a private synchronization function. Test Plan: Existing UT passed Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 669bb0f Pull Request resolved: #137295

kwen2501

Giving a tentative approval.
Can you:

address the placement of synchronizeStream (see comment)
check if there is any customer of work.synchronize() (see comment)
?

kwen2501 · 2024-10-07T22:01:24Z

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

+  // syncrhoize() will block the current stream on the NCCL stream
+  synchronize();


nit: this change seems to move synchronizeStream after blocking wait.
In 99% of cases, this should be a no-change to code behavior, because the main thread is being hijacked by blocking wait.
I just worry about the 1% chance where another thread my have a handle on the compute stream too -- in case that, it would be nice to call synchronizeStream as early as possible.

You are right, the comments lie about the 'no behavior change part'. The behavior change here is only when blockingwait mode is true or timeout is specified, where user is more concerned about blocking the CPU thread till the collective is done. Will monitor if any users is affected by this change, Later we might further remove GPU stream synchronize() for blockingwait mode.

I am actually not concerned about behavior change, as you mentioned, it only relates to blocking wait mode.
My comment was more on robustness, as in, would it be generally better to establish stream dependency asap.

I actually put synchronize() before the cpu wait code in an early version. But it breaks the current barrier behavior which requires the CPU loop happens before the stream sync (https://www.internalfb.com/code/fbsource/[4713fa33ea08c1d66083e1bbe6c1fa4af785b424]/fbcode/caffe2/torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp?lines=695). which I am not sure it is safe to change. I am planning to remove the barrier blocks in the synchronize() method in future PRs and then we can switch the order

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

test/distributed/test_c10d_nccl.py

kwen2501 · 2024-10-07T22:14:50Z

This PR should not change the existing behavior of work.wait()

While that is true, it also introduced a new behavior:
work.wait(timeout)

Can you document this behavior?

And in terms of UX, should user call it like this?

try:
  work.wait(timeout)
except:
  # some handling

fduwjj · 2024-10-07T22:23:40Z

torch/csrc/distributed/c10d/ProcessGroupNCCL.hpp


   private:
-    // Helper function for synchronize
-    void synchronizeInternal(std::chrono::milliseconds timeout);


Nice, this function name is once very confusing to folks.

…nd CPU thread synchronization" code Summary: This PR should not change the existing behavior of work.wait(), just separate the stream synchronization code from the CPU busy wait code. Also, remove the need of a private synchronization function. In a longer term, we would like to give user the flexibility of bypassing the watchdog thread and handle the collective error by themselves. Test Plan: python test/distributed/test_c10d_nccl.py NcclErrorHandlingTest cc XilunWu H-Huang awgu kwen2501 wanchaol fegin fduwjj wz337 wconstab d4l3k c-p-i-o [ghstack-poisoned]

code Summary: This PR should not change the existing behavior of work.wait(), just separate the stream synchronization code from the CPU busy wait code. Also, remove the need of a private synchronization function. Test Plan: Existing UT passed Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: c8a7049 Pull Request resolved: #137295

shuqiangzhang · 2024-10-08T06:11:26Z

@pytorchbot merge

pytorchmergebot · 2024-10-08T06:13:09Z

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

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

shuqiangzhang marked this pull request as draft October 3, 2024 19:06

github-actions bot requested changes Oct 3, 2024

View reviewed changes

shuqiangzhang requested review from fduwjj, kwen2501 and wconstab October 3, 2024 21:58

shuqiangzhang self-assigned this Oct 3, 2024

shuqiangzhang marked this pull request as ready for review October 3, 2024 22:01

shuqiangzhang changed the title ~~[c10d] separte the concepts of stream synchronization and CPU blocking~~ [c10d] separate the codes for GPU stream synchronization and CPU thread synchronization Oct 3, 2024

shuqiangzhang requested a review from c-p-i-o October 7, 2024 17:35

shuqiangzhang mentioned this pull request Oct 7, 2024

[c10d] remove watchdog thread in blocking wait mode #137441

Closed

kwen2501 approved these changes Oct 7, 2024

View reviewed changes

fduwjj reviewed Oct 7, 2024

View reviewed changes

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

pytorchmergebot added the merging label Oct 8, 2024

pytorchmergebot added the Merged label Oct 8, 2024

pytorchmergebot closed this in 76dca1f Oct 8, 2024

pytorchmergebot removed the merging label Oct 8, 2024

github-actions bot deleted the gh/shuqiangzhang/45/head branch November 8, 2024 02:05

		// syncrhoize() will block the current stream on the NCCL stream
		synchronize();

[c10d] separate the codes for GPU stream synchronization and CPU thread synchronization #137295

[c10d] separate the codes for GPU stream synchronization and CPU thread synchronization #137295

Uh oh!

Conversation

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

Uh oh!

Uh oh!

pytorch-bot bot commented Oct 3, 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/137295

❌ 3 Cancelled Jobs

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kwen2501 left a comment

Choose a reason for hiding this comment

Uh oh!

kwen2501 Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

shuqiangzhang Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shuqiangzhang Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kwen2501 commented Oct 7, 2024

Uh oh!

fduwjj Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

shuqiangzhang commented Oct 8, 2024

Uh oh!

pytorchmergebot commented Oct 8, 2024

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shuqiangzhang commented Oct 3, 2024 •

edited

Loading

pytorch-bot bot commented Oct 3, 2024 •

edited

Loading

kwen2501 Oct 7, 2024 •

edited

Loading