[ngxson]

Overview

Fix #28318

• always use causal attn for smaller E2B/E4B models
• make sure global layer stays causal to match transformers impl
• fix vision token budget, supersede mtmd: correct gemma4 min/max tokens #24014

Recap which model has what:

Model	use_bidirectional_attention	Embedding dim (hidden_size)	Per-layer embedding dim (PLE)	Notes
gemma-4-E2B-it	null → causal	1536	256	35 layers, 20 KV-shared
gemma-4-E4B-it	null → causal	2560	256	42 layers, 18 KV-shared
gemma-4-12B-it	"vision" → bidirectional	3840	0	unified (encoder-free)
gemma-4-26B-A4B-it	"vision" → bidirectional	2816	0	MoE, 128 experts / top-8 (read from base repo; -it is identical)
gemma-4-31B-it	"vision" → bidirectional	5376	0	dense, 60 layers
gemma-4-31B-it-assistant	null	1024 (drafter) · backbone_hidden_size 5376	0	4-layer MTP drafter; projects from the 31B backbone
gemma-4-E2B/E4B/12B/26B-A4B-it-assistant	null	not fetched	—	expect same pattern: small drafter width + backbone_hidden_size = target's dim

Requirements

• I have read and agree with the contributing guidelines
• AI usage disclosure: yes

[ngxson]

/bot review

[ggml-gh-bot]

Automated code review

Review of PR #28335 (model, mtmd: fix gemma4 vision handling)

I reviewed the diff plus surrounding code (llama-kv-cache.cpp mask machinery, llama-model.cpp memory creation for gemma4, mtmd-helper.cpp decode path). The swa_full_non_causal bool to llama_non_causal_type enum refactor is a clean generalization, and the per-cache override in llama_kv_cache::set_input_kq_mask is correct for gemma4: it is routed to llama_kv_cache_iswa (llama-model.cpp:2602+), where the base cache is constructed with LLAMA_SWA_TYPE_NONE (llama-kv-cache-iswa.cpp:98) and the SWA cache with hparams.swa_type, so dense layers stay causal while SWA layers go bidirectional. The deepseek4 change is a mechanical rename with unchanged behavior.

Blocking / important

(point 1) Image chunks larger than n_ubatch abort the process for the bidirectional gemma4 models. tools/mtmd/clip.cpp:1639 raises the image budget from 280 to 1120 tokens, while mtmd_helper_decode_image_chunk decodes the whole chunk (up to n_batch = 2048 by default) in one llama_decode call with causal_attn = false. llama_context::decode then hits GGML_ASSERT((cparams.causal_attn || cparams.n_ubatch >= n_tokens_all)) (llama-context.cpp:1724), and with the default n_ubatch = 512 (common/common.h:452) any gemma4 image producing more than 512 tokens - e.g. roughly a 1024x1024 photo - hard-aborts llama-server/mtmd-cli instead of returning an error. The old 280 cap kept this unreachable; the new 1120 cap makes it the common case. The TODO at mtmd-helper.cpp:99 acknowledges the requirement, but nothing enforces it. Suggest either checking llama_n_ubatch(lctx) >= n_tokens in the helper and failing gracefully (with a hint to raise --ubatch-size), or clamping the per-model token budget to what the context can decode non-causally. Note that simply splitting the chunk is not a fix: tokens across split ubatches are not bidirectional to each other.

Will slow the review

(point 2) Hardcoded hidden sizes in mtmd_decode_use_non_causal (tools/mtmd/mtmd.cpp:2176-2178). Gating causality on n_embd_text != 1536 && n_embd_text != 2560 duplicates model-config knowledge (use_bidirectional_attention in the HF config) in a second place, keyed by a number rather than by config. A future gemma4 vision variant that reuses one of those widths would silently get the wrong mask. At minimum, please add a short comment naming where the numbers come from (E2B/E4B use_bidirectional_attention = null); longer term, storing that config value in the GGUF at conversion time and reading it here (via llama_model_meta_val_str) would remove the guesswork entirely and let the llama-side non_causal_type and the mtmd-side decision share one source of truth.

(point 3) Token budget values need a reference. The removed comment at clip.cpp:1636-1637 documented why the old minimum of 40 existed (poor quality with small images). The new (70, 1120) pair has no provenance. Please confirm 70/1120 match the reference image processor config for gemma4, and state in the PR or a one-line comment where they come from - also noting that this case block is shared by both GEMMA4V and GEMMA4UV, so the same budget applies to the unified 12B/26B/31B models.

Nits

(point 4) The stale // TODO: is causal == true correct? might need some changes above build_attn_inp_kv_iswa() in src/models/gemma4.cpp can now be removed or updated - this PR is what resolves that question.

(point 5) The comment "only the SWA cache (or the SWA layers of a single cache) become non-causal" (llama-kv-cache.cpp:1757-1758) overstates what the code does: for a single cache with swa_type != NONE that also holds dense layers, the dense layers would incorrectly become non-causal, since the override keys off the cache's swa_type, not per-layer. No current SWA_ONLY arch hits that (gemma4 uses the iswa split), but the comment should not claim support the code does not have.

(point 6) ctx->n_embd_text is -1 when the mtmd context was created without a text model, which makes the GEMMA4V check return non-causal. Harmless in practice (decode needs an lctx anyway), but if that path is ever reachable it defaults to the more dangerous branch.

Everything else in the diff looks correct: the in_span change is behavior-preserving for deepseek4, the enum default (ALL = 0) matches the old false default for all other archs, and print_info addition is fine. Style and comment conventions (ASCII, concise) are respected.

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

[butameron]

This PR raises the Gemma image-token cap from set_limit_image_tokens(40, 280) to (70, 1120), which now exceeds the default n_ubatch of 512 — so for models that still decode images non-causally (outside the E2B/E4B carve-out added here), a large enough image trips GGML_ASSERT(... "non-causal attention requires n_ubatch >= n_tokens") with no user configuration involved, where the previous cap of 280 always fit. Because that assert goes through ggml_abort() → abort(), the whole llama-server process dies rather than the one request failing, taking every other slot's session down with it. The existing reports of this assert (#21461, #21550, #21427, #23409) all come from explicitly raised token limits, so this seems worth noting as a new path to it; --image-max-tokens 280 restores the old cap at no memory cost, and validating max image tokens against n_ubatch at startup would turn the abort into a clear startup error.

(Disclosure: this investigation was carried out by Claude, an AI assistant. I reviewed it before posting.)