[BE] split seq_id to collective_seq_id and p2p_seq_id #125727

c-p-i-o · 2024-05-08T00:35:03Z

Stack from ghstack (oldest at bottom):

Summary:
Split out seq_id into collective_seq_id and p2p_seq_id. The main idea here is that collectives that go to all machines should have identical collective_seq_id and therefore it makes it easier to spot if one of machines isn't handling a collective operation.
Next, we can attempt to match up p2p operations to ensure that the sender(s)/receivers(s) are in sync.

Resolves issue: #125173

Test Plan:
Unit tests.

Reviewers:

Subscribers:

Tasks:

Tags:

cc @mrshenli @pritamdamania87 @zhaojuanmao @satgera @gqchen @aazzolini @osalpekar @jiayisuse @H-Huang @kwen2501 @awgu @penguinwu @fegin @XilunWu @wanchaol @fduwjj @wz337 @tianyu-l @wconstab @yf225 @chauhang @d4l3k

Summary: Attempt to separate out collectives and P2P for debug purposes. Pass isP2P to record. Test Plan: Unit tests. Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

pytorch-bot · 2024-05-08T00:35:06Z

🔗 Helpful Links

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

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

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

Summary: Attempt to separate out collectives and P2P for debug purposes. Pass isP2P to record. Test Plan: Unit tests. Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: ee518d978d2f03891826e4d3bb1dd13bd51e7005 Pull Request resolved: #125727

Summary: Attempt to separate out collectives and P2P for debug purposes. Pass isP2P to record. Test Plan: Unit tests. Reviewers: Subscribers: Tasks: Tags: cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k [ghstack-poisoned]

Summary: Attempt to separate out collectives and P2P for debug purposes. Pass isP2P to record. Test Plan: Unit tests. Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 3808e1c860578bff32f9cb10db96ba4ea3b8dd9f Pull Request resolved: #125727

Summary: Attempt to separate out collectives and P2P for debug purposes. Pass isP2P to record. Test Plan: Unit tests. Reviewers: Subscribers: Tasks: Tags: cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k [ghstack-poisoned]

Summary: Attempt to separate out collectives and P2P for debug purposes. Pass seqCollective_ and seqP2P_ to the recorder. Pass isP2P to record (not sure if we need this yet). Test Plan: Unit tests. Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: e2ce7773ad108dbd8a633f051e7ac74b405e31b6 Pull Request resolved: #125727

wconstab · 2024-05-08T20:26:57Z

this mostly lgtm, i didn't review carefully

one thought is whether we want to (correctly) bump version major, or, just keep old seq_id and add new ones and later deprecate it.

I think its worth checking if anyone's gonna be broken by this, but since not many users have developed scripts yet its probably fine to just bump the major and drop the old key so we have less baggage. anyone else have a thought on that?

Summary: Attempt to separate out collectives and P2P for debug purposes. Pass isP2P to record. Test Plan: Unit tests. Reviewers: Subscribers: Tasks: Tags: cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k [ghstack-poisoned]

Summary: Attempt to separate out collectives and P2P for debug purposes. Pass seqCollective_ and seqP2P_ to the recorder. Pass isP2P to record (not sure if we need this yet). Test Plan: Unit tests. Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 42b2316af83a7cd16688fb792b56dd2e060c9dcb Pull Request resolved: #125727

[ghstack-poisoned]

Summary: Attempt to separate out collectives and P2P for debug purposes. Pass seqCollective_ and seqP2P_ to the recorder. Pass isP2P to record (not sure if we need this yet). Test Plan: Unit tests. Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: ca25b1f43605c90f232900307413aa94c3df53ba Pull Request resolved: #125727

[ghstack-poisoned]

Summary: Attempt to separate out collectives and P2P for debug purposes. Pass seqCollective_ and seqP2P_ to the recorder. Pass isP2P to record - this flag will help distinguish P2P v/s collective records. Test Plan: Unit tests. Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 8d710b148cc470ef0188b2cea3d7eadc79306ed0 Pull Request resolved: #125727

[ghstack-poisoned]

Summary: Attempt to separate out collectives and P2P for debug purposes. Pass seqCollective_ and seqP2P_ to the recorder. Pass isP2P to record - this flag will help distinguish P2P v/s collective records. Test Plan: Unit tests. Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 14b6c61dda0ad1987e3bfbbc7b8434d3240f92ba Pull Request resolved: #125727

zdevito

Looks good, thanks!

torch/csrc/distributed/c10d/TraceUtils.h

[ghstack-poisoned]

Summary: Split out seq_id into collective_seq_id and p2p_seq_id. The main idea here is that collectives that go to all machines should have identical collective_seq_id and therefore it makes it easier to spot if one of machines isn't handling a collective operation. Next, we can attempt to match up p2p operations to ensure that the sender(s)/receivers(s) are in sync. Resolves issue: #125173 ghstack-source-id: c31b3164d2e51efeab210e6a949cd4c8d1ecd3d7 Pull Request resolved: #125727

c-p-i-o · 2024-05-13T23:52:15Z

one thought is whether we want to (correctly) bump version major, or, just keep old seq_id and add new ones and later deprecate it.

@wconstab - do you have a preference? Shall we keep the old seq_id?
The only one thing I'd have to fix up is the unit tests and keep the old field. But just wanted to make sure if that work is worth it.
How do I find out I won't break anyone?

[ghstack-poisoned]

Summary: Split out seq_id into collective_seq_id and p2p_seq_id. The main idea here is that collectives that go to all machines should have identical collective_seq_id and therefore it makes it easier to spot if one of machines isn't handling a collective operation. Next, we can attempt to match up p2p operations to ensure that the sender(s)/receivers(s) are in sync. Resolves issue: #125173 ghstack-source-id: c67b8ed6bda1415b5f6a2e2006e5bec0ae8b1621 Pull Request resolved: #125727

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

[ghstack-poisoned]

Summary: Split out seq_id into collective_seq_id and p2p_seq_id. The main idea here is that collectives that go to all machines should have identical collective_seq_id and therefore it makes it easier to spot if one of machines isn't handling a collective operation. Next, we can attempt to match up p2p operations to ensure that the sender(s)/receivers(s) are in sync. Resolves issue: #125173 ghstack-source-id: f392686c6e68260fd453c28f2575fcf8bc71ea7f Pull Request resolved: #125727

[ghstack-poisoned]

Summary: Split out seq_id into collective_seq_id and p2p_seq_id. The main idea here is that collectives that go to all machines should have identical collective_seq_id and therefore it makes it easier to spot if one of machines isn't handling a collective operation. Next, we can attempt to match up p2p operations to ensure that the sender(s)/receivers(s) are in sync. Resolves issue: pytorch#125173 ghstack-source-id: cf9bb109c028d7ffe9612d2b9c4fda1df47586d7 Pull Request resolved: pytorch#125727 Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

[ghstack-poisoned]

c-p-i-o · 2024-05-20T23:05:47Z

@pytorchbot merge

pytorchmergebot · 2024-05-20T23:07:46Z

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

[ghstack-poisoned]

pytorchmergebot · 2024-05-21T00:15:55Z

Merge failed

Reason: New commits were pushed while merging. Please rerun the merge command.

Details for Dev Infra team

Raised by workflow job

c-p-i-o · 2024-05-21T00:16:39Z

@pytorchbot merge

pytorchmergebot · 2024-05-21T00:18:23Z

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

[WIP] rename seq_id to collective_seq_id

65c3d7b

Summary: Attempt to separate out collectives and P2P for debug purposes. Pass isP2P to record. Test Plan: Unit tests. Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

c-p-i-o mentioned this pull request May 8, 2024

[BE] Abstract out strings to top of file #125640

Closed

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

c-p-i-o marked this pull request as draft May 8, 2024 00:35

Update

1f81cbb

[ghstack-poisoned]

Update

61cfec0

[ghstack-poisoned]

Update

2dc3a71

[ghstack-poisoned]

c-p-i-o changed the title ~~[WIP] rename seq_id to collective_seq_id~~ [BE] split seq_id to collective_seq_id and p2p_seq_id May 13, 2024

c-p-i-o self-assigned this May 13, 2024

c-p-i-o marked this pull request as ready for review May 13, 2024 17:51

c-p-i-o requested review from wconstab, zdevito and shuqiangzhang May 13, 2024 17:52

zdevito approved these changes May 13, 2024

View reviewed changes

torch/csrc/distributed/c10d/TraceUtils.h Outdated Show resolved Hide resolved

Update

7a34b7e

[ghstack-poisoned]

Update

3eed042

[ghstack-poisoned]

shuqiangzhang reviewed May 16, 2024

View reviewed changes

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp Show resolved Hide resolved

shuqiangzhang reviewed May 16, 2024

View reviewed changes

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp Outdated Show resolved Hide resolved

Update

ae33ca4

[ghstack-poisoned]

Update

dfe420c

[ghstack-poisoned]

c-p-i-o mentioned this pull request May 17, 2024

Capture dtype in Flight Recorder #126576

Closed

c-p-i-o added 2 commits May 17, 2024 15:43

Update

4058f91

[ghstack-poisoned]

Update

183e85b

[ghstack-poisoned]

c-p-i-o mentioned this pull request May 17, 2024

Capture dtype in Flight Recorder #126581

Closed

c-p-i-o added 2 commits May 18, 2024 16:48

Update

bc0cd96

[ghstack-poisoned]

Update

6e7f979

[ghstack-poisoned]

c-p-i-o mentioned this pull request May 20, 2024

Initial commit of flight recorder analyzer #126726

Draft

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

pytorchmergebot added the merging label May 20, 2024

Update

8a9f70a

[ghstack-poisoned]

c-p-i-o mentioned this pull request May 21, 2024

Minor fixes #126733

Draft

pytorchmergebot removed the merging label May 21, 2024

pytorchmergebot added the merging label May 21, 2024

pytorchmergebot added the Merged label May 21, 2024

pytorchmergebot closed this in a83e745 May 21, 2024

pytorchmergebot removed the merging label May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] split seq_id to collective_seq_id and p2p_seq_id #125727

[BE] split seq_id to collective_seq_id and p2p_seq_id #125727

c-p-i-o commented May 8, 2024 •

edited

pytorch-bot bot commented May 8, 2024 •

edited

wconstab commented May 8, 2024

zdevito left a comment

c-p-i-o commented May 13, 2024 •

edited

c-p-i-o commented May 20, 2024

pytorchmergebot commented May 20, 2024

pytorchmergebot commented May 21, 2024

c-p-i-o commented May 21, 2024

pytorchmergebot commented May 21, 2024

[BE] split seq_id to collective_seq_id and p2p_seq_id #125727

[BE] split seq_id to collective_seq_id and p2p_seq_id #125727

Conversation

c-p-i-o commented May 8, 2024 • edited

pytorch-bot bot commented May 8, 2024 • edited

🔗 Helpful Links

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

✅ No Failures

wconstab commented May 8, 2024

zdevito left a comment

Choose a reason for hiding this comment

c-p-i-o commented May 13, 2024 • edited

c-p-i-o commented May 20, 2024

pytorchmergebot commented May 20, 2024

Merge started

pytorchmergebot commented May 21, 2024

Merge failed

c-p-i-o commented May 21, 2024

pytorchmergebot commented May 21, 2024

Merge started

c-p-i-o commented May 8, 2024 •

edited

pytorch-bot bot commented May 8, 2024 •

edited

c-p-i-o commented May 13, 2024 •

edited