fix(LTX25-DIT-ATTN-ARM-PARSE): match every DiT attention arm exactly and refuse a fourth value - #1798
Merged
Merged
Conversation
…fourth value The spec, committed before the implementation it describes. `VLLM_LTX2_DIT_FLASH_ATTN` read its own arms by two different rules. The naive arm tested `arm[0] == '0'`, a PREFIX, so `0x`, `07` and `0flash` each selected `vt::Attention`; the flash arm used `strcmp`, so only the exact string selected `vt::AttentionDenseFlash`; and every value matching neither fell into a bare `else` that ran the FA-2 default in silence. `flash` is the DENOMINATOR of the 2.74x in `ltx25-dit-attn-fa2-hd128.md` section 8, so a mistyped denominator did not fail. It ran the numerator's kernel a second time and yielded ~1.00x, which is also exactly what "no speedup" looks like. The spec argues the three decisions the fix is not free to make silently: unset still means FA-2 because it is the only serving arm; a defined-but-empty value is REFUSED rather than treated as unset, because `export VAR=$ARM` with an unset `ARM` produces exactly that; and the refusal echoes the offending value, because a trailing space is invisible in a shell history. It also records the two harness repairs the parse change forces, filed as #1794 and appended to the issue index here. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
…and refuse a fourth value Closes #1751. Closes #1794. `VLLM_LTX2_DIT_FLASH_ATTN` selects which `vt::` attention op the LTX-2.5 DiT self-attention calls, and its only product is a statement about which rung ran. It read its own arms by two different rules. The naive arm tested `arm[0] == '0'`, a PREFIX, so `0x`, `07` and `0flash` each selected `vt::Attention`; the flash arm used `strcmp`, so only the exact string selected `vt::AttentionDenseFlash`; and every value matching neither -- a typo (`falsh`), a case difference (`FLASH`), a trailing space, a plausible-looking `naive` or `1`, or an empty value from an unset shell variable -- fell into a bare `else` and ran the FA-2 default with no diagnostic. `flash` is the DENOMINATOR of the 2.74x in `ltx25-dit-attn-fa2-hd128.md` section 8, so a mistyped denominator did not fail. It ran the numerator's kernel a second time and yielded ~1.00x, which is also exactly what "no speedup" looks like, so the number could not report its own failure. The recorded measurements are not exposed: `assert_arm_op` reads the op-provider announcement and exits 47 on a mismatch. A person exporting the variable by hand got no such check. The dispatch is now an exact three-way parse and an unrecognised value throws `std::invalid_argument` naming the variable, the value that was set and the three arms. Unset still means FA-2 -- it is the shipped default and the only serving arm, so requiring the variable would refuse every production render. A defined-but-empty value is REFUSED rather than treated as unset, because `export VLLM_LTX2_DIT_FLASH_ATTN=$ARM` with an unset `ARM` produces exactly that and is the case in which an operator most believes they selected an arm. ## The three harness arms this forces, filed as #1794 At #1549 the knob was BINARY and flash was the default. #1551 made it three-way, moved the default up a rung and gave flash its exact spelling. It touched neither #1549-era harness, so three arms kept the old spellings and named a rung they no longer selected: `ltx25-dit-attn-flash-pixel-ab.sh` exports `=1` for `flash` and `flash-ctl`, and `ltx25-dit-attn-flash-ab.sh` leaves the variable UNSET for its `flash` arm. `ltx25-dit-attn-fa2-hd128-ab.sh` was written after the rename, is correct, and carries `assert_arm_op`. One of the three is loud and one is silent, and that asymmetry is why it is filed. The pixel harness's `arm_report` exits 46 on `ROUTING_BAD` -- after the render, an hour of a four-hour lease spent to learn a literal went stale. `ltx25-dit-attn-flash-ab.sh` has no such assertion at all: phase [F] PRINTS the op-provider selections and asserts nothing, so its `flash` arm renders FA-2 and publishes the ratio under the label `flash`. The recorded numbers are not invalidated. `ltx25-dit-attn-flash.md` section 10 records `knob=1` announcing `op=21` and `ROUTING_OK=flash`, so the binary that produced that run is one where `=1` still selected flash. ## Red first, both halves `test_ltx2_device`'s new case ran 9 unrecognised values through `Ltx2DitForwardDevice` -- the production device forward the denoise loop calls at `ltx2_video.cpp:4277`, not a hand-built op call -- and reported 49 assertions, 4 passed, 45 failed. The 4 that passed are the negative half: `0`, `flash` and unset must NOT throw, and a parse strict enough to refuse `falsh` is strict enough to refuse `flash`. Green after: 49 of 49, and 709 of 709 over the suite. `tests/scripts/test_ltx2_dit_attn_knob_arms.py` was red on all three stale arms before the harness repair, on both halves: `'1' not found in {'', '0', 'flash'}` for the two pixel arms, and `'' != 'flash'` for the third. It reads the accepted set out of the dispatch's own `std::strcmp(arm, "...")` calls rather than restating it, and asserts that every arm selects the rung its LABEL claims -- which is the half that matters, because `unset` is a valid value and was still the wrong arm for a whole row. Both extractors carry a count precondition, so a restructured file fails rather than reporting zero arms and passing. The empty-value case cannot use `vllm_test::SetEnv`, which maps an empty value to a DELETE on both platforms by documented design, so it uses a `::setenv` guarded on `!defined(_WIN32)` as that shim's own header instructs. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
…and give the knob suite one reader Three repairs from the fresh review of the #1751 and #1794 change. None of them changes what the dispatch does. `docs/ENVIRONMENT.md` said the first DiT forward throws on an unrecognised value, with no device qualifier, and that sentence is false on one arm. `--device cpu` leaves the device at 0, so the render routes to the host `Ltx2DitForward`, and `ltx2_device.cpp` is the only file in the tree that reads `VLLM_LTX2_DIT_FLASH_ATTN`. A CPU render therefore still ignores a typo in silence. The cell now says so, and says why that gap costs little: on the CPU `kAttention`, `kAttentionDenseFlash` and `kAttentionDenseFa2` are registered to one `AttentionKernel`, so all three rungs are one function and no host measurement can name a rung it did not run. What the host arm lacks is the refusal, not a correct answer. The section 10.3 design table in `.agents/specs/ltx25-dit-attn-flash.md` still spells the flash arms `=1`, a value that now refuses. The table is NOT rewritten. Section 10.4 records that arm announcing `op21_flash=1` and `ROUTING_OK=flash`, so `=1` was the flash arm on the binary that rendered those frames, and a corrected literal would describe a binary that never produced them. A note beneath the table says which knob it is, and points at the runnable path that #1794 already repaired. `tests/scripts/test_ltx2_dit_attn_knob_arms.py` read the dispatch twice under two rules. `accepted_values` regexed the raw file while the refusal tripwires stripped `//` comments first. A comment quoting `std::strcmp(arm, "X")` therefore widened the accepted set while the tripwires still read the real dispatch, so the subset half would admit a value the code rejects. Both readers now go through one `dispatch_code` helper, and its docstring says why they must agree. Measured on this tree with a comment quoting `std::strcmp(arm, "bogus")` added to `ltx2_device.cpp`: `accepted_values()` returned `['', '0', 'bogus', 'flash']` before the fix and `['', '0', 'flash']` after it. The probe was reverted byte-for-byte against a pre-taken sha256 each time. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
Brings the branch current so the preflight commit-trailers and commit-style gates EXECUTE instead of skipping. Both had reported `origin/main ... is not an ancestor of HEAD`, and a skipped gate reports nothing about this tree. No conflict: the three commits on main touch none of this row's files. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
…ping both new suites #1801 landed while this branch waited, and it registers `test_ltx25_ab_memwatch` in the same two places this branch registers `test_ltx2_dit_attn_knob_arms`: `.github/workflows/ci.yml` and `scripts/agent-preflight.sh`. Both conflicts are additive list entries competing for one line, so BOTH sides are kept. Each file now registers both suites; neither displaces the other. `.agents/issue-index.md` conflicted separately. The union driver's output was DISCARDED: git honours `merge=union` (`.gitattributes:7`) and merged without a conflict while RELOCATING this branch's `#1794` row to interior position 647 of 653. That is the third row this driver has moved today rather than appended, and counts stay correct every time; only "is my row the last line" detects it. Resolved by AGENTS.md's rule for a concurrent edit to a keyed record: take the complete target-branch version, then re-apply the scoped edit. Verified three ways — counts and duplicates, the last row is `#1794`, and sha256 of main's complete file equals sha256 of this file minus its last line. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1751. Closes #1794.
VLLM_LTX2_DIT_FLASH_ATTNselects whichvt::attention op the LTX-2.5 DiTself-attention calls. It is not a configuration; it is the same-binary control
that lets one build measure a three-rung ladder, and its only product is a
statement about which rung ran. It read its own arms by two different rules.
The naive arm tested
arm[0] == '0', a PREFIX, so0x,07and0flasheachselected
vt::Attention. The flash arm usedstd::strcmp(arm, "flash"), so onlythe exact string selected
vt::AttentionDenseFlash. Everything matching neitherfell into a bare
elseand ranvt::AttentionDenseFa2, the default, with nodiagnostic: a typo (
falsh), a case difference (FLASH), a trailing space, aplausible-looking
naiveor1, or an empty value from an unset shell variable.flashis the DENOMINATOR of the 2.74x recorded in.agents/specs/ltx25-dit-attn-fa2-hd128.mdsection 8. A mistyped denominator armdid not fail. It ran the NUMERATOR's kernel a second time and yielded ~1.00x,
which is also exactly what "no speedup" looks like, so the number could not report
its own failure. That is the shape
847e22f80already repaired once on this knob.The recorded measurements are not exposed by this:
assert_arm_opinscripts/ltx25-dit-attn-fa2-hd128-ab.shreads theVT_OP_PROVIDER_STATS=1announcement and exits 47 on a mismatch, so every arm in that row's section 8
stated its rung. A person exporting the variable in a shell or a service unit got
no such check, and that is who this row is for.
The parse, and the three decisions in it
vt::AttentionDenseFa2"flash"vt::AttentionDenseFlash"0"vt::Attentionthrow std::invalid_argumentUnset still means FA-2, and that is decided rather than inherited. It is the
shipped default and the only serving arm (
docs/ENVIRONMENT.md), and noproduction render sets the variable, so requiring it would refuse every render
this project ships. The knob has no "off": absence is the serving configuration
and the two named values are the measurement lanes.
A defined-but-empty value is REFUSED rather than treated as unset. This is
the one choice here that is not forced.
export VLLM_LTX2_DIT_FLASH_ATTN=$ARMwith an unset
ARMproduces exactly that, and it is the case in which an operatormost believes they selected an arm.
The refusal echoes the offending value. A trailing space and a case
difference are invisible in a shell history, so a message that lists only the
accepted arms leaves
flashandflashlooking identical to the reader.The read stays FRESH rather than cached, unchanged, so a test can flip the arm
inside one process.
The three harness arms this forces, filed as #1794
Found in flow. At #1549 the knob was BINARY:
=0selectedvt::Attentionandevery other value, unset included, selected
vt::AttentionDenseFlash. #1551 madeit THREE-WAY, moved the unset default up a rung to
vt::AttentionDenseFa2andgave the flash rung the exact spelling
flash. It touched neither #1549-eraharness.
maintodayltx25-dit-attn-flash-pixel-ab.shflash=1ltx25-dit-attn-flash-pixel-ab.shflash-ctl=1ltx25-dit-attn-flash-ab.shflashscripts/ltx25-dit-attn-fa2-hd128-ab.shis the contrast that makes the other twolegible: written after the rename, it exports
flash/""/0and carriesassert_arm_op.One of the three is loud and one is silent, and that asymmetry is why it is filed
rather than folded in quietly. The pixel harness's
arm_reportcountsop=18andop=21in the arm's own log and exits 46 onROUTING_BAD, so its two=1armsabort -- after the render, which is an hour of a four-hour lease spent to learn
that a literal went stale.
ltx25-dit-attn-flash-ab.shhas no such assertion atall: its phase
[F]PRINTS the op-provider selections and asserts nothing, so itsflasharm renders FA-2, is reduced under the labelflash, and the ratio itpublishes is FA-2 against naive while the file's own header says flash against
naive.
The recorded numbers are NOT invalidated.
.agents/specs/ltx25-dit-attn-flash.mdsection 10 records
knob=1announcingop=21andROUTING_OK=flash, so thebinary that produced that run is one where
=1still selected flash. What iswrong is the file as it stands against the binary
mainbuilds today.Both harnesses were TRUE when they were written and both became false in a commit
that touched neither file. Nothing in this tree connected the literal in a shell
script to the literal in the C++ dispatch, so the two drifted apart with no
signal. That is a gate's job, not a reviewer's.
Red first
tests/vllm/models/test_ltx2_device.cpp, "an unrecognisedVLLM_LTX2_DIT_FLASH_ATTNvalue is REFUSED by name", runs nine values throughLtx2DitForwardDevice-- the production device forward the denoise loop calls atsrc/vllm/multimodal/ltx2_video.cpp:4277, never a hand-built op call. Threeprefix values that used to select the naive rung, five that used to select the
default, and a defined-but-empty one.
RED against the unmodified dispatch: 49 assertions, 4 passed, 45 failed, with
0xreportingan empty refusal message, because the forward returned. The four that passed are
the negative half --
0,flashand unset must NOT throw. That half is notdecoration: a parse strict enough to refuse
falshis strict enough to refuseflash, and that would take the 47.84 s and 7.68 s rungs out of this binary,which is the same loss of the A/B from the opposite direction.
GREEN after: 49 of 49, and 709 of 709 across the suite's 23 cases.
tests/scripts/test_ltx2_dit_attn_knob_arms.pywas RED on all three stale armsbefore the harness repair, on both halves:
GREEN after: 9 of 9.
The gate, and why it is not a transcription
The suite reads the accepted set out of the dispatch's own
std::strcmp(arm, "...")calls rather than restating it, and reads eachharness's arm invocations. Neither side is copied into the test; what the test
holds is that the second is a subset of the first, and that every arm selects the
rung its LABEL claims.
The label half is the half that catches the real defect:
unsetis a perfectlyvalid value and was still the wrong arm for a whole row, so subset-alone would
have called
ltx25-dit-attn-flash-ab.shgreen. Both extractors carry a countprecondition, so a restructured harness fails rather than reporting zero arms and
passing vacuously. It reads text and runs nothing -- no GPU, no lease, no
toolchain -- and is registered on the preflight and CI record lanes, asserting
both registrations itself.
The empty-value case cannot use
vllm_test::SetEnv, which maps an empty value toa DELETE on both platforms by documented design (
tests/support/test_env.h,because
_putenv_s(name, "")deletes on Windows whilesetenv(name, "", 1)defines an empty string on POSIX). It uses a
::setenvguarded on!defined(_WIN32), which that shim's own header instructs, because Windows has nodefined-but-empty variable to test.
The fresh review, and what it changed
PASS, with every claimed guarantee MUTATED rather than read. Nine mutations, each
verified applied and each rebuild verified to succeed before its result was
believed: removing the refusal reds 45 assertions; restoring the
arm[0] == '0'prefix reds 15 (three values times five) plus the suite's prefix tripwire;
swapping the
flashand0op calls reds the PRE-EXISTING routing case above it;requiring the variable reds the unset half; dropping the value and dropping the
variable name from the message each red a DISTINCT assertion; deleting the
#if !defined(_WIN32)block drops the count 49 -> 43, so the block is live;restoring the three stale harness arms reds BOTH halves of the guard suite; and
breaking either extractor makes it ASSERT rather than pass vacuously. The tree
was restored byte-for-byte after each, and the rebuilt binary hashed identical to
the pre-mutation baseline.
Three findings were repaired by a second fresh implementer, in
8fa3e6102:The refusal is on the DEVICE arm only, and
docs/ENVIRONMENT.mddid not sayso. A
--device cpurender routes to the hostLtx2DitForward, which neverreads this variable, so an unrecognised value is still ignored there. The
consequence is low -- on CPU
kAttention,kAttentionDenseFlashandkAttentionDenseFa2all register&AttentionKernel(
src/vt/cpu/cpu_ops.cpp:3750-3765), so no host measurement can name a rung itdid not run -- but the sentence was false as written and is now qualified.
.agents/specs/ltx25-dit-attn-flash.mdsection 10.3 still spells the flash arms=1. The table is NOT rewritten: it records what that run actually ran, and thesame spec's section 10 shows
knob=1announcingop=21. A note beneath it sayswhy the value no longer works and that the runnable path is repaired.
The guard suite had two readers of the dispatch and they disagreed on what
counts as code.
accepted_values()read the raw file while the refusaltripwires stripped
//comments, so a future comment quotingstd::strcmp(arm, "X")would widen the accepted set while the tripwires stillread the real dispatch. Both now go through one
dispatch_code()helper. Provedred-first with a probe comment: before,
accepted_values() = ['', '0', 'bogus', 'flash']; after,['', '0', 'flash'].Scope
No arm moves, no threshold moves and no number changes. This row ran no GPU and
took no lease.
FOLLOWING_AGENTS_PROTOCOL
Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:claude-opus-5 [claude-code]