Skip to content

feat(amd): orchestrate AMD clusters with srt-slurm - #2542

Open
cquil11 wants to merge 1 commit into
mainfrom
agent/srt-slurm-amd-integration
Open

cquil11 wants to merge 1 commit into
mainfrom
agent/srt-slurm-amd-integration

Conversation

@cquil11

@cquil11 cquil11 commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Shared base for the MI355X AMD ports. Rebased onto InferenceX main at 96bccfe12b637dad23d32f0bf7ceef0ea86de15a and updated for current srt-slurm v2.

  • Keep one launch_mi355x-amds.sh entrypoint. srt-slurm owns multi-node allocation, workers, native routers, services, logs, and completion; preserve the latest upstream single-node path.
  • Put ROCm visibility, fabric, Slurm directives, mounts, and host prerequisites in runners/srt-slurm/mi355x-amds.yaml, using the merged cluster renderer.
  • Consolidate workflow input/artifact adaptation here, using v2 engine / roles. Submit through shared apply_srt_recipe so speculative AgentX acceptance comes from the measured golden curves.
  • Select the synchronized AMD/MoRI/ATOM/observation fork stack at immutable SHA 03863ab23804f8a31811d9aaccc54abc8961a41c, based on NVIDIA 85086d3c1fe7b319a1d0d7db8781047dd1940772. Other pools' runtime selection and the NVIDIA submodule pin are unchanged.
  • Preserve cluster fabric/GPU-drain/free-hugepage prerequisites through native host setup, and routed evaluation/artifact collection. No engine patches or modifications to benchmark_serving.py.
  • Retire 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.
  • Include small aggregate and 1P1D Qwen smoke recipes. Fixed-sequence clients consume the matrix's actual 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.yml watches PRs targeting main; stacked children can be tested by explicit E2E dispatch when authorized, or by label after the base lands and they retarget main.

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.

@github-actions

Copy link
Copy Markdown
Contributor

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 As a PR reviewer and CODEOWNER, I have reviewed this and have.

For PR verification, add the full-sweep-fail-fast label (strongly recommended) to this PR — the benchmark sweep only runs on labeled PRs. Use full-sweep-enabled only if you need matrix jobs to keep running past a failure.

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 模板,包括保留英文语句 As a PR reviewer and CODEOWNER, I have reviewed this and have

如需进行 PR 验证,请为此 PR 添加 full-sweep-fail-fast 标签(强烈推荐)— 基准测试 sweep 仅在带有标签的 PR 上运行。仅当需要矩阵任务在失败后继续运行时才使用 full-sweep-enabled

PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动(flake),重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档

@cquil11 cquil11 changed the title WIP: bring srt-slurm to AMD clusters / 将 srt-slurm 引入 AMD 集群 WIP: bring srt-slurm to AMD clusters Aug 10, 2026
@cquil11
cquil11 force-pushed the agent/srt-slurm-amd-integration branch from 3d6a75a to 0641575 Compare August 10, 2026 00:02
@cquil11 cquil11 changed the title WIP: bring srt-slurm to AMD clusters feat(amd): orchestrate AMD clusters with srt-slurm Aug 10, 2026
@cquil11
cquil11 marked this pull request as ready for review August 10, 2026 09:29
@cquil11
cquil11 requested a review from a team August 10, 2026 09:29
@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cquil11's task in 8m 2s —— View job


PR Review — feat(amd): orchestrate AMD clusters with srt-slurm

  • Gather context and read the PR diff
  • Validate configs/amd-master.yaml changes (image accessibility, config key prefixes)
  • Validate perf-changelog.yaml (present, appended at end)
  • Review launch scripts (runners/launch_*.sh) for enroot import pattern and bugs
  • Review srt-slurm recipe YAMLs and contract tests
  • Post inline comments and final summary

Summary

No blocking issues found. Two 🟡 warnings posted inline:

  1. HF cache root mismatch (runners/launch_mi355x-amds-srt.sh:66) — the staging job downloads with HF_HUB_CACHE=/hf_hub_cache while the Qwen3.5 production recipe (aligned in d3ed345) reads /hf_hub_cache/hub, so the pre-stage download for the 397B model lands where workers never look. Works today only because the shared cache is warm.
  2. Missing enroot import fallback (runners/launch_mi300x-amds-srt.sh:57-58) — the MI300X launcher only asserts pre-existing .sqsh files exist, and vllm-router-nightly-20260809-d2ba586.sqsh has no documented public source, making the lane unreproducible on fresh nodes. The MI355X launcher in this PR already shows the correct pattern.

Checks that passed: perf-changelog.yaml was updated alongside configs/amd-master.yaml and the new entry is correctly appended at the end of the file; all image: values are public Docker Hub references; runner names resolve via configs/runners.yaml; the CONFIG_FILE opt-in redirect in the two legacy launchers is safe (no existing AMD config sets CONFIG_FILE); env contract (PREFILL_*, DECODE_*, DISAGG, RESULT_FILENAME) matches benchmark-multinode-tmpl.yml; no hardcoded expert-parallel flags; no MTP scripts. One minor doc nit not worth an inline comment: AMD_BRINGUP.md documents 1058248... as "current development pin," but the MI355X launcher pins a different srt-slurm commit (7beb0a1...) — worth clarifying whether the divergent pins are intentional.

Note: I could not execute pytest/bash -n in this sandbox (commands require approval), so test results rely on reading the contract tests and the CI runs linked in the PR description.

Comment thread runners/launch_mi355x-amds-srt.sh Outdated
Comment thread runners/launch_mi300x-amds-srt.sh Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread runners/launch_mi355x-amds-srt.sh Outdated
Comment thread runners/launch_mi300x-amds-srt.sh Outdated
Comment thread runners/launch_mi300x-amds-srt.sh Outdated
@cquil11
cquil11 force-pushed the agent/srt-slurm-amd-integration branch from eb12f0e to cfd30df Compare August 27, 2026 07:00
@cquil11

cquil11 commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

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.

@cquil11

cquil11 commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

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.

@cquil11 cquil11 reopened this Aug 27, 2026
Comment thread runners/launch_mi300x-amds-srt.sh Outdated
Comment on lines +84 to +90
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.…

Comment thread runners/launch_mi300x-amds-srt.sh Outdated
Comment on lines +70 to +81
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}>&-
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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…

@cquil11
cquil11 force-pushed the agent/srt-slurm-amd-integration branch from cfd30df to 8581966 Compare August 27, 2026 16:24
@cquil11
cquil11 force-pushed the agent/srt-slurm-amd-integration branch from 8581966 to 1d4522c Compare August 27, 2026 16:28
Comment thread configs/amd-master.yaml
@cquil11
cquil11 force-pushed the agent/srt-slurm-amd-integration branch from 1d4522c to 6da0605 Compare August 27, 2026 17:27
Comment thread runners/launch_mi355x-amds-srt.sh Outdated
@cquil11
cquil11 force-pushed the agent/srt-slurm-amd-integration branch from 5e1715e to 70f5a6d Compare August 27, 2026 21:25
Comment thread configs/amd-master.yaml
Comment thread runners/launch_mi355x-amds-srt.sh Outdated
@cquil11
cquil11 force-pushed the agent/srt-slurm-amd-integration branch 2 times, most recently from ee70c70 to b687387 Compare August 29, 2026 00:39
Comment thread runners/launch_mi355x-amds.sh
@cquil11
cquil11 force-pushed the agent/srt-slurm-amd-integration branch 3 times, most recently from c84fa81 to c94d0a4 Compare August 31, 2026 15:51
Comment thread benchmarks/multi_node/agentic_srt.sh
Comment thread benchmarks/multi_node/agentic_srt.sh
Comment thread runners/launch_mi355x-amds-srt.sh Outdated
Comment thread configs/amd-master.yaml
Comment thread .github/workflows/benchmark-multinode-tmpl.yml Outdated
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1bb89eb. Configure here.

@cquil11

cquil11 commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

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.

@cquil11

cquil11 commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

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 rocm/atom-dev:nightly_202606101403 and this profile's host verbs/Ionic mounts. Each of 8 GPU processes repeatedly registered 184 GPU-memory regions; 32 batch calls per phase:

Memory-lock setting Failed individual registrations
Inherited soft limit (4,072,153,088 bytes) 7
Unlimited (matching legacy Docker) 0
Restored inherited limit 5
Sequential registration, unlimited 0

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 ulimit -l unlimited restoration in 38be99b. It does not prove the prior full-model EINVAL and this probe's ENOMEM are identical failures. Full-model validation is still pending in https://github.com/SemiAnalysisAI/InferenceX/actions/runs/33782782708 (all 4 ATOM jobs prioritized through the scheduler API). No engine settings, image, matrix, or success thresholds were changed.

Diagnostic log on the cluster: /it-share/gharunners2/srt-slurm/diagnostics/atom-20260903/rdma-42203.log.

@cquil11

cquil11 commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Single-entrypoint cleanup pushed in 1e693fe, then synchronized with origin/main at 131d052 (PR head bd00831).

  • Keep runners/launch_mi355x-amds.sh as the only MI355X entrypoint.
  • Delete launch_mi355x-amds-srt.sh, prepare_mi355x_srt_host.sh, and check_mi355x_rdma.sh.
  • Remove the separate image/model staging allocation and unconditional host-repair/GPU-drain/RDMA preflights.
  • Use native srtctl apply --json and srtctl wait for submission, log following, accounting, and exit status.
  • Keep InferenceX-specific input/eval mapping and job-scoped artifact collection in utils/srt_slurm.py. This adapter contains no Slurm submission, polling, process management, or host mutation.
  • Use provisioned caches or native Pyxis image import. Persistent host resource policy is not silently moved elsewhere or repaired during a benchmark.
  • Pin rebased srt-slurm PR [NVIDIA] Add DSR1 TensorRT Support and Enhanced Plotting #7 at 81d46274, stacked on PR [NVIDIA] Add TRT-LLM 70B FP8 via slurm #1.

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:
https://github.com/SemiAnalysisAI/InferenceX/actions/runs/33782782708
It runs the previous launcher/runtime, so it must not be cited as hardware validation of this cleanup. A new full PR-triggered ATOM sweep is required once this shared head is incorporated into #2627; only that port will be tested at a time.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Fix All in Cursor

❌ 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.

Comment thread utils/srt_slurm.py Outdated
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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 0d7ca6f. Configure here.

@cquil11
cquil11 changed the base branch from main to codex/standardize-srt-slurm-v2 September 14, 2026 14:49
@cquil11
cquil11 added this pull request to stack #3102 September 14, 2026 14:49
@cquil11
cquil11 removed this pull request from stack #3102 September 14, 2026 14:50
@cquil11
cquil11 changed the base branch from codex/standardize-srt-slurm-v2 to main September 14, 2026 14:50
@cquil11
cquil11 force-pushed the agent/srt-slurm-amd-integration branch from 0d7ca6f to 6d439d6 Compare September 18, 2026 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant