Skip to content

Validate archived MI355X MiniMax M3 vLLM disaggregation with srt-slurm - #2824

Closed
cquil11 wants to merge 5 commits into
agent/srt-slurm-amd-integrationfrom
agent/port-minimaxm3-mi355x-vllm-srt-slurm
Closed

cquil11 wants to merge 5 commits into
agent/srt-slurm-amd-integrationfrom
agent/port-minimaxm3-mi355x-vllm-srt-slurm

Conversation

@cquil11

@cquil11 cquil11 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Scope

Port MI355X archived MiniMax M3 FP4 through vLLM + MoRI-IO + native vLLM Router, using the shared AMD-enabled srt-slurm integration. This is the dedicated vLLM/MoRI validation port: two physical nodes, one TP4 prefill worker and one TP4 decode worker, 8K input / 1K output, and nine concurrency points.

  • Convert the existing port recipes to native schema 2 (engine and roles) and preserve their engine, image, topology, and workload settings.
  • Keep the unchanged InferenceX benchmark_serving.py custom-benchmark interface; no engine patches.
  • Put the actual fixed-sequence concurrency lists in the master config and consume caller-owned CONC_LIST, replacing placeholder [1] rows and private recipe sweep lists where present. This preserves the existing measured points while matching current result collection.

This workload is deprecated upstream and remains in configs/deprecated/amd-master.yaml; it is not restored to active sweeps. Retained for the previously requested runtime-path coverage.

Stack

Based directly on shared AMD integration #2542, which is rebased onto InferenceX main at 6c5eff0b8b699530a49c9f2c654cd03219914d01. This child contains only its workload configs, schema-2 recipes, and append-only performance changelog entry. Shared launcher/profile/adapter changes live in #2542.

The base uses NVIDIA srt-slurm v2.22.1, commit 3cbc5dd256af2bfd2fed09b724628c3f5456c85f, through the checked-in submodule. AMD no longer uses a separate fork pin. This port is rebased on #2542; no tests or GPU workflows were rerun for this update.

Refresh status — 2026-09-22

Offline matrix validation: 1 topology/workload rows covering 9 benchmark concurrency points; all 2 prepared throughput/eval configurations pass the pinned runtime's schema/topology checks.

Offline worker-command generation also verifies producer/consumer roles, cluster-fabric addresses, explicit MoRI handshake/notify ports, and discovery-mode router arguments. The pinned vLLM v0.28.0 connector supports these fields; the pinned Router revision implements discovery-aware /health. The first GPU attempt passed host/RDMA setup but failed importing the deleted Router nightly (HTTP 404). Updated only the Router image to official nightly-20260922-5d3c3af (index digest sha256:4e9071e4f1e1670367b8d5d05befb8025e580385688978c7aeab2a7075a4b1a9), with matching metadata and a changelog entry. vLLM v0.28.0, MoRI discovery semantics, and workload options are unchanged. No srt-slurm code was modified.

GPU validation was authorized on 2026-09-22 and exposed a MoRI listener-port collision during decode initialization; the run and evidence are summarized in #2542. Both models loaded and Router discovery registered the workers, but no throughput point completed. The wedged run was stopped, and the srt-slurm allocator issue was reported to the maintainer in Slack instead of patched. Removed the stale per-role SRT_SLURM_COMMIT override before dispatch so the workflow actually inherits the shared runtime pin. All nine concurrency points are included. No E2E pass or performance claim yet; sweep-trigger labels remain removed.

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

github-actions Bot commented Sep 4, 2026

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 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Runtime startup now passes with the clean MoRI port-allocation fix: both allocations completed all TP handshakes, routed generation, and backend health checks on the unchanged official images.

Run 33837126410 exposed a dispatch-input mistake before evaluation began: literal eval-limit=full is passed through to lm-eval as --limit full, which is invalid. The full dataset requires the empty default for this harness. No engine, benchmark, or srt-slurm source change is needed for that error.

Corrected full throughput-plus-eval run: https://github.com/SemiAnalysisAI/InferenceX/actions/runs/33837838363
Exact checkout: 86b6762. Runtime: 33a3d811b7956973e4ef78b1d0cf4a5d56dad6a1. All nine concurrency points; eval-limit empty. The prior throughput job is left intact as additional evidence, not combined with this run to claim full-run success. Validation remains in progress.

@cquil11

cquil11 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

The first full throughput sweep has passed: all nine concurrency points and 5,116/5,116 measured requests completed on the official vLLM v0.28.0 image. Throughput job: https://github.com/SemiAnalysisAI/InferenceX/actions/runs/33837126410/job/100911912515

The raw artifact audit passed for unique point coverage, exact request/token totals, nonempty outputs, and finite metrics. srt-slurm completed and released the allocation without any host-repair steps.

This is not yet complete validation: that workflow still fails because of the earlier eval-limit dispatch mistake. The corrected full run remains https://github.com/SemiAnalysisAI/InferenceX/actions/runs/33837838363, with full GSM8K (empty limit) and all nine throughput points. Its eval is prioritized first in the MI355X queue. No engine patches, config workarounds, or changes to previously validated recipes have been introduced.

@cquil11

cquil11 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Full accuracy validation is now green on the corrected run: https://github.com/SemiAnalysisAI/InferenceX/actions/runs/33837838363/job/100913948095

  • Full GSM8K: 1,319 unique documents in each scoring filter; no evaluation limit.
  • Strict accuracy: 0.9469294920394238. Flexible accuracy: 0.9461713419257013. Both exceed the unchanged 0.90 threshold.
  • Per-sample audit: zero empty responses, unique document IDs/hashes, and recomputed averages exactly matching the native result.
  • The collected artifact agrees: n_eff=1319, infrastructure_success=true, integration_error=null, concurrency 256, TP4/TP4.

No graph-mode override, engine patch, or config workaround was needed. The same corrected workflow is now running its full nine-point throughput job (Slurm 42263), so whole-run validation is still in progress. The earlier nine-point throughput sweep also passed and was independently audited, but is not being used as a fill-in for this run.

@cquil11

cquil11 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Complete validation is green: https://github.com/SemiAnalysisAI/InferenceX/actions/runs/33837838363 (attempt 1).

All nine throughput points passed with 5,116/5,116 measured requests. Full GSM8K passed with 1,319 unique questions, zero empty responses, strict accuracy 0.9469294920394238, and flexible accuracy 0.9461713419257013 against the unchanged 0.90 threshold. Raw and collected artifacts were independently checked for counts, token totals, finite metrics, topology/GPU metadata, and matching scores. Both Slurm allocations completed 0:0 and released normally.

Exact tested checkout: 86b6762; srt-slurm: 33a3d811b7956973e4ef78b1d0cf4a5d56dad6a1. Official images only, no engine patches, graph-mode override, configuration workaround, or fill-in run.

The PR description now contains the complete evidence and caveats. It remains draft solely as an archived compatibility port: the regular changelog processor does not accept archived keys, and this manual e2e run is not stageable or reusable as a native PR sweep. No deprecated production sweep was re-enabled.

@claude

claude Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cquil11's task in 4m 44s —— View job


Review of PR #2824

  • Gather context: diff vs origin/agent/srt-slurm-amd-integration, read AGENTS.md
  • Review recipe disagg-1p1d-tp4.yaml against the shared srt-slurm launcher/profile
  • Review configs/deprecated/amd-master.yaml entries (image, prefix, conc list, runner)
  • Review perf-changelog.yaml entry (position, append-only rules)
  • Run available offline validators — not possible: Python execution and git submodule status are not permitted in this sandbox, so I could not run infx.matrix.generate on the archived config. Findings below are from tracing the code paths by hand.
  • Post findings

Summary

One blocking issue, posted inline on configs/deprecated/amd-master.yaml (lines 64-66 and 72-74): the SRT_SLURM_COMMIT=33a3d811… additional-settings override wins over the pool-level c3a5cecd… pin in runners/runtime_settings.sh, because benchmark-multinode-tmpl.yml sources runtime settings before exporting additional-settings. That contradicts the PR description's claim that the base runtime c3a5cecd is what this schema-2 recipe was validated against, and no other config in the repo pins the srt-slurm commit this way. Remove both lines or update them to the runtime actually being validated.

Everything else checked out: the recipe follows the mandatory Bash conventions (set -eo pipefail, check_env_vars, no fallback defaults), CONC_LIST and RANDOM_RANGE_RATIO are in the adapter's forwarded-env list, the result filename matches the collector's -c<N>.json regex under /results/<job>/fixed-seq, recipe_node_count will resolve two nodes from the schema-2 roles, model.container matches image and the router image tag matches router.version, the archive key is unique, the image is public Docker Hub, and the changelog entry is appended at the tail with the separator byte preserved (no append-only flag, so those rules do not apply). I could not verify offline that the vllm/vllm-router:nightly-20260903-1d10e71 tag exists on Docker Hub.
· Branch

Comment thread configs/deprecated/amd-master.yaml Outdated
Comment on lines +64 to +66
additional-settings:
- "CONFIG_FILE=recipes/vllm/minimax-m3/mi355x-fp4/8k1k/disagg-1p1d-tp4.yaml"
- "SRT_SLURM_COMMIT=33a3d811b7956973e4ef78b1d0cf4a5d56dad6a1"

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.

🔴 BLOCKING: Stale SRT_SLURM_COMMIT=33a3d811… override silently replaces the shared runtime pin this PR claims to validate against.

Why it matters: .github/workflows/benchmark-multinode-tmpl.yml sources runners/runtime_settings.sh (which exports SRT_SLURM_COMMIT=c3a5cecd… for mi355x-amds) before it exports the per-role additional-settings (lines 333 → 346-348), so this value wins and the job will git fetch/checkout 33a3d811 instead of c3a5cecd. The PR description states the base selects c3a5cecd and that the schema-2 engine.connector: moriio recipe was offline-validated against that pinned runtime (native connector resolution, allocator-owned MoRI-IO ports, discovery-aware router readiness are what #2542's changelog entry says c3a5cecd added). Either the older runtime rejects the converted recipe, or it runs on a different runtime than the one described, and srt-slurm-producer-sha.txt will record 33a3d811. No other master-config entry in the repo pins SRT_SLURM_COMMIT this way.

Fix: Drop the override and let the pool-level pin own the runtime (same change needed on the decode block at line 74). If a per-workload pin is actually intended, update it to c3a5cecd… and correct the PR description.

Suggested change
additional-settings:
- "CONFIG_FILE=recipes/vllm/minimax-m3/mi355x-fp4/8k1k/disagg-1p1d-tp4.yaml"
- "SRT_SLURM_COMMIT=33a3d811b7956973e4ef78b1d0cf4a5d56dad6a1"
additional-settings:
- "CONFIG_FILE=recipes/vllm/minimax-m3/mi355x-fp4/8k1k/disagg-1p1d-tp4.yaml"

Comment thread configs/deprecated/amd-master.yaml Outdated
Comment on lines +72 to +74
additional-settings:
- "CONFIG_FILE=recipes/vllm/minimax-m3/mi355x-fp4/8k1k/disagg-1p1d-tp4.yaml"
- "SRT_SLURM_COMMIT=33a3d811b7956973e4ef78b1d0cf4a5d56dad6a1"

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.

🔴 BLOCKING: Same stale SRT_SLURM_COMMIT=33a3d811… override as the prefill block above (see that comment for the precedence trace). Both roles are merged into one export list by the workflow, so both must be removed together.

Suggested change
additional-settings:
- "CONFIG_FILE=recipes/vllm/minimax-m3/mi355x-fp4/8k1k/disagg-1p1d-tp4.yaml"
- "SRT_SLURM_COMMIT=33a3d811b7956973e4ef78b1d0cf4a5d56dad6a1"
additional-settings:
- "CONFIG_FILE=recipes/vllm/minimax-m3/mi355x-fp4/8k1k/disagg-1p1d-tp4.yaml"

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

Code review found no new issues

No new issues were found in this update; 2 findings from earlier reviews are still open above.

This review covers commit ca8f156, which is no longer the latest commit on this pull request; later commits are not covered by it.

@cquil11
cquil11 force-pushed the agent/port-minimaxm3-mi355x-vllm-srt-slurm branch from 30029e3 to a10b64e Compare September 23, 2026 17:44
@cquil11 cquil11 closed this Sep 23, 2026
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