From 94b7fa2c6261244294e46498124580013706c8fc Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Wed, 26 Aug 2026 15:33:48 +0000 Subject: [PATCH 1/4] spec(FIX-KV-GROUP-LAYER-COUNT): the KV byte accounting counts placeholder names, so a 1 GiB budget allocates 8.5 GiB (#1963, #1966) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `KVCacheGroupSpec::layer_names` is upstream's per-layer name list, and upstream bounds its KV allocation with `max(len(group.layer_names) ...)` over that same list, so the count that divides the budget and the count that sizes the allocation are one expression and the product is bounded by construction (`kv_cache_utils.py:1399`, `:1005-1008`, `:1409-1416`). Thirty-three of our thirty-four registries publish a single PLACEHOLDER string per group instead — `{"fa"}`, `{"gdn"}`, `{"mla"}`, `{"kda"}`, `{"fa_draft"}`, `{"encoder"}` — and three consumers read `layer_names.size()` as if it were a layer count. Two of them are wrong: `KVBytesPerBlock` divides an absolute `--kv-cache-memory` budget by ONE layer's page while the runner allocates one buffer per layer (#1963, measured 8.5 GiB for a 1 GiB budget on the 27B), and `recurrent_state_bytes` reports 0.90 GiB against a 43.40 GiB allocation, so the #371 OOM guard does not fire on the only family it can fire for (#1966). Together they account arithmetically for the 98.2 GiB the watchdog saw. The spec records the narrowing that matters and holds: of the two product call sites of `KVBytesPerBlock`, only `ResolveNumBlocks` arm 2 is defective. In `ResolveMaxModelLen` the factor stands on both sides of every comparison and cancels. The design is one resolver, called from the single loader funnel every architecture returns through, which rewrites the placeholders using the runner's OWN allocation predicate rather than a second derivation of the model's shape — and which stands aside for `NemotronHForCausalLM`, the one registry that already publishes real names and knows more than the fallback can. Implementation follows in this pull request; this commit is the spec, committed first so the order proves it. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code] --- .agents/issue-index.md | 2 + .agents/specs/kv-group-layer-count.md | 236 ++++++++++++++++++++++++++ 2 files changed, 238 insertions(+) create mode 100644 .agents/specs/kv-group-layer-count.md diff --git a/.agents/issue-index.md b/.agents/issue-index.md index ff108e7fd..f96cd94e9 100644 --- a/.agents/issue-index.md +++ b/.agents/issue-index.md @@ -745,3 +745,5 @@ 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 | diff --git a/.agents/specs/kv-group-layer-count.md b/.agents/specs/kv-group-layer-count.md new file mode 100644 index 000000000..d38eb7c75 --- /dev/null +++ b/.agents/specs/kv-group-layer-count.md @@ -0,0 +1,236 @@ +# 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`. + +## Now + +`ACTIVE`. One pull request carries the spec and the implementation, in that +commit order. + +## 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 the #1963 collapse arithmetically: at +`--kv-cache-memory 6GiB`, `k=8`, `--max-num-seqs 32` the base tree allocates +`17 * (6GiB/262144) * 131072` = 54.8 GiB of paged pool plus 43.4 GiB of +recurrent state = **98.2 GiB**, against the ~108 GB the watchdog saw. + +## 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 + +`tests/vllm/v1/test_kv_group_layer_count.cpp`, new. Every case is driven from +`MakeQwen3_5KVCacheSpec` — the shipping registry helper — never from a +hand-built `KVCacheGroupSpec`. The 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, chosen so the +placeholder count (1) and the real count (2, 4) differ. The 4-layer config the +existing engine tests use has exactly ONE full-attention layer, where the bug is +invisible. + +1. **`KVBytesPerBlock` counts the layers the runner allocates for.** Real + registry config -> real `GPUModelRunner` on CPU -> `KVBytesPerBlock(kv) * + kv.num_blocks == runner.kv_cache_allocated_paged_bytes()`. RED at the base: + 1 page against 2. +2. **`recurrent_state_bytes` counts the layers the runner allocates for.** + Same runner -> `recurrent_state_bytes(kv, max_num_reqs) == + runner.kv_cache_allocated_bytes() - runner.kv_cache_allocated_paged_bytes()`. + RED at the base: 1 layer against 4. +3. **The budget bounds the allocation, through the production entry point.** + `LoadedEngine` built in memory with `EngineParams::kv_cache_memory_bytes = B` + -> `engine.runner().kv_cache_allocated_paged_bytes() <= B`. RED at the base: + 2x over. This is the reachability case: it enters through the + `LoadedEngine` constructor, so deleting the `ResolveKVCacheGroupLayerNames` + call site in `MakeKVCacheMaybeSpec` must red it. +4. **A registry that publishes real names is not overwritten.** NemotronH's own + `MakeKVCache` through the resolver keeps its 6 attention names and 23 Mamba + names. +5. **Spec-on shape.** `MakeQwen3_5KVCacheSpec(num_spec=k)` -> the `fa_draft` + group carries exactly ONE name, so the divisor is `(n_fa + 1) * page`. + +## Gates + +```sh +cmake --build build -j 4 +./build/tests/vllm_tests -tc='kv-group-layer-count*' +ctest --test-dir build --output-on-failure +scripts/agent-preflight.sh --staged +``` + +## 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. From 1d9fa727faadcbe998b9f1317f43fc3672259e9c Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Wed, 26 Aug 2026 16:16:40 +0000 Subject: [PATCH 2/4] fix(FIX-KV-GROUP-LAYER-COUNT): resolve the placeholder KV group names, so the budget bounds the allocation again (#1963, #1966) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ResolveKVCacheGroupLayerNames` rewrites a registry's PLACEHOLDER group names into the real per-layer names the runner's own allocation classification implies, and `LoadedEngine::MakeKVCacheMaybeSpec` calls it. That is the single funnel every architecture returns through — the speculative branch and `ModelRegistry::MakeKVCache` both — and `MakeKVCacheResolved` passes both the probe and the resized config through it, so one call site reaches all thirty-four registries. The classification is the runner's own predicate, not a second derivation of the model's shape: a layer is recurrent iff the config has a Mamba group and `layer_types[l] == "linear_attention"`; the target attention group is the first non-eagle attention group and covers every other layer; a second attention group is the speculative draft head, one layer at index `num_hidden_layers`; a third gets an empty list, because the runner allocates no buffer for it. Reproducing the allocator's predicate is what makes the accounting unable to disagree with the allocation, which is the property upstream gets for free by reading one list twice (`kv_cache_utils.py:1399`, `:1409-1416`). `NemotronHForCausalLM` already publishes real names and is left alone: one resolvable name anywhere in the config and the resolver returns untouched. Its `layer_types` is empty and its MoE blocks cache nothing, so the fallback would re-introduce the 52-against-6 mis-classification #810 removed. `LayerIndexOfName` moves out of the runner's anonymous namespace to `vllm::v1::KVCacheLayerIndexOfName` so both sides parse a name with one function. `GPUModelRunner::kv_cache_allocated_bytes()` and its paged half sum the size every `CacheBuffer` was constructed with. They exist so the gate compares the sizing arithmetic against what the allocator DID rather than against a second copy of the same formula. Six cases, all driven from `MakeQwen3_5KVCacheSpec` through the `LoadedEngine` constructor rather than from a hand-built `KVCacheGroupSpec`, on a 6-layer hybrid with TWO full-attention and FOUR GDN layers — the 4-layer config the file already had has exactly one full-attention layer, which is the config in which this bug is invisible. With the resolver call site deleted: CHECK( allocated <= kv_cache_memory_bytes ) 2097152 <= 1048576 CHECK( KVBytesPerBlock * num_blocks == ... ) 1048576 == 2097152 CHECK( groups[0].layer_names.size() == 2 ) 1 == 2 CHECK( groups[1].layer_names.size() == 4 ) 1 == 4 CHECK( recurrent_state_bytes(cfg,4) == ... ) 4992 == 19968 Exactly 2x on the paged half and 4x on the recurrent half, which is the defect in bytes. Restored: 21 of 21 assertions pass, and 627 of 627 ctest cases. `--kv-cache-memory` now buys as many times fewer blocks as the model has attention layers, because those blocks were never inside the budget. `docs/USAGE.md` says so beside the flag, and its `8589934592` example becomes honest rather than a footgun; `docs/FEATURES.md` corrects the "group-aware divisor" claim that named the wrong instrument. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code] --- .agents/specs/kv-group-layer-count.md | 113 ++++++++---- docs/FEATURES.md | 2 +- docs/USAGE.md | 10 ++ include/vllm/v1/kv_cache_interface.h | 61 +++++++ include/vllm/v1/worker/gpu/runner.h | 21 +++ src/vllm/entrypoints/model_loader.cpp | 26 ++- src/vllm/v1/kv_cache_interface.cpp | 98 ++++++++++ src/vllm/v1/worker/gpu/runner.cpp | 92 ++++++---- tests/CMakeLists.txt | 5 + .../entrypoints/test_loaded_engine_dense.cpp | 167 ++++++++++++++++++ .../vllm/models/test_nemotron_h_scaffold.cpp | 15 ++ 11 files changed, 533 insertions(+), 77 deletions(-) diff --git a/.agents/specs/kv-group-layer-count.md b/.agents/specs/kv-group-layer-count.md index d38eb7c75..69474dbef 100644 --- a/.agents/specs/kv-group-layer-count.md +++ b/.agents/specs/kv-group-layer-count.md @@ -6,7 +6,7 @@ Work row `FIX-KV-GROUP-LAYER-COUNT`, owned by `ROAD-V1-MEM` 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`. +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 @@ -157,50 +157,91 @@ transcription of the same formula. ## Tests -`tests/vllm/v1/test_kv_group_layer_count.cpp`, new. Every case is driven from -`MakeQwen3_5KVCacheSpec` — the shipping registry helper — never from a -hand-built `KVCacheGroupSpec`. The 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, chosen so the -placeholder count (1) and the real count (2, 4) differ. The 4-layer config the -existing engine tests use has exactly ONE full-attention layer, where the bug is -invisible. - -1. **`KVBytesPerBlock` counts the layers the runner allocates for.** Real - registry config -> real `GPUModelRunner` on CPU -> `KVBytesPerBlock(kv) * - kv.num_blocks == runner.kv_cache_allocated_paged_bytes()`. RED at the base: - 1 page against 2. -2. **`recurrent_state_bytes` counts the layers the runner allocates for.** - Same runner -> `recurrent_state_bytes(kv, max_num_reqs) == - runner.kv_cache_allocated_bytes() - runner.kv_cache_allocated_paged_bytes()`. - RED at the base: 1 layer against 4. -3. **The budget bounds the allocation, through the production entry point.** - `LoadedEngine` built in memory with `EngineParams::kv_cache_memory_bytes = B` - -> `engine.runner().kv_cache_allocated_paged_bytes() <= B`. RED at the base: - 2x over. This is the reachability case: it enters through the - `LoadedEngine` constructor, so deleting the `ResolveKVCacheGroupLayerNames` - call site in `MakeKVCacheMaybeSpec` must red it. -4. **A registry that publishes real names is not overwritten.** NemotronH's own - `MakeKVCache` through the resolver keeps its 6 attention names and 23 Mamba - names. -5. **Spec-on shape.** `MakeQwen3_5KVCacheSpec(num_spec=k)` -> the `fa_draft` - group carries exactly ONE name, so the divisor is `(n_fa + 1) * page`. +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/vllm_tests -tc='kv-group-layer-count*' +./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 ``` +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 diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 4e0e26aa4..c85fcbd29 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -385,7 +385,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 fd419b670..9e1155adb 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -189,6 +189,16 @@ 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. + **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 85c3580f6..c03eb877f 100644 --- a/src/vllm/entrypoints/model_loader.cpp +++ b/src/vllm/entrypoints/model_loader.cpp @@ -41,6 +41,7 @@ #include "vllm/transformers_utils/hf_config.h" // SPEC-DFLASH D5 draft config #include "vllm/platforms/interface.h" // CurrentPlatform() — SelectQueue #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" @@ -1394,14 +1395,33 @@ 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`). + 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 85e7624ce..134771db2 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1663,6 +1663,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 c7925dec5..e464be151 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 ────────────────────── @@ -764,3 +786,148 @@ TEST_CASE( CHECK(CerrOfEngineLoad(c, params).find(kInertNotice) == std::string::npos); } + +// ─── 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; // the same value the ctor hands the runner + + 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 + CHECK(vllm::v1::recurrent_state_bytes(eng.kv_cache_config(), + params.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") { From 8234fdbce98cabafae87c4ec595d60217d7f44c8 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Wed, 26 Aug 2026 17:17:39 +0000 Subject: [PATCH 3/4] record(FIX-KV-GROUP-LAYER-COUNT): correct the collapse arithmetic and state the device prediction before the run (#1963, #1966) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three record repairs to the row's own spec, riding in the pull request whose change made them stale. The collapse figure mixed GiB and GB. At `--kv-cache-memory 6GiB`, k=8, `--max-num-seqs 32` the base tree allocates 54,760,833,024 B of paged pool, which is 51.00 GiB and not 54.8; with 43.40 GiB of recurrent state that is 94.40 GiB of the 100.6 GiB the watchdog measured, not 98.2. The residue is weights and transients, which this row does not claim. `## Now` now names the pull request and carries the device confirmation as a table of PREDICTED values beside the measured base ones, written before any run so the prediction cannot be fitted afterwards. It also says what the predictions assume — 16 target full-attention layers, which is what the base measurement implies rather than something read off the checkpoint — and what a mismatch would mean. `## Gates` records both full-suite results: 627/627 on the pre-merge head, and 625/627 after merging `origin/main` on a box under heavy contention, where `test_dflash2_ctx_capacity` and `test_async_llm` are red under `ctest -j 4` and green when re-run serially on the same binary. That is the starvation case verification.md names. Neither test reads `layer_names`, `KVBytesPerBlock` or `recurrent_state_bytes`. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code] --- .agents/specs/kv-group-layer-count.md | 51 ++++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 5 deletions(-) diff --git a/.agents/specs/kv-group-layer-count.md b/.agents/specs/kv-group-layer-count.md index 69474dbef..f105d9c2a 100644 --- a/.agents/specs/kv-group-layer-count.md +++ b/.agents/specs/kv-group-layer-count.md @@ -10,8 +10,38 @@ Base: `d9a528528`. **Every `file:line` below is read at that base**, including t ## Now -`ACTIVE`. One pull request carries the spec and the implementation, in that -commit order. +`ACTIVE` in [#2000](https://github.com/mudler/vllm.cpp/pull/2000). One pull +request carries the spec and the implementation, in that commit order. + +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 43.40 GiB, guard reports 43.40 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`). + +`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 @@ -75,10 +105,12 @@ 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 the #1963 collapse arithmetically: at +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.8 GiB of paged pool plus 43.4 GiB of -recurrent state = **98.2 GiB**, against the ~108 GB the watchdog saw. +`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 @@ -235,6 +267,15 @@ 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`. + 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 From 4f8638a7ce34d4ad1ea2ff8c40b93d55e472a3ee Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Wed, 26 Aug 2026 21:01:13 +0000 Subject: [PATCH 4/4] record(FIX-KV-GROUP-LAYER-COUNT): the post-merge gate, target by target (#1963, #1966) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The row's spec carried only the pre-merge results. This records the head that merges KV-GDN-STATE-BUDGET (#1999): `BUILD rc=0` and 628 of 628 ctest cases, `CTEST rc=0`, with no flake to re-run and a disk guard that never fired — the two starvation reds the pre-merge run hit did not recur. The interaction surface is also gated target by target with the exit code captured per binary rather than inferred from a summary line, and `test_hybrid_kv_budget` is the one that matters: #1983's own gate, 8 of 8, green against a tree where `num_blocks` means something different from what it meant when that gate was written. That is the executable form of the claim that the two rows compose rather than fight, and it is better evidence than the arithmetic in `## Now`, which only predicts it. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code] --- .agents/specs/kv-group-layer-count.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/.agents/specs/kv-group-layer-count.md b/.agents/specs/kv-group-layer-count.md index 7c59af60b..894f5a9d2 100644 --- a/.agents/specs/kv-group-layer-count.md +++ b/.agents/specs/kv-group-layer-count.md @@ -348,6 +348,26 @@ 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