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 关于重新运行失败任务的文档 |
3d6a75a to
0641575
Compare
|
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 |
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
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
8581966 to
1d4522c
Compare
1d4522c to
6da0605
Compare
5e1715e to
70f5a6d
Compare
ee70c70 to
b687387
Compare
c84fa81 to
c94d0a4
Compare
| 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.
0d7ca6f to
6d439d6
Compare


Summary
Shared base for the MI355X AMD ports. Rebased onto InferenceX
mainat96bccfe12b637dad23d32f0bf7ceef0ea86de15aand updated for current srt-slurm v2.launch_mi355x-amds.shentrypoint. srt-slurm owns multi-node allocation, workers, native routers, services, logs, and completion; preserve the latest upstream single-node path.runners/srt-slurm/mi355x-amds.yaml, using the merged cluster renderer.engine/roles. Submit through sharedapply_srt_recipeso speculative AgentX acceptance comes from the measured golden curves.03863ab23804f8a31811d9aaccc54abc8961a41c, based on NVIDIA85086d3c1fe7b319a1d0d7db8781047dd1940772. Other pools' runtime selection and the NVIDIA submodule pin are unchanged.benchmark_serving.py.amd_utils, its wrappers, and their legacy config entries. The workload PRs below restore the ported entries; merge this base together with the applicable children to restore those production lanes. Keep deprecated workloads archived rather than re-enabling them.CONC_LIST, matching current result validation.Workload PRs
Every child is based directly on this branch, not on another child. Runtime dependencies: #1, #11, #7, #14. The launcher uses #14's optional
srtctl wait; optional deployment and Dynamo drafts are excluded.Refresh checks — no GPU runs
494 local adapter, synthetic-acceptance, profile, matrix, and eval-dispatch tests pass. All eight port matrices and their throughput/eval recipe configurations resolve against the pinned stack. Bash syntax, Python lint/formatting, and append-only changelog checks pass. Native v1→v2 migration equivalence was checked before the explicit workflow-contract updates.
No E2E run or sweep was dispatched or rerun for this refresh, per maintainer request. Sweep-trigger labels have been removed. Current
run-sweep.ymlwatches PRs targetingmain; stacked children can be tested by explicit E2E dispatch when authorized, or by label after the base lands and they retargetmain.Historical E2E references
These links refer to earlier revisions, not validation of the refreshed heads; their prior results must not be reused as exact-head proof.
AI model disclosure
The exact AI model/version for this refresh is not exposed by the runtime and could not be verified. No subagents contributed to this refresh; earlier model identities are not reliably recorded.