Add sequence number support for UCC #85047
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/85047
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 27a7a07: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
The lint issues are real, could you fix them, please? |
| @@ -1569,6 +1571,12 @@ c10::intrusive_ptr<Work> ProcessGroupUCC::recv( | |||
| "ucc:recv"); | |||
| } | |||
|
|
|||
| void ProcessGroupUCC::setSequenceNumberForGroup() {} | |||
There was a problem hiding this comment.
curious why this set function is a no-op?
There was a problem hiding this comment.
The ProcessGroupUcc setSequenceNumberForGroup() and getSequenceNumberForGroup() implementation here is based on the existing ProcessGroupNccl implementation: https://github.com/pytorch/pytorch/blob/master/torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp#L759-L763
Basically the _seq counter is initialized at 0, and it is only incremented during the collective_post() call, which is the equivalent to the ProcessGroupNccl handling during its equivalent collective() call: https://github.com/pytorch/pytorch/blob/master/torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp#L1532
|
/easycla As part of the transition to the PyTorch Foundation, this project now requires contributions be covered under the new CLA. See #85559 for additional details. This comment will trigger a new check of this PR. If you are already covered, you will simply see a new "EasyCLA" check that passes. If you are not covered, a bot will leave a new comment with a link to sign. |
|
Add barrier blocking fix from here: #86961 |
| @@ -7694,12 +7696,14 @@ def _test_verify_model_across_rank(self, use_logger): | |||
|
|
|||
| @require_backend(DistTestCases.backend_feature["gpu"]) | |||
| @require_backends_available(DistTestCases.backend_feature["gpu"]) | |||
| @sandcastle_skip_if(BACKEND == "ucc", "test failing locally with ucc") | |||
There was a problem hiding this comment.
Wondering what error this local failure would manifest to end users?
Asking to see if it is safe to skip.
There was a problem hiding this comment.
Just updated the skip message. For both with and without logger versions, it hangs and then times out after 5 minutes.
| @@ -170,6 +170,9 @@ void read_config() { | |||
| torch_ucc_config.blocking_wait[(std::uint8_t)op] = true; | |||
| } | |||
|
|
|||
| // barrier is always blocking | |||
| torch_ucc_config.blocking_wait[(std::uint8_t)OpType::BARRIER] = true; | |||
|
|
|||
There was a problem hiding this comment.
Just FYI - this change is also in #86961
|
@pytorchbot rebase |
|
@pytorchbot successfully started a rebase job. Check the current status here |
|
Rebase failed due to Command Raised by https://github.com/pytorch/pytorch/actions/runs/3348775193 |
|
@pytorchbot merge |
1 similar comment
|
@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: The following mandatory check(s) failed (Rule Dig deeper by viewing the failures on hud Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot rebase |
|
@pytorchbot successfully started a rebase job. Check the current status here |
|
Rebase failed due to Command Raised by https://github.com/pytorch/pytorch/actions/runs/3352547882 |
|
@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 additional jobs have failed, first few of them are: trunk Details for Dev Infra teamRaised by workflow job |
|
@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 additional jobs have failed, first few of them are: trunk Details for Dev Infra teamRaised by workflow job |
|
@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 |
|
Hey @Fuzzkatt. |
Add sequence number support for UCC, mostly following format of ProcressGroupNCCL. Pass new test: `test_all_gather_object_subgroup` Add skips for gather tests: `test_gather_object` and `test_gather_object_subgroup` cc @mrshenli @pritamdamania87 @zhaojuanmao @satgera @rohan-varma @gqchen @aazzolini @osalpekar @jiayisuse @H-Huang @kwen2501 @awgu Pull Request resolved: pytorch#85047 Approved by: https://github.com/kwen2501
Add sequence number support for UCC, mostly following format of ProcressGroupNCCL. Pass new test: `test_all_gather_object_subgroup` Add skips for gather tests: `test_gather_object` and `test_gather_object_subgroup` cc @mrshenli @pritamdamania87 @zhaojuanmao @satgera @rohan-varma @gqchen @aazzolini @osalpekar @jiayisuse @H-Huang @kwen2501 @awgu Pull Request resolved: pytorch#85047 Approved by: https://github.com/kwen2501
Creates the equivalent c10d test for ucc for https://github.com/pytorch/pytorch/blob/master/test/distributed/test_c10d_gloo.py and https://github.com/pytorch/pytorch/blob/master/test/distributed/test_c10d_nccl.py. Uses test_c10d_gloo.py as the reference and adds all the common ops. More detailed comparison of available ops here: https://docs.google.com/document/d/1yPsa_X9EiEiqo-j2Yn7ierhccBtEjwoqC-B7-amI0MI/edit?usp=sharing Also removes extra line for ProcessGroupUCC.cpp barrier blocking wait that got duplicated from merging #85047. Pull Request resolved: #88110 Approved by: https://github.com/zasdfgbnm, https://github.com/kit1980, https://github.com/kwen2501, https://github.com/malfet
Add sequence number support for UCC, mostly following format of ProcressGroupNCCL.
Pass new test:
test_all_gather_object_subgroupAdd skips for gather tests:
test_gather_objectandtest_gather_object_subgroupcc @mrshenli @pritamdamania87 @zhaojuanmao @satgera @rohan-varma @gqchen @aazzolini @osalpekar @jiayisuse @H-Huang @kwen2501 @awgu