diff --git a/.agents/roadmap_v1.md b/.agents/roadmap_v1.md index b82d22b5c..ce9c3209a 100644 --- a/.agents/roadmap_v1.md +++ b/.agents/roadmap_v1.md @@ -207,8 +207,8 @@ issue is not yet placed. Keyed record: update in place, never append. | [#758](https://github.com/mudler/vllm.cpp/issues/758) | `ENG-MM-INPUT-PIPELINE` | A multimodal refusal cannot distinguish a configured limit from an UNIMPLEMENTED arm. `Qwen3VLChatSupportedMmLimits()` declares the seam's honest ceiling `{"image": 1}` with video/audio absent, but the message a client gets is upstream's generic `At most 0 video(s) may be provided in one prompt.` — identical to what `--limit-mm-per-prompt '{"video": 0}'` produces. AGENTS.md requires an unimplemented arm be refused "with a message naming the missing piece"; #749 claimed the ceiling satisfies that and its review found it does not. The only present signal is by OMISSION (`ValidateNumItems` withholds the `--limit-mm-per-prompt` hint when raising the limit would not help). Not fixed in flow: naming the arm diverges from a verbatim-ported message three suites assert byte-for-byte, so it needs its own spec and fresh review. Found in the #749 review round (#607 wave L2, #686) | bug | | [#651](https://github.com/mudler/vllm.cpp/issues/651) | — | `test_agent_record`'s MODEL-ratchet docstring is two contradictory paragraphs spliced together, and the surviving half records a pin transition that never happened | bug | | [#652](https://github.com/mudler/vllm.cpp/issues/652) | — | `model-matrix.md` prose counters drifted: LTX-2.5 reached the rows and the CI-enforced rollup but none of the five sentences that count them | bug | -| [#659](https://github.com/mudler/vllm.cpp/issues/659) | — | LTX-2.5 device select adopts M3a's platform seam but not its companion capability guard: `ltx2_video.cpp` asks `CurrentPlatform().device_type()` and `TryGetBackend(...)` but never `supports_model_architecture`, so a PARTIAL backend (Metal 15/75 ops, Tenstorrent) is handed a queue and dies in a kernel bind where it used to be refused BY NAME (found while reviewing #553 for landing) | bug | -| [#660](https://github.com/mudler/vllm.cpp/issues/660) | — | `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553) | bug | +| [#659](https://github.com/mudler/vllm.cpp/issues/659) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. LTX-2.5 device select adopts M3a's platform seam but not its companion capability guard: `ltx2_video.cpp` asks `CurrentPlatform().device_type()` and `TryGetBackend(...)` but never `supports_model_architecture`, so a PARTIAL backend (Metal 15/75 ops, Tenstorrent) is handed a queue and dies in a kernel bind where it used to be refused BY NAME (found while reviewing #553 for landing). Guard added at the same site; `tests/vllm/multimodal/test_diffusion_device_seam.cpp` holds it against an injected declining platform, and `test_minimax_h3_video_fold.cpp`'s own predicate asks all THREE questions — asking two made it red on a correct refusal, i.e. on exactly the partial-backend build this issue exists to serve | bug | +| [#660](https://github.com/mudler/vllm.cpp/issues/660) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226 @ 11cc1d589`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553). New `dev_cast` bucket, with **29** individually-asserted RED spellings that are not in the tree (counted, not quoted: M20-M24, M29-M33, M35-M39, M41-M44) — including the literal operand `(vt::DeviceType)1`, which the first cut MISSED while catching `(vt::DeviceType)d`, and whose own discriminator test used an identifier and so could not see the gap it was written to rule out. Rounds 2-4 each found the same shape once more IN THE MESSAGE: round 2, nine spellings that wrote the target type the docstring named as its anchor; round 4, eight more — a C-style REFERENCE and POINTER target, the DECLARATION form's cv-qualifiers in three spellings (a FOURTH cv-position, while M36 asserted "all three"), `bit_cast`'s two-argument form, a cv-qualifier inside the pointer run, and `__builtin_bit_cast`. All compile-verified legal, all now RED, measured 0 new / 0 lost over the scan roots (**760** files at `79ebbce42`, **765** after the merge — the count rots on every unrelated PR, so it carries its SHA) with `\bDeviceType\b`=162 and `\bkCUDA\b`=18 as positive controls in the same pass, unchanged across the merge; `sizeof (vt::DeviceType) + 1` was a pre-existing FALSE positive and is now excluded. The docstring no longer claims the property: it enumerates the spellings it matches, states each blind spot with a reason true of that entry alone (`*(vt::DeviceType*)&x`, a character-literal operand, `memcpy`), and names the residual as a property a text checker cannot decide — [#828](https://github.com/mudler/vllm.cpp/issues/828) tracks the AST-level check. M34/M40/M45 pin the negatives; M46 pins that the declared blind spots are still blind, so message and pattern cannot drift apart silently | bug | | [#674](https://github.com/mudler/vllm.cpp/issues/674) | — | `main` is RED on `sanitize-cpu (address,undefined)` since `cefacd2d0`: `Ltx2LoadVaeWeights` (`ltx2_loader.cpp:1325`) reinterpret_casts the safetensors mmap to `const uint16_t*`, and that payload offset carries NO alignment guarantee — UB everywhere, a real fault on `build-test-cpu-arm64` and Jetson/Orin sm_110. THIRD recurrence of one class after [#301](https://github.com/mudler/vllm.cpp/issues/301) (closed; it left the `vt::LoadUnaligned` seam) and [#627](https://github.com/mudler/vllm.cpp/issues/627) (`qwen3_5_weights.cpp`, still open). The coverage that caught it was ACCIDENTAL — the fixture's JSON header happens to land that tensor odd — so the fix owes a case that FORCES the odd offset and asserts the parity | bug | | [#664](https://github.com/mudler/vllm.cpp/issues/664) | — | **FIXED 2026-08-13, `row/FIX-WINDOWS-POSIX-VIDEO-ENGINE`.** `windows-msvc-cpu` / `windows-msvc-vulkan` were RED on EVERY open PR (9 sampled across 5 unrelated lanes) from ONE file: `video_engine.cpp` reached Windows with ``, `::stat` and `S_ISDIR` (landed `cefacd2d0`), which `check-windows-portability.py:1675-1688` flags under `full_source_posix` — every scanned source, not only the platform-boundary set. `main` was never a denominator because the Windows jobs are PR-only and `skipped` on push (#584). Repaired at the SOURCE, not the checker: `IsDir`/`Exists` now take the `std::error_code` overloads through a file-local `NativePath`, which preserves `::stat`'s return-false-for-an-uninspectable-path behaviour that the THROWING overloads would have turned into a `filesystem_error` escaping a registry query | bug | | [#608](https://github.com/mudler/vllm.cpp/issues/608) | `TOOLS-PARSER-BREADTH` | **W0 (record backfill) landed 2026-08-13; row `INVENTORIED` → `PARTIAL`. W1 then shipped `inkling`, taking the registry to 42 names / 38 families.** Re-derived from the two REGISTRIES rather than from recipe usage: **five** `--tool-call-parser` names are upstream-only at the pin — `openai`, `inkling`, `minimax_m3` (W1, recipe demand) and `cohere_command3`, `cohere_command4` (W2, ZERO recipe uses, so usage-driven audits miss them). **Only `inkling` was PORTABLE from vLLM source, and it LANDED** (a `ParserEngineToolAdapter` over the already-ported Inkling engine; the gap was the registry face, not the grammar). Of the four left: `minimax_m3` is backed by the Rust crate; `openai` is a stub delegating to `vllm/parser/harmony.py`, which IS vLLM source but wraps the out-of-tree `openai_harmony` package (the SGLang secondary-oracle check was run and REFUSED — vLLM implements this path, so the rule does not admit a secondary); both Cohere names are shims over the out-of-tree `cohere_melody` package. So W1-remaining/W2 each owe a recorded decision before code rather than a text port. `nemotron_json`, `kimi_k3` and `ling3` are in NEITHER registry and arrive with the pin advance, not here. W3 ports upstream's shared `ToolParserTestConfig` harness. The earlier "six missing" framing was usage-derived: it listed `nemotron_json` as portable (it is not registered at the pin) and missed both Cohere entries | feature | diff --git a/.agents/specs/ltx25-device-seam-sibling.md b/.agents/specs/ltx25-device-seam-sibling.md new file mode 100644 index 000000000..d86344cbb --- /dev/null +++ b/.agents/specs/ltx25-device-seam-sibling.md @@ -0,0 +1,484 @@ +# The diffusion lane's device seam — the sibling that never adopted it, and the gate that cannot see it + +Row: `LTX25-DEVICE-SEAM-SIBLING` +Issues: [#659](https://github.com/mudler/vllm.cpp/issues/659), [#660](https://github.com/mudler/vllm.cpp/issues/660) +Campaign: [#644](https://github.com/mudler/vllm.cpp/issues/644) +Base: `11cc1d5896b480a1b652db9249319242053aca93` + +Both defects were found by a **peer session's reviewer** while reviewing the +main-red repair for landing, not by this campaign. They are recorded here +because they are in this lane and this campaign owns them. + +## 0. What is wrong today + +`11cc1d589` routed LTX-2.5's device question through the platform seam. It fixed +the lane it was aimed at and left two things standing. + +**(a) #659 — the seam was adopted, its companion guard was not.** +`src/vllm/multimodal/ltx2_video.cpp:549-562` now asks two questions: + +```cpp +const vt::DeviceType accelerator = vllm::platforms::CurrentPlatform().device_type(); +if (accelerator == vt::DeviceType::kCPU || + vt::TryGetBackend(accelerator) == nullptr) { Fail(...); } +``` + +That is "is there an accelerator, and is a backend registered for it". The +precedent it cites, `SelectQueueForModel`, asks a third question — +`src/vllm/entrypoints/model_loader.cpp:97`: + +```cpp +(architecture.empty() || plat.supports_model_architecture(architecture)) +``` + +`supports_model_architecture` exists precisely so a **partial** backend can +decline by name. Two platforms override it and both declare a short list: +`src/vllm/platforms/metal.cpp:70` and `src/vllm/platforms/tenstorrent.cpp:55`. +On such a build a `device = 1` LTX-2.5 load **was refused by name** and is now +accepted, so the failure moves from a refusal that says which piece is missing +into a kernel bind that says nothing. CUDA is unaffected, which is exactly why +this is invisible on the box that runs the gates. + +The refusal that *is* there argues, correctly, that serving the CPU forward +behind an accelerator handle "would make every later timing and every 'it ran on +the GPU' claim false". A partial backend that binds and dies has the same +property one level down: it is a device claim the build cannot honour. + +**(b) #660 — the gate that certified (a) is a token grep, and the sibling lane +spells its way past it.** `scripts/check-device-leakage.py:78` is + +```python +RE_KCUDA = re.compile(r"\bkCUDA\b") +``` + +`src/vllm/multimodal/minimax_h3_video.cpp:221-226 @ 11cc1d589` — anchored on the +base SHA, because this row is what removes it and an unanchored line number here +would point at a blank line the moment it lands — never writes that token: + +```cpp +vt::DeviceType MiniMaxH3VideoDeviceType(int32_t device) { + if (device != 0 && device != 1) { throw ...; } + return static_cast(device); +} +``` + +It hardcodes the ABI's `0/1` into the enum by integer cast and scores **zero** +in the `kcuda` bucket. The same defect, in the sibling diffusion lane, under a +different spelling. + +**The sharpest form of it:** `tests/vllm/models/test_minimax_h3_video_fold.cpp:162` +asserts `MiniMaxH3VideoDeviceType(1) == vt::DeviceType::kCUDA`. The *test* spells +the token honestly and is counted; the *source* launders it and is not. The gate +therefore reads the confession and misses the act. + +This is the shape the project already has a name for — an instrument that cannot +report its own blind spot returns a pass. "`kcuda` 2 → 0" is a true statement +about the token and a weaker statement about the property than it appears to be. + +## 1. Scope + +**In.** + +1. `minimax_h3_video.cpp` resolves its device through the platform seam rather + than by integer cast, mirroring what `ltx2_video.cpp` now does. The public + `MiniMaxH3VideoDeviceType(int32_t)` contract in + `include/vllm/multimodal/minimax_h3_video.h:63` is preserved — `0` is CPU, + anything else is refused or resolved, never cast. +2. `ltx2_video.cpp` asks `supports_model_architecture` alongside the two + questions it already asks, and refuses **by name**, naming the platform and + the architecture, when a registered backend declines the model. +3. `check-device-leakage.py` gains a bucket that sees an integer cast to + `vt::DeviceType`. The bucket must be **derived from the property**, not from + the one spelling we happen to have found — see §3. + +**Out.** + +- Any change to what CUDA does. This row must be a no-op on a CUDA build, and + the gate for that is the existing suite passing unchanged on this box. +- Widening or rewriting `device-leakage-baseline.json` beyond the new bucket's + own entries. **The baseline is a ratchet**: a peer measured that + `--write-baseline` refuses to raise it (`REFUSING to write a HIGHER baseline + (32 → 35)`) but that **hand-editing the JSON to a higher number makes the + checker PASS**. So the only real defence is that the diff is visible and + reviewed. Any baseline line this row touches is called out in the PR body, + with its reason, and the reviewer checks the file by md5 against the base for + every key this row does not claim. +- The H3 video engine's model behaviour. This is a device-resolution change. + +## 2. Upstream anchors + +There is no upstream for this: vLLM has no LTX-2.5 or MiniMax-H3 video engine, +and the device seam is ours. The mirror source is therefore **our own +`SelectQueueForModel`** (`src/vllm/entrypoints/model_loader.cpp:59-104`), which +is the shape every other model path already uses, and +`include/vllm/platforms/interface.h:263`, which defines the capability question. +Mirroring an internal seam is what "route it through the shared surface" means; +a second, parallel device resolution in the diffusion lane is exactly the +hand-rolled path AGENTS.md forbids. + +## 3. Design + +**The capability question, at the same site as the existing two.** One added +clause, one refusal message that names the platform, the architecture and the +fact that the backend declined — not a shape error. It must be possible to read +the refusal and know that the build is partial rather than broken. + +**The sibling's resolution.** `static_cast(device)` is replaced +by an explicit mapping: `0` → `kCPU`, non-zero → the platform's +`device_type()`, refused when that is `kCPU` or its backend is absent or it +declines the architecture. That makes the two diffusion lanes answer the device +question the same way, which is the point of the seam. + +**The gate's new bucket is the hard part, and it is where this row can go +wrong.** A bucket that greps `static_cast` closes the one site +we found and nothing else — the same defect as the token grep, one spelling +later. The bucket is defined by the property: *an integer literal or integer +variable becoming a `vt::DeviceType` without passing through the platform +seam*. Whatever the implementation, the acceptance test is adversarial and +stated up front: + +> The bucket must go RED for **at least three spellings** of the same defect +> that are not the one already in the tree — e.g. a C-style cast, a +> `vt::DeviceType(x)` functional cast, and an assignment through an +> intermediate `int`. If it only catches the one we knew about, it is a +> regression test wearing a gate's clothes, and the row says so rather than +> claiming coverage. + +If the property cannot be expressed at the granularity a text checker allows, +that is a finding to record, not to paper over: the row then states the residual +blind spot in the checker's own message, because **a checker's message is the +authority on what it enforces**. + +## 4. Tests + +RED-first for each of the three. + +1. A build with a platform that declines the architecture must refuse the LTX-2.5 + `device = 1` load **by name**. The existing test fixture pattern for a + partial backend is `metal.cpp` / `tenstorrent.cpp`; if neither is + constructible in the CPU test build, the test injects a stub platform rather + than skipping — a skipped test here is the whole finding. +2. `MiniMaxH3VideoDeviceType` keeps its contract: `0` → `kCPU`, `-1` and `2` + throw (`test_minimax_h3_video_fold.cpp:161-164` already assert this and must + stay green **unchanged**), and `1` resolves through the seam rather than by + cast. The new assertion is that on a CPU-only build `1` is **refused**, which + the cast could never do. +3. The checker's three-spelling adversarial test above, each spelling asserted + RED individually, not as a batch. + +**Mutations that must be run and recorded:** revert each of the three changes +independently and confirm the corresponding test goes RED; and confirm the +existing suite is byte-identical in count on CUDA-absent builds, since a changed +count is RED even when it reads green. + +## 5. Risks + +- **The capability guard could refuse a load that works today.** `metal.cpp` and + `tenstorrent.cpp` are the only overriders, so the blast radius is those two + builds — but if either currently *runs* a diffusion model despite a short + list, this row breaks it. Check before assuming; if it does run, the finding + is in the list, not in the guard. +- **The new bucket could red other lanes.** An integer-to-`DeviceType` cast + elsewhere in the tree is either the same defect (fix it or record it) or a + legitimate deserialization boundary (which needs a stated, per-entry reason, + never a blanket directory exemption). +- **Baseline churn.** See §1 Out. + +## 6. Stop conditions + +- If the capability guard turns out to refuse a currently-working configuration, + stop and return `NEEDS_DECISION` rather than either shipping the refusal or + dropping the guard. +- If the new bucket cannot reach three independent spellings, stop and report + the residual blind spot; do not ship a one-spelling bucket described as + coverage. +- `dgx.casa` is not required for any of this. Nothing here is a GPU measurement. + +## Findings from implementation + +**The brief and this spec disagreed on one line, and this spec won.** The +dispatch brief said `test_minimax_h3_video_fold.cpp:161-164` must stay green +*unchanged*, but §4.2 above enumerates what "the contract" means — `0 → kCPU`, +`-1` and `2` throw — and separately requires that on a CPU-only build `1` is +**refused**. Line 162 asserted `MiniMaxH3VideoDeviceType(1) == kCUDA`, which is +precisely the cast's answer and cannot survive the change. It is now +build-conditional and asserts BOTH arms: `== accelerator` where one is +registered, refused-by-name where none is. 161/163/164 are untouched. + +**`test_minimax_h3_video_fold.cpp`'s CUDA-load case registered a BACKEND and no +PLATFORM.** It could, because the cast never asked whether the build had an +accelerator. It does now, so the fixture supplies both halves. That is the +defect being visible rather than a harness concession: a build with a CUDA +backend registered and no CUDA platform is not a build that runs on CUDA. + +**The architecture key is the FAMILY string** (`ltx-2.5`, `minimax-h3`), not an +HF `architectures[0]` class name. The diffusion lanes are reached through +`LoadVideoEngine`/`VideoModelParams::family` and never read an `architectures` +entry, so the family slug is the only stable identifier they have. It does mean +`supports_model_architecture`'s key space now mixes HF class names +(`OPTForCausalLM`) with family slugs; the two cannot collide, and the refusal +names the string the user actually typed. Flagged for the reviewer as the one +judgement call in the row. + +**`metal.cpp` and `tenstorrent.cpp` run no diffusion model** — searched in their +own vocabulary over `src/vllm/platforms/{metal,tenstorrent}.cpp` and +`src/vt/{metal,tenstorrent}/` with `OPTForCausalLM` as a positive control in the +same command: the control hit three times, `ltx|minimax|h3|diffus|video` hit +zero. So §5's stop condition is not triggered: the guard refuses nothing that +works today. + +**The `dev_cast` bucket produced one false positive on the real tree**, and it is +the interesting kind. `kv_connector.h:225`'s +`supports_worker_transfer_on(vt::DeviceType /*device*/) const` strips to +`(vt::DeviceType )` followed by `const` — textually a C-style cast. Two +discriminators fixed it (the `(` must not be glued to an identifier; the `)` must +not be followed by a declarator suffix) and M29 pins both, together with the +proof that the discriminators did not cost the real detection. + +**Residual blind spots are recorded in the checker's own docstring**, per §3's +instruction: type aliases, macros and template parameters that resolve to +`DeviceType`, `bit_cast`/`memcpy`/union punning, conversions inside the unscanned +`src/vt/` leg, and the fact that nothing type-checks the operand. The bucket +flags every cast *to* `DeviceType` and relies on `DSR-ALLOW` for the legitimate +ones. + +## Findings from review (PR #671, head `094ac9e4`) + +**The bucket missed the purest form of the defect, and the test that ruled that +out could not see it.** `(vt::DeviceType)d` scored 1; `(vt::DeviceType)1` scored +**0**, because the C-style alternative's trailing lookahead admitted only +`[A-Za-z_(]` and `1` is a digit. Naming a device by its literal enum value is +precisely what this bucket exists to police. Worse, M29's "the discriminators did +not cost the real detection" assertion used an *identifier* operand, so the test +could not detect the gap it was written to rule out — a guard that certifies +itself, which is the disease this row exists to fix, reproduced inside the row's +own instrument. M29 now pins the literal and signed-literal operands first. + +**Four more plain spellings read GREEN in scanned files** and are closed rather +than documented, each measured at zero new hits over `src/vllm` + `include/vllm`: +global-scope qualification `static_cast<::vt::DeviceType>`, the +elaborated-type-specifier `static_cast`, direct-list-init in +a *declaration* (`vt::DeviceType dt{raw}` — M23 caught only the unnamed +temporary), and pointer punning `*reinterpret_cast(&raw)`. + +**The pointer form is what makes three of the four cast keywords real.** +`reinterpret_cast`, `const_cast` and `dynamic_cast` to a scoped enum are +ill-formed — compile-checked, all three rejected, with `static_cast` as a live +control that compiles — so before this change those keywords could only ever +match code that does not build, creating an appearance of coverage the pattern +did not have. Matching a pointer target makes them live. + +**The declaration form takes `{` and never `(`, and that costs nothing.** +`vt::DeviceType dt(raw)` is ill-formed (no implicit int→scoped-enum conversion), +so excluding it loses no real spelling, whereas admitting `(` matched every +function *definition* whose return type is `DeviceType` — measured, 3 false +positives including `MiniMaxH3VideoDeviceType` and `ResolveExplicitDeviceType`. +M34 pins that negative. + +**The fold test asked two of the three questions and so red on the build class +#659 exists to serve.** `test_minimax_h3_video_fold.cpp`'s `have_accelerator` +predicate tested only `device_type() != kCPU && TryGetBackend() != nullptr`. On +Metal or Tenstorrent both are true, so the test took the `== accelerator` arm +while the source *correctly* refused, and the refusal surfaced as an uncaught +exception — a false RED, invisible on the CPU and CUDA boxes that run the gates, +which is this row's own thesis about #659 turned against its own test. The +predicate is now three-way and asserts *which* refusal, since a right refusal for +a wrong reason is a wrong diagnosis that reads as a right one. + +**The decline CONSEQUENCE inverts the cited precedent, deliberately.** The PR +described the change as mirroring `model_loader.cpp:97`, and it mirrors that +site's *question* while inverting its *answer*. `:97`'s capability test lives on +the `kAuto` path, whose response to a decline is to fall through to `:103` and +**serve on CPU**; `metal.cpp:65-69` states that policy in as many words ("falls +back to the CPU reference … and runs correctly, just slowly — which is strictly +better than dying inside a kernel bind"). Both diffusion lanes instead **throw**. +That is correct, but it is the *explicit-device* path's polarity, not the +`kAuto` path's: `device = 1` is an explicit accelerator request, and +`model_loader.cpp:71-72` already says of that path "an explicit accelerator whose +queue cannot be created must FAIL the load loudly, never silently serve on CPU" — +the same argument `ltx2_video.cpp:545-548` makes for refusing rather than serving +the CPU forward behind an accelerator handle. So the lanes mirror the capability +question from one path and the failure polarity from the other, and both halves +are the seam's own. + +**Residual, not this row's to fix:** `vulkan.cpp` does not override +`supports_model_architecture`, so it inherits `interface.h:263`'s default `true` +and a partial Vulkan build still binds and dies inside a kernel. Only +`metal.cpp:70` and `tenstorrent.cpp:55` narrow the claim. + +## Findings from review round 2 (PR #671, head `074ef1420`) + +**F5 — the row's own thesis came back for its instrument a THIRD time, and this +one was in the checker's MESSAGE.** M33 had made the pointer target real, and the +docstring then said the bucket is "anchored on the TARGET TYPE, not on the +operand and not on one cast keyword", and listed `std::bit_cast` as unreachable +"because no spelling of the target type appears at the conversion site". Nine +spellings that DO write the target type at the conversion site scored **zero**. +AGENTS.md makes a checker's own message the authority on what it enforces, so an +over-claiming message is a defect in the gate, not a wording nit — and it is #660 +exactly, one sigil later. + +Each spelling was **compile-verified legal** (`g++ -std=c++20 -Wall -Wextra`, +exit 0) before being called a miss, because a "miss" that does not compile is +not a miss; and each measured **0 before / 1 after**, individually: + +| spelling | why it slipped | +|---|---| +| `reinterpret_cast(raw)` | reference target; the standard *defines* it as M33's pointer pun, and `\*?` admits a star but not an ampersand | +| `static_cast(d)` | east const | +| `static_cast(t)` | east const + reference — the form that compiles with **no** warning, so it survives a `-Werror` build where the plain east-const prvalue trips `-Wignored-qualifiers` | +| `(vt::DeviceType const)d` | east const, C-style | +| `(vt::DeviceType)*cursor` | the trailing class admitted a sign but not a dereference — **and this is the wire-decode spelling the docstring itself names as the expected `DSR-ALLOW` case**, so the one site the bucket predicted meeting was the one it could not see | +| `(vt::DeviceType)~mask`, `(vt::DeviceType)!flag` | the same hole | +| `reinterpret_cast(p)` | `\*?` is one star | +| `std::bit_cast(raw)` | the docstring's own reason was false for it: it spells the target in full | + +Three edits close them: the named-cast target takes cv-qualifiers on **either** +side of the name and a **run** of `*`/`&`; the C-style trailing class admits +`*~!`; `bit_cast` joins the cast keywords. + +**What was NOT closed, and why the docstring now says so per-entry.** `&` is +deliberately absent from the C-style trailing class: `) &` and `) &&` are +ref-qualifiers on a member declarator, which is precisely the false positive the +trailing guard exists to reject, and the guard cannot tell +`void note (vt::DeviceType*) &` from a pun. So the **C-style** pointer pun +`*(vt::DeviceType*)&x` stays blind, and is named as blind with the reason that is +true *of it* — the named-cast spelling of the same pun is caught, which is what +makes it a narrow gap rather than the class. The old blind-spot list shared one +reason across three entries and that reason was false for one of them; each entry +now carries its own. + +**Measured, not asserted.** Over the files in `src/vllm` + `include/vllm`, with +`\bDeviceType\b` = **162** matches as a positive control **in the same +command**: **0 new, 0 lost**. (This round said "740 files"; the scanned set is +**760** — see round 3 below, which re-derived it.) Shipped hits 1, widened hits 1 — the same +allowlisted `platform.cpp:85` registry-walk inverse. `dev_cast` stays 0, `total` +stays 32, `scripts/device-leakage-baseline.json` is untouched by this round. + +**M35-M40**, one mutant per newly-closed spelling, each sub-spelling asserted +**individually** (the M29 shape), because a mutant that only exercises the case +the pattern was tuned for is a guard that certifies itself. RED-before is real +rather than narrated: with the pattern reverted in-process to its pre-repair +value, M35-M39 all go RED while M20, M33 and M40 stay GREEN — so a blanket +failure cannot pass for five findings. M40 is the negative the widening could +have cost: pointer- and reference-returning declarations, ref- and +rvalue-ref-qualified members **with a space before the paren**, volatile +members, and `std::vector`. + +**Anchor drift, re-derived at the merge.** `9f2b9bb9a` moved +`tenstorrent.cpp`'s `supports_model_architecture` from `:52` to `:55`; the four +citations on this branch (`ltx2_video.cpp`, `test_diffusion_device_seam.cpp`, and +§0 and the residual note above) are corrected. `metal.cpp:70`, +`interface.h:263` and `model_loader.cpp:97` were re-derived on the merged tree +and all three still hold. Every `path:NN` anchor in this row's files was +resolved mechanically; the only remaining unresolvable ones are upstream Python +paths and two `examples/*/main.cpp` citations that carry their own `@ fc636c76` +and so are provenance rather than drift. The `minimax_h3_video.cpp:221-226` +citation in §0 is now anchored on the base SHA for the same reason. + +## Findings from review round 3 (PR #671, head `79ebbce42`) + +**The blind-spot ENTRY bound two members under one reason, and the reason was +true of only one of them.** The note covered "a C-STYLE cast whose target is a +pointer **or reference**" with the single reason "both are `)` followed by `&`". +That is true of `*(vt::DeviceType*)&x` and **false** of `(vt::DeviceType&)raw`, +whose next character is `r`. The reviewer **constructed the feared false +positive** instead of accepting the argument, which split the entry cleanly: +widening only **inside the parens** catches the reference form *and* +`(vt::DeviceType*)vp` at **zero** new hits, while the ref-qualifier negatives +`void note (vt::DeviceType*) &;` and `… &&;` stay at zero because `&` is still +out of the **trailing** class. So the trailing-class exclusion is justified, the +C-style **pointer pun** legitimately stays blind, and the entry is now two +entries with a reason each. Round 2 closed with "the old blind-spot list shared +one reason across three entries and that reason was false for one of them; each +entry now carries its own" — and the entry it wrote to replace them did the same +thing to two. The spec paragraph was accurate about the pointer pun; the +**checker's own message**, which is the authority, said "pointer or reference". + +**Eight more spellings — counted, not rounded; the table groups two of them on +one row because they share a reason — each compile-verified legal (`g++ 13.3 +-std=c++20 -Wall -Wextra -fsyntax-only`, exit 0) and each measured 0 before / 1 +after, individually:** + +| spelling | why it slipped | +|---|---| +| `(vt::DeviceType&)raw` | the C-style target admitted no `*`/`&` run at all | +| `(vt::DeviceType*)vp` | same | +| `vt::DeviceType const d{raw}` | alternative (3), the DECLARATION form, had **no cv-group** — a **fourth** place a cv-qualifier can sit, while M36 asserted "all three places". An enumeration certifying its own completeness, which is M29's defect returning | +| `vt::DeviceType volatile d{raw}`, `struct Cfg { vt::DeviceType const kD{1}; }` | same, and the member form is the one a person actually writes | +| `std::bit_cast(raw)` | round 2 added `bit_cast` *because* it spells the target at the site, then terminated the target at `>` on the assumption every cast takes one template argument. `bit_cast` is a **function template with two parameters** | +| `reinterpret_cast(p)` | the `[*&]` run stopped at the first cv-qualifier, so the reference-to-const-pointer the docstring claimed was not reachable | +| `__builtin_bit_cast(vt::DeviceType, raw)` | not a template-id at all, so (1)'s `<…>` anchor cannot reach it and (2)'s glued-identifier discriminator rejects it | + +**One pre-existing FALSE positive, closed rather than widened.** +`sizeof (vt::DeviceType) + 1` already scored **1** before this round — `sizeof` +is not glued to its paren, so the identifier discriminator passes, and `+` is in +the trailing class — and admitting `*` inside the parens would have extended it +to `sizeof (vt::DeviceType*) + 1`. `sizeof` and `alignof` convert nothing, so +both are excluded outright. M45 pins it, and is RED against the shipped pattern. + +**What is NOT closed, and why the checker now says so as a PROPERTY.** Four +rounds have each found a spelling the previous round's message already claimed, +and every one closed at **zero** hits — so none was a coverage/false-positive +trade; there was simply always another spelling. Widening again is not the +answer to that, and the docstring no longer implies it is. It now: (a) +enumerates the forms it **measured** itself matching, (b) gives every blind spot +a reason true of that entry alone — the C-style pointer pun `*(vt::DeviceType*)&x` +(closing it needs `&` in the trailing class, which is the ref-qualifier false +positive), a **character-literal** operand `(vt::DeviceType)'\x01'` (character +literals are blanked to whitespace by `strip_comments_and_strings` before the +pattern ever runs, so the operand is gone by match time), `memcpy`/union (no cast +**expression** to anchor on — *not* "the type is not named at the site", which is +false for the idiomatic `std::memcpy(&dt, &raw, sizeof(vt::DeviceType))`), the +unscanned `src/vt/` leg, and the absence of any type check — and (c) states the +residual as a property: **a text checker enforces a set of SPELLINGS, not "an +integer becomes a `vt::DeviceType`"**, so a green `dev_cast` means "none of the +listed spellings is present" and never "no integer becomes a DeviceType here". +The structural answer is an **AST-level check** (clang tooling, where the +destination type canonicalises and the source type is known), filed as +[#828](https://github.com/mudler/vllm.cpp/issues/828) with all four rounds' +evidence, since "we kept finding more spellings" *is* the argument for it. Not +built here. + +**M41-M46 and the M36 repair**, one mutant per newly-closed spelling, each +asserted individually. **M46 is the new shape**: it pins that the **declared +blind spots are still blind**, with `(vt::DeviceType)buf[0]` as a positive +control in the same test — so if a later widening closes one, M46 goes RED and +the message must be corrected in the same change. That is the only mechanism +that keeps a checker's message the authority on what it enforces. RED-before is +real, not narrated: with `RE_DEVTYPE_CAST` reverted in-process to `79ebbce42`, +exactly **6** mutants go RED (M36, M41-M45) and M20/M33/M40/M46 stay GREEN, so a +blanket failure cannot pass for six findings; applied, 0 RED. Suite 48 → **54** +tests, the +6 fully attributed to M41-M46. + +**File count re-derived, and anchored to a SHA, because it rots.** #660's +roadmap row and this spec both said "740 files". The scanned set is **760 at +`79ebbce42`** and **765 at the merge below**, because main added five source +files under the scan roots in between. Both numbers were measured with +`\bDeviceType\b` = **162** and `\bkCUDA\b` = **18** as positive controls in the +same pass, and both are unchanged by those five files, which is why every other +figure in this section holds across the merge. + +That drift is the finding, not an errand. A file count of one tree stored inside +another file is a measurement that every unrelated pull request invalidates, +which is the shape AGENTS.md forbids under "never store a measurement of one +file inside another file". "740" was not wrong when it was written; it rotted. +So both records now carry the SHA the number was measured at, and a reader who +finds a mismatch knows to re-derive rather than to distrust the rest. The +durable statement is the ratio the controls give, not the absolute count. + +## Now + +`READY`, implemented and awaiting a fresh review on +`row/LTX25-DEVICE-SEAM-SIBLING` (PR #671). All three changes plus the F5 repair +are on the branch with their RED, GREEN and mutation evidence in the PR body; +next is a fresh reviewer — not the implementer — on the immutable head, then the +operator's own gate rerun. + +The row stays `READY` in `roadmap_v1.md` deliberately. A lifecycle move to +`ACTIVE` owes `docs/STATUS.md` and `docs/BENCHMARKS.md` in the same change +(scripts/check-doc-checkpoint.py), and those are projections of what the project +CLAIMS — which this row does not change until it lands. Writing them from an +unmerged PR would also put two shared files under a lock for the length of a +review. The operator moves the state, and writes those two surfaces, when it +merges. diff --git a/docs/USAGE.md b/docs/USAGE.md index 257f2cc89..58406ede7 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -1892,7 +1892,14 @@ The LTX-2.5 arm runs on the CPU in f32 and on CUDA in bf16. `device = 0` takes the f32 parity forward; `device = 1` stages the DiT to the GPU one tensor at a time and runs the device-resident forward, so a CUDA handle means a CUDA forward. On a build with no CUDA backend, `device = 1` is refused by name rather than -served the CPU forward behind a CUDA handle. `encoder_path` loads the Gemma-4 +served the CPU forward behind a CUDA handle. It is also refused when the build's +accelerator is a PARTIAL backend that declines this architecture — Metal and +Tenstorrent each register the kernels for a named short list of models, and a +backend that has not registered this one now says so by name instead of binding +a queue and failing later inside a kernel. The same three questions decide +`minimax-h3`'s `device = 1`, which resolves through the platform seam rather +than reading the ABI selector as an enum value, so on a CPU-only build it throws +instead of naming CUDA. `encoder_path` loads the Gemma-4 text tower, and the request's own `prompt` then conditions the render; the tower itself runs on the CPU in f32 whichever device the DiT is on. Without one, conditioning comes from the two prompt-embeds files, which must agree on their diff --git a/include/vllm/multimodal/minimax_h3_video.h b/include/vllm/multimodal/minimax_h3_video.h index 4156d510f..f59a0fdba 100644 --- a/include/vllm/multimodal/minimax_h3_video.h +++ b/include/vllm/multimodal/minimax_h3_video.h @@ -58,8 +58,14 @@ namespace vllm::multimodal { inline constexpr char kMiniMaxH3VideoFamily[] = "minimax-h3"; // Map the stable public video ABI device selector onto the runtime's generic -// backend key. The ABI remains 0=CPU / 1=CUDA; callers below this seam dispatch -// only through the returned DeviceType. +// backend key. The ABI is unchanged — 0 is the CPU, 1 is the accelerator, and +// anything else throws — but 1 is RESOLVED through the platform seam +// (`CurrentPlatform().device_type()` + `TryGetBackend` + +// `supports_model_architecture`), never cast from the integer. It therefore +// THROWS on a build with no accelerator backend, or one whose partial backend +// declines this architecture, instead of naming a device that build cannot run +// (#659, #660). Callers below this seam dispatch only through the returned +// DeviceType. vt::DeviceType MiniMaxH3VideoDeviceType(int32_t device); // ── Load-time parameters (the checkpoint set; the C ABI mirror is diff --git a/scripts/check-device-leakage.py b/scripts/check-device-leakage.py index c8f93352a..7b8e2f866 100644 --- a/scripts/check-device-leakage.py +++ b/scripts/check-device-leakage.py @@ -19,20 +19,130 @@ attention-registry work each added a device test in passing. Leakage grows silently under well-executed work, so it needs a ratchet, not a cleanup. -WHAT IT COUNTS. Four buckets, over non-comment / non-string-literal source: +WHAT IT COUNTS. Five buckets, over non-comment / non-string-literal source: kcuda textual `DeviceType::kCUDA` (or bare `kCUDA`) references is_cuda `is_cuda()` call sites + dev_cast an integer converted into a `vt::DeviceType` by a cast, in any + spelling — the device named by ENUM VALUE instead of by the + platform seam cuda_inc `#include "vt/cuda/…"` / `` NOT inside a CUDA or VT_* preprocessor guard (i.e. a non-CUDA build cannot compile) vt_ifdef `#ifdef VT_*` / `#if defined(VT_*)` build-time kernel-feature gates -Comments and string literals are stripped before matching in the first two +Comments and string literals are stripped before matching in the first three buckets. That is a DELIBERATE correction to the audit's composition, which excluded comment lines from `is_cuda` but not from `kCUDA` — a prose mention of a device name is not device leakage, and making the rule uniform is what keeps the metric honest in both directions. +WHY `dev_cast` EXISTS (#660). `kcuda` is a TOKEN grep. `DeviceType` is +`enum class DeviceType : uint8_t { kCPU = 0, kCUDA = 1, … }` +(include/vt/device.h), so `static_cast(device)` on the public +0/1 ABI selector hardcodes CUDA *by enum value* and never writes the token. That +is exactly what `src/vllm/multimodal/minimax_h3_video.cpp` did: it scored ZERO in +`kcuda` while the test asserting the same mapping spelled `kCUDA` honestly and +WAS counted. The gate read the confession and missed the act. It is also an +enum-ordering hazard — reordering the enum silently re-points every such site. + +The reverse direction (`static_cast(type)`) is the SAFE one and is +deliberately not counted: it is how the seam itself indexes its registry. + +WHAT `dev_cast` ENFORCES, EXACTLY. It is a regular expression over +comment-stripped text, so what it enforces is a SET OF SPELLINGS. It does NOT +enforce the property it was written for — "an integer becomes a DeviceType +outside the platform seam" — and it cannot: it cannot resolve a name to a type, +cannot tell an integer operand from a DeviceType one, and cannot see a conversion +whose target type is not written at the conversion site. Read a green `dev_cast` +as "none of the spellings enumerated below is present", never as "no integer +becomes a DeviceType here". #828 tracks the AST-level check (clang tooling), +where the destination type canonicalises and the source type is known, and which +is what would enforce the property. This pattern is the INTERIM, and says so. + +That distinction is measured, not defensive. FOUR review rounds of this one +bucket each found a spelling the previous round's message already claimed to +cover. #660 shipped it; round 1 found the literal operand `(vt::DeviceType)1`, +and the mutant written to rule that out used an IDENTIFIER operand, so it could +not see the gap it existed to close; round 2 found nine more that all write the +target type the docstring named as its anchor; round 3 found eight more (counted, +not rounded), including a FOURTH place a cv-qualifier can sit while the mutant +asserted "all three". +Every one of them closed at ZERO hits on the scanned tree, so none was a +coverage-versus-false-positive trade — there was simply always another spelling. +So the list below is an enumeration of what was MEASURED, not a claim about a +class. + +WHAT `dev_cast` DOES SEE. Each entry is pinned by its own mutant in +tests/scripts/test_device_leakage.py, asserted individually: + + * the named casts `static_cast`, `reinterpret_cast`, `const_cast`, + `dynamic_cast` and `bit_cast`, plus the builtin spelling + `__builtin_bit_cast(vt::DeviceType, x)`. `bit_cast` is a FUNCTION template + with two parameters, so its second template argument (`From`) may be + written — as long as it is on one line and contains no nested angle + brackets; + * the C-style cast `(vt::DeviceType)x`, the functional casts + `vt::DeviceType(x)` and `vt::DeviceType{x}`, and the same conversion spelled + as a DECLARATION, `vt::DeviceType d{x}`; + * the type written `DeviceType`, `vt::DeviceType`, `::vt::DeviceType` or + `enum vt::DeviceType`, with `const`/`volatile` on EITHER side of the name in + ALL of those forms — the declaration form included, which is the fourth + cv-position and the one round 3 found (`vt::DeviceType const kD{1}`); + * an operand that is an identifier, a literal (`)1`), a signed literal (`)-1`), + a parenthesised expression, or a unary expression (`)*cursor`, `)~mask`, + `)!flag`); + * a target that is a POINTER, a POINTER-TO-POINTER or a REFERENCE — the pun + `reinterpret_cast` performs on an integer's bytes — in the named-cast form + AND the C-style form, with cv-qualifiers anywhere inside the `*`/`&` run + (`reinterpret_cast(p)`); + * a cast clang-format has wrapped across two lines. + +WHAT `dev_cast` STILL CANNOT SEE, stated here because a checker's message is the +authority on what it enforces and an instrument that hides its blind spot +returns a false pass. Every entry states the reason that is true OF THAT ENTRY +ALONE, and no entry binds two cases whose reasons differ — a list whose shared +reason is false for one member is the same false claim in a smaller font, and +that is precisely what rounds 2 and 3 each found here: + + * a cast whose target is a type ALIAS (`using DT = vt::DeviceType;` then + `static_cast
(x)`), a MACRO that expands to the type name, or a TEMPLATE + PARAMETER that resolves to DeviceType. Reason: no spelling of the target + type appears at the conversion site, so there is nothing to anchor on; + * `memcpy` or a UNION punning an integer onto a DeviceType. Reason: there is + no cast EXPRESSION to anchor on. (Not "the type is not named at the site": + the idiomatic `std::memcpy(&dt, &raw, sizeof(vt::DeviceType))` names it right + there, and matching that would mean matching a bare `sizeof(vt::DeviceType)`, + which converts nothing. The old entry gave `bit_cast`'s reason to `memcpy`, + and `bit_cast` is now a keyword above.); + * the C-style POINTER PUN whose operand begins with `&`, `*(vt::DeviceType*)&x`. + Reason: closing it needs `&` in the trailing class, and `) &` / `) &&` are + ref-qualifiers on a member declarator — `void note (vt::DeviceType*) &;` + compiles, and admitting `&` turns it into a false positive (measured: 2). + Every OTHER C-style pointer or reference cast IS caught — `(vt::DeviceType&)` + and `(vt::DeviceType*)` both — as is the named-cast spelling of this same + pun, so this is one operand shape rather than the class; + * a CHARACTER-LITERAL operand, `(vt::DeviceType)'\x01'`. Reason: character + literals are blanked to whitespace by `strip_comments_and_strings` before the + pattern ever runs, so the operand is gone by match time and no trailing class + could see it. `(vt::DeviceType)buf[0]` is caught, so this is that one literal + kind, not the wire-decode shape; + * a conversion that happens inside `src/vt/` and is merely CALLED from the + shared layer. Reason: `src/vt/` is a device leg and is not scanned at all; + * whether the operand really is an integer. Reason: nothing here type-checks. + The bucket flags every cast TO DeviceType and relies on `// DSR-ALLOW()` + for the legitimate ones (a wire-format decode is the expected case). This is + also why a pointer or reference target counts: `const_cast(t)` + removes const rather than converting an integer, and buys its exemption the + same way; + * any spelling that is not on the list above. Reason: this is a spelling list, + and four rounds of finding a new one is the evidence that a spelling list is + not the class. That gap does not close by widening; it closes at #828. + +tests/scripts/test_device_leakage.py M20-M46 pin what it does catch, each +spelling asserted on its own — including, in M29, the literal operand, because a +discriminator tested only on the case it was tuned for is a guard that certifies +itself. + HOW THE RATCHET WORKS. `scripts/device-leakage-baseline.json` holds the accepted per-bucket counts. Any bucket ABOVE its baseline fails. Any bucket BELOW its baseline also fails, with the instruction to re-run `--write-baseline` and commit @@ -73,10 +183,129 @@ SCAN_ROOTS = ("src/vllm", "include/vllm") SOURCE_SUFFIXES = {".h", ".hpp", ".cpp", ".cc", ".cu", ".cuh"} -BUCKETS = ("kcuda", "is_cuda", "cuda_inc", "vt_ifdef") +BUCKETS = ("kcuda", "is_cuda", "dev_cast", "cuda_inc", "vt_ifdef") RE_KCUDA = re.compile(r"\bkCUDA\b") RE_IS_CUDA = re.compile(r"\bis_cuda\s*\(\s*\)") + +# `dev_cast`: an integer becoming a DeviceType. Four alternatives, one per C++ +# spelling of the same conversion, all anchored on the TARGET TYPE: +# +# 1. a named cast static_cast(x) / / … +# and the INDIRECT forms, *reinterpret_cast(&x) +# and reinterpret_cast(x) +# 1b. __builtin_bit_cast __builtin_bit_cast(vt::DeviceType, x) — the same +# conversion as (1)'s `bit_cast`, but a two-argument +# BUILTIN rather than a template-id, so (1)'s `<…>` +# anchor cannot reach it +# 2. a C-style cast (vt::DeviceType)x, (vt::DeviceType)1, +# (vt::DeviceType&)raw and (vt::DeviceType*)vp +# 3. a functional cast vt::DeviceType(x) / vt::DeviceType{x}, and the same +# conversion spelled as a DECLARATION, `DeviceType d{x}` +# +# `_DEVTYPE_QUAL` absorbs the ways the SAME type can be written before its name: +# `vt::DeviceType`, a global-scope-qualified `::vt::DeviceType`, and an +# elaborated-type-specifier `enum vt::DeviceType`. All three name one type, so a +# pattern that recognised only the first would be a spelling grep again — which +# is the exact defect (#660) this bucket exists to answer. +# +# `\b` after the type name keeps `DeviceTypeName(t)` out. The lookbehind on (3) +# keeps `X::DeviceType(` and `foo.DeviceType(` and `vector(` out, so +# a qualified spelling is matched once at its `vt::`, not twice. The negative +# lookahead keeps a zero-argument `DeviceType()` value-initialisation out — that +# converts nothing. Matched over the WHOLE comment-stripped text rather than +# line by line, so a clang-format-wrapped cast cannot slip between two lines. +# +# (2) needs two guards, because `(Type)ident` and a PARAMETER LIST are textually +# the same thing. `supports_worker_transfer_on(vt::DeviceType /*device*/) const` +# in kv_connector.h is a declaration whose parameter name is commented out; after +# stripping it reads exactly like a cast applied to `const`. So the `(` must not +# be glued to an identifier (that is a call or a declarator), except after the +# keywords that legitimately precede a parenthesised expression; and what follows +# must not be a declarator suffix. +# +# (2)'s trailing class admits DIGITS, a leading SIGN and the UNARY OPERATORS +# `*`, `~` and `!`, not just identifiers. `(vt::DeviceType)1` — the device named +# by its literal enum value — is the PUREST form of what this bucket polices, and +# an identifier-only lookahead missed exactly it while catching +# `(vt::DeviceType)d`; `(vt::DeviceType)*cursor` is the WIRE-DECODE spelling this +# docstring itself names as the expected `DSR-ALLOW` case, and a class that could +# not see it was refusing to police the one site it predicted. `&` is deliberately +# NOT admitted: `) &` and `) &&` are ref-qualifiers on a member declarator, which +# is the false positive the trailing guard exists to reject. Measured over +# `src/vllm` + `include/vllm`: admitting `0-9+-*~!` adds ZERO hits. +# +# (2)'s TARGET, by contrast, does take a run of `*` and `&` — INSIDE the parens, +# where no declarator can be confused with it. That is the split the blind-spot +# entry used to get wrong: the old note bound "pointer or reference" together +# under the single reason "both are `)` followed by `&`", which is true of +# `*(vt::DeviceType*)&x` and FALSE of `(vt::DeviceType&)raw`, whose next character +# is an identifier. Widening only inside the parens catches the reference form and +# `(vt::DeviceType*)vp` at zero cost; the ref-qualifier negatives +# `void note (vt::DeviceType*) &;` and `… &&;` stay at zero because `&` is still +# out of the TRAILING class. Both halves compile-verified, both measured. +# +# `sizeof (vt::DeviceType) + 1` was ALREADY a false positive before that widening +# — `sizeof` is not glued to its paren there, so the identifier discriminator +# passes, and `+` is in the trailing class — and admitting `*` inside the parens +# would have extended it to `sizeof (vt::DeviceType*) + 1`. A `sizeof` or +# `alignof` converts nothing, so both are excluded outright rather than left as a +# gap the widening made wider. The lookbehinds are fixed-width, so they cover the +# one-space spelling clang-format produces; `sizeof(` with no space was already +# blocked by the glued-identifier discriminator. Measured: removing these two +# costs nothing on the tree (dev_cast stays at its single allowlisted hit). +# +# (1) and (2) and (3) all take the cv-qualifiers on EITHER SIDE of the type name, +# and (1) takes any run of `*`, `&` and cv-qualifiers INTERLEAVED. +# `vt::DeviceType const` is the same type as `const vt::DeviceType`; +# `reinterpret_cast(raw)` is the same pun as +# `*reinterpret_cast(&raw)`; and `vt::DeviceType* const&` puts a +# cv-qualifier one position further right than a `[*&]`-only run can reach — all +# COMPILE (checked, not read). (3) is where the fourth cv-position hides: +# `vt::DeviceType const d{raw}` is a declaration, and a group present on (1) and +# (2) but absent from (3) is a docstring claiming three places while the code +# covers two. +# +# `bit_cast` joins the four cast keywords because it spells the target type at the +# conversion site, so the blind-spot note below could not honestly claim it as +# unreachable. It is a FUNCTION template, not a cast operator, so `From` may also +# be written — `std::bit_cast(raw)` compiles — and +# the optional `,` tail is what lets (1) reach that form. `__builtin_bit_cast` +# gets its own alternative because it is not a template-id at all. `memcpy` and +# union punning stay blind, and their reason is now their own: there is no cast +# expression to anchor on. +# +# (3)'s DECLARATION form takes `{` only. `DeviceType d{x}` is a real conversion +# (C++17 permits list-initialising a scoped enum with a fixed underlying type), +# but `DeviceType d(x)` is ILL-FORMED — there is no implicit int→scoped-enum +# conversion — so nothing is lost by excluding it, while allowing `(` would match +# every FUNCTION DEFINITION whose return type is DeviceType (measured: 3 false +# positives, e.g. `vt::DeviceType MiniMaxH3VideoDeviceType(`). Adding the east +# cv-group to (3) does not reopen that: `vt::DeviceType const Frozen(int);` still +# scores 0, because `{` is still the only initialiser admitted. +_DEVTYPE_QUAL = r"(?:enum\s+)?(?:::\s*)?(?:vt\s*::\s*)?" +# cv-qualifiers, which C++ permits on either side of the type name. +_CV_WEST = r"(?:(?:const|volatile)\s+)*" +_CV_EAST = r"(?:\s*(?:const|volatile)\b)*" +# A named cast's target suffix: `*`, `&` and cv-qualifiers, any number, any order. +# `vt::DeviceType* const&` needs the cv-qualifier INSIDE the run, not before it. +_TARGET_SUFFIX = r"(?:\s*(?:const\b|volatile\b|[*&]))*" +# `bit_cast`'s second template parameter (`From`), which may be written out. +# One line, no nested angle brackets — deliberately narrow, since the scanned +# roots contain zero `bit_cast` occurrences and a greedy tail would be the only +# way this could cost a false positive. +_TEMPLATE_TAIL = r"(?:\s*,[^<>;{}\n]*)?" +RE_DEVTYPE_CAST = re.compile( + r"(?:static_cast|reinterpret_cast|const_cast|dynamic_cast|bit_cast)\s*<\s*" + + _CV_WEST + _DEVTYPE_QUAL + r"DeviceType\b" + _TARGET_SUFFIX + _TEMPLATE_TAIL + r"\s*>" + r"|__builtin_bit_cast\s*\(\s*" + + _CV_WEST + _DEVTYPE_QUAL + r"DeviceType\b" + _CV_EAST + r"\s*," + r"|(?:(?])" + _DEVTYPE_QUAL + r"DeviceType\b" + _CV_EAST + + r"\s*(?:\w+\s*\{|[({])\s*(?![)}])" +) RE_CUDA_INCLUDE = re.compile(r'^\s*#\s*include\s*[<"](?:vt/cuda/|cuda_runtime)') RE_PP_IF = re.compile(r"^\s*#\s*(ifdef|ifndef|if)\b(.*)$") RE_PP_ELIF = re.compile(r"^\s*#\s*elif\b(.*)$") @@ -114,6 +343,12 @@ "`{kCUDA, kXPU, kVULKAN, kMETAL, kCPU}` — a data list that " "names every platform equally, mirroring upstream's " "`platforms/__init__.py` import probe."), + "dev_cast": (1, "`FindPlatformByName`'s `static_cast(i)`: the " + "registry is an array indexed BY DeviceType, so turning a " + "slot index back into its type is the seam's own inverse, " + "not a model file naming a device by enum value. Budgeted " + "at exactly one — a second cast in this file is not the " + "registry walk and fails."), }, "include/vllm/platforms/interface.h": { "kcuda": (1, "the `is_cuda()` DEFINITION itself " @@ -334,6 +569,15 @@ def scan(root: Path) -> Result: for m in RE_IS_CUDA.finditer(code): del m found["is_cuda"].append(Hit(rel, lineno, "is_cuda", raw_lines[lineno - 1].strip())) + # `dev_cast` matches over the WHOLE stripped text, not line by line: a + # cast wrapped across two lines is the same conversion, and a + # line-oriented matcher would read the halves as two innocent lines. + # The match offset is mapped back to a 1-based line for reporting. + joined = "\n".join(code_lines) + for m in RE_DEVTYPE_CAST.finditer(joined): + lineno = joined.count("\n", 0, m.start()) + 1 + text = raw_lines[lineno - 1].strip() if lineno <= len(raw_lines) else m.group(0) + found["dev_cast"].append(Hit(rel, lineno, "dev_cast", text)) for lineno, line in enumerate(raw_lines, start=1): if RE_CUDA_INCLUDE.match(line) and not guarded[lineno - 1]: found["cuda_inc"].append(Hit(rel, lineno, "cuda_inc", line.strip())) diff --git a/scripts/device-leakage-baseline.json b/scripts/device-leakage-baseline.json index 42eec26f5..1e00af952 100644 --- a/scripts/device-leakage-baseline.json +++ b/scripts/device-leakage-baseline.json @@ -13,6 +13,7 @@ "buckets": { "kcuda": 0, "is_cuda": 0, + "dev_cast": 0, "cuda_inc": 0, "vt_ifdef": 32 } diff --git a/src/vllm/multimodal/ltx2_video.cpp b/src/vllm/multimodal/ltx2_video.cpp index 7a10e6ce2..8b580a853 100644 --- a/src/vllm/multimodal/ltx2_video.cpp +++ b/src/vllm/multimodal/ltx2_video.cpp @@ -563,8 +563,8 @@ std::unique_ptr Ltx2VideoEngine::Load(const VideoModelParams& p // what would make every later timing and every "it ran on the GPU" claim false. im.on_device = params.device != 0; if (im.on_device) { - const vt::DeviceType accelerator = - vllm::platforms::CurrentPlatform().device_type(); + const vllm::platforms::Platform& platform = vllm::platforms::CurrentPlatform(); + const vt::DeviceType accelerator = platform.device_type(); if (accelerator == vt::DeviceType::kCPU || vt::TryGetBackend(accelerator) == nullptr) { Fail("device " + std::to_string(params.device) + @@ -575,6 +575,36 @@ std::unique_ptr Ltx2VideoEngine::Load(const VideoModelParams& p "what is missing is the backend. Refusing rather than running the CPU forward " "behind an accelerator handle."); } + // The THIRD question, which the seam's own precedent asks and this file did + // not (#659). "Is there an accelerator" and "is a backend registered" are + // both true on a PARTIAL backend — Metal registers 15 of 75 ops, Tenstorrent + // a comparable slice — and both name exactly two text architectures in their + // `supports_model_architecture` allow-lists (src/vllm/platforms/metal.cpp:70, + // src/vllm/platforms/tenstorrent.cpp:55). Before the seam landed, such a + // build asked `TryGetBackend(kCUDA)`, got nullptr, and REFUSED BY NAME; after + // it, it is handed a queue and dies later inside a kernel bind with a shape + // error that says nothing about what is missing. CUDA and CPU are unaffected: + // `supports_model_architecture` defaults to true (interface.h:263) and is a + // claim only a partial backend ever narrows. + // + // The refusal above argues that serving the CPU forward behind an accelerator + // handle "would make every later timing and every 'it ran on the GPU' claim + // false". A partial backend that binds and dies is the same thing one level + // down: a device claim this build cannot honour. + // + // The key is the FAMILY string — this lane's stable registry name + // (`VideoModelParams::family`) — because the diffusion engines are reached + // through `LoadVideoEngine`, not through ModelRegistry's HF `architectures`. + if (!platform.supports_model_architecture(kLtx2VideoFamily)) { + Fail("device " + std::to_string(params.device) + " resolves to platform '" + + std::string(vt::DeviceTypeName(accelerator)) + + "', and that platform DECLINES the architecture '" + + std::string(kLtx2VideoFamily) + + "' (Platform::supports_model_architecture): it is a PARTIAL backend that has " + "not registered the kernels this model needs. The build is partial, not " + "broken. Refusing by name rather than binding a queue that would die inside a " + "kernel bind with an error that names none of this."); + } // `vt::CreateQueue(Device)`, NOT `Backend::CreateQueue()`. backend.h:212-217 // records the method as a "temporary index-0 migration shim" and says new // adapter code must use the free function, and the difference is not diff --git a/src/vllm/multimodal/minimax_h3_video.cpp b/src/vllm/multimodal/minimax_h3_video.cpp index c5f1b256b..375a0e882 100644 --- a/src/vllm/multimodal/minimax_h3_video.cpp +++ b/src/vllm/multimodal/minimax_h3_video.cpp @@ -32,6 +32,7 @@ #include "vllm/model_executor/model_loader/gguf_reader.h" #include "vllm/model_executor/model_loader/safetensors_reader.h" #include "vllm/model_executor/models/minimax_h3.h" +#include "vllm/platforms/interface.h" // CurrentPlatform() — which accelerator, if any #include "vllm/tokenizer/tokenizer.h" #include "vt/backend.h" #include "vt/dtype.h" @@ -218,11 +219,65 @@ void FillNoise(std::vector& out, uint64_t seed) { // ── the engine ─────────────────────────────────────────────────────────────── +// ── where this engine runs (#659, #660) ────────────────────────────────────── +// +// `device` is the public video ABI's selector: 0 is the CPU, 1 is "the +// accelerator". WHICH accelerator is the PLATFORM's question, not this model +// file's — the same question `ltx2_video.cpp:549-580` and `SelectQueueForModel` +// (src/vllm/entrypoints/model_loader.cpp:75-104) ask. +// +// This used to be `static_cast(device)`, which is not a mapping +// at all. It reads the ABI selector AS AN ENUM VALUE and is correct only for as +// long as `kCUDA` stays 1 in include/vt/device.h — reorder that enum and every +// H3 device-1 load silently re-points at a different backend. It also named CUDA +// without writing `kCUDA`, so the DSR ratchet's token grep scored it ZERO while +// the test asserting the very same mapping spelled the token honestly and WAS +// counted: the gate read the confession and missed the act (#660). The +// `dev_cast` bucket in scripts/check-device-leakage.py now sees it. +// +// Three questions, the same three the seam answers everywhere else: +// 1. is there an accelerator at all (`CurrentPlatform().device_type()`), +// 2. is a backend registered for it (`vt::TryGetBackend`), +// 3. can that backend actually run THIS model +// (`Platform::supports_model_architecture`, interface.h:263) — a PARTIAL +// backend (Metal 15/75 ops, Tenstorrent) must be able to decline by name +// rather than be handed a queue and die inside a kernel bind (#659). +// +// The architecture key is the family string, because that is this lane's stable +// registry name (`VideoModelParams::family`); the diffusion engines are not +// reached through ModelRegistry's HF `architectures` entry. +// +// On a CUDA box all three pass and this resolves EXACTLY the device the cast +// did. On a CPU-only build device 1 is now REFUSED here instead of returning +// kCUDA and failing one step later in `vt::GetBackend(kCUDA)`, and the refusal +// says which piece is missing. vt::DeviceType MiniMaxH3VideoDeviceType(int32_t device) { if (device != 0 && device != 1) { - throw std::runtime_error("minimax_h3 video: device must be 0 (cpu) or 1 (cuda)"); + throw std::runtime_error( + "minimax_h3 video: device must be 0 (cpu) or 1 (the accelerator this build " + "resolves)"); } - return static_cast(device); + if (device == 0) return vt::DeviceType::kCPU; + + const vllm::platforms::Platform& platform = vllm::platforms::CurrentPlatform(); + const vt::DeviceType accelerator = platform.device_type(); + if (accelerator == vt::DeviceType::kCPU || vt::TryGetBackend(accelerator) == nullptr) { + throw std::runtime_error( + "minimax_h3 video: device 1 asks for an accelerator, but no accelerator backend " + "is registered in this build (the platform seam resolves to '" + + std::string(vt::DeviceTypeName(accelerator)) + + "'). Refusing rather than naming a device this build cannot run on."); + } + if (!platform.supports_model_architecture(kMiniMaxH3VideoFamily)) { + throw std::runtime_error( + "minimax_h3 video: device 1 resolves to platform '" + + std::string(vt::DeviceTypeName(accelerator)) + "', and that platform DECLINES the " + "architecture '" + std::string(kMiniMaxH3VideoFamily) + + "' (Platform::supports_model_architecture): it is a PARTIAL backend that has not " + "registered the kernels this model needs. The build is partial, not broken. " + "Refusing by name rather than binding a queue that would die inside a kernel bind."); + } + return accelerator; } struct MiniMaxH3VideoEngine::Impl { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 7d2acd8d7..139bd6e84 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -250,6 +250,13 @@ vllm_cpp_add_test(test_ltx2_video vllm/multimodal/test_ltx2_video.cpp) vllm_cpp_add_test(test_ltx2_image_cond vllm/multimodal/test_ltx2_image_cond.cpp) target_include_directories(test_ltx2_image_cond PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/vllm/multimodal) +# #659/#660: both diffusion lanes' device resolution held against a PARTIAL +# backend that DECLINES the architecture. Its own executable for the same reason +# test_device_selection is one — it registers a fake platform and backend in the +# global registries (XPU slot, and over the CUDA slot so CurrentPlatform() +# resolves to it on a CUDA build too), and process isolation keeps that out of +# every other suite. +vllm_cpp_add_test(test_diffusion_device_seam vllm/multimodal/test_diffusion_device_seam.cpp) # LTX-2.5 DiT parity gate (row MODEL-DIFFUSION-LTX25, spec phase L2). The goldens # live next to the test source, like every other generated .inc. vllm_cpp_add_test(test_ltx2 vllm/models/test_ltx2.cpp) diff --git a/tests/scripts/test_device_leakage.py b/tests/scripts/test_device_leakage.py index a236b6de1..9e675c9b6 100644 --- a/tests/scripts/test_device_leakage.py +++ b/tests/scripts/test_device_leakage.py @@ -18,8 +18,26 @@ 3. A REDUCTION also fails until the baseline is lowered in the same commit, which is what makes it a ratchet rather than a threshold (M10-M12). 4. The escape hatch is real, bounded and LOUD (M13-M15). + 5. The `dev_cast` bucket is derived from the PROPERTY — an integer becoming a + `vt::DeviceType` outside the platform seam — and not from the one spelling + that happened to be in the tree when it was written (M20-M46, #660) — and + that what it actually enforces is a SET OF SPELLINGS rather than that + property, which is why M46 pins the DECLARED BLIND SPOTS as still blind: + the checker's message is the authority on what it enforces, so the message + and the pattern have to be able to disagree loudly. #828 tracks the + AST-level check that would enforce the property itself. python3 tests/scripts/test_device_leakage.py + +WHY (5) IS ASSERTED SO HARD. `kcuda` is the token grep `\bkCUDA\b`. +`minimax_h3_video.cpp` wrote `static_cast(device)` against +`kCUDA = 1` and scored ZERO in that bucket while the TEST that asserted the same +mapping spelled the token honestly and was counted — the gate read the confession +and missed the act (#660). A bucket that closed only `static_cast` +would be that same defect one spelling later: a regression test wearing a gate's +clothes. So every mutant below is a spelling that is NOT in the tree, and each is +asserted on its own rather than in a batch, because a batch passes when one +alternative in the pattern works. """ from __future__ import annotations @@ -64,6 +82,14 @@ ), "src/vllm/platforms/platform.cpp": ( "static const DeviceType kOrder[] = {DeviceType::kCUDA, DeviceType::kCPU};\n" + # The registry-walk inverse: an array INDEXED by DeviceType turned back + # into its type. Allowlisted at exactly one `dev_cast`, so the fixture + # has to carry it or the two-way allowlist ratchet reads the synthetic + # tree as a platform leg that lost its cast. + "Platform* Find(std::string_view n) {\n" + " const DeviceType type = static_cast(i);\n" + " return nullptr;\n" + "}\n" ), "include/vllm/platforms/interface.h": ( "bool is_cuda() const { return device_type() == DeviceType::kCUDA; }\n" @@ -86,7 +112,7 @@ # The synthetic tree's own DSR: one `kCUDA` in toy.cpp; everything else is either # allowlisted platform leg or outside the scanned roots. -BASE_BUCKETS = {"kcuda": 1, "is_cuda": 0, "cuda_inc": 0, "vt_ifdef": 0} +BASE_BUCKETS = {"kcuda": 1, "is_cuda": 0, "dev_cast": 0, "cuda_inc": 0, "vt_ifdef": 0} class Tree: @@ -356,6 +382,528 @@ def test_M19_device_legs_under_src_vt_are_not_scanned(self) -> None: rc, _out, err = self.tree.run() self.assertEqual(rc, 0, err) + # --- 5. `dev_cast` is a PROPERTY, not a spelling (#660) ------------------ + # + # The spelling already in the tree when this bucket was written was + # `static_cast(device)`. NONE of the mutants below use it. + # Each is asserted individually: a single test that planted all of them at + # once would pass while four of the five alternatives were dead. + + def plant(self, body: str) -> tuple[int, str, str]: + self.tree.append("src/vllm/model_executor/models/toy.cpp", body) + return self.tree.run() + + def test_M20_c_style_cast_to_devicetype_fails(self) -> None: + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t d) { return (vt::DeviceType)d; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M21_functional_cast_to_devicetype_fails(self) -> None: + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t d) { return vt::DeviceType(d); }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M22_cast_of_an_intermediate_int_variable_fails(self) -> None: + # The operand is neither a literal nor the parameter, so a pattern + # anchored on WHAT IS CAST rather than on the TARGET TYPE would miss it. + # The qualifier is dropped too: `DeviceType`, not `vt::DeviceType`. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(const Params& p) {\n" + " int raw = p.device;\n" + " raw = raw ? 1 : 0;\n" + " return static_cast(raw);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M23_brace_initialized_cast_to_devicetype_fails(self) -> None: + # C++17 permits list-initialising a scoped enum with a fixed underlying + # type from an integer, so this is a real, compiling laundering route. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t d) { return vt::DeviceType{d}; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M24_cast_split_across_lines_fails(self) -> None: + # clang-format wraps long casts. A line-oriented matcher reads the two + # halves as two innocent lines; the bucket matches over the whole + # comment-stripped text and maps the offset back to a line. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t device_selector_from_the_public_abi) {\n" + " return static_cast<\n" + " vt::DeviceType>(device_selector_from_the_public_abi);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + # --- and the negatives, which are what keep the bucket usable ------------ + + def test_M25_casting_a_devicetype_TO_an_integer_does_not_count(self) -> None: + # The safe direction. `std::to_string(static_cast(type))` is in the + # platform seam itself; a bucket that counted it would be unlivable. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "int Index(vt::DeviceType t) { return static_cast(t); }\n" + "size_t Slot(vt::DeviceType t) { return static_cast(t); }\n" + "const char* N(vt::DeviceType t) { return vt::DeviceTypeName(t); }\n" + "std::vector All() { return kOrder; }\n", + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + + def test_M26_a_devicetype_cast_in_prose_or_a_string_does_not_count(self) -> None: + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "// history: this was (vt::DeviceType)device before the seam landed\n" + "/* static_cast(1) is what #660 was about */\n" + 'void D() { VT_CHECK(ok, "never write vt::DeviceType(raw) here"); }\n', + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + + def test_M27_the_platform_registry_walk_is_allowlisted_exactly(self) -> None: + # platform.cpp turns a REGISTRY INDEX back into a DeviceType. That is the + # seam's own inverse, not a model file hardcoding a device — but it is + # budgeted at exactly one, so a second cast in that file still fails. + self.tree.write( + "src/vllm/platforms/platform.cpp", + "static const DeviceType kOrder[] = {DeviceType::kCUDA, DeviceType::kCPU};\n" + "Platform* Find(std::string_view n) {\n" + " const DeviceType type = static_cast(i);\n" + " return nullptr;\n" + "}\n", + ) + rc, _out, err = self.tree.run() + self.assertEqual(rc, 0, err) + self.tree.append( + "src/vllm/platforms/platform.cpp", + "DeviceType Sneak(int d) { return (DeviceType)d; }\n", + ) + rc, _out, err = self.tree.run() + self.assertEqual(rc, 1) + self.require(err, "ALLOWLIST STALE: src/vllm/platforms/platform.cpp") + self.require(err, "bucket 'dev_cast' has 2 reference(s)") + + def test_M29_a_parameter_declaration_is_not_a_c_style_cast(self) -> None: + # The false positive this bucket actually produced on the real tree, kept + # as a mutant so it cannot come back. `(vt::DeviceType /*device*/)` strips + # to `(vt::DeviceType )` and is then textually a cast applied to `const`. + # Both discriminators are exercised: the `(` glued to the function name, + # and the declarator suffix after the `)`. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "struct Sink {\n" + " virtual bool supports_on(vt::DeviceType /*device*/) const { return true; }\n" + " virtual void note (vt::DeviceType) const noexcept;\n" + "};\n" + "using Fn = void(vt::DeviceType);\n", + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + # …and the discriminator must not have cost the real thing. The operand + # asserted FIRST is a LITERAL, because that is the form the discriminator + # actually did cost: `(vt::DeviceType)1` — the device named by its raw + # enum value — is the PUREST case this bucket exists to police, and an + # identifier-only trailing class let it through while still catching + # `(vt::DeviceType)d`. A discriminator exercised only on the case it was + # tuned for is a guard that certifies itself, which is the disease this + # row is fixing; so both operand kinds are pinned here. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "vt::DeviceType RLit(int d) { return (vt::DeviceType)1; }\n", + ) + rc, _out, err = self.tree.run() + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + # A SIGNED literal is the same conversion with one more character. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "vt::DeviceType RNeg(int d) { return (vt::DeviceType)-1; }\n", + ) + rc, _out, err = self.tree.run() + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 2 > baseline 0") + # And the identifier operand, in the SAME file as the false-positive + # fixture, so the two discriminators are shown not to have cost it. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "vt::DeviceType RId(int d) { return (vt::DeviceType)d; }\n", + ) + rc, _out, err = self.tree.run() + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 3 > baseline 0") + + def test_M30_a_global_scope_qualified_target_fails(self) -> None: + # `::vt::DeviceType` and `vt::DeviceType` name ONE type. A pattern that + # recognised only the second would be the token grep again, one + # qualification later. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t d) {\n" + " return static_cast<::vt::DeviceType>(d);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M31_an_elaborated_type_specifier_target_fails(self) -> None: + # `enum vt::DeviceType` is the same type spelled the C way. It compiles. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t d) {\n" + " return static_cast(d);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M32_list_initialising_a_NAMED_declaration_fails(self) -> None: + # M23 catches the UNNAMED temporary `vt::DeviceType{d}`. Giving the same + # conversion a name is the spelling a person would actually write, and it + # is the one M23 alone let through. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t d) {\n" + " vt::DeviceType dt{d};\n" + " return dt;\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M33_pointer_punning_onto_a_devicetype_fails(self) -> None: + # The ONE form `reinterpret_cast` can legally take: casting to a scoped + # enum directly is ill-formed, so without the pointer target the three + # non-`static_cast` keywords in the pattern are decoration — they could + # only ever match code that does not compile. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(uint8_t r) {\n" + " return *reinterpret_cast(&r);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M34_a_function_RETURNING_DeviceType_is_not_a_conversion(self) -> None: + # The false positive M32's declaration form produces if it accepts `(` as + # well as `{`. These three shapes are all real sites in this repository + # (`MiniMaxH3VideoDeviceType`, `ResolveExplicitDeviceType`), and reading a + # function definition as a cast would red the tree for declaring the very + # seam this bucket wants people to use. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "vt::DeviceType MiniMaxH3VideoDeviceType(int32_t device);\n" + "vt::DeviceType ResolveExplicitDeviceType(const Params& p) { return kCpu; }\n" + "enum vt::DeviceType Named(int d);\n" + "::vt::DeviceType AlsoNamed(int d) { return kCpu; }\n", + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + + # --- M35-M40: the row's own thesis, turned on the instrument a THIRD time -- + # + # M33 made the pointer target real, and the docstring then claimed the bucket + # is anchored on "the TARGET TYPE, not on the operand and not on one cast + # keyword". Four spellings that DO write the target type at the conversion + # site still scored zero, so the message asserted coverage the pattern did not + # have — which is #660 exactly, one sigil later. Every mutant below is + # compile-verified legal C++ (g++ -std=c++20 -Wall -Wextra, exit 0), because a + # "miss" that does not compile is not a miss. + + def test_M35_a_REFERENCE_target_fails(self) -> None: + # `reinterpret_cast(raw)` is the same pun as M33's + # `*reinterpret_cast(&raw)` — the standard defines the + # reference form in terms of the pointer form — and it is one character + # shorter. A `\*?` that admits a star but not an ampersand is a spelling + # grep wearing the target type's clothes. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(uint8_t& raw) {\n" + " return reinterpret_cast(raw);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M36_an_EAST_const_target_fails(self) -> None: + # `vt::DeviceType const` and `const vt::DeviceType` are ONE type. The + # pattern took the west spelling only, so the east one laundered the same + # conversion. + # + # This comment used to say "all three places a cv-qualifier can sit" and + # then list three. There are FOUR: the named cast, the named cast with a + # reference (the form that compiles WITHOUT -Wignored-qualifiers, so it + # survives a -Werror build), the C-style cast — and the DECLARATION form, + # `vt::DeviceType const d{raw}`, which had no cv-group in the pattern at + # all. An enumeration that certifies its own completeness is the exact + # defect this row exists to fix (it is M29's disease, one round later), so + # the count is now the measured one and the fourth case is asserted below. + # Each is asserted on its own, so one working alternative cannot certify + # the others. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t d) { return static_cast(d); }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + rc, _out, err = self.plant( + "const vt::DeviceType& Ref(const vt::DeviceType& t) {\n" + " return static_cast(t);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 2 > baseline 0") + rc, _out, err = self.plant( + "vt::DeviceType CStyle(int32_t d) { return (vt::DeviceType const)d; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 3 > baseline 0") + # The FOURTH place, and the one the "all three" comment above hid: the + # DECLARATION form. `vt::DeviceType const d{raw}` and + # `vt::DeviceType volatile d{raw}` both compile (g++ -std=c++20 -Wall + # -Wextra, exit 0) and both scored 0, because alternative (3) carried no + # cv-group. The member-declaration spelling — a named device CONSTANT + # initialised from the literal `1` — is the one a person actually writes. + rc, _out, err = self.plant( + "vt::DeviceType DeclConst(uint8_t raw) {\n" + " vt::DeviceType const d{raw};\n" + " return d;\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 4 > baseline 0") + rc, _out, err = self.plant( + "uint8_t DeclVol(uint8_t raw) {\n" + " vt::DeviceType volatile d{raw};\n" + " return static_cast(d);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 5 > baseline 0") + rc, _out, err = self.plant( + "struct Cfg { vt::DeviceType const kD{1}; };\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 6 > baseline 0") + + def test_M37_a_UNARY_operand_after_a_c_style_cast_fails(self) -> None: + # `(vt::DeviceType)*cursor` is the WIRE-FORMAT DECODE the checker's own + # docstring names as the expected `DSR-ALLOW` case — so the one site the + # bucket predicted it would meet was the one its trailing class could not + # see. The class admitted a leading SIGN but not a dereference. `~` and + # `!` are the same hole. Each asserted individually. + rc, _out, err = self.plant( + "vt::DeviceType Decode(const uint8_t* cursor) { return (vt::DeviceType)*cursor; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + rc, _out, err = self.plant( + "vt::DeviceType Inv(uint8_t m) { return (vt::DeviceType)~m; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 2 > baseline 0") + rc, _out, err = self.plant( + "vt::DeviceType Not(bool f) { return (vt::DeviceType)!f; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 3 > baseline 0") + + def test_M38_a_POINTER_TO_POINTER_target_fails(self) -> None: + # `\*?` is ONE star. Two stars is the same pun through one more level of + # indirection, and it compiles. + rc, _out, err = self.plant( + "vt::DeviceType** Table(uint8_t** p) { return reinterpret_cast(p); }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M39_bit_cast_to_devicetype_fails(self) -> None: + # The docstring used to list `std::bit_cast` as unreachable "because no + # spelling of the target type appears at the conversion site". It does + # appear — `std::bit_cast(raw)` writes it in full — so the + # entry gave a REASON that was false for the case it listed. `bit_cast` + # is now a cast keyword; `memcpy` and union punning keep the entry, and + # keep the reason, because for them it is true. + rc, _out, err = self.plant( + "vt::DeviceType Pun(uint8_t raw) { return std::bit_cast(raw); }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M40_the_widening_did_not_swallow_declarators(self) -> None: + # What M35-M38 could have cost, pinned as a negative. A run of `*`/`&` in + # a named cast's target and a `*` in the C-style trailing class are both + # reachable only after a cast token, but a POINTER- or REFERENCE-returning + # declaration and a ref-qualified member function are the shapes that look + # closest to them. The last two carry a SPACE before the `(`, which is + # what defeats the glued-identifier discriminator and leaves only the + # declarator-suffix guard — the reason `&` is deliberately NOT in the + # C-style trailing class, and the reason the C-style POINTER pun is + # declared blind in the checker's docstring instead of being closed here. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "vt::DeviceType* Slot(int i);\n" + "vt::DeviceType& SlotRef(int i);\n" + "const vt::DeviceType& Peek();\n" + "vt::DeviceType** Table();\n" + "struct Sink {\n" + " void note (vt::DeviceType*) &;\n" + " void note2 (vt::DeviceType*) &&;\n" + " void note3 (vt::DeviceType) volatile;\n" + "};\n" + "std::vector AllPtrs();\n", + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + + # --- M41-M46: round 4. The blind-spot ENTRY bound two members under one + # + # reason, and the reason was true of only one of them. The old note covered + # "a C-STYLE cast whose target is a pointer OR REFERENCE" with the single + # reason "both are `)` followed by `&`" — true of `*(vt::DeviceType*)&x`, + # false of `(vt::DeviceType&)raw`, whose next character is an identifier. So + # the reference form was reachable by widening only INSIDE the parens, at + # zero cost, while the entry declared it blind. That is the same defect as + # rounds 1-3, moved from the pattern into the per-entry REASON, which is why + # every closure below is paired with an assertion that its blind-spot twin is + # STILL blind (M46) rather than with a wider claim. + # + # Every mutant below is compile-verified legal C++ (g++ -std=c++20 -Wall + # -Wextra -fsyntax-only, exit 0) and measured individually at 0 before / 1 + # after, over the same 760-file scan roots that keep `dev_cast` at its single + # allowlisted hit. + + def test_M41_a_C_STYLE_pointer_or_reference_target_fails(self) -> None: + # Widening INSIDE the parens, where no declarator can be confused with a + # target. `(vt::DeviceType&)raw` is the C-style spelling of M35's pun and + # `(vt::DeviceType*)vp` is a plain pointer cast off a `void*`; both + # compile, both scored 0. Asserted separately, because one alternative + # working is not the other one working. + rc, _out, err = self.plant( + "vt::DeviceType Ref(uint8_t& raw) { return (vt::DeviceType&)raw; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + rc, _out, err = self.plant( + "vt::DeviceType* Ptr(void* vp) { return (vt::DeviceType*)vp; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 2 > baseline 0") + + def test_M42_bit_cast_may_name_its_SOURCE_type_too(self) -> None: + # M39 added `bit_cast` on the argument that it spells the target type at + # the conversion site. It is a FUNCTION template with TWO parameters, not + # a cast operator with one, so `From` may be written — and terminating the + # target at `>` assumed the one-argument shape the four real casts have. + # `std::bit_cast(raw)` compiles and scored + # 0. Over-matching is not a risk: the scanned roots contain ZERO + # `bit_cast` and ZERO `__builtin_bit_cast` occurrences — measured over the + # comment-stripped text of all 760 files, with `static_cast` = 9950 as the + # positive control in the same pass, so the zero is an absence and not a + # broken grep. + rc, _out, err = self.plant( + "vt::DeviceType Pun(uint8_t raw) {\n" + " return std::bit_cast(raw);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M43_a_cv_qualifier_INSIDE_the_pointer_run_fails(self) -> None: + # M35 made the target suffix a run of `[*&]`, and the docstring then + # claimed a REFERENCE target. `vt::DeviceType* const&` is exactly that — + # a reference to a const pointer — and the run stopped at the first + # cv-qualifier. One position to the right of round 2's own finding. + rc, _out, err = self.plant( + "vt::DeviceType* const& Slot(vt::DeviceType** p) {\n" + " return reinterpret_cast(*p);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M44_the_BUILTIN_spelling_of_bit_cast_fails(self) -> None: + # `__builtin_bit_cast(vt::DeviceType, raw)` compiles under g++ 13.3 and + # names the target type at the conversion site just as fully as + # `std::bit_cast` does — but it is not a template-id, so alternative (1)'s + # `<…>` anchor cannot reach it and alternative (2)'s glued-identifier + # discriminator rejects it. Listing `bit_cast` as caught while this + # spelling scored 0 would be the same overclaim in a smaller font. + rc, _out, err = self.plant( + "vt::DeviceType Pun(uint8_t raw) {\n" + " return __builtin_bit_cast(vt::DeviceType, raw);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M45_sizeof_and_alignof_convert_nothing(self) -> None: + # A false positive the widening WOULD have grown. `sizeof (vt::DeviceType) + # + 1` already scored 1 before this round — `sizeof` is not glued to its + # paren, so the identifier discriminator passes, and `+` is in the + # trailing class — and admitting `*` inside the parens would have extended + # it to the pointer spelling. `sizeof` and `alignof` convert nothing, so + # both are excluded outright rather than left as a gap made wider. The + # first two of these are RED against the SHIPPED pattern. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "size_t A() { return sizeof (vt::DeviceType) + 1; }\n" + "size_t B() { return alignof (vt::DeviceType) + 1; }\n" + "size_t C() { return sizeof (vt::DeviceType*) + 1; }\n" + "size_t D() { return sizeof(vt::DeviceType) + 1; }\n", + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + + def test_M46_the_declared_blind_spots_are_STILL_blind(self) -> None: + # The other half of a per-entry reason: the docstring says these three + # score 0, so this pins that they do. If a later widening closes one, this + # test goes RED and the message has to be corrected in the same change — + # which is the only mechanism that keeps a checker's message the authority + # on what it enforces. It is not an argument that they SHOULD stay blind. + # + # * `*(vt::DeviceType*)&x` — closing it needs `&` in the TRAILING class, + # which is what makes `void note (vt::DeviceType*) &;` a false + # positive (M40 pins that negative); + # * `(vt::DeviceType)'\x01'` — character literals are blanked to + # whitespace by strip_comments_and_strings before the pattern runs, so + # the operand is gone by match time; + # * `std::memcpy(&dt, &raw, sizeof(vt::DeviceType))` — there is no cast + # EXPRESSION to anchor on. Note it DOES name the type at the site, so + # the old shared reason ("the type is named at the declaration, never + # at the conversion") was false for it. + # + # All three compile (g++ -std=c++20 -Wall -Wextra -fsyntax-only, exit 0). + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "vt::DeviceType Pun(uint8_t x) { return *(vt::DeviceType*)&x; }\n" + "vt::DeviceType Chr() { return (vt::DeviceType)'\\x01'; }\n" + "vt::DeviceType Mem(uint8_t raw) {\n" + " vt::DeviceType dt{};\n" + " std::memcpy(&dt, &raw, sizeof(vt::DeviceType));\n" + " return dt;\n" + "}\n", + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + # The positive control for that zero, in the same test: the pattern is + # alive, and one character more of operand IS caught. A null result from a + # dead regex would otherwise prove nothing. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "vt::DeviceType Buf(const uint8_t* b) { return (vt::DeviceType)b[0]; }\n", + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 1) + + def test_M28_dsr_allow_exempts_a_dev_cast_and_says_so_loudly(self) -> None: + # The legitimate case the risk register names: a deserialization boundary + # that reads a device off the wire. It buys an exemption only with a row + # id and a reason, and it is printed on every run. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "// DSR-ALLOW(S9): wire-format decode, validated against kNumDeviceTypes\n" + "vt::DeviceType Decode(uint8_t b) { return (vt::DeviceType)b; }\n", + ) + rc, out, err = self.tree.run() + self.assertEqual(rc, 0, err) + self.require(out, "DSR-ALLOW exemptions in force: 1") + self.require(out, "[dev_cast]") + class RealTreeTests(unittest.TestCase): """Hard expectations about THIS repository, not a synthetic mutant.""" @@ -406,6 +954,27 @@ def test_server_main_dsr_allows_cover_only_the_profiler_guard(self) -> None: f"line {index + 1} has no DSR-ALLOW on it or directly above it", ) + def test_the_diffusion_lanes_resolve_device_through_the_seam(self) -> None: + # #660's actual site, pinned as a fact about THIS tree rather than as a + # number in a baseline. The positive control is in the same assertion: + # the pattern must still fire on the defect's own text, or a null result + # below would prove only that the regex is broken. + defect = "return static_cast(device);" + self.assertTrue( + dl.RE_DEVTYPE_CAST.search(defect), + "positive control failed: the dev_cast pattern no longer matches the " + "very line #660 was filed about, so its absence below proves nothing", + ) + for rel in ( + "src/vllm/multimodal/minimax_h3_video.cpp", + "src/vllm/multimodal/ltx2_video.cpp", + ): + code = "\n".join( + dl.strip_comments_and_strings((ROOT / rel).read_text(encoding="utf-8")) + ) + found = [m.group(0) for m in dl.RE_DEVTYPE_CAST.finditer(code)] + self.assertEqual(found, [], f"{rel} casts an integer into a DeviceType") + def test_every_allowlisted_path_exists(self) -> None: # A stale allowlist entry is a silent exemption for a file that may later # be recreated with different contents. diff --git a/tests/vllm/models/test_minimax_h3_video_fold.cpp b/tests/vllm/models/test_minimax_h3_video_fold.cpp index afad82be7..fada8d9fc 100644 --- a/tests/vllm/models/test_minimax_h3_video_fold.cpp +++ b/tests/vllm/models/test_minimax_h3_video_fold.cpp @@ -39,6 +39,7 @@ #include "vllm/entrypoints/openai/video_api.h" #include "vllm/model_executor/model_loader/gguf_reader.h" #include "vllm/model_executor/models/minimax_h3.h" +#include "vllm/platforms/interface.h" // CurrentPlatform() — the seam device 1 resolves through #include "minimax_h3_video_fold_fixture.h" #include "vt/backend.h" @@ -116,14 +117,42 @@ class CountingCudaBackend final : public vt::Backend { int create_queue_calls = 0; }; +// The PLATFORM half of the same fake. #660: device 1 used to be +// `static_cast(1)`, so registering a backend alone was enough to +// fake an accelerator — the engine never asked whether this build HAS one. It +// asks now, through `CurrentPlatform()`, so a test that wants the accelerator +// path has to supply a platform as well as a backend. That is not a harness +// concession; it is the defect being visible: a build with a CUDA backend +// registered and no CUDA platform is not a build that runs on CUDA. +class FakeCudaPlatform final : public vllm::platforms::Platform { + public: + explicit FakeCudaPlatform(vt::Backend& backend) : backend_(backend) {} + vt::DeviceType device_type() const override { return vt::DeviceType::kCUDA; } + vt::Backend& backend() const override { return backend_; } + vllm::platforms::DeviceCapability get_device_capability() const override { return {}; } + std::vector supported_dtypes() const override { return {vt::DType::kBF16}; } + vllm::platforms::ResidencyPolicy residency_policy() const override { return {}; } + + private: + vt::Backend& backend_; +}; + class ScopedCudaBackendRegistration { public: explicit ScopedCudaBackendRegistration(vt::Backend* replacement) - : previous_(vt::TryGetBackend(vt::DeviceType::kCUDA)) { + : previous_(vt::TryGetBackend(vt::DeviceType::kCUDA)), + previous_platform_(vllm::platforms::HasPlatform(vt::DeviceType::kCUDA) + ? &vllm::platforms::GetPlatform(vt::DeviceType::kCUDA) + : nullptr), + platform_(*replacement) { vt::RegisterBackend(vt::DeviceType::kCUDA, replacement); + vllm::platforms::RegisterPlatform(vt::DeviceType::kCUDA, &platform_); } ~ScopedCudaBackendRegistration() { if (previous_ != nullptr) vt::RegisterBackend(vt::DeviceType::kCUDA, previous_); + if (previous_platform_ != nullptr) { + vllm::platforms::RegisterPlatform(vt::DeviceType::kCUDA, previous_platform_); + } } ScopedCudaBackendRegistration(const ScopedCudaBackendRegistration&) = delete; @@ -131,6 +160,8 @@ class ScopedCudaBackendRegistration { private: vt::Backend* previous_; + vllm::platforms::Platform* previous_platform_; + FakeCudaPlatform platform_; }; void CheckAgainstGoldens(const std::string& out_dir) { @@ -159,9 +190,52 @@ void CheckAgainstGoldens(const std::string& out_dir) { TEST_CASE("minimax_h3 video fold: ABI device selectors map through DeviceType") { CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(0) == vt::DeviceType::kCPU); - CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(1) == vt::DeviceType::kCUDA); CHECK_THROWS(vllm::multimodal::MiniMaxH3VideoDeviceType(-1)); CHECK_THROWS(vllm::multimodal::MiniMaxH3VideoDeviceType(2)); + + // Selector 1 is RESOLVED through the platform seam, not cast from the integer + // (#660: `static_cast(1)` was kCUDA only because kCUDA happens + // to be enum value 1). So the answer depends on what this build registered, + // and this case asserts BOTH arms rather than skipping either — a skip here + // would leave the CPU-only build, which is the one the defect hid on, + // unmeasured. + const vllm::platforms::Platform& platform = vllm::platforms::CurrentPlatform(); + const vt::DeviceType accelerator = platform.device_type(); + const bool have_backend = + accelerator != vt::DeviceType::kCPU && vt::TryGetBackend(accelerator) != nullptr; + // The THIRD question, asked here because the SOURCE asks it. A predicate that + // asks only the first two describes a different function than the one under + // test: on a PARTIAL backend (Metal, Tenstorrent) both of the first two are + // true, the source correctly refuses BY NAME, and a two-question predicate + // routes that correct refusal into the `== accelerator` arm — where it + // surfaces as an uncaught exception. That is a false RED on precisely the + // build class #659 exists to serve, and it is invisible on the CPU and CUDA + // boxes that run the gates, which is this row's own thesis about #659. + const bool accepts_architecture = + have_backend && + platform.supports_model_architecture(vllm::multimodal::kMiniMaxH3VideoFamily); + + if (accepts_architecture) { + // On the CUDA box this is byte-for-byte the old answer. + CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(1) == accelerator); + } else { + // The assertion the cast could never make: device 1 is REFUSED by name + // instead of returning kCUDA and failing one step later inside + // `vt::GetBackend(kCUDA)`. WHICH refusal is itself asserted — a partial + // backend must be told it is partial, not told its backend is missing, and + // a right refusal for a wrong reason is a wrong diagnosis that reads as a + // right one. + const std::string want = + have_backend ? "DECLINES" : "no accelerator backend is registered"; + try { + (void)vllm::multimodal::MiniMaxH3VideoDeviceType(1); + FAIL("device 1 must be refused when this build cannot honour it"); + } catch (const std::exception& e) { + const std::string msg = e.what(); + INFO(msg); + CHECK(msg.find(want) != std::string::npos); + } + } } TEST_CASE("minimax_h3 video fold: CUDA load creates exactly one queue") { diff --git a/tests/vllm/multimodal/test_diffusion_device_seam.cpp b/tests/vllm/multimodal/test_diffusion_device_seam.cpp new file mode 100644 index 000000000..f554e9230 --- /dev/null +++ b/tests/vllm/multimodal/test_diffusion_device_seam.cpp @@ -0,0 +1,214 @@ +// #659 / #660 — the diffusion lane's device seam, held against a PARTIAL +// backend. +// +// The two lanes ask "where does this engine run" and the answer has three parts: +// is there an accelerator, is a backend registered for it, and can that backend +// actually run THIS model. Only the first two were being asked. The third, +// `Platform::supports_model_architecture` (include/vllm/platforms/interface.h), +// exists precisely so a partial backend can decline BY NAME — Metal registers 15 +// of 75 ops and Tenstorrent a comparable slice, and both name exactly two +// architectures (src/vllm/platforms/metal.cpp:70, +// src/vllm/platforms/tenstorrent.cpp:55). On such a build a `device = 1` +// diffusion load WAS refused by name and became a queue bind that dies later +// inside a kernel with a shape error. +// +// CUDA cannot see any of this: `supports_model_architecture` defaults to true, +// so on the box that runs the gates all three questions have always passed. That +// is exactly why the guard needs a platform this test brings with it. +// +// A SEPARATE EXECUTABLE, for the reason tests/vllm/entrypoints/ +// test_device_selection.cpp is one: it registers a fake platform and backend in +// the otherwise-unused XPU slot AND over the CUDA slot, so `CurrentPlatform()` +// resolves to the fake whether or not this build has a real CUDA platform. +// Process isolation keeps those global registries out of every other suite. +// +// WHY THE FAKE REPORTS kXPU. `static_cast(1)` is `kCUDA` +// (include/vt/device.h: kCPU = 0, kCUDA = 1). `kXPU` is 4. So an assertion that +// device 1 resolves to kXPU is one the old integer cast could not satisfy under +// any circumstances — it is the seam being asked, not a constant being returned. +#include + +#include +#include +#include +#include +#include +#include + +#include "vllm/multimodal/ltx2_video.h" +#include "vllm/multimodal/minimax_h3_video.h" +#include "vllm/platforms/interface.h" +#include "vt/backend.h" +#include "vt/device.h" + +namespace { + +class FakeXpuBackend final : public vt::Backend { + public: + void* Alloc(size_t bytes) override { return std::malloc(bytes == 0 ? 1 : bytes); } + void Free(void* p) override { std::free(p); } + void Memset(vt::Queue&, void* p, int value, size_t bytes) override { + std::memset(p, value, bytes); + } + void Copy(vt::Queue&, void* dst, const void* src, size_t bytes) override { + std::memcpy(dst, src, bytes); + } + vt::Queue CreateQueue() override { + return vt::Queue{vt::Device{vt::DeviceType::kXPU, 0}, nullptr}; + } + bool UnifiedMemory() const override { return true; } +}; + +// A PARTIAL backend, shaped exactly like MetalPlatform and TenstorrentPlatform: +// a real accelerator with a registered backend that names the architectures +// whose kernels it has, and declines everything else. `accepts_everything` flips +// it into a COMPLETE backend so the same process can prove the guard does not +// refuse a configuration that works. +class PartialXpuPlatform final : public vllm::platforms::Platform { + public: + explicit PartialXpuPlatform(FakeXpuBackend& backend) : backend_(backend) {} + + vt::DeviceType device_type() const override { return vt::DeviceType::kXPU; } + vt::Backend& backend() const override { return backend_; } + vllm::platforms::DeviceCapability get_device_capability() const override { return {}; } + std::vector supported_dtypes() const override { return {vt::DType::kBF16}; } + vllm::platforms::ResidencyPolicy residency_policy() const override { return {}; } + + bool supports_model_architecture(std::string_view architecture) const override { + if (accepts_everything) return true; + return architecture == "OPTForCausalLM"; + } + + bool accepts_everything = false; + + private: + FakeXpuBackend& backend_; +}; + +FakeXpuBackend& Backend() { + static FakeXpuBackend backend; + return backend; +} + +PartialXpuPlatform& Platform() { + static PartialXpuPlatform platform(Backend()); + return platform; +} + +// Registered into the CUDA slot as well as its own: `CurrentPlatform()` walks +// {kCUDA, kROCM, kXPU, …} and returns the first REGISTERED entry +// (src/vllm/platforms/platform.cpp), so without this a CUDA build would resolve +// to the real CUDA platform and the guard would never be exercised. +void RegisterPartialAccelerator(bool accepts_everything) { + vt::RegisterBackend(vt::DeviceType::kXPU, &Backend()); + vllm::platforms::RegisterPlatform(vt::DeviceType::kXPU, &Platform()); + vllm::platforms::RegisterPlatform(vt::DeviceType::kCUDA, &Platform()); + Platform().accepts_everything = accepts_everything; +} + +// A path that does not exist. If the capability guard were reverted, the LTX-2.5 +// load would sail past it and die opening this instead — a DIFFERENT message, +// which is what makes the assertions below fail for the intended reason rather +// than by accident. +vllm::multimodal::VideoModelParams DeviceOneParams() { + vllm::multimodal::VideoModelParams mp; + mp.family = vllm::multimodal::kLtx2VideoFamily; + mp.dit_path = "/nonexistent/ltx2-dit-that-is-never-opened.safetensors"; + mp.device = 1; + return mp; +} + +std::string LoadError(const vllm::multimodal::VideoModelParams& mp) { + try { + (void)vllm::multimodal::Ltx2VideoEngine::Load(mp); + } catch (const std::exception& e) { + return e.what(); + } + return ""; +} + +} // namespace + +TEST_CASE("ltx2 video: a platform that DECLINES the architecture refuses device 1 by name") { + RegisterPartialAccelerator(/*accepts_everything=*/false); + + // Both of the questions this file already asked PASS here: the platform is an + // accelerator, and a backend is registered for it. Without the third question + // the load proceeds into the DiT. + REQUIRE(vllm::platforms::CurrentPlatform().device_type() == vt::DeviceType::kXPU); + REQUIRE(vt::TryGetBackend(vt::DeviceType::kXPU) != nullptr); + REQUIRE_FALSE(vllm::platforms::CurrentPlatform().supports_model_architecture( + vllm::multimodal::kLtx2VideoFamily)); + + const std::string msg = LoadError(DeviceOneParams()); + INFO(msg); + REQUIRE_FALSE(msg.empty()); + // BY NAME: the platform, the architecture, and the fact that the backend + // declined rather than that something was malformed. + CHECK(msg.find("xpu") != std::string::npos); + CHECK(msg.find(vllm::multimodal::kLtx2VideoFamily) != std::string::npos); + CHECK(msg.find("DECLINES") != std::string::npos); + CHECK(msg.find("supports_model_architecture") != std::string::npos); + // And it must not be the OTHER refusal: a backend IS registered here, so + // blaming a missing one would be a wrong diagnosis that reads as a right one. + CHECK(msg.find("no accelerator backend is registered") == std::string::npos); + // Nor the shape of failure the guard exists to prevent: opening the DiT. + CHECK(msg.find("/nonexistent/") == std::string::npos); +} + +TEST_CASE("ltx2 video: a COMPLETE backend is not refused by the capability guard") { + // The risk the guard carries is refusing a configuration that works. A + // platform that accepts the architecture must get past this clause and fail + // only on the thing that is genuinely wrong — the missing checkpoint. + RegisterPartialAccelerator(/*accepts_everything=*/true); + + const std::string msg = LoadError(DeviceOneParams()); + INFO(msg); + REQUIRE_FALSE(msg.empty()); + CHECK(msg.find("DECLINES") == std::string::npos); + CHECK(msg.find("supports_model_architecture") == std::string::npos); + // POSITIVELY: the load got PAST the capability clause and failed on the thing + // that is genuinely wrong — the checkpoint that does not exist. Asserting only + // the two absences above would pass on any OTHER wrong failure (a refusal + // reworded, a throw from an earlier clause, a message that never mentions the + // path), which would leave "the guard does not refuse a working configuration" + // unproven while reading green. Case 1 at the top of this file asserts the + // mirror image of this line; the pair is what pins the guard to exactly one + // arm each. + CHECK(msg.find("/nonexistent/ltx2-dit-that-is-never-opened.safetensors") != + std::string::npos); +} + +TEST_CASE("minimax_h3 video: device 1 RESOLVES through the seam, it is not enum value 1") { + RegisterPartialAccelerator(/*accepts_everything=*/true); + + // kXPU is 4. The integer cast this replaced returned kCUDA (1) for any build, + // any platform, forever; it could not produce this answer. + CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(1) == vt::DeviceType::kXPU); + CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(1) != vt::DeviceType::kCUDA); + + // The rest of the public contract is untouched by the change. + CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(0) == vt::DeviceType::kCPU); + CHECK_THROWS(vllm::multimodal::MiniMaxH3VideoDeviceType(-1)); + CHECK_THROWS(vllm::multimodal::MiniMaxH3VideoDeviceType(2)); +} + +TEST_CASE("minimax_h3 video: a platform that DECLINES the architecture refuses device 1") { + RegisterPartialAccelerator(/*accepts_everything=*/false); + + std::string msg; + try { + (void)vllm::multimodal::MiniMaxH3VideoDeviceType(1); + FAIL("device 1 must be refused when the resolved platform declines the architecture"); + } catch (const std::exception& e) { + msg = e.what(); + } + INFO(msg); + CHECK(msg.find("xpu") != std::string::npos); + CHECK(msg.find(vllm::multimodal::kMiniMaxH3VideoFamily) != std::string::npos); + CHECK(msg.find("DECLINES") != std::string::npos); + CHECK(msg.find("no accelerator backend is registered") == std::string::npos); + + // 0 still resolves, because the CPU asks none of these questions. + CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(0) == vt::DeviceType::kCPU); +}