Add mooncake store for dsv4 and qwen35#2089
Conversation
Oseltamivir
requested review from
Ankur-singh,
jgangani and
kedarpotdar-nv
as code owners
|
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 关于重新运行失败任务的文档 |
1 similar comment
|
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 关于重新运行失败任务的文档 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=28764172134 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| - config-keys: | ||
| - dsv4-fp4-gb300-dynamo-sglang-mooncake-store | ||
| - qwen3.5-fp4-gb300-dynamo-sglang-mooncake-store | ||
| description: | ||
| - "Add mooncake store-enabled variants for DeepSeek V4 and Qwen 3.5 on GB300, using dedicated _mooncake_store.yaml recipe files with mooncake_kv_store backend (RDMA protocol, 800GB global segment size, MNNVL forced)." | ||
| - "Configure mooncake-specific environment variables." | ||
| - "Wire launcher routing so CONFIG_FILE values ending with _mooncake_store.yaml use the srt-slurm main branch; legacy recipes continue on their previous branch flow." | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2085 |
There was a problem hiding this comment.
🔴 The new perf-changelog entry (lines 4522-4529) will hard-block utils/merge_with_reuse.sh (the canonical merge path) for two reasons: (1) pr-link points to /pull/2085 but this PR is #2089, and (2) the block is not at the tail — 6 later entries (PRs #2074, #2077, #2010, #2007, #2006, #1994) sit at lines 4531-4567 after it, violating the append-to-end rule enforced by compare_entries(). Move the 8-line block to after line 4567 and change pr-link to https://github.com/SemiAnalysisAI/InferenceX/pull/2089 (or an XXX placeholder); attribution to #2085 belongs in the description prose (which the PR body already has).
Extended reasoning...
The added perf-changelog.yaml block for dsv4-fp4-gb300-dynamo-sglang-mooncake-store / qwen3.5-fp4-gb300-dynamo-sglang-mooncake-store has two independent problems, either one of which blocks utils/merge_with_reuse.sh — the documented canonical merge tool per .github/workflows/README.md. Both problems slip past the PR-time check-changelog CI job (which only invokes validate_matrix_compatible_change → process_changelog.py and checks YAML shape / deletions), so the PR looks green but the merge script will refuse to canonicalize.
Problem 1 — entry is not at the tail (lines 4522-4529). AGENTS.md line 78 documents that new entries MUST be appended to the END, never inserted in the middle. This branch inserted its entry between the PR #2067 block and the PR #2074 block, and the two subsequent merges from main (95804d9, and the pull of #1994/#2006/#2007/#2010/#2077) pulled in 6 later entries that now sit at lines 4531-4567. 01267da (the fix commit that restored the malformed PR #2067 block) did not re-append the mooncake entry to the tail. prepare_perf_changelog_merge.py canonicalize calls compare_entries(base_entries, head_entries, pr_number) which iterates positionally; at the index where base (main) has PR #2074, head has the MOONCAKE entry, so without_pr_link(base_entry) != without_pr_link(head_entry) and it raises entry N changed; existing entries are immutable except for pr-link-only corrections. This fires before both validate_raw_change (whose startswith check would also fail) and validate_added_pr_link — so it is the FIRST error the merge script hits.
Problem 2 — wrong pr-link (line 4529). The block has pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2085, but this is PR #2089 (a base-repo mirror of the fork PR #2085, per the PR description). Once Problem 1 is resolved, canonicalize_appended_links → validate_added_pr_link runs:
expected = f'https://github.com/SemiAnalysisAI/InferenceX/pull/{pr_number}'
if link not in PR_LINK_PLACEHOLDERS and link != expected:
raise ChangelogValidationError(...)With pr_number=2089 and link=.../pull/2085, and PR_LINK_PLACEHOLDERS = {"XXX", ".../pull/XXX"} not matching either, this raises new PR entry must use .../2089 or an XXX placeholder; found .../2085.
Step-by-step proof (Problem 1).
git tail perf-changelog.yamlshows the last entry is PR Update Minimax M3 B300 FP4 vllm #1994 at line ~4568, not the mooncake block.git log --oneline mainshows commitsdc7695b/44a797e/3eebb57/d65465d/0c30484/ec81d52/b7ffc28— the entries for PRs [Klaud Cold] Update kimik2.5-fp4-mi355x-vllm vLLM ROCm image to v0.24.0 / 将 kimik2.5-fp4-mi355x-vllm 的 vLLM ROCm 镜像 升级至 v0.24.0 #2074, [Klaud Cold] Update kimik2.5-int4-mi355x-vllm vLLM ROCm image to v0.24.0 / 将 kimik2.5-int4-mi355x-vllm 的 vLLM ROCm 镜像 升级至 v0.24.0 #2077, Update DSV4 GB300 Dynamo vLLM Recipes #2010, Update Minimax M3 FP4 B200 Eagle #2007, Update Minimax M3 FP4 B300 Eagle #2006, Update Minimax M3 B300 FP4 vllm #1994 — all landed onmainafter the branch's original append.merge_with_reuse.shrunspython utils/prepare_perf_changelog_merge.py canonicalize --base-ref origin/main --pr-number 2089.- Inside
canonicalize_appended_links,compare_entrieswalksbase_entries[i]vshead_entries[i]. Whenireaches the PR [Klaud Cold] Update kimik2.5-fp4-mi355x-vllm vLLM ROCm image to v0.24.0 / 将 kimik2.5-fp4-mi355x-vllm 的 vLLM ROCm 镜像 升级至 v0.24.0 #2074 position,base_entries[i]is PR [Klaud Cold] Update kimik2.5-fp4-mi355x-vllm vLLM ROCm image to v0.24.0 / 将 kimik2.5-fp4-mi355x-vllm 的 vLLM ROCm 镜像 升级至 v0.24.0 #2074 buthead_entries[i]is the MOONCAKE entry — theconfig-keysdiffer, sowithout_pr_linkcomparison fails →ChangelogValidationError.
Fix. Move the 8-line block from lines 4522-4529 to after line 4567 (immediately after the PR #1994 entry) so it becomes the true tail, AND change the pr-link to https://github.com/SemiAnalysisAI/InferenceX/pull/2089 (or use an XXX placeholder that canonicalize_appended_links will rewrite at merge time). The PR description already attributes this to #2085, so no attribution is lost.
Impact. Not just style — this hard-blocks the standard merge path used by merge-prs / find-mergeable-claude-prs. Must be fixed before the PR can be merged via the canonical tooling.
| # Topology: 7 prefill (TP=4 / DP=4 / EP=4 / 1 node each) + 1 decode | ||
| # (TP=8 / DP=8 / EP=8 / 2 nodes). 9 nodes total. |
There was a problem hiding this comment.
🟣 The topology comment on lines 61-62 of the new disagg-gb300-1p1d-tp4-tp4-2-c1_mooncake_store.yaml says "7 prefill (TP=4 / DP=4 / EP=4 / 1 node each) + 1 decode (TP=8 / DP=8 / EP=8 / 2 nodes). 9 nodes total" but the resources block right below sets a 1P1D TP4/TP4 2-node topology matching the filename. This is a pre-existing stale comment inherited verbatim from the base recipe disagg-gb300-1p1d-tp4-tp4-2-c1.yaml (same line 61); no runtime impact, but worth correcting in-place since this PR is creating a fresh copy of the file.
Extended reasoning...
What
Lines 61-62 of the new file benchmarks/multi_node/srt-slurm-recipes/sglang/deepseek-v4/8k1k/disagg-gb300-1p1d-tp4-tp4-2-c1_mooncake_store.yaml carry a topology comment that contradicts the recipe it documents:
# Topology: 7 prefill (TP=4 / DP=4 / EP=4 / 1 node each) + 1 decode
# (TP=8 / DP=8 / EP=8 / 2 nodes). 9 nodes total.
resources:
gpu_type: "gb300"
gpus_per_node: 4
prefill_nodes: 1
prefill_workers: 1
gpus_per_prefill: 4
decode_nodes: 1
decode_workers: 1
gpus_per_decode: 4The comment describes a 7P1D 9-node topology; the actual resources: block declares a 1P1D 2-node topology (matching the filename 1p1d-tp4-tp4-2-c1). The sglang_config further down agrees with the block, not the comment: both prefill and decode set tensor-parallel-size: 4 / data-parallel-size: 1 / expert-parallel-size: 1.
Origin
The misleading comment is pre-existing — it appears on the same line 61 of the base recipe disagg-gb300-1p1d-tp4-tp4-2-c1.yaml, from which the mooncake variant was copy-pasted. This PR does not introduce the bug; it only propagates it into a second copy.
Impact
None at runtime. srtctl reads resources:, not the comment. This is purely a documentation/readability issue that can mislead a reviewer or operator glancing at the header to figure out the node budget.
Step-by-step proof
resources.prefill_nodes = 1,resources.prefill_workers = 1,resources.gpus_per_prefill = 4→ one 4-GPU (single-node) prefill worker.resources.decode_nodes = 1,resources.decode_workers = 1,resources.gpus_per_decode = 4→ one 4-GPU (single-node) decode worker.- Total: 1P + 1D = 2 nodes, 8 GPUs — matches filename
1p1d-tp4-tp4-2-c1and the sglang TP=4/DP=1/EP=1 for both roles. - Comment claims 7P (7 nodes × 4 GPUs = 28 GPUs) + 1D (2 nodes × 4 GPUs = 8 GPUs) = 9 nodes / 36 GPUs — off by ~4.5×.
Suggested fix
Replace lines 61-62 with something like:
# Topology: 1 prefill (TP=4 / 1 node) + 1 decode (TP=4 / 1 node). 2 nodes total.Worth applying the same fix to the base recipe in a follow-up so the stale comment does not spread further.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=28764641917 |
Oseltamivir
added
non-canary-full-sweep-enabled
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=28765312234 |
Oseltamivir
added
full-sweep-fail-fast
non-canary-full-sweep-enabled
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=28766816846 |
…s can't see login-node /scratch)
Oseltamivir
added
non-canary-full-sweep-enabled
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=28767862883 |
3 participants
Mirrors #2085 by @alogfans into an in-repo branch so the full sweep can run with secrets. Fork-originated PRs do not receive repository secrets (
secrets.REPO_PAT/HF_TOKENresolve to empty), which made the sweep jobs on #2085 fail atactions/checkoutwith "Input required and not supplied: token". Running from a base-repo branch resolves that.Includes a fix to the malformed
perf-changelog.yamlentry that was failingcheck-changelogon #2085 (the mooncake-store block had been inserted inside the existing PR #2067 entry, producing invalid YAML).Original description: