Skip to content

mtmd: support DeepSeek-V4-Flash-Vision-Exp - #28133

Open
ngxson wants to merge 7 commits into
masterfrom
xsn/dsv4_vision
Open

mtmd: support DeepSeek-V4-Flash-Vision-Exp#28133
ngxson wants to merge 7 commits into
masterfrom
xsn/dsv4_vision

Conversation

@ngxson

@ngxson ngxson commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Overview

Important

Text model need to be reconverted, need to apply this: #28154

Add support for https://huggingface.co/deepseek-ai/DeepSeek-V4-Flash-Vision-Exp (text model is already supported out of the box)

Most of the things are trivial, with only one small catch: because the text model "compress" a block of 4 tokens, the image need to be aligned to this boundary (left-padded with a learned embedding token)

We assume the whole prompt is always fed into mtmd_tokenize for now, so that both the text and image can be counted and thus, the padding can also be counted correctly. For example: if we have 5 text tokens followed by an image, then 3 pad tokens must be added (so that image started at 5+3 = 8, aligned to 4)

Testing

I only tested with a small slice of the real weight

config NMSE (IMAGE rows) sentinels/layout
bf16 mmproj, Metal (default) 1.19e-04 bit-exact
f32 mmproj, Metal 1.42e-04 bit-exact
f32 mmproj, CPU 1.34e-05 bit-exact
f32 mmproj, CPU, -fa off 1.20e-05 bit-exact
truncated to 1 / 4 ViT layers 5.7e-07 bit-exact

Requirements

@ngxson
ngxson requested review from a team and CISC as code owners September 1, 2026 01:06
@github-actions github-actions Bot added mtmd Related to multimodal functionality (video/image/audio) conversion labels Sep 1, 2026
noonghunna pushed a commit to noonghunna/club-3090 that referenced this pull request Sep 1, 2026
Corrects a wrong claim that reached a public announcement (#1122, since
withdrawn), the compose headers, both registry notes, BENCHMARKS.md and
the learnings file.

What was published: "the name says Vision, the weights do not" — that
DeepSeek-V4-Flash-Vision-Exp has no vision capability.

What is true: deepseek-ai/DeepSeek-V4-Flash-Vision-Exp is a genuine VLM.
Tagged image-text-to-text, config.json carries a full vision block
(vision_dim 1024, vision_n_layers 32, vision_patch_size 14), and the
safetensors index holds 263 vision tensors (vision.patch_embed.*,
vision.blocks.0-31.*). It ships inference/vision.py and an image
processor.

The unsloth GGUF carries ZERO of them, so llama-server returns
`image input is not supported` and the slug is text-only IN PRACTICE.

Why no projector exists yet: llama.cpp support was opened upstream
2026-09-01 as ggml-org/llama.cpp#28133 (mtmd: support
DeepSeek-V4-Flash-Vision-Exp, @ngxson, +498/-5, labels mtmd +
conversion). Its notes confirm the text model already works out of the
box -- matching what we see -- and what it adds is the CONVERSION that
emits an mmproj. 23 HF repos checked for this model: none carries an
mmproj, because until that merges the conversion cannot produce one.
Tracked in docs/UPSTREAM.md as a re-test trigger; author discloses ~90%
AI-generated code and testing on a slice of the weights, so merge is not
assured.

vision_capable: false remains correct for this GGUF -- only the recorded
REASON was wrong.

The methodology error, kept in learnings: the GGUF gate was sound for the
question it asked ("can this GGUF serve images?") and its answer was
right, later confirmed by the server's own runtime rejection. The error
was over-generalising it to "this model has no vision" without reading
the source model card. A second error compounded it -- the first
withdrawal notice claimed vision was architecturally integral so no
companion mmproj could exist, which #28133 disproves.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
noonghunna pushed a commit to noonghunna/club-3090 that referenced this pull request Sep 1, 2026
…le too

Missed in 4dd88b3 -- the profile's vision_capable comment still said the
MODEL had no vision. The value stays false (correct for this GGUF); the
reason now points at ggml-org/llama.cpp#28133 and the missing conversion.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@am17an

am17an commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

I think this would break when we have image + text in the same ubatch because of how we handle the the kv-cache i.e. positions are conflated with cells

@ngxson

ngxson commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

@am17an not sure what you mean, but currently we cannot have both image embd + text in batch (or ubatch). that would require #24669 first

the only case where embd + text in same llama_batch that we currently support is MTP embd

@am17an

am17an commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

ah right, but even with pure image batch, like two images it would not work?

@ngxson

ngxson commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

if 2 consecutive images (no text in-between), then they should look exactly like one single image? because input images are already padded, they are aligned to compress ratio, and input batch will contain only embeddings

@ngxson

ngxson commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

/bot review

@ggml-gh-bot

ggml-gh-bot Bot commented Sep 1, 2026

Copy link
Copy Markdown
Automated code review

Static review of PR #28133 (mtmd: DeepSeek-V4-Flash-Vision-Exp)

Overall this is a clean, conventional addition: no new ggml ops, tensor names go through tensor_mapping.py, build_vit is reused, the serialization version is bumped, and the conversion asserts the values the C++ side hardcodes. The layout/index arithmetic is consistent across clip_n_output_tokens, set_input, the graph build, and the preprocessor's grid_tokens (I checked all four copies). Below are the points I would resolve before merge.

Verify (correctness)

(point 1) tools/mtmd/mtmd.cpp:1470 - the lead-pad formula does not match the PR description. Code: lead_pad = align - 1 - (n_past % align) gives 2 pads for 5 preceding text tokens and 3 pads when n_past % 4 == 0, i.e. it places IMAGE_START at position 3 (mod 4) so the aligner grid starts on a block boundary. The description instead says "5 text tokens -> 3 pad tokens, image started at 8", which implies START at 0 (mod 4) - and is internally inconsistent (3 pads would put START at 8 and the grid at 9). One of the two is wrong with respect to inference/image_processor.py; please double-check against the reference and fix whichever does not match. Also worth a one-line comment: the alignment assumes no tokens are injected outside mtmd_tokenize (e.g. a BOS added by the caller would shift everything by one; DeepSeek has none today).

(point 2) The KV-cache/positions question raised in the PR comments (image embeddings + text in the same ubatch, positions conflated with cells) is still open and affects the core premise of this design; please settle it in the discussion before merge.

Will slow the review

(point 3) conversion/deepseek.py:1086 - the wqkv chunk(3) split duplicates existing infrastructure. V_ENC_ATTN_QKV already exists in tensor_mapping.py and build_vit already has a fused-QKV path that splits the activation with ggml_view (the preferred pattern per project guidelines). Mapping vision.blocks.{bid}.attn.wqkv to V_ENC_ATTN_QKV would remove the conversion-side split entirely.

(point 4) The block-layout arithmetic (rows, row_len, pad_last, n_out) is duplicated in four places, each carrying a "keep in sync" comment: tools/mtmd/clip.cpp:4201-4204, tools/mtmd/clip.cpp:5095-5125, tools/mtmd/models/deepseek4v.cpp:88-91, tools/mtmd/mtmd-image.cpp:1110-1119. A single shared helper computing the layout from (n_llm_w, n_llm_h, lead_pad) would remove the sync risk and the comments.

(point 5) tools/mtmd/mtmd-image.cpp:1132 - GGML_ASSERT(max_w > 1) sits on a path driven by user-controlled inputs (image dimensions, --image-max-tokens). I could not construct a reachable case with the current clamps (min 16), but the solver is a loop with a decrementing budget; a future change to the clamp makes this a process abort instead of a catchable error. Prefer a throw or a clamp here, like the other branches.

(point 6) tools/mtmd/mtmd.cpp:1462 - the hardcoded align = 4 duplicates the text model's compressor block size, which the conversion already writes to the text GGUF (dspark_block_size). If a future V4 variant ships a different block size, vision alignment silently breaks. Consider plumbing the value from the text model, or at least document the coupling next to the constant.

(point 7) tools/mtmd/clip.cpp:1595 - rope_theta is hardcoded to 10000.0f while the conversion writes vision_rope_theta (default 10000) into the GGUF. If they ever diverge, the runtime silently ignores the GGUF value. Prefer reading the key with a fallback, or stop writing it in the conversion.

Nits

(point 8) Typo in tools/mtmd/mtmd.cpp:1462: "preceives" -> "perceives".

(point 9) tools/mtmd/clip.cpp:5080 - n_patches_per_col actually holds the number of patches per row (the name is inherited from the pixtral case; a rename to n_patches_per_row would help the new code read correctly).

(point 10) tools/mtmd/models/deepseek4v.cpp header comment says "each aligner row ends with a NEWLINE" - the phantom padding row emits PADs for the entire row including the newline slot (clip.cpp:5117), so the comment slightly overstates; worth a word.

Testing and process notes

All numeric validation was done against a small slice of the weights. The sentinel/layout bit-exactness is convincing, but please run at least one end-to-end sanity check with the full mmproj + text model on a real image before merge.

The AI disclosure is filled in (~90% AI-generated). Per AGENTS.md and the notice in tools/mtmd/models/models.h, the contributor is expected to own every line and be able to explain and maintain this independently of the tooling - please be prepared to do so in the review discussion.

This review was generated automatically by pi coding agent using zai-org/GLM-5.3. It may contain mistakes. Maintainers make the final call.

@am17an

am17an commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Ah I think M-rope wouldn't work, but this model does not seem to use that. So maybe it's ok

Comment thread conversion/deepseek.py Outdated
Comment thread conversion/deepseek.py Outdated
@ggerganov

Copy link
Copy Markdown
Member

Ah I think M-rope wouldn't work, but this model does not seem to use that. So maybe it's ok

So we don't need to resolve this for now?

//FIXME : note that we conflate token positions with rows, which is not true for multi-modal case.

@am17an

am17an commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

@ggerganov since they're not using M-RoPE I would consider this resolved for now, unless you see some other issue with this (apart from not being able to run unified kv-cache)

@ggerganov

Copy link
Copy Markdown
Member

It should probably be OK. Maybe we can add an assert in llama_kv_cache_dsv4::init_batch to guarantee that we never receive tokens with the same sequence position - this is what the logic currently assumes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conversion mtmd Related to multimodal functionality (video/image/audio)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants