Repository navigation
Conversation
Move MPI launcher assembly, connectivity-gate retry, and Broadcast debug-log capture into gin_sdma_harness.py so AllGather, AllToAll, and Broadcast GIN-SDMA pytest harnesses share one implementation and stop drifting. Co-authored-by: Cursor <cursoragent@cursor.com>
Build device-api CI with ENABLE_DEVICE_API + ENABLE_ROCSHMEM_GIN via cmake, dedupe rocshmem RDC link blocks, move GPU AllGather gtest to gpu-only label, add AllGather policy header missing STATUS, and link rocSHMEM into FixturesDebug for GIN unit tests. Co-authored-by: Cursor <cursoragent@cursor.com>
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
There was a problem hiding this comment.
ARGUS
Findings
3 finding(s) that fall outside the diff and could not be anchored to a line
projects/rccl-tests/src/CMakeLists.txt:193(Low) The dedupe stops one target short:broadcast_perfkeeps a verbatim copy of the extracted helper bodyprojects/rccl/tools/ci/run-device-api-ci.sh:19(Low) The script's own Environment overrides block does not document the two new inputs this PR addsprojects/rccl/tools/ci/lib/device-api.sbatch:66(Nit) Step banners 1 and 2 still say /5 after the pipeline was renumbered to six steps
This PR changes 1,209 lines across 8 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.
Bottom line: Worth settling before approval: from gin_sdma_harness import at test_AllGather.py:31 breaks collection for all three modules on any cwd but test/, and the device-api make list omits five binaries the matrix runs.
Comment @argus review to re-run, or @argus re-review after pushing.
ENABLE_ROCSHMEM_GIN was defaulting NCCL_GIN_ANVIL_SDMA_ENABLE to 1, so non-GIN fat objects pulled rocSHMEM anvil_device.hpp and failed under RCCL_POISON_HIP_ATOMICS. Keep SDMA opt-in on GIN kernel TUs and rccl-tests RDC binaries. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the full ARGUS review in The three unanchored findings are also fixed:
Validation:
I kept this as one AICOMRCCL-2221 PR because the shared harness, its collective adopters, and the device-api build/matrix coverage form one CI enablement path; unrelated conn-check and teardown work was already removed during the rebuild. @argus re-review |
There was a problem hiding this comment.
ARGUS
This PR changes 1,353 lines across 12 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.
Bottom line: Worth settling before approval: the device-api matrix now runs six perf binaries that do not link librocshmem against a librccl.so built --rocshmem-gin, which leaves its rocSHMEM symbols to the executable.
Comment @argus review to re-run, or @argus re-review after pushing.
| && MPI_PATH="${MPI_HOME}" ./install.sh -lj "${build_jobs}" \ | ||
| --amdgpu_targets="${amdgpu_targets}" ) | ||
| --amdgpu_targets="${amdgpu_targets}" \ | ||
| --rocshmem-gin --no_clean \ |
There was a problem hiding this comment.
Building RCCL with --rocshmem-gin here leaves the GIN rocSHMEM host symbols unresolved in librccl.so, to be "resolved at runtime from the executable, which links librocshmem.a" (projects/rccl/src/gin/README.md:34-40). Only three rccl-tests binaries do that, and the matrix runs five that do not:
alltoall_perf whole-archive rocshmem rccl-tests/src/CMakeLists.txt:161
all_gather_perf whole-archive rocshmem rccl-tests/src/CMakeLists.txt:168
broadcast_perf whole-archive rocshmem rccl-tests/src/CMakeLists.txt:193
all_reduce_perf none device-api-tests.json:30, :35
reduce_perf none device-api-tests.json:25
sendrecv_perf none device-api-tests.json:25
scatter_perf none device-api-tests.json:25
gather_perf none device-api-tests.json:25
The sibling GIN job only ever launches the three linked binaries. Could the non-GIN suites run against a non-GIN librccl, or the RDC link cover every binary the matrix names?
There was a problem hiding this comment.
Taken the RDC option in f822f0261aa. _rccl_tests_link_rocshmem_gin_rdc now covers every binary the matrix launches: broadcast_perf, reduce_perf, sendrecv_perf, scatter_perf and gather_perf join alltoall_perf and all_gather_perf, so each executable whole-archive links librocshmem.a and re-exports it with -rdynamic for the --rocshmem-gin librccl.so to resolve at runtime.
all_reduce_perf is the one exception, for the reason in your required_perf_bins thread: it still does not build under ENABLE_ROCSHMEM_GIN. Rather than RDC-link a target that does not link, I dropped the lsa-d1 / lsa-d2 suites that launched it. The matrix is now symmetric, gin-d3, gin-d3-allgather, gin-d4, and every bin in it is RDC-linked, so no run loads that library without being able to supply its rocSHMEM symbols. device-api-tests.json carries a note to re-add the LSA suites when allreduce builds.
There was a problem hiding this comment.
Resolved by f822f0261a.
| # explicit 0 on non-stub tests so ENABLE_ROCSHMEM_GIN cannot leak them back in. | ||
| if(ENABLE_ROCSHMEM_GIN AND NOT "${test_executable}" IN_LIST RCCL_ANVIL_SDMA_STUB_TARGETS) | ||
| target_compile_definitions(${test_executable} PRIVATE NCCL_GIN_ANVIL_SDMA_ENABLE=0) | ||
| endif() |
There was a problem hiding this comment.
RCCL_ANVIL_SDMA_STUB_TARGETS receives NCCL_GIN_ANVIL_SDMA_ENABLE=1 from nothing after this flip: line 960 only exempts those two targets from the explicit =0, and the header default they inherited is now 0 (src/include/nccl_device/gin/gin_device_common.h:57). device/GinAnvilSdmaTemplate_test.cpp and device/GinAnvilIpcDevice_test.cpp keep 17 TEST_Fs behind #if NCCL_GIN_ANVIL_SDMA_ENABLE, so rccl-gin-fixtures-anvil-gda (tools/ci/lib/gin-tests.json:69) still reports pass on its GinRocshmemGdaTemplateTest subset while those 17 vanish from the count. The new comment at line 1092 states the opposite.
| endif() | |
| if(ENABLE_ROCSHMEM_GIN) | |
| if("${test_executable}" IN_LIST RCCL_ANVIL_SDMA_STUB_TARGETS) | |
| target_compile_definitions(${test_executable} PRIVATE NCCL_GIN_ANVIL_SDMA_ENABLE=1) | |
| else() | |
| target_compile_definitions(${test_executable} PRIVATE NCCL_GIN_ANVIL_SDMA_ENABLE=0) | |
| endif() | |
| endif() |
There was a problem hiding this comment.
Correct, and not fixed in f822f0261aa - flagging it as still open rather than letting it look handled.
With the header default now 0, exempting RCCL_ANVIL_SDMA_STUB_TARGETS from the explicit =0 leaves them at 0 too, so the 17 TEST_Fs behind #if NCCL_GIN_ANVIL_SDMA_ENABLE in device/GinAnvilSdmaTemplate_test.cpp and device/GinAnvilIpcDevice_test.cpp compile out while rccl-gin-fixtures-anvil-gda still reports pass on its remaining GinRocshmemGdaTemplateTest subset. The comment at line 1092 states the opposite and is wrong as written.
Your suggested shape (explicit =1 for the stub targets, =0 otherwise) is what I intend to apply. I am holding it for one pass together with the gin_device_common.h:57 thread and the GIN_ANVIL_UNIT_TESTS thread, because all three are the same define-wiring question as the device-api build failure on this PR (attempt to use a poisoned identifier in rocshmem/src/sdma/anvil_device.hpp on gin_all_reduce_sdma.o / gin_alltoall_sdma.o). Fixing this site alone risks contradicting whatever the compile-isolation fix needs.
There was a problem hiding this comment.
Fixed in 97377b539e8. The ENABLE_ROCSHMEM_GIN branch now assigns NCCL_GIN_ANVIL_SDMA_ENABLE=1 explicitly to targets in RCCL_ANVIL_SDMA_STUB_TARGETS and =0 to every other test executable. The 17 guarded tests in GinAnvilSdmaTemplate_test.cpp / GinAnvilIpcDevice_test.cpp therefore compile back in for rccl-UnitTestsFixtures, and rccl-UnitTestsGinAnvilPlugin matches the Suite H comment again. The existing BEFORE include paths still ensure both targets consume the no-op test stubs rather than rocSHMEM device implementation headers.
There was a problem hiding this comment.
Resolved by 97377b539e.
| all_gather_perf all_reduce_perf alltoall_perf broadcast_perf | ||
| gather_perf reduce_perf scatter_perf sendrecv_perf | ||
| ) | ||
| for perf_bin in "${required_perf_bins[@]}"; do |
There was a problem hiding this comment.
required_perf_bins hard-requires all_reduce_perf, which lib/gin.sbatch:170-171 says does not build in this configuration: "allreduce fails to build; -k keeps going so alltoall_perf still lands". The cmake flags and the make -k || true above match gin.sbatch:182-189 exactly, so if that still holds the job exits 1 at line 163 before any benchmark runs. Could this list narrow to the binaries the matrix launches, or has the allreduce build been fixed since?
There was a problem hiding this comment.
Confirmed - "allreduce does not build under GIN" is still true, so this would have failed the job at line 163 before any benchmark ran.
Fixed in f822f0261aa:
required_perf_binsdropsall_reduce_perfand lists only the seven binaries the matrix launches.run-device-api-ci.sh:89stops usingall_reduce_perfas its perf-dir sentinel and checksalltoall_perf, the binary this job always needs (and the onegin.sbatch:191guards).- The
lsa-d1/lsa-d2suites that launched it are removed, so nothing in the matrix references it. - The
make -kline now records why it is-k, instead of leaving the reason only ingin.sbatch.
There was a problem hiding this comment.
Resolved by f822f0261a.
| // defaults to 1, every DeviceLinker TU that includes gin__funcs.h pulls | ||
| // rocSHMEM anvil_device.hpp and hits RCCL_POISON_HIP_ATOMICS. GIN-SDMA | ||
| // kernels pass -DNCCL_GIN_ANVIL_SDMA_ENABLE=1 on their own TUs. | ||
| #define NCCL_GIN_ANVIL_SDMA_ENABLE 0 |
There was a problem hiding this comment.
The per-TU opt-in that compensates this flip does not reach gin_alltoall_sdma.cu and gin_all_reduce_sdma.cu in an ENABLE_DEVICE_LINKER=OFF build. projects/rccl/src/CMakeLists.txt:807 only diverts those two to cmake/DeviceLinker.cmake:869 and :903, the only places the =1 is set, inside the device-linker branch; with it off they compile at 0, so kSdmaDeviceBackendCompiled goes false (gin_alltoall_sdma.cu:35) and ncclAllToAllGinSdmaEligible and ginAllReduceBaseEligible return false for every call with no diagnostic. Could the two files carry the define through set_source_files_properties so its value no longer depends on which pipeline compiled them?
There was a problem hiding this comment.
Still open in f822f0261aa, not fixed.
The analysis holds: projects/rccl/src/CMakeLists.txt:807 only diverts gin_alltoall_sdma.cu and gin_all_reduce_sdma.cu into the device-linker branch, and cmake/DeviceLinker.cmake:869 / :903 are the only places the =1 is set. With ENABLE_DEVICE_LINKER=OFF both compile at 0, kSdmaDeviceBackendCompiled goes false (gin_alltoall_sdma.cu:35), and ncclAllToAllGinSdmaEligible / ginAllReduceBaseEligible return false for every call with no diagnostic.
Carrying the define on the two files via set_source_files_properties is the right fix - its value stops depending on which pipeline compiled them. I am deliberately not doing it in isolation: the device-api job on this PR is currently red with the poisoned-identifier failure on exactly these two objects, so the compile isolation and the define wiring need to be settled in the same pass rather than in two steps that could contradict each other. Next push.
There was a problem hiding this comment.
Fixed in 97377b539e8. src/CMakeLists.txt now puts NCCL_GIN_ANVIL_SDMA_ENABLE=1, NCCL_GIN_PROXY_ENABLE=0, and NCCL_GIN_ROCSHMEM_GDA_ENABLE=0 on both GIN-SDMA source files with set_source_files_properties, so an ENABLE_DEVICE_LINKER=OFF build compiles the backend and no longer silently makes both eligibility predicates false. The definitions live in one RCCL_GIN_SDMA_COMPILE_DEFINITIONS list; DeviceLinker.cmake converts that same list to -D flags for its filtered standalone objects, avoiding pipeline drift.
This also fixes the gfx950 poisoned-identifier build failure without turning the RCCL poison off: poison_hip_atomics.h parses external rocSHMEM sdma/anvil_device.hpp before activating the RCCL-only poison, then the include guard prevents reparsing. 097f4da38c3 proves the external header can use __hip_atomic_load; c0b537c3fd7 proves a direct RCCL use after that header is still rejected.
There was a problem hiding this comment.
Resolved by a later commit.
| @@ -1039,7 +1061,8 @@ if(BUILD_TESTS) | |||
| # into the Fixtures binary so the executable satisfies those symbols when | |||
| # the dynamic loader brings in librccl.so. | |||
| if(ENABLE_ROCSHMEM_GIN AND GIN_ANVIL_UNIT_TESTS AND NOT ENABLE_ROCSHMEM | |||
There was a problem hiding this comment.
The GIN_ANVIL_UNIT_TESTS term keeps the new rccl-UnitTestsFixturesDebug case from ever firing. FixturesDebug is only created in a Debug build (line 614), projects/rccl/install.sh has no flag for that option, and the one in-tree Debug plus --rocshmem-gin build, tools/scripts/test_runner/configs/gin_rocshmem_backends_mellanox.json:17-26, does not set it yet runs the binary at :219. Could the FixturesDebug case drop that term, given the undefined-symbol problem this block solves does not depend on it?
There was a problem hiding this comment.
Still open in f822f0261aa.
Agreed on the reasoning: rccl-UnitTestsFixturesDebug only exists in a Debug build (line 614), projects/rccl/install.sh exposes no flag for GIN_ANVIL_UNIT_TESTS, and the one in-tree Debug + --rocshmem-gin configuration (tools/scripts/test_runner/configs/gin_rocshmem_backends_mellanox.json:17-26) does not set it yet runs the binary at :219. So the rocSHMEM link this PR added never fires, and the undefined-symbol hole it exists to close stays open. The term is not what makes the link necessary, so it should come off the FixturesDebug case.
Batched with the other two Anvil-define threads for the next push.
There was a problem hiding this comment.
Fixed in 97377b539e8. The host rocSHMEM link condition now distinguishes the two binaries: rccl-UnitTestsFixtures retains its GIN_ANVIL_UNIT_TESTS gate, while rccl-UnitTestsFixturesDebug links whenever this is an ENABLE_ROCSHMEM_GIN && !ENABLE_ROCSHMEM build with ROCSHMEM_INSTALL_DIR. That matches the in-tree Debug + --rocshmem-gin configuration even though install.sh has no GIN_ANVIL_UNIT_TESTS flag, and closes the undefined-symbol path independently of which optional fixture sources were selected.
There was a problem hiding this comment.
Resolved by 97377b539e.
| args = ( | ||
| mpi_launch_prefix(request, launcher, np, mpi_opts) | ||
| + gin_env_xflags(gin_env) | ||
| + [ |
There was a problem hiding this comment.
The perf-arg vector is still written out three times, here and at test_AllGather.py:167-193 and test_AllToAll.py:117-141, alongside three near-identical hang messages. A shared gin_perf_argv(exe, size, dtype, ctas, extra) next to gin_env_xflags would take roughly 60 lines out, and would make the -A 1 gap either deliberate or visible: AllGather and Broadcast pass it, AllToAll does not.
There was a problem hiding this comment.
Extracted in f822f0261aa. gin_perf_argv(exe, size, dtype, ctas, extra) now sits next to gin_env_xflags, and extra is spliced between -D 3 and -V, which is exactly where the collectives diverge: AllGather and Broadcast pass extra=["-A", "1"], AllToAll passes nothing. gin_hang_msg(collective, timeout_s, size, dtype, size_desc) absorbs the three near-identical hang templates.
AllGather's -B 1 stays appended after the vector rather than going through extra, since it is a trailing devtime flag rather than part of the shared shape.
On making the gap "deliberate or visible": test_BroadcastGinSdmaPerfArgvMakesAverageCountGapExplicit asserts that the AllGather argv carries -A 1 between -D and -V and the AllToAll argv does not, so the difference is now pinned rather than inferred from three copies.
There was a problem hiding this comment.
Resolved by f822f0261a.
| return len(os.environ["HIP_VISIBLE_DEVICES"].split(",")) | ||
| try: | ||
| out = subprocess.check_output( | ||
| 'rocminfo | grep "Device Type:.\\s*.GPU" | wc -l', shell=True |
There was a problem hiding this comment.
detect_ngpus cannot report a failed probe: the pipeline ends in wc -l, so a missing rocminfo still exits 0 with "0" and this returns 0, which the new max(1, ...) at test_AllGather.py:42 and test_Broadcast.py:44 turns into one GPU. A host that has GPUs but no rocminfo on PATH then silently collects a quarter of the matrix and skips every multi-rank case instead of erroring at import. CI sets the rank count explicitly (projects/rccl/tools/ci/run-gin-ci.sh:247), so this only bites manual runs. Would a shutil.which check keep the clamp's purpose without hiding a broken host?
There was a problem hiding this comment.
Fixed in f822f0261aa. detect_ngpus() resolves rocminfo with shutil.which and raises RuntimeError when it is absent, instead of returning 0 from a pipeline that exits 0 regardless. The count now comes from parsing rocminfo output directly rather than grep ... | wc -l.
To keep the clamp's original purpose without hiding a broken host: the module-level max(1, ...) callers in test_AllGather.py and test_Broadcast.py catch that RuntimeError at import, so the GPU-free guards still collect on a machine with no ROCm at all, while the opt-in GIN paths (AG_NP, BCAST_NP, NP) call detect_ngpus() for real and surface the error instead of silently running a one-GPU matrix. test_BroadcastGinSdmaDetectNgpusRequiresRocminfo pins the raise.
There was a problem hiding this comment.
Resolved by f822f0261a.
| ) | ||
|
|
||
|
|
||
| def detect_ngpus(): |
There was a problem hiding this comment.
test_AllToAllDevtime.py is a fourth copy of this harness that did not move: _detect_ngpus at :65, and :112-127 and :151-164 re-implement gin_env_xflags, mpi_launch_prefix and launch_mpi_shell. It is also the only one of the four that device-api CI runs (projects/rccl/tools/ci/run-device-api-ci.sh:215). That file is outside this diff, so this is a follow-up note rather than something to change here.
There was a problem hiding this comment.
Agreed, and thanks for scoping it as a follow-up rather than a change here.
test_AllToAllDevtime.py being the one of the four that device-api CI actually runs (run-device-api-ci.sh:215) is the reason I would rather move it on its own: porting it onto gin_sdma_harness changes the only pytest this job executes, and that deserves a diff where the device-api run is the thing under review, not a rider on an already-large change.
Recorded on AICOMRCCL-2221 so it does not live only in this thread.
| ) | ||
| # Deliberately not end-anchored: rccl-tests may append an algo/proto/nchannels | ||
| # group and a timestamp, and concurrent rank output can splice onto the line. | ||
| BCAST_OOB_RE = re.compile(r"Out of bounds values\s*:\s*(\d+)") |
There was a problem hiding this comment.
| BCAST_OOB_RE = re.compile(r"Out of bounds values\s*:\s*(\d+)") | |
| # Deliberately not end-anchored: rccl-tests may append an algo/proto/nchannels | |
| # group and a timestamp, and concurrent rank output can splice onto the line. | |
| BCAST_ROW_RE = re.compile( | |
| r"^\s*(\d+)\s+(\d+)\s+\S+\s+\S+\s+(\d+)" | |
| r"\s+{n}\s+{n}\s+{n}\s+({w})" | |
| r"\s+{n}\s+{n}\s+{n}\s+({w})".format(n=_NUM, w=_WRONG) | |
| ) | |
| BCAST_OOB_RE = re.compile(r"Out of bounds values\s*:\s*(\d+)") |
There was a problem hiding this comment.
Applied in f822f0261aa. The rationale had been restored in 6c1e471a596 but landed below the regex, where it read as describing BCAST_OOB_RE; it now sits directly above BCAST_ROW_RE as in your suggestion. The BCAST_TIER_RE pointer to bcastReportTier() in src/broadcast.cu is in place just above that regex.
There was a problem hiding this comment.
Resolved by f822f0261a.
| _CONN_GATE_RE = re.compile(r"LSA signal connectivity gate failed|unhandled system error", re.I) | ||
| _DATA_FAIL_RE = re.compile( | ||
| r"#wrong\s*=\s*[1-9]|mismatch|check.*fail|Out of bounds values\s*:\s*[1-9]", re.I) | ||
| # Intermittent gfx950 cuMem-VMM connectivity-gate abort: see gin_sdma_harness.py. |
There was a problem hiding this comment.
| # Intermittent gfx950 cuMem-VMM connectivity-gate abort: see gin_sdma_harness.py. |
There was a problem hiding this comment.
Applied in f822f0261aa. That line was a leftover from the extraction: it sat above _AG_DEVTIME_TIER_RE, which matches the devtime tier line and has nothing to do with the connectivity gate, so it pointed a reader at the wrong thing. The gate rationale lives in gin_sdma_harness.py next to CONN_GATE_RE. Removed.
There was a problem hiding this comment.
Resolved by f822f0261a.
Device-api CI builds one --rocshmem-gin librccl.so, which leaves GIN rocSHMEM host symbols to the executable. Every binary the matrix launches now whole-archive RDC-links rocSHMEM, not only the GIN collectives. all_reduce_perf still does not build under ENABLE_ROCSHMEM_GIN (same as gin.sbatch), so it is not RDC-linked, not required after `make -k`, and the LSA -D 1/-D 2 suites that launched it are dropped until it builds. The runner's perf-dir sentinel moves to alltoall_perf. Harness: extract gin_perf_argv()/gin_hang_msg() so the three collectives share one argv vector and the AllToAll -A 1 gap is explicit; make detect_ngpus() fail when rocminfo is missing instead of reporting zero GPUs; pin CONN_GATE_RE with a non-gate failure case. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pushed AddressedDevice-api matrix vs a
Harness. Validation: 17 offline guards pass, all three modules collect from both the rccl-tests root and Knowingly still openThree threads are not fixed in this push and I said so on each one rather than letting them look handled:
All three are the same define-wiring question as the device-api job failure on this PR: the RCCL DeviceLinker build dies with The @argus re-review - this round is a check on the RDC link change and the harness dedupe; the three Anvil-define threads above are expected to still be open. |
Parse rocSHMEM's external anvil_device.hpp before RCCL poisons the __hip_atomic_* builtins, while retaining the poison for all following RCCL code. Share the GIN-SDMA backend definitions between regular and DeviceLinker compilation so both pipelines opt in consistently. Restore NCCL_GIN_ANVIL_SDMA_ENABLE=1 on stub-backed tests and let Debug fixtures link rocSHMEM without requiring GIN_ANVIL_UNIT_TESTS. Co-authored-by: Cursor <cursoragent@cursor.com>
Compile a guarded stand-in for rocSHMEM's anvil_device.hpp with a __hip_atomic_load call and verify that the explicitly opted-in external header is parsed before RCCL activates its atomics poison. Co-authored-by: Cursor <cursoragent@cursor.com>
After parsing the external Anvil stub, spell __hip_atomic_load directly in the RCCL probe and require the compiler to reject it. This ensures the external header exception does not disable the poison for following RCCL code. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pushed the remaining Anvil define/compile-isolation work in
Local validation: poison-header text guards and Python syntax pass; the HIP compile probes are present but skipped locally because this checkout has no HIP compiler. The PR host-test job has the required compiler and will exercise them. @argus re-review |
There was a problem hiding this comment.
ARGUS
This PR changes 1,557 lines across 16 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.
Bottom line: Worth settling before approval: device-api.sbatch:130 globs an install tree install.sh never produces here, so the job aborts silently before any benchmark runs.
Comment @argus review to re-run, or @argus re-review after pushing.
| --cmake-options "-DROCSHMEM_INSTALL_DIR=${ROCSHMEM_INSTALL_DIR}" | ||
| ) | ||
| rccl_prefix="${workdir}/projects/rccl/build/release" | ||
| rccl_cfg="$(ls "${rccl_prefix}"/lib*/cmake/rccl/*[Cc]onfig.cmake 2>/dev/null | head -n1)" |
There was a problem hiding this comment.
rccl_cfg globs an install tree this job never produces: install.sh is invoked at :124-127 without -i or --prefix, so no install target runs and rocm_export_targets (projects/rccl/CMakeLists.txt:839) never writes lib/cmake/rccl/*config.cmake. Under set -euo pipefail the failing ls propagates through the pipeline and fails the assignment itself, so the job dies here and the [[ -z ]] diagnostic below never prints. lib/gin.sbatch:161 runs the identical glob only because gin.sbatch:126 passes --prefix=. Could this call do the same?
| ) | ||
| out = subprocess.check_output( | ||
| [rocminfo], universal_newlines=True, stderr=subprocess.DEVNULL | ||
| ) |
There was a problem hiding this comment.
detect_ngpus raises subprocess.CalledProcessError when rocminfo exits non-zero, but the import guards at test_AllGather.py:46 and test_Broadcast.py:47 catch RuntimeError only. On a host with ROCm on PATH and no /dev/kfd both modules then error during collection, taking the GPU-free guards with them. The old | wc -l spelling ended in a command that always exited 0, so this is new.
| ) | |
| try: | |
| out = subprocess.check_output( | |
| [rocminfo], universal_newlines=True, stderr=subprocess.DEVNULL | |
| ) | |
| except (OSError, subprocess.SubprocessError) as exc: | |
| raise RuntimeError( | |
| "rocminfo failed; cannot detect GPU count. Set " | |
| "ROCR_VISIBLE_DEVICES, HIP_VISIBLE_DEVICES, or RCCL_TESTS_*_NP." | |
| ) from exc |
| return 1, "#wrong = 1" | ||
|
|
||
| assert run_with_conn_gate_retry(launch, 5, settle_s=0) == (1, "#wrong = 1") | ||
| assert len(calls) == 1 |
There was a problem hiding this comment.
test_BroadcastGinSdmaConnRetryNeverRetriesDataFailure passes with the is_data_failure branch at gin_sdma_harness.py:191-192 deleted: #wrong = 1 matches neither CONN_GATE_RE nor the retry condition, so :200 returns the same tuple after the same one call. That makes it identical to test_BroadcastGinSdmaConnRetryDoesNotRetryNonGateError. Output matching both patterns discriminates: one call with the branch, five without.
| assert len(calls) == 1 | |
| return 1, "unhandled system error: #wrong = 1" | |
| assert run_with_conn_gate_retry(launch, 5, settle_s=0) == ( | |
| 1, | |
| "unhandled system error: #wrong = 1", | |
| ) | |
| assert len(calls) == 1 |
| # and these are the only two test targets that define it. | ||
| # These two targets opt into NCCL_GIN_ANVIL_SDMA_ENABLE=1; other tests keep it at 0. | ||
| if(ENABLE_ROCSHMEM_GIN) | ||
| if(ROCSHMEM_SOURCE_DIR) |
There was a problem hiding this comment.
This ROCSHMEM_SOURCE_DIR ladder is dead after the new helper: _rccl_test_add_rocshmem_includes(rccl-UnitTestsFixtures) at :448 fires under ENABLE_ROCSHMEM_GIN OR GIN_ANVIL_UNIT_TESTS, a superset of :1098, and its :428-436 arm already adds the same src path on both the ROCSHMEM_SOURCE_DIR and the ../rocshmem branches. Could :1099-1103 go, leaving Suite H doing only the BEFORE stub shadowing its comment describes?
| target_include_directories(rccl-UnitTestsFixtures PRIVATE ${ROCSHMEM_BUILD_DIR}) | ||
| list(APPEND _rocshmem_test_includes "${ROCSHMEM_BUILD_DIR}") | ||
| elseif(ROCSHMEM_SOURCE_DIR AND EXISTS "${ROCSHMEM_SOURCE_DIR}/build/include") | ||
| list(APPEND _rocshmem_test_includes "${ROCSHMEM_SOURCE_DIR}/build/include") |
There was a problem hiding this comment.
This elseif still keys on ROCSHMEM_SOURCE_DIR, which add_rocshmem_targets() sets PARENT_SCOPE into src/ only and which test/CMakeLists.txt:70-81 deliberately does not re-derive, so the ../rocshmem arm you just added at :432-435 never gets a matching build/include. Nothing breaks today because arm 1 supplies rocshmem_config.h from ROCSHMEM_INCLUDE_DIR. Could the fallback be symmetric?
| list(APPEND _rocshmem_test_includes "${ROCSHMEM_SOURCE_DIR}/build/include") | |
| elseif(EXISTS "${ROCSHMEM_SOURCE_DIR}/build/include") | |
| list(APPEND _rocshmem_test_includes "${ROCSHMEM_SOURCE_DIR}/build/include") | |
| elseif(EXISTS "${CMAKE_SOURCE_DIR}/../rocshmem/build/include") | |
| list(APPEND _rocshmem_test_includes "${CMAKE_SOURCE_DIR}/../rocshmem/build/include") |
| # GIN-SDMA hybrid AllGather (all_gather_perf -D 3) needs ENABLE_DEVICE_API | ||
| # so the GinHybridAllGatherKernel is compiled into this binary. | ||
| if (ENABLE_DEVICE_API) | ||
| target_compile_definitions(all_gather_perf PRIVATE ENABLE_DEVICE_API) |
There was a problem hiding this comment.
| target_compile_definitions(all_gather_perf PRIVATE ENABLE_DEVICE_API) | |
| target_compile_definitions(all_gather_perf PRIVATE ENABLE_DEVICE_API) |
|
Splitting this PR so the pieces can be reviewed separately. Superseded by:
|
## Summary - Split out of #11559 (2/3). Independent of the poison and device-api PRs. - AllGather, AllToAll, and Broadcast share `gin_sdma_harness.py` (MPI launcher, connectivity-gate retry, Broadcast debug-file capture). - `detect_ngpus()` raises `RuntimeError` when `rocminfo` is missing or fails, so GPU-free collection still works on a host with ROCm on PATH and no `/dev/kfd`. - The data-failure retry guard uses output that matches both the connectivity gate and `#wrong`, so deleting the data-failure branch fails the test. `test_AllToAllDevtime.py` is intentionally not ported here. It is the pytest device-api CI runs, and that move is a follow-up on AICOMRCCL-2221. Same commit as #11894 (`17d7a8680e`). This pull request is opened from `ROCm/rocm-systems` so the RCCL coco PR workflow can run on the `pull_request` event. JIRA ID : AICOMRCCL-2221 ## Test plan - [x] Offline guards: connectivity retry, non-gate error, data failure that also matches the gate - [x] `pytest projects/rccl-tests/test -k GinSdma` collects from the rccl-tests root and from `test/` (38 tests, reported on #11894) - [ ] RCCL coco PR: Ruby and OCI smoke on this pull request Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
gin_sdma_harness.pyfor GIN-SDMA pytest harnesses (AllGather, AllToAll, Broadcast): MPI launcher assembly, connectivity-gate retry, and Broadcast NCCL debug-file capture.ENABLE_DEVICE_API+ENABLE_ROCSHMEM_GINvia cmake (.ci-out/rccl-tests-build) soall_gather_perf -D 3links rocshmem RDC.rccl-tests/src/CMakeLists.txt.rccl-UnitTestsGinSdmaAllGatherGputogpu-only ctest label; add STATUS when AllGather policy header is absent.rccl-UnitTestsFixturesDebugfor GIN Debug unit-test builds.NCCL_GIN_ANVIL_SDMA_ENABLEfromENABLE_ROCSHMEM_GIN. Anvil-SDMA device headers are opt-in per TU so DeviceLinker DDA/symmetric objects do not pullanvil_device.hpp(poisoned HIP atomics on gfx950). GIN kernel TUs and rccl-tests RDC binaries still pass-DNCCL_GIN_ANVIL_SDMA_ENABLE=1.Follow-up to merged #10930 / AICOMRCCL-1798. Conn-check device-link restore remains on #11092 (AICOMRCCL-2217); not included here.
JIRA ID : AICOMRCCL-2221
Test plan
all_gather_perf -D 3with rocshmem RDC linkctest -L unitexcludes GPU-only GIN AllGather testpytest test_Broadcast.py -k GinSdma(no GPU)NCCL_GIN_TYPE=6)Made with Cursor