Bump DSpark draft length to 6 on dsv4 agentic low-concurrency arms / 将 dsv4 agentic 低并发臂的 DSpark 草稿长度提升至 6 - #3256
ichbinblau wants to merge 4 commits into
Conversation
|
Thanks for the contribution!
中文感谢你的贡献！
|
configs/amd-master.yaml: DECODE_MTP_SIZE 3->6 on the conc-list [4] and conc-list [16] arms of dsv4-fp4-mi355x-sglang-disagg-agentic-umbp-dspark. The conc-list [32, 48] HiCache arm and conc-list [128, 192, 256] UMBP-linker arm are untouched. server_sglang.sh: wire a DeepSeek-V4-Pro-0813:6 case into the golden-AL table (DSV4_GOLDEN_AL=3.77, from golden_al_distribution/dsv4-pro-0813-dspark.yaml thinking_on). Without it, agentic (non-eval) decode nodes at draft length 6 hit the DeepSeek-V4-Pro-0813:* catch-all and exit 1 before sglang.launch_server ever runs. perf-changelog.yaml: append entry per docs/configuration-procedures.md (pr-link: TBD until the PR exists). Co-authored-by: Cursor <cursoragent@cursor.com>
96452d0 to
2eb25cf
Compare
ichbinblau
added
AMD
full-sweep-enabled
agentx
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward config bump with a correctly wired golden-AL fallback.
- Confirmed the DECODE_MTP_SIZE 3->6 change in configs/amd-master.yaml touches only the two stated low-concurrency arms (conc-list [4] and [16]); the HiCache and UMBP-linker arms are untouched.
- Verified the new DeepSeek-V4-Pro-0813:6 case's DSV4_GOLDEN_AL=3.77 matches golden_al_distribution/dsv4-pro-0813-dspark.yaml's thinking_on:6 entry exactly.
- Checked DECODE_MTP_SIZE is still validated via check_env_vars, not defaulted with a
${VAR:-...}fallback. - perf-changelog.yaml diff is purely additive at the physical tail (no historical bytes touched).
Extended reasoning...
Overview
The diff touches three files: a two-line numeric bump (DECODE_MTP_SIZE 3->6) on two arms of one recipe in configs/amd-master.yaml, a one-line case-statement addition wiring a golden acceptance-length value for draft length 6 in benchmarks/multi_node/amd_utils/server_sglang.sh, and one appended entry in perf-changelog.yaml documenting both. No logic, control flow, or other recipes/arms are touched.
Security risks
None. This is a numeric configuration change and a static lookup-table addition in a benchmark launcher script; no user input, auth, or crypto paths are involved.
Level of scrutiny
Low-to-moderate is warranted given the repo's stated fairness policy (golden AL values must come from the committed curve, not be hand-picked) and byte-sensitivity of perf-changelog.yaml. I independently verified both: golden_al_distribution/dsv4-pro-0813-dspark.yaml lists thinking_on: {6: 3.77} exactly matching the new DSV4_GOLDEN_AL=3.77 case, and the perf-changelog.yaml diff only adds lines after the prior tail with no modification to existing bytes. I also confirmed DECODE_MTP_SIZE is still required via check_env_vars in server_sglang.sh (line 11) rather than given a silent ${VAR:-default} fallback, consistent with the repo's bash conventions.
Other factors
The PR conversation timeline shows a single commit event with no recorded CHANGES_REQUESTED review or unresolved third-party objection. The bug-hunting system reported zero findings; the one candidate issue it investigated (pr-link: TBD potentially failing CI's validate_perf_changelog.py) was already ruled out by that run, and I confirmed the placeholder-acceptance logic in validate_added_pr_link/PR_LINK_PLACEHOLDERS exists to handle exactly this case on PR runs. The change is small, self-contained, mechanical, and internally consistent, so I'm approving.
This review covers commit 96452d0, which is no longer the latest commit on this pull request; later commits are not covered by it.
|
View unofficial run (performance): https://inferencex.semianalysis.com/inference?unofficialRun=35486388642 View unofficial run (accuracy): https://inferencex.semianalysis.com/evaluation?unofficialRun=35486388642 |
billishyahao
left a comment
billishyahao
left a comment
There was a problem hiding this comment.
As a PR reviewer and CODEOWNER, I have reviewed this and have:
- Verified that as of the moment of typing this, this is the latest version of PR_REVIEW_CHECKLIST.md
- Verified that the general code quality meets the InferenceX standard and does not make the code quality any worse.
- Verified that this PR has passed PR validation. Please link to GitHub Action workflow that shows this. https://github.com/SemiAnalysisAI/InferenceX/actions/runs/35328216206
- Verified that this PR passes evals. Please link to GitHub Action workflow that shows this. https://github.com/SemiAnalysisAI/InferenceX/actions/runs/35328216206
- Verified that speculative decoding PRs uses chat templates to align the AL distribution to real world
- For agentic workloads: verified that speculative-decoding configs (EAGLE / MTP / draft models) run with simulated synthetic acceptance, with the acceptance-length value taken from the committed golden AL curve in golden_al_distribution/ for that model, thinking mode, and draft length. A submission may choose any supported draft length, but it may not substitute a different acceptance target.
- Verified against the current MODELS.md that this PR does not submit a deprecated model, scenario, or model-scenario combination.
- Verified that the model architecture isn't changed with benchmark hacks like using --hf-overrides to skipping indexer for every x layers on models that don't natively support this. As a general rule, we won't accept optimizations that reduces the number of model architecture FLOPs. Anything that makes that same computation run faster is fair game; FLOPs at lower precisions is fine, given that the config passes private evals. As an general north star princple, we should only use optimizations which is used in production by customers that care about accuracy
- If an company claims that they support vLLM/SGLang as first class LLM inference engines on their hardware, I have verified that the respective vLLM submission made using upstream https://hub.docker.com/u/vllm docker repo, upstream SGLang https://hub.docker.com/u/lmsysorg docker repo. The only exceptions are for new hardware, such as MI455X UALoE72, Vera Rubin NVL72, Rubin NVL8, etc., and for new model architectures where there is an actual reason why vLLM/SGLang does not fundamentally support them yet as supported by vLLM/SGLang community maintainers
- If an company claims that they support vLLM/SGLang as first class upstream in-tree LLM inference engines on their hardware, I have have verified that the respective vLLM/SGLang submission has been made before additional frameworks (TRT-LLM, ATOM, etc.). The only exceptions are for new hardware, such as MI455X UALoE72, Vera Rubin NVL72, Rubin NVL8, etc., and for new model architectures where there is an actual reason why vLLM/SGLang does not fundamentally support them yet.
- Verified that every single-node vLLM/SGLang recipe in this PR is documented in the official vLLM recipes and/or the SGLang cookbook:
- I linked the corresponding upstream PR in the vLLM recipe repo or SGLang repo and verified that it is MERGED before this InferenceX PR merges. An opened, draft, or closed-without-merge upstream PR does not satisfy this requirement. If the matching recipe was already published, I linked the published recipe/cookbook page in the additional detail section below.
- Verified that this PR does not patch the inference engine or serving stack — the pinned image must run as shipped. This covers .patch files / git apply / patch, inline patches embedded in benchmark scripts (e.g. a python3/sed heredoc that rewrites installed engine sources before serving), in-place edits of site-packages, monkey-patching, overwriting container files, and installing forked/rebuilt engine wheels on top of the pinned image. The only exception is a patch covered by a filled-out waiver at docs/waiver/
<PR_NUMBER>.md— named after the PR that introduces the patch and filed in that same PR, stating what is patched, why the unmodified upstream image cannot run this benchmark, the upstream PR/issue link, and the removal plan — which I have linked below in the additional detail section. - If this PR uses
append-only: true, verified that it only adds generated points or recipe variants inside a selected existing config/scenario and existing same-image visual curve: every previously generated point remains present with the same recipe, no prior point is removed or rerun, and every benchmark-affecting change in the complete diff can affect only the corresponding newly appended points (never an existing point), regardless of which file contains it. - If any of the above criteria cannot reasonably be satisfied, I have provided additional reasoning below.
Additional detail section:
- insert any additional info here
Signed: billishyahao
|
/reuse-sweep-run 35328216206 |
❌❌❌ REJECTED ❌❌❌@billishyahao — two blockers remain on ❌ Check 5 (Latest checklist template): FAIL — The sign-off omits two items from the current PR_REVIEW_CHECKLIST.md: (1) "Verified that all draft models and heads retain their original weights and precision, with evidence in the additional detail section…" and (2) "Reported measured throughput/E2EL Pareto counts and evidence per affected curve (≥5 points strongly recommended)…". Re-copy the current template into the existing review and check both items with evidence. ❌ Check 13 (Draft weights and precision): FAIL — Draft precision could not be verified: the additional detail section reads "insert any additional info here" and identifies no draft checkpoint/revision, native vs effective precision, or supporting metadata, which CONTRIBUTING.md § Draft-model precision requires for any spec-decode change (this PR changes the DSpark draft length on the c4/c16 arms). Independent inspection of the c16 decode log in run 35328216206: the draft is the DSpark head bundled in Non-blocking: the new Passed and not applicable checks✅ Check 0 (CODEOWNER): PASS — ✅ Check 1 (Passing sweep on in-PR commit): PASS — PR tip ✅ Check 2 (Evals pass): PASS — ➖ Check 3 (Recipe linked/merged): N/A — disaggregated/multi-node submission ( ✅ Check 4 (Reuse-sweep command): PASS — ✅ Check 6 (Upstream image, engine-first): PASS — the only touched entry ✅ Check 7 (Deprecated models/scenarios): PASS — ✅ Check 8 (No architecture hacks): PASS — no ✅ Check 9 (Spec-decode via chat template): PASS — ✅ Check 10 (No engine patches): PASS — the diff adds one ✅ Check 11 (Agentic golden AL): PASS — both changed arms (c4, c16) launched decode with ➖ Check 12 (Append-only): N/A — the new ✅ Check 14 (Pareto coverage): PASS — one affected curve: Assessed commit: |
|
/use 35328216206 |
|
…log.yaml Merging main moved other PRs' entries past ours, breaking the append-only-at-tail invariant. Restore it: remove our entry from its old position and re-append it unchanged at the true tail, per docs/configuration-procedures.md's merge-conflict procedure. No other entry's bytes are touched. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks for the comments, @edwingao28
this PR changes only the DSpark draft length — DECODE_MTP_SIZE 3 → 6 on the two low-conc arms (conc-list [4], [16]). It touches no model, checkpoint, draft architecture, weights, quantization, or KV-cache setting — so draft precision is identical to the original/unchanged arms by construction.
Done |
3 participants
Description
configs/amd-master.yaml: raiseDECODE_MTP_SIZEfrom 3 to 6 on the two low-concurrency arms ofdsv4-fp4-mi355x-sglang-disagg-agentic-umbp-dspark(conc-list: [4]andconc-list: [16]). Theconc-list: [32, 48]HiCache arm and theconc-list: [128, 192, 256]UMBP-linker arm are untouched (still draft length 3).benchmarks/multi_node/amd_utils/server_sglang.sh: wire aDeepSeek-V4-Pro-0813:6case into the golden-AL table (DSV4_GOLDEN_AL=3.77, sourced fromgolden_al_distribution/dsv4-pro-0813-dspark.yaml,thinking_on). Without this, an agentic (non-eval) decode node at draft length 6 falls into theDeepSeek-V4-Pro-0813:*catch-all andexit 1s beforesglang.launch_serverever runs.perf-changelog.yaml: appended an entry documenting both changes at the physical end of the file, perdocs/configuration-procedures.md.Type of Change
Checklist
perf-changelog.yamland have not edited historical entriesOWNER/MEMBER/COLLABORATOR) has commented/use <run_id>(or the legacy/reuse-sweep-run) on this PR. Do this only once there is a final full sweep that is all green with evals passing, since after this comment the sweep label will no longer automatically kick off new sweeps. Remove and re-add the label to force one.中文
改动说明
configs/amd-master.yaml：将dsv4-fp4-mi355x-sglang-disagg-agentic-umbp-dspark两条低并发臂（conc-list: [4]与conc-list: [16]）的DECODE_MTP_SIZE由 3 提升到 6；conc-list: [32, 48]的 HiCache 臂与conc-list: [128, 192, 256]的 UMBP-linker 臂保持草稿长度 3 不变。benchmarks/multi_node/amd_utils/server_sglang.sh：在黄金 AL 表中新增DeepSeek-V4-Pro-0813:6分支（DSV4_GOLDEN_AL=3.77，取自golden_al_distribution/dsv4-pro-0813-dspark.yaml的thinking_on）。否则草稿长度为 6 时，agentic（非 eval）decode 节点会落入DeepSeek-V4-Pro-0813:*兜底分支，在sglang.launch_server启动前就exit 1。perf-changelog.yaml：按照docs/configuration-procedures.md的要求，在文件末尾追加了记录以上两项改动的条目。改动类型
检查清单
perf-changelog.yaml文件末尾追加新条目，且未编辑历史条目OWNER/MEMBER/COLLABORATOR）在本 PR 下评论/use <run_id>（或旧版/reuse-sweep-run）。仅当已有一次全绿并通过 evals 的完整 sweep 时才这样做，因为评论后 sweep 标签将不再自动触发新的 sweep；移除并重新添加标签可强制触发一次。