fix sequence number for group #134578

GSSBMW · 2024-08-27T13:10:01Z

Summary:
Fix sequence number in execution trace dump for matching between collective/p2p op and wait in execution trace replay.

ProcessGroupNCCL has 2 sequence number counter, seqCollective_ and seqP2P_.

pytorch/torch/csrc/distributed/c10d/ProcessGroupNCCL.hpp

Lines 1188 to 1191 in b18ba94

    
           uint64_t seqCollective_{0}; 
        
           // Counting for the sequential number of NCCL P2P calls. 
        
           uint64_t seqP2P_{0};

However, WorkNCCL only has one sequence number member seq_.

pytorch/torch/csrc/distributed/c10d/ProcessGroupNCCL.hpp

Line 387 in b18ba94

uint64_t seq_;

We need to match collective and p2p with wait separately.
facebookresearch/param@29b5a46

Depend on: #135132

Test Plan: buck2 run mode/dev-nosan kineto/libkineto/fb/integration_tests:pytorch_execution_trace_integration_test

Differential Revision:

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

pytorch-bot · 2024-08-27T13:10:04Z

🔗 Helpful Links

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

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

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

linux-foundation-easycla · 2024-08-27T13:10:07Z

The committers listed above are authorized under a signed CLA.

✅ login: GSSBMW / name: Sanshan Gao (1ad6e7a)

GSSBMW · 2024-08-27T13:11:31Z

@shengfukevin @briancoutinho

GSSBMW · 2024-09-16T07:07:01Z

@shengfukevin Has updated and depend on #134619. Please help review!

kwen2501

The PR adds isP2P_ to WorkNCCL. Does WorkNCCL use it somewhere?

kwen2501 · 2024-09-17T16:08:46Z

Do you mind adding in the PR description what issue this PR fixes and how it fixes the issue?
Thanks a lot!

GSSBMW · 2024-09-18T01:17:01Z

The PR adds isP2P_ to WorkNCCL. Does WorkNCCL use it somewhere?
Do you mind adding in the PR description what issue this PR fixes and how it fixes the issue?

Hi, @kwen2501 I'm working with @shengfukevin on Execution Trace replay.
Add isP2P_ to WorkNCCL is for matching p2p op and work.wait. facebookresearch/param#180
Has updated the description.

kwen2501 · 2024-09-18T05:06:57Z

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

Hmm I see. You put isP2P_ into work because of this RECORD_PARAM_COMMS call in work.wait().
Do you think we can infer isP2P from work.opType? (i.e. when it is SEND or RECV?). Or do you think there isn't a mapping at the moment?

Would this example work for you?

pytorch/torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

Line 2472 in a0207c8

bool isP2P = isP2POp(opType);

We can't because we can't deduce whether it's collective or p2p from OpType::COALESCED .

pytorch/torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

Line 2663 in a0207c8

return endCoalescing(OpType::COALESCED);

pytorch/torch/distributed/distributed_c10d.py

Line 2283 in a0207c8

group._start_coalescing(device)

kwen2501 · 2024-09-18T05:15:46Z

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

Curious -- is there a corresponding change to RECORD_PARAM_COMMS that accepts this new argument type?

Yes. It's here.
I can't get the code position link of this PR, so put screen shot here.

kwen2501

Giving approval to unblock.
Please be advised that we cannot guarantee compatibility going forward with the trace player you mentioned -- we may change the sequence number as we see fit.

kwen2501 · 2024-09-23T08:14:23Z

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

@fduwjj @c-p-i-o do you think we can consolidate the bool isP2P = isP2POp(opType) logic below with this change? It may not be so trivial though. See this comment: https://github.com/pytorch/pytorch/pull/134578/files#r1764430474

Sure. I can try to address this in a subsequent change.

GSSBMW · 2024-09-23T08:53:53Z

Thanks, @kwen2501 @shengfukevin
Then, what's the next step for merging?

kwen2501 · 2024-09-25T03:33:29Z

It seems there are lint issues. Can you run
lintrunner -a
locally to fix them?

If CI looks good, you can hit:
pytorchbot merge

GSSBMW · 2024-10-09T14:37:22Z

Hi, @kwen2501
I have fixed the issues of lint and CLA, rebase from main. Still this blocking issue. How should I fix it? Or need your approve again? Thanks!!

GSSBMW · 2024-10-09T15:21:39Z

cc+ @shengfukevin @c-p-i-o

c-p-i-o · 2024-10-09T17:34:05Z

cc+ @shengfukevin @c-p-i-o

Can you run:
"@pytorchbot merge"

This should get this PR into the main branch after CI passes.

GSSBMW · 2024-10-09T23:28:21Z

@pytorchbot merge

pytorchmergebot · 2024-10-09T23:30: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

Summary: The recent DIFF pytorch/pytorch#134578 change the sequence id in ET from sequence id ( an integer) to a [sequence id, is_p2p_op], an array of two integers. The replay code need to make the similar change. Differential Revision: D65099031

Summary: Pull Request resolved: #185 The recent DIFF pytorch/pytorch#134578 change the sequence id in ET from sequence id ( an integer) to a [sequence id, is_p2p_op], an array of two integers. The replay code need to make the similar change. Reviewed By: briancoutinho Differential Revision: D65099031 fbshipit-source-id: 91174f27cf3ced1d25d0c11748adea88b87bdc03

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

pytorchbot added the open source label Aug 27, 2024

zou3519 requested review from c-p-i-o, kwen2501 and wconstab August 27, 2024 14:23

zou3519 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 27, 2024

GSSBMW mentioned this pull request Aug 28, 2024

fix bump up of seqCollective_ in function ProcessGroupNCCL::pointToPoint() #134452

Closed

GSSBMW force-pushed the fix_seq_id branch from 597aa8c to bed7d45 Compare August 28, 2024 07:33

GSSBMW force-pushed the fix_seq_id branch from bed7d45 to dabc3fa Compare September 16, 2024 07:03

GSSBMW mentioned this pull request Sep 16, 2024

Pass in the appropiate sequence no. for "wait" op #134619

Closed

TaekyungHeo mentioned this pull request Sep 16, 2024

fix found issues of comm replay facebookresearch/param#180

Closed

kwen2501 reviewed Sep 17, 2024

View reviewed changes

kwen2501 reviewed Sep 18, 2024

View reviewed changes

GSSBMW mentioned this pull request Sep 18, 2024

coalescing implementation in ProcessGroupNCCL.hpp/cpp #134833

Closed

kwen2501 approved these changes Sep 23, 2024

View reviewed changes

kwen2501 reviewed Sep 23, 2024

View reviewed changes

GSSBMW force-pushed the fix_seq_id branch from dabc3fa to 4346d98 Compare October 9, 2024 14:32

fix sequence number in execution trace dump

1ad6e7a

GSSBMW force-pushed the fix_seq_id branch from 4346d98 to 1ad6e7a Compare October 9, 2024 14:47

c-p-i-o approved these changes Oct 9, 2024

View reviewed changes

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

pytorchmergebot added the merging label Oct 9, 2024

pytorchmergebot added the Merged label Oct 10, 2024

pytorchmergebot closed this in 2491524 Oct 10, 2024

pytorchmergebot removed the merging label Oct 10, 2024

shengfukevin mentioned this pull request Oct 28, 2024

Fix the bug for sequence id facebookresearch/param#185

Closed

	uint64_t seqCollective_{0};

	// Counting for the sequential number of NCCL P2P calls.
	uint64_t seqP2P_{0};

fix sequence number for group #134578

fix sequence number for group #134578

Uh oh!

Conversation

GSSBMW commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Aug 27, 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/134578

✅ No Failures

Uh oh!

linux-foundation-easycla bot commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GSSBMW commented Aug 27, 2024

Uh oh!

GSSBMW commented Sep 16, 2024

Uh oh!

kwen2501 left a comment

Choose a reason for hiding this comment

Uh oh!

kwen2501 commented Sep 17, 2024

Uh oh!

GSSBMW commented Sep 18, 2024

Uh oh!

kwen2501 Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

kwen2501 Sep 18, 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!

GSSBMW Sep 18, 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 Sep 18, 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!

GSSBMW Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

kwen2501 left a comment

Choose a reason for hiding this comment

Uh oh!

kwen2501 Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

c-p-i-o Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

GSSBMW commented Sep 23, 2024

Uh oh!

kwen2501 commented Sep 25, 2024

Uh oh!

GSSBMW commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GSSBMW commented Oct 9, 2024

Uh oh!

c-p-i-o commented Oct 9, 2024

Uh oh!

GSSBMW commented Oct 9, 2024

Uh oh!

pytorchmergebot commented Oct 9, 2024

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

GSSBMW commented Aug 27, 2024 •

edited

Loading

pytorch-bot bot commented Aug 27, 2024 •

edited

Loading

linux-foundation-easycla bot commented Aug 27, 2024 •

edited

Loading

kwen2501 Sep 18, 2024 •

edited

Loading

GSSBMW Sep 18, 2024 •

edited

Loading

kwen2501 Sep 18, 2024 •

edited

Loading

GSSBMW commented Oct 9, 2024 •

edited

Loading