Migrate MI300X runners to the Barite AMD cluster - #2732
Conversation
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
|
Claude finished @cquil11's task in 4m 7s —— View job PR Review
LGTM - no blocking issues found Verified during review:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a large infrastructure migration (new Slurm launcher, renamed cluster/runner labels, and an AMD-master image bump) touching a CODEOWNERS-protected config, a human look would still be worthwhile.
What was reviewed: the rewritten runners/launch_mi300x-amd.sh (node-local /raid staging, flock-guarded enroot import retry loop, trap cleanup EXIT) against the deleted launch_mi300x-amds.sh; the mi300x-amds→mi300x-amd rename across configs/runners.yaml, configs/amd-master.yaml, and the three test fixtures for consistency (no stale mi300x-amds references remain outside the historical changelog entry); the appended perf-changelog.yaml entry for byte-preservation and tail placement; and the e2e-tests.yml bracket-syntax fix for inputs['duration-override'].
Extended reasoning...
Overview
This PR retires the mi300x-amds Slurm pool and its launcher in favor of a new nine-node mi300x-amd ("Barite") pool, with a rewritten runners/launch_mi300x-amd.sh that mounts the GitHub Actions workspace directly from NFS instead of tar/sbcast-staging it to a controller-local /tmp tree, and relocates squash images, HF/model cache, AgentX mmap cache, and Enroot/XDG/Triton state to node-local /raid/inferencex. Config files (configs/runners.yaml, configs/amd-master.yaml) are updated to match the renamed labels and new DRAM capacity, perf-changelog.yaml gets one tail-appended entry, and three test fixtures are updated for the rename. A separate, unrelated fix in .github/workflows/e2e-tests.yml switches four inputs.duration-override dot-access expressions to bracket syntax inputs['duration-override'], which is the correct fix for a hyphenated input name (dot access on a hyphenated property is invalid in GitHub Actions expression syntax).
Security risks
No injection, auth-bypass, or data-exposure concerns were identified. The launcher runs on a Slurm-managed CI runner fleet with the same trust boundary as before; inputs (IMAGE, TP, etc.) come from the same recipe/config pipeline as the prior script. The image tag bump in configs/amd-master.yaml points to a nightly build the author says replaces a deleted upstream tag — this cannot be verified from the diff alone (no external registry access), but it follows the same pattern as the pre-existing dsv4-fp8-mi325x-vllm entry that was left unchanged.
Level of scrutiny
This warrants a higher-than-default level of scrutiny: it is a full replacement of a production CI runner launcher script (new storage layout, new retry/locking logic, new cleanup trap) affecting the entire MI300X fleet, and configs/amd-master.yaml is a CODEOWNERS-protected file requiring named-owner review per .github/CODEOWNERS. The PR author's own validation notes that both manual burn-in workflow runs received a top-level failure conclusion despite all workload jobs succeeding — an anomaly not obviously introduced by files in this diff, but one a human reviewer with cluster access is better positioned to chase down than a static code review.
Other factors
The renamed labels are consistently propagated (config, test fixtures, and no stray old-name references outside historical changelog text), the perf-changelog.yaml entry is tail-appended without disturbing prior bytes, and the workflow duration-override fix is a narrow, well-scoped syntax correction consistent with GitHub Actions expression rules. Given the CODEOWNERS requirement and the scope of the launcher rewrite, I'm deferring rather than approving even though no concrete bug was found.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Closing because the project scope is now limited to porting active, nondeprecated AMD multi-node configurations to srt-slurm. This PR is outside that scope. |
|
Reopening: this PR predates the recent out-of-scope configuration-porting detour and was closed by mistake during an overly broad cleanup. Its prior state is being restored. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 624db70. Configure here.
| model: Qwen/Qwen3.5-397B-A17B-FP8 | ||
| model-prefix: qwen3.5 | ||
| runner: cluster:mi300x-amds | ||
| runner: cluster:mi300x-amd |
There was a problem hiding this comment.
Missing changelog for cluster migration
Medium Severity
This migrates MI300X jobs to a new cluster and cuts available-cpu-dram-mib from 2_321_924 to 1_547_820, which changes the MiniMax DRAM KV-offload budget, but it does not append a perf-changelog.yaml entry. CONTRIBUTING.md and AGENTS.md require a changelog for performance-affecting runner and recipe changes, and prior runner-pool moves followed that. Without it, merge will not select the affected keys for a sweep or record that later results come from Barite.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 624db70. Configure here.
There was a problem hiding this comment.
Automated review ran and found no bugs on the current commit (624db70). Given this rewires live production Slurm infrastructure (workspace mounting, squash/cache paths, CPU allocation) and cursor's inline comments on launch_mi300x-amd.sh lines 45/47 from this exact commit are only self-resolved by the author with no follow-up commit addressing them, a human look is still worthwhile before merge.
What was reviewed:
- Cluster/label rename consistency across configs/runners.yaml and configs/amd-master.yaml
- New launcher's salloc/srun/enroot-import flow, flock-based squash serialization, and container-mount wiring
- Checked and ruled out: EXIT trap exit-code masking, missing chown for root-owned workspace files, dropped self-healing
mkdir -pfor the squash dir, and absence of a perf-changelog.yaml entry despite CPU/cache-path changes
Extended reasoning...
Overview
This PR migrates the MI300X Slurm runner pool from the retired mi300x-amds cluster to a new mi300x-amd cluster ("Barite"). It touches config (configs/runners.yaml, configs/amd-master.yaml), replaces the launcher script wholesale (runners/launch_mi300x-amds.sh deleted, runners/launch_mi300x-amd.sh added with materially different logic — no more sbcast/tar workspace staging, direct $GITHUB_WORKSPACE mount, new /raid/inferencex/* cache/squash paths, halved cpus-per-task, new AIPERF mmap cache mount), and updates three test fixtures to match the renamed cluster and new DRAM figure.
Security risks
No injection, auth, or data-exposure concerns stand out — this is infrastructure/config for an internal CI runner pool, and $IMAGE/$GITHUB_WORKSPACE are the same trust-boundary inputs the prior script already used. The main risk surface is operational correctness (job allocation, mount paths, cleanup) rather than security per se.
Level of scrutiny
This warrants human-in-the-loop scrutiny despite being "just a runner script": it changes how live GPU nodes are provisioned and mounted for every future MI300X benchmark run, and a mistake here fails silently in CI (wrong cache path, wrong CPU count, or masked benchmark failures) rather than loudly. The bug-hunting system ruled out several candidate issues this run — EXIT trap masking benchmark exit status, no chown of root-owned artifacts left by --container-remap-root, dropped mkdir -p for the new squash dir, and a missing perf-changelog entry — but I have no way to independently confirm those rule-outs were conclusive versus judgment calls, and they were investigated by a different phase of this pipeline.
Other factors
The PR conversation shows a cursor[bot] review with two inline comments on runners/launch_mi300x-amd.sh (lines 45 and 47) filed against this exact final commit, both marked resolved — but only by the PR author, with no subsequent commit visible in the timeline that would address them. Per the review protocol, author-self-resolution of a third-party reviewer's objection is not evidence the concern was actually fixed, so I'm treating it as still outstanding and declining to approve, while noting the automated pass itself found no new bugs.
1 participant
Summary
mi300x-amdsrunner pool with ninemi300x-amdrunners on the Barite MI300X clustercomputepartition, which contains only the nine InferenceX-allocated GPU nodes; AMD spare nodes remain in their separate partition/raidValidation
bash -n runners/launch_mi300x-amd.shshellcheck runners/launch_mi300x-amd.sh/raiddirectories are present and writable on all ninecomputenodespython3 -m pytest utils/matrix_logic/ utils/test_validate_reusable_sweep_artifacts.py -q(258 passed)624db70e8: DSR1 and Qwen3.5 passed end-to-end with benchmark, server-log, and GPU-metrics artifacts624db70e8: Qwen3.5 TP8 and TP8/EP8 passed end-to-end with benchmark, trajectory, and server-log artifacts624db70e8: workload, result collection, and success-rate jobs passed with benchmark, trajectory, and server-log artifactsThe two DSV4 FixedSequenceLength entries could not start because their existing
nightly-09663ab...Docker tag returns HTTP 404 from Docker Hub. This PR intentionally leaves those image values identical tomain; the failure occurs during registry import after successful Slurm allocation and is independent of the launcher migration.Live infrastructure
mi300x-amd_00throughmi300x-amd_08are online and idle underext-cjquilicimi300x-amds_*runners have been removed from the repository runner registry/raid/inferencex/squash,/raid/inferencex/models/hub, and/raid/inferencex/aiperf-mmap-cacheNote
Medium Risk
This changes where and how MI300X jobs run (Slurm launcher, filesystem layout, and per-node DRAM metadata), so misconfiguration could break benchmarks or skew agentic memory budgets until burn-in passes.
Overview
Moves MI300X benchmark scheduling off the retired
mi300x-amdspool onto ninemi300x-amdGitHub runners on the Barite cluster.Runner registry:
configs/runners.yamlrenames the logical cluster tocluster:mi300x-amd, swaps label lists tomi300x-amd_00–_08, drops the oldmi300x-disaggslice that pointed at amds nodes, and lowersavailable-cpu-dram-mibto 1,547,820 (from ~2.32M) for agentic DRAM budgeting.Launch path: Deletes
runners/launch_mi300x-amds.sh(sbcast workspace staging, artifact tar-back, node excludes, 256 CPUs) and addsrunners/launch_mi300x-amd.sh, which allocates on Slurmcompute, caches Enroot squash under/raid/inferencex/squash, mounts the Actions workspace directly, and uses node-local HF / AgentX mmap cache paths on/raid.Sweep configs: AgentX entries in
configs/amd-master.yaml(Qwen3.5 and MiniMax M3 on MI300X) now targetrunner: cluster:mi300x-amd. Matrix and eval validation tests pick up the new cluster name, DRAM figure, and physical runner idmi300x-amd_04.Reviewed by Cursor Bugbot for commit 624db70. Bugbot is set up for automated code reviews on this repo. Configure here.