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..dbe80309f --- /dev/null +++ b/.agents/specs/read-f32-scalar-guard.md @@ -0,0 +1,364 @@ +# 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. + +## 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`, PR open, awaiting fresh review. Spec committed before implementation +at `30f704f6c`, red tests at `8cff76113`, guard at `60aefbca6`. 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; 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