From 30f704f6c3f47066678a04cd877454f5ad23271e Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Tue, 18 Aug 2026 03:55:57 +0000 Subject: [PATCH 1/4] spec(FIX-READ-F32-SCALAR-GUARD): a per-tensor scale reader that checks neither the element count nor the dtype (#1181) `ReadF32Scalar` bounds its input with `t.nbytes >= sizeof(float)`, a LOWER bound, then copies four bytes into a `float`. An array passes and is read as element 0. Any dtype passes and is reinterpreted. Both return a finite, plausible float, so the model produces fluent wrong tokens rather than failing, which is the one failure mode a token gate cannot see. The audit corrects the issue's own framing twice. The 27 grep hits are 5 definitions, 20 call sites and 2 comment references, and both counts are short: `ReadCtF32Scalar` in `dense_weight_loaders.h` is a sixth copy under another name, reached from a sixth model file through `LoadCtNvfp4W4A16`. And the defect is not only latent. `unsloth/Qwen3.6-27B-NVFP4` @`ccdaab7e` ships FP8 W8A8 throughout with BF16 per-output-channel scales, both facts already recorded in this tree, and `LoadAttnDense` branches on the weight dtype alone, so those projections reach both defects at once under the tensor name the loader actually asked for. No call site legitimately passes a multi-element or non-F32 tensor, and every existing fixture emits rank-0 or `{1}` `F32`, so nothing in the tree needs the leniency that is being removed. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code] --- .agents/issue-index.md | 1 + .agents/specs/read-f32-scalar-guard.md | 278 +++++++++++++++++++++++++ 2 files changed, 279 insertions(+) create mode 100644 .agents/specs/read-f32-scalar-guard.md diff --git a/.agents/issue-index.md b/.agents/issue-index.md index 5de4ddcbc..988016e6a 100644 --- a/.agents/issue-index.md +++ b/.agents/issue-index.md @@ -359,3 +359,4 @@ rather than merged. `scripts/check-agent-record.py` gates both. | [#1170](https://github.com/mudler/vllm.cpp/issues/1170) | — | All four GDN Triton AOT fast paths reject any geometry whose linear V-head count is not 48 or 32 — `TryTritonPackedDecode` (`src/vt/cuda/cuda_gdn.cu:5207` @ `dd8a3b0e1`), `TryTritonDeltaH` (`:5264`), `TryTritonChunkO` (`:5298`) and `TryTritonWU` (`:5361`), each reading `if (hv_n != 48 && hv_n != 32) return false;` on top of `dk == 128 && dv == 128 && hk_n == 16`. Those two are the only vendored specializations (`src/vt/cuda/triton_aot_vendored/*/gdn_{decode,deltah,chunko,wu}_h{48,32}.*`): 48 is the dense 27B and 32 is `Qwen3.6-35B-A3B`. `Qwen/Qwen3.8-2.4T-A95B` has 128 linear V-heads ([`qwen38-text-only.md`](specs/qwen38-text-only.md)) and clears every other term, so it is rejected on `hv_n` alone and runs the hand CUDA kernels on all four legs — the ones `.agents/kernel-matrix.md` measured by cuobjdump at REG:255 + STACK:48 (spilling) against the vLLM FLA cubin's REG:205 / 0 spill, which is the whole reason the vendored cubins exist and are default-on. `Qwen3.8-27B` is NOT affected: it is the `Qwen3.6-27B` geometry retrained, 48 V-heads, and hits every AOT arm. Neither reference restricts the head count — SGLang's `TritonGDNKernel` sets `supports_packed_decode` from the platform alone and takes `num_v_heads` as a runtime argument (`python/sglang/srt/layers/attention/linear/kernels/gdn_triton.py:43` @ `f63458b5be`), and `VLLM_ENABLE_FLA_PACKED_RECURRENT_DECODE` has no shape term (`vllm/envs.py:124` @ `5559679`); both JIT-compile per shape, which is the property the AOT vendoring trades away for a Python-free runtime. Closing it needs `h128` specializations vendored across the supported architectures, or a stated rule for which head counts get an AOT arm plus a visible fallback cost at the call site. Filed, not fixed: either close needs the checkpoint that motivates it, and this hardware cannot run the 2.4T (~4.8 TB bf16 against 128 GB unified memory), so the fallback cannot be measured here today. Listed under `## Owed` in [`gdn-moe-bf16-out.md`](specs/gdn-moe-bf16-out.md) | perf | | [#1171](https://github.com/mudler/vllm.cpp/issues/1171) | `KERNEL-GDN-REPLAYSSM` | GDN decode rewrites the whole `[HV,V,K]` fp32 state every step (`src/vt/cuda/cuda_gdn.cu:2393` reads the tile, `:2425` writes it back), which at the 27B shape `HV=32, V=128, K=128` is 2,097,152 bytes read plus the same written per layer, per request, per token. ReplaySSM keeps a per-slot ring of the last `L` steps' rank-1 factors `(d, k, g)`, reconstructs the state in registers, and writes it back only every `L` steps. vLLM implements the algorithm at the pin `555967922` for Mamba2 selective-state ONLY (`layers/mamba/ops/selective_state_update_replayssm_output_only.py`, ring shapes/dtypes `mamba_utils.py:84-93,202-221`, `use_replayssm` default `False` at `config/cache.py:152`, introduced `866fea2b` #48018) and it cannot reach GDN: `config/vllm.py:2318-2322` refuses any model not setting `supports_replayssm`, only `NemotronHForCausalLM` sets it (`models/nemotron_h.py:711`), `GDNAttentionMetadataBuilder` does not subclass the Mamba builder that derives the cursor (`v1/attention/backends/gdn_attn.py:82` vs `mamba_attn.py:575-638`), and the kernel hard-requires a scalar-per-head `A` (`:540-542`) with the Mamba2 `(B,C)` group structure (`:529`). Still true 877 commits past the pin. SGLang ported it to GDN at our recorded pin `f63458b5` (`layers/attention/fla/fused_recurrent_linear_replayssm.py`, whose `:50` credits vLLM; `--enable-linear-replayssm` default `False` and `--linear-replayssm-cache-len` default 16 at `server_args.py:1972-1986`; rings `memory_pool.py:465-483`; commit `a10a24e9` #28451), so the algorithm is a vLLM mirror and the GDN application is a secondary-oracle port. PAYOFF UNMEASURED HERE and deliberately not claimed: ReplaySSM removes the state WRITE and not the read, the flush step reads the checkpoint a SECOND time, so the honest state ratio is `(1+2/L)/2` = 0.5625 at `L=16` against SGLang's published 0.53x which models neither the flush re-read nor any ring read; the ring itself adds 395,264 bytes per slot per layer at the 27B shape = **+18.9% KV page**, worse than vLLM's ~7% on Nemotron because GDN's state is `V*K` while the ring is `L*(V+K)`; and SGLang's own end-to-end figure is ~2.3% TPOT at 128 concurrency on an MoE model. Neither upstream is bit-exact against its unbuffered path and neither claims to be. Motivation is the open Qwen3.8-27B bf16 decode gap (c4 total 0.918x, output 0.963x, `docs/BENCHMARKS.md:192-205`). Spec [`gdn-replayssm.md`](specs/gdn-replayssm.md) | perf | | [#1179](https://github.com/mudler/vllm.cpp/issues/1179) | `ENG-CUDAGRAPH-BREAK` | The hand-rolled decode-graph driver count recorded in `9bc4d7f44` is **eight** and is actually **nine**, and the row it feeds was framed as coverage-only when it is also correctness. The ninth is the DFlash draft graph, file-local with no header declaration, at `src/vllm/model_executor/models/qwen3_dflash.cpp:771,870,1038,1091,1095,1106` — its own `int g_state = 0` three-state machine (`:771`), its own `VT_DFLASH_GRAPH` kill switch (`:870`) instead of the `VLLM_CPP_CUDAGRAPH` the six batched drivers read, its own invalidate-on-block-width-change (`:1038-1047`) and its own `try { EndCaptureGraph(); } catch (...) {}` drain (`:1106`). The eight-count is stated in four places, all corrected here: [`sglang-breakable-cuda-graph.md`](specs/sglang-breakable-cuda-graph.md) §4 and `## Owed`, [`.agents/engine-matrix.md`](engine-matrix.md) rows `ENG-CUDAGRAPH-BREAK` and `ENG-CUDAGRAPH-DEDUP` ("times eight drivers", which sizes #1162's signature table), and [`.agents/roadmap_v1.md`](roadmap_v1.md) track `C12`. The reframing is the substantive half: `ENG-CUDAGRAPH-BREAK` was recorded as a COVERAGE row, and the duplication has already cost a SHIPPED model its decode graph. `src/vllm/model_executor/models/qwen3.cpp:961-986` declines the decode graph outright whenever the asynchronous device-token mirror is live, on its own measured battery — `depth-1, graph ON PASS 78/78`; `depth-2, graph OFF PASS 82/82`; `depth-2, graph ON FAIL, slots 1-3 degenerate` — because `Step()` replays against the HOST `input.token_ids` and the combine has patched the DEVICE ids. The comment names the real fix as reading the identifiers at replay time from a stable device buffer, and that fix exists, in exactly one sibling driver, as `StepDevInputs` (`src/vllm/model_executor/models/qwen3_5.cpp:3894`): `grep -c StepDevInputs` returns 41 lines there and 0 in each of `qwen3_moe.cpp`, `qwen3.cpp`, `deepseek_v2.cpp` and `voxtral.cpp`. One capability, written once, unavailable to four models, with a live mitigation standing in its place. This does NOT weaken the framing rule that `ENG-CUDAGRAPH` established: the row still makes no throughput claim, and the prefill refutation (GB10 3.8% host-idle between launches, GPU-busy >96%, 27B prefill gap 92.5% non-GEMM glue) stands unchanged. Coverage AND correctness, never speed. Fixed in flow with the [`eng-cudagraph-break.md`](specs/eng-cudagraph-break.md) review repair ([#1163](https://github.com/mudler/vllm.cpp/issues/1163)) | record | +| [#1181](https://github.com/mudler/vllm.cpp/issues/1181) | `FIX-READ-F32-SCALAR-GUARD` | `ReadF32Scalar` (`src/vllm/model_executor/models/qwen3_5_weights.cpp:312-318` @ `ab6e65216`) bounds its input with `t.data != nullptr && t.nbytes >= sizeof(float)`, a LOWER bound, and then `memcpy`s four bytes into a `float`. Two silent wrong-value paths follow and neither fails: an ARRAY is reduced to element 0, so a block-wise FP8 scale grid of shape `[ceil(N/128), ceil(K/128)]` passes and stands in for the whole weight (measured under [#1166](https://github.com/mudler/vllm.cpp/issues/1166) on `Qwen/Qwen3.8-27B-FP8` @ `017b9c7af6b5689d5dd426a76e0bc077eb5ca20a`, `q_proj.weight_scale_inv` is `[96, 40]`), and ANY dtype is reinterpreted, since that same tensor is `BF16` and its four bytes are two bf16 values read as one float. Both return a finite plausible float, so the output is fluent, plausible and wrong, which is what a token gate cannot see. Upstream makes both facts structural rather than optional: a per-tensor scale is a distinct parameter TYPE that asserts `loaded_weight.shape[0] == 1` (`vllm/model_executor/parameter.py:260-272,304-309` @ `555967922`, plus the `_assert_and_load` shape assert at `:93-96`), the slot is allocated `torch.float32` so a narrow on-disk dtype is VALUE-converted rather than reinterpreted (`utils/fp8_utils.py:1276`), and the declared strategy TENSOR/CHANNEL/BLOCK picks the parameter type before a byte is read (`compressed_tensors/schemes/compressed_tensors_w8a8_fp8.py:63,128`). The AUDIT corrects the issue's own framing twice. The 27 grep hits across five files are 5 definitions, 20 call sites and 2 comment references, and both counts are short: `ReadCtF32Scalar` (`include/vllm/model_executor/models/dense_weight_loaders.h:376`) is a SIXTH copy of the same defect under another name, reached from a SIXTH model file (`src/vllm/model_executor/models/qwen3_weights.cpp:100,126-128` through `LoadCtNvfp4W4A16`). Of the six, three check nothing, `LnReadF32Scalar`/`ShReadF32Scalar` check dtype but not count, and only `nemotron_h_weights.cpp:557-573` is correct, which makes it the model the shared guard generalizes. No call site legitimately passes a multi-element or non-F32 tensor, and every existing fixture emits rank-0 or `{1}` `F32`, so nothing in the tree needed the leniency. It is NOT merely latent: `dense_weight_loaders.h:73-74` and `docs/BENCHMARKS.md:52` both record `unsloth/Qwen3.6-27B-NVFP4` @ `ccdaab7e` as FP8 W8A8 throughout with BF16 PER-OUTPUT-CHANNEL scales, and `LoadAttnDense` branches on the weight dtype alone (`qwen3_5_dense_weights.cpp:478-480`), so those projections enter the per-tensor arm and hit both defects at once under the tensor name the loader actually asked for, with no misspelling to stop them. Fixed in flow by one `dense_loaders::ReadF32Scalar(get, name)` that refuses `numel != 1` naming the shape, refuses a non-`F32` dtype naming the dtype, and requires exactly four readable bytes, with the other five copies deleted onto it and `nemotron_h`'s `Loader`-based twin kept as the one tracked exception. A narrow dtype is refused rather than converted, because a one-element BF16 scale has never been read correctly here and the BF16 layout that IS shipped is per-channel, which the count check refuses first. Per-channel FP8, block-wise FP8 and any explicit narrow-dtype conversion stay owed. Spec [`read-f32-scalar-guard.md`](specs/read-f32-scalar-guard.md) | bug | diff --git a/.agents/specs/read-f32-scalar-guard.md b/.agents/specs/read-f32-scalar-guard.md new file mode 100644 index 000000000..8d3a109b7 --- /dev/null +++ b/.agents/specs/read-f32-scalar-guard.md @@ -0,0 +1,278 @@ +# Read a per-tensor scale as a per-tensor scale + +Row `FIX-READ-F32-SCALAR-GUARD`. Issue +[#1181](https://github.com/mudler/vllm.cpp/issues/1181). + +## Scope + +`ReadF32Scalar` copies four bytes out of a safetensors tensor and calls them a +float. It checks neither how many elements the tensor holds nor what dtype it +declares. Give it both checks, in one shared implementation the whole tree +reads through, and make each refusal name the offending tensor, its shape, and +its dtype. + +Out of scope, and recorded under `## Owed`: block-wise FP8, per-output-channel +FP8 on the per-tensor arm, and any explicit narrow-dtype conversion. Also out of +scope: the `#1166` refusal that landed in `469f38395`, which is untouched here. + +## 0. What is wrong today + +`src/vllm/model_executor/models/qwen3_5_weights.cpp:312-318` at `ab6e65216`: + +```cpp +float ReadF32Scalar(const StTensor& t) { + VT_CHECK(t.data != nullptr && t.nbytes >= sizeof(float), + "qwen3_5 weights: scalar tensor too small for f32"); + float v = 0.0F; + std::memcpy(&v, t.data, sizeof(float)); + return v; +} +``` + +`nbytes >= sizeof(float)` is a LOWER bound, so two wrong-value paths follow and +neither of them fails. + +1. **An array is silently reduced to element 0.** A block-wise FP8 scale grid of + shape `[ceil(N/128), ceil(K/128)]` passes trivially and is read as block + `(0, 0)`, which then stands in for the whole weight. Measured under #1166 on + `Qwen/Qwen3.8-27B-FP8` at revision `017b9c7af6b5689d5dd426a76e0bc077eb5ca20a`: + `q_proj.weight_scale_inv` is `[96, 40]`. +2. **Any dtype is reinterpreted.** The function copies four bytes whatever + `t.dtype` says. That same measured tensor is `BF16`, so the four bytes are + two bf16 values read as one float, and the result is not a scale at all. + +Both return a finite, plausible float. A wrong scale produces fluent, plausible, +wrong tokens, which is the failure mode a token gate cannot see. The tree +already argues exactly this at +`include/vllm/model_executor/models/dense_weight_loaders.h:78-80`, where a +per-output-channel scale read as per-tensor is called out as "silently WRONG +rather than loud". + +## 1. What upstream does, with anchors + +Pinned vLLM `5559679229bc961848b121ccdeaa8fa5d79bec98`, the parity pin in +[`upstream-sync.md`](../upstream-sync.md), verified with `git rev-parse` in the +oracle checkout before citing. + +| Anchor | What it does | +|---|---| +| `vllm/model_executor/parameter.py:260-272` | `PerTensorScaleParameter` is a distinct parameter TYPE. A per-tensor scale is not a raw tensor read, it is a typed slot | +| `vllm/model_executor/parameter.py:304-309` | `_load_into_shard_id` asserts `loaded_weight.shape[0] == 1` for any non-rank-0 scale, then asserts the shapes match. This is the element-count check we lack | +| `vllm/model_executor/parameter.py:93-96` | `_assert_and_load` asserts `self.data.shape == loaded_weight.shape` or the rank-0 to `[1]` case. Same rule on the non-sharded path | +| `vllm/model_executor/layers/quantization/utils/fp8_utils.py:1276` | the scale parameter is allocated `torch.float32`, so `copy_` VALUE-converts a narrower on-disk dtype rather than reinterpreting its bytes | +| `vllm/model_executor/layers/quantization/compressed_tensors/schemes/compressed_tensors_w8a8_fp8.py:63,84,128` | the declared STRATEGY (`TENSOR` / `CHANNEL` / `BLOCK`) picks the parameter type before a byte is read. Shape is never inferred from bytes | +| `vllm/model_executor/layers/quantization/fp8.py:358-366` | non-block FP8 registers `weight_scale` as a `PerTensorScaleParameter`, and the block arm registers a `BlockQuantScaleParameter` instead | +| `vllm/model_executor/layers/quantization/fp8.py:469-483` | at apply time upstream still branches on `weight_scale.numel() == 1` versus per-row. The element count is load-bearing on both sides | + +The mirror is exact and it is not a design question. `ReadF32Scalar` is this +tree's `PerTensorScaleParameter`. It kept the copy and dropped the assertions. + +## 2. The call-site audit + +`grep -rn ReadF32Scalar src include` over the five files the issue names returns +27 hits. Reconciled, those 27 are **5 definitions, 20 call sites, and 2 comment +references**, and the file list is INCOMPLETE. A sixth definition is spelled +differently and a sixth model file reaches it. + +### Definitions + +| Definition | dtype checked | element count checked | +|---|---|---| +| `src/vllm/model_executor/models/qwen3_5_weights.cpp:312` `ReadF32Scalar` | no | no | +| `src/vllm/model_executor/models/qwen3_5_dense_weights.cpp:37` `ReadF32Scalar` | no | no | +| `include/vllm/model_executor/models/dense_weight_loaders.h:376` `ReadCtF32Scalar` | no | no | +| `src/vllm/model_executor/models/laguna_weights.cpp:249` `LnReadF32Scalar` | yes | no | +| `src/vllm/model_executor/models/laguna_shared_fp4.cpp:73` `ShReadF32Scalar` | yes | no | +| `src/vllm/model_executor/models/nemotron_h_weights.cpp:557` `ReadF32Scalar` | yes | yes | + +Six hand-written copies of one function is the parallel path AGENTS.md forbids, +and the table is what a parallel path costs. Three copies check nothing, two +check half, and one is correct. `nemotron_h_weights.cpp:557-573` is the correct +one and is the model this row generalizes: it refuses a non-`F32` dtype by name, +refuses any shape that is neither rank-0 nor `[1]` by name, and requires exactly +four bytes. + +### Call sites, and what each passes + +Every site reads a scale that is per-tensor BY CONTRACT, so no site +LEGITIMATELY passes a multi-element or non-`F32` tensor. Two sites can be +REACHED by one, and that is the second finding below. + +| Site | Tensor | Expected on disk | +|---|---|---| +| `qwen3_5_weights.cpp:458` `LoadFp8Raw` | `.weight_scale` | F32 scalar | +| `qwen3_5_weights.cpp:459` `LoadFp8Raw` | `.input_scale` | F32 scalar | +| `qwen3_5_weights.cpp:480` `LoadFp8Transposed` | `.weight_scale` | F32 scalar | +| `qwen3_5_weights.cpp:510` `LoadNvfp4Raw` | `.weight_scale_2` | F32 scalar (ModelOpt) | +| `qwen3_5_dense_weights.cpp:180` `LoadCtNvfp4Raw` | `.weight_global_scale` | F32 scalar (CT divisor) | +| `qwen3_5_dense_weights.cpp:193` `LoadCtNvfp4Raw` | `.input_global_scale` | F32 scalar (CT divisor) | +| `qwen3_5_dense_weights.cpp:305` `LoadLmHeadAnyDtype` | `_scale_2` | F32 scalar (ModelOpt) | +| `qwen3_5_dense_weights.cpp:312` `LoadLmHeadAnyDtype` | `_global_scale` | F32 scalar (CT) | +| `qwen3_5_dense_weights.cpp:377` `LoadModelOptNvfp4Raw` | `.weight_scale_2` | F32 scalar | +| `qwen3_5_dense_weights.cpp:393` `LoadModelOptNvfp4Raw` | `.input_scale` | F32 scalar, behind `VT_MODELOPT_W4A4` | +| `qwen3_5_dense_weights.cpp:611` | `.weight_global_scale` | F32 scalar (CT) | +| `dense_weight_loaders.h:421` `LoadCtNvfp4W4A16` | `.weight_global_scale` | F32 scalar (CT) | +| `laguna_weights.cpp:296` `LnLoadCtNvfp4Raw` | `.weight_global_scale` | F32 scalar | +| `laguna_weights.cpp:303` `LnLoadCtNvfp4Raw` | `.input_global_scale` | F32 scalar | +| `laguna_weights.cpp:326` `LnLoadSharedExpertBf16` | `.weight_global_scale` | F32 scalar | +| `laguna_shared_fp4.cpp:95` `ShLoadSharedNvfp4W4A16` | `.weight_global_scale` | F32 scalar | +| `nemotron_h_weights.cpp:604` | `.weight_scale_2` | F32 scalar, already guarded | +| `nemotron_h_weights.cpp:626` | `.weight_scale` | F32 scalar, already guarded | +| `nemotron_h_weights.cpp:627` | `.input_scale` | F32 scalar, already guarded | +| `nemotron_h_weights.cpp:691` | `.k_proj.k_scale` | F32 scalar, already guarded | +| `nemotron_h_weights.cpp:692` | `.v_proj.v_scale` | F32 scalar, already guarded | + +Every existing fixture agrees. `tests/vllm/models/test_qwen3_8_text_only.cpp:345,346,355` +emits `{1}` `F32`. `tests/vllm/models/test_laguna_nvfp4_loader.cpp:166,167` and +`tests/vllm/models/test_qwen27_dense_forward.cpp:331,333,398` emit rank-0 or +`{1}` `F32`. `tests/vllm/models/ltx2_nvfp4_te_manifest.inc` is rank-0 `F32` +throughout. Nothing in the tree needs the leniency, so nothing in the tree +breaks when it is removed. + +### Finding A, named not absorbed: a SIXTH definition and a SIXTH model file + +`ReadCtF32Scalar` (`dense_weight_loaders.h:376`) is the same defect under +another name, it already lives in the shared header, and the issue does not list +it. Its one caller `LoadCtNvfp4W4A16` is reached from +`src/vllm/model_executor/models/qwen3_weights.cpp:100,126,127,128`, the Qwen3 +dense additive model, which the issue's five-file list also omits. Folded into +this row: the whole point is one implementation, and leaving a sixth copy out +would leave the class defect open in the file that is supposed to be the seam. + +### Finding B, named not absorbed: a live reachable wrong scale, not only a latent one + +The issue calls the defect latent because #1166's name miss stops the measured +`Qwen/Qwen3.8-27B-FP8` load first. That is true for THAT checkpoint. It is not +true for the class, and the tree already documents the counterexample. + +`dense_weight_loaders.h:73-74` records that `unsloth/Qwen3.6-27B-NVFP4` +@`ccdaab7e` "went FP8 across the whole tower with BF16 per-output-channel +scales", and `docs/BENCHMARKS.md:52` records the same revision as "the same repo +name re-quantized to FP8 W8A8 throughout, not NVFP4". +`qwen3_5_dense_weights.cpp:258` names that layout again for `lm_head`, where it +is handled correctly. + +`LoadAttnDense` branches on the weight dtype alone +(`qwen3_5_dense_weights.cpp:478-480`), so an `F8_E4M3` projection of that +checkpoint enters the per-tensor arm and `LoadFp8Raw` reads +`.weight_scale` through the unchecked reader. A `BF16 [out, 1]` scale +there is read as element `(0, 0)` of the wrong dtype: both defects at once, on a +real published revision, with the tensor NAME the loader asked for. Naming luck +does not protect this one, because nothing is misspelled. + +No recorded gate covers it. Every recorded 27B NVFP4 measurement ran +@`890bdef7` (`qwen3_5_dense_weights.cpp:227-229`, `docs/BENCHMARKS.md:52`), and +@`ccdaab7e` is documented as historically rejected by the bf16 dense loader. +So this row turns a silent wrong scale into a named refusal on a path no gate +was reading. The per-output-channel FP8 arm itself is a separate capability and +is recorded under `## Owed`. + +## 3. Design + +One function, in the shared seam, with the name in the signature so the message +can never be anonymous: + +```cpp +// include/vllm/model_executor/models/dense_weight_loaders.h +inline float ReadF32Scalar(const TensorResolver& get, const std::string& name); +``` + +It refuses in this order, and each refusal names `name`: + +1. `numel(t.shape) != 1`, reporting the shape it actually got and the element + count. Shape comes first because the array reduction is the defect that + survives a correct dtype. +2. `t.dtype != "F32"`, reporting the dtype it actually got. +3. `t.data == nullptr || t.nbytes != sizeof(float)`, an exact byte count rather + than a floor. + +The five other definitions are deleted and their 16 call sites route here. +`nemotron_h_weights.cpp:557` stays as it is: its checks are already a superset, +and it carries `Loader` bookkeeping (`Need`, `RefuseLoad`, `Consumed`) that the +resolver-based helper has no access to. That is the one exact tracked exception, +recorded here rather than in a registry. + +**A narrow dtype is refused, not converted.** Upstream converts by value +(`fp8_utils.py:1276` plus `copy_`), so conversion would be defensible. It is not +implemented, for two reasons that are evidence and not taste. No caller can be +shown to need it: a one-element BF16 scale has never been read correctly here, +because the four-byte copy takes two bytes of the scale and two bytes of +whatever follows it, so there is no working behavior to preserve. And the BF16 +scale layout that IS shipped is per-output-channel, which the element-count +check refuses first whatever the dtype rule says. Adding a conversion would be +untested code on a path no checkpoint reaches. Recorded under `## Owed`. + +Message prefix is `dense loader:`, matching the rest of the shared header, which +already states that a shared helper must not name one architecture +(`dense_weight_loaders.h:7-9`). The tensor name carries the architecture. + +## 4. Port map + +| Upstream | Local | +|---|---| +| `parameter.py:304-309` element-count assert | the `numel != 1` refusal | +| `parameter.py:93-96` shape assert | same, on the non-sharded path | +| `fp8_utils.py:1276` f32 scale slot | the `dtype != "F32"` refusal | +| `compressed_tensors_w8a8_fp8.py:128` strategy-to-type map | why the shape is never inferred from bytes | + +## 5. Tests + +`tests/vllm/models/test_qwen3_8_text_only.cpp`, which already builds synthetic +safetensors and drives the PRODUCTION entry point `vllm::LoadQwen3_5Moe`. The +file is registered at `tests/CMakeLists.txt:476`, so no registration edit is +owed and `scripts/check-test-registration.py` stays satisfied. + +RED first, and each case must fail for its own reason: + +1. a per-output-channel `weight_scale` (`{kMoeQ}` `F32`) on `q_proj` is refused, + and the message names the tensor, its shape and its element count. +2. a one-element `BF16` `weight_scale` is refused, and the message names the + tensor and `BF16`. +3. a multi-element `weight_scale_2` on a routed expert is refused, which reaches + `LoadNvfp4Raw` rather than the FP8 loaders. +4. positive control: the unmodified fixture, one element and `F32`, still loads. + +A happy-path case cannot detect either defect, because both defects ARE the +happy path. Case 4 exists only to prove the guard did not refuse everything. + +Reachability, per [`reachability.md`](../reachability.md): the guard is reached +from `LoadQwen3_5Moe`, not from a hand-built `StTensor`. Deleting the production +call site in a scratch copy must turn cases 1 to 3 red. + +## 6. Gates + +`scripts/agent-preflight.sh --fail-on-skip`, plus the focused +`test_qwen3_8_text_only` run recorded with its case and assertion counts. CPU +only. No GPU lease, no checkpoint download. + +## 7. Risks and decisions + +- **A currently loading checkpoint could start refusing.** Intended, and the + only shape that can do it is the one in finding B, which is loading with a + wrong scale today. A named refusal is strictly better than fluent wrong + tokens, and it is the same trade #1166 made. If a GPU run of + `unsloth/Qwen3.6-27B-NVFP4` @`ccdaab7e` starts refusing after this lands, that + refusal is the DISCOVERY, not the regression, and it belongs to the owed + per-channel FP8 arm. +- **Six copies into one changes six messages.** No test, document or spec + asserts any of the five deleted strings, verified by grep over `tests/`, + `docs/` and `.agents/`. +- **A shared helper in a header the SACRED 27B TU now includes.** The + qwen3_5_weights.cpp include is new. It adds no symbol that collides: the + file's own anonymous-namespace `MakeOwned` and `TransposeBf16` stay + unqualified and keep winning, since the shared ones need + `dense_loaders::`. + +## Owed + +- Per-output-channel FP8 on the per-tensor arm, for + `unsloth/Qwen3.6-27B-NVFP4` @`ccdaab7e` and any republish like it. Needs its + own row, a GPU gate and a scale-aware `Fp8Weight`. +- Block-wise FP8, still owed by + [`fp8-blockwise-refusal.md`](fp8-blockwise-refusal.md) and #1166. +- Explicit narrow-dtype conversion for a one-element `BF16` or `F16` scale, if + a checkpoint is ever measured that ships one. + +## Now + +`ACTIVE`. Spec committed before implementation. From 8cff7611340e44208638bc98c9dccf10b401f8bb Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Tue, 18 Aug 2026 04:01:25 +0000 Subject: [PATCH 2/4] test(FIX-READ-F32-SCALAR-GUARD): RED -- four wrong-scale shapes load CLEAN through LoadQwen3_5Moe (#1181) Six refusal subcases plus a positive control, all driven through the production `vllm::LoadQwen3_5Moe` over a synthetic checkpoint rather than a hand-built `StTensor`, so they measure a capability rather than a class. RED at this commit: 16 of 29 assertions fail, 6 of 6 refusal subcases fail, the positive control passes. The shape of the red is the finding. A per-output-channel `weight_scale` `[8] F32`, a block-wise grid `[2, 4] F32`, a per-output-channel `[8] BF16` and a multi-element `weight_scale_2 [2] F32` all LOAD, with an EMPTY exception message. Eleven of the failed assertions report `message := ` with nothing after it. The checkpoint is accepted, every tensor is found, every dtype the loader asks about is right, and the only wrong thing is a number that will be multiplied into every token. The two one-element BF16 cases do stop, on `scalar tensor too small for f32` at `qwen3_5_weights.cpp:313`, which names no tensor and describes a truncation rather than a dtype. Eight BF16 values are sixteen bytes, so that floor cannot see the shape that is actually published. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code] --- tests/vllm/models/test_qwen3_8_text_only.cpp | 162 +++++++++++++++++++ 1 file changed, 162 insertions(+) diff --git a/tests/vllm/models/test_qwen3_8_text_only.cpp b/tests/vllm/models/test_qwen3_8_text_only.cpp index bd0fdfa50..b09cf08f2 100644 --- a/tests/vllm/models/test_qwen3_8_text_only.cpp +++ b/tests/vllm/models/test_qwen3_8_text_only.cpp @@ -2147,6 +2147,168 @@ TEST_CASE("qwen3_8: the MoE and dense probes classify a projection identically") } } +// =========================================================================== +// 4l. A PER-TENSOR SCALE IS READ AS A PER-TENSOR SCALE (issue #1181) +// +// `ReadF32Scalar` used to bound its input with `t.nbytes >= sizeof(float)`, +// a LOWER bound, and then copy four bytes into a `float` whatever the +// tensor's dtype said. Two wrong-value paths followed and NEITHER failed: +// an array was reduced to element 0, and a narrow dtype was reinterpreted +// rather than converted. Both return a finite, plausible float, so the +// model emits fluent wrong tokens instead of stopping, which is exactly the +// failure a token gate cannot see. +// +// WHY THERE IS NO HAPPY-PATH CASE FOR THIS. Both defects ARE the happy +// path: the load succeeded, every tensor was found, every dtype the loader +// asked about was right, and the only wrong thing was a number. So each +// case below FEEDS the shape or dtype that used to pass and requires a +// refusal that NAMES the tensor. The positive control exists only to prove +// the guard did not start refusing everything. +// +// THE MUTATION IS THE FIXTURE, NOT A HAND-BUILT `StTensor`. Every case runs +// the production `vllm::LoadQwen3_5Moe` over a synthetic checkpoint, so +// deleting the production call site inside `LoadFp8Raw` / `LoadNvfp4Raw` +// turns these red. A unit test that called the reader directly would prove +// the function works and nothing about anything reaching it. +// +// UPSTREAM MAKES BOTH CHECKS STRUCTURAL, at pin `555967922`. A per-tensor +// scale is its own parameter TYPE, and `PerTensorScaleParameter` asserts +// `loaded_weight.shape[0] == 1` (`vllm/model_executor/parameter.py:304-309`, +// with the sibling shape assert in `_assert_and_load` at `:93-96`). The +// slot is allocated `torch.float32` (`utils/fp8_utils.py:1276`), so a +// narrower on-disk dtype is value-converted by `copy_` and never +// reinterpreted. And the declared strategy picks the parameter type before +// a byte is read (`compressed_tensors_w8a8_fp8.py:63,128`), so upstream +// never infers a scale's shape from its byte count. +// =========================================================================== +namespace { + +// Return `specs` with the entry named `name` re-declared at `shape`/`dtype`. +// +// The `REQUIRE` is load-bearing, not decoration. An anchor that matched zero +// entries would hand the case an UNMODIFIED fixture, which loads, and a +// refusal case whose fixture was never mutated reads exactly like a test that +// passed. +std::vector Redeclared(std::vector specs, const std::string& name, + const std::vector& shape, + const std::string& dtype) { + size_t hits = 0; + for (Spec& s : specs) { + if (s.name != name) continue; + s.shape = shape; + s.dtype = dtype; + ++hits; + } + CAPTURE(name); + REQUIRE(hits == 1); + return specs; +} + +} // namespace + +TEST_CASE("qwen3_8: a per-tensor scale that is not ONE F32 element is REFUSED by name") { + auto load = [](const std::vector& specs, const char* tag) { + return CaptureThrow([&specs, tag] { + const TempFile file(BuildSafetensors(specs), tag); + std::vector shards; + shards.push_back(vllm::SafetensorsFile::Open(file.path())); + (void)vllm::LoadQwen3_5Moe(shards, OneLayerMoeConfig()); + }); + }; + + const std::vector good = MoeOneLayerSpecs("model."); + // The FP8 attention scale, read on BOTH arms of `DenseNativeEnabled()`: + // `LoadFp8Raw` on a CUDA + cutlass build, `LoadFp8Transposed` otherwise. That + // is why this case anchors on `weight_scale` rather than on `input_scale`, + // which only the fp8-resident arm reads and which a CPU gate never reaches. + const std::string kWeightScale = + "model.layers.0.self_attn.q_proj.weight_scale"; + // The ModelOpt NVFP4 global, read by `LoadNvfp4Raw` — a different function, + // so the two anchors together prove the guard is shared rather than local. + const std::string kWeightScale2 = + "model.layers.0.mlp.experts.0.gate_proj.weight_scale_2"; + + SUBCASE("POSITIVE CONTROL: one F32 element still loads") { + CHECK(load(good, "scale_ok").empty()); + } + + SUBCASE("a PER-OUTPUT-CHANNEL F32 weight_scale is refused, and the shape is in the message") { + // The shape `unsloth/Qwen3.6-27B-NVFP4` @ccdaab7e ships across its FP8 + // tower. `LoadAttnDense` branches on the WEIGHT dtype alone, so a + // projection like this enters the per-tensor arm, and before this guard + // element (0, 0) silently became the scale of the whole matrix. + const std::string message = + load(Redeclared(good, kWeightScale, {kMoeQ}, "F32"), "scale_rows"); + CAPTURE(message); + CHECK(Mentions(message, kWeightScale.c_str())); + CHECK(Mentions(message, "[8]")); + CHECK(Mentions(message, "8 elements")); + // The two shapes it must NOT degrade into: a complaint about some other + // tensor, or a bare lookup miss. + CHECK_FALSE(Mentions(message, "tensor not found")); + } + + SUBCASE("a BLOCK-WISE F32 weight_scale grid is refused, and the shape is in the message") { + // The `[ceil(N/128), ceil(K/128)]` form measured on `Qwen/Qwen3.8-27B-FP8` + // as `[96, 40]` (#1166). At this fixture's toy dimensions the grid is + // `[2, 4]`, and the point is the RANK and the count, not the divisor. + const std::string message = + load(Redeclared(good, kWeightScale, {2, 4}, "F32"), "scale_grid"); + CAPTURE(message); + CHECK(Mentions(message, kWeightScale.c_str())); + CHECK(Mentions(message, "[2, 4]")); + CHECK(Mentions(message, "8 elements")); + CHECK_FALSE(Mentions(message, "tensor not found")); + } + + SUBCASE("a PER-OUTPUT-CHANNEL BF16 weight_scale is refused, and the message names it") { + // THE SHAPE AND THE DTYPE THAT ARE ACTUALLY PUBLISHED TOGETHER, and the one + // combination the old byte floor could not even trip over: eight bf16 + // values are SIXTEEN bytes, comfortably past `nbytes >= sizeof(float)`, so + // the load used to succeed while reading the first two scales as the + // mantissa and exponent of a number nobody wrote. + const std::string message = + load(Redeclared(good, kWeightScale, {kMoeQ}, "BF16"), "scale_rows_bf16"); + CAPTURE(message); + CHECK(Mentions(message, kWeightScale.c_str())); + CHECK(Mentions(message, "[8]")); + CHECK_FALSE(Mentions(message, "tensor not found")); + } + + SUBCASE("a ONE-ELEMENT BF16 weight_scale is refused, and the dtype is in the message") { + // This one the old byte floor DID stop, at two bytes, and that is the point: + // it stopped with "scalar tensor too small for f32", which names no tensor + // and describes a truncation rather than a dtype. A reader cannot tell from + // it which of hundreds of scales was wrong, or in what way. + const std::string message = + load(Redeclared(good, kWeightScale, {1}, "BF16"), "scale_bf16"); + CAPTURE(message); + CHECK(Mentions(message, kWeightScale.c_str())); + CHECK(Mentions(message, "BF16")); + CHECK(Mentions(message, "F32")); + CHECK_FALSE(Mentions(message, "tensor not found")); + } + + SUBCASE("a multi-element NVFP4 weight_scale_2 is refused, and the message names it") { + const std::string message = + load(Redeclared(good, kWeightScale2, {2}, "F32"), "scale2_multi"); + CAPTURE(message); + CHECK(Mentions(message, kWeightScale2.c_str())); + CHECK(Mentions(message, "[2]")); + CHECK(Mentions(message, "2 elements")); + CHECK_FALSE(Mentions(message, "tensor not found")); + } + + SUBCASE("a ONE-ELEMENT BF16 NVFP4 weight_scale_2 is refused, and the dtype is in the message") { + const std::string message = + load(Redeclared(good, kWeightScale2, {1}, "BF16"), "scale2_bf16"); + CAPTURE(message); + CHECK(Mentions(message, kWeightScale2.c_str())); + CHECK(Mentions(message, "BF16")); + CHECK_FALSE(Mentions(message, "tensor not found")); + } +} + // =========================================================================== // 5. INERTNESS of the gated rows. 27B / 35B / Coder are VL-prefixed // checkpoints; the per-layer public seams keep the VL prefix as their From 60aefbca62ed2ed3a1bbe38c4a3dc1d02fe12ed5 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Tue, 18 Aug 2026 04:18:09 +0000 Subject: [PATCH 3/4] fix(FIX-READ-F32-SCALAR-GUARD): a per-tensor scale is ONE F32 element, and six copies of the reader had stopped saying so (#1181) `ReadF32Scalar` bounded its input with `t.nbytes >= sizeof(float)` and then copied four bytes into a `float`. A floor admits every wrong answer that is large enough, so an array was read as element 0 and any dtype was reinterpreted rather than converted, and both returned a finite plausible number. A plausible wrong scale yields fluent wrong tokens rather than a failure, which is the one defect class a token gate cannot see. One implementation now, `dense_loaders::ReadF32Scalar(get, name)`. It refuses `numel != 1` naming the shape and the count, refuses a non-F32 dtype naming the dtype, and requires exactly four readable bytes. The name is in the signature so a refusal can never be anonymous, which is what the old `scalar tensor too small for f32` was. Five local copies are deleted onto it: `qwen3_5_weights.cpp`, `qwen3_5_dense_weights.cpp`, `laguna_weights.cpp`, `laguna_shared_fp4.cpp`, and `ReadCtF32Scalar` in `dense_weight_loaders.h` itself, which the issue did not list and which a sixth model file reaches through `LoadCtNvfp4W4A16`. `nemotron_h_weights.cpp:557` stays as it is: its checks are already a superset, and it carries `Loader` bookkeeping the resolver-based helper has no access to. That is the one tracked exception. A narrow dtype is refused rather than converted. Upstream converts by value, so conversion would be defensible, but no caller here can be shown to need it: a one-element BF16 scale has never been read correctly on this path, and the BF16 layout that IS published is per-output-channel, which the count check refuses first. Converting would be untested code on a path no checkpoint reaches. Mirrors `PerTensorScaleParameter` at pin `555967922`, which asserts `loaded_weight.shape[0] == 1` before it copies (`vllm/model_executor/parameter.py:304-309`), allocates the slot `torch.float32` (`utils/fp8_utils.py:1276`), and picks the parameter type from the declared TENSOR / CHANNEL / BLOCK strategy before a byte is read (`compressed_tensors_w8a8_fp8.py:63,128`). FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code] --- docs/USAGE.md | 25 ++++++ .../models/dense_weight_loaders.h | 76 ++++++++++++++++--- .../models/laguna_shared_fp4.cpp | 14 ++-- .../model_executor/models/laguna_weights.cpp | 17 ++--- .../models/qwen3_5_dense_weights.cpp | 25 +++--- .../model_executor/models/qwen3_5_weights.cpp | 24 +++--- 6 files changed, 125 insertions(+), 56 deletions(-) diff --git a/docs/USAGE.md b/docs/USAGE.md index 8a567464b..4838477f8 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -551,6 +551,31 @@ missing arm is in this project. To run the same model here, use a per-tensor FP8, BF16, NVFP4, or GGUF checkpoint of it. Issue [#1166](https://github.com/mudler/vllm.cpp/issues/1166) tracks the port. +### A per-tensor scale has to be one F32 number + +Every scale this build reads as a single number is required to be exactly one +element and exactly `F32`. That covers `weight_scale`, `input_scale`, +`weight_scale_2`, `weight_global_scale`, `input_global_scale`, `k_scale` and +`v_scale`. A checkpoint that stores one of them as an array, or in a narrower +dtype, is refused at load with a message naming the tensor, the shape it +shipped, and the dtype it shipped: + +```text +dense loader: 'model.layers.0.self_attn.q_proj.weight_scale' ships shape +[12288, 1] (12288 elements), not the ONE element a per-tensor scale is +``` + +The two layouts this refuses in practice are per-output-channel FP8, which +stores one scale per output row, and block-wise FP8, which stores a grid. Both +used to load. The reader took the first four bytes and used them as the scale +of the whole matrix, which is a finite plausible number and therefore fluent +plausible wrong output rather than a failure. Issue +[#1181](https://github.com/mudler/vllm.cpp/issues/1181) has the detail, and the +per-output-channel arm itself is not implemented yet. + +`lm_head` is not affected. It has always read a per-output-channel scale +correctly, as the table above records. + ### Architectures that resolve but refuse to run A few architectures are registered so their config and weight layout are diff --git a/include/vllm/model_executor/models/dense_weight_loaders.h b/include/vllm/model_executor/models/dense_weight_loaders.h index a4271d280..fc032d957 100644 --- a/include/vllm/model_executor/models/dense_weight_loaders.h +++ b/include/vllm/model_executor/models/dense_weight_loaders.h @@ -12,6 +12,8 @@ // // Helpers (all in `vllm::dense_loaders`): // MakeOwned — allocate a zero-filled OwnedTensor of dtype+shape. +// ReadF32Scalar — one per-tensor F32 scale, count and dtype CHECKED +// (#1181): the six local copies of this disagreed. // TransposeBf16 — bf16 [rows,cols] -> bf16 [cols,rows]. // LoadBf16Direct — copy a BF16 tensor verbatim (optionally reshaped). // LoadBf16Transposed — BF16 [out,in] -> owned bf16 [in,out] (Matmul-B). @@ -52,6 +54,69 @@ inline OwnedTensor MakeOwned(vt::DType dt, const std::vector& shape) { return o; } +// `[96, 40]`, `[8]`, `[]` -- the shape as a reader can compare it against a +// checkpoint's own header. +inline std::string ShapeString(const std::vector& shape) { + std::string s = "["; + for (size_t i = 0; i < shape.size(); ++i) { + if (i != 0) s += ", "; + s += std::to_string(shape[i]); + } + return s + "]"; +} + +// THE per-tensor f32 scale read. One implementation, because six hand-written +// copies of it disagreed about what a scale is (issue #1181). +// +// UPSTREAM, at pin `555967922`. A per-tensor scale is not a raw tensor read +// there, it is a parameter TYPE: `PerTensorScaleParameter` +// (`vllm/model_executor/parameter.py:260-272`) asserts +// `loaded_weight.shape[0] == 1` for any non-rank-0 scale before it copies +// (`:304-309`), with the sibling shape assert in `_assert_and_load` at +// `:93-96`. The slot is allocated `torch.float32` +// (`layers/quantization/utils/fp8_utils.py:1276`), so `copy_` VALUE-converts a +// narrower on-disk dtype and never reinterprets its bytes. And the declared +// strategy -- TENSOR, CHANNEL or BLOCK -- picks the parameter type before a +// byte is read (`compressed_tensors/schemes/compressed_tensors_w8a8_fp8.py:63,128`), +// so upstream never infers a scale's shape from its byte count. Both checks +// below are that type's whole job on this side. +// +// WHY AN EXACT COUNT AND AN EXACT DTYPE, NOT A FLOOR. The previous bound was +// `t.nbytes >= sizeof(float)`, and a floor admits every wrong answer that is +// large enough. A block-wise grid `[ceil(N/128), ceil(K/128)]` passed and was +// read as block (0, 0), which then stood in for the whole matrix. A +// per-output-channel `[out] BF16` scale passed at two bytes an element and was +// read as one float built from the first two entries. Neither raised anything, +// because both produce a finite, plausible number, and a plausible wrong scale +// yields fluent wrong tokens rather than a failure. That is the one defect +// class a token gate cannot see. +// +// A NARROW DTYPE IS REFUSED RATHER THAN CONVERTED, deliberately. Upstream +// converts, so conversion would be defensible, but no caller here can be shown +// to need it: a one-element BF16 scale has never been read correctly on this +// path, since the four-byte copy took two bytes of the scale and two bytes of +// whatever followed it. The BF16 scale layout that IS published is +// per-output-channel, which the count check refuses first whatever the dtype +// rule says. Converting would be untested code on a path no checkpoint reaches. +inline float ReadF32Scalar(const TensorResolver& get, const std::string& name) { + const StTensor& t = get(name); + int64_t numel = 1; + for (const int64_t d : t.shape) numel *= d; + VT_CHECK(numel == 1, + "dense loader: '" + name + "' ships shape " + ShapeString(t.shape) + + " (" + std::to_string(numel) + + " elements), not the ONE element a per-tensor scale is"); + VT_CHECK(t.dtype == "F32", + "dense loader: '" + name + "' ships dtype " + t.dtype + + ", not the F32 a per-tensor scale is"); + VT_CHECK(t.data != nullptr && t.nbytes == sizeof(float), + "dense loader: '" + name + + "' is a one-element F32 scale but does not carry 4 readable bytes"); + float v = 0.0F; + std::memcpy(&v, t.data, sizeof(float)); + return v; +} + // src bf16 [rows, cols] -> dst bf16 [cols, rows]. inline void TransposeBf16(const void* src, int64_t rows, int64_t cols, uint16_t* dst) { @@ -372,15 +437,6 @@ inline OwnedTensor LoadMergedBf16Vector(const TensorResolver& get, // `alpha` is deliberately left 0 so `Nvfp4Weight::IsTrueW4A4()` is false and the // weight routes to the W4A16 (Marlin, bf16-activation) dispatcher. -// Read a per-tensor f32 scalar (the CT global scales are 1-element F32 tensors). -inline float ReadCtF32Scalar(const StTensor& t, const std::string& name) { - VT_CHECK(t.data != nullptr && t.nbytes >= sizeof(float), - "dense loader: scalar tensor too small for f32: " + name); - float v = 0.0F; - std::memcpy(&v, t.data, sizeof(float)); - return v; -} - // True when `proj` is stored as a compressed-tensors NVFP4 linear. This is the // per-layer scheme probe: presence of `.weight_packed` means the config group // matched this Linear (vLLM resolves the same thing through `find_matched_target` @@ -418,7 +474,7 @@ inline Nvfp4Weight LoadCtNvfp4W4A16( " carries input_global_scale (W4A4); the dense NVFP4 loader " "implements the WEIGHT-ONLY W4A16 scheme only"); const float wgs_disk = - ReadCtF32Scalar(get(proj + ".weight_global_scale"), proj); + ReadF32Scalar(get, proj + ".weight_global_scale"); VT_CHECK(wgs_disk != 0.0F, "dense loader: zero weight_global_scale (divisor) for " + proj); diff --git a/src/vllm/model_executor/models/laguna_shared_fp4.cpp b/src/vllm/model_executor/models/laguna_shared_fp4.cpp index 336bf1914..364d77e39 100644 --- a/src/vllm/model_executor/models/laguna_shared_fp4.cpp +++ b/src/vllm/model_executor/models/laguna_shared_fp4.cpp @@ -38,6 +38,7 @@ #include #include "vllm/model_executor/model_loader/safetensors_reader.h" // SafetensorsFile, StTensor +#include "vllm/model_executor/models/dense_weight_loaders.h" // ReadF32Scalar (#1181) #include "vllm/model_executor/models/dense_weight_loaders.h" // dense_loaders::MakeOwned #include "vllm/model_executor/models/qwen3_5_weights.h" // Nvfp4Weight, OwnedTensor #include "vt/dtype.h" // VT_CHECK, vt::DType @@ -69,13 +70,10 @@ bool LagunaHasFp4SharedExpert(const LagunaWeights& w) { namespace { -// F32 scalar read (mirror laguna_weights.cpp LnReadF32Scalar). -float ShReadF32Scalar(const StTensor& t) { - VT_CHECK(t.dtype == "F32" && t.nbytes >= 4, "laguna shared-fp4: F32 scalar expected"); - float v; - std::memcpy(&v, t.data, 4); - return v; -} +// F32 scalar read, from the shared seam. This was a hand-copied mirror of +// laguna_weights.cpp's own copy, and both bounded the size with `nbytes >= 4`, +// a FLOOR, so a scale ARRAY was read as element 0 (#1181). +using dense_loaders::ReadF32Scalar; // W4A16 fp4-raw read of one shared-expert projection. Byte-identical to // laguna_weights.cpp LnLoadCtNvfp4Raw for the weight fields (n/k/scale2/packed/ @@ -92,7 +90,7 @@ Nvfp4Weight ShLoadSharedNvfp4W4A16( VT_CHECK(in_dim % 16 == 0, "laguna shared-fp4: in_dim must be %16 for " + proj); const StTensor& ws = get(proj + ".weight_scale"); VT_CHECK(ws.dtype == "F8_E4M3", "laguna shared-fp4: F8_E4M3 weight_scale for " + proj); - const float wgs = ShReadF32Scalar(get(proj + ".weight_global_scale")); + const float wgs = ReadF32Scalar(get, proj + ".weight_global_scale"); VT_CHECK(wgs != 0.0F, "laguna shared-fp4: zero weight_global_scale for " + proj); Nvfp4Weight r; r.n = out_dim; diff --git a/src/vllm/model_executor/models/laguna_weights.cpp b/src/vllm/model_executor/models/laguna_weights.cpp index 03dd69267..88cfcc0bd 100644 --- a/src/vllm/model_executor/models/laguna_weights.cpp +++ b/src/vllm/model_executor/models/laguna_weights.cpp @@ -245,13 +245,10 @@ std::string LagunaGgufMoeName(int64_t layer, const char* which) { namespace { -// F32 scalar read (weight/input global scales are F32 scalars). -float LnReadF32Scalar(const StTensor& t) { - VT_CHECK(t.dtype == "F32" && t.nbytes >= 4, "laguna nvfp4: F32 scalar expected"); - float v; - std::memcpy(&v, t.data, 4); - return v; -} +// F32 scalar read (weight/input global scales are F32 scalars), from the shared +// seam. The local copy this replaces checked the dtype but bounded the size with +// `nbytes >= 4`, a FLOOR, so a scale ARRAY was read as element 0 (#1181). +using dense_loaders::ReadF32Scalar; // F32 tensor materialized (the e_score_correction_bias is F32 [E] on most shards, but // some poolside NVFP4 shards store it BF16 — upconvert those so the router/topk always @@ -293,14 +290,14 @@ Nvfp4Weight LnLoadCtNvfp4Raw(const TensorResolver& get, const std::string& proj) VT_CHECK(in_dim % 16 == 0, "laguna nvfp4: in_dim must be %16 for " + proj); const StTensor& ws = get(proj + ".weight_scale"); VT_CHECK(ws.dtype == "F8_E4M3", "laguna nvfp4: F8_E4M3 weight_scale for " + proj); - const float wgs = LnReadF32Scalar(get(proj + ".weight_global_scale")); + const float wgs = ReadF32Scalar(get, proj + ".weight_global_scale"); VT_CHECK(wgs != 0.0F, "laguna nvfp4: zero weight_global_scale for " + proj); Nvfp4Weight r; r.n = out_dim; r.k = in_dim; r.weight_global_scale_inv = wgs; r.scale2 = 1.0F / wgs; - const float igs = LnReadF32Scalar(get(proj + ".input_global_scale")); + const float igs = ReadF32Scalar(get, proj + ".input_global_scale"); VT_CHECK(igs != 0.0F, "laguna nvfp4: zero input_global_scale for " + proj); r.input_global_scale_inv = igs; r.alpha = r.scale2 * (1.0F / igs); @@ -323,7 +320,7 @@ OwnedTensor LnLoadSharedExpertBf16(const TensorResolver& get, if (!has(proj + ".weight_packed")) return dense_loaders::LoadBf16Direct(get, proj + ".weight"); // S-2.1 bf16 path const Nvfp4Weight r = LnLoadCtNvfp4Raw(get, proj); // XS NVFP4 path - const float wgs = LnReadF32Scalar(get(proj + ".weight_global_scale")); + const float wgs = ReadF32Scalar(get, proj + ".weight_global_scale"); std::vector f32(static_cast(r.n) * static_cast(r.k)); DequantCtNvfp4WeightToF32(reinterpret_cast(r.packed.bytes.data()), reinterpret_cast(r.scale.bytes.data()), wgs, r.n, r.k, diff --git a/src/vllm/model_executor/models/qwen3_5_dense_weights.cpp b/src/vllm/model_executor/models/qwen3_5_dense_weights.cpp index 6be8446ac..af0caea59 100644 --- a/src/vllm/model_executor/models/qwen3_5_dense_weights.cpp +++ b/src/vllm/model_executor/models/qwen3_5_dense_weights.cpp @@ -29,19 +29,14 @@ namespace vllm { using dense_loaders::LoadBf16Direct; using dense_loaders::LoadBf16Transposed; using dense_loaders::MakeOwned; +// The per-tensor scale read (#1181). The local copy this replaces checked +// neither the element count nor the dtype. +using dense_loaders::ReadF32Scalar; namespace { using TensorExists = std::function; -float ReadF32Scalar(const StTensor& t) { - VT_CHECK(t.data != nullptr && t.nbytes >= sizeof(float), - "qwen3_5 dense: scalar tensor too small for f32"); - float v = 0.0F; - std::memcpy(&v, t.data, sizeof(float)); - return v; -} - // The GDN in-projections stay RAW in the on-disk torch Linear [out, in] // orientation (nk=true, via LoadMergedBf16RawNK), consumed by vt::MatmulBT — // the cuBLASLt TN fast path (K contiguous in both operands, the layout vLLM's @@ -177,7 +172,7 @@ Nvfp4Weight LoadCtNvfp4Raw(const TensorResolver& get, const std::string& proj) { const StTensor& ws = get(proj + ".weight_scale"); VT_CHECK(ws.dtype == "F8_E4M3", "qwen3_5 dense: expected F8_E4M3 weight_scale for " + proj); - const float wgs_disk = ReadF32Scalar(get(proj + ".weight_global_scale")); + const float wgs_disk = ReadF32Scalar(get, proj + ".weight_global_scale"); VT_CHECK(wgs_disk != 0.0F, "qwen3_5 dense: zero weight_global_scale (divisor) for " + proj); @@ -190,7 +185,7 @@ Nvfp4Weight LoadCtNvfp4Raw(const TensorResolver& get, const std::string& proj) { // (used DIRECTLY), and alpha folds both reciprocated globals for the fp4xfp4 // GEMM: alpha = (1/input_divisor)·(1/weight_divisor). input_global_scale is a // per-tensor F32 scalar present on every 27B quantized Linear (§3.2). - const float igs_disk = ReadF32Scalar(get(proj + ".input_global_scale")); + const float igs_disk = ReadF32Scalar(get, proj + ".input_global_scale"); VT_CHECK(igs_disk != 0.0F, "qwen3_5 dense: zero input_global_scale (divisor) for " + proj); r.input_global_scale_inv = igs_disk; // on-disk divisor, used directly @@ -302,14 +297,14 @@ OwnedTensor LoadLmHeadAnyDtype(const TensorResolver& get, const TensorExists& ha // spells it `weight_global_scale` and stores the reciprocal. float disk_divisor = 0.0F; if (has(name + "_scale_2")) { - const float ws2 = ReadF32Scalar(get(name + "_scale_2")); + const float ws2 = ReadF32Scalar(get, name + "_scale_2"); VT_CHECK(ws2 != 0.0F, "qwen3_5 dense: zero " + name + "_scale_2"); disk_divisor = 1.0F / ws2; // ModelOpt scale -> CT divisor convention } else { VT_CHECK(has(name + "_global_scale"), "qwen3_5 dense: NVFP4 " + name + " requires " + name + "_scale_2 (ModelOpt) or " + name + "_global_scale (CT)"); - disk_divisor = ReadF32Scalar(get(name + "_global_scale")); + disk_divisor = ReadF32Scalar(get, name + "_global_scale"); VT_CHECK(disk_divisor != 0.0F, "qwen3_5 dense: zero " + name + "_global_scale (divisor)"); } @@ -374,7 +369,7 @@ Nvfp4Weight LoadNvfp4AnyNaming(const TensorResolver& get, const TensorExists& ha const StTensor& ws = get(proj + ".weight_scale"); VT_CHECK(ws.dtype == "F8_E4M3", "qwen3_5 dense: expected F8_E4M3 weight_scale for " + proj); - const float ws2 = ReadF32Scalar(get(proj + ".weight_scale_2")); + const float ws2 = ReadF32Scalar(get, proj + ".weight_scale_2"); VT_CHECK(ws2 != 0.0F, "qwen3_5 dense: zero weight_scale_2 for " + proj); Nvfp4Weight r; @@ -390,7 +385,7 @@ Nvfp4Weight LoadNvfp4AnyNaming(const TensorResolver& get, const TensorExists& ha // fp4-activation GEMM; leaving alpha at 0 keeps the weight-only dispatcher. const bool w4a4_opt_in = ModelOptW4A4OptIn(); if (w4a4_opt_in && has(proj + ".input_scale")) { - const float is = ReadF32Scalar(get(proj + ".input_scale")); + const float is = ReadF32Scalar(get, proj + ".input_scale"); if (is != 0.0F) { r.input_global_scale_inv = is; r.alpha = r.scale2 * (1.0F / is); @@ -608,7 +603,7 @@ OwnedTensor MaterializeCtNvfp4Bf16Transposed(const TensorResolver& get, const StTensor& wscale = get(proj + ".weight_scale"); VT_CHECK(wscale.dtype == "F8_E4M3", "qwen3_5 dense: expected F8_E4M3 weight_scale for " + proj); - const float wgs_disk = ReadF32Scalar(get(proj + ".weight_global_scale")); + const float wgs_disk = ReadF32Scalar(get, proj + ".weight_global_scale"); // Dequant to f32 [out, in] (the divisor is reciprocated inside), then round to // bf16 while transposing to Matmul-B layout [in, out]. diff --git a/src/vllm/model_executor/models/qwen3_5_weights.cpp b/src/vllm/model_executor/models/qwen3_5_weights.cpp index 8b83fca21..1f94c70aa 100644 --- a/src/vllm/model_executor/models/qwen3_5_weights.cpp +++ b/src/vllm/model_executor/models/qwen3_5_weights.cpp @@ -21,6 +21,7 @@ #endif #include "vllm/model_executor/model_loader/nvfp4_dequant.h" +#include "vllm/model_executor/models/dense_weight_loaders.h" // ReadF32Scalar (#1181) #include "vllm/model_executor/models/qwen3_vl.h" // LoadQwen3VLVisionWeights (#891) #include "vt/backend.h" #include "vt/dtype.h" @@ -309,13 +310,10 @@ OwnedTensor MakeOwned(vt::DType dt, const std::vector& shape) { return o; } -float ReadF32Scalar(const StTensor& t) { - VT_CHECK(t.data != nullptr && t.nbytes >= sizeof(float), - "qwen3_5 weights: scalar tensor too small for f32"); - float v = 0.0F; - std::memcpy(&v, t.data, sizeof(float)); - return v; -} +// The per-tensor scale read, from the shared seam. It used to be a local copy +// bounded by `nbytes >= sizeof(float)`, a FLOOR, so an array was read as +// element 0 and any dtype was reinterpreted (#1181). +using dense_loaders::ReadF32Scalar; // src bf16 [rows, cols] -> dst bf16 [cols, rows]. // @@ -324,8 +322,8 @@ float ReadF32Scalar(const StTensor& t) { // the running byte total of everything ahead of it, so a bf16 tensor that // follows an odd-length one starts on an odd byte and the typed pointer is // undefined to form or load through (issue #627). `vt::LoadUnaligned` is the -// project's seam for that — the same one `ReadF32Scalar` above open-codes with -// memcpy and `dense_loaders::TransposeBf16` already uses for this exact loop. +// project's seam for that — the same one `dense_loaders::ReadF32Scalar` uses +// and `dense_loaders::TransposeBf16` already uses for this exact loop. // The strided form. `src_pitch` is the distance in ELEMENTS between successive // source rows, which is `cols` for a dense 2-D tensor and something larger when // the block is a column-slice of a wider one (the stacked gate/up halves below). @@ -455,8 +453,8 @@ Fp8Weight LoadFp8Raw(const TensorResolver& get, const std::string& proj) { Fp8Weight r; r.n = w.shape[0]; r.k = w.shape[1]; - r.weight_scale = ReadF32Scalar(get(proj + ".weight_scale")); - r.input_scale = ReadF32Scalar(get(proj + ".input_scale")); + r.weight_scale = ReadF32Scalar(get, proj + ".weight_scale"); + r.input_scale = ReadF32Scalar(get, proj + ".input_scale"); r.alpha = r.input_scale * r.weight_scale; r.packed = MakeOwned(vt::DType::kI8, {r.n, r.k}); VT_CHECK(w.nbytes == r.packed.bytes.size(), @@ -477,7 +475,7 @@ OwnedTensor LoadFp8Transposed(const TensorResolver& get, "qwen3_5 weights: expected 2-D weight for " + proj); const int64_t out_dim = w.shape[0]; const int64_t in_dim = w.shape[1]; - const float scale = ReadF32Scalar(get(proj + ".weight_scale")); + const float scale = ReadF32Scalar(get, proj + ".weight_scale"); std::vector dq(static_cast(out_dim) * in_dim); DequantFp8ToBf16(w.data, scale, out_dim * in_dim, dq.data()); @@ -507,7 +505,7 @@ Nvfp4Weight LoadNvfp4Raw(const TensorResolver& get, const std::string& proj) { const StTensor& ws = get(proj + ".weight_scale"); VT_CHECK(ws.dtype == "F8_E4M3", "qwen3_5 weights: expected F8_E4M3 for " + proj + ".weight_scale"); - const float ws2 = ReadF32Scalar(get(proj + ".weight_scale_2")); + const float ws2 = ReadF32Scalar(get, proj + ".weight_scale_2"); Nvfp4Weight r; r.n = out_dim; From 0b51cfb0db215070f7a65d405fdff413ceaa7a90 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Tue, 18 Aug 2026 04:27:22 +0000 Subject: [PATCH 4/4] docs(FIX-READ-F32-SCALAR-GUARD): record the measured outcome, including the check that is NOT gated (#1181) RED before at `8cff76113` was 16 of 29 assertions and 6 of 6 refusal subcases, and eleven of those failures logged an EMPTY exception message, which is the finding: four wrong-scale shapes did not read the wrong element, they loaded and returned a complete model. GREEN after at `60aefbca6` is 29 of 29 focused, 67,845 of 67,845 for the file, 511 of 511 for ctest, and 81 of 81 preflight gates with no skip. Five mutations, each with its `compile_rc` and its applied diff stat, restored byte-identical. Two reachability mutations delete the production call sites in `LoadFp8Transposed` and `LoadNvfp4Raw` and both go red, so the guard is measured as reached from `LoadQwen3_5Moe` rather than as a class that works. The fifth mutation stayed GREEN and is recorded as such. Deleting the exact-byte-count check changes nothing the fixture can see, because the safetensors reader already rejects a header whose shape times dtype width disagrees with its span, so the count and dtype checks imply four bytes for any file that parses. The line is kept for its null-pointer half and named here as ungated rather than presented as tested. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code] --- .agents/specs/read-f32-scalar-guard.md | 88 +++++++++++++++++++++++++- 1 file changed, 87 insertions(+), 1 deletion(-) diff --git a/.agents/specs/read-f32-scalar-guard.md b/.agents/specs/read-f32-scalar-guard.md index 8d3a109b7..dbe80309f 100644 --- a/.agents/specs/read-f32-scalar-guard.md +++ b/.agents/specs/read-f32-scalar-guard.md @@ -273,6 +273,92 @@ only. No GPU lease, no checkpoint download. - Explicit narrow-dtype conversion for a one-element `BF16` or `F16` scale, if a checkpoint is ever measured that ships one. +## Outcome + +Measured on the CPU gate at `ab6e65216` plus this branch, `x86_64`, Ninja, +default `CMAKE_BUILD_TYPE`. + +### RED before, at commit `8cff76113` + +`test_qwen3_8_text_only`, one case, 29 assertions: **16 failed, 6 of 6 refusal +subcases failed**, the positive control passed. + +The SHAPE of that red is the finding, and it is stronger than the issue +predicted. Eleven of the sixteen failed assertions logged `message := ` with +nothing after it. A per-output-channel `weight_scale [8] F32`, a block-wise +grid `[2, 4] F32`, a per-output-channel `[8] BF16` and a multi-element +`weight_scale_2 [2] F32` did not merely read the wrong element. They LOADED, and +`LoadQwen3_5Moe` returned a complete model built on a number nobody wrote. + +The two one-element BF16 cases did stop, on +`vt: qwen3_5 weights: scalar tensor too small for f32 at +src/vllm/model_executor/models/qwen3_5_weights.cpp:313`. That message names no +tensor and describes a truncation rather than a dtype, so it cannot tell a +reader which of hundreds of scales was wrong. And it only fires because ONE bf16 +value is two bytes. Eight bf16 values are sixteen bytes, which is the layout +that is actually published, and the floor never saw it. + +### GREEN after, at commit `60aefbca6` + +- focused: `test_qwen3_8_text_only -tc="...REFUSED by name"`, 1 case, **29 of 29 + assertions**, `SUCCESS` +- whole file: 19 cases, **67,845 of 67,845 assertions**, `SUCCESS` +- full suite: `ctest --test-dir build -j 2`, **511 of 511 passed, 0 failed**, + 181 s, with `test_modelopt_mixed_precision_checkpoint` and `test_voxtral_e2e` + skipped for missing checkpoints as they are on an unmodified tree +- `scripts/agent-preflight.sh --fail-on-skip`: 81 gates, **81 ok, 0 FAIL, 0 + SKIP** + +### Mutations, each with its build status and its applied diff + +Run in place against a clean tree, restored with `git checkout --` and verified +by `sha256sum`. A mutation that fails to build and a mutation that never applied +both read as a passing test, so both are printed rather than assumed. + +| Mutation | diff stat | `compile_rc` | Result | Restore | +|---|---|---|---|---| +| R1 reachability: `LoadFp8Transposed`'s call site replaced by `1.0F` | 1 file, +1 -1 | 0 | RED, 11 of 29, the 4 `weight_scale` subcases | byte-identical | +| R2 reachability: `LoadNvfp4Raw`'s call site replaced by `1.0F` | 1 file, +1 -1 | 0 | RED, 5 of 29, the 2 `weight_scale_2` subcases | byte-identical | +| G1: the element-count check deleted | 1 file, +1 -4 | 0 | RED, 7 of 29, the 4 multi-element subcases | byte-identical | +| G2: the dtype check deleted | 1 file, -3 | 0 | RED, 2 of 29, the 2 one-element BF16 subcases | byte-identical | +| G3: the exact-byte-count check deleted | 1 file, -3 | 0 | **GREEN, 29 of 29** | byte-identical | + +R1 is the reachability proof the `Nothing lands dead` rule asks for. The guard +is reached from `vllm::LoadQwen3_5Moe` over a real synthetic checkpoint, and +`LoadFp8Transposed` rather than `LoadFp8Raw` is the arm a CPU build takes, +because `DenseNativeEnabled()` needs `VT_CUTLASS_FP8`. R2 proves the same +through a second, unrelated loader, which is what makes the guard shared rather +than local. + +### The negative result, recorded rather than quietly kept + +**G3 stayed green, so the exact-byte-count check is NOT independently gated.** +No fixture built by `BuildSafetensors` can reach it, because the safetensors +reader already rejects a header whose shape times dtype width does not equal its +data span, so `numel == 1` and `dtype == "F32"` together imply four bytes for +any file that parses. What the check still buys is the `t.data != nullptr` half, +against a default-constructed `StTensor`, which +`safetensors_reader.h:20-33` documents as a real state in tests. It is kept +because the in-tree reference `nemotron_h_weights.cpp:566-568` carries the same +check, and because a `memcpy` from `nullptr` is worse than a refusal. It is +recorded here as an ungated line rather than presented as a tested one. + +### What surprised us + +The issue framed this as latent, protected by #1166's name miss. The audit says +otherwise on two counts, and both are in the tree already rather than inferred. +`dense_weight_loaders.h:73-74` and `docs/BENCHMARKS.md:52` both record +`unsloth/Qwen3.6-27B-NVFP4` @`ccdaab7e` as FP8 W8A8 throughout with BF16 +per-output-channel scales, and `LoadAttnDense` branches on the weight dtype +alone, so that revision reaches both defects at once under the tensor name the +loader asked for. Nothing was misspelled and nothing stopped it. + +And the reader had SIX copies, not five. `ReadCtF32Scalar` carried the identical +defect under a different name, inside the shared header that exists to stop +exactly that, and it was reached from a sixth model file the issue's file list +did not name. A grep for one identifier measured five sixths of a class defect. + ## Now -`ACTIVE`. Spec committed before implementation. +`ACTIVE`, PR open, awaiting fresh review. Spec committed before implementation +at `30f704f6c`, red tests at `8cff76113`, guard at `60aefbca6`.