diff --git a/.agents/issue-index.md b/.agents/issue-index.md index e0d4dae72..a62fb2282 100644 --- a/.agents/issue-index.md +++ b/.agents/issue-index.md @@ -307,9 +307,13 @@ rather than merged. `scripts/check-agent-record.py` gates both. | [#1066](https://github.com/mudler/vllm.cpp/issues/1066) | `ENG-EXPERT-STREAM` | `Qwen35ExpertStream` (`src/vllm/model_executor/models/qwen3_5.cpp`) is a **process-lifetime singleton** and keyed its slot cache on `(TowerId(base), expert)`, where `base` is the expert tower's host buffer **ADDRESS**. Its own comment stated the premise and drew the wrong conclusion: "A tower's identity is its base pointer, which is stable for the model's life". The premise is true; the conclusion does not follow, because the CACHE is not scoped to one model's life. Free a model, load another, and the allocator hands the new towers addresses the old ones held, so the new model's expert resolves to an entry filled from a DIFFERENT checkpoint — returned as a HIT, which by contract moves no bytes, so no counter moves and nothing downstream has anything to observe. MEASURED on two synthetic 4-layer/4-expert MoE models in one process, instrumenting `KqExpertSlice` to `memcmp` each returned slot against the tower slice it claims to be: **24 towers occupied 21 distinct addresses, and 20 of 222 slices returned another tower's bytes**; end to end the two arms disagreed on all 160 logits while each arm was internally deterministic (0 differing values on a repeat), which rules out nondeterminism. Invisible to every existing test of this row by construction, because all of them build the cache, store and streamer by hand and none runs two models through the production seam. Reachable by any process that loads a model, releases it, and loads another. Fixed by `OwnedTensor::TowerUid()`, a lazily assigned process-unique counter stamped on the tensor and re-stamped when `bytes` moves (so a copy cannot inherit an identity along with a different buffer); a counter cannot collide because it never goes backwards. Found and fixed while repairing the F1-F11 wiring review for [#912](https://github.com/mudler/vllm.cpp/issues/912). Spec [`expert-streaming.md`](specs/expert-streaming.md) | bug | | [#1083](https://github.com/mudler/vllm.cpp/issues/1083) | `MODEL-MUSIC-minimax-music3-mini-max-music3-for-conditional-generation` | `CleanCaption`'s italic unwrap emulated upstream's `(?!\*)` with a CAPTURED `($|[^*])` (`src/vllm/model_executor/models/minimax_music3_ar.cpp:85,114` @ `a332fb98d`), and a captured group is not a zero-width assertion: consuming the trailing neighbour advanced `regex_replace` past it, so an emphasis span opening within ONE character of the previous close was never examined and the surviving asterisks re-paired ACROSS the intended spans. `*a* *b* *c*` -> `a *b c*` where `_clean_caption` (`encoders.py:72` @ diffusers `c6da9936`) gives `a b c`, and `Warm *lo-fi* *jazzy* keys with a *soft* *brushed* snare` -> `Warm lo-fi *jazzy keys with a soft brushed* snare` — a re-association, not a leftover marker, so the caption handed to the tokenizer is a string upstream would never emit. `encoders.py`'s own header states that whitespace-level prompt changes change the generated audio, so this is a checkpoint-contract break. Invisible to the gate because `markdown_and_tags`, the only golden with italics, carries ONE span per line and the defect needs adjacency. FIXED by porting the trailing side LITERALLY as `(?!\*)` — std::regex's ECMAScript grammar has negative lookahead though not lookbehind, so only the leading `(? 0)` in `tests/vllm/model_executor/test_expert_stream_wiring.cpp` was not load-bearing: reinjecting the pre-fix unaligned `madvise` address exits 0 in 40 of 40 runs, because `> 0` over 48 calls is satisfied whenever heap layout page-aligns a single slice — measured `advised=1` against `fills=48`. Tightened to `advised == fills`, which is the true healthy invariant on this arm (madvise runs on the mapping-copy path only, and only on a non-resident key, which is exactly when `EnsureSpan` goes on to fill) and which was verified stable over 50 consecutive runs. **(3)** "Every MoE entry point funnels through here exactly once per forward" was false: `Qwen3_5Model::ForwardDense`, `Qwen3_5MTPModel::Forward`, `Qwen3_5MTPModel::ForwardPaged` and `Qwen3_5ReplayLayer` all reach `ExpertMlpKq -> KqExpertSlice` and marked no step. ONE of them, `Qwen3_5MTPModel::ForwardPaged`, is the production spec-decode DRAFT forward, so draft acquisitions stayed `protected_this_step` across the following target forward; the other three are parity-only entry points and their guards land unreached, which is [#1108](https://github.com/mudler/vllm.cpp/issues/1108). All four now carry the guard — one forward is one step, including a draft, because a draft is a complete forward whose slices are finished with when it returns — and the guard refuses to NEST, so adding one cannot double-advance the hotness clock. `RunMoeBlock` stays deliberately unguarded: it is one block, and qwen3_moe.cpp owns the boundary for the model that composes it. **(4)** `ExpertStreamer::EnsureFile`, the arm every real GGUF-mmap checkpoint takes, was reached by no test, so the `file_offset + offset` composition was unverified; a CPU-local case now drives it from a temp file at a deliberately awkward offset and proves the arm by `advised` staying flat while `fills` grows. **(5)** `OwnedTensor::TowerUid`'s comment claimed identity for "this tensor's CURRENT bytes" while the code keys on `bytes.data()`; the comment now states the address limit and a borrowed-buffer case pins both halves, because [#1066](https://github.com/mudler/vllm.cpp/issues/1066) was that same overclaim on that same field. **(6)** `SetForceFallback` has no production caller and incremented the operator-facing `exhausted_`, telling an operator to raise a budget that was never the reason (measured `exhausted=42` from the test switch alone); it now has its own counter. All six fixed in flow, each mutation-proven: 13 mutations, 13 caught, every one with a non-empty `git diff --stat`, a zero compile status and a non-zero doctest case count. Spec [`expert-streaming.md`](specs/expert-streaming.md) | bug | | [#1073](https://github.com/mudler/vllm.cpp/issues/1073) | `FIX-NAS-PATH-1073` | The NAS moved to `/usr/local/nas_share` and `/mnt/nas_share` is gone, so every tracked default built on `/mnt` broke. `/mnt` is the EPHEMERAL root overlay of the gate box's immutable Kairos OS and does not survive a reboot; `/usr/local` is `COS_PERSISTENT` and does. Observed 2026-08-16 after an 8 h 19 min outage: the mount came back because the `/oem` boot-stage unit worked, `/mnt/nas_share` did not, and the untracked `.env` still declared `CHECKPOINT_ROOT=/mnt/nas_share/checkpoints` — a gate that reads a path `.env` does not declare is not the gate its spec names. `.agents/environment.md` documented NO NAS location at all (measured: the file held no `/mnt` string), so the repair adds the path AND the `COS_PERSISTENT` reason, because a bare path correction invites the next reader to restore the dead location as a symlink. The seven live defaults now derive from `CHECKPOINT_ROOT`, which four sibling scripts already did: `scripts/gen-minimax-music3-manifest.py:17`, `scripts/gen-ltx2-quant-goldens.py:48`, `tools/parity/dump_tokenizer_gpt4o.py:36,39,57`, `tools/gen_pretok_goldens.py:57`, `src/vllm/tokenizer/pretokenizer.cpp:319`, `tests/parity/test_minimax_music3_quant_real.cpp:133,144` and `docs/USAGE.md:3069,3453`. The 41 hits were classified before any edit and the records that cite the old path KEEP it: `.agents/benchmark-record.md`, the LTX-2.5/Nemotron-H specs, `.agents/model-matrix.md`, the captured goldens and the generated `.inc` headers state where a past measurement read its bytes, which is provenance. Spec [`nas-mount-path.md`](specs/nas-mount-path.md) | bug | | [#1077](https://github.com/mudler/vllm.cpp/issues/1077) | — | `.env.example:37`, `.agents/environment.md:29` and `tests/vllm/multimodal/test_ltx2_video.cpp:2128-2132` each state that nothing in the tree reads `CHECKPOINT_ROOT`, and six gates read it: `tests/parity/test_minimax_music3_ar_real.cpp:162`, `_e2e_real.cpp:170`, `_llm_real.cpp:137`, `_quant_real.cpp:130,140`, `tests/vllm/models/test_ltx2_text_encoder.cpp:2299`, and `test_nemotron_h_loader.cpp:161` tells the reader to export it. No product code under `src/` or `include/` reads it, so the accurate statement is that the LIBRARY never reads it while several gates do. It costs more than tidiness: `test_ltx2_video.cpp` reasons FROM the claim when it chooses a separate `LTX2_CHECKPOINT_ROOT` ("this would be its first reader"), and that reasoning is void. Found while repairing [#1073](https://github.com/mudler/vllm.cpp/issues/1073) and NOT fixed there, because reversing a design decision needs its own review rather than a path substitution. Listed under `## Owed` in [`nas-mount-path.md`](specs/nas-mount-path.md) | bug | | [#1079](https://github.com/mudler/vllm.cpp/issues/1079) | `FIX-NAS-PATH-1073` | All four skip messages in `tests/parity/test_minimax_music3_quant_real.cpp` streamed the case name as a `const char*`, and doctest 2.5.2 stringifies that through its bool overload, so every one printed `SKIP 1` and named no case. The comment above the helpers states the obligation the messages then failed: a gate that silently passes when its asset is absent has not reported. It matters here because the binary reports `6 passed` with `assertions: 0` when the checkpoint is absent, so the message text is all that separates a skipped run from a gated one. Pre-existing on `main` at `100026481`. FIXED IN FLOW while landing [#1073](https://github.com/mudler/vllm.cpp/issues/1073), which rewrote those exact messages and would have carried the defect forward under a changed line; the fix streams `std::string(what)`. Scope measured before fixing: 4 hits, all in this one file | bug | +| [#1106](https://github.com/mudler/vllm.cpp/issues/1106) | `ENG-EXPERT-STREAM` | A fresh review of the [#1091](https://github.com/mudler/vllm.cpp/issues/1091) repair ([#1100](https://github.com/mudler/vllm.cpp/pull/1100) @ `3da7b4ca2`) returned FAIL on four findings. The six functional repairs are correct and all 13 mutation claims reproduce independently; what failed is what was SAID about them. **(1)** `qwen3_5_internal.h:421-424` stated that the final statistics line is reached "at process teardown: a static registered the first time streaming is requested, plus the store's own destructor" — there is no such static, `grep -rn 'atexit\|quick_exit'` over `qwen3_5.cpp` returns nothing, and #1100's own body says the hook was deliberately not built; the comment also dropped both qualifiers `docs/USAGE.md` carries (a store must have been BUILT, and static destructors must RUN). Verbatim the defect #1091 finding 5 reports about `TowerUid`, reintroduced one file away in the change that fixes it. **(2)** "Nothing lands dead" was claimed for four step guards and holds for ONE: only `Qwen3_5MTPModel::ForwardPaged` has a production caller (`runner.cpp:2183` -> `spec_decode/mtp/speculator.cpp:107,262`); `Qwen3_5MTPModel::Forward`, `Qwen3_5Model::ForwardDense` and `Qwen3_5ReplayLayer` are parity-only entry points whose every caller is under `tests/`, and per `.agents/reachability.md` a call site inside a test is not reach. The guards are correct where they sit, so the code is unchanged and the CLAIM is; the residual is [#1108](https://github.com/mudler/vllm.cpp/issues/1108). **(3)** The nesting refusal (`qwen3_5.cpp:5517`) was asserted in the source, the spec and the pull request body and pinned by nothing: deleting its `VT_CHECK` left both focused binaries fully GREEN (6/6 and 4/4, rc 0) and it appeared in none of the 13 mutations. Unreachable through production code by construction — every forward that takes expert slices is a complete forward that no other one contains — so `detail::ExpertStreamStepScope` exposes the guard's own `Begin`/`End` and a case asserts the refusal twice: a second scope throws, AND a real `ForwardDense` entered while the scope is held throws too, which is what proves the two share a boundary rather than agreeing by coincidence. Kept UNGATED on `Qwen35ExpertStreamRequested()` on purpose: one forward is one step is a property of the call graph, not of the streaming lane, and arming it only under the rare configuration would let the default path establish a nest nobody sees until streaming is switched on. **(4)** `::setenv` sat at namespace scope in both new gates with no `_WIN32` guard; it is POSIX, MSVC's CRT has only `_putenv_s`, `tests/CMakeLists.txt:1087` adds the target unconditionally and `scripts/build-windows-release.ps1` configures `VLLM_CPP_BUILD_TESTS=ON`, so neither translation unit compiled there and the file comment claiming the step-clock cases are "built everywhere" was false. Both now use `vllm_test::SetEnv` from `tests/support/test_env.h`, the shim [#603](https://github.com/mudler/vllm.cpp/issues/603) landed for exactly this. CI could not report it: the Windows lanes fail earlier, inside the product library, on [#1068](https://github.com/mudler/vllm.cpp/issues/1068), and a lane that never reaches a test TU cannot fail in one. All four fixed in flow; three mutations on the added guarantee, three caught, each with a changed sha256, a zero compile status and a non-zero doctest case count. Spec [`expert-streaming.md`](specs/expert-streaming.md) | bug | +| [#1107](https://github.com/mudler/vllm.cpp/issues/1107) | `ENG-RELEASE-WINDOWS` | `scripts/check-windows-portability.py` exits 0 on a tree carrying an unguarded `::setenv` in a test translation unit — measured `Windows portability contract OK`, rc 0, on the unrepaired `test_expert_stream_steps.cpp`. It misses the class twice over, and either miss alone is enough. SCOPE: `check()` builds its `texts` map from `shipped_server_sources(...)`, the sources reachable from the shipped SERVER target, so no file under `tests/` is read for ANY of its rules. VOCABULARY: `POSIX_PATTERNS` names `fork|execvp|waitpid|pipe|read|write|open|close|fsync|pread|pwrite|getpid|stat` plus the `unistd.h`-family includes, and neither `setenv` nor `unsetenv` appears — so even inside the scanned set the call would pass. The second miss is the one that generalises: the three private `_putenv_s` copies [#603](https://github.com/mudler/vllm.cpp/issues/603) records, and the shim it landed, all exist because this is the recurring call, and the checker holding the Windows contract does not know its name. Invisible in CI because `windows-msvc-*` are PR-only with no `main` baseline ([#584](https://github.com/mudler/vllm.cpp/issues/584)) and are currently red inside the product library on [#1068](https://github.com/mudler/vllm.cpp/issues/1068), so a lane failing before it reaches a test TU cannot report a new test-TU failure; the static checker was the only instrument that could have. Found while repairing [#1106](https://github.com/mudler/vllm.cpp/issues/1106) finding 4 and NOT fixed there: a checker semantic change needs its own spec, red-before test and green-after evidence, and widening the scan to `tests/` has to separate a guarded POSIX call from an unguarded one across a large surface, which wants measurement rather than a guess | bug | +| [#1108](https://github.com/mudler/vllm.cpp/issues/1108) | `ENG-EXPERT-STREAM` | Three of the four `Qwen35ExpertStreamStep` guards [#1091](https://github.com/mudler/vllm.cpp/issues/1091) finding 3 added land UNREACHED. Only `Qwen3_5MTPModel::ForwardPaged` is reachable from a production entry point (`src/vllm/v1/worker/gpu/runner.cpp:2183` -> `src/vllm/v1/worker/gpu/spec_decode/mtp/speculator.cpp:107,262`). `Qwen3_5MTPModel::Forward` is reached only through `ForwardLogitsHost`, which `qwen3_5_mtp.h:135` documents as "standalone parity convenience" and which itself has no caller outside `tests/`; `Qwen3_5Model::ForwardDense` is the parity reference by `qwen3_5.h:234` (callers `tests/parity/test_op_parity.cpp:1107`, `tests/vllm/v1/worker/test_runner.cpp:1278`, `tests/vllm/models/test_qwen35_paged_forward.cpp:293,320,385,403`); `Qwen3_5ReplayLayer` is per-layer parity replay by `qwen3_5.h:322` (only caller `tests/parity/test_op_parity.cpp:1050`). Per `.agents/reachability.md` a call site inside a test is not reach, so "every added path is reached from a production entry point at this commit" was true of one guard in four. NOTHING IS DELETED: the guards are correct where they sit, cost nothing, and become live the moment any of those entry points gains a production caller — and adding the guard later WITH the caller is precisely how this row lost its step boundary in the first place. What is owed is the record, so this is named as a staged slice that lands unreached rather than claimed as reached. Closes when one of those entry points gains a production caller, or when they are retired as parity references; neither is scheduled. Split out of [#1106](https://github.com/mudler/vllm.cpp/issues/1106) finding 2 so the debt stays open after that repair closes. Listed under `## Owed` in [`expert-streaming.md`](specs/expert-streaming.md) | bug | | [#941](https://github.com/mudler/vllm.cpp/issues/941) | `MODEL-TEXT-nemotron-h-nemotron-hfor-causal-lm` | `.agents/specs/nemotron-h-abi-e2e.md` §2 named `dense_attn::AttnBlock` as NemotronH's device attention seam, and that block cannot serve this architecture: it applies `vt::RopeNeox` unconditionally (`include/vllm/model_executor/models/dense_attn_block.h:497` @ `10002648199` — the `:496` cited in-tree at `nemotron_h_device.cpp:56` is a comment line, so the anchor is stale as well as the claim), while Nemotron-H has NO RoPE at all: a case-insensitive grep for `rotary`, `rope`, `q_norm`, `k_norm` over `vllm/model_executor/models/nemotron_h.py` at the pinned oracle `5559679229bc` returns ZERO hits, and its four-line attention forward (`:474-483`) sends `q` and `k` straight into `self.attn` with no positional transform. It also reads `cfg.rms_norm_eps`, which `src/vllm/transformers_utils/hf_config.cpp:551` defaults to `0.0` for a checkpoint that ships `layer_norm_epsilon` and `norm_eps` and no `rms_norm_eps`. There is NO rope-free entry point: the header's whole public surface is enumerated in the A2-P spec §2.3, `:490-493` selects only WHICH rope implementation, and `rotary_dim == 0` ABORTS at `src/vt/ops.cpp:1427-1429` rather than bypassing — so neither failure mode announces itself as one. Same shape as [#810](https://github.com/mudler/vllm.cpp/issues/810): a shared function reconstructing behaviour from HF-config fields the model does not ship, and two of its three failure modes are silent. PARTLY FIXED IN FLOW by the A2-P spec: item 1 (correct the seam claim so no later implementer is sent that way) is answered by [`specs/nemotron-h-a2p-paged-forward.md`](specs/nemotron-h-a2p-paged-forward.md) §2.3 plus the correction block appended to the governing spec's §1; item 2 (a model-local block in the `granite.cpp:84` / `gemma4.cpp:206` idiom) was already answered by A2-R at `598226e96` and is extended by A2-P. STILL OPEN and deliberately not fixed here: item 3, whether defaulting `rms_norm_eps` to `0.0` rather than refusing is right in general — that is tree-wide, needs its own red-before, and is listed under `## Owed` in the A2-P spec | bug | | [#1093](https://github.com/mudler/vllm.cpp/issues/1093) | `ROAD-V1-LTX25` | `TI2VidTwoStagesPipeline` (`ti2vid_two_stages.py:61` @ `fd4ded7f`) has no recipe row, no refusal and no `Ltx2UnportedPipelineFeature` marker, so asking for it gets the generic table refusal (`ltx2_pipeline.cpp:1328-1332`) naming the pair rather than the missing machinery. It is NOT the `distilled_two_stage` we ship: stage 1 is CFG-guided on the FULL model (`ti2vid_two_stages.py:247-259`) where `distilled.py:265-266` uses `SimpleDenoiser`; the distilled LoRA rides stage 2 ALONE (`:151`); stage-1 sigmas are scheduler-derived (`:243-245`) against our fixed `DistilledSigmas()` (`ltx2_pipeline.cpp:1163`); and `distilled.py:94-107` has no `distilled_lora` parameter at all. It is also NOT `TI2VidTwoStagesHQPipeline` (`ti2vid_two_stages_hq.py:59`), which [#921](https://github.com/mudler/vllm.cpp/issues/921) owns and which puts the LoRA on BOTH stages at separate strengths. Blocked on (a) a guided VIDEO denoise loop: the guidance arithmetic IS ported and generic (`ltx2_pipeline.cpp:439-524`) but its only consumer is the audio-only T2A loop (`ltx2_t2a.cpp:367`, `video=nullptr` at `:332-334`), and the joint loop is single-forward (`ltx2_video.cpp:3036-3040`); (b) TWO checkpoints absent from the NAS, `ltx-2.5-22b-distilled-lora-450-bf16.safetensors` (`--distilled-lora` is `required=True`, `utils/args.py:1146`) and the full `ltx-2.5-22b-dev-transformer-bf16.safetensors` stage 1 runs. Control: `find /mnt/nas_share/checkpoints -iname '*lora*'` returns nothing while `-name '*.safetensors'` returns the 8 LTX-2.5 files we hold, every transformer among them a `-distilled-` build. Side finding: `one_stage` writes `video_guidance` at `ltx2_pipeline.cpp:1069` and nothing reads it. Already under `## Owed` in [`ltx25-resolution-envelope.md`](specs/ltx25-resolution-envelope.md) as "not separately filed"; filed now because an umbrella row cannot say what THIS arm is blocked on | feature | | [#1094](https://github.com/mudler/vllm.cpp/issues/1094) | `ROAD-V1-LTX25` | `HDRICLoraPipeline` (`hdr_ic_lora.py:229` @ `fd4ded7f`) is absent with no refusal and no `Ltx2UnportedPipelineFeature` marker. It is the only upstream pipeline returning LINEAR HDR rather than display-referred pixels, so the gap is colour science and not only scheduling. Five citations exist outside `.agents/` and none is an implementation: comments at `ltx2_lora.h:168-169`, `ltx2_pipeline.h:588`, `test_ltx2_lora.cpp:481`, `test_ltx2_video.cpp:542`, plus one string literal inside a `Fail(...)` argument at `ltx2_lora.cpp:246`; `git grep -i HDRICLora` returns zero hits tree-wide. Blocked on (a) a LogC3 / ACEScct decode tail: upstream `ltx-core/hdr.py:37-43` (ARRI EI-800 constants), `:53-65` (compress/decompress), `:82-84` (`HDRTransfer`), `:140-172` (`to_linear`, `to_hdr_linear`), applied at `hdr_ic_lora.py:624` and selected from the adapter's own safetensors metadata (`:178-209`). `git grep -i "logc3|HDRTransfer|to_hdr_linear|acescct"` returns zero product-code hits here, with `git grep -i yuv` as the control (live ffmpeg argv at `minimax_h3_mux.cpp:80`), and the exclusion is already deliberate: [`ltx25-retire-dead-arms.md`](specs/ltx25-retire-dead-arms.md):167 classifies scene-linear HDR colour as "no - colour science". (b) TWO artifacts not on the NAS: `--hdr-lora` (`required=True`, `:833`) from repo `Lightricks/LTX-2.3-22b-IC-LoRA-HDR`, which upstream names only by repo, and `--text-embeddings` (`:834`), since this pipeline loads no text encoder at all (`:275-281`). (c) Per-phase stage-2 tiling and IC-LoRA toggles (`:102-104`, consumed `:485-500`), which our fixed two-phase recipe shape cannot express | feature | diff --git a/.agents/specs/expert-streaming.md b/.agents/specs/expert-streaming.md index 8e178e94d..807cc7544 100644 --- a/.agents/specs/expert-streaming.md +++ b/.agents/specs/expert-streaming.md @@ -1184,6 +1184,182 @@ funnels through exactly once per forward, as an RAII guard so a step that ends b throwing still ends. `qwen3_moe.cpp` composes the same MoE block from another translation unit and marks its own step for the same reason. +## The observability review (#1091): the instrument could not report its own defect + +17 August 2026. A fresh review of the F1-F11 repair above returned FAIL on six +findings. None of them was a red test. Every one was a gap in what the gate could +see, which is the same class as the defect the repair had just fixed. + +**The statistics line could not print the number the docs told an operator to +read.** `ReportStats` had exactly one caller, `EndStep`, and it returned early on +`steps == 0`. So a run whose step boundary is never reached — F1, the reason the +line exists — printed nothing at all. Measured on one binary with +`VT_MOE_EXPERT_STREAM_STATS_EVERY=1`: healthy, 8 lines; F1 reinjected, 0 lines +and only the startup banner. `docs/ENVIRONMENT.md` and `docs/USAGE.md` both told +the operator to read `steps == 0` off a line that could not exist, and the +**absence** was the signature. + +A second consequence found while acting on it: `stats_every_` defaults to 16, so +a short healthy run prints nothing either. A benchmark that reads absence as +failure therefore reports VOID on a working lane, which is what happened to the +streaming benchmark and is why it had to be restarted. + +The repair is one final line, printed from the store's own destructor, once per +process, crossing both early returns. Not a second teardown hook registered when +streaming is REQUESTED, which was the first shape tried: on a CPU-only host that +hook's only unique job — a run that asks for streaming and never builds a store — +is not reachable by any test, because `Reserve` and `Get` are called from the same +call chain. It would have been an untestable branch added to fix an +untestable-branch problem. What replaces it is a protocol the docs now state: +the `[expert-stream] ON ...` banner says a store was built, the final line says +what it did, and each combination of present/absent means one thing. The docs +carry four rather than three: an in-process flush through the exposed seam takes +the once-flag, so a gate that calls it leaves the teardown line absent, and only +a gate can produce that shape (#1106). + +**`CHECK(s.advised > 0)` could not fail for the defect it named.** Reinjecting the +pre-fix unaligned `madvise` address exits 0 in 40 of 40 runs. The measured reason +is that `> 0` over 48 calls is satisfied whenever heap layout happens to +page-align a single slice, and one did: `advised=1` against `fills=48`. The +assertion is now `advised == fills`, which is the true healthy invariant on this +arm — madvise is issued on the mapping-copy path only, and only when the key is +not already resident, which is exactly the condition under which `EnsureSpan` +goes on to fill, with `exhausted == 0` asserted beside it as the premise. Verified +stable over 50 consecutive runs before being asserted, rather than after. + +**"Every MoE entry point funnels through here exactly once per forward" was +false.** Four more forwards reach `ExpertMlpKq -> KqExpertSlice`: +`Qwen3_5Model::ForwardDense`, `Qwen3_5MTPModel::Forward`, +`Qwen3_5MTPModel::ForwardPaged` and `Qwen3_5ReplayLayer`. One of them, +`Qwen3_5MTPModel::ForwardPaged`, is the production spec-decode DRAFT forward, so +a draft's acquisitions stayed `protected_this_step` across the following target +forward — F1 at draft scale. This paragraph said "the MTP pair" until #1106 +finding 2 measured it; the other three are parity-only entry points, and that is +recorded below and under `## Owed` as #1108. + +ONE FORWARD IS ONE STEP, and that is the call the draft forced. Folding a draft +into the target's step would pin the draft's slots across a second forward for no +benefit, so each draft gets its own step and a spec-decode iteration advances the +clock once per draft plus once for the target. The opposite mistake is the one +adding guards invites — a guard nested inside another ends the step twice, which +decays every resident entry an extra tick for a step that never happened — so the +guard now REFUSES to nest, stated as a precondition in the same idiom +`MatmulF32Slice` uses for `expert >= 0` rather than handled. That refusal was +STATED here and pinned by nothing, which the next review measured; see #1106 +below. `RunMoeBlock` stays +deliberately unguarded: it is one block, not a forward, and qwen3_moe.cpp owns +the boundary for the model that composes it. That exemption is what makes the +`steps == 0` case above constructible without breaking anything. + +**Three more, smaller.** `EnsureFile` — the arm every real GGUF-mmap checkpoint +takes — was reached by no test, so the `file_offset + offset` composition was +unverified; it is now driven from a temp file at a deliberately awkward offset +(4109 bytes: past a page, not on a page, not on a 34-byte Q8_0 block), and the +arm is PROVEN rather than assumed by `advised` staying flat while `fills` grows, +which is the one number that separates a pread from an `EnsureSpan`. +`OwnedTensor::TowerUid`'s comment promised an identity for "this tensor's CURRENT +bytes" while the code keys on `bytes.data()`; the comment now states where the +guarantee stops, and a borrowed-buffer case pins both halves, because #1066 was +that same overclaim on that same field. `SetForceFallback` has no production +caller and was incrementing the operator-facing `exhausted_`, so a gate asking for +the unstreamed arm told an operator to raise a budget that was never the reason +(measured: `exhausted=42` from the switch alone); it has its own counter now, kept +off the stderr line because in a production process it is always zero. + +Thirteen mutations, thirteen caught, each recorded with a non-empty +`git diff --stat`, a zero compile status and a non-zero doctest case count. The +first attempt at two of them was INVALID rather than passing — one did not build +(`-Werror` on an unused variable), and two reported the CHILD process's doctest +summary because a failing case dumps the child's output into the parent's log, +so the first `test cases:` match in the file belonged to the child. + +## The review of that repair (#1106): three claims outran the code + +17 August 2026. A fresh review of the pull request above returned FAIL. The six +functional repairs are correct and all thirteen mutation claims reproduce +independently. What failed is what was said about them, and three of the four +findings are the class that pull request was fixing. + +**The comment asserted a mechanism that does not exist.** +`qwen3_5_internal.h` said the final line is reached "at process teardown: a +static registered the first time streaming is requested, plus the store's own +destructor, whichever runs first". There is no such static — the same pull +request says in its own body that the hook was deliberately not built, and the +grep for `atexit` returns nothing. It also claimed "exactly one line per +process, even on a run with zero steps" without the two qualifiers `docs/USAGE.md` +carries: a store must have been BUILT, and the process must RUN its static +destructors. This is #1091 finding 5 — a comment promising more than the code — +reintroduced one file away in the change that fixes it, which is the strongest +argument on record that the class is a habit rather than an accident. +`~Qwen35ExpertStream` is now named as the only production path to the LINE, with +both qualifiers, and the note says what calling the exposed seam costs: it takes +the once-flag, so it suppresses the teardown line for the rest of the process. +The header says that `ExpertStreamFlushStats` itself has ZERO production callers +and that the destructor does not route through it, because the first repair of +this finding headed that comment "`~Qwen35ExpertStream` IS THE ONLY PRODUCTION +CALLER" — true of the line, and read as a call that the destructor deliberately +does not make. + +**"Nothing lands dead" was claimed for four step guards and holds for one.** +Only `Qwen3_5MTPModel::ForwardPaged` has a production caller +(`runner.cpp:2183` -> `spec_decode/mtp/speculator.cpp:107,262`). The other three +sit in parity-only entry points: `Qwen3_5MTPModel::Forward` is reached only +through `ForwardLogitsHost`, itself a "standalone parity convenience" with no +caller outside `tests/`; `Qwen3_5Model::ForwardDense` is the parity reference by +its own header; `Qwen3_5ReplayLayer` is per-layer parity replay. A call site +inside a test is not reach. Nothing is deleted — the guards are correct where +they sit and become live the moment any of those entry points gains a production +caller, and adding the guard later WITH the caller is exactly how this row lost +its step boundary the first time. What changes is the record: they are named as a +staged slice that lands unreached, in the commit body, in the pull request body +and under `## Owed` below, tracked as #1108. + +**The nesting refusal was asserted everywhere and pinned nowhere.** The source, +the spec and the pull request body all stated that the guard refuses to nest. +Deleting its `VT_CHECK` left both focused binaries fully green — 6/6 and 4/4 — +and it appeared in none of the thirteen mutations. It is unreachable through +production code by construction: every forward that takes expert slices is a +complete forward that no other one contains, so no legitimate call graph nests +one. `detail::ExpertStreamStepScope` exists for that and nothing else, and it +forwards to the guard's own `Begin`/`End` rather than restating the flag, so a +gate holding it measures the production boundary. The case asserts the refusal +twice: a second scope throws, AND a real `ForwardDense` entered while the scope +is held throws too — the second is what proves the two share a boundary rather +than agreeing by coincidence, and a mutation that gives the scope a parallel flag +kills only that pair. + +The refusal is deliberately NOT gated on `Qwen35ExpertStreamRequested()`. "One +forward is one step" is a property of the call graph, not of the streaming lane, +so a nest is a defect whether or not a store exists. Arming it only under +streaming — the rare configuration — would let the default path establish a nest +that nobody sees until someone turns streaming on, which is this row's recurring +shape. The cost is that a nest reds every Qwen3.5 forward and not merely the +streamed ones, and that is the intended polarity. + +**The MSVC repair was incomplete.** `::setenv` sat at namespace scope in both new +gates with no `_WIN32` guard. It is POSIX; MSVC's CRT has only `_putenv_s`, the +targets are added unconditionally, and `build-windows-release.ps1` configures +`VLLM_CPP_BUILD_TESTS=ON` — so the translation units did not compile there at +all, and the claim that the step-clock cases are "built everywhere" was false. +Both now use `vllm_test::SetEnv` from `support/test_env.h`, which has been the +one place that branch lives since #603. CI could not report it because the +Windows lanes fail earlier, inside the product library, on #1068; a lane that +never reaches a test translation unit cannot fail in one. The static checker that +could have is blind to it twice over — it scans only the shipped-server sources +and knows neither `setenv` nor `unsetenv` — filed as #1107 against +`ENG-RELEASE-WINDOWS` and not fixed here, because changing a checker's semantics +needs its own spec and red-before evidence. + +Three mutations on the added guarantee, three caught, each with a changed sha256, +a zero compile status and a non-zero doctest case count: deleting the `VT_CHECK` +(7 cases, 1 failed, all six assertions of the new case red, and `Steps()` reading +3 where 1 is correct — the double-count the guard exists to stop); dropping +`Open() = false` from `End` (7 cases, 4 failed); and giving the scope its own +parallel flag (7 cases, 1 failed, exactly the two assertions that pin the shared +boundary). The Windows repair is NOT mutation-proven: no MSVC is reachable from +this host, and the checker that would have caught it statically is the subject of +#1107. + ## Owed Carried debt for this row. Each item names why it is not closed here. @@ -1191,7 +1367,11 @@ Carried debt for this row. Each item names why it is not closed here. | Owed | Why it is open | |---|---| | **Re-measure decode on a LIVE cache.** The `docs/BENCHMARKS.md` decode figure for this row was taken with the step clock dead from token 3 onward and is void. | Needs `dgx.casa` and the 370 GiB checkpoint. The box was unreachable for this repair (`No route to host`), and this host has no CUDA device and cannot hold the model. | -| **The `pread` path has never run on the model.** `EnsureFile` is gated by unit tests only. | Same host. Three earlier attempts were OOM-killed at 48.6 GiB anon beside another session's 32.6 GiB job. | +| **The `pread` path has never run on the model.** `EnsureFile` now has a CPU-local gate that drives it through the production seam from a temp file and proves the `file_offset + offset` composition (#1091 finding 4), so it is no longer UNREACHED. It is still unmeasured on a real checkpoint. | Same host. Three earlier attempts were OOM-killed at 48.6 GiB anon beside another session's 32.6 GiB job. | +| **A run that REQUESTS streaming and never builds a store prints no statistics line.** The `[expert-stream] ON ...` banner is absent in that case too, so no-banner means "nothing reached the lane" and banner-without-line means "the process died"; the docs state all four shapes. | A teardown hook that could report it is not reachable from any test on a CPU-only host, because `Reserve` and `Get` sit in one call chain and a device platform is what separates them. Landing it would have been an untestable branch added to fix an untestable-branch problem. Needs `dgx.casa` (see #1091). There is NO such hook in the tree: `~Qwen35ExpertStream` is the only production path to the final line, and it prints it directly rather than through `ExpertStreamFlushStats`, which has no production caller at all. The header now says both, rather than describing the hook that was rejected (#1106). | +| **Three of the four step guards land UNREACHED.** `Qwen3_5MTPModel::Forward`, `Qwen3_5Model::ForwardDense` and `Qwen3_5ReplayLayer` are parity-only entry points with no caller outside `tests/`, so their `Qwen35ExpertStreamStep` guard is reached by no production path. Only `Qwen3_5MTPModel::ForwardPaged` is (`runner.cpp:2183` -> `spec_decode/mtp/speculator.cpp:107,262`), and even that caller is "UNREACHABLE unless a speculator is configured" (`runner.cpp:2120`) — so a DEFAULT-configuration run reaches none of the four, which is a weaker statement than "one of four is reached" and is recorded here rather than rounded up. Owning row `ENG-EXPERT-STREAM`; tracked as [#1108](https://github.com/mudler/vllm.cpp/issues/1108). | Nothing is deleted, because the guards are correct where they sit and cost nothing, and the alternative — add the guard later, together with the caller — is precisely how this row lost its step boundary in the first place. It closes when one of those entry points gains a production caller, or when they are retired as parity references. Neither is scheduled and neither should be forced by the record. | +| **`check-windows-portability.py` cannot see this class.** It scans only the sources reachable from the shipped server target, so no test translation unit at all, and `setenv`/`unsetenv` are in none of its patterns. Tracked as [#1107](https://github.com/mudler/vllm.cpp/issues/1107) against `ENG-RELEASE-WINDOWS`. | Changing a checker's semantics needs its own spec, a red-before test and green-after evidence, which is a different unit of work from repairing two test files. Widening the scan to `tests/` also has to separate a guarded POSIX call from an unguarded one across a large surface, and that wants measurement rather than a guess. | +| **The Windows repair is not mutation-proven.** Both gates now use `vllm_test::SetEnv`, and nothing here executed an MSVC compile of them. | No MSVC is reachable from this host, and the Windows CI lanes fail earlier in the product library on #1068, so they cannot report a test translation unit either way. The static checker that could have is #1107. | | **The `MADV_WILLNEED` readahead is unmeasured.** It is now well formed and counted; whether it moves decode is unknown. | Same host and the same OOM contention. No speedup is claimed anywhere for it. | | **Windows has no streaming.** `EnsureFile` throws `"EnsureFile needs pread"` on `_WIN32`, and `SourceOfSpan` returns `fd = -1` there, so the lane falls back to the mapping copy. | No `pread(2)`; needs an `OVERLAPPED`/`ReadFile` arm. Refused by name rather than silently degraded. | | **The CUDA arms of [#1029](https://github.com/mudler/vllm.cpp/issues/1029)'s grouped gate have not run on a device.** | Recorded in that issue, which stays open for it. Unchanged by this repair. | diff --git a/docs/ENVIRONMENT.md b/docs/ENVIRONMENT.md index a496c30b9..86563b855 100644 --- a/docs/ENVIRONMENT.md +++ b/docs/ENVIRONMENT.md @@ -79,7 +79,7 @@ allocated up front and never grown — the engine prints the resolved values as | `VT_MOE_EXPERT_STREAM` | off | `=1` serves routed expert slices from the bounded host slot cache instead of reading them straight out of the mmap'd tower. Read once per process, and only the FIRST character is examined: a value starting with `0`, and an empty value, are off; anything else is on. Only the CPU path streams — on a device platform the expert slice is already device-resident and is served unchanged. Turning it on also **disables the default-on grouped-MoE path** (`VT_QWEN35_GROUPED_MOE`), which stages the whole tower and therefore cannot stream; the engine says so once on stderr rather than silently doing no streaming. Set `VT_MOE_EXPERT_STREAM=0` to keep grouping | | `VT_MOE_EXPERT_STREAM_SLOTS` | `64` | How many expert slices stay resident. Parsed as a decimal integer; unset, empty, zero, negative and unparseable values all keep `64`. Every slot acquired during a step is protected from eviction until the step ends, so a budget smaller than one step's working set exhausts the cache: those slices fall back to reading the tower directly, which is correct but slow, and is counted. Sized against a real model this wants to be large — the measured run used `8000` | | `VT_MOE_EXPERT_STREAM_SLOT_BYTES` | the LARGEST of the gate/up/down slices of the first MoE layer reached | Bytes reserved per slot, fixed for the process's life. Parsed as a decimal integer; unset, empty, zero, negative and unparseable values all keep the default. The default is the largest of the three slices rather than the first one taken, because a dynamic (UD) quant keeps `down_proj` at a higher precision than the gate/up pair and sizing from a gate slice then refuses the first down slice mid-decode. A slice that still does not fit is refused BY NAME (`vt: expert stream: a slice of N bytes exceeds the slot budget of M; raise VT_MOE_EXPERT_STREAM_SLOT_BYTES`) rather than truncated or silently routed back to the mmap path, so a streaming benchmark cannot quietly measure the mmap path instead | -| `VT_MOE_EXPERT_STREAM_STATS_EVERY` | `16` | How many decode steps between the expert-stream statistics line on stderr; `0` silences it. Parsed as a decimal integer; unset, empty, negative and unparseable values all keep `16`. The line is `[expert-stream] steps=N hits=H misses=M evictions=E fills=F bytes=B exhausted=X advised=A`. It exists because the row's first published decode figure was measured on a cache that had switched itself off partway through the third token, and nothing in the run could have said so: the process printed one line at startup and none afterwards. **`steps == 0` or `exhausted > 0` means the lane is not streaming**, whatever the startup line claimed | +| `VT_MOE_EXPERT_STREAM_STATS_EVERY` | `16` | How many decode steps between the PERIODIC expert-stream statistics line on stderr; `0` silences the periodic line only. Parsed as a decimal integer; unset, empty, negative and unparseable values all keep `16`. The line is `[expert-stream] steps=N hits=H misses=M evictions=E fills=F bytes=B exhausted=X advised=A`. **Exactly one FINAL line is printed when the process ends**, whatever this is set to and whatever the run did, including `steps=0`, for as long as the lane built a store. That is the line to read, and it exists because the row's first published decode figure was measured on a cache that had switched itself off partway through the third token while nothing in the run could say so. **`steps == 0` or `exhausted > 0` means the lane is not streaming**, whatever the startup line claimed. Absence of the final line means either that no store was ever built — in which case the `[expert-stream] ON ...` banner is absent too, and the lane was never reached — or that the process did not run its static destructors (a crash, a signal, `_exit`). A fourth shape exists but no shipped command can produce it: the line is printed once per process, and an internal test seam that flushes it mid-run takes that one print. `docs/USAGE.md` tabulates all four | ## Rollback and bisect switches diff --git a/docs/FEATURES.md b/docs/FEATURES.md index d3bf3974b..ccd76f96f 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -61,7 +61,7 @@ are our reading of their documented behavior, not measurements. | Scratch allocator keyed by device (two backends, one process) | ✅ since [#516](https://github.com/mudler/vllm.cpp/issues/516); a pool is bound to one backend and refuses any other, and a backend with no registered platform is refused rather than given another's residency cap | ✅ device is field 0 of the allocation handle | ✅ | ✅ | | Automatic memory sizing (no hand-tuned budget) | ☐ hand-typed block count | ☐ percent, hand-tuned | ☐ | ◐ | | Memory cap with a pre-flight error instead of an OOM | ☐ | ◐ KV pool only | ◐ | ☐ | -| Routed-expert weight streaming from disk | ◐ default OFF (`VT_MOE_EXPERT_STREAM=1`), CPU keep-quant towers only; bounded host slot cache, hotness-decayed LFU + LRU tiebreak; refuses an unfittable slice by name. Serves c1-c4 capacity, not throughput | ☐ blanket `cpu_offload_gb`, not expert-granular | ☐ | ◐ mmap only | +| Routed-expert weight streaming from disk | ◐ default OFF (`VT_MOE_EXPERT_STREAM=1`), CPU keep-quant towers only; bounded slot cache; refuses an unfittable slice by name. c1-c4 capacity, not throughput. One `[expert-stream]` line on a clean exit IF a store existed | ☐ blanket `cpu_offload_gb`, not expert-granular | ☐ | ◐ mmap only | ## Quantization and weight formats diff --git a/docs/USAGE.md b/docs/USAGE.md index 7929c6bd5..5be77c415 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -3206,23 +3206,51 @@ and therefore cannot stream. The engine says that once on stderr rather than silently doing no streaming. **Read the statistics line before you believe any number you measure with it.** -Every `VT_MOE_EXPERT_STREAM_STATS_EVERY` steps (default 16, `0` silences it) the -engine prints: +The engine prints one every `VT_MOE_EXPERT_STREAM_STATS_EVERY` steps (default +16, `0` silences the periodic line), and **exactly one more when the process +ends**, whatever the run did: ```text [expert-stream] steps=64 hits=141230 misses=37312 evictions=29312 fills=37312 bytes=92876505088 exhausted=0 advised=37312 ``` -Two of those fields decide whether the run is measuring anything at all: +**The final line is the one to read**, because it is the only one you are +guaranteed to get. The periodic line is skipped whenever the step count is not a +multiple of the interval, so a healthy five-token run prints none of them at the +default 16; and it used to be skipped on `steps == 0` as well, which meant the +one run that most needed reporting — the one where the step boundary is never +reached — printed nothing at all. Treating absence as failure therefore reported +VOID on a working lane. The final line crosses both of those skips, so it is +printed even on a run of zero steps. -- `steps` must advance. If it stays at 0 the decode step boundary is not being - reached and the cache will stop serving as soon as it fills. +Two of the fields decide whether the run is measuring anything at all: + +- `steps` must advance. If the final line says `steps=0` the decode step + boundary is not being reached, and the cache stops serving as soon as it + fills — it will fall back to the memory mapping for the rest of the run. - `exhausted` must stay 0. Anything above 0 means slices were refused and read from the memory mapping instead, which is the slow path streaming exists to replace. The usual cause is a budget smaller than one step's working set: raise `VT_MOE_EXPERT_STREAM_SLOTS`. -A run whose `steps` is 0 or whose `exhausted` is large is not a measurement of +Read it together with the `[expert-stream] ON slots=...` banner, which is printed +once when the lane builds its store. The four shapes are: + +| Banner | Final line | What happened | +|---|---|---| +| absent | absent | Nothing reached the streamed seam. A CUDA run (a device-resident expert is served unchanged), a checkpoint whose experts are not keep-quant towers, or a prompt that never reached an MoE layer | +| present | present | The lane ran. Read `steps` and `exhausted` | +| present | absent, and nothing called `ExpertStreamFlushStats` | The process did not reach its static destructors: a crash, a signal, or `_exit` | +| present | absent, because `ExpertStreamFlushStats` was called | The internal gate seam took the process's single print, so teardown had none left to make. No shipped command or server path calls it, so an operator never reaches this shape | + +The last two shapes are keyed on the CALL and not on what stderr looks like, +because stderr cannot separate them. `ExpertStreamFlushStats` prints the same +line in the same shape as the periodic report, so "a statistics line already +appeared mid-run" is also what a healthy run of 16 steps that then crashes +produces. What distinguishes the two is whether the seam was called, and only a +gate calls it. + +A run whose `steps` is 0, or whose `exhausted` is large, is not a measurement of streaming, whatever the startup line said. See [`docs/ENVIRONMENT.md`](ENVIRONMENT.md) for every knob and its parsing rules. diff --git a/include/vllm/model_executor/models/qwen3_5_weights.h b/include/vllm/model_executor/models/qwen3_5_weights.h index 4d65db088..3c11f21de 100644 --- a/include/vllm/model_executor/models/qwen3_5_weights.h +++ b/include/vllm/model_executor/models/qwen3_5_weights.h @@ -104,8 +104,19 @@ struct OwnedTensor { mutable int mmap_fd = -1; mutable size_t mmap_file_offset = 0; - // A process-unique identity for this tensor's CURRENT bytes, for a cache that - // outlives the model. + // A process-unique identity for the BUFFER this tensor currently points at, + // for a cache that outlives the model. + // + // READ THAT LITERALLY: the identity is keyed on `bytes.data()`, so it is an + // identity for the address, not for the contents. Replacing a buffer's bytes + // IN PLACE — same address, different weights — keeps the old uid, and the + // cache would then serve the old entries for the new contents. Nothing does + // that today: a tower's `bytes` is assigned once when the model loads and is + // only ever replaced wholesale, which moves the address. This comment says + // where the guarantee stops rather than rounding it up, because #1066 was + // caused by a comment on this exact field that rounded it up (it claimed a + // base pointer was a stable identity, which is true for one model's life and + // false for the cache's). `test_qwen36_weights` pins both halves. // // The expert slot cache is a process-lifetime singleton keyed by (tower, // expert), and it used to derive the tower half from the buffer's ADDRESS. diff --git a/src/vllm/model_executor/models/qwen3_5.cpp b/src/vllm/model_executor/models/qwen3_5.cpp index f184a5a85..f24db5807 100644 --- a/src/vllm/model_executor/models/qwen3_5.cpp +++ b/src/vllm/model_executor/models/qwen3_5.cpp @@ -5138,9 +5138,11 @@ Tensor KqResidentSlice(Dev d, const OwnedTensor& w, int64_t N, int64_t K, // A slot is filled by ONE contiguous copy of a whole slice, so the read is // sequential, and a slice already resident costs nothing at all. // -// The cache is keyed by (tower, expert). A tower's identity is its base -// pointer, which is stable for the model's life because the tower is a borrowed -// view into the mapping; the id map is built once per pointer. +// The cache is keyed by (tower, expert). A tower's identity is `TowerUid()`, a +// process-unique counter, NOT the buffer's address: the store outlives any one +// model, and the allocator hands a freed tower's address to the next one (#1066, +// and the note on TowerId below). + // Whether the operator ASKED for streaming, independent of whether a store has // been built yet. The grouped-MoE gate needs this before any expert is touched. inline bool Qwen35ExpertStreamRequested() { @@ -5234,7 +5236,14 @@ class Qwen35ExpertStream { uint8_t* Slice(const uint8_t* base, uint64_t tower_uid, int64_t expert, size_t offset, size_t bytes, int fd, size_t file_offset) { if (ForceFallback()) { - ++exhausted_; + // COUNTED SEPARATELY FROM `exhausted_`, on purpose (#1091 finding 6). + // `exhausted` is the operator-facing number and both docs define it as + // "the budget is smaller than one step's working set; raise + // VT_MOE_EXPERT_STREAM_SLOTS". This switch has no production caller at + // all, so charging it to that counter told an operator to raise a budget + // that was never the reason. It stays out of the stderr line for the same + // reason: in a production process it is always zero. + ++forced_; return nullptr; } const int32_t tower = TowerId(tower_uid); @@ -5270,6 +5279,16 @@ class Qwen35ExpertStream { // way. `advised_` counts the calls that were actually accepted, so a run // can tell a working hint from a silently rejected one. // + // ROUNDING THE END UP CAN LEAVE THE ALLOCATION, and that is the one way + // this call still fails: madvise(2) returns ENOMEM when any page in the + // range is unmapped, so a tower whose last byte sits near the end of its + // final mapped page would not be counted. In production the tower is a + // borrowed view into a file mapping many pages larger than one slice, so + // the trailing page is mapped. On the heap-backed towers the gates build it + // holds because the allocator's arena page is mapped, not because the + // allocation reaches it — which is why `advised == fills` is asserted + // against a measured run rather than assumed from the arithmetic. + // // NO SPEEDUP IS CLAIMED HERE. This makes the call well-formed; whether // readahead moves decode is a measurement the spec records as owed. #if defined(__unix__) @@ -5302,6 +5321,7 @@ class Qwen35ExpertStream { const ExpertStreamer& streamer() const { return *streamer_; } const ExpertSlotCache& cache() const { return *cache_; } int64_t exhausted() const { return exhausted_; } + int64_t forced() const { return forced_; } int64_t advised() const { return advised_; } // ONE line a benchmark can read to prove the lane stayed live. @@ -5314,24 +5334,61 @@ class Qwen35ExpertStream { // immediately are `steps` and `exhausted`: steps==0 means the step clock never // advanced, and exhausted>0 means slices were refused and silently served from // the mapping instead. Both are on this line, and either is wrong at a glance. + // + // `final` IS THE WHOLE POINT AND IT USED TO HAVE NO CALLER (#1091 finding 1). + // The periodic report is skipped on `steps == 0` — so the one run that most + // needs the line, the one where the step boundary is never reached, printed + // nothing at all, and both docs told an operator to read a zero off a line + // that could not exist. It is skipped again whenever `stats_every_` does not + // divide the step count, and the default is 16, so a healthy five-token run + // printed nothing either and a benchmark reading absence as failure reported + // VOID on a working lane. The final report crosses both early returns. void ReportStats(bool final) const { const int64_t steps = cache_->steps(); if (!final) { if (stats_every_ <= 0) return; if (steps == 0 || steps % stats_every_ != 0) return; } - std::fprintf(stderr, - "[expert-stream] steps=%lld hits=%lld misses=%lld " - "evictions=%lld fills=%lld bytes=%lld exhausted=%lld " - "advised=%lld\n", - static_cast(steps), - static_cast(cache_->hits()), - static_cast(cache_->misses()), - static_cast(cache_->evictions()), - static_cast(streamer_->fills()), - static_cast(streamer_->bytes_filled()), - static_cast(exhausted_), - static_cast(advised_)); + PrintStatsLine(steps, cache_->hits(), cache_->misses(), cache_->evictions(), + streamer_->fills(), streamer_->bytes_filled(), exhausted_, + advised_); + } + + // The final line, printed exactly ONCE per process. + // + // NOTHING IN PRODUCTION CALLS THIS, and read the destructor below before you + // conclude otherwise. Teardown produces the LINE but does not route through + // here: the store is a function-local static, so `~Qwen35ExpertStream` runs on + // the normal exit path and calls `ReportStats` itself, for the reason stated + // there. The two share the once-flag, not a call, so exactly one of them + // prints. This entry exists so a GATE can observe the same guarantee from + // inside a running process, because a static destructor fires after main + // returns and nothing in the process can assert on it. + // + // A once-flag rather than two independent prints, so "one line" is a property + // of the process and not of which caller happened to win. The flag is a plain + // bool with constant initialisation and no destructor of its own, so it cannot + // itself be lost to static-destruction ordering. + // + // No store means no line, and that is not a gap: a store that exists always + // announced itself with `[expert-stream] ON ...` first, so banner-without-line + // is a process that died, and no-banner is a lane nothing ever reached. + static void FlushFinalStats() { + if (FinalReported()) return; + Qwen35ExpertStream* s = Existing(); + if (s == nullptr) return; + FinalReported() = true; + s->ReportStats(/*final=*/true); + } + + ~Qwen35ExpertStream() { + // The store holds the numbers, so it prints them before it goes away. Not + // routed through FlushFinalStats: that reads `Existing()`, and the unique_ptr + // this object lives in does not clear itself before running this destructor. + if (!FinalReported()) { + FinalReported() = true; + ReportStats(/*final=*/true); + } } private: @@ -5354,6 +5411,30 @@ class Qwen35ExpertStream { static bool on = false; return on; } + // Constant-initialised and destructor-free, so the "has the final line been + // printed" answer survives every other static's destruction. + static bool& FinalReported() { + static bool done = false; + return done; + } + + // The one place the statistics line's format lives, so the final report and + // the periodic report cannot drift apart into two shapes a parser has to + // know about. + static void PrintStatsLine(int64_t steps, int64_t hits, int64_t misses, + int64_t evictions, int64_t fills, int64_t bytes, + int64_t exhausted, int64_t advised) { + std::fprintf(stderr, + "[expert-stream] steps=%lld hits=%lld misses=%lld " + "evictions=%lld fills=%lld bytes=%lld exhausted=%lld " + "advised=%lld\n", + static_cast(steps), static_cast(hits), + static_cast(misses), + static_cast(evictions), + static_cast(fills), static_cast(bytes), + static_cast(exhausted), + static_cast(advised)); + } explicit Qwen35ExpertStream(size_t slot_bytes) { const char* sb = std::getenv("VT_MOE_EXPERT_STREAM_SLOT_BYTES"); @@ -5407,6 +5488,11 @@ class Qwen35ExpertStream { std::unordered_map tower_ids_; int32_t next_tower_id_ = 0; int64_t exhausted_ = 0; + // Slices the FORCED-fallback switch refused. Separate from `exhausted_` + // because that one is an operator-facing budget diagnosis and this one is a + // gate asking for the unstreamed arm; see the note at the ForceFallback + // branch in Slice. + int64_t forced_ = 0; int64_t advised_ = 0; int64_t stats_every_ = 16; }; @@ -5427,11 +5513,56 @@ class Qwen35ExpertStream { // // A guard rather than a call at the end of the body: ForwardLayers has two // returns and can throw, and a step that ended by throwing still ended. +// +// ONE FORWARD IS ONE STEP, AND THE GUARD REFUSES TO NEST (#1091 finding 3). +// Five forwards in this file take expert slices — `ForwardLayers`, +// `Qwen3_5Model::ForwardDense`, both MTP forwards and `Qwen3_5ReplayLayer` — +// and each is a complete forward that no other one contains. A nested guard +// would end the step twice, which advances the hotness clock for a step that +// never happened and decays every resident entry an extra tick; that is a +// quieter defect than the missing boundary and it is the one adding guards +// invites. So the precondition is stated rather than handled, the same way +// `MatmulF32Slice` states `expert >= 0`: the flag is per-thread because a +// decode step runs on one host thread, which is the assumption the store's own +// locking already makes. +// +// THE REFUSAL IS NOT GATED ON `Qwen35ExpertStreamRequested()`, deliberately. +// "One forward is one step" is a property of the CALL GRAPH, not of the +// streaming lane: a nest is a defect whether or not a store exists, and the +// streamed run is the rare configuration. Arming it only there would let the +// default-on path establish a nest that nobody sees until someone turns +// streaming on, which is the shape this row keeps finding. The cost is that a +// nest reds every Qwen3.5 forward rather than only the streamed ones, and that +// is the intended polarity: loud on the default path is what makes it a gate. +// +// `Begin`/`End` are named rather than living only in the constructor and +// destructor bodies so that `detail::ExpertStreamStepScope` can hold THE SAME +// boundary. A gate that re-implemented the refusal would prove its own copy; +// this way deleting the `VT_CHECK` below is one edit that both changes +// production and takes the gate red. struct Qwen35ExpertStreamStep { - Qwen35ExpertStreamStep() = default; - ~Qwen35ExpertStreamStep() { Qwen35ExpertStream::EndStepIfActive(); } + Qwen35ExpertStreamStep() { Begin(); } + ~Qwen35ExpertStreamStep() { End(); } Qwen35ExpertStreamStep(const Qwen35ExpertStreamStep&) = delete; Qwen35ExpertStreamStep& operator=(const Qwen35ExpertStreamStep&) = delete; + + // Open the step. Throws when one is already open on this thread; the flag is + // then left as it was, so the outer guard's `End` still closes exactly one. + static void Begin() { + VT_CHECK(!Open(), "qwen3_5: a decode step is already open; the expert-stream " + "step guard marks ONE forward and must not nest"); + Open() = true; + } + static void End() { + Open() = false; + Qwen35ExpertStream::EndStepIfActive(); + } + + private: + static bool& Open() { + static thread_local bool open = false; + return open; + } }; // The expert-slice seam. Identical to KqResidentSlice except that, when @@ -7221,6 +7352,7 @@ detail::ExpertStreamStats detail::ExpertStreamSnapshot() { s.fills = st->streamer().fills(); s.bytes_filled = st->streamer().bytes_filled(); s.exhausted = st->exhausted(); + s.forced = st->forced(); s.advised = st->advised(); return s; } @@ -7231,6 +7363,19 @@ void detail::ExpertStreamSetForceFallback(bool on) { void detail::EndExpertStreamStep() { Qwen35ExpertStream::EndStepIfActive(); } +void detail::ExpertStreamFlushStats() { Qwen35ExpertStream::FlushFinalStats(); } + +// The step guard as a scope a gate can hold. These forward to the SAME +// `Begin`/`End` the production guard's constructor and destructor call, so the +// nesting refusal a gate observes here is the one every forward in this file is +// protected by, not a re-statement of it. +detail::ExpertStreamStepScope::ExpertStreamStepScope() { + Qwen35ExpertStreamStep::Begin(); +} +detail::ExpertStreamStepScope::~ExpertStreamStepScope() { + Qwen35ExpertStreamStep::End(); +} + // ENG-ASYNC-SCHED W4: overwrite the REAL prefix of a freshly uploaded input-id // buffer with the device-resident ids the async runner's combine produced. // @@ -7358,11 +7503,37 @@ static DBuf ForwardLayers(Dev d, const Tensor& hidden_in, const std::vector* aux_layer_ids = nullptr, const Tensor* aux_out = nullptr, StepDevInputs* persistent_sdi = nullptr) { - // ONE decode step. Every MoE entry point funnels through here exactly once - // per forward — ForwardBody, the VL path, and the graph driver's eager - // fallback — so this is the step boundary the expert slot cache is defined - // against, and it is neither once per layer nor once per expert. Inert unless - // a store exists, which needs both VT_MOE_EXPERT_STREAM and a slice taken. + // ONE decode step, for the PAGED forwards: ForwardBody, the VL path and the + // graph driver's eager fallback all funnel through here exactly once per + // forward. This is the step boundary the expert slot cache is defined against, + // and it is neither once per layer nor once per expert. + // + // IT IS NOT THE ONLY ENTRY POINT, and the comment this replaces said it was + // (#1091 finding 3). Four more forwards reach `ExpertMlpKq -> KqExpertSlice` + // without passing through here — `Qwen3_5Model::ForwardDense`, + // `Qwen3_5MTPModel::Forward`, `Qwen3_5MTPModel::ForwardPaged` and + // `Qwen3_5ReplayLayer` — and each now carries its own guard. + // + // ONE OF THOSE FOUR HAS A PRODUCTION CALLER, not all of them, and an earlier + // revision of this comment said "the MTP pair" (#1106 finding 2, #1108). It is + // `Qwen3_5MTPModel::ForwardPaged`, the spec-decode DRAFT forward, reached from + // `runner.cpp:2183` through `spec_decode/mtp/speculator.cpp:107,262` — so the + // shape that was actually running is draft forwards that pin every slot they + // touch across the following target forward. That caller is itself + // "UNREACHABLE unless a speculator is configured" (`runner.cpp:2120`), so a + // DEFAULT-configuration run reaches none of these four guards; one of them has + // a production caller, which is not the same claim. `Qwen3_5MTPModel::Forward`, + // `Qwen3_5Model::ForwardDense` and `Qwen3_5ReplayLayer` are parity entry + // points whose every caller is under `tests/`, and per `.agents/reachability.md` + // a call site inside a test is not reach: their guards land UNREACHED, which + // the spec's `## Owed` records as a staged slice rather than claiming. + // + // `RunMoeBlock` is the deliberate exception: it is one block, not a forward, + // and qwen3_moe.cpp owns the boundary for the model that composes it + // (qwen3_moe.cpp:150). + // + // Inert unless a store exists, which needs both VT_MOE_EXPERT_STREAM and a + // slice taken. const Qwen35ExpertStreamStep expert_stream_step; const int64_t T = hidden_in.shape[0]; const int64_t H = config.hidden_size; @@ -7869,6 +8040,10 @@ std::vector Qwen3_5Model::ForwardDense(const std::vector& token_ "qwen3_5 forward: positions length must equal token count"); VT_CHECK(static_cast(weights.layers.size()) == config.num_hidden_layers, "qwen3_5 forward: weights.layers size must equal num_hidden_layers"); + // ONE decode step. This forward does NOT go through ForwardLayers — it runs + // its own unpaged layer loop — so it needs its own boundary, and every MoE + // layer it runs takes expert slices (#1091 finding 3). + const Qwen35ExpertStreamStep expert_stream_step; Dev d{vt::GetBackend(queue.device.type), queue}; const float eps = static_cast(config.rms_norm_eps); @@ -7998,6 +8173,21 @@ Qwen3_5MTPHiddenStates Qwen3_5MTPModel::Forward( "qwen3_5 MTP forward: fc must be raw bf16 [H,2H]"); (void)vocab_size; + // ONE decode step. A DRAFT forward is a complete forward with its own working + // set: its slices are finished with when it returns, and leaving them pinned + // across the target's forward would shrink the evictable set for the whole run + // — F1 at draft scale (#1091 finding 3). A spec-decode iteration therefore + // advances the clock once per draft plus once for the target, which is what + // "one step is one forward" means for a draft+target pair. + // + // THAT PAIR IS RUN BY `ForwardPaged`, NOT BY THIS OVERLOAD. This one is + // reached only through `ForwardLogitsHost`, a standalone parity convenience + // (qwen3_5_mtp.h:135) whose every caller is under `tests/`, so the guard here + // lands unreached and is recorded as a staged slice (#1108). It is kept + // because the reasoning above is what makes it correct the moment this + // overload gains a caller, and adding the guard later with the caller is how + // this row lost its step boundary the first time. + const Qwen35ExpertStreamStep expert_stream_step; Dev device{vt::GetBackend(queue.device.type), queue}; // Qwen3_5MultiTokenPredictor.forward head: shared embedding + independent Gemma @@ -8050,6 +8240,8 @@ Qwen3_5MTPHiddenStates Qwen3_5MTPModel::ForwardPaged( draft_kv.head_size == config_->head_dim, "qwen3_5 MTP paged forward: draft KV cache dims mismatch config"); + // ONE decode step, for the same reason as the unpaged draft forward above. + const Qwen35ExpertStreamStep expert_stream_step; Dev device{vt::GetBackend(queue.device.type), queue}; // Same head math as Forward; the difference is the DECODER LAYER, which runs @@ -8975,6 +9167,10 @@ std::vector Qwen3_5ReplayLayer(const Qwen3_5MoeLayerWeights& layer, const int64_t H = config.hidden_size; VT_CHECK(static_cast(hidden_in.size()) == T * H, "qwen3_5 replay: hidden_in must be [T*H]"); + // ONE decode step. This replays a single layer as a self-contained unit of + // work, so the slices it takes are finished with when it returns; without a + // boundary they stay pinned for the life of the process (#1091 finding 3). + const Qwen35ExpertStreamStep expert_stream_step; Dev d{vt::GetBackend(queue.device.type), queue}; // Seed the fused stream with the combined residual input: res = hidden_in, diff --git a/src/vllm/model_executor/models/qwen3_5_internal.h b/src/vllm/model_executor/models/qwen3_5_internal.h index b71007c4f..86ee7b40e 100644 --- a/src/vllm/model_executor/models/qwen3_5_internal.h +++ b/src/vllm/model_executor/models/qwen3_5_internal.h @@ -391,6 +391,12 @@ struct ExpertStreamStats { // means the budget is smaller than one step's working set, OR that the step // boundary is not being called at all. int64_t exhausted = 0; + // Slices refused because a GATE asked for the unstreamed arm through + // `ExpertStreamSetForceFallback`, which no production path calls. It is + // separate from `exhausted` because that number is an operator-facing budget + // diagnosis, and a test switch inflating it says "raise + // VT_MOE_EXPERT_STREAM_SLOTS" about a budget that was never the reason. + int64_t forced = 0; // madvise(MADV_WILLNEED) calls the kernel ACCEPTED. Zero while slices are // being filled from a mapping means the hint is being rejected, which is what // an unaligned address does silently. @@ -411,4 +417,60 @@ void ExpertStreamSetForceFallback(bool on); // calls it from its layer driver for the same reason. void EndExpertStreamStep(); +// Print the streamed-expert statistics line NOW, once, whatever the run did. +// +// THIS FUNCTION HAS ZERO PRODUCTION CALLERS, and a grep-and-quote reader should +// get that before anything else: it exists for the gate. The only production +// path to the LINE is `~Qwen35ExpertStream`, which does NOT route through here — +// the store is a function-local static, so it is destroyed on the normal exit +// path and prints what the run ended up doing directly. The two share the +// once-flag rather than a call, so exactly one of them prints; an earlier +// revision headed this comment "~Qwen35ExpertStream IS THE ONLY PRODUCTION +// CALLER", which reads as a call that is not there (#1106). +// +// There is no second hook either: nothing registers an `atexit` handler when +// streaming is merely REQUESTED, and none was landed — that shape is recorded +// under the spec's `## Owed` with its reason. An even earlier revision of this +// comment claimed the hook existed, while the change that wrote it was fixing +// exactly this class of overclaim one file away (#1091). Two revisions, two +// overstatements of the same four lines, which is why they now name the +// mechanism rather than summarise it. +// +// So the guarantee carries the same two qualifiers `docs/USAGE.md` does, and it +// is one line per process under both: a store must have been BUILT, and the +// process must RUN its static destructors. No store means no line — and no +// `[expert-stream] ON ...` banner either, which is how the absent pair is read +// — and a crash, a signal or `_exit` prints nothing. Under those it holds even +// on a run with zero steps and with the periodic report silenced, which is what +// makes `steps == 0` readable at all. +// +// This entry exists because a static destructor fires after main returns and +// nothing inside the process can assert on it. Calling it TAKES the once-flag, +// so it suppresses the teardown line for the rest of the process and the caller +// becomes the one place the line appears. A second call prints nothing. +void ExpertStreamFlushStats(); + +// ONE decode step, as a scope, for a gate that needs to hold the boundary +// itself rather than reach it through a forward. +// +// It exists for one question: the step guard REFUSES TO NEST, and no legitimate +// call graph in the tree can ask it to. Every forward that takes expert slices +// is a complete forward that no other one contains, so the refusal was asserted +// in three places and pinned in none — deleting its `VT_CHECK` left both +// focused binaries fully green (#1091 review of #1100). A gate cannot reach it +// through production code, and a gate that re-implemented the flag would prove +// its own copy, so the guard's boundary is exposed here and this scope forwards +// to it. +// +// Constructing a second scope, or entering a forward while one is held, throws +// `std::runtime_error`. That is armed on the DEFAULT path and not only on the +// streaming lane, on purpose: see the note on `Qwen35ExpertStreamStep`. +class ExpertStreamStepScope { + public: + ExpertStreamStepScope(); + ~ExpertStreamStepScope(); + ExpertStreamStepScope(const ExpertStreamStepScope&) = delete; + ExpertStreamStepScope& operator=(const ExpertStreamStepScope&) = delete; +}; + } // namespace vllm::detail diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ca453db25..cc89c3469 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1093,6 +1093,12 @@ target_include_directories(test_expert_stream_wiring PRIVATE ${CMAKE_SOURCE_DIR} # defect, and the store is built from the FIRST model a process runs. vllm_cpp_add_test(test_expert_stream_mixed_slot vllm/model_executor/test_expert_stream_mixed_slot.cpp) target_include_directories(test_expert_stream_mixed_slot PRIVATE ${CMAKE_SOURCE_DIR}/src) +# ENG-EXPERT-STREAM (#1091): the step clock at the MoE entry points ForwardLayers +# does NOT cover, and the final statistics line. Its own binary because it is the +# only one that can observe `steps == 0` -- the store is a process-lifetime +# singleton, so a case asking that question must run before anything ends a step. +vllm_cpp_add_test(test_expert_stream_steps vllm/model_executor/test_expert_stream_steps.cpp) +target_include_directories(test_expert_stream_steps PRIVATE ${CMAKE_SOURCE_DIR}/src) # ENG-MM-INPUT-PIPELINE L1 (#607): the refusal those limits carry — the min() # fold against the model's own ceiling, upstream's exact message, and both call # sites with the enable_mm_embeds escape. diff --git a/tests/support/expert_stream_model.h b/tests/support/expert_stream_model.h new file mode 100644 index 000000000..7b26afb84 --- /dev/null +++ b/tests/support/expert_stream_model.h @@ -0,0 +1,309 @@ +// The synthetic Qwen3.5-MoE model that `test_expert_stream_wiring` and +// `test_expert_stream_steps` both drive (ENG-EXPERT-STREAM, issue #912, repairs +// #1091). +// +// WHY A HEADER AND NOT A DUPLICATE. `VT_MOE_EXPERT_STREAM` is read ONCE into a +// function-local static, and the store behind it is a process-lifetime +// singleton, so each question about the lane needs its own PROCESS and therefore +// its own test binary. Those two ask different questions of the SAME model — a +// four-layer hybrid MoE whose routed experts are uniform Q8_0 keep-quant STACKED +// towers, which is the shape `KqExpertSlice` slices — and a copy per binary +// would let the copies drift apart from the shape the seam serves. +// +// `test_expert_stream_mixed_slot` is deliberately NOT a client: its whole +// subject is a tower set whose gate/up and down slices differ in size (Q4_0 +// against Q8_0), and it must not set `VT_MOE_EXPERT_STREAM_SLOT_BYTES`, because +// that override is what would hide the defect it exists for. Its model is a +// different model, not a copy of this one. +// +// Each client keeps its own environment setup, since which knobs a binary sets +// is part of what it is asking. +#ifndef VLLM_TESTS_SUPPORT_EXPERT_STREAM_MODEL_H_ +#define VLLM_TESTS_SUPPORT_EXPERT_STREAM_MODEL_H_ + +#include + +#include +#include +#include + +#include "vllm/model_executor/models/qwen3_5.h" +#include "vllm/model_executor/models/qwen3_5_weights.h" +#include "vllm/transformers_utils/hf_config.h" +#include "vllm/v1/attention/backend.h" +#include "vllm/v1/attention/backends/gdn_attn.h" +#include "vt/backend.h" +#include "vt/dtype.h" +#include "vt/tensor.h" + +namespace expert_stream_test { + +using vllm::GdnStateCache; +using vllm::HfConfig; +using vllm::PagedKvCache; +using vllm::Qwen3_5MoeWeights; +using vllm::v1::CommonAttentionMetadata; +using vllm::v1::GDNAttentionMetadata; +using vt::DType; + +inline float RandV(uint64_t s) { + s = s * 6364136223846793005ULL + 1442695040888963407ULL; + s ^= s >> 33; + return (static_cast((s >> 40) & 0xFFFF) / 32768.0f) - 1.0f; +} + +inline vllm::OwnedTensor MakeOwned(DType dt, const std::vector& shape, uint64_t seed) { + vllm::OwnedTensor t; + t.dtype = dt; + t.rank = static_cast(shape.size()); + int64_t n = 1; + for (int i = 0; i < t.rank; ++i) { + t.shape[i] = shape[static_cast(i)]; + n *= shape[static_cast(i)]; + } + if (dt == DType::kBF16) { + std::vector b(static_cast(n) * 2); + auto* p = reinterpret_cast(b.data()); + for (int64_t i = 0; i < n; ++i) p[i] = vt::F32ToBF16(RandV(seed + static_cast(i))); + t.bytes = vllm::OwnedBytes(std::move(b)); + } else { + std::vector b(static_cast(n) * 4); + auto* p = reinterpret_cast(b.data()); + for (int64_t i = 0; i < n; ++i) p[i] = RandV(seed + static_cast(i)); + t.bytes = vllm::OwnedBytes(std::move(b)); + } + return t; +} + +// A keep-quant STACKED expert tower: [rows, cols] Q8_0, `nk = true`, exactly the +// shape the GGUF keep-quant loader produces and the only shape KqExpertSlice +// slices. +// +// The blocks are BUILT, not filled with noise. A Q8_0 block is an fp16 scale +// followed by 32 int8 weights, and random bytes put random bit patterns in the +// scale — including the fp16 encodings of inf and NaN, which propagate straight +// through the GEMM and make every later comparison vacuous. The values are +// arbitrary but well-formed, which is all this test needs: every arm decodes the +// SAME bytes, so equality between arms is a real comparison. +inline vllm::OwnedTensor MakeKqTower(int64_t rows, int64_t cols, uint64_t seed) { + vllm::OwnedTensor t; + t.dtype = DType::kQ8_0; + t.nk = true; + t.rank = 2; + t.shape[0] = rows; + t.shape[1] = cols; + const size_t row_bytes = vt::RowSizeBytes(DType::kQ8_0, cols); + const int64_t blocks_per_row = cols / 32; + REQUIRE(cols % 32 == 0); // Q8_0 is a 32-element block quant + REQUIRE(row_bytes == static_cast(blocks_per_row) * 34); + std::vector b(static_cast(rows) * row_bytes); + size_t o = 0; + for (int64_t r = 0; r < rows; ++r) { + for (int64_t blk = 0; blk < blocks_per_row; ++blk) { + const uint16_t d = vt::F32ToF16(0.004f + 0.001f * RandV(seed + static_cast(r * 131 + blk))); + std::memcpy(b.data() + o, &d, 2); + o += 2; + for (int j = 0; j < 32; ++j) { + const int8_t q = static_cast( + static_cast(100.0f * RandV(seed + static_cast((r * 131 + blk) * 32 + j)))); + std::memcpy(b.data() + o, &q, 1); + o += 1; + } + } + } + t.bytes = vllm::OwnedBytes(std::move(b)); + return t; +} + +inline HfConfig MakeConfig() { + HfConfig c; + c.model_type = "qwen3_5_moe_text"; + c.architectures = {"Qwen3_5MoeForConditionalGeneration"}; + c.hidden_size = 32; + c.num_hidden_layers = 4; + c.vocab_size = 40; + c.num_attention_heads = 4; + c.num_key_value_heads = 2; + c.head_dim = 8; + c.layer_types = {"linear_attention", "linear_attention", "linear_attention", + "full_attention"}; + c.num_experts = 4; + c.num_experts_per_tok = 2; + c.moe_intermediate_size = 32; + c.shared_expert_intermediate_size = 16; + c.linear_num_key_heads = 2; + c.linear_num_value_heads = 4; + c.linear_key_head_dim = 8; + c.linear_value_head_dim = 8; + c.linear_conv_kernel_dim = 4; + c.rope_theta = 10000.0; + c.rotary_dim = 4; + c.rms_norm_eps = 1e-6; + c.max_position_embeddings = 64; + return c; +} + +inline vllm::MoeBlockWeights MakeKqMoe(const HfConfig& c, uint64_t s) { + vllm::MoeBlockWeights m; + const int64_t H = c.hidden_size, E = c.num_experts, I = c.moe_intermediate_size, + Is = c.shared_expert_intermediate_size; + m.router_gate = MakeOwned(DType::kBF16, {H, E}, s + 1); + m.shared_gate = MakeOwned(DType::kBF16, {H, 1}, s + 2); + // The routed experts are STACKED keep-quant towers, and the per-expert vectors + // stay empty — the A3 layout the streaming seam is defined against. + m.expert_gate_kq = MakeKqTower(E * I, H, s + 100); + m.expert_up_kq = MakeKqTower(E * I, H, s + 200); + m.expert_down_kq = MakeKqTower(E * H, I, s + 300); + m.shared_gate_proj = MakeOwned(DType::kBF16, {H, Is}, s + 3); + m.shared_up_proj = MakeOwned(DType::kBF16, {H, Is}, s + 4); + m.shared_down_proj = MakeOwned(DType::kBF16, {Is, H}, s + 5); + return m; +} + +inline Qwen3_5MoeWeights MakeWeights(const HfConfig& c, uint64_t base_seed = 0) { + Qwen3_5MoeWeights w; + const int64_t H = c.hidden_size, V = c.vocab_size; + const int64_t Hq = c.num_attention_heads, Hkv = c.num_key_value_heads, + Dh = c.head_dim; + const int64_t Hk = c.linear_num_key_heads, Hv = c.linear_num_value_heads, + Dk = c.linear_key_head_dim, Dv = c.linear_value_head_dim, + Kw = c.linear_conv_kernel_dim; + const int64_t key_dim = Hk * Dk, value_dim = Hv * Dv, + conv_dim = 2 * key_dim + value_dim; + w.embed_tokens = MakeOwned(DType::kBF16, {V, H}, 11); + w.final_norm = MakeOwned(DType::kBF16, {H}, 12); + w.lm_head = MakeOwned(DType::kBF16, {H, V}, 13); + for (int64_t l = 0; l < c.num_hidden_layers; ++l) { + const uint64_t s = base_seed + 1000 + static_cast(l) * 5000; + vllm::Qwen3_5MoeLayerWeights lw; + lw.is_linear_attention = (c.layer_types[static_cast(l)] == "linear_attention"); + lw.input_layernorm = MakeOwned(DType::kBF16, {H}, s + 1); + lw.post_attention_layernorm = MakeOwned(DType::kBF16, {H}, s + 2); + if (lw.is_linear_attention) { + lw.gdn.in_proj_qkv = MakeOwned(DType::kBF16, {H, conv_dim}, s + 10); + lw.gdn.in_proj_z = MakeOwned(DType::kBF16, {H, value_dim}, s + 20); + lw.gdn.in_proj_b = MakeOwned(DType::kBF16, {H, Hv}, s + 30); + lw.gdn.in_proj_a = MakeOwned(DType::kBF16, {H, Hv}, s + 40); + lw.gdn.conv1d_weight = MakeOwned(DType::kBF16, {conv_dim, Kw}, s + 50); + lw.gdn.a_log = MakeOwned(DType::kF32, {Hv}, s + 60); + lw.gdn.dt_bias = MakeOwned(DType::kF32, {Hv}, s + 70); + lw.gdn.norm_weight = MakeOwned(DType::kBF16, {Dv}, s + 80); + lw.gdn.out_proj = MakeOwned(DType::kBF16, {value_dim, H}, s + 90); + } else { + lw.attn.q_proj = MakeOwned(DType::kBF16, {H, 2 * Hq * Dh}, s + 10); + lw.attn.k_proj = MakeOwned(DType::kBF16, {H, Hkv * Dh}, s + 20); + lw.attn.v_proj = MakeOwned(DType::kBF16, {H, Hkv * Dh}, s + 30); + lw.attn.o_proj = MakeOwned(DType::kBF16, {Hq * Dh, H}, s + 40); + lw.attn.q_norm = MakeOwned(DType::kBF16, {Dh}, s + 50); + lw.attn.k_norm = MakeOwned(DType::kBF16, {Dh}, s + 60); + } + lw.moe = MakeKqMoe(c, s + 500); + w.layers.push_back(std::move(lw)); + } + return w; +} + +struct CachePool { + const HfConfig& c; + int64_t num_blocks; + int64_t block_size; + std::vector> full_attn_buf; + std::vector> gdn_ssm_buf; + std::vector> gdn_conv_buf; + std::vector attn_kv; + std::vector gdn_state; + + CachePool(const HfConfig& cfg, int64_t nb, int64_t bs) + : c(cfg), num_blocks(nb), block_size(bs) { + const int64_t Hkv = c.num_key_value_heads, Dh = c.head_dim; + const int64_t Hv = c.linear_num_value_heads, Dv = c.linear_value_head_dim, + Dk = c.linear_key_head_dim, Kw = c.linear_conv_kernel_dim; + const int64_t key_dim = c.linear_num_key_heads * Dk, value_dim = Hv * Dv; + const int64_t conv_dim = 2 * key_dim + value_dim; + for (int64_t l = 0; l < c.num_hidden_layers; ++l) { + if (c.layer_types[static_cast(l)] == "linear_attention") { + gdn_ssm_buf.emplace_back(static_cast(nb * Hv * Dv * Dk), 0.0f); + gdn_conv_buf.emplace_back(static_cast(nb * conv_dim * (Kw - 1)), 0.0f); + } else { + full_attn_buf.emplace_back(static_cast(nb * 2 * bs * Hkv * Dh), 0.0f); + } + } + Rebind(); + } + + void Rebind() { + const int64_t Hkv = c.num_key_value_heads, Dh = c.head_dim; + const int64_t Hv = c.linear_num_value_heads, Dv = c.linear_value_head_dim, + Dk = c.linear_key_head_dim, Kw = c.linear_conv_kernel_dim; + const int64_t key_dim = c.linear_num_key_heads * Dk, value_dim = Hv * Dv; + const int64_t conv_dim = 2 * key_dim + value_dim; + attn_kv.clear(); + gdn_state.clear(); + for (auto& b : full_attn_buf) { + PagedKvCache kv; + kv.data = b.data(); + kv.dtype = DType::kF32; + kv.num_blocks = num_blocks; + kv.block_size = block_size; + kv.num_kv_heads = Hkv; + kv.head_size = Dh; + attn_kv.push_back(kv); + } + for (size_t g = 0; g < gdn_ssm_buf.size(); ++g) { + GdnStateCache gs; + gs.ssm_state = vt::Tensor::Contiguous(gdn_ssm_buf[g].data(), DType::kF32, + vt::Device{vt::DeviceType::kCPU, 0}, + {num_blocks, Hv, Dv, Dk}); + gs.conv_state = vt::Tensor::Contiguous(gdn_conv_buf[g].data(), DType::kF32, + vt::Device{vt::DeviceType::kCPU, 0}, + {num_blocks, conv_dim, Kw - 1}); + gdn_state.push_back(gs); + } + } +}; + +inline vt::Queue Q() { return vt::Queue{vt::Device{vt::DeviceType::kCPU, 0}, nullptr}; } + +inline CommonAttentionMetadata PrefillAttnMeta(int64_t T, const std::vector& blocks, + int64_t block_size, int64_t start_slot) { + CommonAttentionMetadata m; + m.num_reqs = 1; + m.num_actual_tokens = static_cast(T); + m.query_start_loc = {0, static_cast(T)}; + m.query_start_loc_cpu = m.query_start_loc; + m.seq_lens = {static_cast(T)}; + m.seq_lens_cpu = m.seq_lens; + m.max_query_len = static_cast(T); + m.max_seq_len = static_cast(T); + m.block_table_num_cols = static_cast(blocks.size()); + m.block_table_tensor = blocks; + for (int64_t t = 0; t < T; ++t) { + const int64_t blk = blocks[static_cast(t / block_size)]; + m.slot_mapping.push_back(blk * block_size + (start_slot + t) % block_size); + } + m.causal = true; + return m; +} + +inline GDNAttentionMetadata PrefillGdnMeta(int64_t T, int32_t sidx) { + GDNAttentionMetadata g; + g.num_prefills = 1; + g.num_prefill_tokens = static_cast(T); + g.num_decodes = 0; + g.num_decode_tokens = 0; + g.num_actual_tokens = static_cast(T); + g.has_initial_state = std::vector{0}; + g.non_spec_state_indices_tensor = std::vector{sidx}; + g.non_spec_query_start_loc = std::vector{0, static_cast(T)}; + g.prefill_query_start_loc = std::vector{0, static_cast(T)}; + g.prefill_state_indices = std::vector{sidx}; + g.prefill_has_initial_state = std::vector{0}; + const auto conv = + vllm::v1::ComputeCausalConv1dMetadata(*g.non_spec_query_start_loc); + g.batch_ptr = conv.batch_ptr; + g.token_chunk_offset_ptr = conv.token_chunk_offset_ptr; + return g; +} +} // namespace expert_stream_test + +#endif // VLLM_TESTS_SUPPORT_EXPERT_STREAM_MODEL_H_ diff --git a/tests/vllm/model_executor/test_expert_stream_steps.cpp b/tests/vllm/model_executor/test_expert_stream_steps.cpp new file mode 100644 index 000000000..66b7afa64 --- /dev/null +++ b/tests/vllm/model_executor/test_expert_stream_steps.cpp @@ -0,0 +1,461 @@ +// ENG-EXPERT-STREAM (#912, repairs #1091): the step clock, at every MoE entry +// point, and the one statistics line that always prints. +// +// WHY A SECOND WIRING BINARY. `test_expert_stream_wiring` asks whether the paged +// forward reaches the lane at all. It cannot ask these two questions, because +// both need a process whose step clock starts at zero and stays there: the +// singleton store and the once-read `VT_MOE_EXPERT_STREAM` are process-scoped, +// so a case that must observe `steps == 0` has to run before anything ends a +// step, and the reachability case ends three. +// +// WHAT IS UNDER TEST. +// +// 1. `ForwardLayers` is not the only MoE entry point, and its comment used to +// say it was. `Qwen3_5Model::ForwardDense`, both MTP forwards and +// `Qwen3_5ReplayLayer` all reach `ExpertMlpKq -> KqExpertSlice` and none of +// them marked a step. A forward that takes slices and never ends its step +// leaves every entry it acquired `protected_this_step` forever, which is +// defect F1 with a smaller blast radius: on a draft+target pair the draft's +// slots stay pinned across the target's forward and shrink the evictable +// set for the whole run. +// +// WHICH OF THE FOUR PRODUCTION ACTUALLY RUNS: one. Only +// `Qwen3_5MTPModel::ForwardPaged` has a production caller (`runner.cpp:2183` +// -> `spec_decode/mtp/speculator.cpp:107,262`), and that caller runs only +// when a speculator is configured (`runner.cpp:2120`), so no +// default-configuration run reaches any of the four. `Qwen3_5MTPModel::Forward` +// is reached only through `ForwardLogitsHost`, which `qwen3_5_mtp.h:135` +// calls a "standalone parity convenience" and which has no caller outside +// `tests/`; `ForwardDense` and `Qwen3_5ReplayLayer` are parity references +// the same way. An earlier revision of this comment called both MTP +// forwards the production draft path (#1106 finding 2). So for three of the +// four guards this binary is the ONLY driver there is: the cases below pin +// the boundary, they do not demonstrate reach, and #1108 plus the spec's +// `## Owed` carry that debt. +// +// ONE FORWARD IS ONE STEP. That is the definition the cache is built +// against, and it is why the draft gets its own step rather than sharing +// the target's: the draft is a complete forward whose slices are finished +// with when it returns, and folding it into the target's step would pin +// them across a second forward for no benefit. Each case below asserts a +// DELTA of exactly one, so ordering between cases cannot flatter it, and +// an entry point that marked its step twice would fail just as loudly as +// one that never marked it. +// +// 2. The statistics line has to print even when the run did nothing. It used +// to be emitted only from `EndStep`, and only on a step that was a +// multiple of `VT_MOE_EXPERT_STREAM_STATS_EVERY` — so the one run that +// most needed it, the one where the step boundary is never reached, was +// exactly the run that printed nothing at all. Both docs told an operator +// to read `steps == 0` off a line that could not exist. +#include +#if !defined(_WIN32) +// The two questions about the statistics LINE need POSIX: one redirects stderr +// across the flush, the other runs this binary again as a child. The step-clock +// questions below need neither and are built everywhere — which they were NOT +// when this comment was first written. `::setenv` sat at namespace scope with +// no guard, and it is POSIX: MSVC's CRT has only `_putenv_s`, so the whole +// translation unit failed to compile there and none of the cases existed on +// Windows at all. `tests/CMakeLists.txt` adds this target unconditionally and +// `scripts/build-windows-release.ps1` configures `VLLM_CPP_BUILD_TESTS=ON`, so +// the only reason CI stayed quiet is that the Windows lanes already fail +// earlier, inside the product library, on #1068 and never reach a test +// translation unit. The repair is `vllm_test::SetEnv` from +// `support/test_env.h`, which is where the `_putenv_s` branch already lived. +// +// Streaming itself is a POSIX lane — `EnsureFile` refuses on _WIN32 by name — +// but the STEP CLOCK is not: it advances on the mapping-copy fallback too, so +// these cases have something to measure there. +#include +#endif + +#include + +#include +#include +#include +#include +#include + +#include "support/expert_stream_model.h" +#include "support/test_env.h" +#include "vllm/model_executor/models/qwen3_5.h" +#include "vllm/model_executor/models/qwen3_5_internal.h" +#include "vllm/model_executor/models/qwen3_5_moe_block.h" +#include "vllm/model_executor/models/qwen3_5_mtp.h" +#include "vllm/model_executor/models/qwen3_5_weights.h" + +using expert_stream_test::CachePool; +using expert_stream_test::MakeConfig; +using expert_stream_test::MakeKqMoe; +using expert_stream_test::MakeOwned; +using expert_stream_test::MakeWeights; +using expert_stream_test::PrefillAttnMeta; +using expert_stream_test::Q; +using vllm::HfConfig; +using vllm::Qwen3_5MoeWeights; +using vllm::Qwen3_5Model; +using vllm::Qwen3_5MTPKind; +using vllm::Qwen3_5MTPModel; +using vllm::Qwen3_5MTPWeights; +using vt::DType; + +namespace { + +// Same knobs as the reachability binary, and for the same reasons. The one that +// matters here is `STATS_EVERY=0`: it SILENCES the periodic line, so any +// statistics line this process emits can only have come from the final flush. +// +// Through `vllm_test::SetEnv` and not `::setenv`, which is what this file did +// and is the whole of the Windows defect: the shim in `support/test_env.h` is +// the one place the `_putenv_s` branch lives (#603), and a new env-flipping +// test is exactly what it says to use. +struct EnableExpertStreaming { + EnableExpertStreaming() { + vllm_test::SetEnv("VT_MOE_EXPERT_STREAM", "1"); + vllm_test::SetEnv("VT_MOE_EXPERT_STREAM_SLOTS", "64"); + vllm_test::SetEnv("VT_MOE_EXPERT_STREAM_SLOT_BYTES", "8192"); + vllm_test::SetEnv("VT_MOE_EXPERT_STREAM_STATS_EVERY", "0"); + vllm_test::SetEnv("VT_QWEN35_GROUPED_MOE", "0"); + } +}; +const EnableExpertStreaming kEnableExpertStreaming; + +// This process was spawned by the teardown case below and must do the one thing +// that case measures — build a store and exit — and nothing else. +bool IsFlushChild() { return ::getenv("VT_ES_FLUSH_CHILD") != nullptr; } + +int64_t Steps() { return vllm::detail::ExpertStreamSnapshot().steps; } + +// A full-attention MoE layer, which is the only layer type an MTP head has +// (qwen3_5.cpp: "The MTP layer is always layer_type=full_attention"). +vllm::Qwen3_5MoeLayerWeights MakeFullAttnMoeLayer(const HfConfig& c, uint64_t s) { + const int64_t H = c.hidden_size, Hq = c.num_attention_heads, + Hkv = c.num_key_value_heads, Dh = c.head_dim; + vllm::Qwen3_5MoeLayerWeights lw; + lw.is_linear_attention = false; + lw.input_layernorm = MakeOwned(DType::kBF16, {H}, s + 1); + lw.post_attention_layernorm = MakeOwned(DType::kBF16, {H}, s + 2); + lw.attn.q_proj = MakeOwned(DType::kBF16, {H, 2 * Hq * Dh}, s + 10); + lw.attn.k_proj = MakeOwned(DType::kBF16, {H, Hkv * Dh}, s + 20); + lw.attn.v_proj = MakeOwned(DType::kBF16, {H, Hkv * Dh}, s + 30); + lw.attn.o_proj = MakeOwned(DType::kBF16, {Hq * Dh, H}, s + 40); + lw.attn.q_norm = MakeOwned(DType::kBF16, {Dh}, s + 50); + lw.attn.k_norm = MakeOwned(DType::kBF16, {Dh}, s + 60); + lw.moe = MakeKqMoe(c, s + 500); + return lw; +} + +Qwen3_5MTPWeights MakeMtpWeights(const HfConfig& c, uint64_t s) { + const int64_t H = c.hidden_size; + Qwen3_5MTPWeights w; + w.kind = Qwen3_5MTPKind::kMoe; + w.fc = MakeOwned(DType::kBF16, {H, 2 * H}, s + 1); + w.fc.nk = true; // raw torch Linear [H,2H], as the forward's precondition says + w.pre_fc_norm_embedding = MakeOwned(DType::kBF16, {H}, s + 2); + w.pre_fc_norm_hidden = MakeOwned(DType::kBF16, {H}, s + 3); + w.final_norm = MakeOwned(DType::kBF16, {H}, s + 4); + w.moe_layers.push_back(MakeFullAttnMoeLayer(c, s + 1000)); + return w; +} + +} // namespace + +// ───────────────────────────────────────────────────────────────────────────── +// This case runs FIRST on purpose: it is the only place in this binary where +// `steps` is still 0, which is the state the whole finding is about. +// ───────────────────────────────────────────────────────────────────────────── +TEST_CASE("the final statistics line prints on a run whose step clock never advanced") { + const HfConfig c = MakeConfig(); + const Qwen3_5MoeWeights w = MakeWeights(c); + vt::Queue q = Q(); + + // `RunMoeBlock` is the seam qwen3_moe.cpp composes the same MoE block + // through, and it deliberately carries NO step guard: that model's own layer + // driver owns the boundary (qwen3_moe.cpp:150). Driving it directly therefore + // reproduces the exact production shape this line exists to report — expert + // slices taken, no step ended — without having to break anything. + const int64_t T = 2, H = c.hidden_size; + std::vector hidden(static_cast(T * H)); + for (size_t i = 0; i < hidden.size(); ++i) + hidden[i] = vt::F32ToBF16(expert_stream_test::RandV(7 + i)); + const vt::Tensor dh = vt::Tensor::Contiguous( + hidden.data(), DType::kBF16, vt::Device{vt::DeviceType::kCPU, 0}, {T, H}); + const vllm::MoeBlockOutput out = + vllm::RunMoeBlock(q, w.layers[0].moe, c, dh, T); + REQUIRE(out.storage != nullptr); + + const vllm::detail::ExpertStreamStats s = vllm::detail::ExpertStreamSnapshot(); + REQUIRE(s.active); // a store was built, so there ARE numbers to print + REQUIRE(s.fills > 0); // and slices really were taken + REQUIRE(s.steps == 0); // and no step ended: the F1 signature, reproduced + + // The child's job ends here. Its remaining line has to come from teardown, so + // it must not call the flush itself. + if (IsFlushChild()) return; + +#if !defined(_WIN32) + // Capture stderr across the flush. Everything this process printed before now + // (the one-off `[expert-stream] ON ...` banner) is outside the redirect, so a + // statistics line inside it can only be the one under test. + std::FILE* cap = std::tmpfile(); + REQUIRE(cap != nullptr); + std::fflush(stderr); + const int saved = ::dup(STDERR_FILENO); + REQUIRE(saved >= 0); + REQUIRE(::dup2(::fileno(cap), STDERR_FILENO) >= 0); + + vllm::detail::ExpertStreamFlushStats(); + + std::fflush(stderr); + REQUIRE(::dup2(saved, STDERR_FILENO) >= 0); + ::close(saved); + + std::rewind(cap); + std::string captured; + char buf[512]; + size_t n = 0; + while ((n = std::fread(buf, 1, sizeof(buf), cap)) > 0) captured.append(buf, n); + std::fclose(cap); + + // EXACTLY ONE line, and it carries the zero. `stats_every_` is 0 here, which + // silences the periodic report entirely, and `steps` is 0, which the periodic + // report skips as well — so both of the early returns that made this line + // unreachable are being crossed at once. + size_t lines = 0; + for (size_t at = captured.find("[expert-stream] steps="); + at != std::string::npos; + at = captured.find("[expert-stream] steps=", at + 1)) + ++lines; + INFO("captured stderr: ", captured); + CHECK(lines == 1); + CHECK(captured.find("[expert-stream] steps=0 ") != std::string::npos); + CHECK(captured.find(" fills=") != std::string::npos); +#endif // !_WIN32 +} + +TEST_CASE("Qwen3_5Model::ForwardDense marks exactly one step") { + if (IsFlushChild()) return; + const HfConfig c = MakeConfig(); + const Qwen3_5MoeWeights w = MakeWeights(c); + vt::Queue q = Q(); + const std::vector ids = {5, 9, 2}; + const std::vector pos = {0, 1, 2}; + + const int64_t before = Steps(); + const std::vector logits = + Qwen3_5Model::ForwardDense(ids, pos, w, c, q); + REQUIRE(logits.size() == + static_cast(ids.size()) * static_cast(c.vocab_size)); + CHECK(Steps() - before == 1); +} + +TEST_CASE("Qwen3_5MTPModel::Forward marks exactly one step") { + if (IsFlushChild()) return; + const HfConfig c = MakeConfig(); + const Qwen3_5MoeWeights target = MakeWeights(c); + const Qwen3_5MTPWeights mtp = MakeMtpWeights(c, 4242); + const Qwen3_5MTPModel model(mtp, target, c); + vt::Queue q = Q(); + + const int64_t T = 3, H = c.hidden_size; + std::vector th(static_cast(T * H)); + for (size_t i = 0; i < th.size(); ++i) + th[i] = vt::F32ToBF16(expert_stream_test::RandV(31 + i)); + const vt::Tensor target_hidden = vt::Tensor::Contiguous( + th.data(), DType::kBF16, vt::Device{vt::DeviceType::kCPU, 0}, {T, H}); + const std::vector ids = {1, 2, 3}; + const std::vector pos = {0, 1, 2}; + + const int64_t before = Steps(); + const vllm::Qwen3_5MTPHiddenStates h = + model.Forward(ids, pos, target_hidden, q); + REQUIRE(h.storage != nullptr); + CHECK(Steps() - before == 1); +} + +TEST_CASE("Qwen3_5MTPModel::ForwardPaged marks exactly one step") { + if (IsFlushChild()) return; + const HfConfig c = MakeConfig(); + const Qwen3_5MoeWeights target = MakeWeights(c); + const Qwen3_5MTPWeights mtp = MakeMtpWeights(c, 909); + const Qwen3_5MTPModel model(mtp, target, c); + vt::Queue q = Q(); + + const int64_t T = 3, H = c.hidden_size; + std::vector th(static_cast(T * H)); + for (size_t i = 0; i < th.size(); ++i) + th[i] = vt::F32ToBF16(expert_stream_test::RandV(57 + i)); + const vt::Tensor target_hidden = vt::Tensor::Contiguous( + th.data(), DType::kBF16, vt::Device{vt::DeviceType::kCPU, 0}, {T, H}); + const std::vector ids = {4, 5, 6}; + const std::vector pos = {0, 1, 2}; + + // The draft KV cache: one full-attention layer's worth, which is all an MTP + // head has. + CachePool pool(c, /*num_blocks=*/4, /*block_size=*/8); + REQUIRE(!pool.attn_kv.empty()); + const std::vector blocks = {0}; + + const int64_t before = Steps(); + const vllm::Qwen3_5MTPHiddenStates h = + model.ForwardPaged(ids, pos, target_hidden, + PrefillAttnMeta(T, blocks, 8, 0), pool.attn_kv[0], q); + REQUIRE(h.storage != nullptr); + CHECK(Steps() - before == 1); +} + +#if defined(__linux__) +TEST_CASE("the final statistics line is wired to process TEARDOWN") { + // The case above proves the flush prints what it should when something calls + // it. This one proves something calls it, which is the part an in-process + // assertion cannot reach: the flush runs from a static destructor, after + // doctest's main has returned. + // + // So it runs THIS BINARY again as a child with `VT_ES_FLUSH_CHILD` set. In + // that mode every case returns early except the first, which builds a store, + // takes slices, ends no step and — crucially — does NOT call the flush. Any + // statistics line in the child's output therefore came from teardown, and + // `VT_MOE_EXPERT_STREAM_STATS_EVERY=0` rules out the periodic report. + // + // /proc/self/exe rather than argv[0], because doctest's main owns argv and a + // relative argv[0] would depend on the working directory ctest chose. It is + // RESOLVED here rather than handed to the shell: `popen` runs `/bin/sh`, so a + // literal /proc/self/exe in the command line names the SHELL and the child + // would print nothing at all — which looks exactly like the defect. + if (IsFlushChild()) return; + + char exe[4096]; + const ssize_t len = ::readlink("/proc/self/exe", exe, sizeof(exe) - 1); + REQUIRE(len > 0); + exe[len] = '\0'; + const std::string cmd = + std::string("VT_ES_FLUSH_CHILD=1 '") + exe + "' 2>&1"; + + std::FILE* child = ::popen(cmd.c_str(), "r"); + REQUIRE(child != nullptr); + std::string out; + char buf[512]; + size_t n = 0; + while ((n = std::fread(buf, 1, sizeof(buf), child)) > 0) out.append(buf, n); + const int status = ::pclose(child); + + INFO("child output: ", out); + CHECK(status == 0); + + // The child really ran, and really built a store. Without this a filter or a + // crash that produced no output at all would read as "one line, absent", + // which is the same shape as the defect. + CHECK(out.find("[expert-stream] ON slots=") != std::string::npos); + CHECK(out.find("0 failed") != std::string::npos); + + size_t lines = 0; + for (size_t at = out.find("[expert-stream] steps="); + at != std::string::npos; + at = out.find("[expert-stream] steps=", at + 1)) + ++lines; + CHECK(lines == 1); + CHECK(out.find("[expert-stream] steps=0 ") != std::string::npos); + + // And it carries the STORE's numbers. The child filled slots, so a line + // reporting `fills=0` would mean something other than the store printed it — + // exactly what a well-meaning second teardown hook would produce, and it would + // otherwise satisfy every assertion above. + CHECK(out.find(" fills=0 ") == std::string::npos); +} +#endif // __linux__ + +TEST_CASE("Qwen3_5ReplayLayer marks exactly one step") { + if (IsFlushChild()) return; + const HfConfig c = MakeConfig(); + const Qwen3_5MoeWeights w = MakeWeights(c); + vt::Queue q = Q(); + + const int64_t T = 2, H = c.hidden_size; + std::vector hidden_in(static_cast(T * H)); + for (size_t i = 0; i < hidden_in.size(); ++i) + hidden_in[i] = expert_stream_test::RandV(99 + i); + const std::vector pos = {0, 1}; + + const int64_t before = Steps(); + const std::vector out = + vllm::Qwen3_5ReplayLayer(w.layers[3], c, hidden_in, pos, T, q); + REQUIRE(out.size() == hidden_in.size()); + CHECK(Steps() - before == 1); +} + +// ───────────────────────────────────────────────────────────────────────────── +// The nesting refusal, which every case above depends on and none of them can +// reach. +// +// WHY IT NEEDED ITS OWN CASE. "One forward is one step, and the guard REFUSES +// to nest" was stated in the source, in the spec and in the pull request body, +// and deleting the `VT_CHECK` that implements it left BOTH focused binaries +// fully green — 6/6 and 4/4. That is the same shape as every other finding this +// row has carried: an asserted guarantee no gate could see. It is unreachable +// through production code by construction, because every forward that takes +// expert slices is a complete forward that no other one contains, so there is +// no legitimate call graph that nests one. `detail::ExpertStreamStepScope` +// exists for exactly this, and it forwards to the guard's own `Begin`/`End` +// rather than re-stating the flag, so what is measured here is the production +// boundary and not a copy of it. +// ───────────────────────────────────────────────────────────────────────────── +TEST_CASE("the step guard REFUSES to nest, and the refusal reaches a real forward") { + if (IsFlushChild()) return; + const HfConfig c = MakeConfig(); + const Qwen3_5MoeWeights w = MakeWeights(c); + vt::Queue q = Q(); + const std::vector ids = {5, 9, 2}; + const std::vector pos = {0, 1, 2}; + + const int64_t before = Steps(); + { + const vllm::detail::ExpertStreamStepScope outer; + + // A SECOND scope on this thread is refused by name. Not `CHECK_THROWS_WITH_AS`: + // VT_CHECK appends " at :", so an exact-message match would + // break on any edit above it and say nothing about the guarantee. + bool nested_threw = false; + std::string nested_what; + try { + const vllm::detail::ExpertStreamStepScope inner; + (void)inner; + } catch (const std::runtime_error& e) { + nested_threw = true; + nested_what = e.what(); + } + CHECK(nested_threw); + CHECK(nested_what.find("must not nest") != std::string::npos); + + // AND THE SCOPE IS THE PRODUCTION GUARD, not a parallel flag. A real + // forward entered while the scope is held is refused too — which is the + // only way to show that the two share a boundary, and which also measures + // the breadth of the refusal: it is armed on the DEFAULT path, so a nest + // reds every Qwen3.5 forward and not merely the streamed ones. That is the + // intended polarity (the note on `Qwen35ExpertStreamStep` argues it): a + // nest is a defect in the call graph whether or not a store exists, and + // arming it only on the rare configuration would let the default path + // establish one that nobody sees until streaming is switched on. + bool forward_threw = false; + std::string forward_what; + try { + (void)Qwen3_5Model::ForwardDense(ids, pos, w, c, q); + } catch (const std::runtime_error& e) { + forward_threw = true; + forward_what = e.what(); + } + CHECK(forward_threw); + CHECK(forward_what.find("must not nest") != std::string::npos); + } + + // The refusal is NOT sticky. A constructor that throws leaves no object, so + // no destructor runs and no step is charged for it; the outer scope closed + // exactly one. Two refused opens plus one closed scope must therefore be one + // step, and the same forward must now succeed — a guard that leaked its flag + // on the throw would fail here rather than at some unrelated later case. + CHECK(Steps() - before == 1); + const std::vector logits = Qwen3_5Model::ForwardDense(ids, pos, w, c, q); + CHECK(logits.size() == + static_cast(ids.size()) * static_cast(c.vocab_size)); + CHECK(Steps() - before == 2); +} diff --git a/tests/vllm/model_executor/test_expert_stream_wiring.cpp b/tests/vllm/model_executor/test_expert_stream_wiring.cpp index 90b3eda40..4fbd877b6 100644 --- a/tests/vllm/model_executor/test_expert_stream_wiring.cpp +++ b/tests/vllm/model_executor/test_expert_stream_wiring.cpp @@ -40,314 +40,70 @@ #include +#if !defined(_WIN32) +#include // ::fileno, for the pread case below +#endif + #include #include +#include +#include #include +#include #include +#include "support/expert_stream_model.h" +#include "support/test_env.h" #include "vllm/model_executor/models/qwen3_5.h" #include "vllm/model_executor/models/qwen3_5_internal.h" #include "vllm/model_executor/models/qwen3_5_weights.h" -#include "vllm/transformers_utils/hf_config.h" -#include "vllm/v1/attention/backend.h" -#include "vllm/v1/attention/backends/gdn_attn.h" -#include "vt/backend.h" -#include "vt/dtype.h" -#include "vt/tensor.h" - -using vllm::GdnStateCache; + +using expert_stream_test::CachePool; +using expert_stream_test::MakeConfig; +using expert_stream_test::MakeWeights; +using expert_stream_test::PrefillAttnMeta; +using expert_stream_test::PrefillGdnMeta; +using expert_stream_test::Q; using vllm::HfConfig; -using vllm::PagedKvCache; using vllm::Qwen3_5MoeWeights; using vllm::Qwen3_5Model; -using vllm::v1::CommonAttentionMetadata; -using vllm::v1::GDNAttentionMetadata; -using vt::DType; namespace { // Turn the lane on BEFORE anything can read the environment. The read happens in // a function-local static on the first slice, so setting it inside a test body // would work today and break the moment a case ordering changed. +// +// Through `vllm_test::SetEnv` and not `::setenv`. `setenv(3)` is POSIX, MSVC's +// CRT has only `_putenv_s`, and this file is compiled on Windows too: the target +// is added unconditionally and `scripts/build-windows-release.ps1` configures +// `VLLM_CPP_BUILD_TESTS=ON`. The shim in `support/test_env.h` is the one place +// that branch lives (#603). CI cannot currently see the difference, because the +// Windows lanes fail earlier in the product library on #1068 and never reach a +// test translation unit at all. struct EnableExpertStreaming { EnableExpertStreaming() { - ::setenv("VT_MOE_EXPERT_STREAM", "1", 1); + vllm_test::SetEnv("VT_MOE_EXPERT_STREAM", "1"); // Comfortably more than one step's working set: 4 experts x 3 towers x 4 // layers = 48 distinct slices per forward. A budget BELOW that would make // `exhausted` nonzero for an honest reason and mask the defect under test. - ::setenv("VT_MOE_EXPERT_STREAM_SLOTS", "64", 1); - ::setenv("VT_MOE_EXPERT_STREAM_SLOT_BYTES", "8192", 1); - // Quiet under ctest, but overwrite=0 so an operator who sets this var - // still gets the line -- which is the only way to SEE the statistics - // this row added, and a gate that suppresses its own evidence is a - // smaller version of the defect it was written for. - ::setenv("VT_MOE_EXPERT_STREAM_STATS_EVERY", "0", 0); // quiet under ctest + vllm_test::SetEnv("VT_MOE_EXPERT_STREAM_SLOTS", "64"); + vllm_test::SetEnv("VT_MOE_EXPERT_STREAM_SLOT_BYTES", "8192"); + // Quiet under ctest, but only when the operator has not asked otherwise -- + // seeing the line is the only way to SEE the statistics this row added, and + // a gate that suppresses its own evidence is a smaller version of the defect + // it was written for. `vllm_test::SetEnv` has no overwrite=0 form (it is a + // two-argument shim on purpose), so the condition is stated here. + if (std::getenv("VT_MOE_EXPERT_STREAM_STATS_EVERY") == nullptr) + vllm_test::SetEnv("VT_MOE_EXPERT_STREAM_STATS_EVERY", "0"); // The grouped keep-quant path stages the whole tower and cannot stream; the // production code already disables it when streaming is requested. Being // explicit here keeps the test honest about which path it is measuring. - ::setenv("VT_QWEN35_GROUPED_MOE", "0", 1); + vllm_test::SetEnv("VT_QWEN35_GROUPED_MOE", "0"); } }; const EnableExpertStreaming kEnableExpertStreaming; -float RandV(uint64_t s) { - s = s * 6364136223846793005ULL + 1442695040888963407ULL; - s ^= s >> 33; - return (static_cast((s >> 40) & 0xFFFF) / 32768.0f) - 1.0f; -} - -vllm::OwnedTensor MakeOwned(DType dt, const std::vector& shape, uint64_t seed) { - vllm::OwnedTensor t; - t.dtype = dt; - t.rank = static_cast(shape.size()); - int64_t n = 1; - for (int i = 0; i < t.rank; ++i) { - t.shape[i] = shape[static_cast(i)]; - n *= shape[static_cast(i)]; - } - if (dt == DType::kBF16) { - std::vector b(static_cast(n) * 2); - auto* p = reinterpret_cast(b.data()); - for (int64_t i = 0; i < n; ++i) p[i] = vt::F32ToBF16(RandV(seed + static_cast(i))); - t.bytes = vllm::OwnedBytes(std::move(b)); - } else { - std::vector b(static_cast(n) * 4); - auto* p = reinterpret_cast(b.data()); - for (int64_t i = 0; i < n; ++i) p[i] = RandV(seed + static_cast(i)); - t.bytes = vllm::OwnedBytes(std::move(b)); - } - return t; -} - -// A keep-quant STACKED expert tower: [rows, cols] Q8_0, `nk = true`, exactly the -// shape the GGUF keep-quant loader produces and the only shape KqExpertSlice -// slices. -// -// The blocks are BUILT, not filled with noise. A Q8_0 block is an fp16 scale -// followed by 32 int8 weights, and random bytes put random bit patterns in the -// scale — including the fp16 encodings of inf and NaN, which propagate straight -// through the GEMM and make every later comparison vacuous. The values are -// arbitrary but well-formed, which is all this test needs: every arm decodes the -// SAME bytes, so equality between arms is a real comparison. -vllm::OwnedTensor MakeKqTower(int64_t rows, int64_t cols, uint64_t seed) { - vllm::OwnedTensor t; - t.dtype = DType::kQ8_0; - t.nk = true; - t.rank = 2; - t.shape[0] = rows; - t.shape[1] = cols; - const size_t row_bytes = vt::RowSizeBytes(DType::kQ8_0, cols); - const int64_t blocks_per_row = cols / 32; - REQUIRE(cols % 32 == 0); // Q8_0 is a 32-element block quant - REQUIRE(row_bytes == static_cast(blocks_per_row) * 34); - std::vector b(static_cast(rows) * row_bytes); - size_t o = 0; - for (int64_t r = 0; r < rows; ++r) { - for (int64_t blk = 0; blk < blocks_per_row; ++blk) { - const uint16_t d = vt::F32ToF16(0.004f + 0.001f * RandV(seed + static_cast(r * 131 + blk))); - std::memcpy(b.data() + o, &d, 2); - o += 2; - for (int j = 0; j < 32; ++j) { - const int8_t q = static_cast( - static_cast(100.0f * RandV(seed + static_cast((r * 131 + blk) * 32 + j)))); - std::memcpy(b.data() + o, &q, 1); - o += 1; - } - } - } - t.bytes = vllm::OwnedBytes(std::move(b)); - return t; -} - -HfConfig MakeConfig() { - HfConfig c; - c.model_type = "qwen3_5_moe_text"; - c.architectures = {"Qwen3_5MoeForConditionalGeneration"}; - c.hidden_size = 32; - c.num_hidden_layers = 4; - c.vocab_size = 40; - c.num_attention_heads = 4; - c.num_key_value_heads = 2; - c.head_dim = 8; - c.layer_types = {"linear_attention", "linear_attention", "linear_attention", - "full_attention"}; - c.num_experts = 4; - c.num_experts_per_tok = 2; - c.moe_intermediate_size = 32; - c.shared_expert_intermediate_size = 16; - c.linear_num_key_heads = 2; - c.linear_num_value_heads = 4; - c.linear_key_head_dim = 8; - c.linear_value_head_dim = 8; - c.linear_conv_kernel_dim = 4; - c.rope_theta = 10000.0; - c.rotary_dim = 4; - c.rms_norm_eps = 1e-6; - c.max_position_embeddings = 64; - return c; -} - -vllm::MoeBlockWeights MakeKqMoe(const HfConfig& c, uint64_t s) { - vllm::MoeBlockWeights m; - const int64_t H = c.hidden_size, E = c.num_experts, I = c.moe_intermediate_size, - Is = c.shared_expert_intermediate_size; - m.router_gate = MakeOwned(DType::kBF16, {H, E}, s + 1); - m.shared_gate = MakeOwned(DType::kBF16, {H, 1}, s + 2); - // The routed experts are STACKED keep-quant towers, and the per-expert vectors - // stay empty — the A3 layout the streaming seam is defined against. - m.expert_gate_kq = MakeKqTower(E * I, H, s + 100); - m.expert_up_kq = MakeKqTower(E * I, H, s + 200); - m.expert_down_kq = MakeKqTower(E * H, I, s + 300); - m.shared_gate_proj = MakeOwned(DType::kBF16, {H, Is}, s + 3); - m.shared_up_proj = MakeOwned(DType::kBF16, {H, Is}, s + 4); - m.shared_down_proj = MakeOwned(DType::kBF16, {Is, H}, s + 5); - return m; -} - -Qwen3_5MoeWeights MakeWeights(const HfConfig& c, uint64_t base_seed = 0) { - Qwen3_5MoeWeights w; - const int64_t H = c.hidden_size, V = c.vocab_size; - const int64_t Hq = c.num_attention_heads, Hkv = c.num_key_value_heads, - Dh = c.head_dim; - const int64_t Hk = c.linear_num_key_heads, Hv = c.linear_num_value_heads, - Dk = c.linear_key_head_dim, Dv = c.linear_value_head_dim, - Kw = c.linear_conv_kernel_dim; - const int64_t key_dim = Hk * Dk, value_dim = Hv * Dv, - conv_dim = 2 * key_dim + value_dim; - w.embed_tokens = MakeOwned(DType::kBF16, {V, H}, 11); - w.final_norm = MakeOwned(DType::kBF16, {H}, 12); - w.lm_head = MakeOwned(DType::kBF16, {H, V}, 13); - for (int64_t l = 0; l < c.num_hidden_layers; ++l) { - const uint64_t s = base_seed + 1000 + static_cast(l) * 5000; - vllm::Qwen3_5MoeLayerWeights lw; - lw.is_linear_attention = (c.layer_types[static_cast(l)] == "linear_attention"); - lw.input_layernorm = MakeOwned(DType::kBF16, {H}, s + 1); - lw.post_attention_layernorm = MakeOwned(DType::kBF16, {H}, s + 2); - if (lw.is_linear_attention) { - lw.gdn.in_proj_qkv = MakeOwned(DType::kBF16, {H, conv_dim}, s + 10); - lw.gdn.in_proj_z = MakeOwned(DType::kBF16, {H, value_dim}, s + 20); - lw.gdn.in_proj_b = MakeOwned(DType::kBF16, {H, Hv}, s + 30); - lw.gdn.in_proj_a = MakeOwned(DType::kBF16, {H, Hv}, s + 40); - lw.gdn.conv1d_weight = MakeOwned(DType::kBF16, {conv_dim, Kw}, s + 50); - lw.gdn.a_log = MakeOwned(DType::kF32, {Hv}, s + 60); - lw.gdn.dt_bias = MakeOwned(DType::kF32, {Hv}, s + 70); - lw.gdn.norm_weight = MakeOwned(DType::kBF16, {Dv}, s + 80); - lw.gdn.out_proj = MakeOwned(DType::kBF16, {value_dim, H}, s + 90); - } else { - lw.attn.q_proj = MakeOwned(DType::kBF16, {H, 2 * Hq * Dh}, s + 10); - lw.attn.k_proj = MakeOwned(DType::kBF16, {H, Hkv * Dh}, s + 20); - lw.attn.v_proj = MakeOwned(DType::kBF16, {H, Hkv * Dh}, s + 30); - lw.attn.o_proj = MakeOwned(DType::kBF16, {Hq * Dh, H}, s + 40); - lw.attn.q_norm = MakeOwned(DType::kBF16, {Dh}, s + 50); - lw.attn.k_norm = MakeOwned(DType::kBF16, {Dh}, s + 60); - } - lw.moe = MakeKqMoe(c, s + 500); - w.layers.push_back(std::move(lw)); - } - return w; -} - -struct CachePool { - const HfConfig& c; - int64_t num_blocks; - int64_t block_size; - std::vector> full_attn_buf; - std::vector> gdn_ssm_buf; - std::vector> gdn_conv_buf; - std::vector attn_kv; - std::vector gdn_state; - - CachePool(const HfConfig& cfg, int64_t nb, int64_t bs) - : c(cfg), num_blocks(nb), block_size(bs) { - const int64_t Hkv = c.num_key_value_heads, Dh = c.head_dim; - const int64_t Hv = c.linear_num_value_heads, Dv = c.linear_value_head_dim, - Dk = c.linear_key_head_dim, Kw = c.linear_conv_kernel_dim; - const int64_t key_dim = c.linear_num_key_heads * Dk, value_dim = Hv * Dv; - const int64_t conv_dim = 2 * key_dim + value_dim; - for (int64_t l = 0; l < c.num_hidden_layers; ++l) { - if (c.layer_types[static_cast(l)] == "linear_attention") { - gdn_ssm_buf.emplace_back(static_cast(nb * Hv * Dv * Dk), 0.0f); - gdn_conv_buf.emplace_back(static_cast(nb * conv_dim * (Kw - 1)), 0.0f); - } else { - full_attn_buf.emplace_back(static_cast(nb * 2 * bs * Hkv * Dh), 0.0f); - } - } - Rebind(); - } - - void Rebind() { - const int64_t Hkv = c.num_key_value_heads, Dh = c.head_dim; - const int64_t Hv = c.linear_num_value_heads, Dv = c.linear_value_head_dim, - Dk = c.linear_key_head_dim, Kw = c.linear_conv_kernel_dim; - const int64_t key_dim = c.linear_num_key_heads * Dk, value_dim = Hv * Dv; - const int64_t conv_dim = 2 * key_dim + value_dim; - attn_kv.clear(); - gdn_state.clear(); - for (auto& b : full_attn_buf) { - PagedKvCache kv; - kv.data = b.data(); - kv.dtype = DType::kF32; - kv.num_blocks = num_blocks; - kv.block_size = block_size; - kv.num_kv_heads = Hkv; - kv.head_size = Dh; - attn_kv.push_back(kv); - } - for (size_t g = 0; g < gdn_ssm_buf.size(); ++g) { - GdnStateCache gs; - gs.ssm_state = vt::Tensor::Contiguous(gdn_ssm_buf[g].data(), DType::kF32, - vt::Device{vt::DeviceType::kCPU, 0}, - {num_blocks, Hv, Dv, Dk}); - gs.conv_state = vt::Tensor::Contiguous(gdn_conv_buf[g].data(), DType::kF32, - vt::Device{vt::DeviceType::kCPU, 0}, - {num_blocks, conv_dim, Kw - 1}); - gdn_state.push_back(gs); - } - } -}; - -vt::Queue Q() { return vt::Queue{vt::Device{vt::DeviceType::kCPU, 0}, nullptr}; } - -CommonAttentionMetadata PrefillAttnMeta(int64_t T, const std::vector& blocks, - int64_t block_size, int64_t start_slot) { - CommonAttentionMetadata m; - m.num_reqs = 1; - m.num_actual_tokens = static_cast(T); - m.query_start_loc = {0, static_cast(T)}; - m.query_start_loc_cpu = m.query_start_loc; - m.seq_lens = {static_cast(T)}; - m.seq_lens_cpu = m.seq_lens; - m.max_query_len = static_cast(T); - m.max_seq_len = static_cast(T); - m.block_table_num_cols = static_cast(blocks.size()); - m.block_table_tensor = blocks; - for (int64_t t = 0; t < T; ++t) { - const int64_t blk = blocks[static_cast(t / block_size)]; - m.slot_mapping.push_back(blk * block_size + (start_slot + t) % block_size); - } - m.causal = true; - return m; -} - -GDNAttentionMetadata PrefillGdnMeta(int64_t T, int32_t sidx) { - GDNAttentionMetadata g; - g.num_prefills = 1; - g.num_prefill_tokens = static_cast(T); - g.num_decodes = 0; - g.num_decode_tokens = 0; - g.num_actual_tokens = static_cast(T); - g.has_initial_state = std::vector{0}; - g.non_spec_state_indices_tensor = std::vector{sidx}; - g.non_spec_query_start_loc = std::vector{0, static_cast(T)}; - g.prefill_query_start_loc = std::vector{0, static_cast(T)}; - g.prefill_state_indices = std::vector{sidx}; - g.prefill_has_initial_state = std::vector{0}; - const auto conv = - vllm::v1::ComputeCausalConv1dMetadata(*g.non_spec_query_start_loc); - g.batch_ptr = conv.batch_ptr; - g.token_chunk_offset_ptr = conv.token_chunk_offset_ptr; - return g; -} // One full paged forward through the PRODUCTION entry point, over a fresh cache // so every call is independent. @@ -408,7 +164,7 @@ TEST_CASE("decode REACHES the expert streamer, and the step clock advances") { CHECK(s.exhausted == 0); #if defined(__unix__) - // F5: the MADV_WILLNEED hint is ACCEPTED, not merely issued. + // F5: the MADV_WILLNEED hint is ACCEPTED on EVERY fill, not merely issued. // // madvise(2) returns EINVAL on an address that is not page-aligned, and GGUF // tensor data is aligned to `general.alignment`, default 32 @@ -417,7 +173,41 @@ TEST_CASE("decode REACHES the expert streamer, and the step clock advances") { // discarded, which is a hint that never fired and never said so. This counts // only the calls the kernel took. // + // WHY `== fills` AND NOT `> 0` (#1091 finding 2). `> 0` over 48 calls is + // satisfied whenever heap layout happens to page-align a single slice, and + // measured here it is: reinjecting the pre-fix unaligned address exits 0 in + // 40 of 40 runs against `> 0`, so the assertion the fix shipped with cannot + // fail for the defect it names. The equality can: 0 != 48. + // + // AND `fills` IS THE RIGHT DENOMINATOR, not a literal 48. madvise runs on the + // mapping-copy arm only (the pread arm needs no readahead hint) and only when + // the key is NOT already resident, which is exactly the condition under which + // `EnsureSpan` goes on to fill. The two counters therefore move together for + // as long as nothing is refused, and `exhausted == 0` above is that premise + // asserted. Every weight in this test owns its bytes (`mmap_fd == -1`), so + // every fill here is a span fill. + // // NO SPEEDUP IS ASSERTED, here or anywhere. This says the call is well formed. + // + // TWO RESIDUALS THE EQUALITY RESTS ON, stated rather than left to be + // rediscovered. + // + // (1) `Slice` rounds the advised range's END UP to a page, which for a + // heap-backed tower goes past the allocation. madvise(2) returns ENOMEM if + // any page in the range is unmapped, so this holds because the allocator's + // arena page is mapped, not because the arithmetic guarantees it. Production + // towers are file mappings many pages larger than a slice and do not have the + // question. If this ever fails with `advised` short by a small count, that is + // the first thing to check, not the fill path. + // + // (2) The counters are CUMULATIVE over the process, so this equality is a + // statement about everything that ran BEFORE it — and the pread case at the + // end of this file fills without advising, which would break it. The order + // holds under doctest's default file order, and it is not left implicit: the + // `CHECK_FALSE(s0.active)` at the top of this case fails loudly if anything + // ran first, so a reordering shows up as that assertion rather than as a + // confusing `advised != fills` here. + CHECK(s.advised == s.fills); CHECK(s.advised > 0); #endif } @@ -450,7 +240,15 @@ TEST_CASE("a streamed slice and the tower view produce IDENTICAL logits") { // The unstreamed arm really did NOT stream: no new bytes moved, and every // slice it asked for was refused into the fallback. CHECK(off.fills == fills_after_streamed); - CHECK(off.exhausted > 0); + CHECK(off.forced > on.forced); + + // AND IT WAS NOT COUNTED AS A BUDGET REFUSAL (#1091 finding 6). `exhausted` is + // the operator-facing number, documented as "the budget is smaller than one + // step's working set". The forced-fallback switch has no production caller, so + // every increment it contributed to `exhausted` was a gate telling an operator + // that a knob they never turned is too small. The budget here is 64 slots + // against 48 slices and nothing was ever genuinely refused, so this stays 0. + CHECK(off.exhausted == 0); // BIT-EXACT, not close. The slot holds a byte copy of the same tower bytes, so // the two arms feed the kernel identical inputs; anything but equality means @@ -505,3 +303,88 @@ TEST_CASE("a SECOND model does not inherit the first model's slots") { if (!(streamed[i] == truth[i])) ++differing; CHECK(differing == 0); } + +#if !defined(_WIN32) +TEST_CASE("a FILE-backed tower is served by pread, at file_offset + slice offset") { + // #1091 finding 4: `ExpertStreamer::EnsureFile` is the arm every REAL GGUF + // checkpoint takes, because a borrowed mmap tower carries a descriptor, and + // no test reached it. The other cases in this file build owned host vectors, + // so `w.mmap_fd` is -1 and every one of them exercises `EnsureSpan` instead. + // The spec's `## Owed` framed this as unmeasured on the model, which it also + // is; it was additionally UNREACHED, and that part needs neither the box nor + // the 370 GiB checkpoint. + // + // WHAT IS ACTUALLY UNVERIFIED HERE is the address arithmetic. `Slice` preads + // at `file_offset + offset`: the tensor's own position in the shard plus the + // routed expert's row offset within the tower. Get either term wrong and the + // read still succeeds, still returns exactly the bytes asked for, and the GEMM + // multiplies a different expert -- the "wrong shard at a plausible offset" + // shape F4 named. So the tower is written at a deliberately AWKWARD file + // offset: nonzero, not page-aligned and not a multiple of the 34-byte Q8_0 + // block, which makes dropping the term or rounding it detectable. + const HfConfig c = MakeConfig(); + Qwen3_5MoeWeights w = MakeWeights(c, /*base_seed=*/31000); + + std::FILE* f = std::tmpfile(); + REQUIRE(f != nullptr); + const int fd = ::fileno(f); + REQUIRE(fd >= 0); + + // 4109 = 4096 + 13: past a page, not on a page, not on a Q8_0 block. + size_t at = 4109; + const std::vector pad(at, 0xA5); + REQUIRE(std::fwrite(pad.data(), 1, pad.size(), f) == pad.size()); + + // Keep-alives for the borrowed views. A borrow with no owner is exactly the + // dangling view OwnedBytes exists to make unrepresentable. + std::vector>> holds; + + auto to_file_backed = [&](vllm::OwnedTensor& t) { + auto hold = std::make_shared>(t.bytes.begin(), + t.bytes.end()); + REQUIRE(std::fwrite(hold->data(), 1, hold->size(), f) == hold->size()); + t.bytes = vllm::OwnedBytes::Borrow(hold->data(), hold->size(), hold); + t.mmap_fd = fd; + t.mmap_file_offset = at; + at += hold->size(); + holds.push_back(std::move(hold)); + }; + for (auto& layer : w.layers) { + to_file_backed(layer.moe.expert_gate_kq); + to_file_backed(layer.moe.expert_up_kq); + to_file_backed(layer.moe.expert_down_kq); + } + REQUIRE(std::fflush(f) == 0); // the pread must see the bytes, not the buffer + + const std::vector ids = {13, 6, 28, 2}; + + const vllm::detail::ExpertStreamStats before = + vllm::detail::ExpertStreamSnapshot(); + const std::vector streamed = OneForward(c, w, ids); + const vllm::detail::ExpertStreamStats after = + vllm::detail::ExpertStreamSnapshot(); + + // THE ARM IS PROVEN, not assumed. Slices really were filled, and NOT ONE of + // them was advised: `Slice` issues MADV_WILLNEED only on the mapping-copy arm, + // because a pread needs no readahead hint. Equal `advised` across a forward + // that filled slots is therefore the signature of the pread path, and it is + // the one number that separates it from EnsureSpan. + CHECK(after.fills > before.fills); + CHECK(after.advised == before.advised); + + // The ground truth for the same weights, read straight from the borrowed host + // bytes through the fallback. Those bytes and the file's are the same bytes by + // construction, so any difference is the pread landing somewhere else. + vllm::detail::ExpertStreamSetForceFallback(true); + const std::vector truth = OneForward(c, w, ids); + vllm::detail::ExpertStreamSetForceFallback(false); + + REQUIRE(streamed.size() == truth.size()); + size_t differing = 0; + for (size_t i = 0; i < streamed.size(); ++i) + if (!(streamed[i] == truth[i])) ++differing; + CHECK(differing == 0); + + std::fclose(f); +} +#endif // !_WIN32 diff --git a/tests/vllm/test_qwen36_weights.cpp b/tests/vllm/test_qwen36_weights.cpp index 61c4fcb19..84de88e0f 100644 --- a/tests/vllm/test_qwen36_weights.cpp +++ b/tests/vllm/test_qwen36_weights.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -716,9 +717,48 @@ TEST_CASE("a tower's identity survives an address the allocator hands out again" vllm::OwnedTensor c = a; CHECK(c.TowerUid() != uid_a); - // And re-stamping is keyed on the buffer, so replacing the bytes yields a new - // identity rather than silently reusing the old one. + // And re-stamping is keyed on the buffer, so replacing the bytes with a + // DIFFERENTLY ADDRESSED buffer yields a new identity rather than silently + // reusing the old one. const uint64_t uid_c = c.TowerUid(); c.bytes = vllm::OwnedBytes(std::vector(128, 0x33)); + REQUIRE(c.bytes.data() != nullptr); CHECK(c.TowerUid() != uid_c); } + +TEST_CASE("a tower's identity is keyed on its ADDRESS, and says so") { + // #1091 finding 5. The field comment used to promise an identity for "this + // tensor's CURRENT bytes"; the implementation keys on `bytes.data()` and + // re-stamps only when that address moves. The two are not the same claim, and + // the case above cannot tell them apart because a fresh `std::vector` lands + // somewhere else and so satisfies both readings. + // + // This one separates them, deterministically, by borrowing: a borrowed view + // names an address the test controls, so "same address, different contents" + // is constructible rather than a matter of allocator luck. #1066 was a comment + // on this exact field that promised more than the code delivered, so the limit + // is pinned here rather than left to be rediscovered. + auto one = std::make_shared>(64, 0x44); + auto two = std::make_shared>(64, 0x55); + + vllm::OwnedTensor t; + t.dtype = vt::DType::kI8; + t.rank = 1; + t.shape[0] = 64; + t.bytes = vllm::OwnedBytes::Borrow(one->data(), one->size(), one); + const uint64_t uid = t.TowerUid(); + CHECK(uid != 0u); + + // Same address, wholly different contents. The uid does NOT move — which is + // the documented limit, not a defect, because nothing rewrites a tower in + // place. A future caller that did would need a different key. + std::fill(one->begin(), one->end(), 0x99); + CHECK(t.bytes.data() == one->data()); + CHECK(t.TowerUid() == uid); + + // A different address is a different tower, which is the half the cache + // depends on. + t.bytes = vllm::OwnedBytes::Borrow(two->data(), two->size(), two); + REQUIRE(t.bytes.data() != one->data()); + CHECK(t.TowerUid() != uid); +}