Repository navigation
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: the NCCL_GIN_ANVIL_SDMA_ENABLE default flip removes the SDMA dispatch arm from librccl's symmetric GIN kernels, and the new gin-d3-allgather suite sets NCCL_GIN_TYPE=6, which is rocSHMEM GDA rather than Anvil SDMA.
Comment @argus review to re-run, or @argus re-review after pushing.
The device-api job compiled RCCL with --rocshmem-gin but never installed it, so the cmake config glob died before any benchmark, and rccl-tests did not RDC-link the binaries that load that library. Co-authored-by: Cursor <cursoragent@cursor.com>
367e1f0 to
13e642c
Compare
There was a problem hiding this comment.
ARGUS
Findings
1 finding(s) that could not be posted as inline comments
projects/rccl/tools/ci/lib/device-api.sbatch:7(Low) SLURM --time=02:00:00 is unchanged although the build phase gained a rocSHMEM build and seven RDC device links device-api.sbatch:7 still requests--time=02:00:00, while this PR adds a full rocSHMEM build, switches RCCL to--rocshmem-gin, and replaces the Makefile rccl-tests build with a cmake build doing seven-fgpu-rdcdevice links.gin.sbatch:7allots04:00:00for what is now substantially the same build phase, and a SLURM timeout would land as a confusing mid-build kill. Please check.
Bottom line: Worth settling before approval: gin-d3-allgather still sets NCCL_GIN_TYPE=6, which selects rocSHMEM GDA rather than the Anvil SDMA path this PR builds and links for.
Comment @argus review to re-run, or @argus re-review after pushing.
GIN type 6 selects rocSHMEM GDA, so gin-d3-allgather never reached the SDMA kernel this job builds. Point it at type 7 and keep the longer rocSHMEM build from dying on a 2h SLURM limit or a missing rccl.env. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
ARGUS
Findings
1 finding(s) that could not be posted as inline comments
projects/rccl/tools/ci/lib/device-api-tests.json:5(Low) _comment line 5 still documents the launch as ./build/<bin> after PERF_DIR became absolute The_commentblock inlib/device-api-tests.jsonstill documents the launch as./build/<bin>on line 5, butrun-device-api-ci.sh:164now runs"${PERF_DIR}/${bin}", an absolute path taken fromRCCL_TESTS_BIN_DIR(.ci-out/rccl-tests-buildfor this job) or${RCCL_TESTS_DIR}/build. Worth updating so the matrix doc matches what the runner does.
Bottom line: Looks approval ready. The one open question is whether losing CI coverage of the device-API-without-rocSHMEM build is intended.
Comment @argus review to re-run, or @argus re-review after pushing.
…221) Restore the ENABLE_DEVICE_API install.sh build and LSA matrix so this job still covers device-API-without-rocSHMEM, and match gin.sbatch's build-log handling for the remaining Argus nits. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@argus re-review Pushed 3104488: restore non-GIN Branch is also on |
|
Correction on the device-api validation dispatch: https://github.com/ROCm/rocm-systems/actions/runs/36286083204 That run includes the device-API leg (plus gin / ext-plugins). Will update the PR test-plan boxes from the device-API job result. |
…-2221) Keep the ENABLE_DEVICE_API install.sh build for the symmetric matrix, and do not run or require all_reduce_perf -D 1/-D 2 in this job. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Follow-up in 7c6cc22: non-GIN LSA is out of this job. Cancelled the cluster CI run on 3104488 (it still included LSA) and redispatched RCCL cluster CI from this SHA. |
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: the non-GIN matrix runs binaries that cannot resolve the --rocshmem-gin librccl.so's rocSHMEM host symbols, and a failing first matrix is swallowed by the || test_rc=$? at device-api.sbatch:267.
Comment @argus review to re-run, or @argus re-review after pushing.
| ( | ||
| cd "${rccl_tests_src}" \ | ||
| && ./install.sh --mpi --enable-device-api \ | ||
| --mpi_home="${MPI_HOME}" --rccl_home="${rccl_prefix}" \ |
There was a problem hiding this comment.
High
The step 5 build takes --rccl_home=${rccl_prefix}, which is the --rocshmem-gin librccl.so, but the Makefile path links no librocshmem.a and passes no -rdynamic (rccl-tests/src/Makefile:225), so that library's undefined rocSHMEM host symbols have nothing to bind to. rccl/test/CMakeLists.txt:1083-1087 records this exact case. Could this pass build against its own non-GIN RCCL install? Please check.
There was a problem hiding this comment.
Thanks for the flag — checked against the green device-API job on 7c6cc22:
https://github.com/ROCm/rocm-systems/actions/runs/36286627742/job/108528513362
Step 5's install.sh binaries load the --rocshmem-gin librccl.so and complete the full non-GIN symmetric matrix (broadcast_perf, reduce_perf, sendrecv_perf, scatter_perf, gather_perf) with # Out of bounds values : 0 OK. The unresolved-host-symbol case in projects/rccl/test/CMakeLists.txt:1083-1087 is for a unit-test binary that does not get the GIN host objects that --rocshmem-gin packages into librccl.so (via librocshmem_gin_host.a in that same job's RCCL build). Leaving the shared GIN install for this matrix; a second non-GIN RCCL tree would add another full library build to a job already at the 4h wall.
| CONFIG="${ci_dir}/lib/device-api-tests.json" \ | ||
| RCCL_TESTS_BIN_DIR="${RCCL_TESTS_BIN_DIR}" \ | ||
| "${ci_dir}/run-device-api-ci.sh" | ||
| } > "${test_out}" 2> "${test_err}" || test_rc=$? |
There was a problem hiding this comment.
High
test_rc records only the second run-device-api-ci.sh. Bash ignores set -e inside a compound command on the left of ||, so a failing non-GIN matrix neither aborts the group nor is captured, and the group's status is the GIN run's alone. Could each invocation take its own || rc=$??
There was a problem hiding this comment.
Addressed in 2b6f1bf. Each run-device-api-ci.sh now has its own || rc=$? (nongin_rc / gin_rc); the job fails if either is non-zero and reports both statuses. The GIN matrix still runs after a non-GIN failure so both logs land in test-%j.{out,err}.
There was a problem hiding this comment.
Resolved by 2b6f1bf543.
A single || on the test brace group ignored set -e for the non-GIN run, so only the GIN matrix status was retained. Capture each invocation separately. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@argus re-review Pushed 2b6f1bf: each device-api matrix now keeps its own exit status ( Will redispatch RCCL cluster CI from this SHA for the updated exit-status path, and RCCL coco PR for the fork gate. |
|
Dispatched:
Once coco passes, a maintainer still needs to override the red RCCL PR gate check on this fork PR. |
|
Superseded by #12752 and closing in favour of it. Same commits, same content; identical head SHA. Why. This PR was opened from a fork, and the required Note. This branch was already present in Please keep reading this thread. 2 of the 18 review threads here are still unresolved, and inline conversations do not carry across to a new pull request. Argus will re-review #12752 and should re-surface anything still unfixed, but the replies written here exist only on this PR. Closing so this does not keep consuming CI or show up as a duplicate for reviewers; it stays readable and can be reopened. |
> **Replaces #11895.** That PR was opened from a fork, where the required `RCCL PR gate` check can never pass: fork PRs get no secrets, so `RCCL coco PR` skips its `Setup` job and the gate fails closed. This branch lives in `ROCm/rocm-systems`, so the gate dispatches coco to the Ruby and OCI clusters itself. > > Review history from #11895 does not carry over. All 10 review threads on this PR (Argus and @Kapil-Shyam-Pawar) are resolved, and Argus re-reviewed `774335be8d` with LGTM. > > **Rebased onto `develop` on 2026-10-19** (head `41731d7d77`). The only conflict was `device-api-tests.json`, where `develop` added the `gin-d3-nicfusion` suite (#11133). That suite is kept in the GIN matrix. The PR's own diff is unchanged. ## Summary - `device-api.sbatch` installs RCCL with `--prefix`, matching `gin.sbatch`, so `lib/cmake/rccl/*config.cmake` exists. `--rocshmem-gin` builds the sibling rocSHMEM checkout into `projects/rccl/ext/rocshmem`. This job does not call `build-rocshmem.sh`. - The non-GIN matrix (`device-api-nongin-tests.json`, `ENABLE_DEVICE_API=1 ./install.sh` build) runs AllReduce, AllGather and ReduceScatter at `-R 2`, which are the collectives that have symmetric kernels, plus LSA `all_reduce_perf -D 1` and `-D 2`. - The GIN matrix (`device-api-tests.json`, cmake `ENABLE_DEVICE_API` + `ENABLE_ROCSHMEM_GIN` build) is host-proxy alltoall: `gin-d3`, `gin-d4`, and `gin-d3-nicfusion` from `develop`, all with `NCCL_GIN_TYPE=2`. - Anvil SDMA allgather (`NCCL_GIN_TYPE=7`) moves to `gin-tests.json` as `rccl-gin-rocshmem-sdma-ag-d3`, next to the existing SDMA alltoall and broadcast cases. The gin build fails if `all_gather_perf` was not produced, and the gin test results remain non-gating. - rccl-tests' cmake build RDC-links `alltoall_perf`, `all_gather_perf` and `broadcast_perf`. `all_reduce_perf` still does not build in that configuration, so it stays on the non-GIN matrix. - Each matrix records its own exit status, and on test failure the job log shows the runner's `FAILED RUNS` summary from `test_out`. Independent of #11894 (pytest harness). `test_AllToAllDevtime.py` is unchanged. JIRA ID : AICOMRCCL-2221 ## Known failure on ROCm 10.2 nightlies (not caused by this PR) On ROCm 10.2 nightlies (HIP 7.17.26405 to 7.17.26412), communicator teardown in symmetric and device-API runs fails in `ncclCuMemFree`. One rank hits `alloc.h:789`/`:809 HIP failure 'invalid argument'` and another segfaults in `hipMemUnmap`. The `develop` nightly of RCCL cluster CI fails the same way without this PR ([run 38567603219](https://github.com/ROCm/rocm-systems/actions/runs/38567603219), Oct 14). The likely root cause is the HIP VMM import/release regression ROCM-33022. RCCL-side tracking is in **AICOMRCCL-3065**. On this PR it appeared as: - the Ruby coco error on `774335be8d`, where `device-api-symmetric_reduce_perf` exited with rc 139 on `rocm-10.2-latest` only (the 10.1-ga stacks passed); - the `symmetric` and `gin-d4` failures in the Oct 12 device-api run on `91bd83c1f4`. If it recurs, it is this bug, not a regression from this change. This PR touches only CI scripts and the rccl-tests CMake, not RCCL source. ## Test plan RCCL cluster CI on `41731d7d77`: [run 39293555415](https://github.com/ROCm/rocm-systems/actions/runs/39293555415). - [ ] device-api job installs `librccl.so`, finds the RCCL cmake config, and installs rocSHMEM at `projects/rccl/ext/rocshmem` - [ ] non-GIN matrix runs `all_reduce_perf`, `all_gather_perf` and `reduce_scatter_perf` at `-R 2`, plus `all_reduce_perf -D 1` and `-D 2` - [ ] GIN matrix runs `alltoall_perf -D 3` and `-D 4` (`gin-d3`, `gin-d4`, `gin-d3-nicfusion`; `NCCL_GIN_TYPE=2`) - [ ] gin job builds `all_gather_perf` and runs `rccl-gin-rocshmem-sdma-ag-d3` (test result is non-gating) --------- Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
develop. The poison-isolation commit from rccl: isolate GIN-SDMA TUs from HIP atomics poison (AICOMRCCL-2221) [1/3] #11893 is not in this branch. That PR is closed: a gfx950--rocshmem-ginbuild withRCCL_POISON_HIP_ATOMICSforce-included produceslibrccl.soand the two GIN-SDMA device objects with nopoisoned identifier.device-api.sbatchnow installs RCCL with--prefix, matchinggin.sbatch, solib/cmake/rccl/*config.cmakeexists. The old glob aborted underpipefailbefore any benchmark.ENABLE_DEVICE_API+ENABLE_ROCSHMEM_GINand RDC-links every matrix binary.all_reduce_perfis still not required (it does not build in this configuration).Independent of #11894 (pytest harness).
test_AllToAllDevtime.pyis unchanged.JIRA ID : AICOMRCCL-2221
Test plan
librccl.soand finds the RCCL cmake configall_gather_perfare executable under.ci-out/rccl-tests-buildgin-d3-allgather/ AllToAll-D 3actually run