Add reduce_scatter_tensor in place of _reduce_scatter_base by kwen2501 · Pull Request #85867 · pytorch/pytorch

kwen2501 · 2022-09-28T23:08:33Z

This is a twin PR similar to the one for all_gather_into_tensor (#85686).
The philosophy for renaming _reduce_scatter_base instead of merging it is described in #85686.

Cc @rohan-varma @H-Huang @crcrpar @ptrblck @mrshenli

pytorch-bot · 2022-09-28T23:08:36Z

🔗 Helpful Links

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

📄 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 our office hours

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 Failures

As of commit 945f466:

The following jobs have failed:

linux-bionic-cuda11.6-py3.10-gcc7 / test (default, 1, 4, linux.4xlarge.nvidia.gpu)

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

crcrpar

looks good to me, appreciate your swift action

H-Huang

LGTM, thanks!

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

H-Huang · 2022-09-29T19:07:28Z

torch/testing/_internal/distributed/distributed_test.py

+            )
+            return tensor_out
+
+        @sandcastle_skip_if(BACKEND != "nccl", "Only Nccl supports CUDA reduce_scatter_tensor")


nit: I think this decorator @requires_nccl() does a similar check

Suggested change

@sandcastle_skip_if(BACKEND != "nccl", "Only Nccl supports CUDA reduce_scatter_tensor")

@requires_nccl()

Will fix later

H-Huang · 2022-09-29T19:12:25Z

torch/distributed/distributed_c10d.py

+        tensor([0, 2], device='cuda:0') # Rank 0
+        tensor([4, 6], device='cuda:1') # Rank 1
+
+    .. warning::


Another nit, it looks like warning messages are usually placed before examples: https://docs-preview.pytorch.org/85867/distributed.html?highlight=reduce_scatter_tensor#torch.distributed.reduce_scatter_tensor

Will fix later.

kwen2501 · 2022-09-29T19:20:21Z

@pytorchbot merge

pytorchmergebot · 2022-09-29T19:21:52Z

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the land checks (-l) flag. If you did not specify this flag yourself, you are likely enrolled in the land checks rollout. This means that your change will be merged once all checks on your PR have passed since you have added the ciflow/trunk label to your PR (ETA 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

pytorchmergebot · 2022-09-29T20:17:28Z

Merge failed

Reason: 2 additional jobs have failed, first few of them are: trunk ,trunk / linux-focal-rocm5.2-py3.7 / test (default, 1, 2, linux.rocm.gpu)

If you believe this is an error, you can use the old behavior with @pytorchbot merge -g (optionally with the ciflow/trunk to get land checks) or use @pytorchbot merge -f "some reason here". For more information, see the bot wiki.

Please reach out to the PyTorch DevX Team with feedback or questions!

Details for Dev Infra team

Raised by workflow job

kwen2501 · 2022-09-29T21:01:24Z

@pytorchbot rebase -m

pytorch-bot · 2022-09-29T21:01:26Z

❌ 🤖 pytorchbot command failed:

@pytorchbot: error: unrecognized arguments: -m

usage: @pytorchbot [-h] {merge,revert,rebase,label} ...

Try @pytorchbot --help for more info.

kwen2501 · 2022-09-29T21:05:08Z

@pytorchbot rebase -b master

pytorchmergebot · 2022-09-29T21:06:45Z

@pytorchbot successfully started a rebase job. Check the current status here

pytorchmergebot · 2022-09-29T21:06:49Z

Successfully rebased reduce_scatter_tensor onto refs/remotes/origin/master, please pull locally before adding more changes (for example, via git checkout reduce_scatter_tensor && git pull --rebase)

kwen2501 · 2022-09-30T05:46:42Z

Here is the failed check:

TestCommonCUDA.test_python_ref_executor__refs_atanh_executor_aten_cuda_complex32`

It does not seem related to my change.

And the failure reason is:

torch.cuda.OutOfMemoryError: CUDA out of memory. Tried to allocate 16.00 MiB (GPU 0; 7.44 GiB total capacity; 312.49 MiB already allocated; 10.19 MiB free; 1.64 GiB allowed; 860.00 MiB reserved in total by PyTorch) If reserved memory is >> allocated memory try setting max_split_size_mb to avoid fragmentation.  See documentation for Memory Management and PYTORCH_CUDA_ALLOC_CONF

Also does not seem to be related to my change.

@huydhn FYI.

@pytorchbot merge -f "The one failure does not seem related to my change"

pytorchmergebot · 2022-09-30T05:48:14Z

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the force (-f) flag. This means your change will be merged immediately, bypassing any CI checks (ETA: 1-5 minutes). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

github-actions · 2022-09-30T05:49:05Z

Hey @kwen2501.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

@rohan-varma

This is a twin PR similar to the one for `all_gather_into_tensor` (#85686). The philosophy for renaming `_reduce_scatter_base` instead of merging it is described in #85686. Cc @rohan-varma @H-Huang @crcrpar @ptrblck @mrshenli Pull Request resolved: #85867 Approved by: https://github.com/crcrpar, https://github.com/H-Huang

Summary: remove deprecated _all_gather_base and _reduce_scatter_base APIs and use the new replacements - `_all_gather_base` -> `all_gather_into_tensor` - _`reduce_scatter_base` -> `reduce_scatter_tensor` - correct begin size for reduce_scatter_base see pytorch/pytorch#85686 and pytorch/pytorch#85867 for the changes in PyTorch Reviewed By: wesbland Differential Revision: D40703655 fbshipit-source-id: c871c05a8de687a34a124b60857879c983b8cc3d

kwen2501 requested review from H-Huang, awgu, mingzhe09088, mrshenli, pritamdamania87, rohan-varma and zhaojuanmao as code owners September 28, 2022 23:08

pytorch-bot bot added the release notes: distributed (c10d) release notes category label Sep 28, 2022

facebook-github-bot added cla signed oncall: distributed Add this issue/PR to distributed oncall triage queue labels Sep 28, 2022

crcrpar approved these changes Sep 29, 2022

View reviewed changes

H-Huang approved these changes Sep 29, 2022

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Sep 29, 2022

H-Huang reviewed Sep 29, 2022

View reviewed changes

kwen2501 added 2 commits September 29, 2022 21:06

Add reduce_scatter_tensor in place of _reduce_scatter_base

9e981db

Also fix typo in expected error message in test

945f466

pytorchmergebot force-pushed the reduce_scatter_tensor branch from a04af10 to 945f466 Compare September 29, 2022 21:06

pytorchmergebot added the Merged label Sep 30, 2022

pytorchmergebot closed this in ade1c19 Sep 30, 2022

kwen2501 added the topic: new features topic category label Sep 30, 2022

github-actions bot deleted the reduce_scatter_tensor branch March 31, 2024 01:51

	@sandcastle_skip_if(BACKEND != "nccl", "Only Nccl supports CUDA reduce_scatter_tensor")
	@requires_nccl()

Conversation

kwen2501 commented Sep 28, 2022

Uh oh!

pytorch-bot bot commented Sep 28, 2022 • 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/85867

❌ 1 Failures

Uh oh!

crcrpar left a comment

Choose a reason for hiding this comment

Uh oh!

H-Huang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

H-Huang Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

kwen2501 Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

H-Huang Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

kwen2501 Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

kwen2501 commented Sep 29, 2022

Uh oh!

pytorchmergebot commented Sep 29, 2022

Uh oh!

pytorchmergebot commented Sep 29, 2022

Merge failed

Uh oh!

kwen2501 commented Sep 29, 2022

Uh oh!

pytorch-bot bot commented Sep 29, 2022

Uh oh!

kwen2501 commented Sep 29, 2022

Uh oh!

pytorchmergebot commented Sep 29, 2022

Uh oh!

pytorchmergebot commented Sep 29, 2022

Uh oh!

kwen2501 commented Sep 30, 2022

Uh oh!

pytorchmergebot commented Sep 30, 2022

Uh oh!

github-actions bot commented Sep 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pytorch-bot bot commented Sep 28, 2022 •

edited

Loading