From 846301cfb6020283405a6d74032cf23e2e8bc777 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Thu, 13 Aug 2026 07:41:18 +0000 Subject: [PATCH] test(ltx-2.5): the register boundary, made numerically reachable FOLLOWING_AGENTS_PROTOCOL A fresh review of the L9c connector PR passed the change and found a hole in its gate. `prompt_embeds_valid_rows` is the single knob deciding which token positions become the connector's TRAINED `learnable_registers` and which carry caller-supplied conditioning -- on the accepted render, 104 of 128 rows are registers. The reviewer changed `s = im.prompt_valid_rows` to `s = im.prompt_valid_rows + 1` at `ltx2_video.cpp:744` and `test_ltx2_video` stayed fully green: 23/23 cases, 253/253 assertions, exit 0. Reproduced here before anything was written. WHY THE COVERAGE COULD NOT SEE IT. The only case on that knob asserted that a render at valid=4 differs from one at valid=2. ANY monotone corruption of the boundary keeps that true, so the whole family passes it. The defect it cannot see is ONE padded row conditioned on caller junk instead of a trained register: finite, correctly shaped, plausible, and wrong. WHAT MAKES THE BOUNDARY REACHABLE IS THE SUBSTITUTION, NOT THE MASK. `binary_mask * hidden_states + (1 - binary_mask) * registers` (embeddings_connector.py:148-150) REPLACES a masked position's features, so what the caller supplied there cannot reach the render at all. The mask carries nothing: with registers on, :152 returns `torch.zeros_like(mask)`, and `_to_binary_mask`'s `encoded_mask < 0.000001` (embeddings_processor.py:46-48) is satisfied by BOTH values an additive mask holds, so what the DiT receives is all ones either way. Positions therefore separate by whether perturbing them moves a byte, and the boundary is pinned from BOTH sides: rows [valid, N) are REGISTERS -> perturbing them must change NOTHING rows [0, valid) are CAPTION -> perturbing the last one must change something The polarity is `(attention_mask - 1) * finfo.max` (transformer_args.py:203-206) and the contiguity is `_compute_right_pad_order`'s stable descending argsort (embeddings_processor.py:33-38), which is what makes the padded set the suffix `[valid, N)` that the extra names. RED FIRST, on the product code, restored byte-for-byte after each run (`git diff` empty, HEAD unchanged, verified per mutation): + 1 "every row at or past the count is a register" FAILS 110/111 - 1 "the row just BEFORE the count is caption" FAILS 110/111 0 both caption arms FAIL 109/111 v_rows "every row at or past the count is a register" FAILS 110/111 and `+ 1` over the WHOLE suite: 24 cases, 23 passed, 1 failed; 364 assertions, 363 passed, 1 failed. Only the new case moves, which is the finding reproduced and closed in one run. TWO SIBLING FINDINGS FOLDED IN. F3 is a correction of a claim, and then a gate under it. The L9c commit message said `Ltx2ParseConnectorConfig` "mirrors both configurators key for key". It does not: `connector_num_learnable_registers` is read here and by NEITHER configurator (embeddings_connector.py:194-219 and :222-256 both take the class default of 128). The divergence is deliberate and stays -- a checkpoint declaring something else must not be silently run at 128 -- but a read nothing can falsify is a claim rather than a gate, so it now has one: the fixture's stored table stays `[2, dim]` while the config declares 4, and `Ltx2LoadConnectorWeights`' shape check fires. REDs when the read is made inert. The correction itself lands in `porting-inventory.md` and in the test, NOT as a comment beside the parse. `check-doc-checkpoint` classifies any path under `src/vllm/model_executor/models/` as `feature_surface` and would demand a `docs/FEATURES.md` entry for a comment that changes no feature -- and inventing one to satisfy a checker is a false record. The parse's existing comment already says "NOT read by either configurator", so nothing there was untrue; what was missing was the gate, and the gate is what this adds. F4 is an unreachable branch that named the wrong reason. The refusal on a masked cross-attention mask said it fires "when the connector runs with num_learnable_registers = 0". It cannot: with registers disabled the caller's additive mask passes straight through and `< 0.000001` is still true at every position, so the branch is unreachable on every input either reference produces. The comment and the message now say that, name what WOULD reach it, and say why it is not gated. Also in the record: `porting-inventory.md` cited "mutation M3/M4" by labels that existed only in a PR body, which git cannot resolve -- replaced with the test case names, which are in the tree. NOT REPRODUCED, and recorded rather than dropped: the shipped-checkpoint case does not skip silently. `MESSAGE: SKIPPED: set LTX2_CHECKPOINT_ROOT ...` prints in the binary's own output, in the baseline run above. It IS invisible under `ctest`, which captures a passing test's output, and its env is deliberately not the tree-wide `CHECKPOINT_ROOT` -- `.env.example` says that one "declares an INTENT, not a behaviour" and nothing reads it. Both are now stated at the case so a green ctest row cannot be read as "the shipped checkpoints were read". Gate: clean-configured CPU-only Release build, BUILD_EXIT=0, no `No space left`/`BFD assertion`/kill in the log, `ctest -N` 399 tests. Issue: #435 Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code] --- .agents/porting-inventory.md | 29 ++++- src/vllm/multimodal/ltx2_video.cpp | 26 +++- tests/vllm/multimodal/test_ltx2_video.cpp | 147 ++++++++++++++++++++++ 3 files changed, 194 insertions(+), 8 deletions(-) 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); + } +}