Repository navigation
Conversation
9e64953 to
56b7791
Compare
1f3c94c to
e983892
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/run-gin-ci.sh:216(Nit) The pytest arm's bench_timeout assignment is dead after the unconditional rebind at line 267bench_timeout="${GIN_PYTEST_TIMEOUT}"in thepytest)arm at run-gin-ci.sh:216 is dead after this change: line 267 rebinds it fromouter_timeouton every path that reaches the launch, and the two early returns in between never read it. The arm itself still has to exist sopytestdoes not fall through to the unknown-kind*)at line 217, sopytest) ;;is the shape. Could the stale assignment go?
Bottom line: Worth settling before approval: the RS_NP rocminfo probe at test_ReduceScatterGinSdma.py:85 falls back to 0 and skips the whole suite green; gin_sdma_harness.detect_ngpus already raises instead.
Comment @argus review to re-run, or @argus re-review after pushing.
| RS_NP = len(os.environ["HIP_VISIBLE_DEVICES"].split(",")) | ||
| else: | ||
| try: | ||
| RS_NP = int(subprocess.check_output( |
There was a problem hiding this comment.
Medium
RS_NP's rocminfo fallback reinstates the bug gin_sdma_harness.detect_ngpus exists to fix: rocminfo | grep | wc -l still exits 0 when rocminfo is off PATH, so RS_NP lands at 0, every test skips at line 165, and pytest exits 0. CI is immune because run-gin-ci.sh:291 always sets RCCL_TESTS_RS_NP, so this bites the manual runs the header documents. Could this import detect_ngpus, which raises instead and is pinned by test_Broadcast.py:365?
There was a problem hiding this comment.
Resolved by 21b979a8b3.
|
|
||
| _rs_enabled = os.environ.get("RCCL_TESTS_GIN_SDMA_RS", "") not in ("", "0", "false", "False") | ||
|
|
||
| def _env_int(name, default): |
There was a problem hiding this comment.
Medium
_env_int is a character-identical copy of env_int at gin_sdma_harness.py:83, docstring included, and lines 105 to 181 re-implement CONN_GATE_RE/DATA_FAIL_RE, mpi_launch_prefix, gin_env_xflags, gin_perf_argv, launch_mpi_shell and run_with_conn_gate_retry, about 85 lines. The copy also inherits none of the five offline tests at test_Broadcast.py:312-379. test_AllToAll.py:63 imports all of them, and gin_perf_argv's extra= already carries -o. Could this module import the harness too?
There was a problem hiding this comment.
Resolved by 21b979a8b3.
| BENCH_TIMEOUT="${BENCH_TIMEOUT:-600s}" | ||
| BENCH_KILL_AFTER="${BENCH_KILL_AFTER:-30s}" | ||
| GIN_PYTEST_TIMEOUT="${GIN_PYTEST_TIMEOUT:-1800s}" | ||
| GIN_PYTEST_RS_TIMEOUT="${GIN_PYTEST_RS_TIMEOUT:-3600s}" |
There was a problem hiding this comment.
Medium
GIN_PYTEST_RS_TIMEOUT at 3600s plus three new 600s entries take the worst-case test phase past the job's wall clock, and gin.sbatch:7 is still --time=04:00:00:
before: 8 x 600s + 1800s = 6600s
after : 11 x 600s + 1800s + 3600s = 12000s (3h20m)
That leaves under 40 minutes of the 4h for steps 1 to 6, which build OpenMPI, rocSHMEM, RCCL and rccl-tests. A SLURM kill also lands before gin.sbatch:362 writes the sentinel, so all 13 results go with it. Raise --time, or trim the caps?
There was a problem hiding this comment.
Resolved by 21b979a8b3.
| self.assertEqual(parts[2], "pytest") | ||
| self.assertEqual(parts[3], binary) | ||
| self.assertEqual(parts[4], expected_env) | ||
| self.assertEqual(parts[5], "-k GinSdma -v") |
There was a problem hiding this comment.
Low
No test loads the shipped lib/gin-tests.json; this file only parses _write_json temp fixtures. A typo in any of the four new entries reaches the *) arm at run-gin-ci.sh:257 only after steps 1 to 6 have burned an 8 GPU node. Could a case here load the real config and assert every pytest entry's bin is one the runner handles?
There was a problem hiding this comment.
Resolved by 21b979a8b3.
|
|
||
|
|
||
| # CTA ladder probes (int32, 8 ranks -> total = per_rank * 8): | ||
| # 256 KiB/rank -> 2 MiB total -> 32 CTAs (small) |
There was a problem hiding this comment.
Low
The 256 KiB/rank row in this ladder comment documents a probe that never runs: per_rank_mib at line 190 starts at 1 and is an integer MiB, so 256 KiB is inexpressible. The 32 CTA arm is still reached from the large side, since 8 MiB/rank is 64 MiB total at NP=8, so nothing is actually uncovered. Drop the row, or add the case?
There was a problem hiding this comment.
Resolved by 21b979a8b3.
| # Hardware launches in test_ReduceScatterGinSdma.py: 12 CTA-ladder | ||
| # (6 size/op pairs x 2 dtypes) + 6 narrowing-dtype (3 dtypes x sum/avg) | ||
| # + 4 low-CTA SDMA (2 sizes x 2 dtypes) + 3 hang-guard dtypes. | ||
| GIN_PYTEST_RS_HW_CASES="${GIN_PYTEST_RS_HW_CASES:-25}" |
There was a problem hiding this comment.
Low
GIN_PYTEST_RS_HW_CASES is a hand-count of the parametrize lists with nothing keeping the two in sync, and it is the divisor for the derived per-launch cap:
RCCL_TESTS_RS_TIMEOUT_S = (3600 - 30 - 60 - 1) / (25 * 2) = 70s
A 26th case leaves the divisor at 25, so the cap stays 70s, and the self-check at line 200 reads the same stale count and stays quiet. Could the module export its launch count instead?
There was a problem hiding this comment.
Resolved by 21b979a8b3.
| # Both pytest files run through one case. parse_config and format_rows | ||
| # branch on neither bin nor kind -- format_rows joins positionally -- so | ||
| # a second standalone copy of this test could not fail on its own. The | ||
| # env lists differ in length so the join is pinned at both arities. |
There was a problem hiding this comment.
Nit
| # env lists differ in length so the join is pinned at both arities. | |
| # Nothing in parse_config or format_rows branches on bin or kind; the two | |
| # rows only pin the positional join at two env-list arities. |
There was a problem hiding this comment.
Resolved by 21b979a8b3.
| rc, _ = _run_rs_gin_sdma(request, 1 * MiB, dtype, "sum") | ||
| assert rc == 0, "ReduceScatter low-CTA SDMA datacheck failed dtype={}".format(dtype) | ||
| rc, _ = _run_rs_gin_sdma(request, 8 * MiB, dtype, "sum") | ||
| assert rc == 0, "ReduceScatter low-CTA SDMA mid-band datacheck failed dtype={}".format(dtype) |
There was a problem hiding this comment.
Nit
| assert rc == 0, "ReduceScatter low-CTA SDMA mid-band datacheck failed dtype={}".format(dtype) | |
| assert rc == 0, "ReduceScatter low-CTA SDMA 8 MiB/rank datacheck failed dtype={}".format(dtype) |
There was a problem hiding this comment.
Resolved by 21b979a8b3.
56b7791 to
21b979a
Compare
|
Rebased onto the current kernel tip and addressed the ARGUS review items:
@argus re-review |
| if [[ -n "${GIN_PYTEST_RS_HW_CASES}" ]]; then | ||
| hw_cases="${GIN_PYTEST_RS_HW_CASES}" | ||
| else | ||
| hw_cases="$(python3 - "${pytest_file}" <<'PY' |
There was a problem hiding this comment.
Medium
hw_cases from the RS_HW_LAUNCH_COUNT heredoc is used unchecked. Under set +e (line 232) a failed or partial match flows into apply_gin_pytest_inner_budget, which then fails open with no WARNING and no FAILED_RUNS entry:
RS_HW_LAUNCH_COUNT: int = 27 -> hw_cases="" -> denom 1 -> TIMEOUT_S=2309s inner=0
RS_HW_LAUNCH_COUNT = 2 * 13 -> hw_cases=2 -> denom 4 -> TIMEOUT_S=577s inner=2308
Could we require [[ "${hw_cases}" =~ ^[1-9][0-9]*$ ]] before line 288 and record a FAILED_RUNS entry otherwise?
Run the policy unit test, GPU test, and opt-in reduce_scatter_perf pytest from the GIN CI entries.
Run the NCCL_GIN_ANVIL_RS_CTAS=4 datacheck at 1 MiB/rank and 8 MiB/rank and count those four launches in the GIN pytest budget. AICOMRCCL-1803 Co-authored-by: Cursor <cursoragent@cursor.com>
Import gin_sdma_harness, cover the LSA peer-fold arms at NP=4/7, export RS_HW_LAUNCH_COUNT for the CI budget, and trim the RS pytest cap to 2400s so the sequential test caps fit under gin.sbatch's 4h wall clock. AICOMRCCL-1803 Co-authored-by: Cursor <cursoragent@cursor.com>
21b979a to
ed9556f
Compare
reduce_scatter_perf prints "# SKIP" and exits 0 when the device API is unavailable, for example when RCCL disables cuMem on Linux < 6.8. The ReduceScatter tests asserted only rc == 0, so on such a node all 25 hardware cases passed without running the kernel. Each case now also requires no "# SKIP" line, at least one measured row, a numeric zero #wrong in both columns and a zero out-of-bounds count, matching what assert_bcast_ok checks for Broadcast. An offline test locks the verdict against real reduce_scatter_perf output. On MI300X with Linux 5.15 the module now reports 25 failed, 2 passed instead of 26 passed. AICOMRCCL-1803 Co-authored-by: Cursor <cursoragent@cursor.com>
Motivation
Run the new ReduceScatter path from the opt-in pytest and the GIN CI jobs once the kernel in #13303 exists, including the low-CTA SDMA pin.
Technical Details
Third of three stacked PRs that replace #12747. Based on
users/dondai/gin-sdma-rs-d3-kernel.test_ReduceScatterGinSdma.py, gated byRCCL_TESTS_GIN_SDMA_RS=1, defaultNCCL_GIN_TYPE=7. Importsgin_sdma_harness(same pattern as AllToAll / Broadcast).test_ReduceScatterGinSdmaLowCtaSdma:NCCL_GIN_ANVIL_RS_CTAS=4at 1 MiB/rank and 8 MiB/rank, int32 and float.test_ReduceScatterGinSdmaPeerLoopArms: NP=4 and NP=7 so the 4-wide, 2-wide, and odd-peer LSA fold arms inginReduceScatterBodyexecute.RS_HW_LAUNCH_COUNT = 27(12 CTA + 6 narrowing + 4 low-CTA + 2 peer-loop + 3 hang-guard);run-gin-ci.shreads that constant for the inner budget.GIN_PYTEST_RS_TIMEOUTdefault trimmed to 2400s so the worst-case sum of sequential caps stays undergin.sbatch's 4h wall clock.rccl-gin-rocshmem-sdma-rs-d3,rccl-gin-rs-policy-ut,rccl-gin-rs-gpu-ut, andrccl-gin-rs-pytest.Merge #13302 and #13303 first. After #13303 merges, retarget this PR onto
develop.The low-CTA pytest came from fork draft dlamd1dai#17. It checks correctness of the pin. It is not a bandwidth comparison against the LSA ladder.
Issue Tracking
JIRA ID: AICOMRCCL-1803
Test Plan
RCCL_TESTS_GIN_SDMA_RS=1pytesttest_ReduceScatterGinSdma.pywithNCCL_GIN_TYPE=7NCCL_GIN_ANVIL_RS_CTAS=4datacheck at 1 MiB/rank and 8 MiB/rank, int32 and floatrccl-gin-rocshmem-sdma-rs-d3,rccl-gin-rs-policy-ut,rccl-gin-rs-gpu-ut, andrccl-gin-rs-pytest-D 0still skips floatingavgand still runsmulsumTest Result
Not re-run for the split. The pre-tier files match #12747 at
f3606768a3. Follow-up on this branch: harness import, peer-loop coverage,RS_HW_LAUNCH_COUNT, and the 2400s RS pytest cap.Submission Checklist
Made with Cursor