fix found issues of comm replay #180

TaekyungHeo · 2024-09-16T17:30:12Z

Depend On

Summary

fix replay to send/recv
fix blocking option
fix process group update of barrier
Comment out unused function incast and multicast
remove function extractCommsInfo() for deprecatd basic trace
fix wait comm op for both collective and p2p

TestPlan

comm_replay --trace-type et --trace-path /home/sanshang/lustre_storage/000_code/chakra/comm_replay_eval_06/training/et_iter_50_51 --num-replays 10

shengfukevin

HI Sanshan,

Thanks for implementing this P2P feature. Overall looks good. Please check my inline comments.

Also please provide a couple ET for P2P cases. We will put them into our integration tests.

Regards
Sheng

shengfukevin · 2024-10-10T17:42:31Z

et_replay/comm/backend/base_backend.py

@@ -144,8 +144,10 @@ def __init__(self) -> None:
            "reduce_scatter_base": self.reduce_scatter_base,  # pyre-ignore[16]:
            "scatter": self.scatter,  # pyre-ignore[16]:
            "barrier": self.barrier,
-            "incast": self.incast,  # pyre-ignore[16]:


Please remove the code since it is deprecated.

shengfukevin · 2024-10-10T18:39:02Z

et_replay/comm/commsTraceParser.py

@@ -122,10 +122,12 @@ def _parse_comms_op_node(  # noqa: C901
    comm_nodes = (
        node for node in in_trace.nodes.values() if node.name == "record_param_comms"
    )
+    is_seq = lambda x: isinstance(x, list) and len(x) == 2 and isinstance(x[0], int) and isinstance(x[1], bool)


Can is_seq be renamed to is_seq_id? It makes easier to understand the code

shengfukevin · 2024-10-10T18:39:59Z

et_replay/comm/commsTraceParser.py

    for node in comm_nodes:
        # according to macro RECORD_PARAM_COMMS and RECORD_PARAM_COMMS_DATA in torch/csrc/distributed/c10d/ParamCommsUtils.hpp
-        # ["wait", "barrier", "init"] record 1st element as seq, others record starting from input tensor
-        index_base = 0 if isinstance(node.inputs[0], int) else 1
+        # ["wait", "barrier", "init"] record 1st element as seq, whose 1st element is sequence number as int, 2nd element is isP2P as bool


["wait", "barrier", "init"] record 1st element as seq, whose 1st element is sequence number as int, 2nd element is isP2P as bool -->

["wait", "barrier", "init"] record 1st element as seq_id, whose 1st element is an integer for sequence number, and 2nd element is a bool for isP2P

shengfukevin · 2024-10-10T18:46:37Z

et_replay/comm/comms_utils.py

@@ -1435,12 +1435,9 @@ def readArgs(self, parser: ArgumentParser) -> argparse.Namespace:
            help="The backend to be used in PyTorch distributed process group",
        )  #  backend used for the network stack
        parser.add_argument(
-            "--z",


Can we have "--b" as a short version?

shengfukevin · 2024-10-10T19:10:28Z

et_replay/comm/backend/pytorch_dist_backend.py

+
+        # ExecutionTraceObserver dump records from cpp level, which are always async send/recv.
+        # Then replay in torch.distributed API level, we should use isend/irecv.
+        self.collectiveFunc["send"] = self.isend


Since there is no isend and irecv, can we also change self.isend ->self.send, self.irecv -> self.recv

I don't think so. Because

"send" in trace is in c++ context, whose semantics is always async send

self.isend function here is in python context, where send represent sync send, but isend represent async send

So, I think we should keep the name convention here, as I explained in the comment.

shengfukevin · 2024-10-10T19:22:06Z

et_replay/comm/backend/pytorch_dist_backend.py

@@ -474,6 +474,7 @@ def all_gather_base(self, collectiveArgs, retFlag=False, pair=False):
        if retFlag:
            return retObj

+    '''


Let's clean up the dead code instead of commenting it out

shengfukevin · 2024-10-10T19:23:06Z

et_replay/comm/backend/pytorch_dist_backend.py

@@ -527,6 +530,7 @@ def multicast(self, collectiveArgs):
                self.irecv(collectiveArgs, collectiveArgs.srcOrDst)
            else:
                self.recv(collectiveArgs, collectiveArgs.srcOrDst)
+    '''


Remove send and rename isend -> send. The same for recv.

Done for removing.

GSSBMW · 2024-10-11T12:43:32Z

@shengfukevin
Has tried to resolve all your comments. Please help review, thanks!
For ET of P2P, does pytorch test has such cases to generate? Otherwise I need to write test to generate such traces.

Summary: fix replay to send/recv fix blocking option fix process group update of barrier Comment out unused function incast and multicast remove function extractCommsInfo() for deprecatd basic trace fix wait comm op for both collective and p2p Test Plan: comm_replay --trace-type et --trace-path /home/sanshang/lustre_storage/000_code/chakra/comm_replay_eval_06/training/et_iter_50_51 --num-replays 10 ## additional notes This is copied from #180. Differential Revision: D64312970 Pulled By: shengfukevin

c-p-i-o · 2024-10-15T16:45:00Z

@TaekyungHeo, do you want to address the linter errors?
You can run:
make lint followed by lintrunner -a that should hopefully take care of the linter errors for you.

Summary: fix replay to send/recv fix blocking option fix process group update of barrier Comment out unused function incast and multicast remove function extractCommsInfo() for deprecatd basic trace fix wait comm op for both collective and p2p Test Plan: comm_replay --trace-type et --trace-path /home/sanshang/lustre_storage/000_code/chakra/comm_replay_eval_06/training/et_iter_50_51 --num-replays 10 ## additional notes This is copied from #180. Differential Revision: D64312970 Pulled By: shengfukevin

Summary: fix replay to send/recv fix blocking option fix process group update of barrier Comment out unused function incast and multicast remove function extractCommsInfo() for deprecatd basic trace fix wait comm op for both collective and p2p Pull Request resolved: #182 Test Plan: comm_replay --trace-type et --trace-path /home/sanshang/lustre_storage/000_code/chakra/comm_replay_eval_06/training/et_iter_50_51 --num-replays 10 ## additional notes This is copied from #180. Reviewed By: briancoutinho Differential Revision: D64312970 Pulled By: shengfukevin fbshipit-source-id: 42c3657f3ba789a29a8a25db4fdfa4f54a394c27

shengfukevin · 2024-10-21T21:19:48Z

the duplicated PR has been merged aleady.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 16, 2024

TaekyungHeo marked this pull request as ready for review September 17, 2024 02:12

TaekyungHeo requested review from kingchc, louisfeng, sunghlin, shengbao-zheng and briancoutinho as code owners September 17, 2024 02:12

TaekyungHeo changed the title ~~Draft: fix found issues of comm replay~~ fix found issues of comm replay Sep 18, 2024

This was referenced Sep 18, 2024

fix sequence number for group pytorch/pytorch#134578

Closed

coalescing implementation in ProcessGroupNCCL.hpp/cpp pytorch/pytorch#134833

Closed

shengfukevin requested changes Oct 10, 2024

View reviewed changes

TaekyungHeo force-pushed the sanshang/fix_comm_replay branch from 29b5a46 to 094a2ab Compare October 11, 2024 10:09

shengfukevin approved these changes Oct 14, 2024

View reviewed changes

sanshang-nv added 6 commits October 13, 2024 21:36

fix replay to send/recv

69b790f

fix blocking option

cdaf08a

fix process group update of barrier

ca75698

Remove code related to deprecated function incast() and multicast()

6f9a0cb

remove function extractCommsInfo() for deprecatd basic trace

312b2de

fix wait comm op for both collective and p2p

b682ffb

shengfukevin mentioned this pull request Oct 14, 2024

fix found issues of comm replay #182

Closed

TaekyungHeo force-pushed the sanshang/fix_comm_replay branch from 094a2ab to b682ffb Compare October 14, 2024 11:31

shengfukevin closed this Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix found issues of comm replay #180

fix found issues of comm replay #180

TaekyungHeo commented Sep 16, 2024 •

edited

Loading

shengfukevin left a comment

shengfukevin Oct 10, 2024

GSSBMW Oct 11, 2024

shengfukevin Oct 10, 2024

GSSBMW Oct 11, 2024

shengfukevin Oct 10, 2024

GSSBMW Oct 11, 2024

shengfukevin Oct 10, 2024

GSSBMW Oct 11, 2024

shengfukevin Oct 10, 2024

GSSBMW Oct 11, 2024 •

edited

Loading

shengfukevin Oct 10, 2024

GSSBMW Oct 11, 2024

shengfukevin Oct 10, 2024

GSSBMW Oct 11, 2024

GSSBMW commented Oct 11, 2024

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

shengfukevin commented Oct 21, 2024

fix found issues of comm replay #180

fix found issues of comm replay #180

Conversation

TaekyungHeo commented Sep 16, 2024 • edited Loading

Depend On

Summary

TestPlan

shengfukevin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

["wait", "barrier", "init"] record 1st element as seq, whose 1st element is sequence number as int, 2nd element is isP2P as bool -->

["wait", "barrier", "init"] record 1st element as seq_id, whose 1st element is an integer for sequence number, and 2nd element is a bool for isP2P

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GSSBMW Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GSSBMW commented Oct 11, 2024

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

shengfukevin commented Oct 21, 2024

TaekyungHeo commented Sep 16, 2024 •

edited

Loading

GSSBMW Oct 11, 2024 •

edited

Loading