diff --git a/.agents/issue-index.md b/.agents/issue-index.md index 49e3fca0c..035f763da 100644 --- a/.agents/issue-index.md +++ b/.agents/issue-index.md @@ -224,6 +224,7 @@ rather than merged. `scripts/check-agent-record.py` gates both. | [#691](https://github.com/mudler/vllm.cpp/issues/691) | `ROAD-V1-LTX25` | The LTX-2.5 out-of-scope ledger's reachable/marker split gates the MESSAGE TEXT, not reachability: give `kLoraFusion` a product call site and the ledger test still passes while the shipped refusal tells the caller `DECLARED, NOT REQUESTABLE` right after they requested it. Copy the anti-tautological shape of the extras inventory gate | bug | | [#692](https://github.com/mudler/vllm.cpp/issues/692) | `ROAD-V1-LTX25` | Campaign spec `ltx-2-5.md:291` still records the FABRICATED `multishot` arm as owed after row `LTX25-RETIRE-DEAD-ARMS` retired it; same line needs `CFG parallelism` renamed to multi-GPU. Operator-owned file, so filed rather than edited by that row | bug | | [#886](https://github.com/mudler/vllm.cpp/issues/886) | `POLICY-NOTHING-LANDS-DEAD` | The seam checkers police where a capability routes and nothing polices whether anything reaches it, so a change can land wired downward and driven only by its own test, as tensor parallelism did (guide [`reachability.md`](reachability.md)) | feature | +| [#828](https://github.com/mudler/vllm.cpp/issues/828) | — | `check-device-leakage.py`'s `dev_cast` bucket enforces a set of SPELLINGS, not the property "an integer becomes a `vt::DeviceType`": four review rounds each found a spelling the previous round's message already claimed, every one closing at ZERO hits, so widening is not the answer. The structural fix is an AST-level check (clang tooling), where the destination type canonicalises and the source type is known. Listed under `## Owed` in [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md) | feature | | [#673](https://github.com/mudler/vllm.cpp/issues/673) | — | LTX-2.5 prompt-AdaLN: the row's checkpoint-derived evidence is MANUAL and host-local. `LTX2_CHECKPOINT_ROOT` is set by no workflow (`grep -rn CHECKPOINT_ROOT .github/` exits 1 with zero hits, positive control matches in `tests/` and `.agents/`), so CI executes 784 of 9031 assertions — 8.7%, measured 2026-08-15 — of `test_ltx2_video`, at a case count identical in both configurations, and `scripts/measure-ltx2-prompt-adaln.py` is a manual tool no gate invokes. Listed under `## Owed` in [`ltx25-prompt-adaln.md`](specs/ltx25-prompt-adaln.md) | bug | | [#900](https://github.com/mudler/vllm.cpp/issues/900) | — | LTX-2.5 prompt-AdaLN is REACHED from `vllm_video_generate` but no test ENTERS through that entry point and observes it: deleting the term's only consumer (`ltx2_dit.cpp:140`) takes `test_ltx2` red at 3 of 43 cases while `test_ltx2_video` stays GREEN at 37 of 37, so the ABI-level gate measures that the pipeline runs rather than that this capability is in it. The second half of `AGENTS.md` `## Nothing lands dead`, whose rule post-dates the row (#886). Listed under `## Owed` in [`ltx25-prompt-adaln.md`](specs/ltx25-prompt-adaln.md) | bug | | [#891](https://github.com/mudler/vllm.cpp/issues/891) | `MODEL-MM-qwen3-5-qwen3-5-moe-for-conditional-generation` | MoE vision tower (M2/M3): image and video for `Qwen3_5MoeForConditionalGeneration` — the dense arm has it, the MoE arm does not | 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..8408d2ed0 --- /dev/null +++ b/.agents/specs/ltx25-device-seam-sibling.md @@ -0,0 +1,726 @@ +# 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 @ 11cc1d589` — anchored on the base +SHA for the same reason §0(b)'s `minimax_h3_video.cpp:221-226 @ 11cc1d589` +citation below is, because this row rewrites that block and an unanchored number +would point into the middle of the repair the moment it lands — 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:98`: + +```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 @ 62406c30e` — +anchored, because this row rewrites that file's head and the line is now `:224` +— 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 @ 62406c30e` asserts +`MiniMaxH3VideoDeviceType(1) == vt::DeviceType::kCUDA` — anchored, because this +row is what rewrites that assertion and the file now carries it at `:220` +reading `== accelerator`. 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:69` 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:60-105`), 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 @ 62406c30e` 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 @ 62406c30e` 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. Every +number in this paragraph is `@ 62406c30e`: on the branch the four assertions sit +at `:192-194` and the two arms that replace `:162` at `:218-238`. + +**`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:98`, and it mirrors that +site's *question* while inverting its *answer*. `:98`'s capability test lives on +the `kAuto` path, whose response to a decline is to fall through to `:104` 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:72-73` 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:610-613` 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:98` were re-derived on the merged tree +and all three still hold. The `minimax_h3_video.cpp:221-226 @ 11cc1d589` +citation in §0 is anchored on the base SHA for the same reason. + +**The completeness claim this paragraph originally carried was false, and round 4 +withdrew it.** It said "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". `be9b0a6fd` repeated it as "thirty-one +citations, five did not hold", and both were counting a set that did not include +the citations this branch's own edits had just rotted. Round 4 found six more +(§"Findings from review round 4"), all of them stale *inside this pull request*, +two of them seven and fifteen lines (measured at `7502004aa`) from §0(b)'s +`minimax_h3_video.cpp:221-226 @ 11cc1d589` citation, which SHA-anchors itself +with exactly the reasoning that applies to them. An enumeration that +certifies its own completeness is the defect this row keeps finding in its own +instruments, and it found it here in the record rather than in the checker. +No paragraph in this spec now claims that every anchor was checked; §"Findings +from review round 4" states what was re-derived, how, and what the method cannot +see. + +## 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. + +**Re-derived again at the `4a4ab89cb` merge, and it rotted again — as predicted.** +The scanned set is **776 files**, with the controls at `\bDeviceType\b` = **163** +and `\bkCUDA\b` = **18**. `kCUDA` is unchanged across seventeen commits of main +and `DeviceType` moved by one, which is the invariant this section actually +claims; the file count moved by eleven, which is the thing it says will rot. Two +notes for the next reader, because each cost a probe here and each made a correct +figure look wrong. The controls are counted over the +**comment-and-string-stripped** text — the text the pattern is actually matched +against — and NOT over raw source, where the same two patterns read **177 / 82**. +And the enumeration must be the checker's own `rglob` over `SCAN_ROOTS`: a +`git ls-tree` walk filtered with a string prefix returns **777**, because +`include/vllm.h` starts with the characters `include/vllm` while sitting outside +the `include/vllm` root. That off-by-one is the probe's, not the record's. + +## Findings from review round 4 (PR #898, head `7502004aa`) + +The review returned **FAIL** on a change it found correct and a gate it found +honest. All eight findings are record, test-strength or message defects, and +every one of them is this row's own thesis pointed back at the row. + +**F1 — the "refuses BY NAME" assertion could not fail.** The LTX case asserted +`msg.find(kLtx2VideoFamily)`, and `Fail()` in `ltx2_video.cpp` prefixes EVERY +message with `"ltx-2.5 video: "`, which contains the family string verbatim. So +the assertion was satisfied by boilerplate on every refusal the file can throw, +including the two the same case asserts it is NOT. Proven, not deduced: +substituting `""` for the family name in the DECLINES `Fail` BUILT and +left the suite GREEN. The H3 side escaped only by a spelling coincidence — its +prefix is `"minimax_h3 video: "` with an underscore against a hyphenated +`"minimax-h3"` family — which is not a property and is now not relied on. Both +lanes assert the QUOTED SLOT, `architecture ''`, through one +`QuotedArchitecture()` helper that carries the reason. + +**F2 — the H3 half violated `## Nothing lands dead`.** The chain existed +(`vllm_video_engine_load` → `LoadVideoEngine` → the `minimax_h3` registration → +`MiniMaxH3VideoEngine::Load` → `MiniMaxH3VideoDeviceType`), but nothing entered +through it: every H3 device assertion called the resolver directly. Replacing the +`Load`-time call with the pre-row defect `params.device == 0 ? kCPU : kCUDA` +BUILT and left `test_diffusion_device_seam` and `test_minimax_h3_video_fold` +GREEN. The fold suite's `CUDA load creates exactly one queue` case does enter +through `Load`, but its `FakeCudaPlatform` reports `kCUDA`, so the seam and the +cast return the same answer and it cannot separate them. Two H3 cases now enter +at `LoadVideoEngine` against the declining `PartialXpuPlatform`, mirroring the +two LTX cases; and the two LTX cases were moved from `Ltx2VideoEngine::Load` to +`LoadVideoEngine` as well, so both lanes are entered at the same production +point and neither skips the registry hop. + +**F3 — the anchor sweep that claimed completeness was incomplete, and its own +edits are what falsified it.** Six citations were stale, all of them rotted +INSIDE this pull request. Two of them sat seven and fifteen lines (measured at +`7502004aa`) from §0(b)'s `minimax_h3_video.cpp:221-226 @ 11cc1d589` citation, +which SHA-anchors itself with exactly the reasoning that applies to them. + +| citation | was | is | +|---|---|---| +| `check-device-leakage.py:78` (`RE_KCUDA`) | unanchored | `@ 62406c30e` (on the branch: `:224`) | +| `test_minimax_h3_video_fold.cpp:162` (the `kCUDA` assertion) | unanchored | `@ 62406c30e` (on the branch: `:220`, `== accelerator`) | +| `test_minimax_h3_video_fold.cpp:161-164`, twice | unanchored | `@ 62406c30e` (on the branch: the three untouched at `:192-194`, the two arms replacing `:162` at `:218-238`) | +| `ltx2_video.cpp:549-562` (the two questions) | unanchored | `@ 11cc1d589` | +| `ltx2_video.cpp:562-565` (the refusal-to-fake-it argument) | `:562-565` | `:610-613` | +| `model_loader.cpp:97` (the capability clause), twice | `:97` | `:98` | + +Four more were tightened rather than repaired, because a range citation that +starts on the wrong line is the same defect one size smaller: `model_loader.cpp`'s +`SelectQueueForModel` is `:60-105` and was cited `:59-104`; its auto arm is +`:76-104` and was cited `:75-104`; the `kAuto` fall-through to CPU is `:104` and +was cited `:103`; and `ltx2_video.cpp`'s device block runs to the end of the +capability refusal, where the citation stopped four lines short and cut the +`Fail` in half. That last one is now `:609-657`, having been `:566-614` for the +length of one merge — see the next paragraph. + +The completeness sentence is withdrawn, above, with the reason. What replaces it +is a statement of METHOD and of what the method cannot see: every `path:NN` on a +line this pull request ADDS was extracted mechanically from +`git diff -U0 $(git merge-base e8048ef63 HEAD)`, resolved at the final tree, and +read. It deliberately excludes citations this row did not write — the LTX lane's +upstream Python anchors and the other rows' C++ anchors in the same files — +because re-deriving those is a different row's work, and folding them in is how +the last sweep came to believe it had checked everything. A citation this row +inherits and did not touch is therefore NOT covered by this paragraph. + +**It rotted again during this round, which is the paragraph's own point made +twice.** `0785cfc4d` (`LTX25-RETIRE-DEAD-ARMS`) landed on main mid-gate and +edited `ltx2_video.cpp` above this branch's device block, moving all four of its +live line anchors by 43. The re-derivation caught it because it was re-run after +the merge rather than before it, and a run whose denominator moves afterwards +proves nothing. So the merge is taken FIRST and the anchors are derived at the +tree that is pushed. + +Three limits of the method, stated because the previous sweep's failure was +believing it had none. The extractor reads a `path:NN` token; it cannot tell a +LIVE citation from one being QUOTED — the `was` column of the table above writes +`:97`, `:562-565` and two unanchored fold-test numbers deliberately, as the +record of what was wrong, and a later sweep will resolve them to the wrong lines +and must not "repair" them. The count of citations is itself a measurement of +one tree, so it is not stored here: re-derive it rather than compare against a +number that rots for reasons unrelated to this row, which is the same rule the +scanned-file count above already carries. + +And — the limit that let round 5 find two more — **a bare `:NN` whose path sits +in a NEIGHBOURING token is never extracted at all.** The pattern requires the +path and the number to be glued, so `` `:224` `` written one line below +`check-device-leakage.py:78`, and a bare `` `:54` `` pointing at this file +itself, are invisible to it. They are not a rare shape — on the tree this +paragraph is pushed on they are a large minority of this spec's citations, and +the exact figure is deliberately NOT stored here for the reason the limit above +already gives. Re-derive it. That class is what the sweep which wrote this +paragraph could not see, and both of round 5's anchor findings belong to it. An +extractor for them must carry the antecedent path forward and must report when +there is none, because "no antecedent" is exactly the signature of a citation +into this file rather than into the tree. + +**F4 — `## Now` pointed at a closed pull request** (#671) and the pre-rebuild +branch. Corrected, with the relationship between the two stated rather than the +old number silently swapped out, because the round-1 to round-3 findings above +were made against #671's heads and a reader needs to be able to find them. + +**F5 — `dev_cast` over-matches a plain copy-initialisation.** +`vt::DeviceType d{other}` — a local copy, a member default-init, or an init from +a call returning `DeviceType` — fires alternative (3) while converting nothing. +Measured: 1 hit each, against 0 for `vt::DeviceType d{}`. It is NOT narrowed, +because `vt::DeviceType d{raw}` is the real conversion M32/M36 pin and is +textually identical; narrowing to remove the false positive deletes the true +positive. The docstring gains a third section, `WHAT dev_cast OVER-MATCHES`, +stating it with its cost — `dev_cast`'s baseline is a hard 0, so the first such +line under the scan roots fails the ratchet and needs `DSR-ALLOW` — and M47 pins +it in the M46 shape, with the value-init as the negative control in the same +test. Nothing in the tree writes the form today. + +**F6 — `include/vllm.h` still called the selector CUDA.** The public ABI field +`vllm_video_model_params.device`, the v14 `vllm_model_params.device` note that +cites it as precedent, `video_engine.h`'s mirror and `minimax_h3_video.h:88` +nineteen lines below the docstring this row rewrote all read `1 cuda`. +`include/vllm/config/device.h:19` repeats the same sentence and is corrected with +them; it is the fifth instance of one claim, not a fifth claim. + +**F7 — #828 was cited in product code and indexed nowhere.** +`check-device-leakage.py:58,139,158` and this spec name it as what closes the +declared blind spot, and it had no row in `.agents/issue-index.md`. +`check-agent-record.py` passed only because an absent row is nothing to count, +which is the instrument fault this row keeps finding. Appended with no owning row +and listed under `## Owed` below, which is the shape the protocol defines for an +issue a row files and does not fix. + +**F8** — an inverted sentence in the test header, which read as though the +refusal and the kernel death both happened. + +## Findings from review round 5 (PR #898, head `bf77c944e`) + +Every code, test and checker repair passed. All four findings are record +defects, and two of them are the same one: **an anchor into a file the change is +itself editing is stale by default, and this one rotted between the two halves +of a SINGLE commit.** + +**FF1 — `check-device-leakage.py`'s `RE_KCUDA` was recorded at `:188` and the +recording commit is what moved it to `:224`.** `f0b465029` carries both the F3 +anchor table, which writes `:188`, and the F5 `WHAT dev_cast OVER-MATCHES` +docstring section, which adds 36 lines above `RE_KCUDA`. So the F5 half +falsified the F3 half of the same commit, and no merge was involved — the usual +suspect for anchor drift, and here it was innocent. Corrected to `:224` in both +places and in the pull request body, re-derived at the tree that is pushed. + +**FF4 — the `:54` self-citation pointed at a blank line, and it is the paragraph +used as the exemplar of GOOD anchoring practice.** Three places cited "the `:54` +citation that SHA-anchors itself". At `7502004aa`, `:54` was §0(b)'s +`minimax_h3_video.cpp:221-226 @ 11cc1d589` citation, and the distances "seven +and fifteen lines" resolve against it exactly. `f0b465029` then added the very +anchors F3 asked for, §0 grew, and `:54` became blank. It is now named in PROSE +rather than by line number, because a number pointing into this file is the one +citation no re-derivation of the TREE can ever check, and this row has now +rotted it twice. + +**Method.** Every citation was extracted — bare continuations included, which is +the change — and every live repo-local one re-derived at the pushed tree, with +the needle for each derived from what this +spec CLAIMS the span contains rather than read back out of the cited file — a +validator that reads its expectation from its target re-derives `f(x) == f(x)` +and cannot fail. A deliberately wrong row was carried as a positive control in +the same run and reported STALE, so the harness is known to discriminate. 50 +rows FRESH, control STALE. Two further observations, reported and NOT repaired +because neither is wrong: `ltx2_video.cpp:609-657` opens one line inside the +preceding comment separator rather than on the block head at `:614` (the round-4 +repair was to its TAIL, which is correct at `:657`), and +`check-device-leakage.py:58,139,158` is a genuine three-way citation whose needle +cannot be unique by construction. + +## Owed + +- [#828](https://github.com/mudler/vllm.cpp/issues/828) — the AST-level + `dev_cast` check. `check-device-leakage.py:58,139,158` and §"Findings from + review round 3" both name it as what closes the declared blind spot, and + `scripts/check-device-leakage.py`'s docstring names it as what would enforce + the property rather than a spelling list. It is **not built here**: this row + ships the interim regular expression, its measured spelling list, its declared + blind spots and its declared over-match. Filed with all four rounds' evidence, + because "we kept finding more spellings" is the argument for it. Indexed in + [`issue-index.md`](../issue-index.md) with no owning row, which is why it is + listed here. + +## Now + +`READY`, implemented and awaiting a fresh review on +`row/LTX25-DEVICE-SEAM-SIBLING-REBUILD` ([PR +#898](https://github.com/mudler/vllm.cpp/pull/898)). PR #671, named by the +earlier rounds above, is CLOSED — it carried the same work on the pre-rebuild +branch, and the round-1 to round-3 findings recorded above were made against its +heads. All three changes plus the F5 repair and the round-4 repairs are on the +rebuild 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` 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. + +**This row now writes no record file at all, and that is the correct outcome +rather than an omission.** It previously annotated the `#659` and `#660` rows of +`roadmap_v1.md`'s `## Open issues` table in place. `#840` +(`POLICY-ISSUE-INTAKE`, spec [`issue-intake.md`](issue-intake.md)) moved that +table out to `.agents/issue-index.md`, which is append-only and carries +`merge=union`: a row is appended and never edited, and GitHub holds the open and +closed state, so closing `#659` and `#660` costs the index no edit. The in-place +annotation this row carried is exactly the `FIXED IN FLOW` shape that spec +retired, and under a union driver it would have been duplicated rather than +merged. The two index rows are therefore left byte-for-byte as `origin/main` +holds them, `LTX25-DEVICE-SEAM-SIBLING` has no portfolio row in `roadmap_v1.md` +(checked, with `ENG-WEIGHT-OFFLOAD` at two hits as a positive control in the same +command), and the evidence that annotation carried lives here, in the surface +that owns it. diff --git a/docs/USAGE.md b/docs/USAGE.md index e4a9bc55b..0772f1eb0 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -2032,11 +2032,26 @@ only when it reproduces the identical weight contract the shapes describe, and supplying one for a checkpoint that already declares its own is refused rather than ordered. +`vllm_video_model_params.device` is `0` for the CPU and `1` for **the +accelerator this build resolves** — not for CUDA. The value is unchanged and it +is CUDA on a CUDA build, but it is read through the platform seam rather than as +an enum value, so the same `1` selects Metal, Vulkan or Tenstorrent on a build +that registers one of those, and is refused by name on a build that registers +none. The C ABI's text-generation `vllm_model_params.device` is a separate, +later selector with its own `0 = auto / 1 = cpu / 2 = cuda` numbering. + 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 +On a build with no accelerator backend, `device = 1` is refused by name rather +than served the CPU forward behind an accelerator 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.h b/include/vllm.h index 92ec2ed03..f2449b2b4 100644 --- a/include/vllm.h +++ b/include/vllm.h @@ -439,8 +439,9 @@ typedef struct vllm_model_params { * silently replaced by another (mirror of vLLM assigning an explicit * device verbatim, device.py:61-66). * 0 must stay auto so a zero-initialized struct preserves pre-v14 behaviour; - * the cpu-before-cuda value order follows the v12 precedent - * (vllm_video_model_params.device: 0 cpu, 1 cuda) shifted by the auto slot. + * the cpu-before-accelerator value order follows the v12 precedent + * (vllm_video_model_params.device: 0 cpu, 1 the resolved accelerator) + * shifted by the auto slot. * Any other value fails vllm_engine_load with VLLM_ERR_INVALID_ARGUMENT. */ int32_t device; /* ── KV-pool sizing (ABI v16) ────────────────────────────────────────────── @@ -874,7 +875,13 @@ typedef struct vllm_video_model_params { * are byte-structurally identical, so it must be DECLARED; NULL/empty makes * every generate refuse with the guidance (the #77 guard). */ const char* partition; - int32_t device; /* 0 cpu, 1 cuda */ + /* 0 is the CPU; 1 is THE ACCELERATOR THIS BUILD RESOLVES, through the + * platform seam (CurrentPlatform + TryGetBackend + + * supports_model_architecture), never the enum value 1. It is therefore CUDA + * on a CUDA build and refused BY NAME on a build with no accelerator backend, + * or one whose partial backend declines this architecture (#659, #660). The + * ABI value is unchanged; what it means was never "cuda". */ + int32_t device; int32_t dequant_bf16; /* 0 keep-quant, 1 dequant/stream bf16 */ int32_t fp4_resident; /* NVFP4+cuda: keep FP4 packed, Marlin W4A16 GEMM */ /* ── v18 additions (the generalized seam) ───────────────────────────────── diff --git a/include/vllm/config/device.h b/include/vllm/config/device.h index e0f9495b0..6ab0a69b2 100644 --- a/include/vllm/config/device.h +++ b/include/vllm/config/device.h @@ -15,8 +15,9 @@ // - The integer values are the C-ABI wire contract // (vllm_model_params.device, ABI v14): 0 MUST be auto so a zero-initialized // struct preserves the pre-v14 accelerator-first behaviour byte for byte; -// cpu-before-cuda then follows the shipped v12 precedent -// (vllm_video_model_params.device: 0 cpu, 1 cuda) shifted by the auto slot. +// cpu-before-accelerator then follows the shipped v12 precedent +// (vllm_video_model_params.device: 0 cpu, 1 the accelerator that build +// resolves) shifted by the auto slot. #ifndef VLLM_CONFIG_DEVICE_H_ #define VLLM_CONFIG_DEVICE_H_ diff --git a/include/vllm/multimodal/minimax_h3_video.h b/include/vllm/multimodal/minimax_h3_video.h index 4156d510f..9bd086aa3 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 @@ -79,7 +85,9 @@ struct MiniMaxH3VideoModelParams { // declared-but-unknown and the #77 guard refuses every full render // (MiniMaxH3PartitionFromFlag / MiniMaxH3CheckTaskPartition). std::string partition; - int32_t device = 0; // 0 cpu, 1 cuda + // 0 cpu, 1 the accelerator this build resolves — see MiniMaxH3VideoDeviceType + // above; never `static_cast(device)` (#660). + int32_t device = 0; int32_t dequant_bf16 = 0; // 0 keep-quant, 1 dequant/stream bf16 // NVFP4 + cuda only: keep the packed FP4 resident and route the quantized // projections through the Marlin W4A16 GEMM (the pre-fold --fp4-resident). diff --git a/include/vllm/multimodal/video_engine.h b/include/vllm/multimodal/video_engine.h index 3119fa8ca..05edc4fa2 100644 --- a/include/vllm/multimodal/video_engine.h +++ b/include/vllm/multimodal/video_engine.h @@ -59,7 +59,10 @@ struct VideoModelParams { // value that is not registered is refused (it is never treated as a hint). std::string family; - int32_t device = 0; // 0 cpu, 1 cuda + // 0 is the CPU; 1 is the accelerator this build RESOLVES through the platform + // seam, not the enum value 1 (#659, #660). Refused by name when this build has + // no accelerator backend, or its partial backend declines the family. + int32_t device = 0; int32_t dequant_bf16 = 0; // 0 keep-quant, 1 dequant/stream bf16 int32_t fp4_resident = 0; // keep packed FP4 resident + quantized GEMM int64_t encoder_max_layers = 0; // 0 => all layers diff --git a/scripts/check-device-leakage.py b/scripts/check-device-leakage.py index c8f93352a..45ccb0afc 100644 --- a/scripts/check-device-leakage.py +++ b/scripts/check-device-leakage.py @@ -19,20 +19,166 @@ 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. + +WHAT `dev_cast` OVER-MATCHES. The two lists above answer "what is caught" and +"what is missed". Neither answers "what is caught that is not the defect", and a +gate whose message omits its own false positives is the same instrument fault in +the other direction: the reader takes a RED as proof of leakage. + + * a plain COPY-INITIALISATION whose target is a DeviceType and whose operand + ALREADY IS one — `vt::DeviceType d{other}`, a member default-init + `struct S { vt::DeviceType d{kCPU}; }`, or an init from a call + `vt::DeviceType d{platform.device_type()}`. Alternative (3) fires and nothing + is converted. Measured on this pattern: 1 hit each; `vt::DeviceType d{}` + scores 0, because the empty-braces lookahead already rejects a + value-initialisation. + + This is NOT narrowed, and the reason is that narrowing it would delete a real + catch. `vt::DeviceType d{raw}` — the declaration spelling of the conversion, + which review rounds 1 and 3 found and M32/M36 pin — is textually identical to + `vt::DeviceType d{other}`. A text checker cannot tell the two apart, which is + the same statement as "nothing here type-checks", and is exactly what #828 + resolves. The `const_cast(t)` entry above is the other side of + it and must keep firing for the same reason. + + The COST is not zero, so it is stated rather than dismissed: `dev_cast`'s + baseline is a hard 0 outside the one allowlisted `platform.cpp` hit, so the + FIRST such copy-init written under `src/vllm/` or `include/vllm/` fails the + ratchet as a `DSR REGRESSION`. The remedy is `// DSR-ALLOW(): ` + on the line, which is the same remedy the wire-decode case gets, and it is + visible in CI output rather than in the diff. Nothing in the scanned tree + writes this form today — a repo-wide probe over the scan roots finds + `dev_cast` at exactly its allowlisted hit — so this is a documented cost of + the next line, not a live failure. + + M47 pins it, with `vt::DeviceType d{}` as the negative control in the same + test: if a later change makes the copy-init form stop firing, M47 goes RED + and this entry must be corrected in the same change. That is the M46 shape, + applied to a false positive instead of a blind spot. + +tests/scripts/test_device_leakage.py M20-M47 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 +219,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 +379,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 +605,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 08d9392d4..56d0c634b 100644 --- a/src/vllm/multimodal/ltx2_video.cpp +++ b/src/vllm/multimodal/ltx2_video.cpp @@ -281,7 +281,7 @@ constexpr char kLtx2DurationHeadPathExtra[] = "duration_head_path"; // they are no longer trusted: the list below is derived from this file on every // run and compared, and the failure prints the replacement to paste in. // READER ANCHORS (derived and gated by test_ltx2_video): -// 660 715 811 827 829 899 924 1029 1070 +// 690 745 841 857 859 929 954 1059 1100 const char* const kKnownLoadExtras[] = { kLtx2AudioPromptEmbedsExtra, kLtx2PipelineKindExtra, kLtx2ModelVersionExtra, kLtx2AllowUnportedExtra, kLtx2MaxPhaseExtra, kLtx2DitConfigPathExtra, @@ -613,8 +613,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) + @@ -625,6 +625,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..65a69cda0 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,66 @@ 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:609-657` (the comment through the +// end of the capability refusal) and `SelectQueueForModel`'s auto arm +// (src/vllm/entrypoints/model_loader.cpp:76-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 77d8f01c3..54277bcaf 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -272,6 +272,13 @@ target_compile_definitions(test_ltx2_video PRIVATE 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..9c0fc030d 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,566 @@ 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_M47_the_declared_OVER_MATCH_still_fires(self) -> None: + # M46's shape, applied to a FALSE POSITIVE instead of a blind spot. + # + # Alternative (3) matches `vt::DeviceType d{x}` on the target type alone, + # so a plain COPY-INITIALISATION whose operand is already a DeviceType + # fires while converting nothing. The docstring's "WHAT `dev_cast` + # OVER-MATCHES" section states that, because a reader who takes a RED as + # proof of leakage is misled by a message that omits it. + # + # It is not narrowed, and the reason is that `vt::DeviceType d{raw}` — + # the declaration spelling of the real conversion, which M32 and M36 pin + # — is textually identical. Narrowing to remove the false positive + # deletes the true positive with it. So this test pins the CURRENT + # behaviour: if a later change makes any of these stop firing, it goes + # RED and the docstring entry must be corrected in the same change. + # + # All five compile (g++ 13.3 -std=c++20 -Wall -Wextra -fsyntax-only, + # exit 0), and each is asserted on its OWN file rather than appended to a + # shared one, so a spelling that stopped firing cannot be hidden by the + # next one still firing. + probe = "src/vllm/model_executor/models/probe.cpp" + for source in ( + "void L(vt::DeviceType other) { vt::DeviceType d{other}; (void)d; }\n", + "struct S { vt::DeviceType d{vt::DeviceType::kCPU}; };\n", + "void F(const Plat& p) { vt::DeviceType d{p.device_type()}; (void)d; }\n", + "struct T { vt::DeviceType const d{vt::DeviceType::kCPU}; };\n", + ): + with self.subTest(source=source.strip()): + self.tree.write(probe, source) + self.assertEqual(self.tree.scan().counts["dev_cast"], 1) + # The NEGATIVE control in the same test, and the boundary of the entry: a + # value-initialisation converts nothing AND does not fire, because the + # empty-braces lookahead already rejects it. Without this line the four + # assertions above would be consistent with "alternative (3) matches every + # `vt::DeviceType` declaration", which is a different and larger claim. + self.tree.write(probe, "void V() { vt::DeviceType d{}; (void)d; }\n") + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + + 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 +992,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..e4ed0b580 --- /dev/null +++ b/tests/vllm/multimodal/test_diffusion_device_seam.cpp @@ -0,0 +1,321 @@ +// #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 NO LONGER 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. +// +// WHERE EACH CASE ENTERS (AGENTS.md `## Nothing lands dead`). Both lanes are +// entered through a PRODUCTION entry point, not by calling the resolver: +// `vllm_video_engine_load` (include/vllm.h) → `LoadVideoEngine` +// (src/vllm/multimodal/video_engine.cpp) → the family registration → the +// engine's `Load`, which is where each lane's device question is asked. The +// direct `MiniMaxH3VideoDeviceType(1)` cases below stay, because they localise a +// failure — but they are NOT the reach proof, and on their own they measure a +// free function rather than a capability. Measured: replacing H3's `Load`-time +// `MiniMaxH3VideoDeviceType(params.device)` with the pre-row +// `params.device == 0 ? kCPU : kCUDA` left every direct case, and the whole of +// test_minimax_h3_video_fold, GREEN. The case that enters at `LoadVideoEngine` +// is the one that goes RED. +#include + +#include +#include +#include +#include +#include +#include + +#include "vllm/multimodal/ltx2_video.h" +#include "vllm/multimodal/minimax_h3_video.h" +#include "vllm/multimodal/video_engine.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; +} + +// The H3 sibling of DeviceOneParams(), entered through the SAME generic struct +// so it can go in at `LoadVideoEngine` with the family declared. The path does +// not exist for the same reason: a load that got past the capability clause +// would die opening it, with a different message. +vllm::multimodal::VideoModelParams H3DeviceOneParams() { + vllm::multimodal::VideoModelParams mp; + mp.family = vllm::multimodal::kMiniMaxH3VideoFamily; + mp.dit_path = "/nonexistent/h3-dit-that-is-never-opened.gguf"; + mp.device = 1; + return mp; +} + +// The PRODUCTION entry, for BOTH lanes: `vllm_video_engine_load` +// (include/vllm.h) calls `LoadVideoEngine` through src/capi/vllm_c.cpp, and it +// dispatches on the declared family to the registration in the engine's own TU, +// which calls that engine's `Load`. Nothing in this helper names an engine type, +// which is the point — it is the path a user arrives on, and entering at +// `Ltx2VideoEngine::Load` or `MiniMaxH3VideoEngine::Load` instead would skip the +// registry hop and prove one link less. +std::string LoadError(const vllm::multimodal::VideoModelParams& mp) { + try { + (void)vllm::multimodal::LoadVideoEngine(mp); + } catch (const std::exception& e) { + return e.what(); + } + return ""; +} + +// The architecture name in the refusal's QUOTED SLOT, rather than anywhere in +// the message. +// +// `Fail()` in src/vllm/multimodal/ltx2_video.cpp prefixes EVERY message it +// throws with "ltx-2.5 video: ", and `kLtx2VideoFamily` is the string +// "ltx-2.5". So `msg.find(kLtx2VideoFamily)` is satisfied by that boilerplate on +// every refusal this file can produce — including the two refusals the +// assertions below exist to distinguish this one FROM — and no defect in the +// message can make it fail. Measured: replacing the family name with +// "" inside the DECLINES `Fail` left it GREEN. +// +// The row's thesis for #659 is that a partial backend declines BY NAME, so the +// assertion has to be that the architecture appears where the refusal names it. +// The H3 pair happens not to collide — its prefix spells the family with an +// underscore ("minimax_h3 video: ") while the family is hyphenated +// ("minimax-h3") — but that is a coincidence of spelling and not a property, so +// the H3 cases are built the same way. +std::string QuotedArchitecture(std::string_view family) { + return std::string("architecture '") + std::string(family) + "'"; +} + +} // 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(QuotedArchitecture(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(QuotedArchitecture(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); +} + +// ── REACH (AGENTS.md `## Nothing lands dead`) ──────────────────────────────── +// +// The two cases above call `MiniMaxH3VideoDeviceType` directly, which measures +// the free function and NOT whether anything routes to it. The H3 engine's +// `Load` is the only production caller (src/vllm/multimodal/minimax_h3_video.cpp, +// the `MiniMaxH3VideoDeviceType(params.device)` line), and it is reached from +// `vllm_video_engine_load` → `LoadVideoEngine` → the `minimax_h3` registration. +// Delete that one call and every other H3 device assertion in this tree stays +// green; this case is what turns red. +// +// It is the H3 mirror of the two LTX cases at the top of this file, and it +// exists because the LTX half already had this proof and the H3 half did not. +TEST_CASE("minimax_h3 video: a DECLINING platform refuses device 1 through LoadVideoEngine") { + RegisterPartialAccelerator(/*accepts_everything=*/false); + + 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::kMiniMaxH3VideoFamily)); + + const std::string msg = LoadError(H3DeviceOneParams()); + INFO(msg); + REQUIRE_FALSE(msg.empty()); + CHECK(msg.find("xpu") != std::string::npos); + CHECK(msg.find(QuotedArchitecture(vllm::multimodal::kMiniMaxH3VideoFamily)) != + std::string::npos); + CHECK(msg.find("DECLINES") != std::string::npos); + CHECK(msg.find("supports_model_architecture") != std::string::npos); + // Not the OTHER refusal: a backend IS registered for the resolved platform, so + // blaming a missing one is a wrong diagnosis that reads as a right one. + CHECK(msg.find("no accelerator backend is registered") == std::string::npos); + // Nor the failure the guard exists to prevent, one step further on. Reverting + // the `Load` call site to the integer cast lands here instead: `kCUDA` with no + // CUDA backend registered in this process. + CHECK(msg.find("/nonexistent/") == std::string::npos); +} + +TEST_CASE("minimax_h3 video: a COMPLETE backend is not refused through LoadVideoEngine") { + // The mirror arm, for the same reason the LTX pair has one: two absences pass + // on any OTHER wrong failure, so the guard's "does not refuse a working + // configuration" half needs a POSITIVE assertion about where the load got to. + RegisterPartialAccelerator(/*accepts_everything=*/true); + + const std::string msg = LoadError(H3DeviceOneParams()); + INFO(msg); + REQUIRE_FALSE(msg.empty()); + CHECK(msg.find("DECLINES") == std::string::npos); + CHECK(msg.find("supports_model_architecture") == std::string::npos); + CHECK(msg.find("/nonexistent/h3-dit-that-is-never-opened.gguf") != std::string::npos); +}