Skip to content

[NV] Update 8k1k B300 Minimax Dynamo Disagg#2184

Open
jasonlizhengjian wants to merge 7 commits into
mainfrom
agent/update-minimax-m3-b300-8k1k
Open

[NV] Update 8k1k B300 Minimax Dynamo Disagg#2184
jasonlizhengjian wants to merge 7 commits into
mainfrom
agent/update-minimax-m3-b300-8k1k

Conversation

@jasonlizhengjian

Copy link
Copy Markdown
Collaborator

Summary

  • replace the MiniMax-M3 B300 FP4 8k1k DEP2-prefill recipes with TP1-prefill disaggregated recipes
  • cover TP4, TP8, and DEP8 decode points from concurrency 1 through 1024
  • colocate the TP4 decode topologies that fit on one B300 node
  • use the exact tested vLLM nightly image (nightly-2dfaae752b4db0d43cfc0715c780e33be030d0f1)
  • pin the new recipe path to srt-slurm c1fb6989fc5aca803b4ca0f2d17d8be85fad9732
  • split the new 8k1k sweep from the existing 1k1k configuration so this submission does not rerun 1k1k

8k1k sweep

Recipe Concurrency
1P1D TP1/TP4 8, 16, 24, 32, 48, 64
1P1D TP1/TP8 1, 2, 4, 8, 16
2P1D TP1/TP4 128
3P1D TP1/TP4 256
5P2D TP1/DEP8 512
8P2D TP1/DEP8 768, 1024

Validation

  • generated matrix contains exactly six entries, all ISL 8192 / OSL 1024
  • all six recipes pass srtctl dry-run
  • YAML parsing and B300 launcher shell syntax pass
  • 179 matrix generation and validation tests pass

@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 关于重新运行失败任务的文档

…m3-b300-8k1k

# Conflicts:
#	perf-changelog.yaml
@jasonlizhengjian jasonlizhengjian marked this pull request as ready for review July 14, 2026 01:01
@jasonlizhengjian jasonlizhengjian requested a review from a team July 14, 2026 01:01
@jasonlizhengjian jasonlizhengjian changed the title [DoNotMerge] Update 8k1k B300 Minimax Dynamo Disagg [DoNotMerge][WIP] Update 8k1k B300 Minimax Dynamo Disagg Jul 14, 2026
@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jasonlizhengjian's task in 4m 46s —— View job


Review: [DoNotMerge] Update 8k1k B300 Minimax Dynamo Disagg

  • Gather context (diff vs origin/main, read changed files)
  • Review recipe YAMLs (8k1k disagg recipes)
  • Validate configs/nvidia-master.yaml changes (image accessibility, model prefixes, changelog pairing)
  • Validate perf-changelog.yaml entry position
  • Validate runners/launch_b300-nv.sh (enroot import pattern)
  • Post review feedback

Summary

No blocking issues found. One 🟡 WARNING posted inline: the new launcher branch pins srt-slurm at c1fb6989 but skips the get_node_ip.sh patch (NVIDIA/srt-slurm#38) that the legacy minimaxm3 branch applies — please confirm the pinned commit already contains that fix, since srtctl dry-run would not surface a broken multi-node IP resolution path.

Everything else checked out:

  • Master config ↔ recipe consistency: all six CONFIG_FILE paths exist, concurrency lists, worker counts, TP/EP/dp-attn per topology, and the container image all match between configs/nvidia-master.yaml and the recipe YAMLs.
  • The 1k1k/8k1k split is clean — the legacy minimaxm3-fp4-b300-dynamo-vllm key retains only its isl: 1024 scenario and the new minimaxm3-fp4-b300-dynamo-vllm-8k1k-tp1 key holds only isl: 8192, so this submission won't rerun 1k1k.
  • Colocation GPU math fits: the decode_nodes: 0 recipes place 5/6/7 GPUs on one 8-GPU node with allow_prefill_decode_colocation: true; 8P2D uses exactly 8 prefill GPUs on its own node.
  • Launcher glob 8k1k/*-tp1-*.yaml matches all six new recipes and cannot capture the fp8 or 1k1k configs, and it correctly precedes the general minimaxm3 branch.
  • perf-changelog.yaml entry is appended at the end of the file and paired with the master-config change.
  • Image vllm/vllm-openai:nightly-2dfaae752b4db0d43cfc0715c780e33be030d0f1 is a public Docker Hub reference, and the existing enroot import docker:// flow in the launcher handles it.
  • No dangling references to the deleted DEP2 fp4 recipes (remaining dep2-*-8k1k hits are the untouched fp8 directory).

Note: I could not run the pytest matrix suite or reach external APIs (Docker Hub / srt-slurm GitHub) from this sandbox — those checks are covered by CI and the inline comment respectively.

Comment thread runners/launch_b300-nv.sh
@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown
Contributor

@jasonlizhengjian jasonlizhengjian added non-canary-full-sweep-enabled Run the full sweep without the canary gate (full search space, no trim) and removed full-sweep-fail-fast labels Jul 14, 2026
@jasonlizhengjian jasonlizhengjian removed the non-canary-full-sweep-enabled Run the full sweep without the canary gate (full search space, no trim) label Jul 14, 2026
@jasonlizhengjian jasonlizhengjian marked this pull request as draft July 14, 2026 18:03
Jason Li (Engrg-Hardware 1) added 2 commits July 14, 2026 11:18
Add the B300 8P2D TP1-prefill/TEP4-decode 8k1k recipe at concurrency 2048, pinned to the validated newer vLLM nightly and Dynamo 1.3.0.dev20260713.\n\n中文:新增 B300 8P2D TP1 预填充/TEP4 解码的 8k1k 配置,仅测试并发 2048,并固定到已验证的新 vLLM nightly 与 Dynamo 1.3.0.dev20260713。
Restore the proven 4P2D DEP2-prefill/TEP4-decode concurrency-4096 recipe exactly as previously submitted, in an 8k1k-only config that avoids rerunning 1k1k. Remove the experimental TP1/TEP4 concurrency-2048 replacement.\n\n中文:原样恢复已验证的 4P2D DEP2 预填充/TEP4 解码并发 4096 配置,并将其放入仅含 8k1k 的独立配置,避免重新运行 1k1k;移除实验性的 TP1/TEP4 并发 2048 替代配置。
@jasonlizhengjian jasonlizhengjian marked this pull request as ready for review July 14, 2026 18:30
@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jasonlizhengjian's task in 4m 28s —— View job


Re-review: Update 8k1k B300 Minimax Dynamo Disagg

  • Gather context (commits since last review, diff vs origin/main)
  • Review new changes only (8840ef6b, 0c65a6cf, 5b0d83bd, 30be9347)
  • Re-check master-config / perf-changelog / launcher rules on the new diff
  • Post re-review feedback

Re-review: no new blocking issues

Four commits landed since my previous review (01:02 UTC): the --no-preflight bypass, the merge of main with the Dynamo bump, and the two commits adding the legacy-max-tput config. All checked out:

Preflight bypass (8840ef6b)--no-preflight is scoped by the exact same glob (8k1k/*-tp1-*.yaml + framework/model/precision guards) as the pinned srt-slurm branch, so the bypass applies to precisely the recipe set that uses the newer revision with the login-host model.path check, and nothing else. The rationale comment explains why the login host can't stat /scratch-staged weights.

Dynamo bump (merge 0c65a6cf)1.3.0.dev202606141.3.0.dev20260713 is applied consistently: all six TP1 recipes' dynamo.version and the minimaxm3-fp4-b300-dynamo-vllm-8k1k-tp1 router pin match, and the changelog wording was updated to match. The legacy 1k1k key and its router pin are untouched. The merge also correctly kept this PR's changelog entry last in the file (main's new #2191/#2197/#2198 entries were inserted above it).

Legacy max-throughput point (5b0d83bd + 30be9347) — the net result is a faithful restoration:

  • 4p2d-dep2-tep4-8k1k.yaml is byte-identical to origin/main's copy (empty diff), including the legacy image vllm-minimax-m3-perf-x86_64-13.0.1-8b00f41 and Dynamo 1.3.0.dev20260614.
  • The new minimaxm3-fp4-b300-dynamo-vllm-8k1k-legacy-max-tput key reproduces main's conc-4096 search-space point exactly (4×DEP2 prefill / 2×TEP4 decode), with image and router pins matching the recipe.
  • Launcher routing is correct by construction: 4p2d-dep2-tep4-8k1k.yaml does not match the *-tp1-* glob, so it falls through to the existing general minimaxm3 branch (sa-submission-q2-2026 checkout + the Sanitize srun output in node IP resolution NVIDIA/srt-slurm#38 get_node_ip.sh patch) and does not get --no-preflight — i.e., it runs exactly the way this point ran when it was originally submitted.
  • The intermediate experimental 8p2d-tp1-tep4 recipe from 5b0d83bd was fully removed in 30be9347; no dangling references remain (all leftover dep2 hits are the untouched fp8/GB300 directories).

Repo rules re-verified on the new diff: master-config changes are paired with a perf-changelog.yaml entry listing both new config keys; the entry is appended at the very end of the file; all images are public Docker Hub references.

Still open from the previous review: the 🟡 inline warning on runners/launch_b300-nv.sh:84 — please confirm the pinned srt-slurm commit c1fb6989 already contains the NVIDIA/srt-slurm#38 get_node_ip.sh fix, since the TP1 branch (unlike the legacy branch) does not apply that patch. I couldn't verify ancestry from this sandbox (no external network access); if the sweep runs currently in flight bring up multi-node workers successfully, that empirically resolves it.

Comment thread perf-changelog.yaml
Comment on lines 4783 to +4784
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2198
- config-keys:

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.

🔴 The new changelog entry for PR #2184 is appended immediately after the prior pull/2198 entry with no blank separator line, unlike every other entry boundary in this file. utils/validate_perf_changelog.py (run in CI via run-sweep.yml) requires exactly one blank line before an appended - config-keys: entry and will raise ChangelogValidationError here, failing the changelog gate. Fix: insert one blank line before the new - config-keys: line at perf-changelog.yaml:4784.

Extended reasoning...

This PR appends a new perf-changelog entry (for the minimaxm3-fp4-b300-dynamo-vllm-8k1k-tp1 / -legacy-max-tput config keys) directly after the previous entry's pr-link: .../pull/2198 line, with no blank line separating the two entries. Every other entry boundary in perf-changelog.yaml has exactly one blank line before the next - config-keys: line — this new entry breaks that pattern.

This is enforced by a real CI gate, not just a style convention. utils/validate_perf_changelog.py::validate_raw_change() is invoked on every sweep PR via .github/workflows/run-sweep.yml:102. For an appended entry, it computes:

suffix = head_raw[len(base_raw):]
expected_start = (
    b"- config-keys:"
    if base_raw.endswith(b"\n\n")
    else b"\n- config-keys:"
)
if not suffix.startswith(expected_start):
    raise ChangelogValidationError(
        "new changelog entries must be separated from history by one "
        "empty line and appended at the end"
    )

Step-by-step proof:

  1. The base file (before this PR) ends its final entry (PR fix: recover PR 2108 ingest via sweep reuse / 通过扫描复用恢复 PR 2108 的结果入库 #2198) with a single trailing \n — the validator itself forbids a base ending in \n\n via a separate check a few lines down (the final appended changelog entry must end with one newline), so base_raw.endswith(b"\n\n") is False.
  2. Therefore expected_start = b"\n- config-keys:" — the validator requires the appended suffix to begin with a newline (i.e., a blank line) followed by - config-keys:.
  3. Reading the actual file bytes around perf-changelog.yaml:4783-4784 confirms the diff appends - config-keys: immediately after pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2198\n with zero blank lines in between. So suffix begins with - config-keys:, not \n- config-keys:.
  4. suffix.startswith(expected_start) evaluates to False, so validate_raw_change() raises ChangelogValidationError("new changelog entries must be separated from history by one empty line and appended at the end"), which exits non-zero and fails the changelog-gate step of the sweep pipeline.

One verifier raised a refutation arguing that utils/process_changelog.py's get_added_lines() (which extracts only +-prefixed diff lines and YAML-parses them) is indifferent to blank lines, and concluded the missing separator has "zero functional impact." That is true of process_changelog.py, but it is the wrong validator to look at — process_changelog.py handles ingesting changelog content into results, while utils/validate_perf_changelog.py is the distinct gate that CI runs against the raw file bytes to enforce formatting invariants (including the exact one-blank-line separator convention), and that gate does not tolerate this diff. The refutation's conclusion doesn't hold once the correct validator is traced.

Fix: insert a single blank line before the new - config-keys: line (i.e., between the pull/2198 pr-link line and the new entry), matching every other entry boundary in the file. This is a one-line fix but it currently produces a deterministic, CI-blocking failure — this PR's entire purpose is to trigger the 8k1k sweep via run-sweep.yml, and that pipeline step will fail before the sweep even runs.

@github-actions

Copy link
Copy Markdown
Contributor

@jasonlizhengjian jasonlizhengjian changed the title [DoNotMerge][WIP] Update 8k1k B300 Minimax Dynamo Disagg [NV] Update 8k1k B300 Minimax Dynamo Disagg Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant