Repository navigation
fix(youtu_vl): build the window inverse as argsort - #1603
Merged
Merged
Conversation
argsort, not the permutation itselfargsort, not the permutation itself
…on itself (lablup#1600) `reverse_window_indices` sorted `(value, index)` pairs and wrote the rank back by index. For a permutation of `0..N` the element with value `v` always sorts to rank `v`, so that reproduced `window_index` rather than its inverse, and the encoder un-reordered the merger output by applying the window permutation a second time. That is only correct when the permutation is an involution, which needs the window count per edge to equal the window width. With Youtu-VL's 8x8 merged-token window that is a single 256x256 image or a 2048x2048 one; at 512x512, 192 of the 256 merged tokens reached the language model out of place, and at 1024x1024, 960 of 1024. The helper now builds `inverse[window_index[rank]] = rank` directly, bounds-checked so a malformed grid cannot panic, the same construction lablup#1601 landed for Qwen2.5-VL. Four checkpoint-free tests at the upstream defaults pin the minimal `[2, 0, 1]` case, the 512x512 and 384x384 grids with their misplaced counts, the single-window identity case, and a padded two-image batch; all four fail on the old construction. Not done here: the real-checkpoint before/after the issue asks for. No Youtu-VL checkpoint or Metal toolchain is available on this host; the PR body says which image sizes to use. Verified on a CPU-only build: `cargo fmt --all -- --check`, `cargo clippy --profile test-fast -p mlxcel --all-targets -- -D warnings`, and `cargo test --profile test-fast -p mlxcel --lib vision::encoders::youtu_vl` (10 passed). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0134qsVMvsvs9LpZgCVH5m3Y
rapsealk
force-pushed
the
fix/1600-youtu-vl-window-inverse
branch
from
September 3, 2026 05:05
f0f7ef5 to
569ae38
Compare
…ndow index The bounds guard in `reverse_window_indices` exists so a malformed grid cannot index out of range; the value it leaves behind is not a contract. Asserting `[0, 7, 1]` maps to `[0, 2, 0]` promised that corruption, so the assertion is gone. The guard itself matches the Qwen2.5-VL helper from lablup#1601. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0134qsVMvsvs9LpZgCVH5m3Y
6 tasks done
The entry described the corrected window inverse without saying that the family still does not describe images correctly. A real-checkpoint run of tencent/Youtu-VL-4B-Instruct is byte-identical before and after this fix at both 448x448 and 336x336, and the 224x224 single-window fixture, whose permutation is the identity and which this change provably cannot affect, is already described wrong. The dominating defect is the processor's patch emission order, filed as lablup#1610, so the entry now scopes its claim to the token order at that stage.
argsort, not the permutation itself
This was referenced Sep 4, 2026
inureyes
added a commit
that referenced
this pull request
Sep 4, 2026
## Summary `YoutuVLProcessor::try_preprocess_with_spatial` emitted one row per patch in plain raster order over the patch grid with the inner feature order `(c, dy, dx)`. Two independent consumers expect merge-block-major rows with channel-last `(dy, dx, c)` features, so the vision tower was fed patches that do not correspond to the positions and groupings it assumes. The checkpoint's own processor, `convert_image_to_patches` in https://huggingface.co/tencent/Youtu-VL-4B-Instruct/blob/main/image_processing_siglip2_fast.py, reshapes to `(C, nh/m, m, ps, nw/m, m, ps)` and permutes `(1, 4, 2, 5, 3, 6, 0)`. `Siglip2VisionEmbeddings.patch_embedding` is an `nn.Linear` over those rows and `remap_youtu_vl_weights` only renames that weight, so raster rows fed the trained projection a permuted vector. Independently, `YoutuVLVisionEncoder::rot_pos_emb` builds position ids with `reshape(&[h / merge, merge, w / merge, merge])` then `transpose_axes(&[0, 2, 1, 3])`, and `forward_with_spatial` gathers on `[n_groups, spatial_merge_unit, dim]`, so both the rotary position and the 2x2 merge grouping were attached to the wrong patch. The mismatch was provable without an external oracle: the encoder's two inputs disagreed with each other. ## What changed - `src/vision/processors/youtu_vl.rs`: the emission loop now runs `(block_y, block_x, inner_y, inner_x)` with `(dy, dx, c)` inner features. The stale comment claiming upstream uses `unfold` is replaced with one naming this checkpoint's actual construction and linking it. Qwen2-VL's loop shape is mirrored, but not its `(c, dy, dx)` inner order, which is correct for that family because it does unfold; `src/vision/processors/qwen2_vl.rs` is untouched. - `src/vision/processors/youtu_vl.rs`: a new `YoutuVLPreprocessError::UnalignedPatchGrid` makes the merge-divisibility precondition explicit. `smart_resize` rounds both edges to a multiple of `patch_size * spatial_merge_size`, so the grid is always divisible today, but the block loops would silently drop patches rather than fail if that resize policy ever changed. - `src/vision/processors/youtu_vl_tests.rs`: `patches_are_emitted_merge_block_major_with_channel_last_features` pins the row order and the inner feature order on a 4x4 patch grid, which is 2x2 merge blocks. The grid size is the point of the test: at `spatial_merge_size=2` a 2x2 patch grid is a single block, where block-major and raster are the same sequence, so a smaller fixture could not fail on this defect. The three RGB channels carry three non-overlapping codes so one row also separates `(dy, dx, c)` from `(c, dy, dx)`. - `tests/youtu_vl_parity.rs` (new): real-model content tests in the shape of `tests/qwen2_5_vl_parity.rs`, since a test that only counts tokens passes on a fluent invention. ## Real-checkpoint validation `models/mlx/youtu-vl-4b-instruct` (`tencent/Youtu-VL-4B-Instruct`), M1 Ultra, release build with `metal,accelerate`, greedy (`-t 0 -n 48`). Both columns were measured on this machine, before at `64f5d9b41` and after at this branch. **`tests/fixtures/test_image.png`, a 224x224 solid orange square** (prompt: "Describe this image briefly.") - Before: "The image is completely black and contains no visible content, objects, text, or details. It is a solid black square or rectangle with no variation in color or texture." - After: "The image is a solid, uniform block of bright orange color. There are no visible objects, textures, patterns, or variations — it is a pure, flat field of orange with no discernible features or details." This is the fixture that pins the defect. At 224x224 the merged grid is 7x7, a single 8x8 window, so `get_window_index` is the identity and the window inverse from #1600 / #1603 provably cannot affect the result either way. **`tests/fixtures/test_image_shapes.png`, 448x448, a red square, a blue circle and a green triangle on light grey** (prompt: "What shapes and colors are in this image? Answer briefly.") - Before: "The image contains a single, solid black circle on a white background." - After: "The image contains a single, solid black circle on a plain white background." **`tests/fixtures/test_image_shapes_336.png`, the same scene at 336x336** (same prompt) - Before: "The image contains a single, solid black circle on a white background." - After: "The image contains a single white circle on a black background." ## The part that is still wrong, on purpose Both three-shape fixtures changed and are both still wrong. That is the measured, expected outcome of this fix, not a regression and not an incomplete fix of this defect: issue #1610 predicted exactly these two strings before the work started. At least one further defect remains in this family's vision path, it is unidentified, and it is out of scope here. The evidence pattern is worth naming: the only fixture that reads correctly is the only one whose merged grid fits inside a single attention window (7x7). Both fixtures that need more than one window (11x11 after `smart_resize` lifts 336 to 352, and 14x14) are wrong, and they are wrong differently from each other, which is not what a size-independent defect in the embedding or the merger would produce. That is filed as #1618 with the measurements and the candidate areas, and it is deliberately not chased here. Accordingly the two shape tests in `tests/youtu_vl_parity.rs` are written at the full correct answer and gated `#[ignore = "known failing, blocked on #1618 ..."]` rather than weakened to something this build can satisfy. Fixing #1618 is what turns them green. ## Test plan - [x] `cargo test --profile test-fast --features metal,accelerate --lib vision::processors::youtu_vl` (8 passed, including the new order test) - [x] `cargo test --profile test-fast --features metal,accelerate --test youtu_vl_parity -- --ignored youtu_vl_describes_the_solid_color_fixture` (1 passed against the local checkpoint) - [x] `cargo clippy --lib --tests --features metal,accelerate -- -D warnings` - [x] `cargo fmt --all -- --check` - [x] `python3 scripts/ci/check_cross_repo_refs.py` - [x] Real-checkpoint runs on all three fixtures, before and after, recorded above - [ ] The full `--workspace` test and clippy gate, left to CI Closes #1610
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.
Summary
reverse_window_indicesin the Youtu-VL vision encoder returnedwindow_indexitself instead of its inverse, so the merger output was un-reordered by applying the window permutation a second time. That is only correct when the permutation is an involution. The helper now buildsargsort(window_index)directly, the same construction #1601 landed for Qwen2.5-VL, with checkpoint-free tests that pin the affected grids.Related issues
Closes #1600. Same defect as #1596 / #1601 (Qwen2.5-VL), whose corrected helper this copies.
Type of change
feat— new user-visible featurefix— bug fixperf— performance improvement (include before/after numbers in the PR body)refactor— internal restructuring without behavior changechore— build, CI, dependencies, release infrastructuredocs— documentation onlytest— tests onlyWhich grids are affected
A Youtu-VL window is 8x8 merged tokens (
window_size256 overpatch_size16 timesspatial_merge_size2), andbuild_processorcaps the processor atvision_config.num_patches(4096), so the patch grid tops out at 64x64 and the merged grid at 32x32. Enumerating every merged grid the loader can actually produce, the permutation is an involution only when the whole image fits in one window, that is when it resizes to 256x256 or smaller:Every merged grid from 9x9 through 32x32 is a non-involution; there is no second safe size above one window. A grid whose window count per edge equals the window width would be one, but that is a 64x64 merged grid, which needs 16384 patches and is past the 4096 cap, so it is unreachable.
For Qwen2.5-VL the 4-wide window makes the common 16x16 merged grid (448x448) an involution, which is why #1596 was invisible at the default fixture size. Youtu-VL is exposed at every size above a single window instead.
What changed
src/vision/encoders/youtu_vl_window.rs:reverse_window_indicesbuildsinverse[window_index[rank]] = rankdirectly, bounds-checked so a malformed grid cannot panic. The rustdoc records why the old construction degenerated. Name and signature unchanged, so the single call site is untouched.src/vision/encoders/youtu_vl.rs: the call-site comment no longer claims the old code was equivalent toargsort.src/vision/encoders/youtu_vl_tests.rs: four checkpoint-free tests, all failing on the pre-fix code: the minimal non-involution[2, 0, 1](plus an involution), the 512x512 grid with its misplaced count pinned, the single-window identity case, and a two-image batch with padding.CHANGELOG.md: entry under Unreleased / Fixed, scoped to the token order at that stage and pointing at fix(youtu_vl): vision processor emits patches in the wrong order #1610 for the reason the family still misdescribes images.Real-checkpoint validation, and why it does not show a difference
Acceptance criterion 3 asked for a Youtu-VL run at a non-involution size before and after. It was run on
models/mlx/youtu-vl-4b-instruct(tencent/Youtu-VL-4B-Instruct) on an M1 Ultra, greedy (-t 0 -n 48), release build withmetal,accelerate, at 448x448 and at 336x336, both non-involution per the table. The output is byte-identical before and after this change, and wrong in both.test_image_shapes.png(448, 160 of 196 misplaced)test_image_shapes_336.png(352, 99 of 121 misplaced)test_image.png(224, one window, 0 misplaced)The third row is the diagnostic one. 224x224 is a single window, so its permutation is the identity and this change provably cannot alter it, yet the description of a solid orange square is already wrong. So a second defect upstream of the window reordering dominates the vision path at every size, and no window-inverse fix can show a content difference until it is fixed. That defect is filed separately as #1610 and is out of scope here per this issue's own Scope section.
This PR is therefore justified on the arithmetic, which is checked directly by the new unit tests rather than inferred from generation quality: the pre-fix construction is reproduced in the tests with its exact misplaced counts, and the new inverse round-trips to the identity on every grid tried.
Test plan
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --features metal,accelerate -- -D warningscargo test --workspace --profile test-fast --features metal,acceleratecargo deny check(PR CI)Notes for reviewers
The math is not in doubt: the tests reproduce the old construction's exact misplaced counts and prove the new inverse round-trips to the identity on every grid tried. What the real-checkpoint run adds is the knowledge that this family has a second, larger defect, which is why the changelog entry claims a corrected token order at that stage rather than a corrected image description.
Checklist
feat:,fix:, etc.)docs/if user-facing behavior or supported models changed (none needed)// Used by: ...comments on any shared function I modified (unchanged: the module header already carries it).envfiles committed