Skip to content

docs(compat): 004 is not redundant at b10864, and why - #309

Merged
glennneuber merged 2 commits into
mainfrom
docs/004-not-redundant-at-b10864
Sep 18, 2026
Merged

glennneuber merged 2 commits into
mainfrom
docs/004-not-redundant-at-b10864

Conversation

@glennneuber

Copy link
Copy Markdown

Answers a question raised while deploying the v0.34.1 fold, and records the answer where it will be found next time.

The question

b10864 changed llama.cpp's own gemma4 vision defaults from set_limit_image_tokens(40, 280) to (70, 1120) — exactly the endpoints of the ladder 004-llama-cpp-gemma4-budget-fill.patch enforces. That looks like upstream converging on our behaviour, and invites deleting the patch.

The answer: no, and the numbers matching is a coincidence of endpoints

what it is
upstream's change the bounds fed to calc_size_preserved_ratio
what 004 does replaces that function for gemma4 with calc_size_budget_fill

calc_size_preserved_ratio rounds each axis to align, then clamps down over max_pixels or scales up under min_pixels. It neither snaps to the ladder nor fills to the budget — an under-budget image keeps its natural rounded grid. That is the behaviour 004 exists to replace, and b10864 does not change it.

Two checks that settle it:

  • 004 never touches set_limit_image_tokens. That line appears in the patch only as context — which is also why the patch still applies cleanly to b10864.
  • The fork passes --image-min-tokens 70 --image-max-tokens 1120 explicitly on every gemma4 launch, so the payload's own defaults never bound anything here regardless of what they are.

The note says to re-open the question only if upstream adds ladder snapping or budget filling to the dyn_size preprocessor itself — not merely because the numbers match.

Second thing recorded

The MLX path does not use this patch at all: the mlx-metal preflight profiles carry patchset = []. Apple Silicon gets the same geometry from llm.BudgetFillSize, a Go mirror of calc_size_budget_fill. They must stay in lockstep — changing one and not the other silently splits GGUF and MLX onto different grids, and only the GGUF half is covered by this patch's tests. That is easy to miss when reading a llama.cpp patch and concluding it governs everything.

Scope

Documentation only, no behaviour change. go test ./llm/ gemma4 budget tests pass. Worth remembering that the ladder is a box_2d grounding correctness constraint (docs/maxusai/gemma4-bbox-investigation-findings.md), not a performance tuning, so it should not be relaxed for tidiness.

🤖 Generated with Claude Code

glennneuber and others added 2 commits September 18, 2026 17:40
The v0.34.1 fold moved llama.cpp b10630 -> b10864, and at b10864 upstream
changed its own gemma4 vision defaults from set_limit_image_tokens(40, 280)
to (70, 1120) — exactly the endpoints of the ladder 004 enforces. That
invites the conclusion that the patch has been upstreamed. It has not, and
the next payload bump will invite it again, so the answer belongs in the
file rather than in a thread.

Checked at b10864:

- Those bounds are inputs to calc_size_preserved_ratio, which rounds to
  align and then clamps down over max_pixels or scales up under min_pixels.
  It neither snaps to the ladder nor fills to the budget: an under-budget
  image keeps its natural rounded grid. 004 replaces that call for gemma4
  with calc_size_budget_fill. Upstream changed the bounds fed to an
  algorithm; the patch changes the algorithm.
- 004 never touches set_limit_image_tokens. That line appears in the patch
  only as context, which is also why the patch still applies cleanly.
- The fork passes --image-min-tokens 70 --image-max-tokens 1120 explicitly
  on every gemma4 launch, so the payload's own defaults never bound anything
  here regardless of what they are.

Also records that the MLX path does not use this patch at all — the
mlx-metal profiles carry patchset = [] — and gets the same geometry from
llm.BudgetFillSize, which must stay in lockstep with the C++ function.
Changing one and not the other splits GGUF and MLX onto different grids,
and only the GGUF half is covered by this patch's tests.

No behaviour change. go test ./llm/ gemma4 budget tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A b9888..b10864 diff of the gemma4 branch shows image_resize_algo moving to
RESIZE_ALGO_BICUBIC alongside set_limit_image_tokens moving to (70, 1120),
which reads as two simultaneous b10864 changes. Only the token limits are.
b10630 — the payload behind 0.33.2 — already set BICUBIC, so the resize
kernel is identical on both sides of any 0.33.2-vs-0.34.0 comparison, and
the MLX path has always used CatmullRom (Keys a=-0.5, the bicubic family)
regardless.

Also records that 004's context was re-cut for this bump in fb18f5c,
which is why it still applies: at 2b95b4a the context read (40, 280) and
on main it reads (70, 1120).

Documentation only.

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

Copy link
Copy Markdown
Author

Three findings from the v0.34.1 fold session that belong beside this note — all on the same commit the fold pinned in.

1. The only gemma4 change between b10760 and b10864 is 163a40796 "model, mtmd: fix gemma4 vision handling (#28335)", the commit that moved the defaults to (70, 1120). It is also what un-broke E2B/E4B on GGUF: the fold's gate measured e2b scene IoU 0.061 → 0.496, invoice 1/5 → 5/5, fine text 0/0/0/0/0 → 4/4/3/1/0, and e4b 0.354 → 0.462 with name_bbox 0.000 → 0.697 — and #28335 carves E2B/E4B out of non-causal image decoding (they decode images causally now). The veto on that attribution is closed: the four gemma4 cells re-run today on the deployed 0.34.0 image (b10760, ggml034main_1_) reproduce the old baseline cell for cell (e2b's zeros, e4b's 0.354/0.000, 26b-a4b's bcreasoning/bcpinned both on), and the b10864 cells reproduce 4/4 across fresh reps. GGUF think-off is deterministic on both payloads; every gemma4 move is the commit's. The resize kernel is not in play: RESIZE_ALGO_BICUBIC for gemma4 entered with llama.cpp #27594 between b10488 and b10630 (the 0.33.1 fold), unflagged at the time, and the 0.33.2 and 0.33.3 campaigns are identical on all four gemma4 models.

2. The same commit is an open upstream regression, and the fork is immune to it by an accident of the launcher. ggml-org/llama.cpp#28954 (bisected to 163a40796): with the cap at 1120 image tokens, a non-causal gemma4 image chunk larger than llama-server's default n_ubatch of 512 trips GGML_ASSERT(cparams.causal_attn || cparams.n_ubatch >= n_tokens_all) in llama_context::decode and aborts the whole server — every image above ~1.2 Mpx on 12b/26b/31b. The fork passes -b N -ub N (appendBatchArgs), and mtmd_helper_decode_image_chunk splits a chunk into n_batch-sized decodes, so each decode fits its ubatch: gate B, the release canary and production since the deploy show zero asserts.

3. What the split costs instead, on both payloads. With the fork's automatic generation batch of 1024 (ctx > 4096), a top-rung gemma4 image — 1117 to 1121 tokens in the runner logs — is decoded as two non-causal sub-batches, and the first 1024 image tokens never attend to the last ~95: one-way bidirectional attention at the top rung, for as long as the fork has served 1120 (ADR 0008), so not a fold regression but a latent inconsistency the gates have measured consistently (the post-deploy preflight did 16 such 2-batch decodes on production). The clean fork fix is a generation batch ≥ the 1120 ceiling for gemma4 vision runners (2048, at the compute-buffer cost of the larger ubatch); the measurement that decides it is the top-rung bbox cells at num_batch 1024 against 2048. Until upstream fits an image chunk to one ubatch, this is one more reason 004's ladder and the launcher's batch belong in the same review — a register row now says so.

🤖 Generated with Claude Code

@glennneuber

Copy link
Copy Markdown
Author

This lands right on top of something measured today on ROCm/gfx1151, and it sharpens the conclusion: 1120 is not just the rung we pass — it is the only rung where 0.34.x is broken.

The point in this PR that matters most is "the fork passes --image-min-tokens 70 --image-max-tokens 1120 explicitly on every gemma4 launch". That turns what looked like an edge case into the default path.

The measurement

gemma4:31b-it-q4_K_M, run_budget_sweep.sh with min == max pinned per cell, think off, num_ctx 16384, gfx1151 / ROCm 7.2.x. name_bbox_mean_iou (vertical localization):

budget 0.32.1-dynres-5d5b7a72 (b9888) 0.34.1-dynres-649fea9e (b10864)
70 0.000 0.015
280 0.612 0.691
560 0.713 0.714
1120 0.728 0.000

At 280 and 560 the builds are equivalent — 0.34.1 is marginally better. The failure is confined to the top rung. Scene bbox_mean_iou barely moves there (0.961 → 0.932) because the boxes it emits are still accurate; it just emits three of six objects, and the survivors cluster by row.

prompt_eval_count is identical between builds on every test (1684 on scene_single), so the grid and token count are unchanged and only pixel values differ. It reproduces on stock ollama/ollama:0.34.1-rocm with no fork patches, so it is upstream, not ours.

Why it matters for this PR

The conclusion here — keep 004, the ladder is a box_2d grounding correctness constraint, not tuning — is right, and now has a second reason behind it. Because the fork pins 1120 on every gemma4 launch, every gemma4 vision request on a 0.34.x ROCm build runs at the one rung that is broken. The 560 rung measures clean on both builds, which is what ADR 0007 originally chose.

To be explicit about what that does not imply: capping at 560 is not a free mitigation. ADR 0003 raised the ceiling to 1120 for fine-text, and run_budget_sweep.sh does not run the fine-text probe at all (the tier fields come back empty on every budget cell), so the cost of capping is currently unmeasured. I am not proposing the cap.

Scope and caveats

  • Measured on ROCm/gfx1151 only. Per the second half of this PR, MLX does not use 004 and gets its geometry from llm.BudgetFillSize, so this says nothing about Apple Silicon.
  • Mechanism not established. Refuted so far, each by experiment rather than argument: fp16 accumulation in the vision tower (GGML_CUDA_CUBLAS_COMPUTE_TYPE f32 / f16 / unset all ≈ equally broken); multi-sub-batch image decode (num_batch 2048 decodes the image in a single decoding image batch 1/1 and is still broken); and reverting the b10864 RESIZE_ALGO_BICUBIC → BILINEAR flip for GEMMA4V/QWEN3VL (built as a compat patch, verified present in-binary by output-hash diff at identical prompt_eval/eval, and it does not fix it).
  • Controls held: the pinned build re-run today reproduces its own baseline exactly, so the host has not drifted; unaffected by OLLAMA_IGPU_DIRECT_IO=0; both builds run the model fully GPU-resident.

🤖 Generated with Claude Code

@glennneuber

Copy link
Copy Markdown
Author

Why the second commit here matters beyond tidiness: it eliminated a suspect in #310.

While investigating a fine-text OCR regression between 0.33.2 and 0.34.0, a b9888..b10864 diff of the gemma4 branch was raised showing two changes together:

-   hparams.image_resize_algo = RESIZE_ALGO_BILINEAR;   // b9888
+   hparams.image_resize_algo = RESIZE_ALGO_BICUBIC;    // b10864
-   hparams.set_limit_image_tokens(40, 280);            // b9888
+   hparams.set_limit_image_tokens(70, 1120);           // b10864

A resize-kernel change is an extremely plausible cause for a fine-text recall regression — small glyphs are exactly where interpolation quality shows. It would have been reasonable to stop there.

It is not the cause, and the reason is the correction recorded in this PR: b10630 already sets RESIZE_ALGO_BICUBIC (verified in the vendored tree at line 1658). That predates the 0.33.2 baseline, so the kernel is identical on both sides of the comparison. Only the token-limit half is a b10864 change, and the fork overrides those with explicit CLI flags regardless.

The MLX path is unaffected either way — it has always used CatmullRom, the Keys a=-0.5 cubic, the same family as the reference's PIL BICUBIC.

So a diff spanning two payload bumps invited treating both lines as new, and only one is. That is the trap this PR documents, and it cost nothing here only because the vendored b10630 tree was still on disk to check against.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant