diff --git a/.agents/porting-inventory.md b/.agents/porting-inventory.md index 08984f416..d8e71aa27 100644 --- a/.agents/porting-inventory.md +++ b/.agents/porting-inventory.md @@ -1519,7 +1519,20 @@ Examples: `examples/cli` ✅ (C-API client), `examples/server` ✅ (OpenAI serve identical `(video_attn_mask < 1e-6)` and the identical multiply. Checked on both BEFORE mirroring it, because the reading a port arrives at by reasoning about intent (`>= 0`) is the opposite at padded positions. Gated as the surprising - behaviour, so "fixing" it REDs (`test_ltx2_pipeline`, mutation M3). + behaviour, so "fixing" it REDs: `test_ltx2_pipeline`, case "ltx2 the processor's + binary mask mirrors a comparison that looks backwards". + * **THE REGISTER BOUNDARY IS NOW NUMERICALLY REACHABLE (2026-08-13, review + finding F1 on the L9c PR).** `prompt_embeds_valid_rows` is the single knob + deciding which positions become the connector's TRAINED registers, and the + only case covering it asserted that valid=4 renders differently from valid=2. + Every monotone corruption of that boundary keeps that true, so a fresh + reviewer's `prompt_valid_rows + 1` left the suite fully green — one padded row + conditioned on caller junk instead of a register: finite, correctly shaped, + plausible, wrong. Closed by asserting WHICH positions are registers, from both + sides: perturbing rows `[valid, N)` must move NOTHING (they are substituted at + `embeddings_connector.py:148-150`) and perturbing row `valid - 1` must move + something. `test_ltx2_video`, case "the register boundary sits EXACTLY at the + valid-row count"; RED under `+ 1`, `- 1`, `0` and `v_rows`. * **A CONFIG VALUE THAT IS NOT NEAR ITS DEFAULT.** LTX-2.5 declares `connector_positional_embedding_max_pos = [4096]` where `Embeddings1DConnector`'s class default is `[1]`, and `get_fractional_positions` DIVIDES the token index by @@ -1527,11 +1540,23 @@ Examples: `examples/cli` ✅ (C-API client), `examples/server` ✅ (OpenAI serve RoPE angle wrong. `positional_embedding_theta` is deliberately NOT read from the DiT config even though one is declared, because neither configurator passes it — reading it would be a re-invention rather than a port. + * **ONE KEY WHERE THIS PARSE DIVERGES RATHER THAN MIRRORS (2026-08-13, review + finding F3).** `Ltx2ParseConnectorConfig` reads + `connector_num_learnable_registers`, which NEITHER configurator does + (`embeddings_connector.py:194-219` and `:222-256` both leave it at the class + default of 128), so "mirrors both configurators key for key" is not literally + true of this one key. The divergence is kept — a checkpoint declaring something + else must not be silently run at 128 — and is now ENFORCED rather than asserted: + `test_ltx2_video`, case "a connector config that disagrees with the FILE is + refused", subcase "a register count the file's TABLE does not carry is refused", + which REDs when the read is made inert. * **The record changed, not just the code.** The two connector families are no longer reported as unported: this port reads them, so naming them would say something untrue about the tree and would demand `allow_unported_modules` from a caller whose checkpoint is read completely. Asserted as an ABSENCE in - `test_ltx2_loader`, so restoring the old behaviour REDs (mutation M4). + `test_ltx2_loader`, case "ltx2 loader: the unported families are refused by name, + not absorbed" (`CHECK(conn_ck.unported.empty())`), so restoring the old behaviour + REDs. * **OWED, and precisely:** the Gemma-4 TOWER, so what ENTERS the connector is still whatever the caller put in the prompt-embeds file — the link below the tower is real, the tower is not. `prompt_embeds_valid_rows` exists because a file carries diff --git a/src/vllm/multimodal/ltx2_video.cpp b/src/vllm/multimodal/ltx2_video.cpp index ac345a780..07bdde3f8 100644 --- a/src/vllm/multimodal/ltx2_video.cpp +++ b/src/vllm/multimodal/ltx2_video.cpp @@ -770,17 +770,31 @@ std::unique_ptr Ltx2VideoEngine::Load(const VideoModelParams& p // honour (embeddings_processor.py:89). `Ltx2ModalityInput` carries no // context mask, so a mask with a masked position would be silently dropped // — the DiT would attend over register-free padding as if it were caption. - // With registers enabled every position is attendable and the mask is all - // ones, which is the case the shipped checkpoint is in; anything else is - // refused by name rather than ignored. + // + // THIS LOOP IS UNREACHABLE ON EVERY INPUT EITHER REFERENCE PRODUCES, and + // saying which case reaches it is how the claim stays checkable. It is NOT + // the `num_learnable_registers = 0` case, which an earlier version of this + // comment named and which cannot get here: with registers disabled + // `Ltx2ConnectorForward` passes the caller's ADDITIVE mask straight + // through, and `_to_binary_mask`'s `encoded_mask < 0.000001` + // (embeddings_processor.py:46-48) is satisfied by BOTH values an additive + // mask holds — 0.0 and -finfo(f32).max — so the binary mask is one + // everywhere there too. With registers enabled :152 returns + // `torch.zeros_like(mask)` and the answer is one everywhere for the same + // reason. What would reach it is a connector whose output mask carries a + // value at or above +1e-6, which no path in `ltx_core` or `diffusers` + // emits today. It is kept as a guard on that future, refused by name + // rather than ignored, and it is deliberately not gated: a test would have + // to fabricate a mask neither reference can produce. for (const float m : encoded.mask) { if (m == 1.0f) continue; Fail( "the embeddings connector returned a cross-attention mask with masked " "positions, and `Ltx2ModalityInput` carries no context mask to pass it through. " - "That happens when the connector runs with num_learnable_registers = 0, where " - "padding stays padding instead of becoming a register. Refusing rather than " - "dropping the mask, which would condition the DiT on unmasked padding."); + "No path in either reference emits such a mask — `_to_binary_mask` is one " + "everywhere for both values an additive mask holds — so this is a connector " + "whose output mask this port does not model. Refusing rather than dropping " + "the mask, which would condition the DiT on unmasked padding."); } im.video_prompt_embeds = encoded.video; im.audio_prompt_embeds = encoded.audio; diff --git a/tests/vllm/multimodal/test_ltx2_video.cpp b/tests/vllm/multimodal/test_ltx2_video.cpp index bc4918a39..7f0187767 100644 --- a/tests/vllm/multimodal/test_ltx2_video.cpp +++ b/tests/vllm/multimodal/test_ltx2_video.cpp @@ -100,6 +100,24 @@ std::string ReadAll(const std::string& path) { return std::string((std::istreambuf_iterator(in)), std::istreambuf_iterator()); } +// A prompt-embeds file as floats, and back. The register cases need to change +// ONE row of a supplied stream and leave every other byte alone, which the +// fixture's seeded writer cannot express. +std::vector ReadFloats(const std::string& path) { + const std::string bytes = ReadAll(path); + REQUIRE(bytes.size() % sizeof(float) == 0); + std::vector out(bytes.size() / sizeof(float)); + std::memcpy(out.data(), bytes.data(), bytes.size()); + return out; +} + +void WriteFloats(const std::string& path, const std::vector& values) { + std::ofstream out(path, std::ios::binary); + REQUIRE_MESSAGE(out.good(), "cannot write ", path); + out.write(reinterpret_cast(values.data()), + static_cast(values.size() * sizeof(float))); +} + bool Registered(const std::string& family) { const std::vector all = vllm::multimodal::RegisteredVideoFamilies(); return std::find(all.begin(), all.end(), family) != all.end(); @@ -841,6 +859,20 @@ TEST_CASE("ltx2 video: an ABI client loads, detects and generates through vllm.h // otherwise rather than silently passing: a gate that quietly does nothing when // its input is absent is how "we tested the real checkpoint" becomes untrue. // +// HOW FAR THAT ANNOUNCEMENT CARRIES, stated so nobody over-reads it. The +// `MESSAGE` below is printed by the doctest binary's own run, so `test_ltx2_video` +// executed directly says SKIPPED on its own output. `ctest` captures a passing +// test's output and prints only the pass line, so under `ctest` this case is +// indistinguishable from one that ran. Read "the shipped checkpoints were read" +// off the binary's output or off an explicitly set env, never off a green ctest +// row. +// +// The env is deliberately NOT the tree-wide `CHECKPOINT_ROOT`. That one "declares +// an INTENT, not a behaviour" and nothing in the tree reads it (`.env.example`), +// while this needs a path to one specific publisher tree whose internal layout +// (`diffusion_models/…`) the case walks. Pointing it at the shared root would +// make this its first reader and change what that variable means. +// // It deliberately stops short of the DiT FORWARD. At the shipped 21.00B geometry // the f32 parity forward needs ~76 GB of weights and ~2.6e14 FLOPs per step, and // the device path cannot feed it (see the `device = 1` refusal). What is checked @@ -1131,6 +1163,28 @@ TEST_CASE("ltx2 video: a connector config that disagrees with the FILE is refuse CHECK(msg.find("transformer_1d_blocks.2") != std::string::npos); } + SUBCASE("a register count the file's TABLE does not carry is refused") { + // `connector_num_learnable_registers` is the ONE key `Ltx2ParseConnectorConfig` + // reads that NEITHER upstream configurator does: `Embeddings1DConnectorConfigurator` + // (embeddings_connector.py:194-219) and its audio sibling (:222-256) both leave + // it at the class default of 128, so "mirrors both configurators key for key" is + // not literally true of this one key and the divergence is deliberate — a + // checkpoint declaring something else must not be silently run at 128. + // + // A read that nothing can falsify is a claim, not a gate, and this is what + // falsifies it: the fixture's stored table stays `[2, dim]` while the config + // declares 4, and `Ltx2LoadConnectorWeights`' shape check is what has to fire. + ltx2_fixture::ReducedDitOptions relabelled; + relabelled.transformer_overrides["connector_num_learnable_registers"] = 4; + const std::string path = ws.root + "/dit_conn_registers.safetensors"; + ltx2_fixture::WriteReducedDit(dit, path, relabelled); + vllm::multimodal::VideoModelParams mp = FixtureParams(ws.paths); + mp.dit_path = path; + const std::string msg = RefusalOf(mp); + INFO(msg); + CHECK(msg.find("learnable_registers") != std::string::npos); + } + SUBCASE("gating the config declares but the file does not carry is refused") { ltx2_fixture::ReducedDitOptions ungated; ungated.connector.gated = false; // writes NO to_gate_logits tensors... @@ -1197,3 +1251,96 @@ TEST_CASE("ltx2 video: the valid-row count decides which positions are registers CHECK(msg.find(vllm::multimodal::kLtx2PromptValidRowsExtra) != std::string::npos); } } + +TEST_CASE("ltx2 video: the register boundary sits EXACTLY at the valid-row count") { + // WHY THE CASE ABOVE IS NOT ENOUGH, and this one exists. It asserts only that + // a render at valid=4 differs from one at valid=2. ANY monotone corruption of + // the boundary keeps that true — `prompt_valid_rows + 1` still renders + // differently from valid=2 — so the whole family passes it. The defect it + // cannot see is ONE padded row conditioned on caller junk instead of the + // connector's TRAINED register: finite, correctly shaped, plausible, wrong. + // + // WHAT MAKES THE BOUNDARY NUMERICALLY REACHABLE is the substitution itself. + // `binary_mask = additive_attention_mask[:, 0, 0, :] >= 0` then + // `binary_mask * hidden_states + (1 - binary_mask) * registers` + // (embeddings_connector.py:148-150) REPLACES a masked position's features + // outright, so what the caller supplied there cannot reach the render at all. + // Positions therefore separate by whether perturbing them moves a byte: + // + // rows [valid, N) are REGISTERS -> perturbing them must change NOTHING + // rows [0, valid) are CAPTION -> perturbing the last one must change something + // + // Together those pin the boundary from both sides, which is what "which + // positions are registers" means when the conditioning itself is not + // observable from outside the engine. `+ 1` breaks the first (row `valid` + // keeps caller junk); `- 1` breaks the second (row `valid - 1` becomes a + // register and its content stops mattering). The reviewer's mutation was the + // former and the suite stayed fully green. + // + // THE POLARITY AND THE CONTIGUITY ARE BOTH UPSTREAM'S. + // `_prepare_attention_mask` builds `(attention_mask - 1) * finfo.max` + // (transformer_args.py:203-206), so 0.0 is kept and -finfo.max is padded; and + // `_compute_right_pad_order`'s stable descending argsort + // (embeddings_processor.py:33-38) makes the padded set the CONTIGUOUS SUFFIX + // `[valid, N)` that `prompt_embeds_valid_rows` names. Neither the mask + // comparison nor the returned mask can carry this: with registers on, :152 + // returns `torch.zeros_like(mask)` and `_to_binary_mask`'s `< 0.000001` + // (:46-48) is satisfied by BOTH values an additive mask holds, so the mask the + // DiT receives is all ones either way. The substitution is the only carrier. + Workspace ws; + const vllm::Ltx2DitParams dit = ltx2_fixture::ReducedDitParams(); + const int64_t width = dit.cross_attention_dim; + const int64_t rows = 4; // `ltx2_fixture::WriteFixture`'s `prompt_tokens` + const int64_t valid = 2; // two caption rows, two padded ones + + const std::vector base = ReadFloats(ws.paths.video_embeds); + REQUIRE(base.size() == static_cast(rows * width)); + + // A perturbation big enough that no 8-bit frame byte can absorb it, and + // column-varying so it cannot turn one row into a copy of another. + auto perturb_rows = [&](const std::string& name, int64_t first, int64_t last) { + std::vector values = base; + for (int64_t r = first; r < last; ++r) { + for (int64_t c = 0; c < width; ++c) { + values[static_cast(r * width + c)] += 3.0f + 0.25f * static_cast(c); + } + } + const std::string path = ws.root + "/" + name + ".f32"; + WriteFloats(path, values); + return path; + }; + + vllm::multimodal::VideoModelParams half = FixtureParams(ws.paths); + half.extras[vllm::multimodal::kLtx2PromptValidRowsExtra] = std::to_string(valid); + const std::string reference = RenderBytes(half, ws.root + "/reg_ref"); + // Without this an `==` below could pass on two empty reads rather than on two + // renders. + REQUIRE(reference.size() > 1000); + // ...and the same request twice is byte-identical, which is what makes the + // equality an assertion about the registers rather than about noise. + REQUIRE(RenderBytes(half, ws.root + "/reg_ref2") == reference); + + SUBCASE("every row at or past the count is a register, so its content is inert") { + vllm::multimodal::VideoModelParams padded = half; + padded.prompt_embeds_path = perturb_rows("perturbed_padding", valid, rows); + // REDs at `prompt_valid_rows + 1`: row `valid` would keep the caller's junk + // instead of becoming the trained register. + CHECK(RenderBytes(padded, ws.root + "/reg_padded") == reference); + } + + SUBCASE("the row just BEFORE the count is caption, so its content is not") { + vllm::multimodal::VideoModelParams caption = half; + caption.prompt_embeds_path = perturb_rows("perturbed_last_caption", valid - 1, valid); + // REDs at `prompt_valid_rows - 1`: row `valid - 1` would be replaced by a + // register and the perturbation would stop reaching the DiT. + CHECK(RenderBytes(caption, ws.root + "/reg_caption") != reference); + } + + SUBCASE("the FIRST row is caption too, so the boundary is not the whole prompt") { + // Guards the degenerate reading where nothing is caption: a mask of all + // -finfo.max renders the register table and only the register table. + vllm::multimodal::VideoModelParams first = half; + first.prompt_embeds_path = perturb_rows("perturbed_first", 0, 1); + CHECK(RenderBytes(first, ws.root + "/reg_first") != reference); + } +}