Skip to content

fix(models): settle the GELU formula for the families #2423 left open - #2785

Merged
inureyes merged 3 commits into
mainfrom
fix/issue-2548-gelu-formula-decisions
Oct 31, 2026
Merged

inureyes merged 3 commits into
mainfrom
fix/issue-2548-gelu-formula-decisions

Conversation

@inureyes

@inureyes inureyes commented Oct 31, 2026 •

Copy link
Copy Markdown
Member

Summary

Settles the GELU formula for every call site #2423 left as an open decision, using one rule: the checkpoint config decides, and where the key is omitted, the Hugging Face config-class default for that model_type decides, evaluated as ACT2FN does. Where the mlx-lm or mlx-vlm port departs from Hugging Face, mlxcel follows Hugging Face and the row records the divergence. The full per-row table with reasons is in docs/gelu-call-sites.md section 0b, and every affected inventory row carries a #2548 note.

Closes #2548

Changes

  • Gemma (input-dtype tanh, the same formula the quantized Gemma kernels already used): compiled_gelu_mlp_forward_fp16 (Gemma v1 bf16, Gemma 4 dense bf16 MLP, Gemma 4 assistant drafter), the Gemma 4 tensor-parallel per-layer gate, Gemma3n compiled_gelu_topk and the RecurrentGemma MLP. The Gemma3n XLA sparse_gelu_stages tape, the gemma3n_math host oracle and the two xla-diagnostics manual graphs moved to tanh with it.
  • Compiled f32 tanh (GeluActivation::TanhF32Compiled): Phi, Phixtral, GPT-2, StarCoder2 (eager), Moondream2/3 LM and vision, RecurrentGemma y-branch, and the SigLIP-style towers of Phi-4-MM, DeepSeek-VL2, Youtu-VL, Jina-VLM, LFM2-VL, PaddleOCR-VL, Aya Vision, Granite Vision and Bunny.
  • quick_gelu (GeluActivation::Fast): Qwen2-VL and Molmo ViT MLPs in eager and in the XLA emitters (new numeric_ops::quick_gelu), LLaVA 1.5 / NeXT CLIP and Phi-3.5-V CLIP.
  • Shared vision config: an omitted vision_config.hidden_act resolves by tower model_type (SigLIP / SigLIP2 to tanh, CLIP to quick_gelu); a declared quick_gelu now parses as fast in the eager VisionConfig and in the XLA llava vision config.
  • MiniCPM-o reads its vision MLP activation from vision_config.hidden_act.
  • Nemotron-NAS parses hidden_act once at load (NasActivation): GELU names go through GeluActivation::from_hf_name instead of falling back to SiLU, and an unknown name is a load error.
  • The Nemotron codec ConvNeXt (erf) and VoiceChat TTS backbone (tanh) private GELU copies fold onto GeluActivation, each with a tolerance test against the copy it replaced.

Kept as they were, with the reason recorded: GPT-NeoX, ModernBERT, Llama 4 vision (HF gelu), Kokoro PLBert, Molmo2, Qwen3-VL-MoE, Qwen3-Omni, LLaVA-Interleave and Llama-Nemotron-VL-Embed (already matched their config).

Decisions that need a maintainer look

  • StarCoder2 resolves opposite to the issue's framing. The issue treated the XLA tanh graph as the bug. starcoder2-3b-4bit declares gelu_pytorch_tanh (also the Starcoder2Config default), and the XLA StarCoder2 goldens were frozen after that graph ran token-exact against the HF fp32 oracle. Under the decision rule eager is the side that moves, so eager switched to tanh and assets/starcoder2/*.mlir is untouched. mlx-lm's nn.gelu departs from HF here.
  • GPT-BigCode (T5) is the same situation as StarCoder2 and is left unchanged for a follow-up.
  • The fused single-kernel tanh GELU raised in the issue thread is out of scope here and tracked in perf(cuda): single-kernel tanh GELU so tanh-switched families match erf prefill cost #2780; nothing in this PR changes compiled_gelu_tanh_f32.
  • Gemma3n eager and XLA round differently for bf16 input (pre-existing, the erf versions had the same split). In the eager compiled_gelu_topk, the bf16 stddev multiplied by the f32 std_multiplier array promotes the cutoff, shift and tanh to f32 with one final rounding; the XLA sparse_gelu_stages tape rounds each intermediate through bf16, as mlx-lm's weak Python scalars do. docs/gelu-call-sites.md G15/G17 now say so. The new FFI test cannot separate the two (its reference computes mean and variance in f32). A same-input eager vs XLA comparison needs an IREE host (see below).

Tests

  • mlxcel-core (CUDA, GB10, --test-threads=1, MLX_ENABLE_TF32=0): 26 GELU tests pass, including two new ones. test_compiled_gelu_topk_is_the_tanh_gelu_of_the_shifted_input checks f32 within 2e-6 of tanh and more than 1e-4 from erf. test_compiled_gelu_mlp_forward_fp16_uses_the_tanh_geglu does the same for the dense GeGLU.
  • Root crate, narrow filters, all pass: models::gemma3_backbone (6), audio::nemotron_codec (8), models::nemotron_nas (12), vision::config (3), vision::encoders::siglip (5), vision::encoders::minicpmo (3), models::nemotron_voicechat::tts (14), models::gemma3n:: (4), models::recurrent_gemma (9), models::phi (61), models::gpt2 (28), vision::encoders::molmo (1), loading::vlm (250). The Gemma3 backbone fold measures worst error 0 (f32), 4.15e-3 (bf16) and 9.77e-4 (f16) against mlx.nn.gelu_approx.
  • mlxcel-xla: 64 emitter and validation:: tests pass, StarCoder2 byte-exact goldens included.
  • tests/cuda_custom_kernel_disk_cache.rs: pass.
  • cargo clippy --lib --tests --features cuda -D warnings is clean for the root crate. The same check is clean for mlxcel-core and mlxcel-xla. cargo fmt --check is clean.
  • Full CUDA workspace suite (cargo test --workspace --profile test-fast --features cuda --no-fail-fast -- --test-threads=1) on this branch: 14859 passed, 1 failed. The failure is tests/step3p7_parity.rs::text_decode_delegates_sequence_local_native_batching, which fails identically on the base commit 29c2f0c2, so it is not from this PR.
  • --features xla-diagnostics was not compiled: there is no IREE distribution on this host (IREE_DIST unset). The changed diagnostics graphs in gemma3n.rs were reviewed by hand. mlxcel_core::tanh is exported and the renamed export keys have no other consumers. Run them on an IREE host.

Smoke (GB10, greedy, one mlxcel generate and one mlxcel-server request each)

Vision prompts used tests/fixtures/test_image.png, a flat orange square.

Family (rows) Checkpoint CLI Server
StarCoder2 (T6) starcoder2-3b-4bit correct fibonacci body same
Phi (T1) phi-2-4bit correct fibonacci body same
Moondream2 (T19, V27) moondream2 "...a vibrant orange background..." "...orange square with uniform color"
Moondream3 (T17, V28) moondream3-preview-mlx-4bit "A solid orange background with a uniform color." same wording
Gemma3n (G15-G18) gemma3n-e2b-4bit, gemma3n-e4b-bf16 "The capital of France is Paris." same
Gemma 4 drafter (G12) gemma-4-31b-it-4bit + gemma-4-31b-it-assistant-bf16 Paris, MTP acceptance 6/6 Paris; MTP exactness probe passed byte-identical
Aya Vision (S3) aya-vision-8b cityscape (see note) same
Granite Vision (S8) granite-vision-3.2-2b-4bit "A bright orange square with a black border." same
DeepSeek-VL2 (V11) deepseek-vl2-small-4bit "The image is a solid orange color." same
Youtu-VL (V14) youtu-vl-4b-instruct solid orange block same
Jina-VLM (V16) jina-vlm-mlx solid orange square solid orange rectangle
PaddleOCR-VL (V29) paddleocr-vl-bfloat16 "graphic design, no chart" (OCR model) same
LLaVA 1.5 (S9) llava-1.5-7b-4bit "A bright orange background..." same
LLaVA-NeXT (S9) llava-next-mistral-7b-4bit solid bright orange same
Bunny (S11) bunny-llama3-8b-4bit "A bright orange fabric..." same
Phi-3.5-V (S12) phi-3.5-vision-4bit solid orange same
LFM2-VL (V25) lfm2-vl-450m-4bit, lfm2-vl-1.6b-4bit solid orange orange (1.6B), "red" (450M)
Qwen2-VL (V19) qwen2-vl-2b-instruct-4bit white cat (see note) same
Molmo (V22) molmo-7b-d-0924-4bit solid orange square same
MiniCPM-o (V5) minicpm-o-4.5-4bit "An orange background..." same

Notes:

  • Not introduced by this PR: Qwen2-VL "white cat" and Aya "cityscape" on the flat orange fixture, and Moondream2's leading "omentous" fragment. A build with those three files reverted to main produces the same Qwen2-VL and Aya output token for token, and the same Moondream2 "omentous" prefix (one later word differs). On tests/fixtures/test_image_shapes.png, both main and this branch name the red square, blue circle and green triangle correctly for Qwen2-VL and Aya. The wording differs between the two, which shows the switched activation runs.
  • Not run (no local checkpoint; the synthetic test that covers the row is in parentheses): Gemma v1 bf16 and Gemma 4 dense bf16 (the dense GeGLU FFI test, plus the drafter smoke on the same kernel), RecurrentGemma (models::recurrent_gemma), Phixtral, GPT-2 (models::gpt2), Phi-4-MM, and the Gemma 4 TP gate (needs multi-host).
  • TTS smoke not run (no checkpoint); covered by backbone_gelu_stays_within_rounding_of_mlx_nn.
  • Nemotron-NAS was not run. The only local checkpoint (llama-3.3-nemotron-super-49b-v1-4bit) declares silu, so its path is unchanged. models::nemotron_nas covers the parsing.

Apply one rule to every GELU call site #2423 listed as an open decision: the checkpoint config, or the Hugging Face config-class default when the key is omitted, decides the formula as ACT2FN evaluates it. Where the mlx-lm or mlx-vlm port departs from Hugging Face, follow Hugging Face and record the divergence. The per-row decisions and reasons are in docs/gelu-call-sites.md section 0b.

- Gemma: the dense bf16 GeGLU kernel (Gemma v1, Gemma 4 dense MLP, Gemma 4 assistant drafter), the Gemma 4 tensor-parallel gate, Gemma3n gelu_topk and the RecurrentGemma MLP now use the tanh GELU in the input dtype, matching the quantized kernels. The Gemma3n XLA sparse GELU tape and host oracle moved with it.
- Phi, Phixtral, GPT-2, StarCoder2 (eager), Moondream2/3 and the SigLIP-style towers of Phi-4-MM, DeepSeek-VL2, Youtu-VL, Jina-VLM, LFM2-VL, PaddleOCR-VL, Aya Vision, Granite Vision and Bunny switch from erf to the compiled f32 tanh GELU. The StarCoder2 XLA graph was already tanh and token-exact against the HF fp32 oracle, so eager moves to it.
- Qwen2-VL, Molmo, LLaVA CLIP and Phi-3.5-V CLIP use quick_gelu, in eager and in the XLA emitters.
- An omitted vision_config.hidden_act now resolves by tower model_type (SigLIP/SigLIP2 to tanh, CLIP to quick_gelu), and a declared quick_gelu parses as fast.
- MiniCPM-o reads its vision activation from vision_config.hidden_act.
- Nemotron-NAS parses hidden_act once at load: GELU names go through GeluActivation::from_hf_name instead of falling back to SiLU, and unknown names are a load error.
- The Nemotron codec and VoiceChat TTS private GELU copies fold onto GeluActivation with tolerance tests.

The fused single-kernel tanh GELU raised in the issue thread is deferred to a follow-up.

Closes #2548
@inureyes inureyes added status:review Under review type:bug Bug fixes, error corrections, or issue resolutions priority:medium Medium priority area:models Model architectures, weights, loading, metadata area:core mlxcel-core: MLX FFI, primitives, KV cache, layers labels Oct 31, 2026
@inureyes

Copy link
Copy Markdown
Member Author

Implementation Review Summary

Intent

Settle the GELU formula for every #2548 row by the checkpoint config (or the HF config-class default), keeping eager and OpenXLA in step and recording each decision in docs/gelu-call-sites.md section 0b.

Findings Addressed

No CRITICAL or HIGH findings, so no code fixes. Checked and confirmed: each changed row matches the local checkpoint config or remote modeling code (StarCoder2 and Gemma v1 gelu_pytorch_tanh, Phi-2 gelu_new, Molmo image_mlp_activations="quick_gelu", DeepSeek-VL2 siglip_vit.py nn.GELU(approximate="tanh"), Phi-3.5-V openai/clip-vit-large-patch14-336). compiled_gelu_mlp_forward_fp16 has only Gemma callers. The shared VisionConfig serde default stays erf, and no local checkpoint declares quick_gelu explicitly. Nemotron-NAS rejects unknown names at load. Molmo2 does not reuse the Molmo encoder.

Implementer deviations: StarCoder2 eager moving to tanh is correct under rule 1 (the XLA goldens were frozen after a token-exact match against an HF oracle built with gelu_pytorch_tanh, PR #561). Leaving T5 out is acceptable as out of scope. xla-diagnostics cannot build without IREE_DIST (build.rs requires it).

Remaining Items

  • Gemma3n eager vs XLA tape carrier (MEDIUM, pre-existing). compiled_gelu_topk multiplies a bf16 stddev by an f32 array(std_multiplier), so MLX promotes everything from the cutoff onward to f32 and rounds once at the end. sparse_gelu_stages rounds every intermediate through bf16 (the mlx-lm weak-scalar tape). The erf tape had the same shape. The doc label "tanh (input dtype)" for G15/G17 is therefore f32 for bf16 inputs. This also explains why the new FFI test could not separate the two carriers: its reference computes mean and variance in f32, while the kernel computes them in bf16. Settle it on an IREE host.
  • Idefics2 (S6) and Granite 4 Vision (S7) force quick_gelu while HF evaluates tanh (MEDIUM, outside fix(models): decide the GELU formula for the families #2423 left open #2548). They now contradict the rule this PR adopts. Follow-up issue.
  • GPT-BigCode (T5) declares gelu_pytorch_tanh and still runs erf (LOW, disclosed follow-up).
  • Doc row T17 says "No local ckpt", but moondream3-preview-mlx-4bit was smoked (LOW).
  • C++ compiled_gelu is now used only by tests (LOW).

Verification

  • All stated requirements implemented
  • No placeholder/mock code remaining
  • Integrated into project code flow
  • Project conventions followed
  • Existing modules reused where applicable
  • No unintended structural changes
  • Tests pass (one pre-existing step3p7_parity failure, identical on the base commit)

Technical report added in 7aa6558d.

@inureyes

Copy link
Copy Markdown
Member Author

Security and performance review

Reviewed at 7aa6558d against #2548 and its thread. No CRITICAL or HIGH findings, so no fixes were committed.

Untrusted config parsing

  • NasActivation::from_hf_name, mlp_activation_from_vision_config (MiniCPM-o) and the XLA LlavaVisionConfig all return errors on unknown names; no new unwrap, expect or indexing on checkpoint JSON in non-test code. A non-string MiniCPM-o hidden_act is treated as omitted (tanh), which is lenient but harmless.
  • MEDIUM (not fixed, below threshold): the shared eager VisionHiddenActivation keeps #[serde(other)] ExactGelu, so gelu_new, gelu_fast, relu or silu on a SigLIP/CLIP tower still run erf silently, while GeluActivation::from_hf_name and the XLA config treat gelu_new/gelu_fast as tanh and reject the rest. This contradicts the PR's "evaluated as ACT2FN does" rule for every loader on the shared VisionConfig. Rare on vision towers; worth a follow-up that parses through from_hf_name and errors on non-GELU names.

Hot-path activation per family

  • TanhF32Compiled: Phi, Phixtral, GPT-2, StarCoder2, Moondream2/3 LM and vision, RecurrentGemma y-branch, Phi-4 SigLIP, DeepSeek-VL2, Youtu-VL, Jina-VLM, LFM2-VL, PaddleOCR-VL, MiniCPM-o, Nemotron-NAS tanh names, and the shared SigLIP MLP for omitted-key towers (Aya, Granite, Bunny, SigLIP LLaVA).
  • Compiled input-dtype tanh GeGLU: Gemma v1 and Gemma 4 dense bf16 MLP, Gemma 4 TP gate (argument order matches gemma4.rs:2970), RecurrentGemma MLP, Gemma3n compiled_gelu_topk.
  • Eager Tanh: only BACKBONE_GELU (Gemma3 backbone, TTS GeluMlp), eager before this PR by design; it now cubes with x*x*x instead of power, which removes the bf16 NaN hazard. gemma3n.rs:785 is unreachable.
  • Eager Fast: Qwen2-VL, Molmo, LLaVA CLIP, Phi-3.5-V CLIP; dtype-preserving and cheaper than the eager erf it replaces.
  • No new eager TanhF32 site. LOW (pre-existing from investigate: gelu_approx computes erf GELU where upstream models use the tanh approximation #2423): a declared gelu_pytorch_tanh on the shared SigLIP MLP still maps to eager TanhF32.
  • LOW: StarCoder2 moved from the single fused erf kernel to the compiled f32 tanh, which on CUDA still runs as more than one module; decode cost unmeasured here (benchmarks not run on this host), tracked by perf(cuda): single-kernel tanh GELU so tanh-switched families match erf prefill cost #2780.

Kernel cache keys

Compiled-graph modules are named from the op tape plus a constants hash, and the PTX disk cache keys on that module name. Erf to tanh changes the tape, so old entries are orphaned rather than reused. compiled_gelu_mlp_forward_fp16 now reuses the existing compiled_geglu_approx_activation graph (one fewer distinct module) and compiled_gelu_topk gets a new entry. f16 overflow in the input-dtype tanh saturates through tanh(+-inf) to finite results, no NaN path.

Row T17 claimed Moondream3 had no local checkpoint, but models/moondream3-preview-mlx-4bit exists and was smoke-tested after the tanh change. Rows G15 and G17 (and the summary row) labelled the Gemma3n compiled_gelu_topk path "tanh (input dtype)"; for bf16 inputs the eager kernel promotes the cutoff, shift and tanh to f32 and rounds once, while the XLA tape rounds each intermediate through bf16. The label now says so and records the eager/XLA gap as pre-existing, needing a same-input comparison on an IREE host.

Validation: docs-only change; fmt and clippy checks run on the root crate, mlxcel-core and mlxcel-xla.

Refs #2548
@inureyes inureyes added status:done Completed and removed status:review Under review labels Oct 31, 2026
@inureyes
inureyes merged commit 7750a13 into main Oct 31, 2026
33 checks passed
@inureyes
inureyes deleted the fix/issue-2548-gelu-formula-decisions branch October 31, 2026 13:11
inureyes added a commit that referenced this pull request Oct 31, 2026
Five rule-breakers outside #2548's row list, all following the section-0b rule: the checkpoint config decides, an omitted key falls back to the Hugging Face config-class default, and mlxcel follows HF where mlx-lm or mlx-vlm departs.

- GPT-BigCode now reads activation_function (absent or null means the GPTBigCodeConfig default gelu_pytorch_tanh), resolves it once at load onto MLP, and rejects non-GELU names in validate(); santacoder's declared tanh runs the compiled f32 kernel instead of the erf mlx-lm applies.
- Idefics2 and Granite 4 Vision stop forcing use_fast_gelu (mlx-vlm's FastGELUMLP): the tower config decides, with Idefics2 resolving an omitted key to the Idefics2VisionConfig default via default_missing_hidden_act_to_siglip.
- The shared VisionHiddenActivation maps every ACT2FN GELU name through GeluActivation::from_hf_name (gelu_new / gelu_fast now tanh) and rejects non-GELU names as load errors instead of silently running erf; Bunny's sparse-config fallback propagates the error rather than swallowing it.
- A declared gelu_pytorch_tanh on the shared SigLIP MLP runs the compiled kernel like the omitted-key path (VisionMlpActivation::PytorchTanh removed, SiglipDefaultTanh folded in).
- The dead C++ compiled_gelu is removed with its three FFI tests and both compile-audit entries; the XLA llava vision config accepts the same GELU name set. Docs rows T5, S6, S7 and the shared-config rule updated.

Validated: cargo fmt, clippy --all-targets -D warnings (mlxcel, mlxcel-core, mlxcel-xla), the mlxcel-xla vision_config tests (10 passed), and a clean cargo check on the three packages. The GPU lib-test gate and VLM smoke runs were not run before this commit: the GB10 gpu-lock was held by another session; the orchestrator's contract gate covers them. No GPT-BigCode checkpoint exists on this host, so that smoke cannot run here.

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

Labels

area:core mlxcel-core: MLX FFI, primitives, KV cache, layers area:models Model architectures, weights, loading, metadata priority:medium Medium priority status:done Completed type:bug Bug fixes, error corrections, or issue resolutions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(models): decide the GELU formula for the families #2423 left open

1 participant