From 99ef23ca4566c5f5a2d53825d85ac7b175bf9c2b Mon Sep 17 00:00:00 2001 From: Praneeth Vallem Date: Fri, 2 Oct 2026 11:38:23 -0700 Subject: [PATCH 1/3] test(rccl): add binary-mode Recorder RedOp record tests Recorder.RedOpRecordJson only sees what the JSON format strings print, so the raw rcclApiCall fields written in binary mode, and anything Destroy must not copy, are unchecked. Recorder.RedOpRecordBinary reads both records back from the binary dump and is registered next to RedOpRecordJson in every test_runner config. --- projects/rccl/test/_RecorderTests.cpp | 61 +++++++++++++++++++ .../test_runner/configs/ci-precheckin.json | 1 + .../configs/mi300_mellanox_ib_func.json | 5 ++ .../configs/mi300x_mellanox_ib.json | 1 + .../configs/mi355x_thor2_roce.json | 5 ++ .../configs/mi355x_thor2_roce_func.json | 5 ++ .../test_runner/configs/mi455_ainic_roce.json | 5 ++ .../configs/mi455_ainic_roce_func.json | 5 ++ 8 files changed, 88 insertions(+) diff --git a/projects/rccl/test/_RecorderTests.cpp b/projects/rccl/test/_RecorderTests.cpp index 3a406970627f..7a14db6d5c16 100644 --- a/projects/rccl/test/_RecorderTests.cpp +++ b/projects/rccl/test/_RecorderTests.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #define HIPCALL(cmd) \ @@ -134,6 +135,66 @@ namespace RcclUnitTesting ); } + /** + * \brief Verify binary mode writes PreMulSum scalar/datatype/residence as sendbuff/datatype/root; Destroy drops them + * ******************************************************************************************/ + TEST(Recorder, RedOpRecordBinary) + { + RUN_ISOLATED_TEST_WITH_ENV( + "RedOpRecordBinary", + []() + { + int ranks[] = {2, 3, 5}; + auto comm = std::make_unique(); + comm->nRanks = 4; + comm->localRank = 1; + comm->localRankToRank = ranks; + float scalar = 2.0f; + double ignored = 4.0; + // Non-default datatype and residence, so a dropped or inverted PreMulSum guard cannot match the defaults. + ASSERT_EQ(ncclSuccess, rccl::Recorder::instance().record(rccl::rrRedOpCreatePreMulSum, ncclMax, comm.get(), + ncclFloat32, ncclScalarHostImmediate, &scalar)); + // Destroy gets non-default scalar arguments too, so a guard that always copies them is caught. + ASSERT_EQ(ncclSuccess, rccl::Recorder::instance().record(rccl::rrRedOpDestroy, ncclMax, comm.get(), + ncclFloat64, ncclScalarHostImmediate, &ignored)); + + char host[256]; + gethostname(host, sizeof(host)); + std::string filename = "/tmp/test_redop." + std::to_string(getpid()) + "." + std::string(host) + ".bin"; + std::ifstream fp(filename, std::ios::binary); + ASSERT_TRUE(fp.is_open()) << "Recorder did not create expected file: " << filename; + // One spare slot so a stray extra record shows up in bytesRead. + rccl::rcclApiCall calls[3]; + fp.read(reinterpret_cast(calls), sizeof(calls)); + std::streamsize bytesRead = fp.gcount(); + fp.close(); + std::remove(filename.c_str()); + // Binary mode writes exactly one raw struct per call and no header. + ASSERT_EQ(bytesRead, static_cast(2 * sizeof(rccl::rcclApiCall))); + + const rccl::rcclApiCall& create = calls[0]; + EXPECT_EQ(create.type, rccl::rrRedOpCreatePreMulSum); + EXPECT_EQ(create.op, ncclMax); + EXPECT_EQ(create.comm, comm.get()); + // nRanks and globalRank are taken from the comm and only reach disk in binary mode. + EXPECT_EQ(create.nRanks, 4); + EXPECT_EQ(create.globalRank, 3); + EXPECT_EQ(create.sendbuff, static_cast(&scalar)) << "sendbuff must come from scalar"; + EXPECT_EQ(create.datatype, ncclFloat32) << "datatype must come from the datatype argument"; + EXPECT_EQ(create.root, static_cast(ncclScalarHostImmediate)) << "root must come from residence"; + + const rccl::rcclApiCall& destroy = calls[1]; + EXPECT_EQ(destroy.type, rccl::rrRedOpDestroy); + EXPECT_EQ(destroy.op, ncclMax); + EXPECT_EQ(destroy.comm, comm.get()); + EXPECT_NE(destroy.sendbuff, static_cast(&ignored)) << "Destroy must not record the scalar"; + EXPECT_NE(destroy.datatype, ncclFloat64) << "Destroy must not record the datatype"; + EXPECT_NE(destroy.root, static_cast(ncclScalarHostImmediate)) << "Destroy must not record the residence"; + }, + {{"RCCL_REPLAY_FILE", "/tmp/test_redop.bin"}} + ); + } + /** * \brief Verify RCCL Recorder's integrity in multithread context by comparing Recorder * instance across different threads. diff --git a/projects/rccl/tools/scripts/test_runner/configs/ci-precheckin.json b/projects/rccl/tools/scripts/test_runner/configs/ci-precheckin.json index 5c81618f52a1..741d4954ddef 100644 --- a/projects/rccl/tools/scripts/test_runner/configs/ci-precheckin.json +++ b/projects/rccl/tools/scripts/test_runner/configs/ci-precheckin.json @@ -86,6 +86,7 @@ {"name": "Recorder.ParseJson", "description": "Recorder parse JSON tests", "test_filter": "Recorder.ParseJson", "timeout": 60}, {"name": "Recorder.VerifyMultithread", "description": "Recorder multithread verification tests", "test_filter": "Recorder.VerifyMultithread", "timeout": 60}, {"name": "Recorder.RedOpRecordJson", "description": "Recorder RedOp record JSON output", "test_filter": "Recorder.RedOpRecordJson", "timeout": 60}, + {"name": "Recorder.RedOpRecordBinary", "description": "Recorder RedOp record binary output", "test_filter": "Recorder.RedOpRecordBinary", "timeout": 60}, {"name": "ProxyTraceTestFixture.nonEmptySingleton", "description": "ProxyTrace non-empty singleton test", "test_filter": "ProxyTraceTestFixture.nonEmptySingleton", "timeout": 60}, {"name": "ProxyTraceTestFixture.addTraceOp", "description": "ProxyTrace add trace operation test", "test_filter": "ProxyTraceTestFixture.addTraceOp", "timeout": 60}, {"name": "ProxyTraceTestFixture.getMapSizeMB", "description": "ProxyTrace get map size test", "test_filter": "ProxyTraceTestFixture.getMapSizeMB", "timeout": 60}, diff --git a/projects/rccl/tools/scripts/test_runner/configs/mi300_mellanox_ib_func.json b/projects/rccl/tools/scripts/test_runner/configs/mi300_mellanox_ib_func.json index 493eedda8db5..723db355e34f 100644 --- a/projects/rccl/tools/scripts/test_runner/configs/mi300_mellanox_ib_func.json +++ b/projects/rccl/tools/scripts/test_runner/configs/mi300_mellanox_ib_func.json @@ -1833,6 +1833,11 @@ "description": "Recorder RedOp record JSON output", "test_filter": "Recorder.RedOpRecordJson" }, + { + "name": "Recorder.RedOpRecordBinary", + "description": "Recorder RedOp record binary output", + "test_filter": "Recorder.RedOpRecordBinary" + }, { "name": "ProxyTrace.nonEmptySingleton", "description": "Proxy trace non-empty singleton", diff --git a/projects/rccl/tools/scripts/test_runner/configs/mi300x_mellanox_ib.json b/projects/rccl/tools/scripts/test_runner/configs/mi300x_mellanox_ib.json index 434d95f40411..18c560f58e55 100644 --- a/projects/rccl/tools/scripts/test_runner/configs/mi300x_mellanox_ib.json +++ b/projects/rccl/tools/scripts/test_runner/configs/mi300x_mellanox_ib.json @@ -1196,6 +1196,7 @@ {"name": "Recorder.ParseJson", "description": "Recorder parse JSON trace", "test_filter": "Recorder.ParseJson"}, {"name": "Recorder.VerifyMultithread", "description": "Recorder multithreaded verification", "test_filter": "Recorder.VerifyMultithread"}, {"name": "Recorder.RedOpRecordJson", "description": "Recorder RedOp record JSON output", "test_filter": "Recorder.RedOpRecordJson"}, + {"name": "Recorder.RedOpRecordBinary", "description": "Recorder RedOp record binary output", "test_filter": "Recorder.RedOpRecordBinary"}, {"name": "IbvSymbolsEnv.DefaultLoadsLibrary", "description": "NCCL_IBVERBS_LIB unset loads default libibverbs", "test_filter": "IbvSymbolsEnv.DefaultLoadsLibrary"}, {"name": "IbvSymbolsEnv.EnvOverrideValidSoname", "description": "NCCL_IBVERBS_LIB override to a valid soname is honored", "test_filter": "IbvSymbolsEnv.EnvOverrideValidSoname"}, {"name": "IbvSymbolsEnv.BogusPathFallsBackToDefault", "description": "NCCL_IBVERBS_LIB bogus path falls back to default libibverbs", "test_filter": "IbvSymbolsEnv.BogusPathFallsBackToDefault"}, diff --git a/projects/rccl/tools/scripts/test_runner/configs/mi355x_thor2_roce.json b/projects/rccl/tools/scripts/test_runner/configs/mi355x_thor2_roce.json index 46b968edf3b6..aaa08a78e799 100644 --- a/projects/rccl/tools/scripts/test_runner/configs/mi355x_thor2_roce.json +++ b/projects/rccl/tools/scripts/test_runner/configs/mi355x_thor2_roce.json @@ -1427,6 +1427,11 @@ "description": "Recorder RedOp record JSON output", "test_filter": "Recorder.RedOpRecordJson" }, + { + "name": "Recorder.RedOpRecordBinary", + "description": "Recorder RedOp record binary output", + "test_filter": "Recorder.RedOpRecordBinary" + }, { "name": "ProxyTrace.nonEmptySingleton", "description": "Proxy trace non-empty singleton", diff --git a/projects/rccl/tools/scripts/test_runner/configs/mi355x_thor2_roce_func.json b/projects/rccl/tools/scripts/test_runner/configs/mi355x_thor2_roce_func.json index 5e53129a6129..f735df4d386f 100644 --- a/projects/rccl/tools/scripts/test_runner/configs/mi355x_thor2_roce_func.json +++ b/projects/rccl/tools/scripts/test_runner/configs/mi355x_thor2_roce_func.json @@ -1883,6 +1883,11 @@ "description": "Recorder RedOp record JSON output", "test_filter": "Recorder.RedOpRecordJson" }, + { + "name": "Recorder.RedOpRecordBinary", + "description": "Recorder RedOp record binary output", + "test_filter": "Recorder.RedOpRecordBinary" + }, { "name": "ProxyTrace.nonEmptySingleton", "description": "Proxy trace non-empty singleton", diff --git a/projects/rccl/tools/scripts/test_runner/configs/mi455_ainic_roce.json b/projects/rccl/tools/scripts/test_runner/configs/mi455_ainic_roce.json index b3d7c47e1d40..0fb4550416dd 100644 --- a/projects/rccl/tools/scripts/test_runner/configs/mi455_ainic_roce.json +++ b/projects/rccl/tools/scripts/test_runner/configs/mi455_ainic_roce.json @@ -1970,6 +1970,11 @@ "description": "Recorder RedOp record JSON output", "test_filter": "Recorder.RedOpRecordJson" }, + { + "name": "Recorder.RedOpRecordBinary", + "description": "Recorder RedOp record binary output", + "test_filter": "Recorder.RedOpRecordBinary" + }, { "name": "ProxyTrace.nonEmptySingleton", "description": "Proxy trace non-empty singleton", diff --git a/projects/rccl/tools/scripts/test_runner/configs/mi455_ainic_roce_func.json b/projects/rccl/tools/scripts/test_runner/configs/mi455_ainic_roce_func.json index 17c3afb09e74..2f2058c24994 100644 --- a/projects/rccl/tools/scripts/test_runner/configs/mi455_ainic_roce_func.json +++ b/projects/rccl/tools/scripts/test_runner/configs/mi455_ainic_roce_func.json @@ -2024,6 +2024,11 @@ "description": "Recorder RedOp record JSON output", "test_filter": "Recorder.RedOpRecordJson" }, + { + "name": "Recorder.RedOpRecordBinary", + "description": "Recorder RedOp record binary output", + "test_filter": "Recorder.RedOpRecordBinary" + }, { "name": "ProxyTrace.nonEmptySingleton", "description": "Proxy trace non-empty singleton", From e511278c8a640df8268b3e65e00e17f4e2dfebf9 Mon Sep 17 00:00:00 2001 From: Praneeth Vallem Date: Fri, 2 Oct 2026 12:42:05 -0700 Subject: [PATCH 2/3] test(rccl): pin Recorder RedOp Destroy fields to their recorded values --- projects/rccl/test/_RecorderTests.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/projects/rccl/test/_RecorderTests.cpp b/projects/rccl/test/_RecorderTests.cpp index 7a14db6d5c16..532e1fa9b9ca 100644 --- a/projects/rccl/test/_RecorderTests.cpp +++ b/projects/rccl/test/_RecorderTests.cpp @@ -187,9 +187,10 @@ namespace RcclUnitTesting EXPECT_EQ(destroy.type, rccl::rrRedOpDestroy); EXPECT_EQ(destroy.op, ncclMax); EXPECT_EQ(destroy.comm, comm.get()); - EXPECT_NE(destroy.sendbuff, static_cast(&ignored)) << "Destroy must not record the scalar"; - EXPECT_NE(destroy.datatype, ncclFloat64) << "Destroy must not record the datatype"; - EXPECT_NE(destroy.root, static_cast(ncclScalarHostImmediate)) << "Destroy must not record the residence"; + // record() builds from the aggregate ncclInfo{.op, .comm}, so the fields Destroy must not set are value-initialised. + EXPECT_EQ(destroy.sendbuff, nullptr) << "Destroy must not record the scalar"; + EXPECT_EQ(destroy.datatype, ncclInt8) << "Destroy must not record the datatype"; + EXPECT_EQ(destroy.root, 0) << "Destroy must not record the residence"; }, {{"RCCL_REPLAY_FILE", "/tmp/test_redop.bin"}} ); From 0cf8a04bd59b6920914054f342a63a5b4db27cf4 Mon Sep 17 00:00:00 2001 From: Praneeth Vallem Date: Fri, 2 Oct 2026 12:47:14 -0700 Subject: [PATCH 3/3] test(rccl): keep Recorder Destroy comment within 120 columns --- projects/rccl/test/_RecorderTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/projects/rccl/test/_RecorderTests.cpp b/projects/rccl/test/_RecorderTests.cpp index 532e1fa9b9ca..0379ac5a0fea 100644 --- a/projects/rccl/test/_RecorderTests.cpp +++ b/projects/rccl/test/_RecorderTests.cpp @@ -187,7 +187,7 @@ namespace RcclUnitTesting EXPECT_EQ(destroy.type, rccl::rrRedOpDestroy); EXPECT_EQ(destroy.op, ncclMax); EXPECT_EQ(destroy.comm, comm.get()); - // record() builds from the aggregate ncclInfo{.op, .comm}, so the fields Destroy must not set are value-initialised. + // record() builds from the aggregate ncclInfo{.op, .comm}, so fields Destroy must not set are zero. EXPECT_EQ(destroy.sendbuff, nullptr) << "Destroy must not record the scalar"; EXPECT_EQ(destroy.datatype, ncclInt8) << "Destroy must not record the datatype"; EXPECT_EQ(destroy.root, 0) << "Destroy must not record the residence";