KEMBAR78
Enforce contiguity for alltoall by xw285cornell · Pull Request #141816 · pytorch/pytorch · GitHub
Skip to content

Enforce contiguity for alltoall #141816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Enforce contiguity for alltoall #141816

wants to merge 1 commit into from

Conversation

@xw285cornell
Copy link
Contributor

@xw285cornell xw285cornell commented Nov 30, 2024 •
edited by pytorch-bot bot
Loading

Summary: We cannot relax the alltoall contiguous requirement which will lead to wrong results.

Test Plan: Added a test.

Differential Revision: D66560930

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

@pytorch-bot
Copy link

pytorch-bot bot commented Nov 30, 2024 •
edited
Loading

🔗 Helpful Links

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

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

❌ 1 New Failure, 10 Unrelated Failures

As of commit 4b41e54 with merge base f911361 (image):

NEW FAILURE - The following job has failed:

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

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

@pytorch-bot pytorch-bot bot added oncall: distributed Add this issue/PR to distributed oncall triage queue release notes: distributed (c10d) release notes category labels Nov 30, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66560930

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 30, 2024
fduwjj
fduwjj reviewed Nov 30, 2024
View reviewed changes
Copy link
Contributor

@fduwjj fduwjj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @kwen2501 to take a look since you made the change for P2P ops.

xw285cornell added a commit to xw285cornell/pytorch that referenced this pull request Dec 2, 2024
@xw285cornell @facebook-github-bot
Enforce contiguity for alltoall (pytorch#141816)
83b147d 
Summary:

We cannot relax the alltoall contiguous requirement which will lead to wrong results.

Test Plan: Added a test.

Differential Revision: D66560930
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66560930

@xw285cornell @facebook-github-bot
Enforce contiguity for alltoall (pytorch#141816)
4b41e54 
Summary:

We cannot relax the alltoall contiguous requirement which will lead to wrong results.

Test Plan: Added a test.

Reviewed By: fduwjj

Differential Revision: D66560930
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66560930

@xw285cornell
Copy link
Contributor Author

I looked (very hard) at the errors and none of them seem relevant. So I'm landing it - if it's breaking anything please feel free to revert.

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

10 similar comments
@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

8 similar comments
@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -i

(Initiating merge automatically since Phabricator Diff has merged, merging with -i because oss signals were bypassed internally)

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged while ignoring the following 11 checks: pull / cuda12.4-py3.10-gcc9-sm75 / build, pull / linux-focal-cuda11.8-py3.10-gcc9 / build, pull / linux-focal-py3.11-clang10 / build, pull / linux-focal-cuda12.4-py3.10-gcc9-sm86 / build, pull / linux-jammy-cuda11.8-cudnn9-py3.9-clang12 / build, pull / linux-focal-rocm6.2-py3.10 / build, pull / linux-focal-py3.9-clang10 / build, trunk / linux-focal-cuda12.1-py3.10-gcc9-no-ops / build, trunk / linux-focal-cuda12.4-py3.10-gcc9-no-ops / build, trunk / linux-focal-rocm6.2-py3.10 / build, trunk / linux-focal-cuda12.4-py3.10-gcc9-sm86 / build

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

pobin6 pushed a commit to pobin6/pytorch that referenced this pull request Dec 5, 2024
@xw285cornell
Enforce contiguity for alltoall (pytorch#141816)
00a26f4 
Summary: We cannot relax the alltoall contiguous requirement which will lead to wrong results.

Test Plan: Added a test.

Differential Revision: D66560930

Pull Request resolved: pytorch#141816
Approved by: https://github.com/Skylion007, https://github.com/kwen2501, https://github.com/fduwjj, https://github.com/fegin, https://github.com/yoyoyocmu
AmdSampsa pushed a commit to AmdSampsa/pytorch that referenced this pull request Dec 9, 2024
@xw285cornell @AmdSampsa
Enforce contiguity for alltoall (pytorch#141816)
51784f1 
Summary: We cannot relax the alltoall contiguous requirement which will lead to wrong results.

Test Plan: Added a test.

Differential Revision: D66560930

Pull Request resolved: pytorch#141816
Approved by: https://github.com/Skylion007, https://github.com/kwen2501, https://github.com/fduwjj, https://github.com/fegin, https://github.com/yoyoyocmu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Skylion007 Skylion007 Skylion007 approved these changes

@fegin fegin fegin approved these changes

@kwen2501 kwen2501 kwen2501 approved these changes

@fduwjj fduwjj fduwjj approved these changes

@yoyoyocmu yoyoyocmu yoyoyocmu approved these changes

Assignees

No one assigned

Labels

ciflow/trunk Trigger trunk jobs on your pull request fb-exported Merged oncall: distributed Add this issue/PR to distributed oncall triage queue release notes: distributed (c10d) release notes category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@xw285cornell @facebook-github-bot @pytorchmergebot @Skylion007 @fegin @kwen2501 @fduwjj @yoyoyocmu