Repository navigation
Conversation
gin.sbatch already builds rccl-UnitTestsGinAnvilPlugin under GIN_ANVIL_UNIT_TESTS=ON; execute it next to the SDMA factory gtest so logical-context bind coverage is exercised in CI (AICOMRCCL-2339). 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. |
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
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/gin.sbatch:23(Low) Header step map still ends step 6 at the SDMA factory gtest and omits the new 6b run The header step map at projects/rccl/tools/ci/lib/gin.sbatch:23 still ends step 6 atrccl-UnitTestsGinAnvilSdmaFactory, so it no longer lists the Anvil plugin gtest that now runs as 6b. A line next to the step 6 entry keeps the header an accurate map of the script.
Bottom line: Worth settling before approval: this binary already runs in step 7 via gin-tests.json, so the gap the PR sets out to close may not exist.
Comment @argus review to re-run, or @argus re-review after pushing.
|
|
||
| # --- unit test: Anvil host plugin gtest (already built in step 4). --- | ||
| # Covers bind-signals / logical-context stripe coverage that install.sh -t skips. | ||
| echo "==> [6b/7] run Anvil plugin unit tests" |
There was a problem hiding this comment.
Medium
Step 7 already runs this binary, so the stated gap does not hold: projects/rccl/tools/ci/lib/gin-tests.json:58 registers rccl-UnitTestsGinAnvilPlugin as rccl-gin-conn-check-plugin-ut, and projects/rccl/tools/ci/run-gin-ci.sh:202 resolves that to the same ${rccl_build}/test path this line uses. That run is also timeout wrapped and feeds the tests-failed sentinel, while this one only echoes to stderr. Could this step be dropped?
| "${anvil_plugin_test}" --gtest_output="xml:${logs_dir}/anvil-plugin-test.xml" \ | ||
| || echo "WARNING: Anvil plugin unit tests failed (non-gating)" >&2 | ||
| else | ||
| echo " SKIP: ${anvil_plugin_test} not found (GIN_ANVIL_UNIT_TESTS may be off)" |
There was a problem hiding this comment.
Low
This SKIP branch cannot be reached: line 227 already exits 1 when the plugin binary is not executable, and line 303 skips the whole test phase whenever the build failed. Its hint is wrong too, since projects/rccl/test/CMakeLists.txt:466 gates this target on ENABLE_ROCSHMEM_GIN and ROCM_VERSION >= 60400, not on GIN_ANVIL_UNIT_TESTS.
| [[ -d "${_sp}" ]] && _sysdeps_lib="${_sp}" | ||
| done | ||
| export LD_LIBRARY_PATH="${RCCL_INSTALL_PREFIX}/lib:${ROCSHMEM_INSTALL_DIR}/lib:${ROCM_PATH}/lib${_sysdeps_lib:+:${_sysdeps_lib}}:${LD_LIBRARY_PATH:-}" | ||
| "${anvil_plugin_test}" --gtest_output="xml:${logs_dir}/anvil-plugin-test.xml" \ |
There was a problem hiding this comment.
Low
This gtest runs with no wall clock cap, unlike every fixtures entry in projects/rccl/tools/ci/run-gin-ci.sh:255, which wraps the same binary in timeout --kill-after. A stall here would burn the job's --time=04:00:00 budget before step 7 ever starts. Could it be wrapped the same way?
| for _sp in "${ROCM_PATH}/lib/rocm_sysdeps/lib" "${ROCM_PATH}/core/lib/rocm_sysdeps/lib"; do | ||
| [[ -d "${_sp}" ]] && _sysdeps_lib="${_sp}" | ||
| done | ||
| export LD_LIBRARY_PATH="${RCCL_INSTALL_PREFIX}/lib:${ROCSHMEM_INSTALL_DIR}/lib:${ROCM_PATH}/lib${_sysdeps_lib:+:${_sysdeps_lib}}:${LD_LIBRARY_PATH:-}" |
There was a problem hiding this comment.
Low
Lines 341-345 are byte identical to 325-329 in the SDMA factory block, and the rest of this block mirrors 320-334 with only the names swapped. A helper taking binary, xml name and label would collapse both sites and stop them drifting; it needs to sit at top level, since the build subshell that holds the other helpers ends at line 299.
| printf 'export RCCL_TESTS_BIN_DIR=%q\n' "${rccl_tests_build}" | ||
| printf 'export RCCL_TESTS_DIR=%q\n' "${rccl_tests_src}" | ||
| printf 'export RCCL_FIXTURES_BIN_DIR=%q\n' "$(dirname "${rccl_fixtures_bin}")" | ||
| printf 'export RCCL_ANVIL_PLUGIN_TEST=%q\n' "${rccl_conn_plugin_bin}" |
There was a problem hiding this comment.
Low
RCCL_ANVIL_PLUGIN_TEST duplicates the fallback that consumes it: RCCL_FIXTURES_BIN_DIR is the dirname of line 121, the same ${rccl_build}/test directory line 122 builds this path from, so both branches of line 339 resolve to the same file. It is not an override hook either, since line 316 re-sources rccl.env last and overwrites any inherited value. Could this use the plain form line 323 uses?
|
Superseded by #12745 and closing in favour of it. Same commits, same content; only the head branch moved. Why. This PR was opened from a fork, and the required Please keep reading this thread. 5 of the 5 review threads here are still unresolved, and inline conversations do not carry across to a new pull request. Argus will re-review #12745 and should re-surface anything still unfixed, but the replies and reasoning written here only exist on this PR, so check them before merging the replacement. No further work is needed on this branch. Closing rather than leaving it open so it does not keep consuming CI or show up as a duplicate for reviewers; it stays readable and can be reopened. |
Motivation
Follow-up to #11569 / AICOMRCCL-2339.
gin.sbatchalready buildsrccl-UnitTestsGinAnvilPluginunderGIN_ANVIL_UNIT_TESTS=ON, but only the SDMA factory gtest is executed. Logical-context bind coverage therefore has no automated GIN-CI run.Technical Details
RCCL_ANVIL_PLUGIN_TESTfrom the build stageJIRA ID: AICOMRCCL-2339
Test Plan
rccl-UnitTestsGinAnvilPluginTest Result
Submission Checklist
Made with Cursor