From ee4ea1ad9bf81b0effa3921207f94d58f2a22be0 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Mon, 5 Oct 2026 14:49:28 +0000 Subject: [PATCH 01/26] fix(rccl): score madengine training runs on the running-average throughput _METRIC_ALIASES matched tok_per_s_per_gpu, which madengine fills from the final logged iteration alone. That is a one-sample estimate: against a 50-iteration llama-3.1-70b reference it lands 1.8% low on roughly 3 of 48 iterations, a hair under the 2% regression gate, so a green build could fail on jitter. The tok_per_s_per_gpu_avg row carries Megatron's running average over the measured iterations and stays within 1% of the 50-iteration value from the third iteration onward, on both BF16 and FP8. It also removes a disagreement between the two parsing paths: parse_live_log_metrics() already reads the running average out of the live log, so the structured and fallback paths could score the same run differently. _TFLOPS_METRICS moves for the same reason, and because the field it feeds is already named tflops_avg. Baselines shift slightly, all well inside the gate: on llama-3.1-70b the measured difference is 0.04% on BF16 and 0.5% on FP8. Co-authored-by: Cursor --- projects/rccl/ci/scripts/test_madengine.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index 5ea5eaf49bf1..19bd6f8639ee 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -50,10 +50,17 @@ # madengine's perf_entry_super.json uses short metric names that differ # from our canonical workload-config keys. Map each config key to the # set of madengine metric names we accept as a match. +# +# The ``_avg`` rows hold Megatron's running average over the measured +# iterations; the plain rows hold the final iteration alone. Two reasons to +# score on the average: a single iteration is a one-sample estimate that +# sporadically dips below the 2% regression gate on its own, and +# parse_live_log_metrics() already reads the average from the live log, so +# until now the structured and fallback paths could score one run differently. _METRIC_ALIASES: dict[str, set[str]] = { - "tokens_per_second_per_gpu": {"tok_per_s_per_gpu"}, + "tokens_per_second_per_gpu": {"tok_per_s_per_gpu_avg"}, } -_TFLOPS_METRICS = {"TFLOPS_per_gpu"} +_TFLOPS_METRICS = {"TFLOPS_per_gpu_avg"} REGRESSION_WINDOW = 5 REGRESSION_THRESHOLD_TRAINING = 0.02 # 2% From 1226015cfb70028a02a001fc9bec0d97f30c009a Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Mon, 5 Oct 2026 15:04:21 +0000 Subject: [PATCH 02/26] fix(rccl): point llama-4-scout at its own multiple_results CSV The entry carried perf_primus-megatron-Llama-3.1-70B.csv, copied from the 70B workload it was derived from. MAD writes the scout results to perf_primus-megatron-Llama-4-Scout-17B-16E.csv, so the configured name either found nothing or, when both workloads had run into the same work directory, scored the scout job on the 70B numbers. Co-authored-by: Cursor --- projects/rccl/ci/scripts/test_madengine.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index 19bd6f8639ee..679854f63e78 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -115,7 +115,7 @@ "base_image": "rocm/primus:v26.4", "gpu_target": "gfx950", "metric_key": "tokens_per_second_per_gpu", - "multiple_results": "perf_primus-megatron-Llama-3.1-70B.csv", + "multiple_results": "perf_primus-megatron-Llama-4-Scout-17B-16E.csv", "reference_values": { "2N": 2734, "4N": 2337, From 4d0a68a0835aea2de06e88d36b335313838be182 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Tue, 6 Oct 2026 10:59:59 +0000 Subject: [PATCH 03/26] feat(rccl): score the madengine gate against a same-node baseline The nightly compared one run against the rolling mean of the previous five. Those runs land wherever the scheduler puts them, and the node pair -- not RCCL -- dominates the absolute throughput: across four weeks of history it moves the number by more than the 2% gate, while a repeat on one and the same pair reproduced to a fraction of a percent on two different RCCL builds. The gate was therefore mostly reporting node assignment, and it fired on roughly a third of scored runs. Run the workload twice instead, inside a single allocation: once on the stock base image and once on the same image with the CI-built RCCL laid over it. Score the ratio. Node effects divide out, so the 2% threshold sits well clear of the noise rather than below it. The baseline has to be the stock image. The overlay is tagged per RCCL commit and, with no registry configured, never leaves the node that built it, so "yesterday's overlay" is not reachable. One allocation, not two jobs: a job pinned to another job's nodes is not schedulable until those 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. Each phase gets its own working directory -- madengine writes perf.csv, perf_entry_super.json and slurm_output/ under fixed relative names, so sharing one would let the second run overwrite the first. Both absolute values and their ratio go to the datastore, now with the node list and job id attached, which the four-week analysis had to recover by hand. The madengine pin moves to a fork commit: running a multi-node workload in place inside an existing allocation is not on develop yet. Temporary, and marked as such in the source. Ref: AICOMNET-420 Co-authored-by: Cursor --- .../workflows/therock-rccl-test-madengine.yml | 4 +- projects/rccl/ci/scripts/test_madengine.py | 764 +++++++++++------- 2 files changed, 492 insertions(+), 276 deletions(-) diff --git a/.github/workflows/therock-rccl-test-madengine.yml b/.github/workflows/therock-rccl-test-madengine.yml index 86de5e24c41e..842380d5fdb6 100644 --- a/.github/workflows/therock-rccl-test-madengine.yml +++ b/.github/workflows/therock-rccl-test-madengine.yml @@ -163,8 +163,10 @@ jobs: --output-dir="${OUTPUT_ARTIFACTS_DIR}" \ --rccl + # The step now covers a baseline run and a candidate run inside one + # allocation, plus however long that allocation waits in the queue. - name: Run MADEngine workload - timeout-minutes: 210 + timeout-minutes: 360 env: NCCL_DEBUG: WARN MAD_SECRETS_HFTOKEN: ${{ secrets.HF_TOKEN || '' }} diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index 679854f63e78..fb8aaf55897f 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -4,10 +4,12 @@ This script handles: 1. Installing madengine from source into the CI venv 2. Building a Docker overlay image with the CI-built RCCL - 3. Generating a manifest.json for the requested workload - 4. Running the workload via `madengine run` - 5. Parsing perf.csv results and checking for regressions - 6. Appending results to a JSONL datastore for trend analysis + 3. Generating a manifest.json per A/B phase + 4. Running both phases back to back inside one SLURM allocation: the stock + base image (baseline) and the same image with the CI RCCL laid over it + (candidate) + 5. Scoring the candidate against the baseline measured on the same nodes + 6. Appending both absolute values and their ratio to a JSONL datastore Usage from GitHub Actions (on ruby-linux-slurm-scale-runner): python3 rocm-systems/projects/rccl/ci/scripts/test_madengine.py \ @@ -27,6 +29,7 @@ import logging import os import re +import shlex import shutil import subprocess import sys @@ -62,18 +65,31 @@ } _TFLOPS_METRICS = {"TFLOPS_per_gpu_avg"} -REGRESSION_WINDOW = 5 +# Applied to the candidate/baseline ratio of a single A/B pair, not to a +# rolling mean of absolute throughput. The absolute number is dominated by +# which node pair the scheduler picked: over four weeks of history it varies +# by 2.2% (BF16) and 2.7% (FP8) across pairs, while the one pair that repeated +# reproduced to 0.02% and 0.25% on two different RCCL builds. Comparing two +# runs on the same nodes divides that term out, so 2% is well clear of the +# noise floor rather than below it (AICOMNET-420). REGRESSION_THRESHOLD_TRAINING = 0.02 # 2% REGRESSION_THRESHOLD_INFERENCE = 0.05 # 5% +BASELINE = "baseline" +CANDIDATE = "candidate" + # Kept in its own directory: it doubles as the build context the compute # nodes use, and WORK_DIR holds the venv, the clones and the live sbatch # logs -- tarring a file that is still growing fails the build. OVERLAY_CTX = "overlay_ctx" OVERLAY_DOCKERFILE = "Dockerfile.rccl-overlay" -MADENGINE_REPO = "https://github.com/ROCm/madengine.git" -MADENGINE_REF = "98217cd7ba721f5a5f2a8fb4729120cce1d57eac" # v2.2.0, 2026-09-08 +# TEMPORARY PIN — must move back to ROCm/madengine before this leaves draft. +# The A/B below needs both runs in one allocation, which requires madengine to +# execute a multi-node workload in place instead of submitting its own sbatch +# (ROCm/madengine#213). That is not on develop yet, so the fork is pinned. +MADENGINE_REPO = "https://github.com/mkuznet1/madengine.git" +MADENGINE_REF = "4ece67d7ce32dc7abd503970fd66748f12751903" # madengine#213 MAD_REPO = "https://github.com/ROCm/MAD.git" MAD_REF = "b4b296310e52ba5cd67d898825165b06b60cf9bf" # mad-rccl, 2026-09-08 MAD_BRANCH = "mad-rccl" @@ -99,7 +115,7 @@ }, "slurm_partition": "meta64", "gpus_per_node": 8, - "time_limit": "03:00:00", + "time_limit": "04:00:00", # one allocation, two runs, plus image staging "docker_mounts": {"/dev/infiniband": "/dev/infiniband"}, "docker_run_options": "--privileged --group-add render --shm-size 64G " "--device=/dev/infiniband --cap-add IPC_LOCK " @@ -122,7 +138,7 @@ }, "slurm_partition": "meta64", "gpus_per_node": 8, - "time_limit": "02:00:00", + "time_limit": "03:00:00", # one allocation, two runs, plus image staging "docker_mounts": {"/dev/infiniband": "/dev/infiniband"}, "docker_run_options": "--privileged --group-add render --shm-size 64G " "--device=/dev/infiniband --cap-add IPC_LOCK " @@ -788,9 +804,10 @@ def generate_manifest( overlay_image: str, nodes: int, work_dir: Path, - nodelist: str = "", registry: str = "", rccl_lib: Path | None = None, + run_dir: Path | None = None, + pull_only: bool = False, ) -> Path: """Generate a madengine manifest.json for the workload. @@ -798,6 +815,15 @@ def generate_manifest( deployment config under ``deployment_config``, env vars inside both ``context.docker_env_vars`` and ``deployment_config.env_vars``, mounts in ``context.docker_mounts``. + + *run_dir* is where madengine will be invoked from, and therefore where the + manifest and all of the run's relative outputs (``perf.csv``, + ``perf_entry_super.json``, ``slurm_output/``) land. The A/B gives each + phase its own, so the two runs do not overwrite each other. + + *pull_only* describes the baseline image: a stock registry tag with no + Dockerfile behind it. madengine then pulls it per node instead of taking + the local-image path, which would try to build it and stage a tar. """ gpus_per_node = workload_config["gpus_per_node"] @@ -884,34 +910,45 @@ def generate_manifest( "exclusive": True, "enable_node_check": False, "network_interface": socket_ifname, - **({"nodelist": nodelist} if nodelist else {}), } overlay_dockerfile = work_dir / OVERLAY_CTX / OVERLAY_DOCKERFILE + if pull_only: + image_entry = { + "docker_image": overlay_image, + "local_image": False, + "registry_image": overlay_image, + "registry": None, + "base_docker": overlay_image, + "build_status": "SKIPPED", + "build_duration": 0, + "gpu_vendor": "AMD", + } + else: + image_entry = { + "docker_image": overlay_image, + "local_image": not bool(registry), + "registry_image": overlay_image if registry else None, + "registry": registry or None, + "base_docker": workload_config["base_image"], + "build_status": "SKIPPED", + "build_duration": 0, + "gpu_vendor": "AMD", + # Lets a compute node build the image it can neither find nor + # pull; the context is this file's own directory, overlay_ctx. + # Advertised only when it exists: --skip-overlay-build writes no + # Dockerfile, and a dead path would turn madengine's fallback + # into a guaranteed failure. + **( + {"dockerfile": str(overlay_dockerfile)} + if overlay_dockerfile.is_file() + else {} + ), + } + manifest = { - "built_images": { - image_key: { - "docker_image": overlay_image, - "local_image": not bool(registry), - "registry_image": overlay_image if registry else None, - "registry": registry or None, - "base_docker": workload_config["base_image"], - "build_status": "SKIPPED", - "build_duration": 0, - "gpu_vendor": "AMD", - # Lets a compute node build the image it can neither find nor - # pull; the context is this file's own directory, overlay_ctx. - # Advertised only when it exists: --skip-overlay-build writes no - # Dockerfile, and a dead path would turn madengine's fallback - # into a guaranteed failure. - **( - {"dockerfile": str(overlay_dockerfile)} - if overlay_dockerfile.is_file() - else {} - ), - }, - }, + "built_images": {image_key: image_entry}, "built_models": { image_key: { "name": model_repo, @@ -960,40 +997,111 @@ def generate_manifest( }, } - manifest_path = work_dir / "manifest.json" + manifest_path = (run_dir or work_dir) / "manifest.json" manifest_path.write_text(json.dumps(manifest, indent=2)) log.info("Manifest written to: %s", manifest_path) return manifest_path -def run_madengine( - manifest_path: Path, - output_csv: Path, +def prepare_phase_dir(work_dir: Path, phase: str) -> Path: + """Create the directory madengine will be invoked from for one phase. + + madengine writes ``perf.csv``, ``perf_entry_super.json`` and + ``slurm_output/`` relative to its working directory under fixed names, and + resolves the model's ``scripts/...`` path the same way. Giving each phase + its own directory with its own copy of the scripts is what keeps the two + runs from overwriting each other's results. + """ + run_dir = work_dir / "ab" / phase + run_dir.mkdir(parents=True, exist_ok=True) + + scripts_src = work_dir / "scripts" + scripts_dst = run_dir / "scripts" + if scripts_src.is_dir() and not scripts_dst.exists(): + subprocess.run(["cp", "-r", str(scripts_src), str(scripts_dst)], check=True) + + return run_dir + + +def run_ab_in_one_allocation( + phases: list[tuple[str, Path]], work_dir: Path, - timeout_minutes: int = 120, -) -> int: - """Run madengine with the given manifest and return the exit code. - - The manifest already contains deployment_config with slurm, distributed, - and env_vars sections. madengine merges deployment_config into - additional_context automatically (run_orchestrator.py:225-234), so we - do not need to duplicate those here. + nodes: int, + workload_name: str, + workload_config: dict, + cluster_config: dict, + timeout_minutes: int, + nodelist: str = "", +) -> dict: + """Run every phase back to back inside a single SLURM allocation. + + The phases are only comparable if they land on the same nodes. Which node + pair the scheduler picks moves the absolute throughput by more than the + regression threshold, so two independently scheduled jobs would be + measuring the cluster as much as RCCL (AICOMNET-420). + + Submitting a second job pinned to the first one's nodes does not solve it: + such a job is not schedulable until those exact nodes free up again, and + while it waits it reserves nothing and accrues no priority. On a busy + shared partition that wait is unbounded, so the comparison would sometimes + not happen at all — worse than a noisy gate. The allocation is therefore + taken once and held across both runs. + + madengine picks up SLURM_JOB_ID from salloc and runs the workload in place + through srun instead of submitting its own sbatch (ROCm/madengine#213). + + Returns ``{"exit_codes": {phase: int}, "job_id": str, "nodelist": str}``. """ + alloc_info = work_dir / "allocation.txt" + alloc_info.unlink(missing_ok=True) + + script_lines = [ + "#!/bin/bash", + "# Generated by test_madengine.py. Deliberately no `set -e`: a failed", + "# baseline must still let the candidate run, so the report can say", + "# which side broke.", + 'echo "Allocation: job=${SLURM_JOB_ID} nodes=${SLURM_JOB_NODELIST}"', + 'printf "%s\\n%s\\n" "${SLURM_JOB_ID}" "${SLURM_JOB_NODELIST}" > ' + + shlex.quote(str(alloc_info)), + ] + for phase, run_dir in phases: + script_lines += [ + "", + f'echo "===== phase: {phase} ====="', + f"cd {shlex.quote(str(run_dir))} || exit 1", + "madengine run -m manifest.json -o perf.csv --live-output --verbose", + "echo $? > " + shlex.quote(str(run_dir / "exit_code")), + ] + + script_path = work_dir / "ab" / "run_phases.sh" + script_path.parent.mkdir(parents=True, exist_ok=True) + script_path.write_text("\n".join(script_lines) + "\n") + cmd = [ - "madengine", "run", - "-m", str(manifest_path), - "-o", str(output_csv), - "--live-output", - "--verbose", + "salloc", + "--nodes", str(nodes), + "--ntasks-per-node", "1", + "--exclusive", + "--partition", + cluster_config.get("slurm_partition", workload_config["slurm_partition"]), + "--time", workload_config["time_limit"], + "--job-name", f"rccl-ab-{workload_name}", ] + qos = cluster_config.get("slurm_qos", "") + if qos: + cmd += ["--qos", qos] + if nodelist: + cmd += ["--nodelist", nodelist] + cmd += ["bash", str(script_path)] log.info("Running: %s", " ".join(cmd)) - log.info("Timeout: %d minutes", timeout_minutes) + log.info("Phases: %s", ", ".join(p for p, _ in phases)) + log.info("Timeout: %d minutes (includes time spent queueing)", timeout_minutes) - # Pre-warm: madengine's SLURM deployment validates CLI availability by - # running `madengine --version` itself. A cold import of its heavy - # dependencies (kubernetes, aiohttp, paramiko) off NFS is slow, so run it - # once here to populate the bytecode cache. + # Pre-warm: madengine validates CLI availability by running + # `madengine --version` itself. A cold import of its heavy dependencies + # (kubernetes, aiohttp, paramiko) off NFS is slow, so run it once here to + # populate the bytecode cache. try: subprocess.run(["madengine", "--version"], capture_output=True, timeout=120) except subprocess.TimeoutExpired: @@ -1007,17 +1115,32 @@ def run_madengine( env["MAD_DOCKER_BUILDS"] = str(docker_builds_dir) try: - proc = subprocess.run( - cmd, - cwd=work_dir, - env=env, - timeout=timeout_minutes * 60, - ) - log.info("madengine exit code: %d", proc.returncode) - return proc.returncode + subprocess.run(cmd, cwd=work_dir, env=env, timeout=timeout_minutes * 60) except subprocess.TimeoutExpired: - log.error("madengine timed out after %d minutes", timeout_minutes) - return 124 + log.error("Allocation timed out after %d minutes", timeout_minutes) + + exit_codes = {} + for phase, run_dir in phases: + marker = run_dir / "exit_code" + try: + exit_codes[phase] = int(marker.read_text().strip()) + except (OSError, ValueError): + # No marker means the phase never ran: salloc never got the nodes, + # or the timeout above cut it short. + log.error("Phase %s produced no exit code — it did not run", phase) + exit_codes[phase] = 124 + else: + log.info("Phase %s exit code: %d", phase, exit_codes[phase]) + + job_id, granted_nodes = "", "" + try: + job_id, granted_nodes = alloc_info.read_text().splitlines()[:2] + except (OSError, ValueError): + log.warning("Could not read allocation details from %s", alloc_info) + else: + log.info("Allocation: job=%s nodes=%s", job_id, granted_nodes) + + return {"exit_codes": exit_codes, "job_id": job_id, "nodelist": granted_nodes} def parse_perf_results(work_dir: Path) -> list[dict]: @@ -1128,64 +1251,107 @@ def parse_live_log_metrics(work_dir: Path) -> list[dict]: return runs -def check_regression( - results_dir: Path, - workload_name: str, - scale: str, - current_value: float, - workload_type: str, - precision: str | None = None, -) -> tuple[bool, str]: - """Check if current metric is a regression vs rolling average. +def collect_phase_results(run_dir: Path, metric_key: str) -> list[dict]: + """Per-precision metrics for one phase. - Returns (is_regression, message). + Prefers madengine's structured output and falls back to scraping the live + log. Each entry carries precision, metric value and a pass/fail status, so + the A/B comparison and the datastore writes are driven from one list. """ - datastore = results_dir / PERF_DATASTORE - if not datastore.exists(): - return False, "No historical data yet — skipping regression check" + perf_results = parse_perf_results(run_dir) + results: list[dict] = [] - threshold = ( - REGRESSION_THRESHOLD_TRAINING - if workload_type == "training" - else REGRESSION_THRESHOLD_INFERENCE - ) + if perf_results: + # perf_entry_super.json rows are long-format: metric name is a + # value in the ``metric`` column, performance in ``performance``. + # Filter to the configured metric_key (or its madengine alias) + # and key by precision. + accepted_metrics = _METRIC_ALIASES.get(metric_key, {metric_key}) + for row in perf_results: + if row.get("metric") not in accepted_metrics: + continue + perf_val = row.get("performance", "") + precision = (row.get("training_precision") + or row.get("multi_results", {}).get("precision", "")) + row_status = row.get("status", "") + if not perf_val: + continue + try: + val = float(perf_val) + except (ValueError, TypeError): + continue + results.append({ + "precision": precision, + "metric_value": val, + "status": ("pass" if row_status.upper() in ("", "PASS", "SUCCESS") + else "fail"), + "source": "structured", + }) + log.info("Structured result: %s %s = %.1f (status=%s)", + precision, metric_key, val, row_status) - historical = [] - with open(datastore) as f: - for line in f: - line = line.strip() - if not line: + # Attach TFLOPS from companion rows, keyed by precision. + tflops_by_precision: dict[str, float] = {} + for row in perf_results: + if row.get("metric") not in _TFLOPS_METRICS: continue + prec = (row.get("training_precision") + or row.get("multi_results", {}).get("precision", "")) try: - entry = json.loads(line) - if ( - entry.get("workload") == workload_name - and entry.get("scale") == scale - and entry.get("precision") == precision - and entry.get("status") == "pass" - and entry.get("metric_value") is not None - ): - historical.append(entry["metric_value"]) - except json.JSONDecodeError: + tflops_by_precision[prec] = float(row["performance"]) + except (KeyError, ValueError, TypeError): + pass + for pr in results: + if "tflops_avg" not in pr: + pr["tflops_avg"] = tflops_by_precision.get(pr["precision"]) + + if not results: + for run in parse_live_log_metrics(run_dir): + val = run.get("tokens_per_second_per_gpu") + if val is None: continue + results.append({ + "precision": run.get("precision"), + "metric_value": val, + "tflops_avg": run.get("tflops_avg"), + "status": "pass" if run.get("completed", False) else "fail", + "source": "live_log", + "iter": run.get("iter", 0), + "total": run.get("total", 0), + "log_file": run.get("log_file"), + }) + log.info("Live-log result: %s = %.1f (completed=%s)", + run.get("precision"), val, run.get("completed")) + + return results - if len(historical) < 3: - return False, f"Only {len(historical)} historical data points — need at least 3 for regression check" - window = historical[-REGRESSION_WINDOW:] - rolling_avg = sum(window) / len(window) - if rolling_avg == 0: - return False, "Rolling average is 0 — skipping regression check" - pct_change = (current_value - rolling_avg) / rolling_avg +def check_ab_regression( + baseline: float, + candidate: float, + workload_type: str, +) -> tuple[bool, str]: + """Score the candidate against the baseline measured on the same nodes. + + This replaces the rolling mean of absolute throughput, which could not + separate an RCCL change from a change of node pair. Returns + (is_regression, message). + """ + if baseline <= 0: + return False, "Baseline is not a positive number — cannot compare" + threshold = ( + REGRESSION_THRESHOLD_TRAINING + if workload_type == "training" + else REGRESSION_THRESHOLD_INFERENCE + ) + ratio = candidate / baseline msg = ( - f"Current: {current_value:.1f}, " - f"Rolling avg ({len(window)} runs): {rolling_avg:.1f}, " - f"Change: {pct_change:+.1%}, " - f"Threshold: -{threshold:.0%}" + f"candidate {candidate:.1f} vs baseline {baseline:.1f} = " + f"{ratio:.4f} ({ratio - 1:+.1%}), threshold: -{threshold:.0%}" ) - if pct_change < -threshold: + if ratio - 1 < -threshold: return True, f"REGRESSION DETECTED — {msg}" return False, f"No regression — {msg}" @@ -1231,15 +1397,59 @@ def append_result( run_dir.mkdir(parents=True, exist_ok=True) +def pair_by_precision( + baseline_results: list[dict], + candidate_results: list[dict], +) -> list[dict]: + """Join the two phases into one comparison row per precision. + + A precision present in only one phase still gets a row, with the other + side left as None, so the report can say which side is missing instead of + silently dropping it. + """ + comparisons: dict[str, dict] = {} + for phase, rows in ((BASELINE, baseline_results), (CANDIDATE, candidate_results)): + for row in rows: + precision = row.get("precision") or "" + entry = comparisons.setdefault( + precision, + {"precision": precision, BASELINE: None, CANDIDATE: None}, + ) + entry[phase] = row + + for entry in comparisons.values(): + base, cand = entry[BASELINE], entry[CANDIDATE] + entry["ratio"] = None + if base and cand: + base_val, cand_val = base.get("metric_value"), cand.get("metric_value") + if base_val and cand_val: + entry["ratio"] = cand_val / base_val + + return [comparisons[p] for p in sorted(comparisons)] + + +def _format_phase(row: dict | None) -> str: + """One side of a comparison line, or why it is absent.""" + if not row: + return "missing" + val = row.get("metric_value") + if val is None: + return "no metric" + tflops = row.get("tflops_avg") + suffix = f" ({tflops:.1f} TFLOP/s/GPU)" if tflops else "" + return f"{val:.1f}{suffix} [{row['status']}]" + + def generate_summary_report( workload_name: str, scale: str, exit_code: int, - metric_value: float | None, regression_msg: str, rccl_commit: str, cluster: str, - precision_results: list[dict] | None = None, + comparisons: list[dict] | None = None, + nodelist: str = "", + job_id: str = "", ) -> str: """Generate a plain-text summary report.""" status = "PASSED" if exit_code == 0 else "FAILED" @@ -1253,20 +1463,22 @@ def generate_summary_report( f"Scale: {scale}", f"Cluster: {cluster}", f"RCCL: {rccl_commit}", + f"Nodes: {nodelist or 'unknown'} (job {job_id or 'unknown'})", + "", + "Both numbers below were measured on those same nodes, back to back,", + "inside one allocation. Baseline is the stock image; candidate is the", + "same image with the CI-built RCCL laid over it. Throughput in", + "tok/s/GPU.", "", ] - if precision_results: - for r in precision_results: - prec = r.get("precision", "?") - val = r.get("metric_value") - tflops = r.get("tflops_avg") - val_s = f"{val:.1f}" if val else "N/A" - tflops_s = f"{tflops:.1f}" if tflops else "" - suffix = f" ({tflops_s} TFLOP/s/GPU)" if tflops_s else "" - lines.append(f"{prec:>4}: {val_s} tok/s/GPU{suffix} [{r['status']}]") - elif metric_value is not None: - lines.append(f"Throughput: {metric_value:.1f} tok/s/GPU") + if comparisons: + for c in comparisons: + ratio = c.get("ratio") + ratio_s = f"{ratio:.4f} ({ratio - 1:+.1%})" if ratio else "N/A" + lines.append(f"{c['precision'] or '?':>5}: ratio {ratio_s}") + lines.append(f" baseline {_format_phase(c[BASELINE])}") + lines.append(f" candidate {_format_phase(c[CANDIDATE])}") else: lines.append("Throughput: N/A (workload did not produce metrics)") @@ -1327,8 +1539,9 @@ def main() -> None: parser.add_argument( "--timeout-minutes", type=int, - default=190, - help="Timeout for madengine run in minutes (default: 190)", + default=330, + help="Timeout for the whole allocation, queueing included, in minutes " + "(default: 330 — a 4h allocation plus 90min of queueing)", ) parser.add_argument( "--notify-email", @@ -1405,9 +1618,17 @@ def main() -> None: registry=args.registry, ) - # Step 4: Generate manifest + # Step 4: Generate one manifest per A/B phase + # + # The baseline is the untouched base image, and that is forced rather than + # chosen: the overlay is tagged {base_image}-rccl-{gpu_target}-{commit} and + # with no --registry it never leaves the node that built it, so "yesterday's + # overlay" is not a baseline this job can reach. Scoring the CI RCCL against + # the RCCL already shipping in the image answers the same question and uses + # an image every node can pull. + # # When no registry is configured, the overlay image only exists on the - # node that built it. Pin the SLURM job to that node so madengine can + # node that built it. Pin the allocation to that node so madengine can # find the image locally. nodelist = "" if args.nodes == 1 and not args.registry: @@ -1419,31 +1640,53 @@ def main() -> None: if hostname: nodelist = hostname if nodelist: - log.info("No registry — pinning SLURM job to build node: %s", nodelist) + log.info("No registry — pinning allocation to build node: %s", nodelist) + + phases: list[tuple[str, Path]] = [] + for phase, image, pull_only in ( + (BASELINE, workload_config["base_image"], True), + (CANDIDATE, overlay_image, False), + ): + run_dir = prepare_phase_dir(work_dir, phase) + generate_manifest( + args.workload, + workload_config, + cluster_config, + image, + args.nodes, + work_dir, + registry=args.registry, + # The bind-mount of the CI library is what makes a --skip-overlay-build + # run the candidate; the baseline must keep the image's own copy. + rccl_lib=( + rccl_lib if args.skip_overlay_build and phase == CANDIDATE else None + ), + run_dir=run_dir, + pull_only=pull_only, + ) + phases.append((phase, run_dir)) + + phase_dirs = dict(phases) - manifest_path = generate_manifest( + # Step 5: Run both phases on the same nodes, in one allocation + allocation = run_ab_in_one_allocation( + phases, + work_dir, + args.nodes, args.workload, workload_config, cluster_config, - overlay_image, - args.nodes, - work_dir, + args.timeout_minutes, nodelist=nodelist, - registry=args.registry, - rccl_lib=rccl_lib if args.skip_overlay_build else None, ) + exit_code = max(allocation["exit_codes"].values(), default=1) - # Step 5: Run the workload - output_csv = work_dir / "perf.csv" - exit_code = run_madengine( - manifest_path, output_csv, work_dir, args.timeout_minutes, - ) - - # Step 5b: Verify RCCL replacement (runs in both overlay and bind-mount modes) + # Step 5b: Verify RCCL replacement. Only the candidate is meant to carry the + # CI library — the baseline runs the image's bundled copy by design. rccl_verification_failed = False if rccl_fingerprint.get("version"): rccl_ok, rccl_msg = verify_rccl_replacement( - work_dir, rccl_fingerprint, + phase_dirs[CANDIDATE], rccl_fingerprint, ) if rccl_ok: log.info("RCCL verification: %s", rccl_msg) @@ -1452,147 +1695,108 @@ def main() -> None: rccl_verification_failed = True exit_code = max(exit_code, 1) - # Step 6: Parse results — structured output first, live log fallback - perf_results = parse_perf_results(work_dir) - live_log_runs = parse_live_log_metrics(work_dir) - - # Save run artifacts - run_id = os.environ.get("GITHUB_RUN_ID", "local") - run_artifacts = results_dir / "runs" / run_id - try: - run_artifacts.mkdir(parents=True, exist_ok=True) - for f in ["perf.csv", "perf_entry_super.csv", "perf_entry_super.json"]: - src = work_dir / f - if src.exists(): - shutil.copy2(str(src), str(run_artifacts / f)) - except OSError as exc: - log.warning("Could not save run artifacts to %s: %s", run_artifacts, exc) - - # Build per-precision results from structured output (primary) or - # live-log scraping (fallback). Each entry carries precision, metric - # value, and a pass/fail status so downstream regression checks and - # datastore writes are driven from one list. + # Step 6: Parse each phase's results and save its artifacts metric_key = workload_config["metric_key"] - precision_results: list[dict] = [] - - if perf_results: - # perf_entry_super.json rows are long-format: metric name is a - # value in the ``metric`` column, performance in ``performance``. - # Filter to the configured metric_key (or its madengine alias) - # and key by precision. - accepted_metrics = _METRIC_ALIASES.get(metric_key, {metric_key}) - for row in perf_results: - if row.get("metric") not in accepted_metrics: - continue - perf_val = row.get("performance", "") - precision = (row.get("training_precision") - or row.get("multi_results", {}).get("precision", "")) - row_status = row.get("status", "") - if not perf_val: - continue - try: - val = float(perf_val) - except (ValueError, TypeError): - continue - precision_results.append({ - "precision": precision, - "metric_value": val, - "status": ("pass" if row_status.upper() in ("", "PASS", "SUCCESS") - else "fail"), - "source": "structured", - }) - log.info("Structured result: %s %s = %.1f (status=%s)", - precision, metric_key, val, row_status) - - # Attach TFLOPS from companion rows, keyed by precision. - tflops_by_precision: dict[str, float] = {} - for row in perf_results: - if row.get("metric") not in _TFLOPS_METRICS: - continue - prec = (row.get("training_precision") - or row.get("multi_results", {}).get("precision", "")) - try: - tflops_by_precision[prec] = float(row["performance"]) - except (KeyError, ValueError, TypeError): - pass - for pr in precision_results: - if "tflops_avg" not in pr: - pr["tflops_avg"] = tflops_by_precision.get(pr["precision"]) + phase_results = {} + run_id = os.environ.get("GITHUB_RUN_ID", "local") + for phase, run_dir in phases: + log.info("Collecting %s results from %s", phase, run_dir) + phase_results[phase] = collect_phase_results(run_dir, metric_key) - if not precision_results and live_log_runs: - for run in live_log_runs: - val = run.get("tokens_per_second_per_gpu") - if val is None: - continue - precision_results.append({ - "precision": run.get("precision"), - "metric_value": val, - "tflops_avg": run.get("tflops_avg"), - "status": "pass" if run.get("completed", False) else "fail", - "source": "live_log", - "iter": run.get("iter", 0), - "total": run.get("total", 0), - "log_file": run.get("log_file"), - }) - log.info("Live-log result: %s = %.1f (completed=%s)", - run.get("precision"), val, run.get("completed")) + run_artifacts = results_dir / "runs" / run_id / phase + try: + run_artifacts.mkdir(parents=True, exist_ok=True) + for f in ["perf.csv", "perf_entry_super.csv", "perf_entry_super.json"]: + src = run_dir / f + if src.exists(): + shutil.copy2(str(src), str(run_artifacts / f)) + except OSError as exc: + log.warning("Could not save %s artifacts to %s: %s", + phase, run_artifacts, exc) - metric_value = precision_results[-1]["metric_value"] if precision_results else None + all_rows = [r for rows in phase_results.values() for r in rows] # Override exit_code if training actually completed successfully. # madengine can report failure (exit code 3) when its perf collector # can't parse the output format, even though training ran to completion. - if exit_code != 0 and not rccl_verification_failed and precision_results: - all_pass = all(r["status"] == "pass" for r in precision_results) - has_metric = all(r.get("metric_value") is not None for r in precision_results) - if all_pass and has_metric: + if exit_code != 0 and not rccl_verification_failed and all_rows: + every_phase_scored = all(phase_results.get(p) for p, _ in phases) + all_pass = all(r["status"] == "pass" for r in all_rows) + has_metric = all(r.get("metric_value") is not None for r in all_rows) + if every_phase_scored and all_pass and has_metric: log.info( - "Overriding madengine exit code %d → 0: all %d precision run(s) " - "passed with metrics", - exit_code, len(precision_results), + "Overriding madengine exit code %d → 0: all %d run(s) across " + "both phases passed with metrics", + exit_code, len(all_rows), ) - for r in precision_results: - log.info(" %s: %.1f %s", r.get("precision", "?"), - r["metric_value"], metric_key) exit_code = 0 - # Step 7: Per-precision regression check - regression_msg = "N/A" - is_regression = False - if precision_results: - regression_msgs = [] - for pr in precision_results: - if pr["metric_value"] is not None: - reg, msg = check_regression( - results_dir, args.workload, scale, pr["metric_value"], - workload_config["type"], precision=pr.get("precision"), - ) - regression_msgs.append(f"[{pr.get('precision', '?')}] {msg}") - if reg: - is_regression = True - log.warning(msg) - else: - log.info(msg) - regression_msg = "; ".join(regression_msgs) if regression_msgs else "N/A" - if is_regression: - exit_code = max(exit_code, 1) - - # Step 8: Append result to datastore (one record per precision run) - extra = {"cluster": args.cluster, "overlay_image": overlay_image} - if precision_results: - for pr in precision_results: - append_result( - results_dir, - args.workload, - scale, - pr["metric_value"], - pr["status"], - rccl_commit, - extra=extra, - precision=pr.get("precision"), - tflops=pr.get("tflops_avg"), - tokens_per_sec=pr["metric_value"], + # Step 7: Score the candidate against the baseline, per precision + comparisons = pair_by_precision( + phase_results.get(BASELINE, []), phase_results.get(CANDIDATE, []), + ) + regression_msgs = [] + for c in comparisons: + prec = c["precision"] or "?" + base, cand = c[BASELINE], c[CANDIDATE] + if c["ratio"] is None: + # One side is missing or produced no number. That is a failure of + # the job, not evidence about RCCL, so say so rather than guess. + regression_msgs.append( + f"[{prec}] no comparison — baseline={_format_phase(base)}, " + f"candidate={_format_phase(cand)}" ) + log.error("No comparison possible for %s", prec) + exit_code = max(exit_code, 1) + continue + reg, msg = check_ab_regression( + base["metric_value"], cand["metric_value"], workload_config["type"], + ) + regression_msgs.append(f"[{prec}] {msg}") + if reg: + log.warning(msg) + exit_code = max(exit_code, 1) + else: + log.info(msg) + + if not comparisons: + regression_msgs.append("no metrics from either phase — nothing to compare") + exit_code = max(exit_code, 1) + regression_msg = "; ".join(regression_msgs) + + # Step 8: Append to datastore — one record per phase per precision, with + # absolute values kept for trend analysis and the ratio on the candidate. + extra = { + "cluster": args.cluster, + "overlay_image": overlay_image, + "nodelist": allocation["nodelist"], + "slurm_job_id": allocation["job_id"], + } + phase_images = { + BASELINE: workload_config["base_image"], + CANDIDATE: overlay_image, + } + if comparisons: + for c in comparisons: + for phase in (BASELINE, CANDIDATE): + row = c[phase] + if row is None: + continue + phase_extra = {**extra, "phase": phase, "image": phase_images[phase]} + if phase == CANDIDATE and c["ratio"] is not None: + phase_extra["ratio"] = c["ratio"] + append_result( + results_dir, + args.workload, + scale, + row.get("metric_value"), + row["status"], + rccl_commit, + extra=phase_extra, + precision=c["precision"], + tflops=row.get("tflops_avg"), + tokens_per_sec=row.get("metric_value"), + ) else: append_result( results_dir, @@ -1607,15 +1811,25 @@ def main() -> None: # Step 9: Generate and distribute report status = "pass" if exit_code == 0 else "fail" report = generate_summary_report( - args.workload, scale, exit_code, metric_value, + args.workload, scale, exit_code, regression_msg, rccl_commit, args.cluster, - precision_results=precision_results if precision_results else None, + comparisons=comparisons, + nodelist=allocation["nodelist"], + job_id=allocation["job_id"], ) log.info("\n%s", report) write_github_summary(report) set_github_output("madengine_status", status) - if metric_value is not None: - set_github_output("madengine_metric", f"{metric_value:.1f}") + candidate_values = [ + c[CANDIDATE]["metric_value"] + for c in comparisons + if c[CANDIDATE] and c[CANDIDATE].get("metric_value") is not None + ] + if candidate_values: + set_github_output("madengine_metric", f"{candidate_values[-1]:.1f}") + ratios = [c["ratio"] for c in comparisons if c["ratio"] is not None] + if ratios: + set_github_output("madengine_ratio", f"{min(ratios):.4f}") summary_path = work_dir / "madengine_summary.txt" summary_path.write_text(report) From ec19e97a0cc640c4a0bdd76f805e41b94adb19d3 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Tue, 6 Oct 2026 14:16:47 +0000 Subject: [PATCH 04/26] ci(rccl): bump the madengine pin to 10a0414 (madengine#213) Picks up the in-place fixes the A/B depends on: the outer wait now honours the model card instead of capping each phase at 2h when no --timeout is passed, a failed phase no longer scancels the shared allocation, and a timed-out phase stops its srun step. Co-authored-by: Cursor --- projects/rccl/ci/scripts/test_madengine.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index fb8aaf55897f..09a0effa05e7 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -89,7 +89,7 @@ # execute a multi-node workload in place instead of submitting its own sbatch # (ROCm/madengine#213). That is not on develop yet, so the fork is pinned. MADENGINE_REPO = "https://github.com/mkuznet1/madengine.git" -MADENGINE_REF = "4ece67d7ce32dc7abd503970fd66748f12751903" # madengine#213 +MADENGINE_REF = "10a0414b644d204e45437ab01d9e795176e0ee4f" # madengine#213 MAD_REPO = "https://github.com/ROCm/MAD.git" MAD_REF = "b4b296310e52ba5cd67d898825165b06b60cf9bf" # mad-rccl, 2026-09-08 MAD_BRANCH = "mad-rccl" From 4cb6bbd903144009ef14a9574058e8023ad75664 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Tue, 6 Oct 2026 15:09:17 +0000 Subject: [PATCH 05/26] ci(rccl): run madengine from a conda env the compute nodes can execute Every compute node runs `madengine run` itself, from the environment it inherits through PATH. The venv sat on setup-python's 3.12, an Ubuntu build that needs glibc 2.35 and the runner's libssl, so it does not start on the nodes; a template patch papered over that by building a node-local venv and pip-installing madengine on every node of every job. Since the A/B moved the manifest into ab// that patch also looked for the madengine source in the wrong directory, and both phases of run 37466645356 died with "madengine source not found". - Replace setup-python + venv with a pinned, checksummed Miniforge under WORK_DIR and `conda create -p env python=3.12`. A conda-forge interpreter needs glibc 2.17 and carries its own libssl, so the same environment runs on the runner and on the nodes; the cleanup step still removes it. - Drop the per-node install patch. The stock template check now passes. - Drop patches that no longer do anything at the pinned madengine: $HOME/.local/bin is in the template already, NFS detection matches nfs[0-9]*, the multi-node SLURM_TMPDIR line carries the run suffix (so the anchor no longer matches), and run_orchestrator already wraps `yum info` in a timeout. - Replace the --gpus-per-node patch with madengine's skip_gpus_directive. Co-authored-by: Cursor --- .../workflows/therock-rccl-test-madengine.yml | 39 ++-- projects/rccl/ci/scripts/test_madengine.py | 170 +----------------- 2 files changed, 28 insertions(+), 181 deletions(-) diff --git a/.github/workflows/therock-rccl-test-madengine.yml b/.github/workflows/therock-rccl-test-madengine.yml index 842380d5fdb6..eef9ac5693f8 100644 --- a/.github/workflows/therock-rccl-test-madengine.yml +++ b/.github/workflows/therock-rccl-test-madengine.yml @@ -101,19 +101,28 @@ jobs: exit 1 fi - - name: Set up Python - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 - with: - python-version: '3.12' - - # A venv on top of it: these runners are shared and long-lived, so anything - # installed straight into the interpreter would persist into the next job. - # This one lives under WORK_DIR and the cleanup step deletes it. - - name: Create Python virtual environment + # The pool is not uniform -- 3.10, 3.12, and one machine with no pip at + # all -- and the compute nodes run another distro again, yet they run + # madengine from this environment through the inherited PATH. + # setup-python's build needs glibc 2.35 and the runner's own libssl, so + # it does not start there; a conda-forge interpreter needs glibc 2.17 + # and carries its libraries. It lives under WORK_DIR, which the nodes + # see, and the cleanup step deletes it: these runners are shared and + # long-lived. + - name: Create Python environment + env: + MINIFORGE_VERSION: "26.7.2-0" + MINIFORGE_SHA256: "281b0ac7d550802efc81af633225a5e6116d29ae72f3ab4eae7168c3931a4c05" run: | mkdir -p "${WORK_DIR}" - python3 -m venv "${WORK_DIR}/venv" - echo "${WORK_DIR}/venv/bin" >> "$GITHUB_PATH" + installer="${WORK_DIR}/Miniforge3.sh" + curl -fsSL -o "${installer}" \ + "https://github.com/conda-forge/miniforge/releases/download/${MINIFORGE_VERSION}/Miniforge3-${MINIFORGE_VERSION}-Linux-x86_64.sh" + echo "${MINIFORGE_SHA256} ${installer}" | sha256sum -c - + bash "${installer}" -b -p "${WORK_DIR}/conda" + rm -f "${installer}" + "${WORK_DIR}/conda/bin/conda" create -y -q -p "${WORK_DIR}/env" python=3.12 + echo "${WORK_DIR}/env/bin" >> "$GITHUB_PATH" # The datastore is only appended to at the very end of the workload, so # an unwritable RESULTS_DIR would surface three hours in. A directory @@ -171,7 +180,6 @@ jobs: NCCL_DEBUG: WARN MAD_SECRETS_HFTOKEN: ${{ secrets.HF_TOKEN || '' }} run: | - source "${WORK_DIR}/venv/bin/activate" export PYTHONPATH="rocm-systems/projects/rccl/ci/scripts:${PYTHONPATH}" NOTIFY_ARGS="" @@ -213,9 +221,6 @@ jobs: - name: Cleanup work directory if: always() run: | - # node_venv is the per-node bootstrap: on a single-node run the template - # puts WORKSPACE on the shared filesystem, so it lands here rather than - # in node-local storage and would otherwise outlive the run. - rm -rf "${WORK_DIR}/madengine" "${WORK_DIR}/MAD" "${WORK_DIR}/venv" \ - "${WORK_DIR}/node_venv" "${WORK_DIR}/overlay_ctx" \ + rm -rf "${WORK_DIR}/madengine" "${WORK_DIR}/MAD" "${WORK_DIR}/conda" \ + "${WORK_DIR}/env" "${WORK_DIR}/overlay_ctx" \ "${WORK_DIR}/docker_builds" diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index 09a0effa05e7..23fc4ffb5331 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -2,7 +2,7 @@ """Run MADEngine AI workloads against CI-built RCCL and track performance. This script handles: - 1. Installing madengine from source into the CI venv + 1. Installing madengine from source into the CI Python environment 2. Building a Docker overlay image with the CI-built RCCL 3. Generating a manifest.json per A/B phase 4. Running both phases back to back inside one SLURM allocation: the stock @@ -79,8 +79,8 @@ CANDIDATE = "candidate" # Kept in its own directory: it doubles as the build context the compute -# nodes use, and WORK_DIR holds the venv, the clones and the live sbatch -# logs -- tarring a file that is still growing fails the build. +# nodes use, and WORK_DIR holds the Python environment, the clones and the +# live sbatch logs -- tarring a file that is still growing fails the build. OVERLAY_CTX = "overlay_ctx" OVERLAY_DOCKERFILE = "Dockerfile.rccl-overlay" @@ -245,142 +245,10 @@ def install_madengine(work_dir: Path) -> Path: return madengine_dir -def patch_madengine_for_cluster( - madengine_dir: Path, - no_gres: bool = False, -) -> None: +def patch_madengine_for_cluster(madengine_dir: Path) -> None: """Patch madengine source for cluster-specific compatibility.""" src = madengine_dir / "src" / "madengine" - if no_gres: - template = src / "deployment" / "templates" / "slurm" / "job.sh.j2" - if not template.exists(): - log.warning("SLURM template not found at %s", template) - else: - content = template.read_text() - patched = content.replace( - "#SBATCH --gpus-per-node={{ gpus_per_node }}\n", "" - ) - if patched != content: - template.write_text(patched) - log.info("Patched SLURM template: removed --gpus-per-node directive") - else: - log.info("SLURM template already patched (no --gpus-per-node)") - - template = src / "deployment" / "templates" / "slurm" / "job.sh.j2" - if template.exists(): - content = template.read_text() - marker = "# Load required modules" - if marker in content and "$HOME/.local/bin" not in content: - patched = content.replace( - marker, - 'export PATH="$HOME/.local/bin:$PATH"\n\n' + marker, - ) - template.write_text(patched) - log.info( - "Patched SLURM template: added $HOME/.local/bin to PATH " - "(SLURM jobs do not inherit user shell PATH)" - ) - else: - log.info("SLURM template PATH patch already present or marker not found") - - template = src / "deployment" / "templates" / "slurm" / "job.sh.j2" - if template and template.exists(): - content = template.read_text() - # The whole template is the sbatch script, so both verification blocks - # run on a compute node, and both inherit the submission environment - # whose PATH leads with the head node's venv. That interpreter belongs - # to another distro (3.10/3.12 there against 3.9 here), so madengine is - # not usable from it and both blocks need the node-local bootstrap. - verify_str = 'echo "Verifying madengine availability..."' - heredoc_idx = content.find("TASK_SCRIPT_EOF") - # The single-node block is bounded by the heredoc: the multi-node pass - # inserts text starting with verify_str, so an unbounded search could - # land inside what it just patched. - blocks = [ - ("single-node", 0, heredoc_idx, "# Single-node: Create local execution manifest"), - ("multi-node", heredoc_idx, len(content), "# Create local execution manifest"), - ] - # Right to left, so patching one does not move the other's offsets. - for label, search_from, search_to, end_str in sorted( - blocks, key=lambda b: b[1], reverse=True - ): - if search_from < 0 or search_to < 0: - log.warning("Could not locate the %s verification block", label) - continue - verify_idx = content.find(verify_str, search_from, search_to) - end_idx = ( - content.find(end_str, verify_idx, search_to) if verify_idx != -1 else -1 - ) - if verify_idx == -1 or end_idx == -1: - log.warning("Could not locate the %s verification block", label) - continue - replacement = ( - 'echo "Verifying madengine availability..."\n' - 'MAD_CLI_COMMAND=""\n' - 'if command -v madengine >/dev/null 2>&1 && ' - 'madengine --help >/dev/null 2>&1; then\n' - ' MAD_CLI_COMMAND="madengine"\n' - ' echo " ✓ madengine available: ' - '$(madengine --version 2>&1 | head -1)"\n' - 'fi\n' - 'if [ -z "$MAD_CLI_COMMAND" ]; then\n' - # PATH still leads with the head node's venv, whose - # interpreter belongs to another distro. - ' NODE_PYTHON=""\n' - # Building the venv is the probe: `import venv` succeeds on - # distro pythons whose ensurepip is missing, and the failure - # would land under `set -e` before the next candidate is tried. - ' for cand in /usr/bin/python3 /usr/local/bin/python3; do\n' - ' [ -x "$cand" ] || continue\n' - ' rm -rf "$WORKSPACE/node_venv"\n' - ' if "$cand" -m venv "$WORKSPACE/node_venv" >/dev/null 2>&1; then\n' - ' NODE_PYTHON="$cand"; break\n' - ' fi\n' - ' done\n' - ' if [ -z "$NODE_PYTHON" ]; then\n' - ' echo " ✗ no usable python3 on $(hostname)"\n' - ' exit 1\n' - ' fi\n' - ' echo " ⚠ madengine not functional — ' - 'installing for this node\'s Python ' - '($("$NODE_PYTHON" --version 2>&1) at $NODE_PYTHON)"\n' - ' SUBMISSION_DIR={{ manifest_file | dirname }}\n' - ' MADENGINE_SRC="$SUBMISSION_DIR/madengine"\n' - ' if [ -d "$MADENGINE_SRC" ] && [ -f "$MADENGINE_SRC/pyproject.toml" ]; then\n' - ' source "$WORKSPACE/node_venv/bin/activate"\n' - # ~/.cache/pip is on a small NFS home quota. - ' pip install --no-cache-dir --upgrade pip setuptools wheel 2>&1 | tail -3\n' - ' pip install --no-cache-dir "$MADENGINE_SRC" 2>&1 | tail -20\n' - ' if madengine --version >/dev/null 2>&1; then\n' - ' MAD_CLI_COMMAND="madengine"\n' - ' echo " ✓ madengine installed: ' - '$(madengine --version 2>&1 | head -1)"\n' - ' else\n' - ' echo " ✗ madengine install failed"\n' - ' exit 1\n' - ' fi\n' - ' else\n' - ' echo " ✗ madengine source not found at $MADENGINE_SRC"\n' - ' exit 1\n' - ' fi\n' - 'fi\n' - 'echo ""\n\n' - ) - content = content[:verify_idx] + replacement + content[end_idx:] - log.info("Patched SLURM template: per-node madengine install (%s)", label) - template.write_text(content) - - template = src / "deployment" / "templates" / "slurm" / "job.sh.j2" - if template and template.exists(): - content = template.read_text() - old_nfs_pattern = r"\bnfs\b" - new_nfs_pattern = r"\bnfs[0-9]*\b" - if old_nfs_pattern in content and new_nfs_pattern not in content: - content = content.replace(old_nfs_pattern, new_nfs_pattern) - template.write_text(content) - log.info("Patched SLURM template: NFS detection now matches nfs4") - # /var/tmp is persistent and shared, so job-scope the workspace and delete # it after use. Anchor-guarded: a no-op once ROCm/madengine#190 is pinned. template = src / "deployment" / "templates" / "slurm" / "job.sh.j2" @@ -388,13 +256,6 @@ def patch_madengine_for_cluster( content = template.read_text() original = content patches = [ - ( - # Matches the /tmp branch below, job-scoped all along. - "multi-node workspace scoping", - " WORKSPACE=$SLURM_TMPDIR/madengine_node_${SLURM_PROCID}\n", - " WORKSPACE=$SLURM_TMPDIR" - "/madengine_job_${SLURM_JOB_ID}_node_${SLURM_PROCID}\n", - ), ( # Bare SLURM_TMPDIR would rsync the project into /var/tmp. "single-node workspace scoping", @@ -447,23 +308,6 @@ def patch_madengine_for_cluster( if content != original: template.write_text(content) - run_orch = src / "orchestration" / "run_orchestrator.py" - if run_orch.exists(): - content = run_orch.read_text() - patched = content.replace( - 'print(self.console.sh("yum info rocm-libs", canFail=True))', - 'print(self.console.sh("rpm -qi rocm-libs 2>/dev/null ' - '|| echo rocm-libs not installed as RPM", canFail=True))', - ) - if patched != content: - run_orch.write_text(patched) - log.info( - "Patched run_orchestrator.py: replaced 'yum info' with 'rpm -qi' " - "to avoid interactive GPG prompt hang" - ) - else: - log.info("run_orchestrator.py already patched or yum string not found") - def get_rccl_commit(rccl_lib: Path | None = None) -> str: """Derive a unique identifier for the RCCL build. @@ -910,6 +754,7 @@ def generate_manifest( "exclusive": True, "enable_node_check": False, "network_interface": socket_ifname, + "skip_gpus_directive": cluster_config.get("slurm_no_gres", False), } overlay_dockerfile = work_dir / OVERLAY_CTX / OVERLAY_DOCKERFILE @@ -1598,10 +1443,7 @@ def main() -> None: # Step 2: Install madengine madengine_dir = install_madengine(work_dir) - patch_madengine_for_cluster( - madengine_dir, - no_gres=cluster_config.get("slurm_no_gres", False), - ) + patch_madengine_for_cluster(madengine_dir) # Step 3: Build overlay image (or use pre-built) if args.skip_overlay_build: From 2bd3daee427ebacfe9865c24004d6daa5bc3ae9d Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Wed, 7 Oct 2026 10:01:04 +0000 Subject: [PATCH 06/26] ci(rccl): name the baseline and candidate RCCL builds in the madengine summary Read the 'RCCL version :' banner each phase's node logs printed, so the report states which library was actually loaded on each side of the A/B (e.g. 2.28.9-HEAD:d34cbb6 from the stock image vs 2.32.3-develop:90d4836 from the CI build) instead of only the CI run tag. Co-authored-by: Cursor --- projects/rccl/ci/scripts/test_madengine.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index 23fc4ffb5331..95d0522e7059 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -409,6 +409,17 @@ def _split_rccl_version(version: str) -> tuple[str, str, bool]: return m.group(1), (m.group(2) or "").lower(), m.group(3) == "+" +def runtime_rccl_versions(work_dir: Path) -> list[str]: + """Distinct ``RCCL version :`` banners across one phase's node logs.""" + log_dir = work_dir / "slurm_output" + versions: list[str] = [] + for log_file in sorted(log_dir.glob("*node_*.out")) if log_dir.is_dir() else []: + for v in re.findall(r"RCCL version\s*:\s*(\S+)", log_file.read_text(errors="replace")): + if v not in versions: + versions.append(v) + return versions + + def verify_rccl_replacement( work_dir: Path, expected: dict, @@ -1295,9 +1306,11 @@ def generate_summary_report( comparisons: list[dict] | None = None, nodelist: str = "", job_id: str = "", + phase_rccl: dict[str, str] | None = None, ) -> str: """Generate a plain-text summary report.""" status = "PASSED" if exit_code == 0 else "FAILED" + phase_rccl = phase_rccl or {} lines = [ "RCCL MADEngine Workload Test Report", "=" * 40, @@ -1307,7 +1320,8 @@ def generate_summary_report( f"Workload: {workload_name}", f"Scale: {scale}", f"Cluster: {cluster}", - f"RCCL: {rccl_commit}", + f"Baseline: RCCL {phase_rccl.get(BASELINE) or 'unknown'}", + f"Candidate: RCCL {phase_rccl.get(CANDIDATE) or 'unknown'} (CI build {rccl_commit})", f"Nodes: {nodelist or 'unknown'} (job {job_id or 'unknown'})", "", "Both numbers below were measured on those same nodes, back to back,", @@ -1658,6 +1672,10 @@ def main() -> None: comparisons=comparisons, nodelist=allocation["nodelist"], job_id=allocation["job_id"], + phase_rccl={ + phase: ", ".join(runtime_rccl_versions(run_dir)) + for phase, run_dir in phases + }, ) log.info("\n%s", report) write_github_summary(report) From 84a58162326132df38aababf4efd0315cb6d0035 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Wed, 7 Oct 2026 10:46:05 +0000 Subject: [PATCH 07/26] ci(rccl): upload the madengine results from each A/B phase directory The A/B writes perf CSVs, manifests and slurm_output under WORK_DIR/ab//, so the artifact paths at WORK_DIR root matched only madengine_summary.txt. Co-authored-by: Cursor --- .github/workflows/therock-rccl-test-madengine.yml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/workflows/therock-rccl-test-madengine.yml b/.github/workflows/therock-rccl-test-madengine.yml index eef9ac5693f8..b938eea817fd 100644 --- a/.github/workflows/therock-rccl-test-madengine.yml +++ b/.github/workflows/therock-rccl-test-madengine.yml @@ -208,14 +208,14 @@ jobs: # nodes can only be read by someone with cluster access: the job log # shows that SLURM failed, not why. path: | - ${{ env.WORK_DIR }}/perf.csv - ${{ env.WORK_DIR }}/perf_entry_super.csv - ${{ env.WORK_DIR }}/perf_entry_super.json - ${{ env.WORK_DIR }}/perf_super.csv ${{ env.WORK_DIR }}/madengine_summary.txt - ${{ env.WORK_DIR }}/manifest.json - ${{ env.WORK_DIR }}/slurm_output/*.out - ${{ env.WORK_DIR }}/slurm_output/*.err + ${{ env.WORK_DIR }}/ab/*/perf.csv + ${{ env.WORK_DIR }}/ab/*/perf_entry_super.csv + ${{ env.WORK_DIR }}/ab/*/perf_entry_super.json + ${{ env.WORK_DIR }}/ab/*/perf_super.csv + ${{ env.WORK_DIR }}/ab/*/manifest.json + ${{ env.WORK_DIR }}/ab/*/slurm_output/*.out + ${{ env.WORK_DIR }}/ab/*/slurm_output/*.err retention-days: 30 - name: Cleanup work directory From d0fdac6fbb80f0cf3f4c5dfd2d13e3afe6721c47 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Mon, 12 Oct 2026 10:41:44 +0000 Subject: [PATCH 08/26] ci(rccl): pin madengine at #213 rebased on #190; drop the template patch madengine#213 is now stacked on #190, which job-scopes the single-node workspace under SLURM_TMPDIR and removes the node-local one after a successful task. Those are the two edits patch_madengine_for_cluster() made to job.sh.j2, so the patch goes. The new head also reads the job id out of run-tagged workspace names in the stale-workspace sweep. Co-authored-by: Cursor --- projects/rccl/ci/scripts/test_madengine.py | 70 +--------------------- 1 file changed, 2 insertions(+), 68 deletions(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index 95d0522e7059..a8bd72115e64 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -89,7 +89,7 @@ # execute a multi-node workload in place instead of submitting its own sbatch # (ROCm/madengine#213). That is not on develop yet, so the fork is pinned. MADENGINE_REPO = "https://github.com/mkuznet1/madengine.git" -MADENGINE_REF = "10a0414b644d204e45437ab01d9e795176e0ee4f" # madengine#213 +MADENGINE_REF = "95685108f27bd4eb4d44209f5ee2e7e86d5451c3" # madengine#213, on #190 MAD_REPO = "https://github.com/ROCm/MAD.git" MAD_REF = "b4b296310e52ba5cd67d898825165b06b60cf9bf" # mad-rccl, 2026-09-08 MAD_BRANCH = "mad-rccl" @@ -245,70 +245,6 @@ def install_madengine(work_dir: Path) -> Path: return madengine_dir -def patch_madengine_for_cluster(madengine_dir: Path) -> None: - """Patch madengine source for cluster-specific compatibility.""" - src = madengine_dir / "src" / "madengine" - - # /var/tmp is persistent and shared, so job-scope the workspace and delete - # it after use. Anchor-guarded: a no-op once ROCm/madengine#190 is pinned. - template = src / "deployment" / "templates" / "slurm" / "job.sh.j2" - if template.exists(): - content = template.read_text() - original = content - patches = [ - ( - # Bare SLURM_TMPDIR would rsync the project into /var/tmp. - "single-node workspace scoping", - ' WORKSPACE=$SLURM_TMPDIR\n' - ' WORKSPACE_TYPE="local-slurm"\n', - ' WORKSPACE=$SLURM_TMPDIR/madengine_job_${SLURM_JOB_ID:-$$}\n' - ' mkdir -p $WORKSPACE\n' - ' WORKSPACE_TYPE="local-slurm"\n', - ), - ] - # Any upstream removal of the workspace wins: #190 keeps it when the - # task failed or artifacts did not all copy out, and a second - # unconditional delete here would undo that. - if 'rm -rf "$WORKSPACE"' not in content: - patches.append( - ( - # Last in the per-node task script, so artifacts are in - # $NODE_COLLECTION_DIR on shared storage by now. Only the - # node-local workspace: shared-nfs is the submission dir. - "node-local workspace cleanup", - "\nexit $TASK_EXIT\nTASK_SCRIPT_EOF\n", - "\n" - "# Nothing else reclaims this on a persistent" - " SLURM_TMPDIR; a failed task keeps it for the" - " post-mortem.\n" - 'if [ $TASK_EXIT -eq 0 ] && ' - '[ "$WORKSPACE_TYPE" = "local-multinode" ] && ' - '[ -n "$WORKSPACE" ]; then\n' - ' echo "Node ${SLURM_PROCID}: removing local workspace ' - '$WORKSPACE"\n' - ' cd / && rm -rf "$WORKSPACE" || true\n' - "fi\n" - "\n" - "exit $TASK_EXIT\n" - "TASK_SCRIPT_EOF\n", - ) - ) - - for label, old, new in patches: - if old not in content: - log.warning( - "SLURM template: %s anchor not found — already patched " - "upstream, or the template drifted", - label, - ) - continue - content = content.replace(old, new) - log.info("Patched SLURM template: %s", label) - - if content != original: - template.write_text(content) - - def get_rccl_commit(rccl_lib: Path | None = None) -> str: """Derive a unique identifier for the RCCL build. @@ -1455,9 +1391,7 @@ def main() -> None: rccl_fingerprint["size"]) # Step 2: Install madengine - madengine_dir = install_madengine(work_dir) - - patch_madengine_for_cluster(madengine_dir) + install_madengine(work_dir) # Step 3: Build overlay image (or use pre-built) if args.skip_overlay_build: From 81523a980eac88dd4da26f1e0d42fc88f67ffc35 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Mon, 12 Oct 2026 10:42:10 +0000 Subject: [PATCH 09/26] fix(rccl): give each A/B phase the allocation's time limit f7ef205e08 said the bumped pin makes the outer wait honour the model card instead of capping each phase at 2h. It does, but this manifest wrote "timeout": None, and madengine treats a null card timeout as unset, so every phase still got the 7200s default. Write the workload's time_limit in seconds instead, leaving the salloc --time as the only cap. Co-authored-by: Cursor --- projects/rccl/ci/scripts/test_madengine.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index a8bd72115e64..83c36737f14b 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -588,6 +588,12 @@ def build_rccl_overlay_image( return tag +def slurm_time_seconds(limit: str) -> int: + """Seconds in a SLURM ``HH:MM:SS`` time limit.""" + hours, minutes, seconds = (int(part) for part in limit.split(":")) + return (hours * 60 + minutes) * 60 + seconds + + def generate_manifest( workload_name: str, workload_config: dict, @@ -755,7 +761,9 @@ def generate_manifest( "additional_docker_run_options": docker_run_opts, "data": "", "cred": "", - "timeout": None, + # None falls back to madengine's 2h default, which would stop a + # phase early; the allocation's own limit is the real cap. + "timeout": slurm_time_seconds(workload_config["time_limit"]), }, }, "context": { From 165ec7cf401ccf34d86e4253a06f9a7feb7ff29c Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Wed, 14 Oct 2026 10:10:05 +0000 Subject: [PATCH 10/26] ci(rccl): pin madengine on ROCm develop and MAD on the current mad-rccl madengine#190 and #213 are merged, so the in-place A/B no longer needs the fork: pin ROCm/madengine develop at 00ee3e8. It differs from the 9568510 the gate last ran on only by keeping an array task id intact in the workspace sweep and by a k8s test fix. Move MAD to the mad-rccl tip, 7a9a25e. Between the old pin and it, scripts/primus_megatron-lm changes only in its benchmark report and tests; the rest is Primus image bumps, GPT-OSS-120B on 2 nodes (#271), and jax/xdit fixes this gate does not run. Co-authored-by: Cursor --- projects/rccl/ci/scripts/test_madengine.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index 83c36737f14b..cafb1a246bc4 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -84,14 +84,13 @@ OVERLAY_CTX = "overlay_ctx" OVERLAY_DOCKERFILE = "Dockerfile.rccl-overlay" -# TEMPORARY PIN — must move back to ROCm/madengine before this leaves draft. # The A/B below needs both runs in one allocation, which requires madengine to # execute a multi-node workload in place instead of submitting its own sbatch -# (ROCm/madengine#213). That is not on develop yet, so the fork is pinned. -MADENGINE_REPO = "https://github.com/mkuznet1/madengine.git" -MADENGINE_REF = "95685108f27bd4eb4d44209f5ee2e7e86d5451c3" # madengine#213, on #190 +# (ROCm/madengine#213), so the pin must not predate it. +MADENGINE_REPO = "https://github.com/ROCm/madengine.git" +MADENGINE_REF = "00ee3e80c0a308599633caf74e5d91a72164a2fe" # develop, has #190 and #213 MAD_REPO = "https://github.com/ROCm/MAD.git" -MAD_REF = "b4b296310e52ba5cd67d898825165b06b60cf9bf" # mad-rccl, 2026-09-08 +MAD_REF = "7a9a25e742ef60e8deffb6e5fd9b559857d2018d" # mad-rccl, 2026-10-09 MAD_BRANCH = "mad-rccl" WORKLOAD_CONFIGS = { From 02fe825eadae2e7a7b1fdb9e2a725d201cd418e9 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Wed, 14 Oct 2026 10:10:50 +0000 Subject: [PATCH 11/26] fix(rccl): fail the A/B when an expected precision is missing from both phases pair_by_precision() built a row only for a precision that came back, so an FP8 leg missing from both phases produced no row and the gate stayed green on BF16. Seed the rows from the workload's "precisions" list, so a leg disabled upstream shows up as "no comparison" and fails the run. Llama-3.1-70B lists BF16 and FP8, which is what MAD's run.sh trains on gfx950. The datastore append now keys on whether any phase returned a row, so a run with nothing measured still records its fail entry. Co-authored-by: Cursor --- projects/rccl/ci/scripts/test_madengine.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index cafb1a246bc4..0bf7b678514b 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -105,6 +105,7 @@ "gpu_target": "gfx950", "metric_key": "tokens_per_second_per_gpu", "multiple_results": "perf_primus-megatron-Llama-3.1-70B.csv", + "precisions": ["BF16", "FP8"], # what MAD's run.sh trains on gfx950 "reference_values": { "2N": 1685, "4N": 1600, @@ -1199,14 +1200,20 @@ def append_result( def pair_by_precision( baseline_results: list[dict], candidate_results: list[dict], + expected: list[str] | None = None, ) -> list[dict]: """Join the two phases into one comparison row per precision. A precision present in only one phase still gets a row, with the other side left as None, so the report can say which side is missing instead of - silently dropping it. + silently dropping it. So does an expected precision neither phase + returned: a leg disabled upstream must fail the gate, not pass it on the + precisions that are left. """ - comparisons: dict[str, dict] = {} + comparisons: dict[str, dict] = { + precision: {"precision": precision, BASELINE: None, CANDIDATE: None} + for precision in expected or [] + } for phase, rows in ((BASELINE, baseline_results), (CANDIDATE, candidate_results)): for row in rows: precision = row.get("precision") or "" @@ -1531,6 +1538,7 @@ def main() -> None: # Step 7: Score the candidate against the baseline, per precision comparisons = pair_by_precision( phase_results.get(BASELINE, []), phase_results.get(CANDIDATE, []), + expected=workload_config.get("precisions", []), ) regression_msgs = [] for c in comparisons: @@ -1573,7 +1581,7 @@ def main() -> None: BASELINE: workload_config["base_image"], CANDIDATE: overlay_image, } - if comparisons: + if all_rows: for c in comparisons: for phase in (BASELINE, CANDIDATE): row = c[phase] From dfc2388aea0d5edf94273d34a5957798247f1289 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Wed, 14 Oct 2026 10:11:11 +0000 Subject: [PATCH 12/26] fix(rccl): keep each workload's archived results apart; lock the datastore The per-run archive was runs//. Matrix legs share GITHUB_RUN_ID, so a second workload overwrote the first one's perf.csv and perf_entry_super.* there. Add the workload to that path. madengine_results.jsonl stays the single trend file. Those legs append to it in parallel on a shared filesystem, so take an exclusive flock around each write. Co-authored-by: Cursor --- projects/rccl/ci/scripts/test_madengine.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index 0bf7b678514b..f8d12f466f15 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -24,6 +24,7 @@ import argparse import csv +import fcntl import hashlib import json import logging @@ -1188,7 +1189,9 @@ def append_result( if extra: entry.update(extra) + # Matrix legs append in parallel on a shared filesystem. with open(datastore, "a") as f: + fcntl.flock(f, fcntl.LOCK_EX) f.write(json.dumps(entry) + "\n") log.info("Result appended to %s", datastore) @@ -1507,7 +1510,8 @@ def main() -> None: log.info("Collecting %s results from %s", phase, run_dir) phase_results[phase] = collect_phase_results(run_dir, metric_key) - run_artifacts = results_dir / "runs" / run_id / phase + # Matrix legs share GITHUB_RUN_ID, so the workload keeps them apart. + run_artifacts = results_dir / "runs" / run_id / args.workload / phase try: run_artifacts.mkdir(parents=True, exist_ok=True) for f in ["perf.csv", "perf_entry_super.csv", "perf_entry_super.json"]: From c27329f103da562d14f51b83eb694e8f8cd28038 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Wed, 14 Oct 2026 15:49:22 +0000 Subject: [PATCH 13/26] docs(rccl): state what the madengine gate does, not how it got there Comments narrated the change ("until now", "This replaces", "now covers") or repeated the node-pair rationale that run_ab_in_one_allocation() already gives. Keep only what holds for the code as it stands. Co-authored-by: Cursor --- .../workflows/therock-rccl-test-madengine.yml | 3 +- projects/rccl/ci/scripts/test_madengine.py | 29 +++++-------------- 2 files changed, 8 insertions(+), 24 deletions(-) diff --git a/.github/workflows/therock-rccl-test-madengine.yml b/.github/workflows/therock-rccl-test-madengine.yml index b938eea817fd..e6c95a6e967a 100644 --- a/.github/workflows/therock-rccl-test-madengine.yml +++ b/.github/workflows/therock-rccl-test-madengine.yml @@ -172,8 +172,7 @@ jobs: --output-dir="${OUTPUT_ARTIFACTS_DIR}" \ --rccl - # The step now covers a baseline run and a candidate run inside one - # allocation, plus however long that allocation waits in the queue. + # Both A/B phases in one allocation, plus its queue wait. - name: Run MADEngine workload timeout-minutes: 360 env: diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index f8d12f466f15..0aaa70803522 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -55,24 +55,15 @@ # from our canonical workload-config keys. Map each config key to the # set of madengine metric names we accept as a match. # -# The ``_avg`` rows hold Megatron's running average over the measured -# iterations; the plain rows hold the final iteration alone. Two reasons to -# score on the average: a single iteration is a one-sample estimate that -# sporadically dips below the 2% regression gate on its own, and -# parse_live_log_metrics() already reads the average from the live log, so -# until now the structured and fallback paths could score one run differently. +# Score the ``_avg`` rows (Megatron's running mean over measured iterations), +# matching parse_live_log_metrics(); the plain rows are one noisy sample. _METRIC_ALIASES: dict[str, set[str]] = { "tokens_per_second_per_gpu": {"tok_per_s_per_gpu_avg"}, } _TFLOPS_METRICS = {"TFLOPS_per_gpu_avg"} -# Applied to the candidate/baseline ratio of a single A/B pair, not to a -# rolling mean of absolute throughput. The absolute number is dominated by -# which node pair the scheduler picked: over four weeks of history it varies -# by 2.2% (BF16) and 2.7% (FP8) across pairs, while the one pair that repeated -# reproduced to 0.02% and 0.25% on two different RCCL builds. Comparing two -# runs on the same nodes divides that term out, so 2% is well clear of the -# noise floor rather than below it (AICOMNET-420). +# Applied to the candidate/baseline ratio of one same-node A/B pair, which +# repeats to within 0.25% (AICOMNET-420). REGRESSION_THRESHOLD_TRAINING = 0.02 # 2% REGRESSION_THRESHOLD_INFERENCE = 0.05 # 5% @@ -1134,9 +1125,7 @@ def check_ab_regression( ) -> tuple[bool, str]: """Score the candidate against the baseline measured on the same nodes. - This replaces the rolling mean of absolute throughput, which could not - separate an RCCL change from a change of node pair. Returns - (is_regression, message). + Returns (is_regression, message). """ if baseline <= 0: return False, "Baseline is not a positive number — cannot compare" @@ -1427,12 +1416,8 @@ def main() -> None: # Step 4: Generate one manifest per A/B phase # - # The baseline is the untouched base image, and that is forced rather than - # chosen: the overlay is tagged {base_image}-rccl-{gpu_target}-{commit} and - # with no --registry it never leaves the node that built it, so "yesterday's - # overlay" is not a baseline this job can reach. Scoring the CI RCCL against - # the RCCL already shipping in the image answers the same question and uses - # an image every node can pull. + # The baseline is the untouched base image: with no --registry an overlay + # never leaves the node that built it, so no earlier overlay is reachable. # # When no registry is configured, the overlay image only exists on the # node that built it. Pin the allocation to that node so madengine can From f85ed45034f596793f478450a8462c21119f4cd3 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Wed, 14 Oct 2026 17:00:44 +0000 Subject: [PATCH 14/26] fix(rccl): cancel the allocation when the A/B times out subprocess.run(timeout=) SIGKILLs only salloc. run_phases.sh and its madengine/srun children kept running on the allocation, while the workflow's cleanup removed env/ under them. Start salloc in its own session; on timeout scancel the job id from allocation.txt, then kill the whole process group. A request still queued has no job id yet, and killing salloc withdraws it. Checked with stub salloc/scancel/madengine and a 1-minute timeout: the old code left run_phases.sh and the hung madengine alive; this one cancels job 4242 and leaves no process behind. Co-authored-by: Cursor --- projects/rccl/ci/scripts/test_madengine.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index 0aaa70803522..ca35da07d3e3 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -32,6 +32,7 @@ import re import shlex import shutil +import signal import subprocess import sys import tempfile @@ -906,10 +907,24 @@ def run_ab_in_one_allocation( docker_builds_dir.mkdir(exist_ok=True) env["MAD_DOCKER_BUILDS"] = str(docker_builds_dir) + # Own process group: killing salloc alone leaves run_phases.sh and its + # madengine/srun children running on the allocation. + proc = subprocess.Popen(cmd, cwd=work_dir, env=env, start_new_session=True) try: - subprocess.run(cmd, cwd=work_dir, env=env, timeout=timeout_minutes * 60) + proc.wait(timeout=timeout_minutes * 60) except subprocess.TimeoutExpired: log.error("Allocation timed out after %d minutes", timeout_minutes) + try: + granted_job = alloc_info.read_text().split()[0] + except (OSError, IndexError): + granted_job = "" # still queued; killing salloc withdraws it + if granted_job: + try: + subprocess.run(["scancel", granted_job], timeout=60) + except (OSError, subprocess.SubprocessError) as exc: + log.warning("scancel %s failed: %s", granted_job, exc) + os.killpg(proc.pid, signal.SIGKILL) + proc.wait() exit_codes = {} for phase, run_dir in phases: From bb20fbce3df90394fa33d2219dd10e3d27ab64e4 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Wed, 14 Oct 2026 18:03:39 +0000 Subject: [PATCH 15/26] fix(rccl): expect BF16 from llama-4-scout too Without a "precisions" list the missing-precision guard had nothing to check for Scout. MAD's run.sh trains Llama-4-Scout-17B-16E on BF16 only on gfx950 (it falls to the default branch), so that is the list. Co-authored-by: Cursor --- projects/rccl/ci/scripts/test_madengine.py | 1 + 1 file changed, 1 insertion(+) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index ca35da07d3e3..36136220e187 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -125,6 +125,7 @@ "gpu_target": "gfx950", "metric_key": "tokens_per_second_per_gpu", "multiple_results": "perf_primus-megatron-Llama-4-Scout-17B-16E.csv", + "precisions": ["BF16"], # what MAD's run.sh trains on gfx950 "reference_values": { "2N": 2734, "4N": 2337, From 76fe4199488f326dab8da8cd05957a5dd4ee4769 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Mon, 19 Oct 2026 14:12:54 +0000 Subject: [PATCH 16/26] fix(rccl): keep conda and pip out of the runner account's home Runs 39238797725 and 39238800940 failed "Create Python environment" with "[Errno 122] Disk quota exceeded" while the work directory's filesystem had terabytes free: the quota that ran out is the runner account's home, which every other job of that account fills too. This job writes almost nothing there, but conda cannot finish without writing something: the Miniforge installer creates ~/.conda, conda create appends the new env to ~/.conda/environments.txt (and rolls the transaction back when that fails), and both cache notices under ~/.cache. pip caches every wheel it fetches under ~/.cache/pip. Point HOME into the conda prefix for that step only, and PIP_CACHE_DIR there for the rest of the job. The cleanup step already deletes the prefix. HOME is not exported to later steps, so whatever reaches the compute nodes still sees the real one. Checked by running this step's body, a pip install and the cleanup step with a read-only HOME: the old step fails at mkdir ~/.conda, the new one passes, HOME stays empty and nothing is left in WORK_DIR. Co-authored-by: Cursor --- .github/workflows/therock-rccl-test-madengine.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/therock-rccl-test-madengine.yml b/.github/workflows/therock-rccl-test-madengine.yml index e6c95a6e967a..a64482f70fe5 100644 --- a/.github/workflows/therock-rccl-test-madengine.yml +++ b/.github/workflows/therock-rccl-test-madengine.yml @@ -114,6 +114,14 @@ jobs: MINIFORGE_VERSION: "26.7.2-0" MINIFORGE_SHA256: "281b0ac7d550802efc81af633225a5e6116d29ae72f3ab4eae7168c3931a4c05" run: | + # The runner account's home is a small quota shared with every other + # job of that account, and conda will not finish without writing + # there: the installer creates ~/.conda, and conda create registers + # the env in it and caches notices in ~/.cache. A full home failed + # this step with EDQUOT. Inside the conda prefix, the cleanup step + # deletes it; only for this step, so the nodes keep the real home. + export HOME="${WORK_DIR}/conda/home" + echo "PIP_CACHE_DIR=${HOME}/.cache/pip" >> "$GITHUB_ENV" mkdir -p "${WORK_DIR}" installer="${WORK_DIR}/Miniforge3.sh" curl -fsSL -o "${installer}" \ From 7cd9603cd60efb6b6627ab69a27be9bd507cad39 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Tue, 20 Oct 2026 15:57:23 +0000 Subject: [PATCH 17/26] fix(rccl-ci): build the madengine overlay only on the compute nodes The runner host built the candidate overlay under a per-run tag that was never removed, and the nodes never used it: with no registry they rebuild the same tag from the manifest's Dockerfile (node 0 builds and saves, the others load). Every runner on that host shares its Docker disk. - Only stage the overlay's Dockerfile and build context on the runner host; drop --registry and the single-node pin to the build host. - Remove this run's built tag from every allocated node after the phases. - Clean up remaining CI tags and the workloads' base images on the runner host, without -f so an image still in use stays. Co-authored-by: Cursor --- .../workflows/therock-rccl-test-madengine.yml | 15 +++ projects/rccl/ci/scripts/test_madengine.py | 119 +++++++----------- 2 files changed, 62 insertions(+), 72 deletions(-) diff --git a/.github/workflows/therock-rccl-test-madengine.yml b/.github/workflows/therock-rccl-test-madengine.yml index a64482f70fe5..93845a997e6e 100644 --- a/.github/workflows/therock-rccl-test-madengine.yml +++ b/.github/workflows/therock-rccl-test-madengine.yml @@ -231,3 +231,18 @@ jobs: rm -rf "${WORK_DIR}/madengine" "${WORK_DIR}/MAD" "${WORK_DIR}/conda" \ "${WORK_DIR}/env" "${WORK_DIR}/overlay_ctx" \ "${WORK_DIR}/docker_builds" + + # Nothing on this host builds or pulls an image any more; the nodes + # build them. Earlier versions did both, under per-run tags nothing + # removed, and every runner on this host shares its Docker disk: a + # pull of the sglang base ran it out of space. Remove those tags and + # the bases they were built from, but not -f, so an image a container + # still uses stays. Layers of a failed pull have no name and stay too. + docker system df || true + { + docker images --format '{{.Repository}}:{{.Tag}}' \ + | grep -E -- '-(rccl-gfx[0-9a-z]+|primus-v[0-9.]+|mad-[0-9a-f]{8})-' || true + python3 -c 'import sys; sys.path.insert(0, sys.argv[1]); import test_madengine as t; print(*sorted({c["base_image"] for c in t.WORKLOAD_CONFIGS.values()}), sep="\n")' \ + rocm-systems/projects/rccl/ci/scripts + } | sort -u | xargs -r docker image rm 2>&1 | grep -v 'No such image' || true + docker system df || true diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index 36136220e187..8f3c00c498e6 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -452,32 +452,21 @@ def _needed_sonames(lib: Path) -> list[str]: return [s for s in sonames if s.startswith("libamd_smi.")] -def build_rccl_overlay_image( +def stage_rccl_overlay( rccl_lib: Path, base_image: str, gpu_target: str, work_dir: Path, - registry: str = "", ) -> str: - """Build a Docker overlay image with the CI-built RCCL and push to registry. + """Stage the candidate image: the base with the CI-built RCCL swapped in. - When a registry is provided, the image is tagged and pushed so that - SLURM compute nodes can pull it automatically. Returns the final - image tag (registry-qualified if pushed). + Writes the Dockerfile and its build context under ``overlay_ctx`` and + returns the tag. Nothing is built here: node 0 of the allocation builds it + from the manifest's Dockerfile and the other nodes load it. """ rccl_commit = get_rccl_commit(rccl_lib) tag = f"{base_image}-rccl-{gpu_target}-{rccl_commit}" - image_exists = subprocess.run( - ["docker", "image", "inspect", tag], - capture_output=True, - ).returncode == 0 - if image_exists: - log.info("Overlay image already exists on head node: %s", tag) - - # Staged even on a cache hit: the manifest points the compute nodes at this - # Dockerfile, and WORK_DIR is new on every attempt while the tag is not, so - # a hit would otherwise advertise a build context that was never written. ctx_dir = work_dir / OVERLAY_CTX ctx_dir.mkdir(exist_ok=True) dockerfile = ctx_dir / OVERLAY_DOCKERFILE @@ -560,25 +549,7 @@ def build_rccl_overlay_image( ENV NCCL_DEBUG=WARN """) - if not image_exists: - log.info("Building overlay image: %s", tag) - subprocess.run( - ["docker", "build", "-t", tag, - "-f", str(dockerfile), str(ctx_dir)], - check=True, - ) - log.info("Overlay image built: %s", tag) - - if registry: - safe_base = base_image.replace("/", "-").replace(":", "-") - push_tag = f"{registry}/rccl-ci:{safe_base}-{rccl_commit}" - log.info("Tagging overlay for registry: %s -> %s", tag, push_tag) - subprocess.run(["docker", "tag", tag, push_tag], check=True) - log.info("Pushing overlay image to registry: %s", push_tag) - subprocess.run(["docker", "push", push_tag], check=True) - log.info("Overlay image pushed: %s", push_tag) - return push_tag - + log.info("Candidate (CI RCCL), built on the nodes: %s", tag) return tag @@ -595,7 +566,6 @@ def generate_manifest( overlay_image: str, nodes: int, work_dir: Path, - registry: str = "", rccl_lib: Path | None = None, run_dir: Path | None = None, pull_only: bool = False, @@ -720,9 +690,9 @@ def generate_manifest( else: image_entry = { "docker_image": overlay_image, - "local_image": not bool(registry), - "registry_image": overlay_image if registry else None, - "registry": registry or None, + "local_image": True, + "registry_image": None, + "registry": None, "base_docker": workload_config["base_image"], "build_status": "SKIPPED", "build_duration": 0, @@ -817,6 +787,20 @@ def prepare_phase_dir(work_dir: Path, phase: str) -> Path: return run_dir +# Run on every node once the phases are done. The images are per-run tags that +# no later run can reuse, and each is tens of GB on a node's local disk. +CLEANUP_IMAGES = """\ +#!/bin/bash +for image in "$@"; do + if docker image rm -f "$image" >/dev/null 2>&1 \\ + || sudo -n docker image rm -f "$image" >/dev/null 2>&1; then + echo " removed $(hostname -s) $image" + fi +done +exit 0 +""" + + def run_ab_in_one_allocation( phases: list[tuple[str, Path]], work_dir: Path, @@ -825,7 +809,7 @@ def run_ab_in_one_allocation( workload_config: dict, cluster_config: dict, timeout_minutes: int, - nodelist: str = "", + built_images: list[str] | None = None, ) -> dict: """Run every phase back to back inside a single SLURM allocation. @@ -844,6 +828,10 @@ def run_ab_in_one_allocation( madengine picks up SLURM_JOB_ID from salloc and runs the workload in place through srun instead of submitting its own sbatch (ROCm/madengine#213). + *built_images* are the tags the nodes build for this run; they are removed + from every node once both phases are done. A run killed by the timeout + leaves them behind. + Returns ``{"exit_codes": {phase: int}, "job_id": str, "nodelist": str}``. """ alloc_info = work_dir / "allocation.txt" @@ -866,6 +854,18 @@ def run_ab_in_one_allocation( "madengine run -m manifest.json -o perf.csv --live-output --verbose", "echo $? > " + shlex.quote(str(run_dir / "exit_code")), ] + if built_images: + cleanup = work_dir / "ab" / "cleanup_images.sh" + cleanup.parent.mkdir(parents=True, exist_ok=True) + cleanup.write_text(CLEANUP_IMAGES) + script_lines += [ + "", + 'echo "===== cleanup: this run\'s images on every allocated node ====="', + 'srun --ntasks="${SLURM_JOB_NUM_NODES}" --ntasks-per-node=1 ' + f"--overlap bash {shlex.quote(str(cleanup))} " + + " ".join(shlex.quote(i) for i in built_images) + + " || true", + ] script_path = work_dir / "ab" / "run_phases.sh" script_path.parent.mkdir(parents=True, exist_ok=True) @@ -884,8 +884,6 @@ def run_ab_in_one_allocation( qos = cluster_config.get("slurm_qos", "") if qos: cmd += ["--qos", qos] - if nodelist: - cmd += ["--nodelist", nodelist] cmd += ["bash", str(script_path)] log.info("Running: %s", " ".join(cmd)) @@ -1372,12 +1370,6 @@ def main() -> None: default="", help="Send summary report to this Teams webhook URL", ) - parser.add_argument( - "--registry", - type=str, - default="", - help="Container registry to push overlay image to (e.g. ghcr.io/rocm/rocm-systems)", - ) parser.add_argument( "--skip-overlay-build", action="store_true", @@ -1415,41 +1407,23 @@ def main() -> None: # Step 2: Install madengine install_madengine(work_dir) - # Step 3: Build overlay image (or use pre-built) + # Step 3: Stage the candidate image (or use pre-built). Images are built + # on the allocation's nodes, never here: this host is shared by every + # runner and has neither the disk for them nor any use for them. if args.skip_overlay_build: if not args.overlay_image: log.error("--skip-overlay-build requires --overlay-image") sys.exit(1) overlay_image = args.overlay_image else: - overlay_image = build_rccl_overlay_image( + overlay_image = stage_rccl_overlay( rccl_lib, workload_config["base_image"], cluster_config["gpu_target"], work_dir, - registry=args.registry, ) # Step 4: Generate one manifest per A/B phase - # - # The baseline is the untouched base image: with no --registry an overlay - # never leaves the node that built it, so no earlier overlay is reachable. - # - # When no registry is configured, the overlay image only exists on the - # node that built it. Pin the allocation to that node so madengine can - # find the image locally. - nodelist = "" - if args.nodes == 1 and not args.registry: - nodelist = os.environ.get("SLURM_NODELIST", "") - if not nodelist: - hostname = subprocess.run( - ["hostname", "-s"], capture_output=True, text=True, - ).stdout.strip() - if hostname: - nodelist = hostname - if nodelist: - log.info("No registry — pinning allocation to build node: %s", nodelist) - phases: list[tuple[str, Path]] = [] for phase, image, pull_only in ( (BASELINE, workload_config["base_image"], True), @@ -1463,7 +1437,6 @@ def main() -> None: image, args.nodes, work_dir, - registry=args.registry, # The bind-mount of the CI library is what makes a --skip-overlay-build # run the candidate; the baseline must keep the image's own copy. rccl_lib=( @@ -1485,7 +1458,9 @@ def main() -> None: workload_config, cluster_config, args.timeout_minutes, - nodelist=nodelist, + # A --skip-overlay-build image is not ours to remove, and the baseline + # is the stock base. + built_images=[] if args.skip_overlay_build else [overlay_image], ) exit_code = max(allocation["exit_codes"].values(), default=1) From 36b8720edeb382d34e4f3b2ca7ae2c8904a288a7 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Thu, 22 Oct 2026 11:07:19 +0000 Subject: [PATCH 18/26] fix(rccl-ci): tighten the same-node A/B gate and its cleanup Review follow-ups: - Reject --nodes < 2. madengine runs a workload inside the allocation only when it spans more than one node; with one, each phase becomes its own sbatch and the two can land on different nodes. - Require a node log from every allocated node before verifying the RCCL banner. A node that produced no log used to go unnoticed. - Drop non-finite and non-positive metrics before pairing. NaN compared false against the threshold, so it read as "no regression" and could clear a failed phase; a dropped row now reports the phase as missing. - Record artifact_run_id and rccl_md5 in the datastore. GITHUB_RUN_ID names the test run, not the RCCL binary it tested. - Remove WORK_DIR/ab after the upload step; it holds the per-node logs and is the only part of the work directory that grows with the run. - Remove this job's node workspaces after the phases. The containers run as root, so the task's own rm leaves their files behind and madengine's stale-workspace sweep only looks at the user's own directories. Falls back to sudo, then to a root container from the base image the node already has. Co-authored-by: Cursor --- .../workflows/therock-rccl-test-madengine.yml | 4 +- projects/rccl/ci/scripts/test_madengine.py | 101 ++++++++++++++---- 2 files changed, 86 insertions(+), 19 deletions(-) diff --git a/.github/workflows/therock-rccl-test-madengine.yml b/.github/workflows/therock-rccl-test-madengine.yml index 93845a997e6e..4eeefee3981a 100644 --- a/.github/workflows/therock-rccl-test-madengine.yml +++ b/.github/workflows/therock-rccl-test-madengine.yml @@ -228,9 +228,11 @@ jobs: - name: Cleanup work directory if: always() run: | + # `ab` holds the per-node logs, the only part that grows with the run; + # the upload step above has already kept them as an artifact. rm -rf "${WORK_DIR}/madengine" "${WORK_DIR}/MAD" "${WORK_DIR}/conda" \ "${WORK_DIR}/env" "${WORK_DIR}/overlay_ctx" \ - "${WORK_DIR}/docker_builds" + "${WORK_DIR}/docker_builds" "${WORK_DIR}/ab" # Nothing on this host builds or pulls an image any more; the nodes # build them. Earlier versions did both, under per-run tags nothing diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index 8f3c00c498e6..15aaf5fbd490 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -28,6 +28,7 @@ import hashlib import json import logging +import math import os import re import shlex @@ -353,13 +354,15 @@ def runtime_rccl_versions(work_dir: Path) -> list[str]: def verify_rccl_replacement( work_dir: Path, expected: dict, + nodes: int, slurm_job_id: str = "", ) -> tuple[bool, str]: """Verify that every node used the CI-built RCCL, not its bundled copy. - Checks *all* ``*node_*.out`` logs for ``RCCL version :`` and compares - against the expected fingerprint. A library that reached only one node - still fails — this is the multi-node failure this job exists to catch. + Requires a ``*_node_.out`` log from each of the *nodes* nodes, checks + *all* of them for ``RCCL version :`` and compares against the expected + fingerprint. A library that reached only one node still fails — this is + the multi-node failure this job exists to catch. Returns (ok, message). Generic across frameworks — any RCCL-based application prints the version string during init. @@ -371,6 +374,17 @@ def verify_rccl_replacement( node_logs = sorted(log_dir.glob("*node_*.out")) if log_dir.is_dir() else [] if not node_logs: return False, "No node logs found — cannot verify RCCL" + logged = { + int(m.group(1)) + for log_file in node_logs + if (m := re.search(r"_node_(\d+)\.out$", log_file.name)) + } + missing = sorted(set(range(nodes)) - logged) + if missing: + return False, ( + f"no log from node(s) {', '.join(map(str, missing))} of {nodes} — " + f"cannot verify RCCL there" + ) verified_nodes: list[str] = [] for log_file in node_logs: @@ -789,8 +803,32 @@ def prepare_phase_dir(work_dir: Path, phase: str) -> Path: # Run on every node once the phases are done. The images are per-run tags that # no later run can reuse, and each is tens of GB on a node's local disk. +# +# The containers run as root, so the task's own `rm -rf` of its workspace leaves +# whatever they wrote, and madengine's stale-workspace sweep only looks at this +# user's directories. Only this job's workspaces are touched. $1 is an image the +# node already has, for when sudo is not available. CLEANUP_IMAGES = """\ #!/bin/bash +helper="$1"; shift +if [ -n "${SLURM_JOB_ID}" ]; then + for ws in "${SLURM_TMPDIR:-/tmp}"/madengine_job_"${SLURM_JOB_ID}"_* \\ + /tmp/madengine_job_"${SLURM_JOB_ID}"_*; do + [ -d "$ws" ] || continue + rm -rf "$ws" 2>/dev/null || sudo -n rm -rf "$ws" 2>/dev/null + if [ -e "$ws" ] && [ -n "$helper" ]; then + args=(run --rm -u 0 -v "$(dirname "$ws"):/w" --entrypoint rm + "$helper" -rf "/w/$(basename "$ws")") + docker "${args[@]}" >/dev/null 2>&1 \\ + || sudo -n docker "${args[@]}" >/dev/null 2>&1 + fi + if [ -e "$ws" ]; then + echo " could not remove $(hostname -s) $ws" + else + echo " removed $(hostname -s) $ws" + fi + done +fi for image in "$@"; do if docker image rm -f "$image" >/dev/null 2>&1 \\ || sudo -n docker image rm -f "$image" >/dev/null 2>&1; then @@ -829,8 +867,8 @@ def run_ab_in_one_allocation( through srun instead of submitting its own sbatch (ROCm/madengine#213). *built_images* are the tags the nodes build for this run; they are removed - from every node once both phases are done. A run killed by the timeout - leaves them behind. + from every node once both phases are done, together with this job's node + workspaces. A run killed by the timeout leaves them behind. Returns ``{"exit_codes": {phase: int}, "job_id": str, "nodelist": str}``. """ @@ -854,18 +892,19 @@ def run_ab_in_one_allocation( "madengine run -m manifest.json -o perf.csv --live-output --verbose", "echo $? > " + shlex.quote(str(run_dir / "exit_code")), ] - if built_images: - cleanup = work_dir / "ab" / "cleanup_images.sh" - cleanup.parent.mkdir(parents=True, exist_ok=True) - cleanup.write_text(CLEANUP_IMAGES) - script_lines += [ - "", - 'echo "===== cleanup: this run\'s images on every allocated node ====="', - 'srun --ntasks="${SLURM_JOB_NUM_NODES}" --ntasks-per-node=1 ' - f"--overlap bash {shlex.quote(str(cleanup))} " - + " ".join(shlex.quote(i) for i in built_images) - + " || true", - ] + cleanup = work_dir / "ab" / "cleanup_images.sh" + cleanup.parent.mkdir(parents=True, exist_ok=True) + cleanup.write_text(CLEANUP_IMAGES) + script_lines += [ + "", + 'echo "===== cleanup: this run\'s workspaces and images on every node ====="', + 'srun --ntasks="${SLURM_JOB_NUM_NODES}" --ntasks-per-node=1 ' + f"--overlap bash {shlex.quote(str(cleanup))} " + + " ".join( + shlex.quote(i) for i in [workload_config["base_image"], *(built_images or [])] + ) + + " || true", + ] script_path = work_dir / "ab" / "run_phases.sh" script_path.parent.mkdir(parents=True, exist_ok=True) @@ -1057,6 +1096,16 @@ def parse_live_log_metrics(work_dir: Path) -> list[dict]: return runs +def _usable_metric(val: float) -> bool: + """Whether a throughput can be scored. + + NaN compares false against any threshold, so it would read as "no + regression", and a dropped row is what makes the A/B report the phase as + missing instead. + """ + return math.isfinite(val) and val > 0 + + def collect_phase_results(run_dir: Path, metric_key: str) -> list[dict]: """Per-precision metrics for one phase. @@ -1086,6 +1135,10 @@ def collect_phase_results(run_dir: Path, metric_key: str) -> list[dict]: val = float(perf_val) except (ValueError, TypeError): continue + if not _usable_metric(val): + log.warning("Ignoring structured result: %s %s = %r", + precision, metric_key, val) + continue results.append({ "precision": precision, "metric_value": val, @@ -1116,6 +1169,10 @@ def collect_phase_results(run_dir: Path, metric_key: str) -> list[dict]: val = run.get("tokens_per_second_per_gpu") if val is None: continue + if not _usable_metric(val): + log.warning("Ignoring live-log result: %s = %r", + run.get("precision"), val) + continue results.append({ "precision": run.get("precision"), "metric_value": val, @@ -1383,6 +1440,11 @@ def main() -> None: ) args = parser.parse_args() + # madengine runs a workload inside this allocation only when it spans more + # than one node; with one, each phase is its own sbatch and the two can land + # on different nodes. + if args.nodes < 2: + parser.error("--nodes must be at least 2 for a same-node A/B") workload_config = WORKLOAD_CONFIGS[args.workload] cluster_config = CLUSTER_CONFIGS[args.cluster] @@ -1469,7 +1531,7 @@ def main() -> None: rccl_verification_failed = False if rccl_fingerprint.get("version"): rccl_ok, rccl_msg = verify_rccl_replacement( - phase_dirs[CANDIDATE], rccl_fingerprint, + phase_dirs[CANDIDATE], rccl_fingerprint, args.nodes, ) if rccl_ok: log.info("RCCL verification: %s", rccl_msg) @@ -1556,6 +1618,9 @@ def main() -> None: "overlay_image": overlay_image, "nodelist": allocation["nodelist"], "slurm_job_id": allocation["job_id"], + # run_id names this test run; these name the RCCL binary it tested. + "artifact_run_id": os.environ.get("ARTIFACT_RUN_ID", ""), + "rccl_md5": rccl_fingerprint["md5"], } phase_images = { BASELINE: workload_config["base_image"], From 3abc26e2dcf7b2ee8627801169c43b807e0c2367 Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Thu, 22 Oct 2026 11:18:30 +0000 Subject: [PATCH 19/26] fix(rccl-ci): never pull an image to clean up a node workspace The root fallback ran rm in the stock base image, which need not be on the node under that tag: an image built FROM the base digest leaves none. A docker run would then pull tens of GB just to delete a directory. Try each of this run's images with --pull never instead, stopping at the first that removes the workspace. Co-authored-by: Cursor --- projects/rccl/ci/scripts/test_madengine.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index 15aaf5fbd490..bbcc4b6529db 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -806,8 +806,9 @@ def prepare_phase_dir(work_dir: Path, phase: str) -> Path: # # The containers run as root, so the task's own `rm -rf` of its workspace leaves # whatever they wrote, and madengine's stale-workspace sweep only looks at this -# user's directories. Only this job's workspaces are touched. $1 is an image the -# node already has, for when sudo is not available. +# user's directories. Only this job's workspaces are touched. Without sudo, rm +# runs as root in whichever of this run's images the node has; never pulled, as +# each is tens of GB. $1 is kept, the rest are removed. CLEANUP_IMAGES = """\ #!/bin/bash helper="$1"; shift @@ -816,12 +817,14 @@ def prepare_phase_dir(work_dir: Path, phase: str) -> Path: /tmp/madengine_job_"${SLURM_JOB_ID}"_*; do [ -d "$ws" ] || continue rm -rf "$ws" 2>/dev/null || sudo -n rm -rf "$ws" 2>/dev/null - if [ -e "$ws" ] && [ -n "$helper" ]; then - args=(run --rm -u 0 -v "$(dirname "$ws"):/w" --entrypoint rm - "$helper" -rf "/w/$(basename "$ws")") + for image in "$helper" "$@"; do + [ -e "$ws" ] || break + [ -n "$image" ] || continue + args=(run --rm --pull never -u 0 -v "$(dirname "$ws"):/w" + --entrypoint rm "$image" -rf "/w/$(basename "$ws")") docker "${args[@]}" >/dev/null 2>&1 \\ || sudo -n docker "${args[@]}" >/dev/null 2>&1 - fi + done if [ -e "$ws" ]; then echo " could not remove $(hostname -s) $ws" else From 9827ce5ec4eca8eb4deb01ff8167224599533b0a Mon Sep 17 00:00:00 2001 From: mkuznet1 Date: Thu, 22 Oct 2026 15:26:53 +0000 Subject: [PATCH 20/26] style(rccl-ci): trim comments to the constraints they state Co-authored-by: Cursor --- .../workflows/therock-rccl-test-madengine.yml | 26 +--- projects/rccl/ci/scripts/test_madengine.py | 118 +++++------------- 2 files changed, 35 insertions(+), 109 deletions(-) diff --git a/.github/workflows/therock-rccl-test-madengine.yml b/.github/workflows/therock-rccl-test-madengine.yml index 4eeefee3981a..bebd3756979c 100644 --- a/.github/workflows/therock-rccl-test-madengine.yml +++ b/.github/workflows/therock-rccl-test-madengine.yml @@ -101,25 +101,15 @@ jobs: exit 1 fi - # The pool is not uniform -- 3.10, 3.12, and one machine with no pip at - # all -- and the compute nodes run another distro again, yet they run - # madengine from this environment through the inherited PATH. - # setup-python's build needs glibc 2.35 and the runner's own libssl, so - # it does not start there; a conda-forge interpreter needs glibc 2.17 - # and carries its libraries. It lives under WORK_DIR, which the nodes - # see, and the cleanup step deletes it: these runners are shared and - # long-lived. + # Conda-forge Python under WORK_DIR: the compute nodes (another distro, older + # glibc) run madengine from it via PATH, where setup-python's build won't start. - name: Create Python environment env: MINIFORGE_VERSION: "26.7.2-0" MINIFORGE_SHA256: "281b0ac7d550802efc81af633225a5e6116d29ae72f3ab4eae7168c3931a4c05" run: | - # The runner account's home is a small quota shared with every other - # job of that account, and conda will not finish without writing - # there: the installer creates ~/.conda, and conda create registers - # the env in it and caches notices in ~/.cache. A full home failed - # this step with EDQUOT. Inside the conda prefix, the cleanup step - # deletes it; only for this step, so the nodes keep the real home. + # conda must write to $HOME, a small shared quota; redirect it into the + # conda prefix for this step only, so the nodes keep the real home. export HOME="${WORK_DIR}/conda/home" echo "PIP_CACHE_DIR=${HOME}/.cache/pip" >> "$GITHUB_ENV" mkdir -p "${WORK_DIR}" @@ -234,12 +224,8 @@ jobs: "${WORK_DIR}/env" "${WORK_DIR}/overlay_ctx" \ "${WORK_DIR}/docker_builds" "${WORK_DIR}/ab" - # Nothing on this host builds or pulls an image any more; the nodes - # build them. Earlier versions did both, under per-run tags nothing - # removed, and every runner on this host shares its Docker disk: a - # pull of the sglang base ran it out of space. Remove those tags and - # the bases they were built from, but not -f, so an image a container - # still uses stays. Layers of a failed pull have no name and stay too. + # Remove per-run tags and bases left by earlier versions of this job on + # the shared Docker disk; no -f, so images a container still uses stay. docker system df || true { docker images --format '{{.Repository}}:{{.Tag}}' \ diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index bbcc4b6529db..8ef4541ae515 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -64,23 +64,19 @@ } _TFLOPS_METRICS = {"TFLOPS_per_gpu_avg"} -# Applied to the candidate/baseline ratio of one same-node A/B pair, which -# repeats to within 0.25% (AICOMNET-420). +# Applied to the candidate/baseline ratio of one same-node A/B pair. REGRESSION_THRESHOLD_TRAINING = 0.02 # 2% REGRESSION_THRESHOLD_INFERENCE = 0.05 # 5% BASELINE = "baseline" CANDIDATE = "candidate" -# Kept in its own directory: it doubles as the build context the compute -# nodes use, and WORK_DIR holds the Python environment, the clones and the -# live sbatch logs -- tarring a file that is still growing fails the build. +# Its own directory because it is also the nodes' build context: WORK_DIR has +# live sbatch logs, and tarring a file that is still growing fails the build. OVERLAY_CTX = "overlay_ctx" OVERLAY_DOCKERFILE = "Dockerfile.rccl-overlay" -# The A/B below needs both runs in one allocation, which requires madengine to -# execute a multi-node workload in place instead of submitting its own sbatch -# (ROCm/madengine#213), so the pin must not predate it. +# Must not predate ROCm/madengine#213: the A/B runs madengine inside salloc. MADENGINE_REPO = "https://github.com/ROCm/madengine.git" MADENGINE_REF = "00ee3e80c0a308599633caf74e5d91a72164a2fe" # develop, has #190 and #213 MAD_REPO = "https://github.com/ROCm/MAD.git" @@ -359,10 +355,8 @@ def verify_rccl_replacement( ) -> tuple[bool, str]: """Verify that every node used the CI-built RCCL, not its bundled copy. - Requires a ``*_node_.out`` log from each of the *nodes* nodes, checks - *all* of them for ``RCCL version :`` and compares against the expected - fingerprint. A library that reached only one node still fails — this is - the multi-node failure this job exists to catch. + Requires a ``*_node_.out`` log from each of the *nodes* nodes; a library + that reached only some of them fails. Returns (ok, message). Generic across frameworks — any RCCL-based application prints the version string during init. @@ -472,11 +466,9 @@ def stage_rccl_overlay( gpu_target: str, work_dir: Path, ) -> str: - """Stage the candidate image: the base with the CI-built RCCL swapped in. + """Stage the candidate image (base + CI-built RCCL) under ``overlay_ctx``. - Writes the Dockerfile and its build context under ``overlay_ctx`` and - returns the tag. Nothing is built here: node 0 of the allocation builds it - from the manifest's Dockerfile and the other nodes load it. + Returns the tag. Nothing is built here; the allocation's nodes build it. """ rccl_commit = get_rccl_commit(rccl_lib) tag = f"{base_image}-rccl-{gpu_target}-{rccl_commit}" @@ -591,14 +583,8 @@ def generate_manifest( ``context.docker_env_vars`` and ``deployment_config.env_vars``, mounts in ``context.docker_mounts``. - *run_dir* is where madengine will be invoked from, and therefore where the - manifest and all of the run's relative outputs (``perf.csv``, - ``perf_entry_super.json``, ``slurm_output/``) land. The A/B gives each - phase its own, so the two runs do not overwrite each other. - - *pull_only* describes the baseline image: a stock registry tag with no - Dockerfile behind it. madengine then pulls it per node instead of taking - the local-image path, which would try to build it and stage a tar. + *run_dir*: madengine's working directory for this phase; gets the manifest. + *pull_only*: a stock registry image, pulled per node rather than built. """ gpus_per_node = workload_config["gpus_per_node"] @@ -782,13 +768,9 @@ def generate_manifest( def prepare_phase_dir(work_dir: Path, phase: str) -> Path: - """Create the directory madengine will be invoked from for one phase. + """Create one phase's madengine working directory, with its own scripts copy. - madengine writes ``perf.csv``, ``perf_entry_super.json`` and - ``slurm_output/`` relative to its working directory under fixed names, and - resolves the model's ``scripts/...`` path the same way. Giving each phase - its own directory with its own copy of the scripts is what keeps the two - runs from overwriting each other's results. + madengine writes its outputs under fixed names relative to its cwd. """ run_dir = work_dir / "ab" / phase run_dir.mkdir(parents=True, exist_ok=True) @@ -801,14 +783,8 @@ def prepare_phase_dir(work_dir: Path, phase: str) -> Path: return run_dir -# Run on every node once the phases are done. The images are per-run tags that -# no later run can reuse, and each is tens of GB on a node's local disk. -# -# The containers run as root, so the task's own `rm -rf` of its workspace leaves -# whatever they wrote, and madengine's stale-workspace sweep only looks at this -# user's directories. Only this job's workspaces are touched. Without sudo, rm -# runs as root in whichever of this run's images the node has; never pulled, as -# each is tens of GB. $1 is kept, the rest are removed. +# Run on every node after both phases. Container output is root-owned, so the +# workspace is removed from inside a local image (never pulled); $1 is kept. CLEANUP_IMAGES = """\ #!/bin/bash helper="$1"; shift @@ -852,26 +828,9 @@ def run_ab_in_one_allocation( timeout_minutes: int, built_images: list[str] | None = None, ) -> dict: - """Run every phase back to back inside a single SLURM allocation. - - The phases are only comparable if they land on the same nodes. Which node - pair the scheduler picks moves the absolute throughput by more than the - regression threshold, so two independently scheduled jobs would be - measuring the cluster as much as RCCL (AICOMNET-420). - - Submitting a second job pinned to the first one's nodes does not solve it: - such a job is not schedulable until those exact nodes free up again, and - while it waits it reserves nothing and accrues no priority. On a busy - shared partition that wait is unbounded, so the comparison would sometimes - not happen at all — worse than a noisy gate. The allocation is therefore - taken once and held across both runs. - - madengine picks up SLURM_JOB_ID from salloc and runs the workload in place - through srun instead of submitting its own sbatch (ROCm/madengine#213). + """Run every phase back to back inside one SLURM allocation, on the same nodes. - *built_images* are the tags the nodes build for this run; they are removed - from every node once both phases are done, together with this job's node - workspaces. A run killed by the timeout leaves them behind. + *built_images* are removed from every node afterwards (not on a timeout). Returns ``{"exit_codes": {phase: int}, "job_id": str, "nodelist": str}``. """ @@ -880,9 +839,8 @@ def run_ab_in_one_allocation( script_lines = [ "#!/bin/bash", - "# Generated by test_madengine.py. Deliberately no `set -e`: a failed", - "# baseline must still let the candidate run, so the report can say", - "# which side broke.", + "# Generated by test_madengine.py. No `set -e`: a failed baseline must", + "# still let the candidate run.", 'echo "Allocation: job=${SLURM_JOB_ID} nodes=${SLURM_JOB_NODELIST}"', 'printf "%s\\n%s\\n" "${SLURM_JOB_ID}" "${SLURM_JOB_NODELIST}" > ' + shlex.quote(str(alloc_info)), @@ -932,10 +890,8 @@ def run_ab_in_one_allocation( log.info("Phases: %s", ", ".join(p for p, _ in phases)) log.info("Timeout: %d minutes (includes time spent queueing)", timeout_minutes) - # Pre-warm: madengine validates CLI availability by running - # `madengine --version` itself. A cold import of its heavy dependencies - # (kubernetes, aiohttp, paramiko) off NFS is slow, so run it once here to - # populate the bytecode cache. + # Pre-warm the bytecode cache: madengine runs `madengine --version` itself, + # and a cold import of its dependencies off NFS is slow. try: subprocess.run(["madengine", "--version"], capture_output=True, timeout=120) except subprocess.TimeoutExpired: @@ -1100,22 +1056,12 @@ def parse_live_log_metrics(work_dir: Path) -> list[dict]: def _usable_metric(val: float) -> bool: - """Whether a throughput can be scored. - - NaN compares false against any threshold, so it would read as "no - regression", and a dropped row is what makes the A/B report the phase as - missing instead. - """ + """Whether a throughput can be scored; NaN would pass any threshold.""" return math.isfinite(val) and val > 0 def collect_phase_results(run_dir: Path, metric_key: str) -> list[dict]: - """Per-precision metrics for one phase. - - Prefers madengine's structured output and falls back to scraping the live - log. Each entry carries precision, metric value and a pass/fail status, so - the A/B comparison and the datastore writes are driven from one list. - """ + """Per-precision metrics for one phase: structured output, else the live log.""" perf_results = parse_perf_results(run_dir) results: list[dict] = [] @@ -1270,11 +1216,8 @@ def pair_by_precision( ) -> list[dict]: """Join the two phases into one comparison row per precision. - A precision present in only one phase still gets a row, with the other - side left as None, so the report can say which side is missing instead of - silently dropping it. So does an expected precision neither phase - returned: a leg disabled upstream must fail the gate, not pass it on the - precisions that are left. + A precision missing from a phase, or *expected* but absent from both, keeps + a row with None so the gate fails instead of skipping it. """ comparisons: dict[str, dict] = { precision: {"precision": precision, BASELINE: None, CANDIDATE: None} @@ -1443,9 +1386,8 @@ def main() -> None: ) args = parser.parse_args() - # madengine runs a workload inside this allocation only when it spans more - # than one node; with one, each phase is its own sbatch and the two can land - # on different nodes. + # madengine runs inside our allocation only for multi-node workloads; a + # single-node phase gets its own sbatch, possibly on other nodes. if args.nodes < 2: parser.error("--nodes must be at least 2 for a same-node A/B") @@ -1472,9 +1414,8 @@ def main() -> None: # Step 2: Install madengine install_madengine(work_dir) - # Step 3: Stage the candidate image (or use pre-built). Images are built - # on the allocation's nodes, never here: this host is shared by every - # runner and has neither the disk for them nor any use for them. + # Step 3: Stage the candidate image (or use pre-built). Images are built on + # the allocation's nodes, never here: this host's disk is shared by runners. if args.skip_overlay_build: if not args.overlay_image: log.error("--skip-overlay-build requires --overlay-image") @@ -1590,8 +1531,7 @@ def main() -> None: prec = c["precision"] or "?" base, cand = c[BASELINE], c[CANDIDATE] if c["ratio"] is None: - # One side is missing or produced no number. That is a failure of - # the job, not evidence about RCCL, so say so rather than guess. + # A missing side is a job failure, not evidence about RCCL. regression_msgs.append( f"[{prec}] no comparison — baseline={_format_phase(base)}, " f"candidate={_format_phase(cand)}" From 19be2dee47f634838072249e3c198ab3344c8e14 Mon Sep 17 00:00:00 2001 From: i-kosarev Date: Thu, 29 Oct 2026 10:45:23 +0000 Subject: [PATCH 21/26] fix(rccl-ci): give the nodes back when the driver is interrupted salloc runs in a session of its own, so a signal to the driver never reaches it, and only the timeout path cancelled the job. A cancelled workflow left the allocation holding its nodes until --time expired: four exclusive ones for five hours in the 405B case. Cancelling and killing the group moves into one function, called from the timeout path and from BaseException alike. SIGTERM gets a handler because it ends the interpreter without unwinding, so no `except` or `finally` would otherwise run -- and Actions escalates SIGINT to SIGTERM when a run is cancelled. SIGKILL, which follows, cannot be caught; the allocation's own --time stays the backstop for that. With stub SLURM commands, SIGINT and SIGTERM each left the allocation alive and scancel uncalled before this, and release both after. --- projects/rccl/ci/scripts/test_madengine.py | 40 +++++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index 8ef4541ae515..ee262a9a570d 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -818,6 +818,11 @@ def prepare_phase_dir(work_dir: Path, phase: str) -> Path: """ +def _raise_on_term(signum, frame): # noqa: ARG001 - signal handler signature + """Make SIGTERM unwind the stack the way SIGINT already does.""" + raise KeyboardInterrupt(f"signal {signum}") + + def run_ab_in_one_allocation( phases: list[tuple[str, Path]], work_dir: Path, @@ -907,10 +912,16 @@ def run_ab_in_one_allocation( # Own process group: killing salloc alone leaves run_phases.sh and its # madengine/srun children running on the allocation. proc = subprocess.Popen(cmd, cwd=work_dir, env=env, start_new_session=True) - try: - proc.wait(timeout=timeout_minutes * 60) - except subprocess.TimeoutExpired: - log.error("Allocation timed out after %d minutes", timeout_minutes) + + def release_allocation(reason: str) -> None: + """Give the nodes back: cancel the job, then kill the whole group. + + The group is in a session of its own, so nothing the caller is killed + by reaches it. Whatever ends this function has to do it explicitly or + the allocation keeps the nodes until its own --time expires -- four + exclusive ones for five hours, in the 405B case. + """ + log.error("Releasing the allocation: %s", reason) try: granted_job = alloc_info.read_text().split()[0] except (OSError, IndexError): @@ -920,9 +931,28 @@ def run_ab_in_one_allocation( subprocess.run(["scancel", granted_job], timeout=60) except (OSError, subprocess.SubprocessError) as exc: log.warning("scancel %s failed: %s", granted_job, exc) - os.killpg(proc.pid, signal.SIGKILL) + try: + os.killpg(proc.pid, signal.SIGKILL) + except ProcessLookupError: + pass proc.wait() + # Actions cancels a run with SIGINT and escalates to SIGTERM. SIGINT + # already arrives as KeyboardInterrupt, but SIGTERM ends the interpreter + # without unwinding, so neither `except` nor `finally` would run: turn it + # into the same exception. SIGKILL, which follows, cannot be caught at + # all -- the allocation's own --time is the only backstop left for it. + previous_term = signal.signal(signal.SIGTERM, _raise_on_term) + try: + proc.wait(timeout=timeout_minutes * 60) + except subprocess.TimeoutExpired: + release_allocation(f"timed out after {timeout_minutes} minutes") + except BaseException: + release_allocation("the driver was interrupted") + raise + finally: + signal.signal(signal.SIGTERM, previous_term) + exit_codes = {} for phase, run_dir in phases: marker = run_dir / "exit_code" From 614ff32d1de8db526daf29d288c725ec6057f3ff Mon Sep 17 00:00:00 2001 From: i-kosarev Date: Thu, 29 Oct 2026 16:41:40 +0000 Subject: [PATCH 22/26] ci(rccl): release the allocation from a step the runner itself starts The signal handling added in 19be2dee47f does not cover a cancelled workflow. Measured on run 40879865705: the allocation was granted, the run was cancelled, and SLURM job 104884 kept its four nodes with 4:48 left while the job already read `cancelled`. The log says why -- the runner kills this job's process tree and reports "Terminate orphan process" for madengine and srun. Nothing signals the driver, so no Python handler runs, and salloc holds the allocation in a session of its own. A step with `if: cancelled()` is started by the runner itself, which is the one piece of cleanup a cancellation cannot skip. It reads the job id the script already records in WORK_DIR/allocation.txt -- that file survives, since the cleanup step does not run either -- and scancels it. The Python path stays: it is what releases the allocation on the script's own timeout and on an interactive Ctrl-C, neither of which this step sees. --- .../workflows/therock-rccl-test-madengine.yml | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/.github/workflows/therock-rccl-test-madengine.yml b/.github/workflows/therock-rccl-test-madengine.yml index bebd3756979c..a7e1983a62b3 100644 --- a/.github/workflows/therock-rccl-test-madengine.yml +++ b/.github/workflows/therock-rccl-test-madengine.yml @@ -196,6 +196,33 @@ jobs: --work-dir "${WORK_DIR}" \ $NOTIFY_ARGS + # Cancelling a run does not signal the driver: the runner kills this + # job's process tree outright and then reports "Terminate orphan + # process" for madengine and srun. No Python handler gets to run, and + # salloc holds the allocation in a session of its own, so the nodes stay + # taken until its --time expires -- four of them, for five hours, in the + # 405B case. Measured on run 40879865705: the job reached `cancelled` + # while SLURM job 104884 kept running with 4:48 left. + # + # The runner starts this step itself, which is what makes it the one + # piece of cleanup a cancellation cannot skip. + - name: Release the SLURM allocation + if: cancelled() + run: | + recorded="${WORK_DIR}/allocation.txt" + if [ ! -s "${recorded}" ]; then + echo "No allocation was granted; nothing to release." + exit 0 + fi + job="$(head -n1 "${recorded}" | tr -d '[:space:]')" + case "${job}" in + ''|*[!0-9]*) + echo "::warning::no usable job id in ${recorded}, got '${job}'" + exit 0 ;; + esac + echo "Releasing SLURM job ${job}" + scancel "${job}" || echo "::warning::scancel ${job} failed" + - name: Upload results if: always() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 From 31d4b5d45d3698debc4366e0096e7a82d3fec8c4 Mon Sep 17 00:00:00 2001 From: i-kosarev Date: Fri, 30 Oct 2026 04:30:38 +0000 Subject: [PATCH 23/26] fix(rccl-ci): kill the allocation's group even if scancel is interrupted 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 --- projects/rccl/ci/scripts/test_madengine.py | 32 +++++++++++++--------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index ee262a9a570d..cfcb7c2bc54a 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -923,19 +923,23 @@ def release_allocation(reason: str) -> None: """ log.error("Releasing the allocation: %s", reason) try: - granted_job = alloc_info.read_text().split()[0] - except (OSError, IndexError): - granted_job = "" # still queued; killing salloc withdraws it - if granted_job: try: - subprocess.run(["scancel", granted_job], timeout=60) - except (OSError, subprocess.SubprocessError) as exc: - log.warning("scancel %s failed: %s", granted_job, exc) - try: - os.killpg(proc.pid, signal.SIGKILL) - except ProcessLookupError: - pass - proc.wait() + granted_job = alloc_info.read_text().split()[0] + except (OSError, IndexError): + granted_job = "" # still queued; killing salloc withdraws it + if granted_job: + try: + subprocess.run(["scancel", granted_job], timeout=60) + except (OSError, subprocess.SubprocessError) as exc: + log.warning("scancel %s failed: %s", granted_job, exc) + finally: + # A second signal during scancel raises out of this function; the + # group must still be killed and reaped. + try: + os.killpg(proc.pid, signal.SIGKILL) + except ProcessLookupError: + pass + proc.wait() # Actions cancels a run with SIGINT and escalates to SIGTERM. SIGINT # already arrives as KeyboardInterrupt, but SIGTERM ends the interpreter @@ -1191,7 +1195,9 @@ def check_ab_regression( f"{ratio:.4f} ({ratio - 1:+.1%}), threshold: -{threshold:.0%}" ) - if ratio - 1 < -threshold: + # Not `ratio - 1 < -threshold`: the subtraction rounds, so a loss of + # exactly the threshold failed for some inputs and passed for others. + if ratio < 1 - threshold: return True, f"REGRESSION DETECTED — {msg}" return False, f"No regression — {msg}" From 2e990a0ad95017477b0e82bc665cb755407353eb Mon Sep 17 00:00:00 2001 From: i-kosarev Date: Fri, 30 Oct 2026 04:52:58 +0000 Subject: [PATCH 24/26] fix(rccl-ci): cancel a still-queued allocation, and never score stale 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 --- .../workflows/therock-rccl-test-madengine.yml | 21 ++++++---- projects/rccl/ci/scripts/test_madengine.py | 42 ++++++++++++++----- 2 files changed, 45 insertions(+), 18 deletions(-) diff --git a/.github/workflows/therock-rccl-test-madengine.yml b/.github/workflows/therock-rccl-test-madengine.yml index a7e1983a62b3..553421197550 100644 --- a/.github/workflows/therock-rccl-test-madengine.yml +++ b/.github/workflows/therock-rccl-test-madengine.yml @@ -206,22 +206,27 @@ jobs: # # The runner starts this step itself, which is what makes it the one # piece of cleanup a cancellation cannot skip. + # + # It cancels by job name, which the script records before salloc starts: + # a request still queued has no job id yet, and a killed salloc does not + # withdraw it, so it would start later and hold the nodes all the same. - name: Release the SLURM allocation if: cancelled() run: | - recorded="${WORK_DIR}/allocation.txt" + recorded="${WORK_DIR}/allocation.name" if [ ! -s "${recorded}" ]; then - echo "No allocation was granted; nothing to release." + echo "salloc was never started; nothing to release." exit 0 fi - job="$(head -n1 "${recorded}" | tr -d '[:space:]')" - case "${job}" in - ''|*[!0-9]*) - echo "::warning::no usable job id in ${recorded}, got '${job}'" + name="$(head -n1 "${recorded}" | tr -d '[:space:]')" + case "${name}" in + rccl-ab-*) ;; + *) + echo "::warning::unexpected job name in ${recorded}, got '${name}'" exit 0 ;; esac - echo "Releasing SLURM job ${job}" - scancel "${job}" || echo "::warning::scancel ${job} failed" + echo "Releasing SLURM job ${name}" + scancel --me --name "${name}" || echo "::warning::scancel --name ${name} failed" - name: Upload results if: always() diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index cfcb7c2bc54a..b17d13fa38d4 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -771,9 +771,22 @@ def prepare_phase_dir(work_dir: Path, phase: str) -> Path: """Create one phase's madengine working directory, with its own scripts copy. madengine writes its outputs under fixed names relative to its cwd. + + Recreated from scratch: on a reused --work-dir an earlier run's exit_code, + perf files and node logs would otherwise still be there, and an allocation + that never ran a phase would be scored on them as if it had. Fails if the + old directory cannot be removed rather than run on top of it. """ run_dir = work_dir / "ab" / phase - run_dir.mkdir(parents=True, exist_ok=True) + if run_dir.exists(): + try: + shutil.rmtree(run_dir) + except OSError as exc: + raise RuntimeError( + f"{run_dir} holds an earlier run's results and cannot be removed " + f"({exc}); use a fresh --work-dir" + ) from exc + run_dir.mkdir(parents=True) scripts_src = work_dir / "scripts" scripts_dst = run_dir / "scripts" @@ -876,6 +889,18 @@ def run_ab_in_one_allocation( script_path.parent.mkdir(parents=True, exist_ok=True) script_path.write_text("\n".join(script_lines) + "\n") + # Unique per run, because cancelling by name is the only way to reach a + # request that is still queued: it has no job id in allocation.txt yet, + # and SIGKILL, which salloc cannot handle, leaves it queued to start later + # with nothing attached (InactiveLimit is 0 on ruby). Recorded before + # salloc starts so the workflow's release step can read it too. + run_tag = ( + f"{os.environ['GITHUB_RUN_ID']}-{os.environ.get('GITHUB_RUN_ATTEMPT', '1')}" + if os.environ.get("GITHUB_RUN_ID") else f"local-{os.getpid()}" + ) + job_name = f"rccl-ab-{workload_name}-{run_tag}" + (work_dir / "allocation.name").write_text(job_name + "\n") + cmd = [ "salloc", "--nodes", str(nodes), @@ -884,7 +909,7 @@ def run_ab_in_one_allocation( "--partition", cluster_config.get("slurm_partition", workload_config["slurm_partition"]), "--time", workload_config["time_limit"], - "--job-name", f"rccl-ab-{workload_name}", + "--job-name", job_name, ] qos = cluster_config.get("slurm_qos", "") if qos: @@ -923,15 +948,12 @@ def release_allocation(reason: str) -> None: """ log.error("Releasing the allocation: %s", reason) try: + # By name, not by the id in allocation.txt: the name also reaches + # a request that is still queued, which has no id there yet. try: - granted_job = alloc_info.read_text().split()[0] - except (OSError, IndexError): - granted_job = "" # still queued; killing salloc withdraws it - if granted_job: - try: - subprocess.run(["scancel", granted_job], timeout=60) - except (OSError, subprocess.SubprocessError) as exc: - log.warning("scancel %s failed: %s", granted_job, exc) + subprocess.run(["scancel", "--me", "--name", job_name], timeout=60) + except (OSError, subprocess.SubprocessError) as exc: + log.warning("scancel --name %s failed: %s", job_name, exc) finally: # A second signal during scancel raises out of this function; the # group must still be killed and reaped. From a629835732faa9f2ba0a786739cc62576a210893 Mon Sep 17 00:00:00 2001 From: i-kosarev Date: Fri, 30 Oct 2026 07:08:53 +0000 Subject: [PATCH 25/26] feat(rccl-ci): score the A/B ratio against the previous run's ratio too 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 --- projects/rccl/ci/scripts/test_madengine.py | 101 ++++++++++++++++++++- 1 file changed, 98 insertions(+), 3 deletions(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index b17d13fa38d4..23c05d4b7292 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -1225,6 +1225,71 @@ def check_ab_regression( return False, f"No regression — {msg}" +def check_ratio_regression( + ratio: float, + previous: float, + workload_type: str, +) -> tuple[bool, str]: + """Score this run's candidate/stock ratio against the previous run's. + + The stock baseline is the same image every night, so it divides out the + node pair the scheduler handed each run: the two ratios are comparable + where the absolute numbers are not. The vs-stock check above cannot do + this job on its own -- a workload whose image ships a much slower RCCL + has a ratio far above 1 and passes it whatever the candidate does. + + Returns (is_regression, message). + """ + threshold = ( + REGRESSION_THRESHOLD_TRAINING + if workload_type == "training" + else REGRESSION_THRESHOLD_INFERENCE + ) + change = ratio / previous + msg = ( + f"ratio {ratio:.4f} vs {previous:.4f} previously = " + f"{change:.4f} ({change - 1:+.1%}), threshold: -{threshold:.0%}" + ) + if change < 1 - threshold: + return True, f"REGRESSION DETECTED — {msg}" + return False, f"No regression — {msg}" + + +def previous_comparison( + results_dir: Path, + workload_name: str, + scale: str, + precision: str, +) -> dict | None: + """The latest earlier candidate row with a usable ratio for this workload. + + Called before this run appends its own rows, so the latest match is the + previous run's. Same workload, scale and precision only: a ratio depends + on all three. + """ + try: + with open(results_dir / PERF_DATASTORE) as f: + fcntl.flock(f, fcntl.LOCK_SH) + lines = f.readlines() + except OSError: + return None + for line in reversed(lines): + try: + row = json.loads(line) + except ValueError: + continue + if ( + row.get("phase") == CANDIDATE + and row.get("workload") == workload_name + and row.get("scale") == scale + and row.get("precision") == precision + and isinstance(row.get("ratio"), (int, float)) + and _usable_metric(float(row["ratio"])) + ): + return row + return None + + def append_result( results_dir: Path, workload_name: str, @@ -1355,11 +1420,17 @@ def generate_summary_report( lines.append(f"{c['precision'] or '?':>5}: ratio {ratio_s}") lines.append(f" baseline {_format_phase(c[BASELINE])}") lines.append(f" candidate {_format_phase(c[CANDIDATE])}") + if c.get("previous_ratio") is not None: + lines.append( + f" previous ratio {c['previous_ratio']:.4f} " + f"(run {c.get('previous_run_id') or '?'})" + ) else: lines.append("Throughput: N/A (workload did not produce metrics)") lines.append("") - lines.append(f"Regression: {regression_msg}") + lines.append("Regression:") + lines.extend(f" {line}" for line in regression_msg.splitlines()) lines.append("") run_url = os.environ.get("GITHUB_SERVER_URL", "") @@ -1600,7 +1671,28 @@ def main() -> None: reg, msg = check_ab_regression( base["metric_value"], cand["metric_value"], workload_config["type"], ) - regression_msgs.append(f"[{prec}] {msg}") + regression_msgs.append(f"[{prec}] vs stock: {msg}") + if reg: + log.warning(msg) + exit_code = max(exit_code, 1) + else: + log.info(msg) + + # Read before Step 8 appends this run's rows, so it is the previous run. + prev = previous_comparison(results_dir, args.workload, scale, c["precision"]) + if prev is None: + regression_msgs.append( + f"[{prec}] vs previous: no earlier comparison recorded — not scored" + ) + continue + c["previous_ratio"] = float(prev["ratio"]) + c["previous_run_id"] = prev.get("run_id", "") + reg, msg = check_ratio_regression( + c["ratio"], c["previous_ratio"], workload_config["type"], + ) + regression_msgs.append( + f"[{prec}] vs previous: {msg} (run {c['previous_run_id'] or '?'})" + ) if reg: log.warning(msg) exit_code = max(exit_code, 1) @@ -1610,7 +1702,7 @@ def main() -> None: if not comparisons: regression_msgs.append("no metrics from either phase — nothing to compare") exit_code = max(exit_code, 1) - regression_msg = "; ".join(regression_msgs) + regression_msg = "\n".join(regression_msgs) # Step 8: Append to datastore — one record per phase per precision, with # absolute values kept for trend analysis and the ratio on the candidate. @@ -1636,6 +1728,9 @@ def main() -> None: phase_extra = {**extra, "phase": phase, "image": phase_images[phase]} if phase == CANDIDATE and c["ratio"] is not None: phase_extra["ratio"] = c["ratio"] + if c.get("previous_ratio") is not None: + phase_extra["previous_ratio"] = c["previous_ratio"] + phase_extra["previous_run_id"] = c["previous_run_id"] append_result( results_dir, args.workload, From 453c3ea18566085a6864ecf99f8da8735c118f19 Mon Sep 17 00:00:00 2001 From: i-kosarev Date: Fri, 30 Oct 2026 09:32:49 +0000 Subject: [PATCH 26/26] fix(rccl-ci): take only a passing run's ratio as the reference 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 --- projects/rccl/ci/scripts/test_madengine.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/projects/rccl/ci/scripts/test_madengine.py b/projects/rccl/ci/scripts/test_madengine.py index 23c05d4b7292..20699b26d6ca 100644 --- a/projects/rccl/ci/scripts/test_madengine.py +++ b/projects/rccl/ci/scripts/test_madengine.py @@ -1261,11 +1261,17 @@ def previous_comparison( scale: str, precision: str, ) -> dict | None: - """The latest earlier candidate row with a usable ratio for this workload. + """The latest earlier passing run's candidate row for this workload. Called before this run appends its own rows, so the latest match is the previous run's. Same workload, scale and precision only: a ratio depends on all three. + + Only runs whose own verdict was a pass count. A failed run's ratio would + otherwise become the reference: a regression that failed one night would + pass the next, and a run whose candidate never loaded the CI RCCL would + set it to about 1.0. Rows written before the verdict was recorded have + none and are skipped for the same reason. """ try: with open(results_dir / PERF_DATASTORE) as f: @@ -1283,6 +1289,7 @@ def previous_comparison( and row.get("workload") == workload_name and row.get("scale") == scale and row.get("precision") == precision + and row.get("verdict") == "pass" and isinstance(row.get("ratio"), (int, float)) and _usable_metric(float(row["ratio"])) ): @@ -1714,6 +1721,10 @@ def main() -> None: # run_id names this test run; these name the RCCL binary it tested. "artifact_run_id": os.environ.get("ARTIFACT_RUN_ID", ""), "rccl_md5": rccl_fingerprint["md5"], + # The run's own gate verdict, final by now: it covers the phase exit + # codes, RCCL verification and both regression checks. A later run + # takes only a passing one as its reference. + "verdict": "pass" if exit_code == 0 else "fail", } phase_images = { BASELINE: workload_config["base_image"],