feat(amd): orchestrate AMD clusters with srt-slurm - #2542
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 关于重新运行失败任务的文档 |
cquil11
changed the title
3d6a75a to
0641575
Compare
cquil11
changed the title
|
Claude finished @cquil11's task in 8m 2s —— View job PR Review — feat(amd): orchestrate AMD clusters with srt-slurm
SummaryNo blocking issues found. Two 🟡 warnings posted inline:
Checks that passed: Note: I could not execute |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review ran; one nit-level bug was found and posted inline (MI355X qwen3.5 staging writing to the wrong HF cache root). Beyond that, a few other candidates were checked and ruled out: the producer-SHA file written by the srt-slurm launchers, the MI355X sglang disagg conc-list vs. the custom benchmark's actual concurrency sweep, and the AMD_BRINGUP.md translation/pin-staleness questions — none of these turned out to be functional issues. Given the size and novelty of this orchestration integration (new launcher scripts, cluster profiles, and recipes), a human look is still worthwhile.
Extended reasoning...
This PR introduces a large, brand-new AMD orchestration surface (srt-slurm integration): two new launcher scripts with nontrivial Slurm/Pyxis staging logic, two cluster profiles, five new recipe YAMLs, and two new contract test files, spanning MI300X and MI355X. Given the scale, the novelty of the code paths (remote staging via sbatch/srun, cache-path plumbing, git-pinned runtime fetch), and that CI already flags this PR for CODEOWNER sign-off, this falls well outside the bar for full automated approval regardless of the bug severity found. The one inline finding is a nit (redundant download risk, not a hard failure per the PR's own successful validation run), so it does not itself block merging, but the overall change warrants a human pass.
72ee996 to
2182835
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
eb12f0e to
cfd30df
Compare
|
Closing because the project scope is now limited to porting existing active, nondeprecated AMD multi-node configurations. This validation or generic upstreaming PR is outside that final 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. |
| if [[ ! -d "\$srt_runtime/.git" ]]; then | ||
| git clone --quiet "${SRT_SLURM_REPOSITORY}" "\$srt_runtime" | ||
| fi | ||
| git -C "\$srt_runtime" fetch --quiet origin "${SRT_SLURM_COMMIT}" | ||
| git -C "\$srt_runtime" checkout --quiet --detach "${SRT_SLURM_COMMIT}" | ||
| test "\$(git -C "\$srt_runtime" rev-parse HEAD)" = "${SRT_SLURM_COMMIT}" | ||
| make -C "\$srt_runtime" --no-print-directory setup-compute ARCH=x86_64 |
There was a problem hiding this comment.
🔴 Unlocked concurrent git clone/checkout/make on shared srt-slurm runtime dir keyed only by commit, not per-run
Extended reasoning...
REMOTE_SRT_RUNTIME="${REMOTE_BASE}/runtime/srt-slurm-${SRT_SLURM_COMMIT}" is a node-local path shared by ALL runs pinned to the same commit (not namespaced by RUN_KEY), and this directory later becomes SRTCTL_RUNTIME_SOURCE_DIR for the actual serving containers (line 134). Unlike the two container-image imports two lines above, which are explicitly wrapped in a per-target flock, the git clone/fetch/checkout --detach/make setup-compute sequence at lines 84-90 has no locking at all. If two overlapping CI executions (e.g. the srt-agg and srt-disagg MI300X matrix rows, or a rerun overlapping a still-running job) each submit their own 5-node stage sbatch job and Slurm schedules both onto the same compute node at the same time, both srun tasks race on the same .git directory: one process's git checkout --detach can collide with another's git clone/fetch (e.g. "Unable to create '.git/index.lock': File exists", or a clone racing a concurrent checkout leaving a partially-populated worktree). Because the whole stage step runs under set -euo pipefail, any such git failure aborts the entire…
Verification: normal. In runners/launch_mi300x-amds-srt.sh the per-node staging srun wraps container-image imports in a per-target flock (ensure_container_image: flock -w 2400 "$lock_fd"), but the git clone/fetch/checkout --detach/make setup-compute sequence (the if [[ ! -d "$srt_runtime/.git" ]]; then git clone ... block through make -C "$srt_runtime" ... setup-compute) has no locking.…
| exec {lock_fd}>"\${target}.lock" | ||
| flock -w 2400 "\$lock_fd" | ||
| if ! unsquashfs -s "\$target" >/dev/null 2>&1; then | ||
| tmp="\${target}.tmp.\${SLURM_JOB_ID}" | ||
| rm -f "\$tmp" | ||
| enroot import -o "\$tmp" "docker://\${image}" | ||
| unsquashfs -s "\$tmp" >/dev/null | ||
| mv "\$tmp" "\$target" | ||
| fi | ||
| flock -u "\$lock_fd" | ||
| exec {lock_fd}>&- | ||
| } |
There was a problem hiding this comment.
🟡 sweep:tmp="${target}.tmp.${SLURM_JOB_ID}" -- staged squashfs import writes to a per-job .tmp. path (mi300x-amds-srt.sh:73, mi355x-amds-srt.sh:50) with no trap/cleanup on failure; only success path mvs it away.
Extended reasoning...
If enroot import or the follow-up unsquashfs -s fails or the 45-min staging job times out mid-import (network hiccup, oversized ROCm image, node reboot), the partially-written multi-GB .tmp.<jobid> squashfs is left on node-local /raid (mi300x) or the shared /it-share filesystem (mi355x). The next run always picks a fresh SLURM_JOB_ID for its tmp name, so it never revisits or removes the stale file, and the rm -f "$tmp" guard only ever clears the current job's own (not-yet-created) tmp path. Repeated failed CI attempts on the same lane accumulate orphaned multi-GB files, eventually exhausting node-local RAID or the shared HF-cache filesystem used by every other AMD lane. A correct fix cleans stale *.tmp.* siblings of the target (e.g. via a glob-and-remove pass under the lock, or an EXIT trap) before/at the start of ensure_container_image.
Verification: nit. At runners/launch_mi300x-amds-srt.sh:73-77 (and mi355x-amds-srt.sh ~50) the staging import writes to a per-job path tmp="${target}.tmp.${SLURM_JOB_ID}", and the only cleanup is rm -f "$tmp" on line 74 which removes only the current job's own (not-yet-created) tmp. Under set -euo pipefail, if enroot import -o "$tmp" (line 75) or unsquashfs -s "$tmp" (line 76) fails, or the…
cfd30df to
8581966
Compare
# Conflicts: # configs/amd-master.yaml
| echo "[clear_caches] ${base_url}: L1+L2 flushed" | ||
| else | ||
| echo "[clear_caches] WARN ${base_url}: L1+L2 flush not confirmed after ${timeout_seconds}s" >&2 | ||
| fi |
There was a problem hiding this comment.
Cache flush match can stall replay
Medium Severity
clear_agentic_worker_caches treats a flush as successful only when the body contains Cache flushed. A 2xx response with empty or JSON text keeps retrying until FLUSH_DRAIN_TIMEOUT, adding about two minutes per worker at every concurrency point, then continues without a confirmed L1/L2 reset.
Reviewed by Cursor Bugbot for commit 1bb89eb. Configure here.
|
The ATOM validation exposed a host/container environment leak, fixed in 01676c3. The workflow sets PYTHONPYCACHEPREFIX=/tmp/inferencex-pycache for host Python tools. Slurm inherited it into serving containers, whereas the legacy Docker ATOM launch did not forward it. The pinned image starts in /, exposing CPython issue python/cpython#82916 during PyTorch generated-module imports. A CPU-only A/B check in the exact rocm/atom-dev:nightly_202606101403 image reproduced the failure with that prefix (exit 1) and passed without it (exit 0), keeping cwd=/ in both cases. The shared launcher now removes only the ambient host prefix when invoking srtctl apply; recipe-declared container environment remains supported. No image, engine flag, working directory, transport, or workload changes. The previous native run 33717234735 reached ATOM worker launch successfully with the NATS correction, then both 1P1D jobs failed on this import error. Its two still-queued 2P1D jobs were canceled before starting so the corrected revision can be validated in one fresh native full sweep. No unrelated cluster jobs were canceled. |
|
RDMA memory-lock restoration now has a controlled on-cluster A/B/A check. Slurm diagnostic 42203 completed on MI355X g09 with the unchanged ATOM image
The image's Mooncake batch API returned success for every batch, including batches with logged registration failures; process exit status alone is insufficient here. Restoring the inherited limit reproduced failures, ruling out a simple warmup/cache-order explanation in this probe. This supports the orchestration-only Diagnostic log on the cluster: |
|
Single-entrypoint cleanup pushed in 1e693fe, then synchronized with origin/main at 131d052 (PR head bd00831).
Verification: 18 adapter/runner tests pass; shell syntax and touched-file lint pass. Compared the old and new recipe transforms for nine real migrated recipes across throughput, eval-only, and combined modes: all 27 resulting serving/benchmark/eval configurations match. Historical perf-changelog bytes remain unchanged; this cleanup appends a new entry. The existing ATOM run is deliberately untouched: |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0d7ca6f. Configure here.
| if "MORI_MAX_DISPATCH_TOKENS_DECODE" in decode_environment: | ||
| mtp_size = int(environment.get("DECODE_MTP_SIZE", "0")) | ||
| decode_environment["MORI_MAX_DISPATCH_TOKENS_DECODE"] = str(dispatch_tokens * (mtp_size + 1)) | ||
| decode_environment["SGLANG_MORI_DISPATCH_INTER_KERNEL_SWITCH_THRESHOLD"] = str(2 * dispatch_tokens) |
There was a problem hiding this comment.
Decode DP+EP tied to prefill flags
Medium Severity
DP+EP admission and MoRI decode dispatch overrides run only when prefill is DP+EP, and they always rewrite decode as well. The retired launcher sized prefill and decode independently, so asymmetric topologies get the wrong max-running-requests and dispatch budget.
Reviewed by Cursor Bugbot for commit 0d7ca6f. Configure here.
1 participant
Summary
Integrates the SemiAnalysisAI srt-slurm AMD/ROCm work into InferenceX without Dynamo.
utils/bench_serving/benchmark_serving.pythrough srt-slurm's custom benchmarkScope boundary
Dynamo, NATS, etcd, NIXL, and Dynamo's NVIDIA-specific frontend/runtime assumptions are intentionally excluded from the AMD paths. MI300X uses direct vLLM or native vLLM Router; MI355X uses native SGLang Router. KV transfer uses MoRIIO for vLLM and AMD MoRI for SGLang.
Validation
The final review-head runs are on exact InferenceX head
e22bf36a306acc18545ea1475efa0493d063f278. MI300X disaggregate Slurm job11787completed0:0on two nodes, and MI300X aggregate Slurm job11790completed0:0on one node. MI355X production Slurm job38274completed0:0on two nodes. Native SGLang Router reported exactly one healthy prefill and one healthy decode worker. Runtime identity checks passed for SGLang0.5.17.dev20260809+g7120f3ee13, SGLang Router0.3.2, and AMD MoRI at the matching SGLang commit.The unchanged MI355X fixed-sequence benchmark completed 8/8 warmups and 16/16 measured ISL 8192 / OSL 1024 / c8 requests with no HTTP, router, MoRI registration, HIP/OOM, or NCCL failure. It reached 0.83 req/s, 847.73 output tok/s, and 7,629.56 total tok/s. The final review fix aligns MI355X staging and every serving role on
$HF_HOME/hub; the realized runtime prefetch and both worker environments confirmed/hf_hub_cache/hub. MI300X now recreates missing node-local engine and router squashfs files atomically from the pinned public images. Both final MI300X runs passed c1 and c4 fixed-sequence requests, and all result JSON, runtime logs, aggregation, and success-rate jobs uploaded successfully.Static validation
perf-changelog.yamlfinal newline and generated matrix validation against currentorigin/mainStack
This is the InferenceX base PR for the AMD migration. Configuration-specific child PRs are based directly on this branch: #2555, #2627, #2628, #2629, #2630, #2631, and #2633.
Runtime dependencies:
Note
Medium Risk
Deletes the legacy amd_utils multinode launch path and changes agentic replay semantics (routing header and cache clears); any workflow still pointing at amd_utils would break until fully on srt-slurm.
Overview
Removes the in-tree
benchmarks/multi_node/amd_utilsstack (SLURM submit/job flow, SGLang/vLLM/ATOM server launchers, model YAMLs,bench.sh, RDMA preflight, and the DSV4 agentic recipe that called into it), completing the move to srt-slurm for AMD multinode disaggregated runs.agentic_srt.shpicks up parity with the old amd_utils trace replay: optional SGLang router correlation routing when prefill DP-attention is on, and per-concurrency worker cache flush (L1/L2 via/flush_cache, best-effort L3 HiCache clear) whenCLEAR_CACHE_BETWEEN_CONCis enabled.benchmark_lib.shbuilds the lm-eval OpenAI base URL fromEVAL_SERVER_HOST(default0.0.0.0) so eval clients on a different node than the API can reach the routed server.Benchmark CI workflows add a pre-checkout step to
chownlegacybenchmarks/multi_node/amd_utilsfiles left root-owned by containers on persistent self-hosted workspaces.Reviewed by Cursor Bugbot for commit 0d7ca6f. Bugbot is set up for automated code reviews on this repo. Configure here.