-
Notifications
You must be signed in to change notification settings - Fork 229
[AgentX] vLLM DeepSeek-V4 B300 aggregate / vLLM DeepSeek-V4 B300 聚合配置 #2225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
cquil11
wants to merge
3
commits into
main
Choose a base branch
from
agent/split-pr-2202-b300
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+147
−100
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 The new
vllm-simplebranch (lines 98-116) builds a--kv-transfer-confignamingSimpleCPUOffloadConnectorbut never exportsVLLM_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 usingkv-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: addexport VLLM_USE_SIMPLE_KV_OFFLOAD=1right before buildingOFFLOAD_CONFIGin thevllm-simplecase, mirroring the sibling recipes.Extended reasoning...
The bug: In the new
vllm-simplecase of theKV_OFFLOAD_BACKENDswitch (dsv4_fp4_b300_vllm.sh:98-116), the script builds a--kv-transfer-configJSON withkv_connector: SimpleCPUOffloadConnector, but nowhere in that branch does itexport VLLM_USE_SIMPLE_KV_OFFLOAD=1. This is the only branch in the entire repository that constructs aSimpleCPUOffloadConnectortransfer config without setting that env var.Repo-wide convention (verified directly): Every sibling recipe that builds a
SimpleCPUOffloadConnectorkv-transfer-config setsVLLM_USE_SIMPLE_KV_OFFLOAD=1immediately 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) defensivelyunsets 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 atdsv4_fp4_mi355x_vllm.sh:113-120spells 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 newvllm-simplebranch 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-simpleonly 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:
KV_OFFLOAD_BACKEND=vllm-simple,kv-offloading=dram.case "" in vllm-simple)at line 99.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.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).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=1in thevllm-simple)case, right before (or after) settingPYTHONHASHSEED=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.