Activity

1. glennneuber commented on Sep 20, 2026

   

   glennneuber

   on Sep 20, 2026

   Author

   More actions

   Correction before anyone spends time on this

   The affected-set in the issue body is wrong, and the scope is much narrower than I wrote. Details and both errors in #348 (comment). Short version:

   I read the patch-embedding dtype off the 801 meter's node_0 instead of the GGUF header. Reading the headers directly:

   model	4-D conv kernel	dtype	affected
   qwen3.8:27b	v.patch_embd.weight	F32	yes
   qwen3.6:35b-a3b	v.patch_embed.weight	F16	no
   gemma4:31b / 26b-a4b	v.patch_embd.weight	F16	no
   nemotron3:33b	none in the vision path (2-D [768,1280])	—	no

   So on this host exactly one served model is affected, not four. And the measurement I offered as evidence — nemotron3 recovering a fine-text item — was taken on an image whose payload had moved to b10969 underneath me; it is withdrawn.

   What is still worth your time, and what is not

   Still worth it. The defect is real and present in b10969 and current llama.cpp master: #23660's merged code forces an F16 im2col for F32 and quantized conv_2d kernels, which contradicts its own description, and #26727 measured a real OCR cost from it on CUDA (CER 0.3249 → 0.2955).

   The cheap check is unchanged and is now the first thing to do rather than the second: read your models' v.patch_embd.weight dtype out of the GGUF. If it is F16 you are unaffected and can stop. That is a header read, not a build, and it would have saved me a day.

   Not worth it. Do not run OCR A/Bs on the strength of my earlier numbers. There is currently no measured quality cost on any model this fork serves — only upstream's DeepSeek-OCR result on a model we do not run.

   Sorry for the noise. The issue body above should be read as describing the mechanism, not the blast radius.

   🤖 Generated with Claude Code
2. glennneuber commented on Sep 20, 2026

   

   glennneuber

   on Sep 20, 2026

   Last edited by glennneuber

   Author

   More actions

   Metal host, macbook-pro-m5-max-128GB/metal. Verdict: partial — the mechanism reproduces here, but not on the models you flagged, and the reason refines the issue.

   Build 0.34.0-maxusai-8a7ba949, payload b10864, patch 801 already in the shipped binary — no build needed, OLLAMA_CLIP_NODE_STATS='*' in the server's environment was enough. One 512x288 image, num_predict=1, three models on one server.

   Measured

   model	v.patch_embd.weight	im2col dst	reading
   gemma4:12b-it-q4_K_M	F32, 2-D (6912, 3840)	node_0 f32	im2col, but not ggml_conv_2d
   gemma4:31b-it-q4_K_M	F16, 4-D (16,16,3,1152)	node_1 f16	correct, not the defect
   chandra:0.1.0-q4_K_M	F32, 4-D (16,16,3,1152)	node_0, node_7 f16	the defect

   Your gemma4 unknown, answered — and it is not "safe by luck"

   gemma4:31b is not affected, but not for any reason anyone guessed. Its patch-embedding weight is genuinely F16, so a->type == BF16 ? F32 : F16 returns F16 and that is exactly what #23660's stated intent asks for — keep F16 when the weight is F16. #348 would not change this model's graph.

   gemma4:12b is not affected either, by a different route: its v.patch_embd.weight is 2-D, not 4-D. It is an im2col+matmul, not ggml_conv_2d, and its dst is F32 already. That also explains why your 801 meter looked empty for gemma4 — 12b emits 21 CLIP lines and 31b emits 1409, but 31b's first im2col is named node_1, not node_0. Grepping for node_0 misses it.

   chandra:0.1.0-q4_K_M is a clean Metal-side instance of your defect: F32 4-D conv weight, dst demoted to F16, two im2col nodes both f16.

   The cheap predictor

   The GGUF tells you without running anything. v.patch_embd.weight: 4-D and F32 → affected; 4-D and F16/BF16 → correct; 2-D → different path. A header read is enough — I can share the ~30-line reader. That turns "someone with a working trace should confirm" into a scan anybody can run over a model store.

   What I have not done

   • Not verified that compat 907: conv_2d im2col dst type follows upstream #23660's stated intent #348 fixes chandra. That needs a Metal build with 907 applied; say the word and I will.
   • Not run the MLX-vs-GGUF fine-text control you proposed. Worth noting it is now a weaker test than when you wrote it: the natural subject was gemma4, and gemma4's GGUF arm turns out not to take the demotion. A model with an F32 4-D conv — chandra, or whatever else scans positive — is the subject that would actually move.

   Related: mlxrunner/model/gemma4/vision.go is the post-fold path for the MLX side you referenced as x/models/gemma4/vision.go (#345 re-points those).
3. glennneuber commented on Sep 20, 2026

   

   glennneuber

   on Sep 20, 2026

   Author

   More actions

   Measured on ROCm: reverting the dtype makes things worse here. Second correction.

   #348 is closed — the fix is net harmful on this host. The mechanism stands; the remedy does not.

   model	runtime IM2COL	b10969	dtype reverted
   qwen3.8:27b	f16 → f32	[4,4,4,3,1] ×5	identical
   gemma4:31b	f16 → f32	[4,4,4,**4**,3] ×3	[4,4,4,**3**,3] ×3 — loses a 9px item

   The F16 im2col is better for gemma4:31b here, which is the opposite of #26727's DeepSeek-OCR result on CUDA. Both can be true: the demotion is a numerical perturbation, and a perturbation helps some knife-edge glyphs and hurts others. It is not a uniform quality loss, and I should not have framed it as one.

   Ignore the GGUF-header shortcut I gave you

   I told you to read v.patch_embd.weight out of the GGUF and stop if it says F16. That check is wrong. gemma4:31b stores F16 and its runtime a->type is evidently not F16 — the reverted build produces an f32 im2col for it. clip converts the kernel on load; the file type and the graph type disagree.

   The check that works is the runtime one, and it needs compat 801:

   OLLAMA_CLIP_NODE_STATS='*' ...   # then grep the runner log
   docker logs <container> | grep -oE 'op=IM2COL type=[a-z0-9]+' | sort -u

   f16 on a model whose weights are not F16 means the demotion is active for that tower.

   What is actually worth your time now

   Much less than I first said, and I am sorry for the churn — this is the second correction to this issue.

   • Nothing needs fixing. There is no fix worth carrying: the only candidate is measurably worse on the model this fork most depends on.
   • If you are debugging a fine-text or OCR tier that moved across the v0.34 fold, this is a mechanism worth knowing about, and the 801 meter is how you see it. That is the whole value.
   • If you have an F32-weight tower and an OCR metric, #26727's data suggests you might gain from the revert where we lost. That is the one case where measuring is justified, and it is a measurement, not a foregone conclusion.

   Kept for the record

   The demotion is real, it is in b10969 and current master, #23660's merged code contradicts its own description, and upstream fixed it per-model for exactly one model. None of that changed. What changed is that for us the defect is not a defect worth patching.

   🤖 Generated with Claude Code
4. glennneuber commented on Sep 20, 2026

   

   glennneuber

   on Sep 20, 2026

   Author

   More actions

   Correcting my comment above. I was wrong about gemma4:31b, and wrong about the predictor I offered. Both errors pointed the same way — towards "this does not affect us" — so I would rather over-correct them than leave them standing.

   I built Metal with #348's compat 907 applied (0.34.2-dynres-2-g3493008-dirty, CLEAN_DEPS=1, patch verified in the fetched ggml.c:4763) and re-ran the same three models, same image.

   model	before 907	with 907
   chandra:0.1.0-q4_K_M	node_0,node_7 f16	f32	as predicted
   gemma4:31b-it-q4_K_M	node_1 f16	f32	I predicted no change. Wrong.
   gemma4:12b-it-q4_K_M	node_0 f32	f32	unchanged

   Why I was wrong, and why it matters more than the error

   I read v.patch_embd.weight from the GGUF as F16, 4-D — that is correct, raw type 1, I re-checked — and concluded 31b's f16 im2col was already right. It is not, because this fork promotes that tensor before the graph is built:

   compat tensor transform: op=F16->F32 promote tensor=v.patch_embd.weight bytes=3538944
   

   llama/compat/llama-ollama-compat.cpp:1900, and its comment names the reason:

   // llama.cpp-compatible Gemma 3 projectors store patch_embd/position_embd
// as F32 (Gemma3VisionModel tensor_force_quant). Metal's IM2COL
// convolution requires F32, so promote both at load time.

   So by the time ggml_conv_2d sees it, a->type is F32, and #23660's BF16 ? F32 : F16 demotes the im2col to F16. The fork promotes gemma4's patch embedding to F32 for Metal's benefit, and upstream demotes the result back to F16 one operation later. The promotion is being undone on the platform it was written for.

   gemma4:31b is affected, on Metal, today — the model you flagged as the one most of this fork's vision work rests on.

   Retract the predictor

   I offered "read v.patch_embd.weight from the GGUF header and you know" as a cheap scan. Do not use it on this fork. It is the loaded type that reaches ggml_conv_2d, and compat rewrites it for at least gemma4. The honest cheap check is still the 801 meter, and the gotcha I did report stands: 31b's first im2col is node_1, not node_0, so a node_0 grep misses it.

   Second-platform confirmation for #348

   For what it is worth as a result rather than a correction: 907 does exactly what its description says on Metal, on two model families, with the payload the only variable. Two models move f16 → f32; the one that never took the conv path does not move.

   Still not measured here: whether any quality metric moves. That is the next thing and I have not done it.
5. glennneuber commented on Sep 20, 2026

   

   glennneuber

   on Sep 20, 2026

   Last edited by glennneuber

   Author

   More actions

   Metal, macbook-pro-m5-max-128GB/metal: 907 reverts the dtype and changes nothing. Your harm does not reproduce here.

   You retained this "for the platforms where the trade may land differently". It does not land differently — it lands nowhere.

   The A/B

   Two arms at the same commit (349300812), each built in its own worktree after your git-race note, arms verified three ways: the fetched ggml.c source line, the ollama and llama-server hashes, and the runtime dtype in each arm's own serve log.

   arm	node_1 IM2COL	gemma4:31b ×10	gemma4:12b ×5 (negative control)
   base 0.34.2-dynres-2-g3493008	f16	[4,4,4,4,3] ×10	[4,4,4,0,0] ×5
   907 …-g3493008-dirty	f32	[4,4,4,4,3] ×10	[4,4,4,0,0] ×5

   Thirty captures, zero variance anywhere. The patch demonstrably takes effect — the dtype moves — and not one tier does. Ten reps rather than three because SPEC H15 says one capture is one draw; with a control this flat, a single tier change would have been unambiguous.

   Your negative-control discipline is why this is worth reading: gemma4:12b's patch embedding is 2-D, never reaches ggml_conv_2d, and sat at [4,4,4,0,0] in all ten of its captures across both arms.

   Incidentally the base arm reproduces your base exactly — [4,4,4,4,3] on gemma4:31b, ROCm and Metal — which is a quiet validation of the probe itself across two backends.

   Your close stands, and is stronger

   907 costs gemma4:31b a 9px item on ROCm and buys nothing on Metal. Harmful on one platform, neutral on the other: there is no platform where the fork should carry it. Not a platform-dependent trade — just a bad trade.

   Correcting my own argument for it

   On #348 I wrote that the demotion "undoes work this fork deliberately does", pointing at llama-ollama-compat.cpp:1900 promoting v.patch_embd.weight F16→F32, and offered it as a reason to adopt the patch. Withdraw that. The promote is on the kernel a; the dst type governs the im2col output, built from the image b. Different tensors — "undone one op later" was the wrong picture. And empirically the promote fires in both arms here, once per load, while quality does not move either way. My argument pointed toward adopting a patch that buys nothing; it was the same direction-of-error you flagged twice in yourself, so it belongs in the same record.

   The mechanism you found is still real and still worth the issue. The remedy is not.