diff --git a/.agents/issue-index.md b/.agents/issue-index.md index 7cd159742..80295c225 100644 --- a/.agents/issue-index.md +++ b/.agents/issue-index.md @@ -748,6 +748,8 @@ rather than merged. `scripts/check-agent-record.py` gates both. | [#526](https://github.com/mudler/vllm.cpp/issues/526) | `SERVE-TOOL-HISTORY-ARGS` | OpenAI multi-turn tool history reaches chat templates with string-valued arguments | bug | | [#1934](https://github.com/mudler/vllm.cpp/issues/1934) | `BACKEND-ROCM` | `RocmPlatform::needs_weight_staging()` is stale-false (a W0-era placeholder never revisited despite #523/#509/#506/ROCM_ATTN/hipGraph landing since), so `CheckDeviceWeightFit` — the #1123/#1870 load-time refusal, including the `policy_forces_full_expand` fix — never runs on ROCm: measured directly, `VT_DEVICE_WEIGHT_BUDGET_BYTES=1` produced no refusal on a real load. The actual device allocation the refusal guards is not gated on this flag, so #1870's crash stays reachable until this closes; owed, not fixed in flow, because flipping the flag also moves `DirectDeviceLoadEligible` and several GDN kernel-dispatch defaults that each need their own correctness check | bug | | [#1978](https://github.com/mudler/vllm.cpp/issues/1978) | `MODEL-MM-QWEN4-EXP` | **`Qwen/Qwen3.8-Flash-Next` declares `Qwen4ExpForConditionalGeneration` / `qwen4_exp`, a new architecture vLLM does not implement, so the port runs on a split oracle: transformers for the ALGORITHM, vLLM ops for the OPTIMIZED PATH.** Released 2026-08-24, 180B total / 6B activated, image-text-to-text. The `Qwen3.8` in the name is marketing continuity: `.agents/specs/qwen38-27b-bf16-gate.md`'s "one config key differs" precedent does NOT extend here. Read live 2026-08-26 at vLLM `origin/main` = `6a5e8f5979`: no `qwen4*` path, no registry entry, and a repository-wide GitHub search for `qwen4` returns ZERO results; `vllm-omni` likewise. That is absence from vLLM `main` rather than staleness in our pin `555967922`, so a pin advance does not reach it. What exists is transformers [#48337](https://github.com/huggingface/transformers/pull/48337) "Add Qwen4Exp model", MERGED 2026-08-26, 5211 lines, and SGLang [#36497](https://github.com/sgl-project/sglang/pull/36497), still OPEN and therefore inadmissible. **Developer direction 2026-08-26, recorded verbatim: "use transformers as oracle for algorithmic side. but use ops from vllm so we account for optimized path."** Justified rather than convenient: `Qwen4ExpTextQSAIndexer.forward` loops in Python over `(batch_idx, query_idx)` and says "we only allow eager and sdpa", so porting it as written yields a correct model at an indefensible speed, while AGENTS.md's mirror-vLLM polarity still binds every primitive vLLM implements. `Qwen4ExpTextModel` inherits from `Qwen3_5MoeTextModel` and leaves rotary, MLP, experts, TopK router and the ENTIRE vision tower unchanged (`class Qwen4ExpVisionModel(Qwen3_5MoeVisionModel): pass`), all of which this tree has; GDN is an exact match for our AOT gate (`K=V=128, Hg=16, Hv=48` against `src/vt/cuda/cuda_gdn.cu`'s `H in {48,32}`). The delta is four things, and **exactly two have no vLLM op at all**: the PLE dilated depthwise conv (kernel 4, dilation 3; `git grep dilation` over vLLM `layers/mamba/` = 0 hits) and the n-gram hashed embedding. **The survey's load-bearing finding, and it REVERSES this row's first reading: QSA's structural twin is DeepSeek-V4's C4 indexer lane, NOT MiniMax-M3.** The original call was that QSA, being plain GQA rather than MLA, had to map onto vLLM's non-MLA block-sparse case; that reasoning rested on treating `MLAAttentionSpec` as an MLA claim, and **it is not one** — M3's own indexer cache uses it while M3 is a plain-GQA model, with the comment "Key-only: MLAAttentionSpec budgets one vector/token (not 2x for K+V)". It is a budget shape. Remove that prop and the GQA-vs-MLA argument collapses. Verified line by line at `6a5e8f5979`: **nine independent structural matches with DSv4**, `compress_ratio == 4` literally the same number — MQA index with 1 key head at dim 128; `relu(q.k)` summed over index heads vs `(score.relu() * weights).sum(dim=0)`; `1/sqrt(head_dim)`; one score set per query token with no head axis vs `topk_indices_buffer[num_tokens, topk]`; pooling boundary `(position+1) % COMPRESS_RATIO == 0`; RMSNorm on the pooled key; **RoPE at the block-start position** vs `compressed_pos = (position // CR) * CR`; candidate count `visible // compress_ratio`; and one stored state per 4 tokens via `MLAAttentionSpec(tokens_per_state=compress_ratio)`, a first-class KV field documented as "Ints > 1 compress multiple tokens into one state (DSv4 sparse MLA)" that has no M3 equivalent. **M3 is a DIFFERENT ALGORITHM**, not a worse fit: its score is `tl.max(qk, axis=1)` over 128 RAW token dots with no pooling, no relu and no head reduction, it asserts `num_idx_heads == num_kv_heads` ("no topk index reduce") so it emits one block set PER KV HEAD, and its `SPARSE_BLOCK_SIZE = 128` is welded to the KV page size ("One sparse block == one KV page") on both the score and the attend side — moving it to 4 forces a page size of 4 and breaks `tl.dot`, whose tile needs >= 16. M3 contributes exactly ONE thing and it is a wiring precedent, not an algorithm: that a plain-GQA model can own a key-only side cache through `MLAAttentionSpec` and a private indexer backend. **The genuinely new work is the CONSUMER and nothing upstream supplies it** — every DSv4 sparse consumer attends to COMPRESSED MLA KV (one state per 4 tokens) and M3's attend to raw tokens only at page granularity, while QSA attends to RAW tokens selected at ratio-4 granularity. Two silent-failure traps follow: wiring QSA's top-k into a DSv4 sparse-MLA consumer attends a POOLED key/value and still emits plausible tokens, and **a short-prompt token gate cannot catch it because at context <= `indexer_budget` 2048 every candidate is selected** — so any QSA gate must run past 2048 tokens of context, which is now a stated `## Gates` requirement; and `SparseAttnCompressNormRopeStoreC4Kernel` does NOT mean-pool despite its name — it is a learned softmax pool over an OVERLAPPING window of 8 using a score channel this checkpoint does not have, and the CuteDSL variant refuses `overlap=False` at compile, so the **Triton** `head_dim=128` variant is the correct starting point. Two structural consequences beyond the module list: the residual stream is `hc_count * hidden_size` = **4 x 2560 = 10240 wide through the whole stack** with a low-rank read gate and per-branch scalar write gate around both attention and MLP, which is a change to the per-layer loop and every residual buffer rather than a drop-in module; and `number_of_conv_states = 3` on a PLE layer (GDN conv, PLE conv, n-gram token history) plus the indexer side cache, adjacent to [#1963](https://github.com/mudler/vllm.cpp/issues/1963) and [#1966](https://github.com/mudler/vllm.cpp/issues/1966). **NOTHING PUBLISHED FITS**, read live from the HF API against ~119 GB usable on GB10: BF16 ~360 GB, official FP8 ~180 GB, `RadixArk/...-NVFP4` ~128 GB (NVFP4 backbone with the n-gram table left at FP8, 51.2 GB) and `unsloth/...-GGUF` is a README with ZERO weight files. No GGUF exists and no tool can make one, because llama.cpp has no `qwen4_exp` either, so the standing k-quant requirement means authoring the arch on our side AND states that the quantized arms have NO llama.cpp oracle. **The chosen arm does NOT load today, and the blocker is neither the offload nor the budget: this tree cannot keep a gather table quantized at all.** `KeepQuantKDim` returns `-1` for `GgufTensorRole::kEmbeddingTable` (`src/vllm/model_executor/model_loader/gguf_keep_quant.cpp`), and `qwen3_5_gguf_weights.cpp` asserts it by name — "the embedding table cannot keep quant blocks" — so a Q4_K or Q8_0 n-gram table EXPANDS to bf16 and 51.2B params become **102.4 GB of anonymous memory**; the arm dies before the first forward. The reason was already sitting in a header comment ("a gather, not a GEMM ... A quantized-gather op is a follow-up row") and **no such row exists**. The only non-expanding gather residency is `kKeepF16`, requiring ggml type 1 exactly (102.4 GB on disk) and CPU-ONLY, because `EmbeddingKernelCuda` refuses anything but f32/bf16. **Second blocker:** `moe_intermediate_size = 640` makes `ffn_down_exps` Q4_K-illegal on its reduction dim (640 % 256 = 128), as does `hc_lowrank = 320`; llama.cpp's substitution is believed to be Q5_0 (**UNVERIFIED, owed against the pinned llama.cpp oracle**) and the dependent fact IS verified in-tree — our reader knows ggml ids `0,1,2,8,10..14,16,18,19,22..28,30,39,40,41,66` and has **no entry for 3, 6, 7 or 20**, so a stock `llama-quantize -Q4_K_M` file fails at header parse. We author the converter, so the fix is Q4_0 (block 32, same 4.5 bpw). **`ENG-WEIGHT-OFFLOAD` will not help** — it moves zero bytes today (`ConsiderWeight` has no production callers, pinned by a test) and is documented inert on GB10; the tier that DOES work already ships and is proven by the 2.4T model serving 369.97 GiB from a 119.631 GiB box at ~62 GiB resident: mmap `MAP_PRIVATE`, borrow in place, alias the host pointer, `prefault: false`. Corrected sizing: backbone ~67.7 GiB, whole process ~73.5 GiB of 119.631 at 32K single-stream, ~46 GiB of headroom for the page cache, so the ~76 GB estimate was right within 10%. The design works because per-token demand is **<= 64 KiB of reads** (16 lookups x 160 dims over at most 16 pages) against the 2.4T expert lane's 6.95 GB/token. The architecture supplies its own lever: the per-token n-gram cost is `(ngram_size-1)*heads_per_ngram` = 16 lookups of 160 dims, so **51 GB of the 180 GB, 28% of the model, is a table touched 16 times per token** and making it non-resident is the intended design point (RadixArk reached the same split independently). Sizing arithmetic, NOT measurement: Q8_0 throughout ~191 GB (no), Q4_K_M throughout ~109 GB (yes, ~10 GB left for KV), Q4_K_M backbone with the table non-resident ~76 GB. GB10 is UNIFIED memory so "offload to host" is not a move there; non-resident means disk-backed, and its cost is unmeasured. **Two decisions were put to the developer as explicit accept-or-reject and BOTH are settled 2026-08-26, recorded in place rather than left open.** (1) `.agents/oracles/transformers.md` pins transformers to 5.14.1, deliberately tied to what the pinned vLLM environment resolves so the environment cannot hold two `transformers` at once, and **5.14.1 does not contain `Qwen4Exp`**; the lane-scoped second pin is **ACCEPTED**, on the argument that the invariant guards a vLLM environment against drifting from its transformers and here there is no vLLM implementation to drift from, and it expires the moment vLLM registers `qwen4_exp`. **The lane pin is a real release, not a branch SHA**, which was not the expected outcome: `Qwen4Exp` merged to `main` at 12:03:40Z on 2026-08-26 and `v5.16.0` published at 12:35:15Z, and this was BOUNDED rather than assumed by fetching `models/qwen4_exp/modeling_qwen4_exp.py` at each tag — `v5.16.0` HTTP **200**, `v5.15.0` HTTP **404** — making 5.16.0 the FIRST release carrying the architecture and therefore the tightest available pin. The version string is UNMEASURED (it is the release proven to contain the model, not a `transformers.__version__` read off a running oracle) and `gateable` stays `no`. (2) The first runnable arm is the **Q4_K_M backbone with the n-gram table NON-RESIDENT** (~76 GB). Q8_0 was raised and does not fit at ~191 GB, and no partial-Q8 split reaches 119 GB with the backbone at 8 bits; Q4_K_M-throughout fits on paper at ~109 GB but leaves ~10 GB for KV and activations on a 262144-native-context model, which is not a margin. This promotes the non-resident table from a note to a first-class W6 deliverable, and it is NOT free: GB10 is UNIFIED memory, so the existing host-pinned offload seam (`ENG-WEIGHT-OFFLOAD`, mirroring vLLM's `cpu_offload_gb`) does not by itself solve it there and the mechanism must be disk-backed or genuinely unloaded — established before it is designed around. Spec: [`specs/qwen4-exp-flash-next.md`](specs/qwen4-exp-flash-next.md). No product code lands under the spec pull request | feature | +| [#1963](https://github.com/mudler/vllm.cpp/issues/1963) | `ROAD-V1-MEM` | At ctx=32768 `--max-num-seqs 32` our engine consumes ~108 GB during load and never serves; vLLM and SGLang both serve there. Root cause found and fixed by `FIX-KV-GROUP-LAYER-COUNT` ([spec](specs/kv-group-layer-count.md)): thirty-three of thirty-four registries publish ONE placeholder name per KV group, `KVBytesPerBlock` reads `layer_names.size()` as the layer count, and `ResolveNumBlocks` arm 2 therefore divides an absolute `--kv-cache-memory` budget by ONE layer's page while the runner allocates one buffer per layer — measured 8.5 GiB allocated for a 1 GiB budget on the 27B | bug | +| [#1966](https://github.com/mudler/vllm.cpp/issues/1966) | `ROAD-V1-MEM` | The #371 recurrent-state OOM guard is 48x under and passes a config that allocates 43.4 GiB, because `recurrent_state_bytes` counts placeholder layer names. Same root defect as [#1963](https://github.com/mudler/vllm.cpp/issues/1963), different code path; both fixed by `FIX-KV-GROUP-LAYER-COUNT` ([spec](specs/kv-group-layer-count.md)) | bug | | [#1982](https://github.com/mudler/vllm.cpp/issues/1982) | `SERVE-STREAM-USAGE` | **`ChatSseStream::next` writes the `/v1/chat/completions` role frame before it reads anything from the engine, so `vllm bench serve --backend openai-chat` stamps TTFT on an empty frame and our TTFT through that harness is an HTTP round trip, not a time to first token.** Upstream builds the role chunk under `if first_iteration:` inside `async for res in result_generator:` (`vllm/entrypoints/openai/chat_completion/serving.py:477,487`) and says why at `:484-486`: an exception in the generator "needs to be sent as the FIRST response". `vllm/benchmarks/lib/endpoint_request_func.py:404-408` guards on the presence of `choices`, not on non-empty `delta.content`, and our role frame carries `delta.content = ""` with no `usage`. vLLM and SGLang order the frame after the first result, so their rows on the same harness are honest and only ours is not; this blocks the #1574 three-engine TTFT row. `.agents/specs/stream-options.md` scoped the buffering to continuous usage on purpose and both its passages are corrected here. Fixed by removing the `usage_.include_continuous_usage` guard around the first-result buffering loop, so the default path buffers too. Spec: [`specs/chat-role-frame-ordering.md`](specs/chat-role-frame-ordering.md) | bug | | [#1992](https://github.com/mudler/vllm.cpp/issues/1992) | — | **Neither `ChatSseStream::next` nor `CompletionSseStream::next` converts an engine exception into a `data: {"error": …}` frame, so a streaming request that fails is a truncated 200 and the cause reaches only `stderr`.** Upstream yields the error frame and then `data: [DONE]` from the generator's `except GenerationError` / `except Exception` arms (`vllm/entrypoints/openai/chat_completion/serving.py:827-833` at the pin `555967922`), and that frame is what makes the first-iteration ordering at `:484-486` mean anything: the role chunk is built inside the loop so an exception can be the FIRST response, which needs a response to exist. Ours propagates out of `next()` into the cpp-httplib chunked content provider (`src/vllm/entrypoints/openai/api_server.cpp::ApiServer::register_routes`), which logs `sse: stream aborted mid-flight:` and aborts, so a client cannot tell a failed request from a short one. Found while fixing [#1982](https://github.com/mudler/vllm.cpp/issues/1982) and NOT fixed in that flow: upstream's `try` wraps the whole generator, so the frame is owed for mid-stream failures on both endpoints, and that is a different blast radius needing its own red-first cases for the payload shape, the trailing `[DONE]` and the separate `GenerationError` converter. Owed by [`specs/chat-role-frame-ordering.md`](specs/chat-role-frame-ordering.md) `## Owed` | bug | | [#1983](https://github.com/mudler/vllm.cpp/issues/1983) | `KV-GDN-STATE-BUDGET` | **The GDN recurrent-state pool is preallocated per CONFIGURED sequence, on an axis no flag bounds.** `GPUModelRunner::initialize_kv_cache` sizes `gdn_state_slots_ = max_num_reqs * (num_spec + 1)` and allocates one conv and one SSM buffer per GDN layer from it, each `Memset` to zero at construction, so every byte is resident before the first request. Re-derived for `Qwen3.8-27B` (48 linear-attention layers, `Hk/Hv/Dk/Dv/conv = 16/48/128/128/4`, `mamba_ssm_dtype = float32`) at `num_speculative_tokens = 8`: one slot costs 3,371,008 B per layer, 154.31 MiB across 48 layers, so one sequence costs 1.356 GiB and `--max-num-seqs 32` costs **43.40 GiB** that `--kv-cache-memory`, `--num-blocks` and `--gpu-memory-utilization` all fail to bound. The per-sequence cost is NOT the divergence — upstream charges the same `1 + num_speculative_blocks` state blocks (`vllm/v1/kv_cache_interface.py::MambaSpec.max_memory_usage_bytes`) and our `f32` SSM mirrors the checkpoint's own `mamba_ssm_dtype` — the AXIS is: `max_num_seqs` sizes no allocation anywhere in vLLM. Upstream raises the attention block size until one attention page holds one mamba page (`vllm/platforms/interface.py::Platform.check_and_update_config`), pads the mamba page to match, and then draws BOTH from one budgeted pool whose tensors are `shared_by` one layer from each group (`kv_cache_utils.py::_get_kv_cache_config_uniform_page_size`), so its recurrent allocation is a function of available memory and never of the concurrency cap. Fixed by mirroring that arithmetic in `ComputeHybridKvBudget` — `unified_block_tokens = align * cdiv(mamba_page, align * attn_bytes_per_token)`, `max_state_seqs = (num_blocks * block_size / unified_block_tokens) / (1 + num_spec)` — and resolving ONE `max_num_seqs` from it for the runner, the scheduler and the #371 guard alike. The bound reads no layer count (upstream's per-layer page equality cancels it), so it does not depend on the placeholder-layer-name repair owned by [#1963](https://github.com/mudler/vllm.cpp/issues/1963) and [#1966](https://github.com/mudler/vllm.cpp/issues/1966), and it lands in its own translation unit so the three rows share no edit surface. Spec: [`specs/gdn-state-kv-budget.md`](specs/gdn-state-kv-budget.md) | bug | diff --git a/.agents/specs/kv-group-layer-count.md b/.agents/specs/kv-group-layer-count.md new file mode 100644 index 000000000..894f5a9d2 --- /dev/null +++ b/.agents/specs/kv-group-layer-count.md @@ -0,0 +1,410 @@ +# FIX-KV-GROUP-LAYER-COUNT — the KV byte accounting counts placeholder names, so the engine allocates what it was never given + +Work row `FIX-KV-GROUP-LAYER-COUNT`, owned by `ROAD-V1-MEM` +([`kv-sizing.md`](kv-sizing.md), issue +[#83](https://github.com/mudler/vllm.cpp/issues/83)) — which is the row the two +issue-index entries name, because the KV pool sizing is its surface. +Issues: [#1963](https://github.com/mudler/vllm.cpp/issues/1963), +[#1966](https://github.com/mudler/vllm.cpp/issues/1966). +Base: `d9a528528`. **Every `file:line` below is read at that base**, including the ones in files this row edits, so a reader diffing them against the head will find them moved. + +## Now + +`ACTIVE` in [#2000](https://github.com/mudler/vllm.cpp/pull/2000). One pull +request carries the spec and the implementation, in that commit order. + +**Merged with `KV-GDN-STATE-BUDGET` ([#1999](https://github.com/mudler/vllm.cpp/pull/1999), +issue [#1983](https://github.com/mudler/vllm.cpp/issues/1983)), which landed +first.** Five files overlap; four three-way-merged and one conflicted. Every one +was resolved by the rule in AGENTS.md `## Records` rather than by accepting the +automatic merge: take `origin/main`'s complete file, prove it byte-identical, +re-apply this row's scoped edit at an anchor asserted unique, then confirm +`git diff origin/main -- ` carries only this row's lines. The index was +union-appended and checked by row-ID set difference (729 base + 2 ours + 1 +theirs = 732; 0 lost, 0 invented, 0 duplicated). The test file was resolved the same +way — `origin/main`'s complete file, then this row's three scoped edits +re-applied — and checked by assertion, not by eye: 30 `TEST_CASE`s (main's 25 +plus this row's 5), zero duplicate names, zero repeated top-level identifiers, +zero duplicate includes, and a brace balance equal to `origin/main`'s. + +**That last check is there because the first attempt passed every other one on a +file that did not compile.** Taking the two conflict sides verbatim looked +right and was wrong: git had hoisted the closing `}` both blocks end with out of +the conflict region as shared trailing context, so `` and `` each +arrived one brace short and a single `}` closed the pair. `git diff` showed a +clean additive change, marker count was zero, `TEST_CASE` names were unique, and +the identifier and include checks were green — six agreeing instruments, none of +which was measuring whether the file parsed. The compiler was, in ten lines: +`error: cannot declare static function inside another function`, five times over. + +The lesson is the one [`verification.md`](../verification.md) states about +instruments pointed at the wrong thing, and it costs nothing to fix: a +structural check on a merge resolution must include a structural INVARIANT of +the language (here, brace balance against the pre-merge file), and the +resolution is not verified until the target is BUILT. A name-uniqueness check +proves two blocks do not collide; it cannot prove either block is intact. +Taking `origin/main`'s complete file and re-applying scoped edits at unique +anchors — the AGENTS.md `## Records` rule — avoids the whole failure mode, +because no block is ever reconstructed from a conflict region. + +One case changed in that merge and it is a correctness change, not a textual +one. Case 4 asserted `recurrent_state_bytes(cfg, params.max_num_seqs)`, while +#1983 makes the constructor hand the runner the RESOLVED concurrency. The two +agree only while `ResolveMaxNumSeqs` does not clamp — true here by a 64x margin +(256 seats against the 4 asked), which is right by accident rather than by +construction. It now reads `eng.max_num_seqs()`. + +Three things are still outstanding and none of them is this row's to do alone: +a fresh scoped review of the immutable head, the operator rerunning the gate, +and the device confirmation below, which needs a lease this row does not hold. + +**The device confirmation, with its predicted values stated first.** Same +launch as [#1963](https://github.com/mudler/vllm.cpp/issues/1963) — +`r0b0tlab/Qwen3.8-27B-NVFP4-MTP-sm121` plus the `qwen3.8-27b-dflash2` draft, +`--max-model-len 32768 --max-num-seqs 32 --no-enable-prefix-caching +--speculative-config '{"method":"dflash","num_speculative_tokens":8}'`, with +`VT_KV_ALLOC_LOG=1`. + +| Run | Base measured | Predicted with this change | +|---|---|---| +| `--kv-cache-memory 1073741824` | `page_size_bytes=131072 num_blocks=4096` | `page_size_bytes=131072 num_blocks=481` | +| paged bytes allocated, same run | 17 x 4096 x 131072 = 9126805504 B = 8.50 GiB | 17 x 481 x 131072 = 1071775744 B = 0.998 GiB | +| `--kv-cache-memory 6442450944` | `num_blocks=24576`, 51.00 GiB paged | `num_blocks=2891`, 5.999 GiB paged | +| recurrent state, either run | allocated 43.40 GiB, guard REPORTED 0.90 GiB | allocated 2.71 GiB, guard reports 2.71 GiB | + +The recurrent allocation does not change; only what the guard says about it +does, so on a box with more than 43.40 GiB free the guard still does not +refuse — which is correct. The observable is the number in the refusal message +when it is forced (lower `MemAvailable`, or raise `--max-num-seqs`). + +**The recurrent row changed when `KV-GDN-STATE-BUDGET` (#1983) landed first, +and the two fixes compose rather than fight.** `ComputeHybridKvBudget` never +reads `layer_names`; the only input of its arithmetic this row moves is +`kv_cfg.num_blocks`. Upstream's `num_blocks` is a PER-LAYER count +(`kv_cache_utils.py:1008` divides by `num_layers`), which is the meaning its +unification against one attention page assumes — and before this row the +byte-budget path handed it a count inflated by the layer count, so its clamp +was too permissive. Feeding it a truthful pool is what makes its seat count +correct. + +Worked through for the run above, at `attn_bytes_per_token` = 131072/32 = 4096 +and a 3,371,008-byte GDN state: `unified_block_tokens` = 32 x ceil(3371008 / +131072) = **832**, `unified_num_blocks` = 481 x 32 / 832 = **18**, +`slots_per_seq` = 1 + 8 = 9, so **2 seats** and `max_num_seqs` clamps 32 -> 2. +The recurrent allocation is then 3371008 x 48 x (2 x 9) = 2,912,550,912 B = +**2.71 GiB**, and the guard reports that same number. Whole-engine KV: **3.71 +GiB against the base tree's 51.90 GiB** at the same flag. + +That arithmetic is checkable against #1983's own output rather than against +itself: its engine prints `The KV pool (3072 blocks) holds 118 unified pages of +832 tokens`, and 3072 x 32 / 832 = 118.15 -> 118. The formula above reproduces +both numbers. + +**The operational consequence, which looks like a regression and is not.** At a +fixed `--kv-cache-memory` the seat count now falls by the same factor the pool +does — 8.5x on this spec-on launch (17 pages against the base's 2), 16x +spec-off. To seat 32 concurrent sequences at k=8 the budget must be +`32 x 9 x 832 / 32` = 7488 blocks, i.e. `--kv-cache-memory` >= 16684941312 +(15.54 GiB). `--num-blocks` is unaffected and always was: `ResolveNumBlocks` +arm 1 returns it verbatim, and only the byte-budget path converts differently. + +`num_blocks` is written for **16** target full-attention layers, which is what +the base measurement implies (the base divisor was two pages, one `fa` and one +`fa_draft`, and 1 GiB / 262144 = 4096). If the checkpoint has N rather than 16, +the prediction is `1073741824 / ((N + 1) * 131072)`, and a mismatch is a fact +about the checkpoint rather than about this change — but say so, because it +would mean the 8.5x in the issue is also the wrong multiple. + +## The defect + +`KVCacheGroupSpec::layer_names` is upstream's per-layer name list +(`vllm/v1/core/kv_cache_utils.py:1209-1211` builds it by appending EVERY layer +that shares a spec object, and `:1399` reads `len(group.layer_names)` as the +layer count that bounds the allocation). Thirty-three of our thirty-four +registries publish a single PLACEHOLDER string instead — +`std::vector{"fa"}`, `{"gdn"}`, `{"mla"}`, `{"kda"}`, +`{"fa_draft"}`, `{"encoder"}`. `NemotronHForCausalLM` is the only one that +publishes real names (`nemotron_h_registry.cpp:274-277`), and it does so because +[#810](https://github.com/mudler/vllm.cpp/issues/810) found the same class of +bug from the other side. + +The runner states the convention in its own source +(`src/vllm/v1/worker/gpu/runner.cpp:826-829`): *"Every registry shipping today +publishes a single PLACEHOLDER name per group"*. Three consumers then read +`layer_names.size()` as if it were the layer count. + +| Consumer | `file:line` at the base | Verdict | +|---|---|---| +| `KVBytesPerBlock` | `src/vllm/v1/kv_cache_interface.cpp:263-264` | **WRONG** — under by the real layer count | +| `recurrent_state_bytes` | `src/vllm/v1/core/kv_cache_utils.cpp:975-981` | **WRONG** — under by the real layer count | +| `GroupLayerMask` (runner) | `src/vllm/v1/worker/gpu/runner.cpp:369-371` | correct — it refuses a placeholder by design and falls back | + +`KVBytesPerBlock` itself has exactly two product call sites, and only one of +them is defective: + +| Call site | `file:line` | Verdict | +|---|---|---| +| `ResolveNumBlocks` arm 2 | `src/vllm/entrypoints/model_loader.cpp:1416` | **WRONG** — divides an ABSOLUTE byte budget by a per-layer page | +| `ResolveMaxModelLen` | `src/vllm/entrypoints/model_loader.cpp:1564` | correct — the factor appears on BOTH sides (`available = num_blocks * bpb`, `needed = ceil(len/bs) * bpb`, `estimate_max_model_len(available, bpb, bs)`, `auto_fit_max_model_len(derived, available, bpb, bs)`) and cancels, reducing to a comparison in blocks | + +The narrowing the dispatch stated therefore HELD, and no fourth product consumer +exists (`grep -rn 'KVBytesPerBlock' src include` returns those two plus header +prose and one comment cross-reference at `kv_cache_utils.cpp:1018`). + +### #1963 — the pool is allocated N times over + +`ResolveNumBlocks` computes `num_blocks = budget / KVBytesPerBlock(probe)`. +`GPUModelRunner::initialize_kv_cache` then allocates one `CacheBuffer` of +`num_blocks * page_size_bytes()` **per full-attention layer** +(`runner.cpp:974-977`) plus one for the draft layer (`:1131-1133`). With +placeholder names the divisor counts one layer per group, so the allocation +overshoots the budget by the real layer count. + +Measured on device before this row (operator run, `--kv-cache-memory +1073741824`): `[kv-alloc] ... page_size_bytes=131072 num_blocks=4096`. Divisor += 131072 (`fa`) + 131072 (`fa_draft`) = 262144, and 1 GiB / 262144 = 4096. +The 27B has 64 layers, 48 of them `linear_attention`, so 16 full-attention +layers plus 1 draft layer: `17 * 4096 * 131072` = **8.5 GiB allocated for a +1 GiB budget**. + +### #1966 — the #371 recurrent-state OOM guard is 48x under + +`recurrent_state_bytes` multiplies the Mamba page by `layer_names.size()` == 1 +where the runner allocates a conv and an SSM buffer for each of the 48 GDN +layers (`runner.cpp:920-931`). At `--max-num-seqs 32`, `k=8` it reports 0.90 GiB +against a 43.40 GiB allocation. That guard exists because an oversized recurrent +state *"took the machine down rather than failing, which is exactly what it did +four times on 2026-08-11"* (`model_loader.cpp:1885-1890`), and on the only +family it can fire for it does not fire. + +The two together account for most of the #1963 collapse arithmetically: at +`--kv-cache-memory 6GiB`, `k=8`, `--max-num-seqs 32` the base tree allocates +`17 * (6GiB / 262144) * 131072` = 54,760,833,024 B = **51.00 GiB** of paged +pool plus **43.40 GiB** of recurrent state = **94.40 GiB**, against the ~108 GB +(100.6 GiB) the watchdog saw. The residue is the weights and the transients the +issue's third candidate names, and this row does not claim it. + +## What upstream does, and why it cannot have this bug + +`vllm/v1/core/kv_cache_utils.py` at the pin `555967922`: + +- `:1399` `group_size = max(len(group.layer_names) for group in kv_cache_groups)` +- `:1400-1402` one uniform `page_size` for every group +- `:1005-1008` `get_num_blocks` -> `available_memory // page_size // num_layers` +- `:1409-1416` allocates exactly `group_size` tensors of `page_size * num_blocks` + +The count that divides the budget and the count that multiplies the allocation +are **the same expression over the same list**, so the product is bounded by +construction. Ours are two independent derivations of one number, which is the +shape that can disagree — and did. + +## Design + +**Make `layer_names` mean what upstream means by it, in ONE place, derived from +the same predicate the runner allocates by.** + +A new `vllm::v1::ResolveKVCacheGroupLayerNames(KVCacheConfig&, num_hidden_layers, +layer_types)` rewrites placeholder group names into the real per-layer module +names the runner's own classification implies. It is called from +`LoadedEngine::MakeKVCacheMaybeSpec` (`model_loader.cpp`), the single funnel +through which every production `KVCacheConfig` passes — the spec-on branch +(`MakeQwen3_5KVCacheSpec`) and the spec-off branch +(`ModelRegistry::MakeKVCache`, all 34 registries) both return through it, and +`MakeKVCacheResolved` calls it for both the probe and the resized config. + +The classification is a **mirror of `runner.cpp`'s fallback**, not a second +derivation of it: + +- `is_gdn(l)` == `has_mamba_group && !layer_types.empty() && layer_types[l] == + "linear_attention"` — character for character the runner's own predicate + (`runner.cpp:905-908`). +- the TARGET attention group is the first non-eagle attention-kind group, which + is the runner's own first-wins selection (`runner.cpp:586-592`). +- a SECOND attention group is the draft layer: exactly one layer, at index + `num_hidden_layers`, mirroring upstream's MTP head index + (`qwen3_5_mtp.py:105-112`) and the runner's single draft buffer + (`runner.cpp:1128-1134`, which `break`s after one). +- a THIRD or later attention group gets an EMPTY list, because the runner + allocates no buffer for it. Zero is the honest count, and no registry emits + one today. + +Because the resolver reproduces the allocator's predicate rather than +re-deriving the model's shape, the accounting cannot disagree with the +allocation. That equality is what the gate asserts, in bytes, from a real +registry config. + +**A registry that already publishes real names is never overwritten.** If any +group carries a name that `KVCacheLayerIndexOfName` resolves, the resolver +returns untouched. NemotronH is that case, and it knows more than the fallback +can: its `layer_types` is empty and its MoE blocks cache nothing, which is the +whole point of #810. + +`LayerIndexOfName` moves out of `runner.cpp`'s anonymous namespace into +`vllm/v1/kv_cache_interface.{h,cpp}` as `KVCacheLayerIndexOfName`, so the +resolver and the runner parse names with one function rather than two copies. + +`is_eagle_group` is deliberately NOT set on the `fa_draft` group. It would be +semantically apt, but `KVCacheCoordinator` reads that flag +(`kv_cache_coordinator.cpp:104-114`) and flipping it changes which groups the +eagle path treats as draft groups. That is a different change with a different +blast radius. + +### The instrument + +`GPUModelRunner::kv_cache_allocated_bytes()` sums the byte size of every +`CacheBuffer` `initialize_kv_cache` created (`full_attn_buf_`, `draft_attn_buf_`, +`ssm_buf_`, `conv_buf_`), and `kv_cache_allocated_paged_bytes()` sums only the +block-scaled half. `CacheBuffer` records the size it was constructed with. These +report what the allocator DID, in its own output, so the gate compares an +accounting number against a measurement rather than against a second +transcription of the same formula. + +## Tests + +The cases live where their harness already lives, not in a new file with a +copied harness: the engine-level ones in +`tests/vllm/entrypoints/test_loaded_engine_dense.cpp` (which builds a +`LoadedEngine` over synthetic in-memory weights, the production entry point), +and the "real names survive" one in +`tests/vllm/models/test_nemotron_h_scaffold.cpp` (which builds the real +NemotronH KV config). Every case is named `kv-group-layer-count: ...` so one +doctest filter runs the set. + +Every engine case is driven from `MakeQwen3_5KVCacheSpec` — the shipping +registry helper — through the loader, never from a hand-built +`KVCacheGroupSpec`. The pre-existing +`tests/vllm/v1/test_kv_cache_interface.cpp:425-433` is exactly the trap this row +exists to close: it hands the function `KVCacheGroupSpec{{"layer1", "layer2"}, +ref}`, a shape no registry emits, so the multiplier under test was never the +multiplier in production. That case stays — it pins the formula — and is no +longer the only coverage. + +The config is a 6-layer Qwen3.5 hybrid, `layer_types` = [LA, LA, FA, LA, LA, +FA]: **two** full-attention layers and **four** GDN layers, so the placeholder +count is wrong on both halves and by different factors. `MakeDenseConfig`, which +every other case in that file uses, has [LA, LA, LA, FA] — exactly ONE +full-attention layer, which is the config in which this bug is invisible. + +1. **`--kv-cache-memory` bounds the bytes the runner allocates.** `LoadedEngine` + with `kv_cache_memory_bytes = 1 MiB` -> + `runner().kv_cache_allocated_paged_bytes() <= 1 MiB`. +2. **`KVBytesPerBlock` equals the per-block cost the allocator paid.** + `KVBytesPerBlock(cfg) * cfg.num_blocks == + runner().kv_cache_allocated_paged_bytes()`. +3. **The loader resolves the placeholders.** The raw registry config carries one + name per group; the engine's carries 2 and 4. +4. **`recurrent_state_bytes` equals the recurrent bytes the runner allocates.** + `recurrent_state_bytes(cfg, max_num_seqs) == allocated - paged`. +5. **The `fa_draft` group weighs exactly one layer.** + `MakeQwen3_5KVCacheSpec(num_spec=4)` -> `KVBytesPerBlock == page * 3`, and + the draft name is `model.layers..self_attn.attn`. +6. **NemotronH is not overwritten.** Its 6 attention names and 23 Mamba names + survive the resolver and `KVBytesPerBlock` stays `page * 6`. + +Cases 1 to 4 enter through the `LoadedEngine` constructor, so they are also the +reachability gate: the chain is `LoadedEngine` ctor -> `MakeKVCacheResolved` -> +`MakeKVCacheMaybeSpec` -> `ResolveKVCacheGroupLayerNames` -> `ResolveNumBlocks` +-> `GPUModelRunner::initialize_kv_cache`, and deleting the resolver call site +reds all four. Case 5 calls the resolver directly and stays green under that +mutation, which is correct and is why it is not the reachability case. + +### Red before, green after + +Mutation: the `ResolveKVCacheGroupLayerNames` call in `MakeKVCacheMaybeSpec` +replaced by `// MUTATION: production call site deleted.`, rebuilt, rerun. + +``` +[doctest] test cases: 5 | 1 passed | 4 failed | 19 skipped +[doctest] assertions: 21 | 16 passed | 5 failed | + +CHECK( allocated <= params.kv_cache_memory_bytes ) 2097152 <= 1048576 +CHECK( KVBytesPerBlock(kv) * kv.num_blocks == ...paged ) 1048576 == 2097152 +CHECK( kv_cache_groups[0].layer_names.size() == 2 ) 1 == 2 +CHECK( kv_cache_groups[1].layer_names.size() == 4 ) 1 == 4 +CHECK( recurrent_state_bytes(cfg, 4) == recurrent ) 4992 == 19968 +``` + +Exactly 2x on the paged half (2 full-attention layers) and exactly 4x on the +recurrent half (4 GDN layers), which is the defect stated in bytes. Restored, +rebuilt, rerun: `5 passed | 0 failed`, `21 assertions | 21 passed`. + +## Gates + +```sh +cmake -S . -B build -G Ninja -DVLLM_CPP_BUILD_TESTS=ON +cmake --build build -j 4 +./build/tests/test_loaded_engine_dense -tc='kv-group-layer-count*' +./build/tests/test_nemotron_h_scaffold +ctest --test-dir build --output-on-failure +scripts/agent-preflight.sh --staged +``` + +**Results.** On the pre-merge head: `627/627 tests passed`, `CTEST rc=0`. On +the head after merging `origin/main` and with the box under heavy contention +from other worktrees' builds: `625/627`, with `test_dflash2_ctx_capacity` and +`test_async_llm` red under `ctest -j 4`. Both are green when re-run serially on +the same binary — `6/6` and `77 assertions` for the first, `15/15` and `494 +assertions` for the second — which is the starvation case +[`verification.md`](../verification.md) names, not a regression. Neither test +reads `layer_names`, `KVBytesPerBlock` or `recurrent_state_bytes`. + +**On the head that merges `KV-GDN-STATE-BUDGET` (#1999): `BUILD rc=0` and +`628/628 tests passed`, `CTEST rc=0`**, with no flake to re-run and a disk guard +that never fired. The interaction surface was also gated target by target, +exit code captured per binary rather than inferred from a summary line: + +| target | rc | result | +|---|---|---| +| `test_hybrid_kv_budget` (#1983's own) | 0 | 8/8 | +| `test_kv_state_budget` (#371's guard) | 0 | 5/5 | +| `test_kv_cache_interface` | 0 | 43/43 | +| `test_runner` | 0 | 20/20 | +| `test_nemotron_h_scaffold` | 0 | 14/14 | +| `test_kv_cache_fp8_wiring` | 0 | 31/31 | +| `test_loaded_engine_dense` | 0 | 30/30, 128 assertions | + +`test_hybrid_kv_budget` passing unchanged is the executable form of the claim +that the two rows compose: #1983's own gate is green against a tree where +`num_blocks` means something different from what it meant when that gate was +written. + +The build type is left unset, exactly as the CPU CI job configures it +(`.github/workflows/ci.yml`). A `RelWithDebInfo` tree of this repository links +about 170 test executables against a static `libvllm.a` carrying debug info and +takes **86 GB**; that filled the disk twice during this row and made unrelated +checkers emit ENOSPC as policy refusals. The CI configuration builds the same +sources into about 4 GB. + +## Risks + +- **The runner's by-name membership path now engages for hybrids.** With real + names published, `GroupLayerMask` resolves and `membership_by_name` becomes + true for Qwen3.5 and Kimi-Linear, where it was false. The masks are derived + from `layer_types` — the exact predicate the fallback uses — so the + classification is identical by construction. Case 1 and case 2 measure the + resulting allocation in bytes rather than asserting the claim. +- **Dense models are untouched in the runner.** `gdn_layer_mask` is only + computed when the config has a Mamba group, so a dense or MLA model keeps the + fallback classification whatever its names are. Only the byte accounting + changes, from 1 layer to `num_hidden_layers`. +- **A pool sized from a budget now buys N times fewer blocks.** That is the + fix, not a regression: the blocks it stops promising were never allocated + within the budget. A user who had tuned `--kv-cache-memory` against the old + behaviour will see a shorter auto-fitted context, and `ResolveMaxModelLen` + reports that on stderr already. + +## Out of scope + +- `--gpu-memory-utilization` being inert ([#83](https://github.com/mudler/vllm.cpp/issues/83)) + and `kFallbackNumBlocks = 256`. Neither reads `layer_names`. +- Making the registries publish real names at their own call sites. The central + resolver reaches all 34 without 33 hand-written loops that could each be + wrong; a registry that wants finer classification than `layer_types` gives + publishes its own names and the resolver stands aside, exactly as NemotronH + does today. + +## Stop conditions + +Stop and report if the resolver changes the allocated byte total for any +existing model other than through the layer count — that would mean the mirror +of the runner's predicate is not a mirror. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index ab7e0a0f8..1f15e0949 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -386,7 +386,7 @@ CPU elementwise GEMM (f32/f16/bf16) runs AVX2 and AVX-512 tiers on x86 where the | ROCm | W0 community-verified on 5 gfx archs; classic-dense and GDN-hybrid e2e run all-native; correctness gaps remain | 49 registered ops including the GDN state/conv/postconv/recurrence set, MoE combine/gate, and keep-quant expert GEMM; APU managed-allocation branch remains unverified. [ROCm guide](ROCM.md) | | XPU, TPU | Not started | CUDA, CPU, Metal and Vulkan are the built backends | | Custom logits processors on CUDA | Open, not root-caused | Segfaults in a CUDA build, 232/232 green on CPU | -| Memory budgeting (`ROAD-V1-MEM`, #83) | M1+M2 landed (absolute bytes) | `--kv-cache-memory` sizes the KV pool from an absolute byte budget (ABI v16, group-aware divisor); `--num-blocks` overrides; `--gpu-memory-utilization` needs the M3 profile run (dgx-gated). See `specs/kv-sizing.md` | +| Memory budgeting (`ROAD-V1-MEM`, #83) | M1+M2 landed (absolute bytes) | `--kv-cache-memory` sizes the KV pool from an absolute byte budget (ABI v16, per-layer divisor since #1963 — the group-aware one counted placeholder names and overshot by the layer count); `--num-blocks` overrides; `--gpu-memory-utilization` needs the M3 profile run (dgx-gated). See `specs/kv-sizing.md` | | Gemma4 MoE ROCm FP8 + SharedK-WMMA | Partial | Dual-GPU FP8 resident experts, SharedK-WMMA prefill (RDNA4); decode-graph and forward extract deferred. Env `VT_GEMMA4_*`/`VT_ATTN_*`, seam `test_gemma4_rocm_fp8_seams`. [spec](../.agents/specs/gemma4-rocm-fp8-moe.md) | ## How to read this page diff --git a/docs/USAGE.md b/docs/USAGE.md index 7ff8d3cc7..79febfbc5 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -208,6 +208,24 @@ Note that `--kv-cache-memory` is what turns the halved block into twice the pool. Without it the server falls back to a fixed block count, and `fp8` then halves the KV bytes for the same context instead. +**`--kv-cache-memory` now bounds the whole pool, and it did not before.** The +value is an absolute budget for the paged KV cache, and the engine sizes the +block count so that everything it allocates fits inside it — which is what vLLM +means by the flag. Until #1963 the divisor counted one layer per KV group while +the engine allocated a buffer per layer, so the same number bought as many times +the memory as the model has attention layers: 8.5 GiB of buffers for +`--kv-cache-memory 1073741824` on the 27B. If you tuned this flag against the +old behaviour, the same value now gives a shorter served context; raise it, and +the auto-fit line on stderr tells you what it settled on. + +`--num-blocks` is unaffected: it names a per-layer block count and always did, +so a launch line that sizes the pool that way means exactly what it meant +before. Only the byte budget converts differently. The recurrent-state clamp +(#1983) reads the resolved block count, so at a fixed `--kv-cache-memory` it +now seats fewer concurrent sequences than it did — it is being told the pool's +true size for the first time, and the `INFO recurrent-state budget:` line names +what it compared. + **It costs you the fast attention kernels, and we have not measured the net.** An fp8 KV cache is read by the tiled prefill and block decode kernels only. FA-2 prefill, all three FA-2 decode topologies, the WMMA ladder and the diff --git a/include/vllm/v1/kv_cache_interface.h b/include/vllm/v1/kv_cache_interface.h index 6dd40c876..8c4a13120 100644 --- a/include/vllm/v1/kv_cache_interface.h +++ b/include/vllm/v1/kv_cache_interface.h @@ -78,6 +78,7 @@ #include #include #include +#include #include #include "vllm/v1/kv_cache_dtype.h" @@ -559,6 +560,66 @@ struct KVCacheConfig { // spec's own `page_size_bytes()` throws (deferred quantized-KV math). int64_t KVBytesPerBlock(const KVCacheConfig& config); +// The model layer index encoded in an upstream-style KV layer name, or nullopt +// when the name carries no layer identity. +// +// Upstream's `KVCacheGroupSpec.layer_names` holds real module paths +// ("model.layers.5.self_attn.attn", "backbone.layers.12.mixer"); this returns +// the integer of the `.layers..` segment. It deliberately returns nullopt +// for a PLACEHOLDER group name — "fa", "gdn", "mla", "kda", "fa_draft", +// "encoder" — because such a name names no layer at all, and that is the +// discriminator `ResolveKVCacheGroupLayerNames` and the runner's +// `GroupLayerMask` both key on. +// +// Lived in `gpu/runner.cpp`'s anonymous namespace until +// FIX-KV-GROUP-LAYER-COUNT needed the same parse in the sizing path. One +// function rather than two copies, because a second derivation of one rule is +// the thing that can disagree. +std::optional KVCacheLayerIndexOfName(std::string_view name); + +// FIX-KV-GROUP-LAYER-COUNT (#1963, #1966). Replace PLACEHOLDER group names with +// the real per-layer names the runner's own allocation classification implies, +// so that every consumer weighting a group by `layer_names.size()` weights it +// by the layers the runner will actually allocate for. +// +// WHY THIS EXISTS. `layer_names` is upstream's per-layer name list +// (`kv_cache_utils.py:1208-1210` appends every layer sharing a spec object), and +// upstream bounds its allocation with `max(len(group.layer_names) ...)` +// (`:1399`) over that list, dividing the budget by the same count it multiplies +// the allocation by (`:1005-1008`, `:1409-1416`). Thirty-three of our +// thirty-four registries publish a single placeholder string instead, so +// `KVBytesPerBlock` divided a byte budget by ONE layer's page while the runner +// allocated one buffer PER layer — 8.5 GiB for a 1 GiB budget on the 27B — and +// the #371 recurrent-state OOM guard read 0.90 GiB against a 43.40 GiB +// allocation. +// +// THE CLASSIFICATION IS THE RUNNER'S OWN, not a second derivation of the +// model's shape: +// - a layer is recurrent iff the config has a Mamba group AND `layer_types` +// is non-empty AND `layer_types[l] == "linear_attention"` — the predicate at +// `gpu/runner.cpp`'s `is_gdn` fallback; +// - the TARGET attention group is the FIRST non-eagle attention-kind group, +// which is the runner's own first-wins selection, and it covers every +// non-recurrent layer; +// - a SECOND attention group is the speculative draft layer: exactly ONE +// layer, at index `num_hidden_layers` (upstream's MTP head index, +// `qwen3_5_mtp.py:105-112`), because the runner allocates exactly one draft +// buffer and breaks; +// - a THIRD or later attention group gets an EMPTY list, because the runner +// allocates no buffer for it at all. Zero is the honest count. No registry +// emits one. +// +// A REGISTRY THAT ALREADY PUBLISHES REAL NAMES IS NEVER OVERWRITTEN. If any +// group carries a name `KVCacheLayerIndexOfName` resolves, this returns with the +// config untouched. `NemotronHForCausalLM` is that case and it knows more than +// the fallback can — its `layer_types` is empty and its MoE blocks cache +// nothing, which is exactly what #810 fixed. +// +// Idempotent: a second call finds real names and returns. +void ResolveKVCacheGroupLayerNames(KVCacheConfig& config, + int64_t num_hidden_layers, + const std::vector& layer_types); + // KV-FP8 W3 — HALF-SIZED KV BLOCKS. Rewrite every ATTENTION spec in `config` to // the resolved KV storage dtype, then hand the fp8 interpretation and the // per-tensor scales to the same specs. diff --git a/include/vllm/v1/worker/gpu/runner.h b/include/vllm/v1/worker/gpu/runner.h index ed58c9e0d..042bd5f2b 100644 --- a/include/vllm/v1/worker/gpu/runner.h +++ b/include/vllm/v1/worker/gpu/runner.h @@ -355,6 +355,21 @@ class GPUModelRunner final : public ModelRunnerBase { // value the old HF-config arithmetic could not. int64_t fa_page_size_bytes() const { return fa_page_size_bytes_; } + // FIX-KV-GROUP-LAYER-COUNT (#1963, #1966). What `initialize_kv_cache` + // ALLOCATED, summed over every buffer it created, so a gate can compare the + // sizing arithmetic against the allocation rather than against a second copy + // of the same formula. Both are 0 before `initialize_kv_cache` runs. + // + // ...paged_bytes() — the block-scaled half: one buffer per full-attention + // layer plus the speculative draft layer's. This is the half a + // `--kv-cache-memory` budget is supposed to bound, and + // `KVBytesPerBlock(cfg) * cfg.num_blocks` is supposed to equal it. + // ...allocated_bytes() — that plus the recurrent (GDN/Mamba) conv and SSM + // state, which is sized per sequence slot and not per block, and which + // `recurrent_state_bytes(cfg, max_num_reqs)` is supposed to equal. + int64_t kv_cache_allocated_paged_bytes() const; + int64_t kv_cache_allocated_bytes() const; + // #810: the per-layer KV class `initialize_kv_cache` RESOLVED, index == model // layer index, one entry per hidden layer. `kNone` is a layer that no KV // cache group named and that therefore caches nothing — NemotronH's 23 @@ -469,9 +484,15 @@ class GPUModelRunner final : public ModelRunnerBase { return backend_resident_ ? backend_data_ : host_data_.data(); } + // The byte size this buffer was constructed with — what the allocation + // COST, not what a formula predicts it cost. `kv_cache_allocated_bytes()` + // below sums these (FIX-KV-GROUP-LAYER-COUNT, #1963). + size_t bytes() const { return bytes_; } + private: vt::Device device_; bool backend_resident_ = false; + size_t bytes_ = 0; void* backend_data_ = nullptr; std::vector host_data_; }; diff --git a/src/vllm/entrypoints/model_loader.cpp b/src/vllm/entrypoints/model_loader.cpp index d6c230a63..3d2a38cbb 100644 --- a/src/vllm/entrypoints/model_loader.cpp +++ b/src/vllm/entrypoints/model_loader.cpp @@ -43,6 +43,7 @@ #include "vllm/platforms/interface.h" // CurrentPlatform() — SelectQueue #include "vllm/v1/core/hybrid_kv_budget.h" #include "vllm/v1/core/kv_cache_utils.h" // check_enough_kv_cache_memory (M4) +#include "vllm/v1/kv_cache_interface.h" // FIX-KV-GROUP-LAYER-COUNT resolver #include "vllm/v1/structured_output/backend_native.h" // MakeNativeBackendFactory #include "vllm/v1/structured_output/jump_forward.h" // JumpForwardEnabled (SW3) #include "vt/dtype.h" @@ -1439,14 +1440,43 @@ std::optional LoadedEngine::ResolveSpecConfig( vllm::v1::KVCacheConfig LoadedEngine::MakeKVCacheMaybeSpec( const LoadedModel& model, const HfConfig& config, int block_size, int num_blocks, const std::optional& spec) { + vllm::v1::KVCacheConfig kv; if (spec.has_value()) { // Speculation is Qwen3.5/3.6-only at this pin (both gate checkpoints); build // the widened spec KV directly (extra GDN k+1 state slots + widened conv row // + the `fa_draft` full-attn group). MakeQwen3_5KVCacheSpec(num_spec>0). - return vllm::MakeQwen3_5KVCacheSpec(config, block_size, num_blocks, - spec->ResolvedNumSpeculativeTokens()); + kv = vllm::MakeQwen3_5KVCacheSpec(config, block_size, num_blocks, + spec->ResolvedNumSpeculativeTokens()); + } else { + kv = ModelRegistry::MakeKVCache(model, config, block_size, num_blocks); } - return ModelRegistry::MakeKVCache(model, config, block_size, num_blocks); + // FIX-KV-GROUP-LAYER-COUNT (#1963, #1966). THE single funnel: both branches + // above return through here, so one call reaches every architecture and both + // the probe and the resized config MakeKVCacheResolved builds. + // + // Thirty-three of the thirty-four registries publish ONE placeholder name per + // KV group, and `KVBytesPerBlock` / `recurrent_state_bytes` read + // `layer_names.size()` as the layer count. Without this line a + // `--kv-cache-memory` budget is divided by ONE layer's page and then + // multiplied by every layer when the runner allocates: 1 GiB in, 8.5 GiB + // allocated on the 27B, and the #371 recurrent-state OOM guard reads 0.90 GiB + // against a 43.40 GiB allocation. Upstream cannot have that bug because the + // count that divides the budget and the count that sizes the allocation are + // the same expression over the same list (`kv_cache_utils.py:1399`, + // `:1005-1008`, `:1409-1416`). + // + // ORDERING, against `ResolveMaxNumSeqs` (#1983, which landed first): that + // resolver reads `kv_cfg_`, which is `MakeKVCacheResolved`'s result, so it + // always sees names this call has already resolved. Its seat count is + // `num_blocks`-linear, and `num_blocks` is the one input of its arithmetic + // this change moves — which is the point: upstream's `num_blocks` is + // per-layer (`kv_cache_utils.py:1008` divides by `num_layers`), and that is + // the meaning its unification against one attention page assumes. Before this + // line the byte-budget path handed it a count inflated by the layer count, so + // its clamp was too permissive. The two fixes agree; they do not fight. + vllm::v1::ResolveKVCacheGroupLayerNames(kv, config.num_hidden_layers, + config.layer_types); + return kv; } int LoadedEngine::ResolveNumBlocks(const EngineParams& params, diff --git a/src/vllm/v1/kv_cache_interface.cpp b/src/vllm/v1/kv_cache_interface.cpp index 20a8854ff..8868b3da1 100644 --- a/src/vllm/v1/kv_cache_interface.cpp +++ b/src/vllm/v1/kv_cache_interface.cpp @@ -266,6 +266,104 @@ int64_t KVBytesPerBlock(const KVCacheConfig& config) { return bytes; } +// See kv_cache_interface.h for the argument. Body moved verbatim from +// `gpu/runner.cpp`'s `LayerIndexOfName`. +std::optional KVCacheLayerIndexOfName(std::string_view name) { + constexpr std::string_view kSep = ".layers."; + const size_t at = name.find(kSep); + if (at == std::string_view::npos) return std::nullopt; + size_t i = at + kSep.size(); + const size_t start = i; + int64_t value = 0; + while (i < name.size() && name[i] >= '0' && name[i] <= '9') { + value = value * 10 + (name[i] - '0'); + if (value > (1 << 20)) return std::nullopt; // not a layer index + ++i; + } + if (i == start) return std::nullopt; // ".layers.mixer" + if (i < name.size() && name[i] != '.') return std::nullopt; // ".layers.5x" + return value; +} + +void ResolveKVCacheGroupLayerNames( + KVCacheConfig& config, int64_t num_hidden_layers, + const std::vector& layer_types) { + // Nothing to name. A caller with no layer count cannot be given one here, and + // inventing one is how the placeholder count became a layer count in the + // first place. + if (num_hidden_layers <= 0) return; + + // A registry that already publishes REAL names knows more than the fallback + // classification below: NemotronH's `layer_types` is EMPTY and its MoE blocks + // register no attention module at all, so overwriting it would re-introduce + // exactly the 52-against-6 mis-classification #810 removed. One resolvable + // name anywhere is the whole config's answer, which also makes this + // idempotent. + for (const KVCacheGroupSpec& group : config.kv_cache_groups) { + for (const std::string& name : group.layer_names) { + if (KVCacheLayerIndexOfName(name).has_value()) return; + } + } + + bool has_mamba_group = false; + for (const KVCacheGroupSpec& group : config.kv_cache_groups) { + if (group.kv_cache_spec != nullptr && + group.kv_cache_spec->kind() == KVCacheSpecKind::kMamba) { + has_mamba_group = true; + } + } + + // The runner's own `is_gdn` fallback predicate, and nothing else. A dense + // model (no Mamba group) and a hybrid whose config does not spell + // `layer_types` both classify every layer as attention here, which is what the + // runner does with them too. + std::vector recurrent; + std::vector attention; + for (int64_t l = 0; l < num_hidden_layers; ++l) { + const size_t idx = static_cast(l); + const bool is_gdn = has_mamba_group && idx < layer_types.size() && + layer_types[idx] == "linear_attention"; + if (is_gdn) { + recurrent.push_back("model.layers." + std::to_string(l) + ".linear_attn"); + } else { + attention.push_back("model.layers." + std::to_string(l) + + ".self_attn.attn"); + } + } + + bool target_named = false; + bool draft_named = false; + for (KVCacheGroupSpec& group : config.kv_cache_groups) { + if (group.kv_cache_spec == nullptr) continue; + const KVCacheSpecKind kind = group.kv_cache_spec->kind(); + if (kind == KVCacheSpecKind::kMamba) { + group.layer_names = recurrent; + continue; + } + if (dynamic_cast(group.kv_cache_spec.get()) == + nullptr) { + continue; // not an attention group and not recurrent: leave it alone. + } + if (!group.is_eagle_group && !target_named) { + group.layer_names = attention; + target_named = true; + } else if (!draft_named) { + // The speculative draft layer. Upstream registers the MTP head as one + // extra decoder layer at index num_hidden_layers + // (`qwen3_5_mtp.py:105-112`), and the runner allocates exactly one buffer + // for it before breaking out of its search. + group.layer_names = {"model.layers." + + std::to_string(num_hidden_layers) + + ".self_attn.attn"}; + draft_named = true; + } else { + // The runner allocates NO buffer for a further attention group, so the + // honest weight is zero. Unreachable for every registry shipping today. + group.layer_names.clear(); + } + } +} + namespace { // The ONE arithmetic statement W3 makes about block sizing, written where it can diff --git a/src/vllm/v1/worker/gpu/runner.cpp b/src/vllm/v1/worker/gpu/runner.cpp index cbca63f13..b8b2dfcf9 100644 --- a/src/vllm/v1/worker/gpu/runner.cpp +++ b/src/vllm/v1/worker/gpu/runner.cpp @@ -331,31 +331,25 @@ std::vector group_block_sizes(const KVCacheConfig& cfg) { // `layer_types`) has an EMPTY one. // // Our runner still indexes buffers by layer POSITION, so a published name has -// to be resolved back to an index. `LayerIndexOfName` does exactly that and -// nothing else: the integer of the `.layers..` segment of an upstream-style -// module path ("backbone.layers.5.mixer", "model.layers.12.self_attn"). +// to be resolved back to an index. `KVCacheLayerIndexOfName` does exactly that +// and nothing else: the integer of the `.layers..` segment of an +// upstream-style module path ("backbone.layers.5.mixer", +// "model.layers.12.self_attn"). // // It deliberately returns nullopt for a PLACEHOLDER group name — "fa", "gdn", -// "mla", "kda", "fa_draft", the single-name convention every other registry -// uses today — because such a name carries no layer identity at all. That is -// what keeps this additive: a group that does not publish per-layer names falls -// back to the historical `config_.layer_types` predicate, byte for byte. -std::optional LayerIndexOfName(std::string_view name) { - constexpr std::string_view kSep = ".layers."; - const size_t at = name.find(kSep); - if (at == std::string_view::npos) return std::nullopt; - size_t i = at + kSep.size(); - const size_t start = i; - int64_t value = 0; - while (i < name.size() && name[i] >= '0' && name[i] <= '9') { - value = value * 10 + (name[i] - '0'); - if (value > (1 << 20)) return std::nullopt; // not a layer index - ++i; - } - if (i == start) return std::nullopt; // ".layers.mixer" - if (i < name.size() && name[i] != '.') return std::nullopt; // ".layers.5x" - return value; -} +// "mla", "kda", "fa_draft" — because such a name carries no layer identity at +// all. That is what keeps this additive: a group that does not publish +// per-layer names falls back to the historical `config_.layer_types` predicate, +// byte for byte. +// +// FIX-KV-GROUP-LAYER-COUNT (#1963, #1966) moved the body to +// `vllm::v1::KVCacheLayerIndexOfName` (kv_cache_interface.cpp), because the +// SIZING path now has to tell a placeholder name from a real one as well, and +// two copies of one rule are what disagree. It is also the reason the +// placeholder is no longer what this function usually sees: the loader resolves +// the placeholders into real names before the config reaches the runner, so a +// registry that publishes nothing still arrives here named. +using vllm::v1::KVCacheLayerIndexOfName; // The per-layer membership mask of one KV cache group, or nullopt when the // group does not publish per-layer names. @@ -369,7 +363,7 @@ std::optional> GroupLayerMask(const KVCacheGroupSpec& group, if (group.layer_names.empty()) return std::nullopt; std::vector mask(static_cast(num_layers), false); for (const std::string& name : group.layer_names) { - const std::optional l = LayerIndexOfName(name); + const std::optional l = KVCacheLayerIndexOfName(name); if (!l.has_value() || *l < 0 || *l >= num_layers) return std::nullopt; if (mask[static_cast(*l)]) return std::nullopt; // duplicate index mask[static_cast(*l)] = true; @@ -500,7 +494,7 @@ GPUModelRunner::GPUModelRunner(const HfConfig& config, GPUModelRunner::CacheBuffer::CacheBuffer(vt::Device device, vt::Queue& queue, size_t bytes, bool backend_resident) - : device_(device), backend_resident_(backend_resident) { + : device_(device), backend_resident_(backend_resident), bytes_(bytes) { if (!backend_resident_) { host_data_.assign(bytes, uint8_t{0}); return; @@ -524,6 +518,22 @@ GPUModelRunner::CacheBuffer::~CacheBuffer() { } } +// FIX-KV-GROUP-LAYER-COUNT (#1963, #1966): the measurement half of the fix. See +// runner.h for what each of the two sums is meant to equal. +int64_t GPUModelRunner::kv_cache_allocated_paged_bytes() const { + int64_t total = 0; + for (const auto& b : full_attn_buf_) total += static_cast(b->bytes()); + for (const auto& b : draft_attn_buf_) total += static_cast(b->bytes()); + return total; +} + +int64_t GPUModelRunner::kv_cache_allocated_bytes() const { + int64_t total = kv_cache_allocated_paged_bytes(); + for (const auto& b : ssm_buf_) total += static_cast(b->bytes()); + for (const auto& b : conv_buf_) total += static_cast(b->bytes()); + return total; +} + void GPUModelRunner::initialize_kv_cache(const KVCacheConfig& kv_cache_config) { num_blocks_ = kv_cache_config.num_blocks; // ENGINE-LEVEL ATTENTION-BACKEND SELECTION (M3, issue #41) happens INSIDE the @@ -820,18 +830,26 @@ void GPUModelRunner::initialize_kv_cache(const KVCacheConfig& kv_cache_config) { // attention pages actually needed (52 against 6 real GQA layers). // // BYTE-NEUTRALITY CONTRACT, mirroring the one `per_layer_attn_specs` states - // at `include/vllm/v1/kv_cache_interface.h:354-374`: the by-name path is - // entered ONLY when the recurrent group — and the target attention group, if - // there is one — publish per-layer names that all resolve to distinct - // in-range layer indices. Every registry shipping today publishes a single - // PLACEHOLDER name per group ("fa"/"gdn", "mla"/"kda", "fa_draft"), which - // resolves to nothing, so every existing model takes the `layer_types` - // fallback below and gets byte-identical allocation, view, indexing and - // kernel dispatch to before this field was read. This is a capability probe - // on the record the model published, NOT a per-architecture switch: any - // future hybrid that publishes real names is routed correctly with no new - // branch, which is the whole point (`hf_config.cpp:484-528` synthesizing - // Qwen3.5's dialect for Kimi-Linear is the anti-pattern this replaces). + // at `include/vllm/v1/kv_cache_interface.h`: the by-name path is entered ONLY + // when the recurrent group — and the target attention group, if there is one — + // publish per-layer names that all resolve to distinct in-range layer indices. + // This is a capability probe on the record the model published, NOT a + // per-architecture switch: a hybrid that publishes real names is routed + // correctly with no new branch, which is the whole point + // (`hf_config.cpp:484-528` synthesizing Qwen3.5's dialect for Kimi-Linear is + // the anti-pattern this replaces). + // + // FIX-KV-GROUP-LAYER-COUNT (#1963, #1966): the registries still publish a + // single PLACEHOLDER name per group ("fa"/"gdn", "mla"/"kda", "fa_draft"), + // but `ResolveKVCacheGroupLayerNames` now rewrites those into real per-layer + // names in the LOADER, before the config reaches this function, using exactly + // the `layer_types` predicate the fallback below uses. So a hybrid arrives + // here named and takes the by-name path, and the classification it gets is + // the one the fallback would have produced — which is what makes that + // rewrite byte-neutral for the allocation and, at the same time, what makes + // `KVBytesPerBlock` and `recurrent_state_bytes` count the layers this loop + // actually allocates for. Before that fix they counted ONE, and a 1 GiB + // budget bought 8.5 GiB of buffers. const int64_t num_layers = config_.num_hidden_layers; std::optional> gdn_layer_mask; std::optional> attn_layer_mask; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 6a616d781..358cdad35 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1675,6 +1675,11 @@ vllm_cpp_add_test(test_output_processor vllm/v1/test_output_processor.cpp) vllm_cpp_add_test(test_async_llm vllm/v1/test_async_llm.cpp) vllm_cpp_add_test(test_llm_engine vllm/v1/test_llm_engine.cpp) vllm_cpp_add_test(test_loaded_engine_dense vllm/entrypoints/test_loaded_engine_dense.cpp) +# FIX-KV-GROUP-LAYER-COUNT (#1963, #1966): the KV-budget cases drive +# MakeQwen3_5KVCacheSpec, the shipping registry helper, whose header is private +# to src/. A hand-built KVCacheGroupSpec is a shape no registry emits and cannot +# see the defect, which is exactly why the pre-existing unit coverage missed it. +target_include_directories(test_loaded_engine_dense PRIVATE ${CMAKE_SOURCE_DIR}/src) # KV-FP8 W3 (#1593): half-sized KV blocks, --kv-cache-dtype threading and the # checkpoint k/v-scale path, entered through the LoadedEngine loader. vllm_cpp_add_test(test_kv_cache_fp8_wiring vllm/entrypoints/test_kv_cache_fp8_wiring.cpp) diff --git a/tests/vllm/entrypoints/test_loaded_engine_dense.cpp b/tests/vllm/entrypoints/test_loaded_engine_dense.cpp index 8edeaa922..3ebdfa1eb 100644 --- a/tests/vllm/entrypoints/test_loaded_engine_dense.cpp +++ b/tests/vllm/entrypoints/test_loaded_engine_dense.cpp @@ -39,6 +39,9 @@ #include #include "vllm/model_executor/models/qwen3_5_dense.h" +#include "vllm/model_executor/models/qwen3_5_common.h" // MakeQwen3_5KVCacheSpec +#include "vllm/v1/core/kv_cache_utils.h" // recurrent_state_bytes (#1966) +#include "vllm/v1/kv_cache_interface.h" // KVBytesPerBlock (#1963) #include "vllm/sampling_params.h" #include "vllm/tokenizer/bpe.h" #include "vllm/tokenizer/tokenizer.h" @@ -280,6 +283,25 @@ std::string CerrOfEngineLoad(const HfConfig& c, const EngineParams& params) { // sentence fails on the fact it dropped rather than on all of them at once. constexpr const char* kInertNotice = "--gpu-memory-utilization"; +// ── FIX-KV-GROUP-LAYER-COUNT (#1963, #1966) ───────────────────────────────── +// +// MakeDenseConfig above has [LA, LA, LA, FA] — exactly ONE full-attention +// layer and therefore exactly the config in which this bug is INVISIBLE, since +// the placeholder count (1) and the real count (1) agree. This one has TWO +// full-attention layers and FOUR GDN layers, so the placeholder count is wrong +// on both halves and by different factors. +HfConfig MakeHybridTwoAttnConfig() { + HfConfig c = MakeDenseConfig(); + c.num_hidden_layers = 6; + c.layer_types = {"linear_attention", "linear_attention", "full_attention", + "linear_attention", "linear_attention", "full_attention"}; + return c; +} + +// Small enough that the resolved block count stays modest, large enough that +// the pre-fix over-allocation is unambiguous rather than a rounding artefact. +constexpr int64_t kKvBudgetBytes = 1 << 20; // 1 MiB + } // namespace // ─── 1. Arch-select: the FromModelDir dispatch decision ────────────────────── @@ -935,3 +957,156 @@ TEST_CASE( REQUIRE(out.outputs.size() == 1); CHECK(static_cast(out.outputs[0].token_ids.size()) == 4); } + +// ─── FIX-KV-GROUP-LAYER-COUNT: the budget must BOUND the allocation ────────── +// +// #1963. `ResolveNumBlocks` arm 2 divides an ABSOLUTE byte budget by +// `KVBytesPerBlock(probe)`, and `GPUModelRunner::initialize_kv_cache` then +// allocates `num_blocks * page_size_bytes()` PER full-attention layer. With the +// registry publishing one placeholder name per group, the divisor counted one +// layer and the allocation used every layer, so a 1 GiB budget bought 8.5 GiB +// of buffers on the 27B. +// +// These cases enter through the LOADER — the `LoadedEngine` constructor — not +// through the resolver, and they read what the ALLOCATOR did +// (`kv_cache_allocated_*_bytes()` sums the byte size every `CacheBuffer` was +// constructed with) rather than re-deriving the same formula a second time. The +// chain under test is +// LoadedEngine ctor -> MakeKVCacheResolved -> MakeKVCacheMaybeSpec -> +// ResolveKVCacheGroupLayerNames -> ResolveNumBlocks -> +// GPUModelRunner::initialize_kv_cache +// so deleting the `ResolveKVCacheGroupLayerNames` call site reds them. +// +// RED at `d9a528528`: 2 full-attention layers against a 1-name divisor, so the +// allocation is 2 MiB for a 1 MiB budget. + +TEST_CASE( + "kv-group-layer-count: --kv-cache-memory bounds the bytes the runner " + "allocates") { + const HfConfig c = MakeHybridTwoAttnConfig(); + EngineParams params; + params.kv_cache_memory_bytes = kKvBudgetBytes; + + LoadedEngine eng(c, MakeDenseWeights(c), FreshFixture(), params); + + const int64_t allocated = eng.runner().kv_cache_allocated_paged_bytes(); + // The allocation happened at all — a zero here would make the bound + // vacuously true, which is the shape a "green that measured nothing" takes. + REQUIRE(allocated > 0); + REQUIRE(eng.runner().attn_kv().size() == 2); // both full-attention layers + CHECK(allocated <= params.kv_cache_memory_bytes); +} + +TEST_CASE( + "kv-group-layer-count: KVBytesPerBlock equals the per-block cost the " + "allocator actually paid") { + const HfConfig c = MakeHybridTwoAttnConfig(); + EngineParams params; + params.kv_cache_memory_bytes = kKvBudgetBytes; + + LoadedEngine eng(c, MakeDenseWeights(c), FreshFixture(), params); + + const vllm::v1::KVCacheConfig& kv = eng.kv_cache_config(); + REQUIRE(kv.num_blocks > 0); + // The identity upstream gets by construction (`kv_cache_utils.py:1399` and + // `:1409-1416` read one list) and we have to assert: the number that DIVIDED + // the budget, times the block count it produced, is the number of bytes the + // allocator handed out. + CHECK(vllm::v1::KVBytesPerBlock(kv) * kv.num_blocks == + eng.runner().kv_cache_allocated_paged_bytes()); +} + +TEST_CASE( + "kv-group-layer-count: the loader resolves the registry's PLACEHOLDER " + "group names into per-layer names") { + const HfConfig c = MakeHybridTwoAttnConfig(); + EngineParams params; + params.kv_cache_memory_bytes = kKvBudgetBytes; + + // What the registry publishes on its own: one placeholder string per group, + // which is the defect's whole cause. Asserted here so the case says WHAT it + // is comparing rather than only that the answer is 2. + const vllm::v1::KVCacheConfig raw = vllm::MakeQwen3_5KVCacheSpec( + c, params.block_size, /*num_blocks=*/8, /*num_spec=*/0); + REQUIRE(raw.kv_cache_groups.size() == 2); + CHECK(raw.kv_cache_groups[0].layer_names.size() == 1); // "fa" + CHECK(raw.kv_cache_groups[1].layer_names.size() == 1); // "gdn" + + LoadedEngine eng(c, MakeDenseWeights(c), FreshFixture(), params); + const vllm::v1::KVCacheConfig& kv = eng.kv_cache_config(); + REQUIRE(kv.kv_cache_groups.size() == 2); + CHECK(kv.kv_cache_groups[0].layer_names.size() == 2); // layers 2 and 5 + CHECK(kv.kv_cache_groups[1].layer_names.size() == 4); // layers 0,1,3,4 +} + +// ─── FIX-KV-GROUP-LAYER-COUNT: the #371 recurrent-state guard (#1966) ──────── +// +// `recurrent_state_bytes` is what `check_enough_state_memory` refuses on, and +// it read `layer_names.size()` too — 0.90 GiB against a 43.40 GiB allocation on +// the 27B at `--max-num-seqs 32`, k=8. The guard exists to turn an OOM reboot +// into a refusal, so a guard that is 48x under is a guard that does not fire. +// +// The comparison is against what the runner ALLOCATED for the recurrent half, +// not against a second copy of the formula. +TEST_CASE( + "kv-group-layer-count: recurrent_state_bytes equals the recurrent bytes " + "the runner allocates") { + const HfConfig c = MakeHybridTwoAttnConfig(); + EngineParams params; + params.kv_cache_memory_bytes = kKvBudgetBytes; + params.max_num_seqs = 4; + + LoadedEngine eng(c, MakeDenseWeights(c), FreshFixture(), params); + + const int64_t recurrent_allocated = + eng.runner().kv_cache_allocated_bytes() - + eng.runner().kv_cache_allocated_paged_bytes(); + REQUIRE(recurrent_allocated > 0); + REQUIRE(eng.runner().gdn_state().size() == 4); // all four GDN layers + // `eng.max_num_seqs()`, NOT `params.max_num_seqs` (#1983). The constructor + // hands the runner the RESOLVED concurrency, which `ResolveMaxNumSeqs` may + // clamp below the configured one to fit the recurrent-state budget the KV + // pool affords. For this config the budget holds 256 seats against the 4 + // asked, so the two are equal today — and that is exactly why the configured + // one must not be what is asserted: it would be right by a 64x margin rather + // than by construction, and a later change to the geometry would make this + // case start measuring the wrong quantity while staying green. + CHECK(vllm::v1::recurrent_state_bytes(eng.kv_cache_config(), + eng.max_num_seqs()) == + recurrent_allocated); +} + +// ─── FIX-KV-GROUP-LAYER-COUNT: the speculative draft layer counts ONE ──────── +// +// With `num_spec > 0` the registry appends a third group, `fa_draft`, and the +// runner allocates exactly ONE buffer for it before breaking out of its search. +// One is therefore the RIGHT weight for that group and the wrong weight for the +// other two, which is why the divisor cannot be a per-group constant. +// +// Driven from `MakeQwen3_5KVCacheSpec` — the shipping registry helper — rather +// than from a hand-built `KVCacheGroupSpec`, because a fixture that spells its +// own layer names is a shape no registry emits and cannot see this defect. +TEST_CASE( + "kv-group-layer-count: the fa_draft group weighs exactly one layer") { + const HfConfig c = MakeHybridTwoAttnConfig(); + vllm::v1::KVCacheConfig kv = vllm::MakeQwen3_5KVCacheSpec( + c, /*block_size=*/32, /*num_blocks=*/8, /*num_spec=*/4); + REQUIRE(kv.kv_cache_groups.size() == 3); // fa, gdn, fa_draft + + vllm::v1::ResolveKVCacheGroupLayerNames(kv, c.num_hidden_layers, + c.layer_types); + CHECK(kv.kv_cache_groups[0].layer_names.size() == 2); // target attention + CHECK(kv.kv_cache_groups[1].layer_names.size() == 4); // recurrent + REQUIRE(kv.kv_cache_groups[2].layer_names.size() == 1); // the draft layer + // Upstream registers the MTP head as one extra decoder layer at index + // num_hidden_layers (qwen3_5_mtp.py:105-112), and the name says so. + CHECK(kv.kv_cache_groups[2].layer_names[0] == + "model.layers." + std::to_string(c.num_hidden_layers) + + ".self_attn.attn"); + + const auto* fa = dynamic_cast( + kv.kv_cache_groups[0].kv_cache_spec.get()); + REQUIRE(fa != nullptr); + // 2 target layers + 1 draft layer, not 1 + 1 and not 2 + 2. + CHECK(vllm::v1::KVBytesPerBlock(kv) == fa->page_size_bytes() * 3); +} diff --git a/tests/vllm/models/test_nemotron_h_scaffold.cpp b/tests/vllm/models/test_nemotron_h_scaffold.cpp index f4e603481..9ef4ec78a 100644 --- a/tests/vllm/models/test_nemotron_h_scaffold.cpp +++ b/tests/vllm/models/test_nemotron_h_scaffold.cpp @@ -666,6 +666,21 @@ TEST_CASE("NemotronH KV: the het groups mirror mamba2_state_shape") { const int64_t ssm_bytes = 64 * 64 * 128 * 4; // f32 CHECK(mamba.page_size_bytes() == conv_bytes + ssm_bytes); CHECK(vllm::v1::KVBytesPerBlock(kv) == attn->page_size_bytes() * 6); + + // FIX-KV-GROUP-LAYER-COUNT (#1963, #1966). The loader now rewrites + // PLACEHOLDER group names into per-layer names, because thirty-three + // registries publish one string per group and the byte accounting reads that + // string count as a layer count. NemotronH is the one registry that does not, + // and it MUST be left alone: its `layer_types` is empty and its 23 MoE blocks + // register no attention module, so the fallback classification the resolver + // uses would call all 52 layers full-attention — which is exactly the + // 52-against-6 mis-classification #810 removed. + vllm::v1::KVCacheConfig resolved = kv; + vllm::v1::ResolveKVCacheGroupLayerNames(resolved, config.num_hidden_layers, + config.layer_types); + CHECK(resolved.kv_cache_groups[0].layer_names == expected_attn); + CHECK(resolved.kv_cache_groups[1].layer_names == expected_mamba); + CHECK(vllm::v1::KVBytesPerBlock(resolved) == attn->page_size_bytes() * 6); } TEST_CASE("NemotronH: the unported arms REFUSE BY NAME") {