[test]kimik3-fp4-b300-vllm-agentic-dspark: K=7, native offload - #2419
[test]kimik3-fp4-b300-vllm-agentic-dspark: K=7, native offload#2419xinli-sw wants to merge 1 commit into
Conversation
|
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 关于重新运行失败任务的文档 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| PREFIX_MATCH_ARGS=() | ||
| OFFLOAD_ARGS=( | ||
| --kv-transfer-config | ||
| "{\"kv_connector\":\"SimpleCPUOffloadConnector\",\"kv_role\":\"kv_both\",\"kv_connector_extra_config\":{\"cpu_bytes_to_use_per_rank\":${CPU_BYTES_PER_RANK},\"lazy_offload\":false}}" | ||
| --kv-offloading-backend native | ||
| --kv-offloading-size "$TOTAL_CPU_DRAM_GB" | ||
| --kv-transfer-config '{"kv_load_failure_policy":"recompute"}' | ||
| ) | ||
| ;; | ||
| *) | ||
| echo "Error: unsupported KV_OFFLOAD_BACKEND='$KV_OFFLOAD_BACKEND' (expected empty or vllm-simple)" >&2 | ||
| echo "Error: unsupported KV_OFFLOAD_BACKEND='$KV_OFFLOAD_BACKEND' (expected empty or native)" >&2 | ||
| exit 1 |
There was a problem hiding this comment.
🔴 In the new native) branch, VLLM_USE_SIMPLE_KV_OFFLOAD=1 is exported while also passing --kv-offloading-backend native, but per this repo's own sibling recipes (dsv4_fp4_mi355x_vllm.sh, kimik2.5_fp4_mi355x.sh, minimaxm3_fp4_mi355x.sh) setting that env var to 1 diverts the native shortcut to SimpleCPUOffloadConnector instead of the intended OffloadingConnector. This likely means the K=7 native-offload sweep arm silently benchmarks SimpleCPUOffloadConnector (with --kv-offloading-size and --kv-transfer-config kv_load_failure_policy both meaningless to that connector), invalidating the experiment perf-changelog.yaml describes as 'native offload'. Fix: unset VLLM_USE_SIMPLE_KV_OFFLOAD in this branch, matching the sibling recipes.
Extended reasoning...
The bug: In kimik3_fp4_b300_vllm_mtp.sh's new native) case, the script does:
native)
require_agentic_kv_offload_backend native
export PYTHONHASHSEED=42
export VLLM_USE_SIMPLE_KV_OFFLOAD=1
PREFIX_MATCH_ARGS=()
OFFLOAD_ARGS=(
--kv-offloading-backend native
--kv-offloading-size "$TOTAL_CPU_DRAM_GB"
--kv-transfer-config '{"kv_load_failure_policy":"recompute"}'
)
;;It exports VLLM_USE_SIMPLE_KV_OFFLOAD=1 in the same branch that passes the --kv-offloading-backend native shortcut. Every other recipe in this repo that uses this exact shortcut form does the opposite and explains why in an inline comment. For example, dsv4_fp4_mi355x_vllm.sh:104-121:
vllm-native)
require_agentic_kv_offload_backend vllm-native
unset VLLM_USE_SIMPLE_KV_OFFLOAD
...
# Use vLLM's regular native KV-offload path (OffloadingConnector),
# NOT the SimpleCPUOffloadConnector. The "vllm-native" backend resolves to
# OffloadingConnector by default; setting VLLM_USE_SIMPLE_KV_OFFLOAD=1
# would switch it to SimpleCPUOffloadConnector. We intentionally leave
# that env var UNSET here so the regular OffloadingConnector path is
# used. The shortcut --kv_offloading_backend native + --kv_offloading_size
# form constructs the KVTransferConfig at engine startup
# (vllm/config/vllm.py:662).
OFFLOAD_ARGS=(
--kv_offloading_backend native
--kv_offloading_size "$TOTAL_CPU_DRAM_PARTITION_GB"
)
;;The same pattern (unset before the native shortcut args) appears in kimik2.5_fp4_mi355x.sh and minimaxm3_fp4_mi355x.sh. This is not incidental style — it documents a real, load-bearing hazard: the env var flips which KV connector class vLLM actually instantiates for the native shortcut.
Why this branch triggers it: the new native) case in this PR combines both ingredients that the sibling recipes take pains to keep apart — the native shortcut flags AND VLLM_USE_SIMPLE_KV_OFFLOAD=1. Per the documented behavior, that combination resolves to SimpleCPUOffloadConnector, not OffloadingConnector, at engine startup.
Why nothing else in the diff prevents this: require_agentic_kv_offload_backend native only validates that KV_OFFLOAD_BACKEND is set to the string native; it doesn't touch which connector class gets constructed. There's also no test or assertion in this script (or visible in the diff) that inspects the server log for which connector actually loaded.
Impact: if this analysis is correct, the entire K=7 native-offload sweep arm (configs/nvidia-master.yaml's new conc-list: [14, 16, 18, 20, 22, 24] row with kv-offload-backend: { name: native }) runs SimpleCPUOffloadConnector under a label and changelog entry (perf-changelog.yaml: "native offload") that says otherwise. Two of the three new flags in the branch become vestigial for that connector: --kv-offloading-size "$TOTAL_CPU_DRAM_GB" (SimpleCPUOffloadConnector sizes itself via cpu_bytes_to_use_per_rank in its own connector-extra-config, not this flag) and --kv-transfer-config '{"kv_load_failure_policy":"recompute"}' (an OffloadingConnector-only option). The run would likely still complete without crashing — it just silently measures the wrong connector, producing misleading throughput/latency numbers attributed to "native KV offload."
Step-by-step proof:
KV_OFFLOAD_BACKEND=nativeis set by the sweep matrix entry (kv-offload-backend: { name: native }inconfigs/nvidia-master.yaml).- The script enters the
native)case, exportsVLLM_USE_SIMPLE_KV_OFFLOAD=1, and buildsOFFLOAD_ARGSwith--kv-offloading-backend native --kv-offloading-size ... --kv-transfer-config '{"kv_load_failure_policy":"recompute"}'. vllm serveis launched with those args and that env var present.- Per the sibling recipes' documented behavior of vLLM's config resolution (
vllm/config/vllm.py:662), thenativeshortcut normally resolves toOffloadingConnector— butVLLM_USE_SIMPLE_KV_OFFLOAD=1overrides that resolution toSimpleCPUOffloadConnectorinstead. SimpleCPUOffloadConnectordoes not consume--kv-offloading-sizeorkv_load_failure_policy, so those flags are silently ignored, and the offload pool sizing/failure-recovery behavior the recipe intends is not what's actually exercised.- The sweep still runs and produces numbers, but they measure SimpleCPUOffloadConnector performance while being logged, labeled, and changelogged as the "native offload" (OffloadingConnector) experiment.
Fix: replace export VLLM_USE_SIMPLE_KV_OFFLOAD=1 with unset VLLM_USE_SIMPLE_KV_OFFLOAD in the native) branch, matching every other recipe using this shortcut.
| # K=7 golden AL 3.84 -> ~1.3x low conc, ~0.54x saturated | ||
| # The drafter's KV footprint (~25% of GPU KV) is unaffected by K and remains a | ||
| # genuine cost of running spec decoding at all on 100k+ prompts. | ||
| NUM_SPEC_TOKENS=2 | ||
| NUM_SPEC_TOKENS=7 | ||
| TOKENS_PER_SEQ=$((1 + NUM_SPEC_TOKENS)) | ||
| # Committed golden AL at K=2 on the probabilistic curve we run | ||
| # Committed golden AL at K=7 on the probabilistic curve | ||
| # (golden_al_distribution/kimik3_dspark_probabilistic_sample_method_block_rejection_sample_method.yaml: | ||
| # thinking_on 2 -> 2.51). The greedy/standard curve's K=2 value is 2.45 -- do not | ||
| # thinking_on 7 -> 3.84). The greedy/standard curve's K=7 value is 3.78 -- do not | ||
| # mix curves. | ||
| SYNTHETIC_ACCEPT_LEN=2.51 | ||
| SYNTHETIC_ACCEPT_LEN=3.84 | ||
|
|
||
| # Throughput runs pin synthetic acceptance; the EVAL_ONLY accuracy run must use | ||
| # real target verification instead. Synthetic acceptance commits drafted tokens |
There was a problem hiding this comment.
🟡 The K=7/native-offload refactor left several nearby comments un-updated, so the documented rationale now contradicts or misdescribes the code: (1) lines 162-172 still argue K=2 beats K=7 ("K=7 ... is negative", "~0.54x saturated") even though NUM_SPEC_TOKENS=7 is this PR's own change; (2) TOKENS_PER_SEQ (line 174) is now dead code since the cudagraph-capture-size loop that consumed it was replaced by a fixed COMPILATION_CONFIG literal; (3) lines 126-128 still describe TOTAL_CPU_DRAM_GB as divided per-rank for the removed SimpleCPUOffloadConnector ("~220 GiB per rank"), but the per-rank division was deleted along with that connector and the raw aggregate is now passed via --kv-offloading-size. None of this affects runtime behavior, but the comments should be updated to reflect the actual K=7/native-offload design.
Extended reasoning...
This PR migrates the recipe from K=2 DSpark speculation to K=7 and swaps SimpleCPUOffloadConnector for a native offload backend with a fixed COMPILATION_CONFIG cudagraph literal, but three comment blocks that document the why behind the old design were left untouched, so the script's own stated rationale now contradicts (or misdescribes) its configured behavior.
1. K=7 vs K=2 rationale (lines 162-172). The comment opens with "Level 2 rather than 7" and walks through a cost model concluding K=7 "loses to no-speculation once the GPU saturates" and is "negative" at ~0.54x saturated throughput, versus K=2 which "clears break-even at both ends." Immediately below, line 173 sets NUM_SPEC_TOKENS=7 — this PR's own change from 2. Only the narrow golden-AL sub-comment (now correctly saying K=7 -> 3.84) was updated; the actual decision paragraph was not, so a reader hits an explicit argument against the exact value the script configures.
2. Dead TOKENS_PER_SEQ (line 174). This variable used to feed the CUDA_GRAPH_CAPTURE_SIZES loop (num_seqs * TOKENS_PER_SEQ), which built the graph list in tokens because vLLM's adjust_cudagraph_sizes_for_spec_decode rounds to multiples of 1 + num_speculative_tokens. That whole loop was deleted in favor of a fixed COMPILATION_CONFIG literal (max_cudagraph_capture_size: 2304), so TOKENS_PER_SEQ is now computed and never read anywhere else in the file (confirmed via grep — one occurrence, its own assignment).
3. Stale per-rank KV-offload comment (lines 126-128). This comment still says TOTAL_CPU_DRAM_GB "is sized per rank" for SimpleCPUOffloadConnector and resolves to "~220 GiB per rank across the 8 TP ranks." The PR deletes the vllm-simple case arm entirely, including CPU_BYTES_PER_RANK = TOTAL_CPU_DRAM_GB * 1e9 / TP, and the new native arm instead passes the raw aggregate directly via --kv-offloading-size "$TOTAL_CPU_DRAM_GB" — no per-rank division happens anymore. The comment sits directly above the case block it now misdescribes.
Why nothing catches this: none of the three affect control flow or generated flags — they're prose above code that behaves independently of what the comment says, so there's no test or lint that would flag a comment/code mismatch. The risk is purely to future readers/maintainers, who would reasonably trust the recipe's own documented rationale (this file leans heavily on inline comments to justify each numeric choice) and be misled about why K=7 was chosen, what TOKENS_PER_SEQ is used for, or how the offload budget is consumed.
Step-by-step proof (case 1, the clearest example): (a) diff shows NUM_SPEC_TOKENS changed from 2 to 7 at line 173. (b) Lines 162-170, unchanged by the diff, state: "Level 2 rather than 7 ... at K=7 the target verifies 8 positions per step, which loses to no-speculation once the GPU saturates" and the table's last row: "K=7 golden AL 3.84 -> ~1.3x low conc, ~0.54x saturated." (c) Reading top-to-bottom, the comment tells the reader K=7 underperforms and K=2 was chosen for that reason, then the very next line sets K=7 — a direct contradiction that would confuse anyone auditing this script's configuration against its stated design.
Fix: update the K=7-vs-K=2 rationale paragraph (and the "Level 2" reference in the file header) to justify why K=7 is now the chosen value, delete the now-unused TOKENS_PER_SEQ line, and rewrite the DRAM-budget comment to describe the aggregate --kv-offloading-size sizing used by the native backend instead of the removed per-rank connector math.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30514592545 |
1f928c5 to
5249311
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30547432466 |
…c ladders, gsm8k eval / DSpark K=7、native 卸载、修订并发梯度、gsm8k 评估
5249311 to
2e63f85
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30604895002 |
1 participant
No description provided.