[c10d] PGNCCL refactor part 1: adds assert size==1 #119099

kwen2501 · Feb 2, 2024

Breaking #118674 into multiple smaller PRs.
This is the first one.
It adds assert size==1 to PGNCCL, and refactors some old tests written in multi-device style (which would otherwise fail at the assert).

cc @mrshenli @pritamdamania87 @zhaojuanmao @satgera @rohan-varma @gqchen @aazzolini @osalpekar @jiayisuse @H-Huang @awgu @penguinwu @fegin @XilunWu @wanchaol @fduwjj @wz337 @tianyu-l @wconstab @yf225 @ezyang @gchanan

pytorch-bot · Feb 2, 2024

🔗 Helpful Links

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

📄 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 fd49e29 with merge base 9afd539 ():
💚 Looks good so far! There are no failures yet. 💚

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

wconstab

Sgtm
When should we update docs?

wconstab · Feb 3, 2024

test/cpp/c10d/ProcessGroupNCCLErrorsTest.cpp

@@ -286,7 +286,6 @@ TEST_F(ProcessGroupNCCLErrorsTest, testNCCLErrorsBlocking) {

  auto work = pg.allreduce(tensors_);
  work->wait();
-  EXPECT_TRUE(work->isSuccess());


This is bc single device style is blocking and multi was non blocking ? Or what

No, it is just because the is_success API has been deprecated for a long time at the pybind place:
https://github.com/pytorch/pytorch/blob/main/torch/csrc/distributed/c10d/init.cpp#L2556-L2560
so taking this chance to take out the c++ code as well.
Also because its c++ code would complicate my refactorization.

It makes sense (to deprecate it) -- how can we tell a collective is successful? Are we telling user that the result is correct? We obviously don't/cannot do data check.. So at best we can only tell is_complete().

wconstab · Feb 3, 2024

test/distributed/test_c10d_nccl.py

@@ -2926,7 +2907,7 @@ def tearDown(self):

    @property
    def op_timeout_sec(self):
-        return 1
+        return 3


Just to increase the timeout limit by a little to avoid a false timeout, so that the chance of being flaky is smaller. Didn't mean that my change requires increase of this timeout. Just a minor tweak as I see it.

is this gonna sleep for 3x longer or is it a nice timeout that only waits that long if needed? i'm a little mindful of our reputation for too-long CI jobs..

of course, avoiding flaky tests is the most important imo. But if all that stands between your test and a flake is a 1-2 sec knob tuning, it might be a bad test design? haven't looked at where this is used.

"a nice timeout that only waits that long if needed"

kwen2501 · Feb 3, 2024

Sgtm When should we update docs?

The doc has been updated for a while now:
https://pytorch.org/docs/stable/distributed.html#multi-gpu-collective-functions
(I can add more words to it in a following PR advising alternatives, e.g. multi-proc or multi-thread)

facebook-github-bot · Feb 5, 2024

@kwen2501 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

wconstab · Feb 5, 2024

test/cpp/c10d/ProcessGroupNCCLTest.cpp

+  // maybe refactor the guts rather than copy-pasta, but it may not be
+  // worth it.
+  for (auto test : {&test1, &test2}) {
+    const int numDevices = test->numDevices();


i dont really understand what this test is doing. is this a new test btw?

do we expect numDevices > 1 here?

This is an old test, refactored from

TEST_F(ProcessGroupNCCLTest, testSplittingCommunicator)

test->numDevices() should return 1 now (I hard set numDevices_ to 1 in test class).

wconstab

nice cleanup.

facebook-github-bot · Feb 6, 2024

@kwen2501 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

kwen2501 · Feb 6, 2024

@pytorchbot merge

pytorchmergebot · Feb 6, 2024

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

Breaking pytorch#118674 into multiple smaller PRs. This is the first one. It adds `assert size==1` to PGNCCL, and refactors some old tests written in multi-device style (which would otherwise fail at the assert). Pull Request resolved: pytorch#119099 Approved by: https://github.com/wconstab

atalman · Feb 6, 2024

@pytorchmergebot revert -c ghfirst -m "Breaks internal tests"

pytorchmergebot · Feb 6, 2024

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

This reverts commit 4ab852b. Reverted #119099 on behalf of https://github.com/atalman due to Breaks internal tests ([comment](#119099 (comment)))

pytorchmergebot · Feb 6, 2024

@kwen2501 your PR has been successfully reverted.

facebook-github-bot · Feb 7, 2024

@kwen2501 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

XilunWu

thx for splitting the original PR into small PRs.

kwen2501 · Feb 7, 2024

@pytorchbot merge

pytorchmergebot · Feb 7, 2024

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

Breaking #118674 into multiple smaller PRs. This is the first one. It adds `assert size==1` to PGNCCL, and refactors some old tests written in multi-device style (which would otherwise fail at the assert). Pull Request resolved: #119099 Approved by: https://github.com/wconstab

This reverts commit 4ab852b. Reverted #119099 on behalf of https://github.com/atalman due to Breaks internal tests ([comment](#119099 (comment)))

Breaking #118674 into multiple smaller PRs. This is the first one. It adds `assert size==1` to PGNCCL, and refactors some old tests written in multi-device style (which would otherwise fail at the assert). Pull Request resolved: #119099 Approved by: https://github.com/wconstab, https://github.com/XilunWu

…device style (#119421) Part 2 and last part of #118674: Introduce actual "single-device" code change to ProcessGroupNCCL. assert size == 1 and test refactor have been done in #119099. Pull Request resolved: #119421 Approved by: https://github.com/shuqiangzhang

Breaking #118674 into multiple smaller PRs. This is the first one. It adds `assert size==1` to PGNCCL, and refactors some old tests written in multi-device style (which would otherwise fail at the assert). Pull Request resolved: #119099 Approved by: https://github.com/wconstab

This reverts commit 4ab852b. Reverted #119099 on behalf of https://github.com/atalman due to Breaks internal tests ([comment](#119099 (comment)))

Breaking #118674 into multiple smaller PRs. This is the first one. It adds `assert size==1` to PGNCCL, and refactors some old tests written in multi-device style (which would otherwise fail at the assert). Pull Request resolved: #119099 Approved by: https://github.com/wconstab, https://github.com/XilunWu

…device style (#119421) Part 2 and last part of #118674: Introduce actual "single-device" code change to ProcessGroupNCCL. assert size == 1 and test refactor have been done in #119099. Pull Request resolved: #119421 Approved by: https://github.com/shuqiangzhang

kwen2501 added 2 commits February 2, 2024 15:04

Refactor tests written in multi-device style

e65f5b7

Add assert size==1 to PGNCCL

cfdb922

kwen2501 added the module: bc-breaking Related to a BC-breaking change label Feb 2, 2024

kwen2501 requested a review from albanD February 2, 2024 23:38

pytorch-bot bot added release notes: distributed (c10d) release notes category topic: bc_breaking labels Feb 2, 2024

github-actions bot added the oncall: distributed Add this issue/PR to distributed oncall triage queue label Feb 2, 2024

kwen2501 mentioned this pull request Feb 3, 2024

[c10d] Simplify ProcessGroupNCCL into single-device style #118674

Closed

wconstab reviewed Feb 3, 2024

View reviewed changes

wconstab reviewed Feb 5, 2024

View reviewed changes

wconstab approved these changes Feb 5, 2024

View reviewed changes

Make internal test happy

5a40684

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

pytorchmergebot added the merging label Feb 6, 2024

pytorchmergebot added Merged and removed merging labels Feb 6, 2024

pytorchmergebot added the Reverted label Feb 6, 2024

kwen2501 added 2 commits February 7, 2024 11:00

Skip test at SetUp if no CUDA

7ccb5b8

Add ;

fd49e29

XilunWu approved these changes Feb 7, 2024

View reviewed changes

pytorchmergebot added the merging label Feb 7, 2024

pytorchmergebot closed this in 029a16c Feb 7, 2024

pytorchmergebot removed the merging label Feb 7, 2024

This was referenced Feb 7, 2024

[c10d] PGNCCL refactor part 2: Simplify ProcessGroupNCCL into single-device style #119421

Closed

avoid redundant event records and event blocks #119359

Closed

kwen2501 mentioned this pull request Feb 9, 2024

[c10d] Refactor ProcessGroupNCCL to use single device #117756

Closed

github-actions bot deleted the single_dev_test branch March 9, 2024 01:51

Search code, repositories, users, issues, pull requests...

[c10d] PGNCCL refactor part 1: adds assert size==1 #119099

[c10d] PGNCCL refactor part 1: adds assert size==1 #119099

Uh oh!

Conversation

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

Uh oh!

Uh oh!

pytorch-bot bot commented Feb 2, 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/119099

✅ No Failures

Uh oh!

wconstab left a comment

Choose a reason for hiding this comment

Uh oh!

wconstab Feb 3, 2024

Choose a reason for hiding this comment

Uh oh!

kwen2501 Feb 3, 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!

wconstab Feb 3, 2024

Choose a reason for hiding this comment

Uh oh!

kwen2501 Feb 3, 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!

wconstab Feb 5, 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!

kwen2501 Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

facebook-github-bot commented Feb 5, 2024

Uh oh!

wconstab Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

kwen2501 Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

wconstab left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 6, 2024

Uh oh!

kwen2501 commented Feb 6, 2024

Uh oh!

pytorchmergebot commented Feb 6, 2024

Merge started

Uh oh!

atalman commented Feb 6, 2024

Uh oh!

pytorchmergebot commented Feb 6, 2024

Uh oh!

pytorchmergebot commented Feb 6, 2024

Uh oh!

facebook-github-bot commented Feb 7, 2024

Uh oh!

XilunWu left a comment

Choose a reason for hiding this comment

Uh oh!

kwen2501 commented Feb 7, 2024

Uh oh!

pytorchmergebot commented Feb 7, 2024

Merge started

Uh oh!

Uh oh!

kwen2501 commented Feb 2, 2024 •

edited

Loading

pytorch-bot bot commented Feb 2, 2024 •

edited

Loading

kwen2501 Feb 3, 2024 •

edited

Loading

kwen2501 Feb 3, 2024 •

edited

Loading

wconstab Feb 5, 2024 •

edited

Loading

kwen2501 commented Feb 3, 2024 •

edited

Loading