Repository navigation
fix(youtu_vl): expand the template image placeholder per merged feature - #2133
Merged
Merged
Conversation
The Youtu-VL chat template renders one `<|vision_start|><|image_pad|><|vision_end|>` per image, and the checkpoint's own processor then replaces each `<|image_pad|>` with one token per merged vision feature. `insert_youtu_vl_image_tokens` read any image token in the prompt as "already expanded" and left the single placeholder alone, so the masked scatter placed only the first (top-left) feature and dropped the rest. A uniform image survives that, which is why the solid orange control passed; every multi-object fixture was described from its corner. The insertion now mirrors the processor: one placeholder per image is expanded in place, an already expanded prompt is left alone, a prompt with no placeholder keeps the splice-after-BOS fallback, and any other count is an error. `YoutuVLModel::get_input_embeddings` also refuses a placeholder/feature count mismatch, as upstream does, instead of truncating silently. A stage-by-stage diff against the transformers reference ruled out the window path named in the issue: with the reference's own pixels the mlxcel tower output matches per merged token (cosine mean 0.997 or better on the 11x11 and 14x14 grids), and the processor's pixels are bit-identical at 448. A unit test pins `get_window_index` to reference values on multi-window grids. The two known-failing tests in tests/youtu_vl_parity.rs are un-gated and pass on the real checkpoint on CUDA, and CLI and mlxcel-server output now matches the transformers greedy output word for word on all three fixtures. Closes #1618
6 tasks done
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
Root cause of #1618 is not in the window path. The Youtu-VL chat template renders one
<|image_pad|>per image, and the checkpoint's processor (YoutuVLProcessor.__call__) replaces it with one token per merged feature.insert_youtu_vl_image_tokenssaw an image token and treated the prompt as already expanded, so the prompt carried 1 image token for 121 or 196 features and the masked scatter placed only the first (top-left) feature. A solid-color image reads correctly from any single token, which is why the 224 control passed and the "single window" pattern looked like a window bug.YoutuVLModel::get_input_embeddingsrefuses a placeholder/feature count mismatch (upstream raises) instead of truncating silently.#[ignore]d content tests intests/youtu_vl_parity.rskeep only the checkpoint requirement; stale comments attributing the 224 pass to the identity window permutation are corrected.How the alternatives were ruled out
Stage-by-stage diff against transformers 4.56.0 (checkpoint's own remote code, f32, CPU):
The mlxcel run is bf16, so per-token cosine near 1 with a few low tokens is precision, not a permutation.
get_window_index,cu_window_seqlens(including partial windows and dedup), and the full-attention boundaries agree with the reference; a new unit test pinsget_window_indexto reference values on 11x11, 14x14, a batched pair, and a non-square grid. The decisive evidence was the prompt dump: mlxcel sent128262, 128264, 128263where the reference sends128262, 128264 x 196, 128263.Test plan
Before and after on CUDA (GB10),
mlxcel generate ... -n 48 -t 0, "What shapes and colors are in this image? Answer briefly.":youtu_vl_parity --ignoredon the real checkpoint: 3 passed (both previously known-failing tests now pass)/v1/chat/completionson all three fixtures: same text as the CLI,prompt_tokens82 / 154 / 229, equal to the reference processor's token counts--lib --tests --features cuda -D warnings,cargo fmt --checkNot verified on this host
The issue's evidence was on M1 Ultra (Metal). The failure reproduced word for word on CUDA and the fix is backend-independent (token ids only), but the Metal run itself was not repeated here.
Closes #1618