Repository navigation
mtmd : fix llava vision layer count (#25817) - #25845
Closed
waleedabujaish wants to merge 1 commit into
Closed
waleedabujaish wants to merge 1 commit into
waleedabujaish wants to merge 1 commit into
Conversation
|
Hi @waleedabujaish, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
Author
1 task 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.
Overview
Fixes #25817.
For llava-projector models, the "skip the last ViT layer" step (LLaVA's
vision_feature_layer = -2) happens twice. The legacy converter alreadywrites
block_count = num_hidden_layers - 1and drops the last layer'stensors, and the graph then subtracts again (
il_last = hparams.n_layer - 1in
models/llava.cpp). Net effect: features are computed one layer short ofwhat the projector was trained on, and the last stored layer (
v.blk.22forCLIP-L/14-336) is loaded but never executed. Static check: the mmproj metadata
says
clip.vision.block_count = 23(tensorsv.blk.0..22) while the graphbuilds 22 layers.
This PR sets
il_last = hparams.n_layerand removes the MiniCPM-V/GLM-Edgeil_last += 1compensation, which existed only to undo the subtraction.Per-family arithmetic:
+= 1)feature_layers)Verification (LLaVA-1.5-7B F16 mmproj, fixed input, fp32 reference validated
against HF
get_image_features(); CLS ordering held at the graph's currentbehavior - see companion #25814; method and raw data in the repo linked
below):
With the companion #25814 fix also applied, output matches the HF reference at
0.9965 (the F16/flash-attn floor), and on LLaVA-1.6 (same
block_count = 23metadata) all 5 tiles match the correct computation at 0.99997.
Non-regression, proven bit-exact: GLM-Edge-V-2B (
THUDM/glm-edge-v-2b-gguf,SigLIP encoder, full block count - exercises the deleted
+= 1path) producesbit-identical encoder output on master and on this PR (
np.array_equaloverthe full 578x2048 embedding).
The
tools/mtmd/tests.shllava vision test invocations(
second-state/Llava-v1.5-7B-GGUF:Q2_K,cjpais/llava-1.6-mistral-7b-gguf:Q3_K_M)pass with this fix alone and with both fixes (replicated against the fixed
build - the script rebuilds
build/in place). Vision encode gets slower forthe affected families (measured +5.9% against a same-session control;
theoretical +1/22 ~ +4.5%) - that's the previously-dead layer now executing,
i.e. the correct computation, not a regression. Prefill and decode are
unchanged.
Additional information
graph; happy to combine into one PR if preferred.
together, and every refactor since preserved them.
regenerating every published mmproj file, while this graph-side fix
("stored block_count = layers to run when no explicit feature_layers")
repairs all files in circulation. No current converter can produce a
full-count GGUF for these projector types, so the reinterpretation is safe.
features now come from the layer the projector was trained on.
references: https://github.com/waleedabujaish/llama.cpp-visual-token-pruning
Requirements
(embedding-comparison harness in the linked repo); the fix itself and all
verification runs were executed and reviewed by me.