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:
|
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.
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)cluster:mi300x-amdworkload jobs succeededLive infrastructure
mi300x-amd_00throughmi300x-amd_08are online 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 all MI300X CI benchmarks run (Slurm allocation, container mounts, and artifact paths), though scope is limited to runner config and launch scripts with burn-in validation noted in the PR.
Overview
Moves MI300X benchmark execution from the retired
mi300x-amdspool to ninemi300x-amdrunners on the Barite cluster.configs/runners.yamlswaps runner names, replacescluster:mi300x-amdswithcluster:mi300x-amd, drops the separatemi300x-disaggnode list, and updates per-node CPU DRAM to 1,547,820 MiB. Twoamd-master.yamlagentic MTP sweeps now targetcluster:mi300x-amd.launch_mi300x-amds.shis removed andlaunch_mi300x-amd.shis added. The new launcher allocates Slurm jobs with 128 CPUs (no node exclude list), keeps squash imports on node-local/raid/inferencex/squashwith flock, and runs benchmarks by mountingGITHUB_WORKSPACEinto the container plus/raidpaths for HF hub and AgentX mmap cache—instead of staging the repo with sbcast and streaming artifacts back to the controller.Matrix and validation tests are updated to the new cluster key and DRAM values.
Reviewed by Cursor Bugbot for commit 624db70. Bugbot is set up for automated code reviews on this repo. Configure here.