Repository navigation
feat(rccl): score the madengine gate against a same-node baseline - #12899
Conversation
✅ 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 |
coketaste
left a comment
There was a problem hiding this comment.
The same-node A/B is the right gate. Across this job's own history the node pair moves throughput by more than the 2% threshold, and a repeat on one pair stays well under it, so scoring candidate against baseline inside one allocation is what makes that threshold mean something. The stock image is the only baseline this workflow can pull, and giving each phase its own directory is required because madengine writes perf.csv and slurm_output/ under fixed names.
Not ready to leave draft:
MADENGINE_REPO/MADENGINE_REFstill point atmkuznet1/madengine10a0414. Until that is onROCm/madengine, the innermadengine runsubmits its ownsbatchand both phases leave the allocation.- This has not been run end to end. The 35 local checks are not in the tree, so CI cannot re-run them (the policy bot auto-passes unit tests on this script).
Two behaviors to fix or explicitly accept are noted inline.
Four fixes from review. The first two were fail-closed. Metric alias. extract_maxtext_perf.py writes `tok_per_s_per_gpu`, already an average of the last 10 steps, and madengine copies the name through. The global alias accepts only `tok_per_s_per_gpu_avg` after #12899, so every structured throughput row was filtered out and the live-log fallback only understands Megatron's format: a finished run produced no comparison and failed closed. Accepted per workload via a new `metric_aliases` key rather than widened globally -- Megatron emits both names with different meanings, and widening would score llama-3.1-70b on its final iteration again. The stale comment claiming the name was already in _METRIC_ALIASES was true before the rebase onto #12899 and is gone. Timeout. The script's budget covers queueing, because the allocation is taken inside it. At the 330 default a 5 h time_limit left 30 minutes of queue slack, against the 90 the 2-node workloads get; a dispatch of this job waited 13 h 14 m for a runner, so it would have been killed while still queued, reporting that neither phase ran. `timeout_minutes` is an input again, 360 by default so the matrix workloads are unchanged, 420 for this one -- 5 h plus the same 90 minutes. Node count. The workflow's `nodes` input defaults to 2, so a hand dispatch selecting this workload and leaving the rest alone took a 2-node exclusive allocation and OOMed in step 0. `required_nodes` is checked in main() before anything allocates. Registry. generate_manifest() marked any non-pull_only image as registry-backed when --registry was set, but build_maxtext_baseline_image() has nothing to push to, so nodes would pull a tag that was never uploaded instead of rebuilding from the advertised Dockerfile. A `local_only` flag keeps that phase on the local-image path. The weekly job passes no --registry, so this was latent. patchelf's RPATH-to-RUNPATH conversion is left as MAD#228 has it, per review -- the gate shows resolution is unaffected, and changing it without a run that shows the loaded libamd_smi moved would be an untested variation. Verified: the 405B phase now collects its row while llama-3.1-70b still ignores the un-suffixed name; all four workloads get >= 90 minutes of queue slack; the baseline stays local_image under --registry while the candidate does not; all six Megatron phase manifests remain byte-identical.
…e gate Adds `llama-3.1-405b-maxtext` at 4 nodes (32 GPUs) as the first JAX workload in the gate, and the first whose base image is not rocm/primus. Stacked on Draft until #12899 and #13180 land. The enabling work is ROCm/MAD#228, which validated this workload at 4N. Model family ------------ Until now every workload was PyTorch/Megatron-LM out of MAD's scripts/primus_megatron-lm. A "family" key now dispatches the three places that differ: install_madengine() copies scripts/jax-maxtext and checks Primus out, generate_manifest() emits `--config_path <yaml>` plus a `distributed.primus` block instead of `--model_repo`, and the overlay builder uses a different recipe. Settings lifted from MAD#228's template are each a failure mode, not taste: XLA_GPU_AUTOTUNE_LEVEL=0 Primus defaults to level 4, which at 32 ranks presents as a dead hang -- no steps, no NCCL output -- rather than a slow start. The MAD-native XLA_AUTOTUNE_LEVEL is a silent no-op. per_device_batch_size=1 the shipped config OOMs at 241.25 GiB for jit_train_step at 4 nodes. --ulimit nofile=1048576 a 32-device clique exhausts the default and stalls silently at "Initialize clique". They ride the per-workload env_vars key #13180 introduced. Why this family needs its own A/B baseline ------------------------------------------ The Megatron baseline is the stock registry image. That cannot work here: rocm/jax-training:maxtext-* ships MaxText but no Primus, so scripts/jax-maxtext/run.sh would not find primus-cli and the baseline phase would die before step 0. So the baseline is built: base image plus the Primus payload, stock RCCL. The candidate is that same Dockerfile plus the librccl swap -- literally a prefix, so the payload layers come out of the cache and only the swap builds twice. The pair therefore differs by the RCCL swap and nothing else, which is tighter than the Megatron A/B rather than looser. The baseline manifest advertises its own Dockerfile. Left at the default a compute node would rebuild the swapped image under the baseline tag, giving an A/B that compares the CI RCCL against itself. Overlay ------- The swap for this family is discovery-based -- find, per-target RUNPATH preservation, and an unresolved-soname set gate -- ported from MAD#228, rather than aimed at the Primus SDK venv paths, which this base lacks. The libamd_smi soname bridge is kept and now runs over every directory the sweep finds, ordered after the copy and before the post-swap measurement so a .27-against-.26 skew is reconciled instead of failing the gate. Unlike MAD#228's dockerfile the Primus payload is gated on primus-cli, not examples/run_pretrain.sh: AMD-AGI/Primus#999 retired that file and run.sh drives primus-cli. v26.7.0 still carries both so MAD's gate passes today, but it would hard-fail on jax-maxtext-v26.8. PRIMUS_REF is pinned here. --skip-overlay-build is refused for this family: its bind-mount targets are Primus-image paths absent from this base, and `docker run -v` creates a missing source as an empty directory rather than refusing, so the run would look healthy while measuring the image's own RCCL. Scheduling ---------- Its own job rather than a third matrix entry: the weekly gate has to be a job-level `if:`, where the matrix context is unavailable, and it is fixed at 4 nodes while the matrix entries take madengine_nodes. WORK_DIR already carries inputs.workload, so they do not collide. Not gated on the matrix job -- a 70B failure should not hide a 405B result. time_limit is 05:00:00 for the A/B pair, against MAD#228's 12:00:00 for a single run: a 4-node exclusive request with a 12 h limit backfills badly, the measured run is ~17 min of training (MAD#228 reports 17.40 s/step) plus prep, and 300 min stays inside the step's existing 330 so the script still reports. The existing 360/330 budget needed no change. Risk to the existing workloads ------------------------------ MAD_REF moves 07ecef61 -> 7acfac7c, which is exactly one commit, ROCm/MAD#279, repointing tools/fetch_primus.sh at a ref that still exists. It touches benchmark/jax_maxtext/README.md, docker/primus_maxdiffusion.ubuntu.amd.Dockerfile and tools/fetch_primus.sh -- nothing under scripts/primus_megatron-lm. I rendered both A/B phase manifests for llama-3.1-70b, llama-4-scout and gpt-oss-120b against this branch and its base: byte-identical, all six. Also verified: the 405B manifest matches MAD#228's validated template field for field; the baseline and candidate manifests differ only in image and dockerfile; both generated Dockerfiles parse under `bash -n`, with the bridge ordered after the copy and before the gate. Not yet run on hardware. The first run has to establish whether the TheRock kpack librccl loads on this base at all, and whether its libamd_smi soname needs the bridge here. JIRA ID AICOMNET-420
Four fixes from review. The first two were fail-closed. Metric alias. extract_maxtext_perf.py writes `tok_per_s_per_gpu`, already an average of the last 10 steps, and madengine copies the name through. The global alias accepts only `tok_per_s_per_gpu_avg` after #12899, so every structured throughput row was filtered out and the live-log fallback only understands Megatron's format: a finished run produced no comparison and failed closed. Accepted per workload via a new `metric_aliases` key rather than widened globally -- Megatron emits both names with different meanings, and widening would score llama-3.1-70b on its final iteration again. The stale comment claiming the name was already in _METRIC_ALIASES was true before the rebase onto #12899 and is gone. Timeout. The script's budget covers queueing, because the allocation is taken inside it. At the 330 default a 5 h time_limit left 30 minutes of queue slack, against the 90 the 2-node workloads get; a dispatch of this job waited 13 h 14 m for a runner, so it would have been killed while still queued, reporting that neither phase ran. `timeout_minutes` is an input again, 360 by default so the matrix workloads are unchanged, 420 for this one -- 5 h plus the same 90 minutes. Node count. The workflow's `nodes` input defaults to 2, so a hand dispatch selecting this workload and leaving the rest alone took a 2-node exclusive allocation and OOMed in step 0. `required_nodes` is checked in main() before anything allocates. Registry. generate_manifest() marked any non-pull_only image as registry-backed when --registry was set, but build_maxtext_baseline_image() has nothing to push to, so nodes would pull a tag that was never uploaded instead of rebuilding from the advertised Dockerfile. A `local_only` flag keeps that phase on the local-image path. The weekly job passes no --registry, so this was latent. patchelf's RPATH-to-RUNPATH conversion is left as MAD#228 has it, per review -- the gate shows resolution is unaffected, and changing it without a run that shows the loaded libamd_smi moved would be an untested variation. Verified: the 405B phase now collects its row while llama-3.1-70b still ignores the un-suffixed name; all four workloads get >= 90 minutes of queue slack; the baseline stays local_image under --registry while the candidate does not; all six Megatron phase manifests remain byte-identical.
…e gate Adds `llama-3.1-405b-maxtext` at 4 nodes (32 GPUs) as the first JAX workload in the gate, and the first whose base image is not rocm/primus. Stacked on Draft until #12899 and #13180 land. The enabling work is ROCm/MAD#228, which validated this workload at 4N. Model family ------------ Until now every workload was PyTorch/Megatron-LM out of MAD's scripts/primus_megatron-lm. A "family" key now dispatches the three places that differ: install_madengine() copies scripts/jax-maxtext and checks Primus out, generate_manifest() emits `--config_path <yaml>` plus a `distributed.primus` block instead of `--model_repo`, and the overlay builder uses a different recipe. Settings lifted from MAD#228's template are each a failure mode, not taste: XLA_GPU_AUTOTUNE_LEVEL=0 Primus defaults to level 4, which at 32 ranks presents as a dead hang -- no steps, no NCCL output -- rather than a slow start. The MAD-native XLA_AUTOTUNE_LEVEL is a silent no-op. per_device_batch_size=1 the shipped config OOMs at 241.25 GiB for jit_train_step at 4 nodes. --ulimit nofile=1048576 a 32-device clique exhausts the default and stalls silently at "Initialize clique". They ride the per-workload env_vars key #13180 introduced. Why this family needs its own A/B baseline ------------------------------------------ The Megatron baseline is the stock registry image. That cannot work here: rocm/jax-training:maxtext-* ships MaxText but no Primus, so scripts/jax-maxtext/run.sh would not find primus-cli and the baseline phase would die before step 0. So the baseline is built: base image plus the Primus payload, stock RCCL. The candidate is that same Dockerfile plus the librccl swap -- literally a prefix, so the payload layers come out of the cache and only the swap builds twice. The pair therefore differs by the RCCL swap and nothing else, which is tighter than the Megatron A/B rather than looser. The baseline manifest advertises its own Dockerfile. Left at the default a compute node would rebuild the swapped image under the baseline tag, giving an A/B that compares the CI RCCL against itself. Overlay ------- The swap for this family is discovery-based -- find, per-target RUNPATH preservation, and an unresolved-soname set gate -- ported from MAD#228, rather than aimed at the Primus SDK venv paths, which this base lacks. The libamd_smi soname bridge is kept and now runs over every directory the sweep finds, ordered after the copy and before the post-swap measurement so a .27-against-.26 skew is reconciled instead of failing the gate. Unlike MAD#228's dockerfile the Primus payload is gated on primus-cli, not examples/run_pretrain.sh: AMD-AGI/Primus#999 retired that file and run.sh drives primus-cli. v26.7.0 still carries both so MAD's gate passes today, but it would hard-fail on jax-maxtext-v26.8. PRIMUS_REF is pinned here. --skip-overlay-build is refused for this family: its bind-mount targets are Primus-image paths absent from this base, and `docker run -v` creates a missing source as an empty directory rather than refusing, so the run would look healthy while measuring the image's own RCCL. Scheduling ---------- Its own job rather than a third matrix entry: the weekly gate has to be a job-level `if:`, where the matrix context is unavailable, and it is fixed at 4 nodes while the matrix entries take madengine_nodes. WORK_DIR already carries inputs.workload, so they do not collide. Not gated on the matrix job -- a 70B failure should not hide a 405B result. time_limit is 05:00:00 for the A/B pair, against MAD#228's 12:00:00 for a single run: a 4-node exclusive request with a 12 h limit backfills badly, the measured run is ~17 min of training (MAD#228 reports 17.40 s/step) plus prep, and 300 min stays inside the step's existing 330 so the script still reports. The existing 360/330 budget needed no change. Risk to the existing workloads ------------------------------ MAD_REF moves 07ecef61 -> 7acfac7c, which is exactly one commit, ROCm/MAD#279, repointing tools/fetch_primus.sh at a ref that still exists. It touches benchmark/jax_maxtext/README.md, docker/primus_maxdiffusion.ubuntu.amd.Dockerfile and tools/fetch_primus.sh -- nothing under scripts/primus_megatron-lm. I rendered both A/B phase manifests for llama-3.1-70b, llama-4-scout and gpt-oss-120b against this branch and its base: byte-identical, all six. Also verified: the 405B manifest matches MAD#228's validated template field for field; the baseline and candidate manifests differ only in image and dockerfile; both generated Dockerfiles parse under `bash -n`, with the bridge ordered after the copy and before the gate. Not yet run on hardware. The first run has to establish whether the TheRock kpack librccl loads on this base at all, and whether its libamd_smi soname needs the bridge here. JIRA ID AICOMNET-420
Four fixes from review. The first two were fail-closed. Metric alias. extract_maxtext_perf.py writes `tok_per_s_per_gpu`, already an average of the last 10 steps, and madengine copies the name through. The global alias accepts only `tok_per_s_per_gpu_avg` after #12899, so every structured throughput row was filtered out and the live-log fallback only understands Megatron's format: a finished run produced no comparison and failed closed. Accepted per workload via a new `metric_aliases` key rather than widened globally -- Megatron emits both names with different meanings, and widening would score llama-3.1-70b on its final iteration again. The stale comment claiming the name was already in _METRIC_ALIASES was true before the rebase onto #12899 and is gone. Timeout. The script's budget covers queueing, because the allocation is taken inside it. At the 330 default a 5 h time_limit left 30 minutes of queue slack, against the 90 the 2-node workloads get; a dispatch of this job waited 13 h 14 m for a runner, so it would have been killed while still queued, reporting that neither phase ran. `timeout_minutes` is an input again, 360 by default so the matrix workloads are unchanged, 420 for this one -- 5 h plus the same 90 minutes. Node count. The workflow's `nodes` input defaults to 2, so a hand dispatch selecting this workload and leaving the rest alone took a 2-node exclusive allocation and OOMed in step 0. `required_nodes` is checked in main() before anything allocates. Registry. generate_manifest() marked any non-pull_only image as registry-backed when --registry was set, but build_maxtext_baseline_image() has nothing to push to, so nodes would pull a tag that was never uploaded instead of rebuilding from the advertised Dockerfile. A `local_only` flag keeps that phase on the local-image path. The weekly job passes no --registry, so this was latent. patchelf's RPATH-to-RUNPATH conversion is left as MAD#228 has it, per review -- the gate shows resolution is unaffected, and changing it without a run that shows the loaded libamd_smi moved would be an untested variation. Verified: the 405B phase now collects its row while llama-3.1-70b still ignores the un-suffixed name; all four workloads get >= 90 minutes of queue slack; the baseline stays local_image under --registry while the candidate does not; all six Megatron phase manifests remain byte-identical.
|
@amd-argus review |
There was a problem hiding this comment.
ARGUS
Findings
3 more finding(s), not posted inline
projects/rccl/ci/scripts/test_madengine.py:1203(Low) New gate logic (pair_by_precision, check_ab_regression) has no offline test; both E2E runs only hit the passing pathprojects/rccl/ci/scripts/test_madengine.py:716(Low) generate_manifest builds two image_entry dicts that repeat docker_image, build_status, build_duration and gpu_vendorprojects/rccl/ci/scripts/test_madengine.py:1584(Low) phase_images re-states the phase-to-image mapping already in the phase loop at line 1453
This PR changes 1,110 lines across 2 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: Looks approval ready.
Comment @argus review to re-run, or @argus re-review after pushing.
…e gate Adds `llama-3.1-405b-maxtext` at 4 nodes (32 GPUs) as the first JAX workload in the gate, and the first whose base image is not rocm/primus. Stacked on Draft until #12899 and #13180 land. The enabling work is ROCm/MAD#228, which validated this workload at 4N. Model family ------------ Until now every workload was PyTorch/Megatron-LM out of MAD's scripts/primus_megatron-lm. A "family" key now dispatches the three places that differ: install_madengine() copies scripts/jax-maxtext and checks Primus out, generate_manifest() emits `--config_path <yaml>` plus a `distributed.primus` block instead of `--model_repo`, and the overlay builder uses a different recipe. Settings lifted from MAD#228's template are each a failure mode, not taste: XLA_GPU_AUTOTUNE_LEVEL=0 Primus defaults to level 4, which at 32 ranks presents as a dead hang -- no steps, no NCCL output -- rather than a slow start. The MAD-native XLA_AUTOTUNE_LEVEL is a silent no-op. per_device_batch_size=1 the shipped config OOMs at 241.25 GiB for jit_train_step at 4 nodes. --ulimit nofile=1048576 a 32-device clique exhausts the default and stalls silently at "Initialize clique". They ride the per-workload env_vars key #13180 introduced. Why this family needs its own A/B baseline ------------------------------------------ The Megatron baseline is the stock registry image. That cannot work here: rocm/jax-training:maxtext-* ships MaxText but no Primus, so scripts/jax-maxtext/run.sh would not find primus-cli and the baseline phase would die before step 0. So the baseline is built: base image plus the Primus payload, stock RCCL. The candidate is that same Dockerfile plus the librccl swap -- literally a prefix, so the payload layers come out of the cache and only the swap builds twice. The pair therefore differs by the RCCL swap and nothing else, which is tighter than the Megatron A/B rather than looser. The baseline manifest advertises its own Dockerfile. Left at the default a compute node would rebuild the swapped image under the baseline tag, giving an A/B that compares the CI RCCL against itself. Overlay ------- The swap for this family is discovery-based -- find, per-target RUNPATH preservation, and an unresolved-soname set gate -- ported from MAD#228, rather than aimed at the Primus SDK venv paths, which this base lacks. The libamd_smi soname bridge is kept and now runs over every directory the sweep finds, ordered after the copy and before the post-swap measurement so a .27-against-.26 skew is reconciled instead of failing the gate. Unlike MAD#228's dockerfile the Primus payload is gated on primus-cli, not examples/run_pretrain.sh: AMD-AGI/Primus#999 retired that file and run.sh drives primus-cli. v26.7.0 still carries both so MAD's gate passes today, but it would hard-fail on jax-maxtext-v26.8. PRIMUS_REF is pinned here. --skip-overlay-build is refused for this family: its bind-mount targets are Primus-image paths absent from this base, and `docker run -v` creates a missing source as an empty directory rather than refusing, so the run would look healthy while measuring the image's own RCCL. Scheduling ---------- Its own job rather than a third matrix entry: the weekly gate has to be a job-level `if:`, where the matrix context is unavailable, and it is fixed at 4 nodes while the matrix entries take madengine_nodes. WORK_DIR already carries inputs.workload, so they do not collide. Not gated on the matrix job -- a 70B failure should not hide a 405B result. time_limit is 05:00:00 for the A/B pair, against MAD#228's 12:00:00 for a single run: a 4-node exclusive request with a 12 h limit backfills badly, the measured run is ~17 min of training (MAD#228 reports 17.40 s/step) plus prep, and 300 min stays inside the step's existing 330 so the script still reports. The existing 360/330 budget needed no change. Risk to the existing workloads ------------------------------ MAD_REF moves 07ecef61 -> 7acfac7c, which is exactly one commit, ROCm/MAD#279, repointing tools/fetch_primus.sh at a ref that still exists. It touches benchmark/jax_maxtext/README.md, docker/primus_maxdiffusion.ubuntu.amd.Dockerfile and tools/fetch_primus.sh -- nothing under scripts/primus_megatron-lm. I rendered both A/B phase manifests for llama-3.1-70b, llama-4-scout and gpt-oss-120b against this branch and its base: byte-identical, all six. Also verified: the 405B manifest matches MAD#228's validated template field for field; the baseline and candidate manifests differ only in image and dockerfile; both generated Dockerfiles parse under `bash -n`, with the bridge ordered after the copy and before the gate. Not yet run on hardware. The first run has to establish whether the TheRock kpack librccl loads on this base at all, and whether its libamd_smi soname needs the bridge here. JIRA ID AICOMNET-420
Four fixes from review. The first two were fail-closed. Metric alias. extract_maxtext_perf.py writes `tok_per_s_per_gpu`, already an average of the last 10 steps, and madengine copies the name through. The global alias accepts only `tok_per_s_per_gpu_avg` after #12899, so every structured throughput row was filtered out and the live-log fallback only understands Megatron's format: a finished run produced no comparison and failed closed. Accepted per workload via a new `metric_aliases` key rather than widened globally -- Megatron emits both names with different meanings, and widening would score llama-3.1-70b on its final iteration again. The stale comment claiming the name was already in _METRIC_ALIASES was true before the rebase onto #12899 and is gone. Timeout. The script's budget covers queueing, because the allocation is taken inside it. At the 330 default a 5 h time_limit left 30 minutes of queue slack, against the 90 the 2-node workloads get; a dispatch of this job waited 13 h 14 m for a runner, so it would have been killed while still queued, reporting that neither phase ran. `timeout_minutes` is an input again, 360 by default so the matrix workloads are unchanged, 420 for this one -- 5 h plus the same 90 minutes. Node count. The workflow's `nodes` input defaults to 2, so a hand dispatch selecting this workload and leaving the rest alone took a 2-node exclusive allocation and OOMed in step 0. `required_nodes` is checked in main() before anything allocates. Registry. generate_manifest() marked any non-pull_only image as registry-backed when --registry was set, but build_maxtext_baseline_image() has nothing to push to, so nodes would pull a tag that was never uploaded instead of rebuilding from the advertised Dockerfile. A `local_only` flag keeps that phase on the local-image path. The weekly job passes no --registry, so this was latent. patchelf's RPATH-to-RUNPATH conversion is left as MAD#228 has it, per review -- the gate shows resolution is unaffected, and changing it without a run that shows the loaded libamd_smi moved would be an untested variation. Verified: the 405B phase now collects its row while llama-3.1-70b still ignores the un-suffixed name; all four workloads get >= 90 minutes of queue slack; the baseline stays local_image under --registry while the candidate does not; all six Megatron phase manifests remain byte-identical.
There was a problem hiding this comment.
ARGUS
This PR changes 1,090 lines across 2 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: on timeout, test_madengine.py:910 kills only salloc, so run_phases.sh and its madengine/srun children can keep running on the allocation.
Comment @argus review to re-run, or @argus re-review after pushing.
The stack conflicts when it lands in order, and this comment is why. #12899 reworded it to a one-liner after #13180 branched, so #13180 -- and this branch on top of it -- still carry the old two-line form. Landing #12899 first and then this branch makes both sides changed in the same region. Removing the `timeout-minutes` line from the step is substantive and stays. The explanation of why it is gone does not need to live in that hunk, so it moves up to the job-level timeout it is actually about, and the pre-existing comment is left exactly as #13180 has it. Only one side then touches those lines and the merge resolves on its own. The real staleness is that #13180 sits on an older ci/madengine-same-node-ab; rebasing it is that PR's call, not this one's. This just stops this branch from adding to the collision.
There was a problem hiding this comment.
ARGUS
This PR changes 1,286 lines across 2 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: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
pr-rocm-systems-12899-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
@speriaswamy-amd fixed and verified on a real allocation. Cancelled run 40942856424 after SLURM job 105807 was granted: the Release the SLURM allocation step printed "Releasing SLURM job 105807" and the job went CANCELLED 43 s later, versus TIMEOUT after 5 h before. |
The SIGTERM handler stays installed while release_allocation() runs, so a second signal during scancel raised out of it and skipped killpg() and wait(): the separately started group survived. Both now run in a `finally`. The regression gate compared `ratio - 1 < -threshold`, and the subtraction rounds: a loss of exactly the threshold failed for some inputs (1000 -> 980) and passed for others (200.7 -> 196.686). Comparing `ratio < 1 - threshold` treats the boundary the same way every time. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Reusing the work directory retains stale exit codes, metrics, and logs, allowing a failed or timed-out allocation to produce a PASSED report without running a new comparison.
1 open issue
2 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
ARGUS
This PR changes 1,292 lines across 2 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: a cancel or timeout while salloc is still queued SIGKILLs it without a scancel (test_madengine.py:929), so the pending job can later start and hold the nodes.
Comment @argus review to re-run, or @argus re-review after pushing.
… results A cancel or timeout while salloc is still queued left the request behind: allocation.txt has no job id yet, and SIGKILL, which salloc cannot handle, does not withdraw it. With InactiveLimit at 0 on ruby the job then starts with nothing attached and holds the nodes until --time. On ruby, a pending salloc killed this way stayed PENDING until it was cancelled by hand. The job name is now unique per run and recorded before salloc starts, and both the Python path and the workflow's release step cancel by that name, which reaches a queued request as well as a granted one. prepare_phase_dir() now recreates the phase directory. On a reused --work-dir an earlier run's exit_code, perf files and node logs stayed in place, and an allocation that never ran a phase was scored on them. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
ARGUS
This PR changes 1,319 lines across 2 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: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
The gate compared candidate/stock with 1.0. That works only while the image ships an RCCL close to the CI build: on the Megatron workloads the ratio sits at 0.985-1.004, but on 405B MaxText it is 2.6-2.7, and a candidate could lose more than half its throughput and still clear 0.98. Each precision is now also scored against the ratio the previous run of the same workload and scale recorded. The stock image is the same every night, so it divides out the node pair each run was given and the two ratios are comparable where the absolute numbers are not. The same threshold applies, and either check fails the gate. The report lists the two checks on separate lines, and the candidate row in the datastore records which ratio and run it was compared against. A first run, with nothing earlier in the datastore, is not scored against a previous one. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
ARGUS
This PR changes 1,408 lines across 2 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: previous_comparison (test_madengine.py:1287) also takes a failed run's ratio as the reference, so a regression passes from the next night on; and the 360-minute job default can cut the driver off before it reports.
Comment @argus review to re-run, or @argus re-review after pushing.
previous_comparison() took the latest candidate row with a ratio, whatever that run's verdict. A drop that failed one night then became the reference and the next night passed, and a run whose candidate never loaded the CI RCCL set the reference to about 1.0. Each row now records the run's verdict, which by Step 8 covers the phase exit codes, RCCL verification and both regression checks, and only rows from passing runs are used. Rows written before this have no verdict and are skipped, so the first run after it is not scored against a previous. Co-Authored-By: Claude <noreply@anthropic.com>
pr-rocm-systems-12899-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
pr-rocm-systems-12899-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
There was a problem hiding this comment.
ARGUS
This PR changes 1,419 lines across 2 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: previous_comparison (test_madengine.py:1292) does not key on base_image, so a base-image bump that shifts the ratio past the threshold keeps the vs-previous gate failing with no reset.
Comment @argus review to re-run, or @argus re-review after pushing.
The stock image is the ratio's denominator, so a base_image bump moves the ratio with no change in the candidate. With only passing runs taken as references, a bump that moved it past the threshold was flagged every later night against the last pre-bump row, with nothing to reset it but editing the datastore. Rows now record base_image and the reference must match it. After a bump the first run has no previous and becomes the new reference. Both keys this gate reads, base_image and verdict, now open the datastore row instead of closing it, where the provenance fields stacked on this branch are added. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
ARGUS
This PR changes 1,431 lines across 2 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: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
pr-rocm-systems-12899-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
pr-rocm-systems-12899-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
speriaswamy-amd
left a comment
There was a problem hiding this comment.
Approved at c0e4d6d2308c. The cancellation finding is addressed: SIGINT/SIGTERM release the allocation, process-group cleanup runs even if scancel is interrupted, and the workflow has a cancellation release step. The queued-allocation and stale-results fixes also look sound.
Follow-up verification passed: 28 targeted assertions covering cancellation, interruption during cleanup, queued timeout, allocation failure, stale phase results, threshold boundaries, and historical-reference selection; plus 97 existing CI-script tests. The linked real cancellation run also confirms the workflow release step executed successfully. The current cancellation-by-name path was validated locally with stubs.
Remaining steps before merge:
- Resolve the remaining base-image reference review thread; the latest commit implements the requested filter.
- Update the PR description to explain both gates: candidate versus stock, and the current ratio versus the last passing ratio. Describe the initial run/new-base-image behavior when no historical reference exists.
- Let Math precheckin and its summary finish, and rerun the policy bot. Its current failure is a timeout waiting for checks that subsequently passed.
I also recommend committing the targeted MADEngine regression tests and wiring them into the CPU workflow. The existing green CI-script check covers the planner and ACCL driver, so it does not preserve coverage for these MADEngine behaviors.
No remaining blocking code finding from this review.
baseline (#12899)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
What this changes
The nightly MADEngine job scored one run against the rolling mean of the previous five. This replaces that with an A/B: the workload runs twice inside a single SLURM allocation — once on the stock base image, once on the same image
with the CI-built RCCL laid over it — and the gate is the ratio of the two.
Why
Each nightly lands on whichever nodes the scheduler picks, and the node pair, not RCCL, dominates the absolute throughput. Measured over four weeks of this job's own history:
across node pairs same pair, repeated
BF16 cv 2.18%, peak-to-peak 7.47% 0.02%
FP8 cv 2.70%, peak-to-peak 9.57% 0.25%
The repeat was three weeks apart on two different RCCL builds. So the workload is reproducible to a fraction of a percent on fixed hardware, and the 2% threshold sits below one standard deviation of the node-assignment noise.
Replaying the shipped gate over that history trips 3 of 11 scored runs on BF16 and 4 of 11 on FP8. AICOMRCCL-2456 was one of those false positives.
Comparing two runs on the same nodes divides the node term out. The ratio is not 1.0 and does not need to be — it needs to be stable, and a real RCCL regression moves it while an unlucky node pair does not.
[rocm-systems] ROCm/rocm-systems#12899 (commit a2a565b)
TheRock Submodule Bump ActivityNewest first
|
## What this changes Adds `gpt-oss-120b-training` at 2 nodes (16 GPUs) to the MADEngine gate as a second training workload. It runs through the same same-node A/B as llama-3.1-70b. Stacked on #12899 and based on its branch, so the diff shows only the commits below. Merge after #12899. - **Workload matrix.** The scheduled caller hardcoded `llama-3.1-70b-training`, so it becomes a matrix with `fail-fast: false`. `WORK_DIR` now includes the workload name, because matrix jobs share `run_id` and `run_attempt`. The legs run one at a time (`max-parallel: 1`): each wants an exclusive 2-node `salloc` on `meta64`, and queue time counts against its `--timeout-minutes`. The `workflow_dispatch` input becomes a `choice`. - **GPT-OSS-120B entry.** The `mad-rccl` pin from #12899 (`7a9a25e`) contains ROCm/MAD#271 (`07ecef61`), which makes GPT-OSS-120B fit at 16 GPUs and adds `PRIMUS_TRAIN_ITERS` and `PRIMUS_DISABLE_PROFILE`. The workload sets them through a new per-workload `env_vars` key, merged into the container env: 15 iterations with the profiler off. It expects BF16 and FP8, so a leg that stops running fails the gate. A workload's `env_vars` now also wins for `NCCL_SOCKET_IFNAME` and `GLOO_SOCKET_IFNAME`; neither workload sets them. - **Weekly, not nightly.** The nightly gate keeps only `llama-3.1-70b-training`. GPT-OSS-120B moves to a new `therock-test-madengine-weekly` job that runs only when `test_scope` is `all`, which the Saturday cron sets and a dispatch can request. It is a matrix of `include` entries with `max-parallel: 1`, so later weekly workloads queue behind each other rather than compete for exclusive nodes. It `needs` the nightly job for ordering only, and `!cancelled()` keeps a failed nightly from skipping it. Nodes are pinned per entry, because each workload was validated at one scale. - **Log size.** RCCL prints `ncclAllToAllv is deprecated` at WARN on every call. GPT-OSS's MoE all-to-all produced 777600 such lines per node per phase, 97% of a 180 MB node log; run 38238634668 alone left 2.1 GB under the runner account's disk quota. After the allocation, before anything reads or uploads the logs, `squash_log_spam()` keeps the first occurrence of each such line and replaces the repeats with a count: 1.1 GB to 24 MB on that run's baseline phase, with identical RCCL banners and parsed results. Real warnings are untouched. Tested in `tests/test_log_squash.py`. - **llama-4-scout dropped.** The entry predates this work and has never run on ruby, in CI or by hand. It pins `rocm/primus:v26.4`, while the MAD template keeps Scout on v26.3. It reached the dispatch choices only because that list mirrors `WORKLOAD_CONFIGS`. The gate now has exactly the two workloads that have run end to end. ## Why the profiler has to be off The shipped config profiles iterations 6-7. At 2 nodes those cost 3.2x (BF16) and 3.7x (FP8) a steady iteration, and iterations 8-9 stay elevated while the trace is written. Primus averages from iteration 3, so the profiler window would sit inside the measured window. With 15 iterations, 13 are measured, at about 65 s (BF16) and 116 s (FP8) each. ## Risk to the existing workloads - The two knobs are unset for llama-3.1-70b. I rendered its manifest to confirm: no `PRIMUS_*` variables appear, so its command is byte-identical. ## Testing and cost [Run 38238634668](https://github.com/ROCm/rocm-systems/actions/runs/38238634668) on ruby, 2x8 MI355X (job 70409): PASSED, BF16 ratio 0.9993, FP8 0.9924, with results read from `perf_primus-megatron-GPT-OSS-120B.csv` rather than the live-log fallback. Phases took 53 and 61 min, so about 2 h per A/B. `time_limit` is 4 h, which fits inside the existing `--timeout-minutes` 330. On the weekly schedule it adds about 2 h plus runner wait to the Saturday run. Peak HBM at 2 nodes was 191.6 GB, about 60 GB under the smaller nodes in `meta64`, so no exclude list is needed. That margin is arithmetic: the workload has not yet run on a small node. ## Not in this PR - A recorded baseline. The A/B gate is a ratio and needs no absolute reference. - Changing the llama iteration count. JIRA ID AICOMNET-427


What this changes
The nightly MADEngine job scored one run against the rolling mean of the previous five. This replaces that with an A/B: the workload runs twice inside a single SLURM allocation — once on the stock base image, once on the same image
with the CI-built RCCL laid over it — and the gate is the ratio of the two.
Why
Each nightly lands on whichever nodes the scheduler picks, and the node pair, not RCCL, dominates the absolute throughput. Measured over four weeks of this job's own history:
The repeat was three weeks apart on two different RCCL builds. So the workload is reproducible to a fraction of a percent on fixed hardware, and the 2% threshold sits below one standard deviation of the node-assignment noise.
Replaying the shipped gate over that history trips 3 of 11 scored runs on BF16 and 4 of 11 on FP8. AICOMRCCL-2456 was one of those false positives.
Comparing two runs on the same nodes divides the node term out. The ratio is not 1.0 and does not need to be — it needs to be stable, and a real RCCL regression moves it while an unlucky node pair does not.
Design notes
Why the baseline is the stock image. Not a free choice. The overlay is tagged
{base_image}-rccl-{gpu_target}-{rccl_commit}and is never pushed anywhere, so it only exists inside the allocation that built it. "Yesterday'soverlay as baseline" is not implementable without standing up a registry first. The base image is pullable and its layers are already on the node from the overlay build, so the extra pull is cheap. What the gate then measures is the
CI RCCL against the RCCL shipping in the image, on identical hardware.
Why one allocation and not two jobs. A second job pinned to the first one's nodes is not schedulable until those exact nodes free up, reserves nothing while it waits and accrues no priority — so on a busy shared partition the comparison would intermittently not happen at all. A gate that sometimes produces no answer is worse than a noisy one.
sallocholds the nodes across both runs; madengine picks upSLURM_JOB_IDand runs in place viasruninstead of submitting its ownsbatch.Images are built on the compute nodes only. The runner host used to
docker buildthe overlay under a per-run tag that nothing removed, and the nodes never used it: they rebuild the same tag from the manifest's Dockerfile (node 0 builds anddocker saves toMAD_DOCKER_BUILDSon shared storage, the othersdocker load). All runners on that host share its Docker disk, which ran out of space (run 39407041026). Now the runner host only stages the Dockerfile and build context;--registryand the single-node pin to the build host are gone (the workflow never passed--registry). After both phases the run removes its built tag from every allocated node (ab/cleanup_images.sh; a run killed by the timeout leaves it). The tar on shared storage is deleted by the cleanup step, which also removes from the runner host the tags earlier versions built and the workloads' base images, without-f, so an image still in use stays.Why each phase gets its own directory. madengine writes
perf.csv,perf_entry_super.jsonandslurm_output/under fixed names relative to its working directory, so a shared one would let the second run overwrite the first.Datastore. Absolute values are still recorded, one row per phase per precision, now carrying
phase,image,nodelist,slurm_job_id,artifact_run_id,rccl_md5and, on the candidate,ratio.run_idnames the test run;artifact_run_idandrccl_md5name the RCCL binary it tested. The node list is new: the analysis above had to recover it by joiningperf_entry_super.json:machine_nameagainstsacct.Every expected precision is scored. A workload lists the precisions MAD trains (
"precisions"; BF16 and FP8 for Llama-3.1-70B on gfx950). One missing from either phase, or from both, is a "no comparison" failure rather than a green run on the precisions that are left.Phase timeout. Each phase's madengine timeout is the allocation's
time_limit, sosalloc --timeis the only cap; a null card timeout would fall back to madengine's 2h default.Cost. Two runs per nightly instead of one.
time_limitgoes 3h→4h (70B) and 2h→3h (scout),--timeout-minutes190→330, and the workflow step 210→360 minutes.At least two nodes. madengine runs a workload inside the allocation only when it spans more than one node; with
--nodes 1each phase would be its ownsbatchand could land elsewhere, so the driver rejects it.RCCL verification needs every node. The candidate passes only with an
RCCL version :banner from each allocated node's log; a node that produced no log fails it instead of going unnoticed.Unusable metrics are dropped. NaN compares false against any threshold and would read as "no regression", so non-finite and non-positive values are discarded before pairing and the phase reports as missing.
A failed baseline still lets the candidate run. The generated
run_phases.shhas noset -e, so the report can say which side broke.Node workspaces. Containers run as root, so the task's own
rm -rfof its workspace leaves their files and madengine's stale-workspace sweep only covers the user's own directories. After the phases the run removes this job'smadengine_job_<jobid>_*workspaces on every node:rm, thensudo -n, thenrmas root inside one of the run's images already on the node (--pull never).WORK_DIR/abis deleted after the upload step.Python environment. The runner pool has Python 3.10, 3.12 and one machine without pip, and the compute nodes run another distro but use this environment through the inherited
PATH. setup-python's build needs glibc 2.35 and the runner's libssl; a conda-forge interpreter needs glibc 2.17 and ships its libraries. Conda's home writes are redirected underWORK_DIR: the runner account's home is a small shared quota and a full one failed the step with EDQUOT.Dependencies
ROCm/madenginedevelop00ee3e8, which carries ROCm/madengine#213 (run a multi-node workload in place inside an existing allocation) and ROCm/madengine#190 (job-scoped node workspaces, stale-results cleanup). Without chore: unset executable permission #213 the innermadengine runwould submit its ownsbatchand both phases would leave the allocation.mad-rccltip7a9a25e.Testing
End to end on ruby (gfx950, 2 nodes,
meta64):The in-place execution path was also validated on a live two-node allocation while developing madengine#213: two runs under one job id with distinct run ids, non-colliding per-node logs, and
srunreaching both nodes.JIRA ID AICOMNET-420