[AgentX] vLLM DeepSeek-V4 B300 aggregate / vLLM DeepSeek-V4 B300 聚合配置#2225
[AgentX] vLLM DeepSeek-V4 B300 aggregate / vLLM DeepSeek-V4 B300 聚合配置#2225cquil11 wants to merge 3 commits into
Conversation
cquil11
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 关于重新运行失败任务的文档 |
cquil11
added
NVIDIA
full-sweep-enabled
agentx
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| vllm-simple) | ||
| require_agentic_kv_offload_backend vllm-simple | ||
| CPU_BYTES_PER_RANK=$(( TOTAL_CPU_DRAM_GB * 1000 * 1000 * 1000 / GPU_COUNT )) | ||
| # Identical prefixes must hash to identical block keys across DP ranks. | ||
| export PYTHONHASHSEED=42 | ||
| OFFLOAD_CONFIG=$(cat <<EOF | ||
| { | ||
| "kv_connector": "SimpleCPUOffloadConnector", | ||
| "kv_role": "kv_both", | ||
| "kv_connector_extra_config": { | ||
| "cpu_bytes_to_use": ${CPU_BYTES_PER_RANK}, | ||
| "enable_cross_layers_blocks": "true" | ||
| } | ||
| } | ||
| EOF | ||
| ) | ||
| OFFLOAD_ARGS=( | ||
| --kv-transfer-config | ||
| "$OFFLOAD_CONFIG" |
There was a problem hiding this comment.
🔴 The new vllm-simple branch (lines 98-116) builds a --kv-transfer-config naming SimpleCPUOffloadConnector but never exports VLLM_USE_SIMPLE_KV_OFFLOAD=1, unlike every other recipe in the repo that constructs this same connector (including ones using the identical explicit-JSON form, e.g. kimik2.5_int4_h200.sh:50-51, kimik2.5_fp4_b300_mtp.sh:74). Without it, the DEP4 sweep arm using kv-offload-backend: vllm-simple (conc [8,16,24,32,40,48,56,64,72] in nvidia-master.yaml) may not actually engage the intended SimpleCPUOffloadConnector semantics, risking misleading KV-offload benchmark results for that entire sweep row. Fix: add export VLLM_USE_SIMPLE_KV_OFFLOAD=1 right before building OFFLOAD_CONFIG in the vllm-simple case, mirroring the sibling recipes.
Extended reasoning...
The bug: In the new vllm-simple case of the KV_OFFLOAD_BACKEND switch (dsv4_fp4_b300_vllm.sh:98-116), the script builds a --kv-transfer-config JSON with kv_connector: SimpleCPUOffloadConnector, but nowhere in that branch does it export VLLM_USE_SIMPLE_KV_OFFLOAD=1. This is the only branch in the entire repository that constructs a SimpleCPUOffloadConnector transfer config without setting that env var.
Repo-wide convention (verified directly): Every sibling recipe that builds a SimpleCPUOffloadConnector kv-transfer-config sets VLLM_USE_SIMPLE_KV_OFFLOAD=1 immediately beforehand:
kimik2.5_fp4_b300_mtp.sh:74— identical explicit JSON form.kimik2.5_fp4_b300.sh:46,kimik2.5_int4_b200.sh:42,kimik2.5_int4_h100.sh:42— shortcut form.kimik2.5_int4_h200.sh:50-51— explicit JSON form, and the surrounding comment explicitly says this JSON form is chosen 'rather than the--kv_offloading_backend nativeshortcut' to passlazy_offload=true, and it still sets the env var on the line immediately before. This rules out the theory that the var is only needed for the shortcut-resolution path — it's required for the connector to actually take effect even when named explicitly in JSON.
Conversely, every branch that intentionally uses a different connector (OffloadingConnector, MooncakeStoreConnector, lmcache) defensively unsets the var: this same file's mooncake branch (line 170), dsv4_fp4_mi355x_vllm.sh:108/202/275, dsv4_fp4_b200_vllm.sh:165, kimik2.5_fp4_b200.sh:97, kimik2.5_fp4_mi355x.sh:68. The comment at dsv4_fp4_mi355x_vllm.sh:113-120 spells out the mechanism explicitly: the 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.' That unset is only meaningful if a sibling branch is expected to set it — which is exactly the missing line in the new vllm-simple branch of this file.
Why existing code doesn't prevent it: There is no validation that the env var matches the requested connector; require_agentic_kv_offload_backend vllm-simple only checks that the backend name is allowed, not that the corresponding env var is exported. The script will run to completion and launch vLLM successfully either way — the failure mode is silent (a benchmark that runs and reports numbers, just for a possibly-wrong KV path), not a crash.
Step-by-step proof of the gap:
- A DEP4 sweep point runs with
KV_OFFLOAD_BACKEND=vllm-simple,kv-offloading=dram. - The script enters
case "" in vllm-simple)at line 99. - It computes
CPU_BYTES_PER_RANK, setsPYTHONHASHSEED=42, and buildsOFFLOAD_CONFIGnamingkv_connector: SimpleCPUOffloadConnector— but at no point in this branch does it exportVLLM_USE_SIMPLE_KV_OFFLOAD. - Compare to
kimik2.5_int4_h200.sh:50-51, which builds the functionally identical explicit-JSONSimpleCPUOffloadConnectorconfig but sets the env var on the immediately preceding line — the validated, working pattern this PR's branch was clearly modeled on (matching field nameskv_connector,kv_role: kv_both,kv_connector_extra_config) but missing this one line. vllm serveis invoked with"${OFFLOAD_ARGS[@]}"containing the JSON, but without the env var vLLM may not engage the SimpleCPUOffloadConnector-specific runtime behavior the recipe intends (the unanimous repo convention treats the var as required alongside the JSON, not redundant with it).- This affects the entire DEP4
vllm-simplesweep row added inconfigs/nvidia-master.yaml(conc-list: [8,16,24,32,40,48,56,64,72]), whose purpose is specifically to characterize the SimpleCPU-offload cliff against the Mooncake row it sits beside — if the connector semantics aren't actually engaged, those 9 benchmark points measure the wrong thing while looking like valid data.
Fix: Add export VLLM_USE_SIMPLE_KV_OFFLOAD=1 in the vllm-simple) case, right before (or after) setting PYTHONHASHSEED=42, mirroring every sibling recipe.
Residual uncertainty: This PR pins a brand-new nightly image (vllm/vllm-openai:nightly-dev-x86_64-cu13.0.1-904e4ec), so it's conceivable upstream vLLM changed connector resolution so the env var is no longer needed when the connector is named explicitly. However, the convention is unanimous across every other recipe in the repo — including ones using the identical explicit-JSON form — and the sibling mooncake branch in this very file still defensively unsets the var, which only makes sense if a peer branch sets it. The burden is on proving the exception, and the safe, minimal fix (add the one-line export) matches the validated pattern everywhere else.
|
|
||
| - config-keys: | ||
| - dsv4-fp4-b300-vllm-agentic | ||
| description: | ||
| - "Update B300 AgentX: KV offload, sparse DSV4 attention, mega-MoE, and FULL_DECODE_ONLY CUDA graphs." | ||
| - "Image: vllm/vllm-openai:nightly-dev-x86_64-cu13.0.1-904e4ec" | ||
| - "B300: GPU-resident TP4 at conc [1,2,4,6,8,16]; DEP4 at conc [8,16,24,32,40,48,56,64,72] with both SimpleCPU and Mooncake 0.3.11.post1." | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2202 |
There was a problem hiding this comment.
🔴 The appended changelog entry's pr-link points to .../pull/2202 (the source PR this was split from) instead of .../pull/2225 (this PR). Since utils/merge_with_reuse.sh unconditionally runs prepare_perf_changelog_merge.py canonicalize after a clean merge with main, and that requires the appended entry's link to be either .../pull/2225 or the XXX placeholder, this will raise ChangelogValidationError and abort the standard reuse-merge path. Fix by setting pr-link to https://github.com/SemiAnalysisAI/InferenceX/pull/2225 (or the XXX placeholder), matching the convention used by every sibling entry (e.g. the 2222 entry links to pull/2222).
Extended reasoning...
The bug: the newly appended perf-changelog.yaml entry for dsv4-fp4-b300-vllm-agentic (lines 4798-4805) sets pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2202. Per the PR metadata and description, this change is landing as PR #2225 (a split-out of the original combined PR #2202), not #2202 itself. Every other entry in the changelog links its own merging PR number (e.g. the immediately preceding entry links pull/2222), so this is a copy-paste artifact from when the content lived in #2202 before the split.
The code path that triggers it: utils/merge_with_reuse.sh (the supported /reuse-sweep-run merge tool) merges origin/main into the PR branch, and then — regardless of whether that merge had a conflict on perf-changelog.yaml — runs prepare_perf_changelog_merge.py canonicalize --pr-number 2225 unconditionally (this call has no if-guard, unlike the conflict-resolution branch a few lines above it which force-overwrites the link but only executes when the changelog merge actually conflicts). canonicalize calls canonicalize_appended_links(), which immediately calls compare_entries(base_entries, head_entries, pr_number=2225). Inside compare_entries, every newly appended entry is passed to validate_added_pr_link(link, 2225). That function (validate_perf_changelog.py:144-160) requires link == 'https://github.com/SemiAnalysisAI/InferenceX/pull/2225' or link in PR_LINK_PLACEHOLDERS ({'XXX', '.../pull/XXX'}). .../pull/2202 satisfies neither, so it raises ChangelogValidationError: new PR entry must use '.../pull/2225' or an XXX placeholder; found '.../pull/2202'.
Why nothing else catches or prevents this first: canonicalize_appended_links has its own later check at line 112 (if link not in PR_LINK_PLACEHOLDERS: raise ...) intended to guard the same invariant, but validate_added_pr_link inside compare_entries fires first since it's called earlier in the same function (line 92-96 precedes line 105+). Either way the outcome is identical: an unhandled ChangelogValidationError. merge_with_reuse.sh runs under set -e/set -euo pipefail with this call unguarded, so the script aborts. Notably, the PR-level CI check-changelog job invokes validate_perf_changelog.py without --pr-number, which takes the pr_number is None branch in validate_added_pr_link — that branch only checks the link matches the canonical regex shape, not that it matches the merging PR number, so .../pull/2202 passes CI and the PR looks green in the GitHub UI. The failure is confined to the merge tooling itself.
Step-by-step proof:
- PR [AgentX] vLLM DeepSeek-V4 B300 aggregate / vLLM DeepSeek-V4 B300 聚合配置 #2225 merges cleanly with
origin/main(no changelog conflict) — exactly what the PR description states happened. - A maintainer or bot runs
utils/merge_with_reuse.shfor PR 2225. - The script merges
origin/main;merge_status=0(no conflict), so the conflict-resolution/force-overwrite branch is skipped entirely. - The script unconditionally runs
prepare_perf_changelog_merge.py canonicalize --pr-number 2225 --changelog-file perf-changelog.yaml --base-ref origin/main. - That process parses
base_entries(fromorigin/main) andhead_entries(from the PR branch), finds the one appended entry (the dsv4-fp4-b300-vllm-agentic block), and callscompare_entries(base_entries, head_entries, 2225). compare_entriescallsvalidate_added_pr_link('https://github.com/SemiAnalysisAI/InferenceX/pull/2202', 2225).2202is not inPR_LINK_PLACEHOLDERSand!= 'https://github.com/SemiAnalysisAI/InferenceX/pull/2225', soChangelogValidationErroris raised.- The Python process exits non-zero; the unguarded call in
merge_with_reuse.sh(underset -e) aborts the script before it can finish preparing/authorizing the merge commit.
Impact: the standard, documented reuse-merge path for this PR hard-fails and requires manual intervention (an author edit + re-push) before the merge can complete, even though CI shows green.
Fix: change pr-link at line ~4805 from https://github.com/SemiAnalysisAI/InferenceX/pull/2202 to https://github.com/SemiAnalysisAI/InferenceX/pull/2225 (or use the XXX placeholder, which canonicalize will rewrite to the correct link automatically), matching the convention every other entry in the file follows.
ivanium
force-pushed
the
agent/split-pr-2202-b300
branch
from
dfe01f8 to
407404b
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29432194923 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29445287553 |
1 similar comment
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29445287553 |
|
vLLM recipe PR: vllm-project/recipes#645 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29445287553 |
1 similar comment
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29445287553 |
2 participants
Summary
origin/main.Original PR: #2202
Earlier source PR referenced by #2202: #2188
Testing
bash -n benchmarks/single_node/agentic/dsv4_fp4_b300_vllm.shpython -m pytest utils/matrix_logic/ -q— 221 passed.中文说明
origin/main。原 PR：#2202
#2202 引用的更早来源 PR：#2188
测试
bash -n benchmarks/single_node/agentic/dsv4_fp4_b300_vllm.shpython -m pytest utils/matrix_logic/ -q— 221 项测试通过。