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

shengfukevin · 2024-08-27T22:38:10Z

Summary:
In a ProcessGroupNCCL, there are two different sequence no. for collective and p2p op. "wait" op need to know what type of op it waits.

In this DIFF, I changed RECORD_PARAM_COM interface to add a new entry "isP2P", this is a boolean varaible to flag if the op is a point to point op or not.

Differential Revision: D61882985

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

pytorch-bot · 2024-08-27T22:38:13Z

🔗 Helpful Links

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

📄 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.

❌ 2 New Failures, 2 Unrelated Failures

As of commit 3974e03 with merge base 86e03a6 ():

NEW FAILURES - The following jobs have failed:

Lint / lintrunner-clang / linux-job (gh)
>>> Lint for torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp:
trunk / linux-focal-cuda12.4-py3.10-gcc9-experimental-split-build-test / test (default, 3, 5, linux.4xlarge.nvidia.gpu) (gh)
test_transformers.py::TestSDPACudaOnlyCUDA::test_mem_efficient_attention_attn_mask_vs_math_ref_grads_batch_size_1_seq_len_q_152_seq_len_k_37_head_dim_64_is_causal_False_dropout_p_0_22_float32_scale_l1_cuda_float32

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

pull / linux-focal-cuda12.1-py3.10-gcc9-sm86 / test (default, 4, 5, linux.g5.4xlarge.nvidia.gpu) (gh) (similar failure)
inductor/test_cuda_cpp_wrapper.py::TestCudaWrapper::test_mm_plus_mm2_cuda_cuda_wrapper
trunk / linux-focal-cuda12.4-py3.10-gcc9-sm86 / test (default, 4, 5, linux.g5.4xlarge.nvidia.gpu) (gh) (similar failure)
inductor/test_cuda_cpp_wrapper.py::TestCudaWrapper::test_mm_plus_mm2_cuda_cuda_wrapper

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

facebook-github-bot · 2024-08-27T22:38:22Z

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

facebook-github-bot · 2024-08-27T23:16:41Z

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

GSSBMW · 2024-08-30T02:42:35Z

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

Why ++ here?

facebook-github-bot · 2024-08-30T05:03:46Z

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

…pytorch#134619) Summary: Pull Request resolved: pytorch#134619 In a ProcessGroupNCCL, there are two different sequence no. for collective and p2p op. "wait" op need to know what type of op it waits. In this DIFF, I changed RECORD_PARAM_COM interface to add a new entry "isP2P", this is a boolean varaible to flag if the op is a point to point op or not. Test Plan: buck2 run mode/opt kineto/libkineto/fb/integration_tests:pytorch_execution_trace_integration_test Differential Revision: D61882985

facebook-github-bot · 2024-08-30T05:08:53Z

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

GSSBMW · 2024-08-30T05:58:46Z

torch/csrc/distributed/c10d/ParamCommsUtils.hpp


 #define RECORD_PARAM_COMMS(                                                    \
    seq,                                                                       \
+    isP2P,                                                                     \


How about making seq be a tuple of (seq_id, is_p2p), but not a stand alone integer? Then make RECORD_PARAM_COMM and RECORD_PARAM_COMM_DATA have the same trace fromat, exept the first tensor. Otherwise, replayer needs to branch positional parsing.
After that, we will use (seq_id, is_p2p, pg_id) as key to match comm op and wait.

GSSBMW · 2024-08-30T06:09:32Z

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

 c10::intrusive_ptr<Work> ProcessGroupNCCL::barrier(const BarrierOptions& opts) {
  RECORD_PARAM_COMMS(
      static_cast<int>(
          this->getSequenceNumberForGroup() + 1), // seq + 1 to match collective


seems we should remove the function getSequenceNumberForGroup(), but use seqCollective_ directly.

func name can't represent it returns collective sequence id;

keep conssistant with P2P, which use seqP2P directly;

Or we can reimplement getSequenceNumberForGroup() to:

uint64_t ProcessGroupNCCL::getSequenceNumberForGroup(OpType typ, bool batchP2P = false) { return isP2P(typ, batchP2P) ? seqP2P_ : seqCollective_; }

getSequenceNumberForGroup is a virtual function defined in root class BackEnd. P2P seems only used in ProcessGroupNCCL. Add a new input parameter probably does not make sense to other sub-classes.

got it. Then how about using seqCollective_ directly?

GSSBMW · 2024-08-30T06:16:52Z

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

      rank,
      opType,
-      seqCollective_,
+      isP2POp(opType) ? seqP2P_ : seqCollective_,


according to isP2POp, batched P2P is not P2P op. So we need to check coalesing here.

batched P2P is implemented through Coalescing. The id we want to pick is based on if it is either a single P2P op/coalesced P2P op or a single collective op/coalesced collective op, does not matter it is coalesced or not.

Ok. If it's coalesed p2p, seqCollective_ is bumped up in startCoalesing() and we pick seqCollective_ here.
Not sure whether this will be changed/rectified according to this comment from @fduwjj

fduwjj

LGTM

GSSBMW · 2024-08-31T01:24:07Z

Let's wait for a conclusion of this issue?

facebook-github-bot · 2024-08-31T01:52:44Z

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

…pytorch#134619) Summary: Pull Request resolved: pytorch#134619 In a ProcessGroupNCCL, there are two different sequence no. for collective and p2p op. "wait" op need to know what type of op it waits. In this DIFF, I changed RECORD_PARAM_COM interface to add a new entry "isP2P", this is a boolean varaible to flag if the op is a point to point op or not. Test Plan: buck2 run mode/opt kineto/libkineto/fb/integration_tests:pytorch_execution_trace_integration_test Reviewed By: c-p-i-o Differential Revision: D61882985

facebook-github-bot · 2024-08-31T01:59:11Z

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

GSSBMW · 2024-09-02T04:18:08Z

torch/csrc/distributed/c10d/ProcessGroupNCCL.cpp

  RECORD_PARAM_COMMS_DATA(
      static_cast<int>(
-          this->getSequenceNumberForGroup() + 1), // seq + 1 to match collective
+          this->seqP2P_ + 1), // seqP2P_ + 1 to match pointToPoint op


seems better to use tuple (seq, is_p2p) for RECORD_PARAM_COMMS_DATA. Otherewise, replayer needs to copy the logic of PyTorch to check collectiveName_. Keep consistant pattern would be better.

GSSBMW · 2024-09-16T07:08:15Z

@shengfukevin Please refer to this PR (#134578) or use it directly. Thanks!

github-actions · 2024-11-15T07:34:30Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

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

facebook-github-bot added the fb-exported label Aug 27, 2024

shengfukevin force-pushed the export-D61882985 branch from 0b29c7e to ac35fdf Compare August 27, 2024 23:16

GSSBMW reviewed Aug 30, 2024

View reviewed changes

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

Copy link

GSSBMW Aug 30, 2024

Choose a reason for hiding this comment

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

Why ++ here?

shengfukevin force-pushed the export-D61882985 branch from ac35fdf to 2c98af6 Compare August 30, 2024 05:03

shengfukevin force-pushed the export-D61882985 branch from 2c98af6 to 361be42 Compare August 30, 2024 05:08

GSSBMW reviewed Aug 30, 2024

View reviewed changes

fduwjj approved these changes Aug 30, 2024

View reviewed changes

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

shengfukevin force-pushed the export-D61882985 branch from 361be42 to d9bf310 Compare August 31, 2024 01:52

shengfukevin force-pushed the export-D61882985 branch from d9bf310 to 3974e03 Compare August 31, 2024 01:59

GSSBMW reviewed Sep 2, 2024

View reviewed changes

GSSBMW mentioned this pull request Sep 16, 2024

fix sequence number for group #134578

Closed

github-actions bot added the Stale label Nov 15, 2024

github-actions bot closed this Dec 15, 2024

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

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

Uh oh!

Conversation

shengfukevin commented Aug 27, 2024 • edited by pytorch-bot bot 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/134619

❌ 2 New Failures, 2 Unrelated Failures

Uh oh!

facebook-github-bot commented Aug 27, 2024

Uh oh!

facebook-github-bot commented Aug 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Aug 30, 2024

Uh oh!

facebook-github-bot commented Aug 30, 2024

Uh oh!

GSSBMW Aug 30, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fduwjj left a comment

Choose a reason for hiding this comment

Uh oh!

GSSBMW commented Aug 31, 2024

Uh oh!

facebook-github-bot commented Aug 31, 2024

Uh oh!

facebook-github-bot commented Aug 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GSSBMW commented Sep 16, 2024

Uh oh!

github-actions bot commented Nov 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shengfukevin commented Aug 27, 2024 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Aug 27, 2024 •

edited

Loading

GSSBMW Aug 30, 2024 •

edited

Loading