Skip to content

fix(core): reject quantization params MLX would abort on, at load - #956

Merged
inureyes merged 3 commits into
mainfrom
fix/issue-929-quantization-bounds
Jul 28, 2026
Merged

fix(core): reject quantization params MLX would abort on, at load#956
inureyes merged 3 commits into
mainfrom
fix/issue-929-quantization-bounds

Conversation

@inureyes

@inureyes inureyes commented Jul 28, 2026

Copy link
Copy Markdown
Member

Summary

An unvalidated quantization block in config.json reached MLX and terminated the process. reconcile_quantization_layout returned a non-positive group_size / bits pair unchanged on a trust-the-caller basis, and MLX reconstructs a quantized matrix's unpacked width as w.shape(-1) * 32 / bits in validate_quantized_input, so a declared "bits": 0 is a division by zero and anything above 32 collapses the quotient to something no real scales width can match. quantized_matmul, gather_qmm and dequantize all cross the cxx bridge as UniquePtr<MlxArray> rather than Result, so the C++ throw is an uncatchable std::terminate at the first forward pass rather than a load error: a repository with real .scales and "bits": 0 loaded cleanly and took mlxcel-server down on the first request that touched a quantized projection. The second half of the issue is the same shape: validate_embedding_table skipped the width check for a quantized table, so a table honestly packed for a different model width kept exactly the right row count, passed load, and then fed a wrong-width hidden state into fast::layer_norm / fast::rms_norm.

Both are now rejected at load, on every quantizing family, dense and MoE.

What changed

src/lib/mlxcel-core/src/layers.rs

  • New validate_quantization_params(group_size, bits): rejects bits outside 1..=32 and group_size < 1, with a message naming the offending field and value. It is a bounds check rather than an allowlist of the widths MLX supports, because mlxcel deliberately re-derives bit width from tensor shapes when the declared value disagrees, and an allowlist of {2,3,4,5,6,8} would reject the mixed-precision exports that behavior serves (the Qwen3.5/3.6 MoE per-path overrides, the minicpm-v mxfp4 group-size case).
  • reconcile_quantization_layout calls it before anything else. The old early return folded "degenerate shapes" and "degenerate params" into one condition, which is what let an empty shape launder a hostile pair; the shape case stays permissive, the param case does not.
  • New validate_quantized_packing plus QuantizedTensorShapes, hoisted verbatim (messages included) out of bailing_moe.rs. It reconstructs the input width the way MLX does, as scales.shape(-1) * group_size using the reconciled group size, and checks .biases shape equals .scales shape. Works for a 2-D projection and a 3-D stacked expert plane alike.
  • New infer_quantization_mode(has_biases, group_size, bits), deduplicating the identical mode-inference block that sat in UnifiedLinear::from_weights, QuantizedEmbedding::from_weights and bailing_moe::quant_mode.
  • New UnifiedEmbedding::quantized() accessor, so a load-time validator can reach the scales shape and the reconciled params that is_quantized() alone cannot give it.

src/models/switch_layers.rs (the scope correction the issue records)

SwitchLinear::from_stacked_parts never calls the reconciler, re-derives bits itself and accepts the result only in 2..=8, and never infers group_size at all, so a declared 0 zeroed the denominator, skipped inference entirely, and was stored on SwitchLinear::Quantized and handed to gather_qmm, which reaches the same /bits line through the same extract_quantized_matmul_dims. A reconciler-only guard would have left every quantized MoE checkpoint exposed. It is now fallible, takes the prefix so the error names the tensor, bounds the declared pair itself, and additionally refuses a stored triple that fails MLX's own predicate from validate_quantized_input, evaluated in i64 so the multiply cannot wrap on a large expert plane. That second check matters because the existing fallback to the declared bits whenever the shapes solve outside 2..=8 guaranteed a throw. The mixed-precision inference the fallback exists for is untouched (a genuinely 8-bit plane under a 4-bit config default still reconciles), degenerate shapes evaluate to 0 == 0 and stay permissive, and a non-quantized expert plane is not gated on quantization params at all.

The families that carry a local quantized expert type are unchanged and are not in this diff, and they are not covered by this PR. An earlier version of this body claimed they were, on the grounds that each loads embed_tokens through UnifiedEmbedding::from_weights first. That is wrong and has been retracted: UnifiedEmbedding::from_weights only reaches the reconciler when .scales exists, so a float embedding with quantized expert planes (the canonical gpt-oss shape) never touches a guarded loader; gpt_oss resolves quantization params per weight prefix, so a hostile pair scoped to the experts leaves every guarded loader with a valid one; and mamba / mamba2 build QuantizedEmbedding by hand, bypassing the reconciler entirely. Those eighteen call sites are filed as #958.

src/models/gpt2.rs

validate_embedding_table no longer skips the width check for a quantized table. The dequantized width is recoverable at load, so the quantized case goes through the shared validate_quantized_packing and the four families behind that helper (BailingMoe, Gpt2, GptBigCode, GptNeoX) get the check instead of one family owning a private copy.

src/models/bailing_moe.rs, src/models/gpt_neox.rs, src/models/helium.rs

The three duplicated per-family bounds checks are folded into the shared guard and kept only as delegating early diagnostics that name the family and fire during config validation, before any tensor is touched. bailing_moe::validate_quantized_packing becomes a thin WeightMap adapter over the shared function. The tree-wide grep for bits < 1 | bits > 32 | group_size < 1 now matches only layers.rs, down from three model files plus the shared crate.

Notes

bits == 0 is C++ undefined behavior and the targets diverge: on AArch64 the divide returns 0 so std::invalid_argument fires, while on x86-64 the hardware raises SIGFPE and kills the process before any exception exists. Only the AArch64 outcome is reachable from this machine; the x86-64 path is recorded in the doc comment rather than tested.

The regression tests deliberately drive the bad values through the real load path rather than the pure shape helpers, so a regression aborts the test binary with SIGABRT instead of failing cleanly. Every hostile case is paired with a positive control, so no guard can pass by rejecting everything quantized.

Review follow-ups applied

A review of the first commit found two correctness holes in the guards this PR adds, both fixed in the second commit:

  • validate_quantized_packing compared only the scales side against the config width, which is not MLX's test. MLX compares the scales side against w.shape(-1) * 32 / bits, and the block-float reconciler keeps the declared group_size in both of its fallbacks, so a triple could satisfy the config check while the packed weight described a different width and MLX still threw. Both sides are now compared, in i64.
  • group_size was bounded below but not above. A declared i32::MAX survived the block-float fallback and reached quantized_matmul, where MLX multiplies it by the scales width in C++ int: signed overflow, and therefore undefined behavior reached before the shape check that would have thrown. It is now bounded at 2^20, an overflow bound rather than an allowlist, with the arithmetic that justifies it recorded on the constant.

Also from that review: the i32 num_groups * group_size in from_stacked_parts moved to i64 (it wrapped in release and panicked in an overflow-checked build), a zero or absent last axis is now rejected instead of satisfying 0 == 0 and aborting later, the MoE path gained the biases-shape check that is a second validate_quantized_input throw, and several documentation and message-wording items. The weight dtype condition from validate_quantized_input is deliberately still unchecked, with the reason recorded in the doc comment: the in-tree fixtures build packed weights as f32.

Test plan

  • cargo test --release --lib --features metal,accelerate models::switch_layers (21 passed, including the two new MoE guards over both the pre-stacked and per-expert layouts)
  • cargo test --release --lib --features metal,accelerate models::gpt2 (27 passed, including the quantized position-table width guard and the shared-guard load rejection)
  • cargo test --release --lib --features metal,accelerate models::bailing_moe (42 passed, unchanged messages after the hoist)
  • cargo test --release --lib --features metal,accelerate models::gpt_neox (32), models::helium (27), models::gpt_bigcode (20)
  • cargo test --release -p mlxcel-core --lib --features metal,accelerate layers::tests (61 passed, including the four new quantization tests)
  • MoE regression sweep: models::{mixtral,qwen2_moe,olmoe,phimoe,deepseek,dots1,gemma4,minimax} all green
  • cargo clippy --release --lib --tests --features metal,accelerate clean apart from the two warnings already on main in src/multimodal/host_preprocessor.rs and host_preprocessor_export.rs
  • cargo fmt --all -- --check
  • cargo test --release -p mlxcel-core --lib layers::tests re-run after the review fixes (62 passed, including the new weight-side packing test and the group_size upper bound), plus the full model sweep above
  • Real quantized checkpoint load, to confirm normal loading is unaffected. Not run in this environment: the release binaries are not built here and the MoE reference checkpoint is 33.6 GB. Left for the follow-up validation pass. This matters more than usual for the embedding-table change, which converts one class of currently-loading-but-garbage-decoding tables (an affine table packed at group 32 under a config default of 64) into a load error.

Closes #929

An unvalidated `quantization` block in `config.json` reached MLX and terminated the process. `reconcile_quantization_layout` returned a non-positive `group_size` / `bits` pair unchanged on a trust-the-caller basis, and MLX reconstructs a quantized matrix's unpacked width as `w.shape(-1) * 32 / bits` in `validate_quantized_input`, so a declared `"bits": 0` is a division by zero and anything above 32 collapses the quotient to something no real `scales` width can match. `quantized_matmul`, `gather_qmm` and `dequantize` all cross the cxx bridge as `UniquePtr<MlxArray>` rather than `Result`, so the C++ throw is an uncatchable `std::terminate` at the first forward pass rather than a load error: a checkpoint with real `.scales` and `"bits": 0` loaded cleanly and took `mlxcel-server` down on the first request that touched a quantized projection.

`reconcile_quantization_layout` now calls a new shared `validate_quantization_params` before anything else and returns `Err` for `bits` outside `1..=32` and for `group_size < 1`. Degenerate shapes stay permissive; that early return used to fold both conditions together, which is what let an empty shape launder a hostile pair. The guard is a bounds check rather than an allowlist of the widths MLX supports, because mlxcel deliberately re-derives bit width from tensor shapes when the declared value disagrees, and an allowlist would reject the mixed-precision exports that behavior serves.

The reconciler alone does not cover MoE. `SwitchLinear::from_stacked_parts` never calls it, re-derives bits itself and accepts the result only in `2..=8`, and never infers `group_size` at all, so a declared 0 zeroed the denominator, skipped inference, and was stored on `SwitchLinear::Quantized` and handed to `gather_qmm`, which reaches the same `/bits` line through the same `extract_quantized_matmul_dims`. It now bounds the declared pair itself and additionally refuses a stored triple that fails MLX's own predicate, evaluated in i64 so the multiply cannot wrap: the existing fallback to the declared `bits` whenever the shapes solve outside `2..=8` guaranteed a throw. The mixed-precision inference the fallback exists for is untouched, and a non-quantized expert plane is not gated on quantization params at all. The roughly fifteen families carrying a local `SwitchLinear` are unchanged and stay covered in practice, because every one of them loads `embed_tokens` through `UnifiedEmbedding::from_weights` before it builds an expert plane.

Scope item two: `validate_embedding_table` skipped the width check for a quantized table, so a table honestly packed for a different model width kept exactly the right row count, passed load, and then fed a wrong-width hidden state into `fast::layer_norm` / `fast::rms_norm`, which throws the same uncatchable way. The dequantized width is recoverable at load, so the check is restored for the quantized case via `mlxcel_core::layers::validate_quantized_packing`, hoisted verbatim out of `bailing_moe.rs` (messages included) so BailingMoe, Gpt2, GptBigCode and GptNeoX share one reconstruction instead of one family owning a private copy. `bailing_moe::validate_quantized_packing` is now a thin `WeightMap` adapter over it.

The three duplicated per-family bounds checks in `gpt_neox.rs`, `helium.rs` and `bailing_moe.rs` are folded into the shared guard and kept only as delegating early diagnostics that name the family and fire during config validation. The tree-wide grep for the check now matches only `layers.rs`. `quant_mode` is likewise deduplicated into a shared `infer_quantization_mode` used by the linear loader, the embedding loader, and BailingMoe weight validation.

Regression tests drive the bad values through the real load path rather than the pure helpers, so a regression aborts the test binary with SIGABRT instead of failing cleanly: `UnifiedLinear::from_weights` and `UnifiedEmbedding::from_weights` in mlxcel-core, `SwitchLinear::from_weights` on both the pre-stacked and per-expert layouts, and `Gpt2Model::from_weights` for the quantized position table (GPT-2 carries no family-level check, so it exercises the shared guard). Every case pairs with a positive control so a guard cannot pass by rejecting everything quantized.

Note the platform split behind the division: `bits == 0` is C++ undefined behavior, and on AArch64 the divide returns 0 so `std::invalid_argument` fires, while on x86-64 the hardware raises `SIGFPE` and kills the process before any exception exists. Only the AArch64 outcome is reachable from this machine.

Validated with `cargo test --release --lib --features metal,accelerate` over `models::switch_layers`, `models::gpt2`, `models::bailing_moe`, `models::gpt_neox`, `models::helium`, `models::gpt_bigcode`, the MoE families `models::{mixtral,qwen2_moe,olmoe,phimoe,deepseek,dots1,gemma4,minimax}`, and `-p mlxcel-core layers::tests`, plus `cargo clippy --release --lib --tests` and `cargo fmt --all -- --check`. Real-checkpoint validation is not run here.

Closes #929
@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 Jul 28, 2026
`validate_embedding_table` no longer skips the width check for a quantized table, so the two comments in `bailing_moe.rs` and `bailing_moe_tests.rs` that explained why Bailing MoE had to reconstruct the width itself were stale. Both now say what the family-level call is for after the shared guard landed: a redundant early diagnostic that runs during `validate_weights`, ahead of the loader, and names the table rather than the constructor that reached it.

Comment-only; `models::bailing_moe` still passes 42 tests and `cargo fmt --all -- --check` is clean.

Refs #929
Review findings on the first commit of this PR, all in the guards it adds.

The shared `validate_quantized_packing` compared only `scales.shape(-1) * group_size` against the config width. MLX compares that against `w.shape(-1) * 32 / bits`, and the two are not the same test: the block-float branch of `reconcile_quantization_layout` returns the declared `group_size` unchanged in both of its fallbacks (`32 % bits != 0`, and `in_features % num_groups != 0`), so a triple can satisfy the scales-side check while the packed weight describes a different width. Concretely, weight `[rows, 8]` with scales `[rows, 3]` at group_size 32 / 4-bit mxfp4 under a declared width of 96 passed, while MLX evaluates 64 against 96 and throws. The check now mirrors MLX's own predicate on both sides, in i64. The dtype condition from `validate_quantized_input` is deliberately still not checked, and the doc comment says why: the in-tree fixtures build packed weights as f32.

`validate_quantization_params` now bounds `group_size` above as well, at 2^20. Without it a declared `i32::MAX` survives the block-float fallback unchanged and reaches `quantized_matmul`, where MLX multiplies it by the scales width in C++ `int`: signed overflow, so undefined behavior reached before the shape check that would have thrown. The bound is an overflow bound rather than an allowlist, and the constant carries the arithmetic that justifies it. The block-float `packed_in * (32 / bits)` is now a `checked_mul` for symmetry with the affine path.

`SwitchLinear::from_stacked_parts` gained three things: the intermediate `num_groups * group_size` moved to i64, since the previous i32 version wrapped in release and panicked in an overflow-checked build on the line directly under the new guard; a rejection of a zero or absent last axis on either tensor, which used to satisfy every arithmetic check as `0 == 0` and then abort inside `extract_quantized_matmul_dims`; and the `biases` shape check, which is a second `validate_quantized_input` throw that the MoE path could still reach.

Also corrected: `validate_embedding_table` no longer appends the model-width note to rejections that have nothing to do with width, `reconcile_quantization_layout`'s doc list now includes the out-of-range outcome, `QuantizedTensorShapes` derives the same traits as the neighbouring `ReconciledQuant`, and a stale comment in `gpt_neox_tests.rs` that still described the pre-fix trust-the-caller behavior.

Not addressed here, filed as #958: eighteen `gather_qmm` call sites sit behind a family-local quantized expert type that reaches MLX through neither shared loader. The claim in this PR's original body that they stay covered through `UnifiedEmbedding::from_weights` is wrong and has been corrected, because a float embedding with quantized experts (the canonical gpt-oss shape) never touches a guarded loader, gpt_oss resolves params per weight prefix, and mamba / mamba2 build `QuantizedEmbedding` by hand.

Validated with `models::{switch_layers,gpt2,bailing_moe,gpt_neox,helium,gpt_bigcode,dots1,gemma4,mixtral,olmoe,phimoe,deepseek,minimax,llada2_moe}` and `-p mlxcel-core layers::tests` (62 passed), plus clippy and fmt.

Refs #929
@inureyes inureyes added status:done Completed and removed status:review Under review labels Jul 28, 2026
@inureyes
inureyes merged commit 8f94e6a into main Jul 28, 2026
5 checks passed
@inureyes
inureyes deleted the fix/issue-929-quantization-bounds branch July 28, 2026 14:41
inureyes added a commit that referenced this pull request Jul 31, 2026
… loaders (#972)

## Summary

#929 (PR #956) bounded the declared `quantization` pair at the two shared loaders, `reconcile_quantization_layout` and `SwitchLinear::from_stacked_parts`. The families that keep their own quantized expert type never reach either, so a config-derived `"bits": 0` was still stored verbatim and handed to `gather_qmm`, which reaches the same `w.shape(-1) * 32 / bits` division inside `extract_quantized_matmul_dims`. `gather_qmm`, `quantized_matmul` and `dequantize` cross the cxx bridge as `UniquePtr<MlxArray>` rather than `Result`, so the C++ throw is an uncatchable `std::terminate` at the first routed forward pass rather than a load error.

Every one of those call sites is now bounded at load.

## Approach: why the storage point rather than the load boundary

The issue asked me to evaluate whether a single call at each family's load boundary covers the same ground more cheaply. I traced it, and it does not. The bound goes at the point the pair is stored, and that choice is forced rather than stylistic:

- **The load boundary is duplicated in at least a dozen places.** The pipeline stage executors for glm4, glm4_moe_lite, deepseek_v3 and llama4 call `TransformerBlock::from_weights` / `DecoderLayer::from_weights` directly in a loop, never entering the family's `Model::from_weights`. `DeepSeekV32StageModel::from_filtered_weights`, `Qwen3NextStageModel::from_filtered_weights`, `Qwen35StageModel::from_filtered_weights` and `GptOssStageModel::from_filtered_weights` are second in-file boundaries that each re-derive `group_size` / `bits` independently. The VLM text wrappers `glm4v_moe`, `ernie4_5_moe_vl` and `loading/vlm_step3p7.rs` build expert planes through their own chains, and `glm_moe_dsa::to_dsv32_args` feeds a GLM config into the deepseek_v32 loader.
- **Three families build the quantized variant as a bare struct literal from a different module**, which no boundary check can reach at all: `qwen3_vl_moe.rs` builds `qwen3_moe::SwitchLinear::Quantized` directly, `ernie4_5_moe_vl.rs` builds `ernie4_5_moe::SwitchLinear::Quantized` directly, and `nemotron_h.rs` has no named constructor for `QuantizedSwitchLinear` at all.
- **The pair arrives from more than one config type per expert enum**, so bounding the producer would not have covered it either. `qwen3_next::SwitchLinear` is fed by `Qwen3NextConfig`, by `Qwen35Config` through `to_qwen3next_config`, and by a config synthesized from `Qwen3OmniSpeechConfig` in `audio/qwen3_omni_moe/talker.rs`.
- **The cost is roughly equal anyway.** There are about as many load boundaries as storage points, so the boundary version buys nothing in call-site count while requiring a whole-program reachability argument that a new `pub` caller silently invalidates.

Two of those config types also default to `group_size: 0, bits: 0` rather than 64/4 when no `quantization` block was inherited (`Qwen3VLMoeConfig` and `Ernie45MoeVlTextConfig`), which is the hostile pair itself, latent behind the fact that such a checkpoint currently has no `.scales`.

New `switch_layers::validate_expert_quantization_params(prefix, group_size, bits)` delegates to `mlxcel_core::layers::validate_quantization_params` and prefixes the tensor name, so every family reports the same bound with the offending plane named. Only the quantized arms are gated; a bf16 expert plane carries no packing and stays loadable at any declared pair.

## Should `QuantizedEmbedding::new` be fallible? Yes

`QuantizedEmbedding::new` and `QuantizedMultiLinear::new` now return `Result<Self, String>`. They are the only ways to build a quantized layer without going through `from_weights_with_mode`, and therefore without the reconciler and the bound it carries. A caller that hand-builds one is exactly the caller whose declared pair has never been looked at.

That is also why `mamba` / `mamba2` needed only a `?` rather than being routed through `from_weights_with_mode`. Routing them would not have been behavior-preserving: both take the table out of the map by `remove`, under either the `backbone.embeddings` or the `model.embed_tokens` spelling, so neither can address a single-prefix map loader; `from_weights_with_mode` additionally reconciles against the tensor shapes and stores the reconciled pair rather than the declared one, treats `.biases` as optional where mamba requires both, and would need the `.or_else()` prefix fallback re-expressed at the call site. Making `new` fallible fixes both families with one line each *and* closes the door for the next family, which routing them would not have done. `QuantizedMultiLinear::new` had no callers at all, so making it fallible was free and pre-emptive.

## Verified inventory versus the issue's scope list

The issue's list is accurate except for one entry, and it misses four call sites.

**Confirmed as listed** (17): `qwen3_moe`, `qwen3_vl_moe`, `deepseek`, `deepseek_v2`, `deepseek_v3`, `deepseek_v32`, `glm4_moe`, `glm4_moe_lite`, `ernie4_5_moe`, `exaone_moe`, `hunyuan_moe`, `llama4`, `nemotron_h`, `qwen3_next` (and `qwen3_5` through it), `step3p5`, `gpt_oss`, plus `mamba` / `mamba2`.

**Does not match**: `jamba`. Its `SwitchLinear` enum is `#[allow(dead_code)]` and the `Quantized` variant is never constructed anywhere in the tree; only `Regular` is built, at three sites. There is no constructor to bound and no reachable `gather_qmm`. Left unchanged and recorded here rather than given a guard with no call site.

**Missed by the issue** (4), all the same defect and all fixed here:

- `mlxcel_core::layers::QuantizedMultiLinear::from_weights` is a **shared** loader in mlxcel-core that stores the declared pair verbatim and hands it to `quantized_matmul` on every MLA forward, without ever calling the reconciler. It sits in exactly the blind spot `from_stacked_parts` occupied before #929, and it is reached by DeepSeek V3, DeepSeek V3.2, GLM4 MoE Lite and LongCat Flash NGram through their `embed_q` / `unembed_out`. This is the highest-value item in the diff: one fix covers four families that #956 and this issue both missed.
- `src/models/ernie4_5_moe_vl.rs` has its own `load_switch_linear` that does not call the `ernie4_5_moe` constructor the issue lists, so fixing that family alone would have left the VLM sibling open.
- `src/models/kimi_linear.rs` keeps a private `MultiLinear` (distinct from the mlxcel-core one) that feeds `quantized_matmul` directly.
- `longcat_flash_ngram::sanitize_weights` and `youtu_vl_lm_sanitize::sanitize_text_weights` derive a pair from `kv_lora_rank` and a tensor axis during MLA `kv_b_proj` decomposition and hand it to `dequantize`. Same uncatchable abort, during weight sanitization rather than at first forward. longcat's sanitizer becomes fallible for the check, a two-caller cascade.

## The gpt-oss case is real but not for the reason the issue gives

The issue says a hostile pair scoped to the expert prefix leaves the global defaults valid. The code is the mirror image of that: `MLPBlock::from_weights` reads `Quantization::defaults()` for the experts, **not** `quant_for`, so a per-prefix override on the expert prefix never reaches them. The gap is the other direction, and it is worse. The canonical `gpt-oss-20b-mxfp4` config carries valid per-prefix overrides for `model.embed_tokens`, the four attention projections and the router, with the mxfp4 expert parameters living in the top-level defaults. So a hostile top-level pair reaches the experts alone while every reconciler-guarded loader in the model sees a good per-prefix pair. Verified against the real config in `models/gpt-oss-20b-mxfp4/config.json`.

Handled two ways: the authoritative bound at `ExpertLinear::from_weights`, plus a family-level early diagnostic `ModelArgs::validate_quantization` that walks the top-level pair and every per-prefix override in one pass at both gpt_oss load boundaries. The test covers both directions.

## On scope: the issue's Scope section is incomplete, not wrong

Every family the issue names is real and every one except jamba got a guard. What the Scope section does not capture is that the same defect exists on a second axis it never looks at: the MLA `kv_b_proj` decomposition. Five sanitizers (DeepSeek V3, DeepSeek V3.2, KimiLinear, LongCat Flash NGram, Youtu-VL) each carry a byte-identical copy of arithmetic that solves a quantization pair from `kv_lora_rank` and a tensor axis and hands it straight to `dequantize`. That is the same uncatchable-abort class from the same untrusted `config.json`, differing only in that the pair is derived rather than declared.

It belongs in this PR rather than a follow-up for three reasons. First, four of the five files were already being modified here for their expert loaders, so splitting would have meant two PRs touching the same functions. Second, the first commit's guard on two of those sites was placed after the division it needed to protect, which is a defect this PR introduced and therefore has to fix. Third, the fix is a single shared helper that all five call, so landing it piecemeal would have left copies diverging mid-series.

What was deliberately NOT folded in is filed rather than left as an observation: #973 (the `mode` string is bounded nowhere and reaches `gather_qmm` as an unvalidated string, flagged independently by both reviewers), #974 (Jamba never builds a quantized expert plane at all, which is why the issue named it but there was nothing to guard), #975 (`deepseek::ModelArgs` silently drops an inherited `quantization` block), and #976 (mamba / mamba2 treat a scales-without-biases embedding as non-quantized).

## Review follow-ups applied

Two review passes over the first two commits found four things, all fixed in the third and fourth commits:

- **Three more MLA `kv_b_proj` sites had the identical defect and were missed.** `DeepSeekV3Model::sanitize_weights`, `DeepSeekV32Model::sanitize_weights` and `KimiLinearModel::sanitize_weights` each solve the packed pair from `kv_lora_rank` and a tensor axis and hand it to `dequantize`, exactly like the LongCat and Youtu sanitizers the first commit converted. There were five copies of that arithmetic, not two. All five now share `infer_mla_quantization_params`, and the three new ones cascade fallibility through `sanitize_weights_with_args` into `loading/vlm_kimi_vl.rs`, `loading/special.rs`, `models/glm_moe_dsa.rs` and the deepseek_v3 and glm_moe_dsa pipeline stage executors.
- **The bound was placed after the division it needed to protect.** `inferred_bits = (packed_in * 32) / kv_lora_rank` divides by a `config.json` field and `inferred_gs = kv_lora_rank / num_groups` by a checkpoint axis. Rust integer division by zero panics, so a `kv_lora_rank` of 0 or a zero-length scales axis took the process down before the guard could reject anything, and `packed_in * 32` wraps `i32` in release. The shared helper now checks each divisor before dividing, evaluates the multiply in `i64`, rejects a rank-0 tensor instead of indexing out of bounds, and additionally verifies the solved pair against MLX's own predicate, because both divisions truncate and an in-range pair is not yet a describable one.
- **`FusedQKVLinear::from_weights_separate_with_mode` is another unguarded shared loader.** It never calls the reconciler, and `infer_quantization_bits` early-returns the caller's `bits` unchanged whenever `group_size <= 0`, so a declared `(0, 0)` was stored verbatim on the fused `QuantizedWeight`. Shadowed in every current family by a sibling `UnifiedLinear` that sees the same pair and rejects it first, so this was not a live hole, but it is the same shape as the one #929 left open and it is one line.
- **The tests' SIGABRT claim was false and is corrected.** The comment in sixteen files said a regression would take the test binary down with SIGABRT. That is what happens in production; these tests assert on the load result and never run a forward pass, so a regression fails cleanly at the assertion. The wording would have misled whoever next debugs a failure here. The `QuantizedEmbedding::new` doc made a similar overclaim (the fallible signature does not make the bound impossible to skip, because the fields are `pub`) and now says so.

Also from review: the `MultiLinear` enum `Used by:` list gained LongCat Flash NGram, and `FusedQKVLinear::from_weights_separate_with_mode` had a stale list naming a Phi3 caller that does not exist while omitting eleven that do. Both corrected against grep.

A finalizer pass then found two more things, fixed in the sixth commit:

- **The five MLA sanitizers were covered only by the pure-helper unit test**, which contradicts this PR's own stated test philosophy. All five now have a test driving the real `sanitize_weights` / `sanitize_text_weights`. LongCat Flash NGram had no `#[cfg(test)]` block at all before this, and DeepSeek V3's inline module had no quantized-MLA case. One trap is worth recording: unlike the expert-plane tests, which stop at the loader's `Result`, these drive a sanitizer that actually evaluates the dequantize, so the packed fixture must be `UINT32`. A float fixture aborts the whole test binary with SIGABRT on the positive control, which is what the first draft of all five did and what running them caught. The reason is now recorded at each fixture.
- **`docs/adding-models.md` never mentioned the convention**, so the next family ported with a local quantized expert type would silently reintroduce the bug. It gains a `Quantization Parameter Bounds` section stating the rule and why the load boundary does not dominate, a table of what already carries the bound so a porter inherits it for free, and a checklist item pointing at it.

Both reviewers independently flagged that `quantization.mode` is bounded nowhere and reaches `gather_qmm` as an unvalidated string, which is the same abort class. That is a different config field and a different fix (an allowlist, which already exists at `gemma4::SUPPORTED_QUANT_SCHEMES` but is wired only into gemma4), so it is filed as a follow-up rather than folded in here.

## Test plan

Both acceptance criteria on testing hold, and the second one needs a correction to how the issue phrases it.

**Real load path, not a pure helper.** Every test calls the family's own loader (`SwitchLinear::from_weights`, `load_switch_linear`, `ExpertLinear::from_weights`, `Step3p5SwitchGLU::from_weights`, `MultiLinear::from_weights`) over a synthetic `WeightMap` carrying an honestly packed expert plane, so the guard is exercised at the point a real checkpoint reaches it. Four go further and drive the whole model constructor: `MambaModel::from_weights`, `Mamba2Model::from_weights` and `NemotronHModel::from_weights` (the last including its C++ model registration), plus the gpt_oss config walk over the real gpt-oss-20b-mxfp4 quantization shape.

**Positive control first, always.** Each test loads the honest pair before it tries any hostile one, and every test whose loader has a non-quantized branch additionally asserts that a bf16 plane with no `.scales` still loads under a hostile pair. A guard that rejected everything quantized, or that gated the float path, fails on those two assertions before it reaches the hostile loop.

The issue expects a regression to surface as a SIGABRT from the test binary. It does not, and that is better rather than worse: these tests assert on the load `Result` and never run a forward pass, so a lost guard fails cleanly at the assertion instead of aborting. SIGABRT is what happens in production, which is the thing being prevented. The doc comments that repeated the issue's phrasing were corrected in the fourth commit, because they would have misled whoever next debugs a failure here.

- [x] `cargo test --release --lib --features metal,accelerate quantization_params` (19 passed: the 18 new per-family guards plus the existing switch_layers one)
- [x] `cargo test --release --lib --features metal,accelerate models::gpt_oss` (3 passed, including the per-prefix config walk over the real gpt-oss-20b-mxfp4 quantization shape)
- [x] `cargo test --release --lib --features metal,accelerate models::ernie4_5_moe_vl` (5), `models::mamba` (8), `models::nemotron_h` (19)
- [x] `cargo test --release -p mlxcel-core --lib --features metal,accelerate layers::tests` (65 passed, including the two tests for the fallible by-hand constructors and the shared MultiLinear loader, plus the MLA divisor and overflow test)
- [x] Affected-family sweep, all green: `deepseek` (28), `deepseek_v2` (1), `deepseek_v3` (24), `deepseek_v32` (12), `glm4_moe` (7), `glm4_moe_lite` (1), `ernie4_5_moe` (6), `exaone_moe` (3), `hunyuan_moe` (1), `jamba` (4), `llama4` (1), `qwen3_next` (8), `qwen3_5` (5), `step3p5` (12), `qwen3_moe` (1), `qwen3_vl_moe` (2), `kimi_linear` (1), `longcat_flash_ngram`, `youtu_vl_lm` (3)
- [x] MoE regression sweep for the families this does NOT change, to confirm the shared-helper additions are inert for them: `switch_layers` (22), `mixtral`, `qwen2_moe`, `olmoe`, `phimoe` (5), `dots1` (9), `gemma4` (90), `minimax` (15), `bailing_moe` (42)
- [x] `cargo test --release --lib --features metal,accelerate kv_b_proj` (5 passed: one per MLA sanitizer, each driving the real `sanitize_weights` / `sanitize_text_weights` rather than the shared helper)
- [x] `cargo test --release --lib --features metal,accelerate models::` (774 passed, 0 failed), `loading::` (220), `distributed::` (1268)
- [x] Anti-vacuity check, run empirically rather than argued: the guard in `qwen3_moe::SwitchLinear::from_weights` was temporarily replaced with `let _ = validate_expert_quantization_params(..)` and the suite re-run. The test failed on exactly the intended assertion (`(group_size 64, bits 0) must be refused at load, not stored for gather_qmm` at `qwen3_moe_tests.rs:86`) rather than passing for some other reason, and passed again once the guard was restored. No hostile pair in `HOSTILE_QUANT_PARAMS` is rejected by any pre-existing check, so these tests detect a regression rather than merely accompanying one.
- [x] `cargo clippy --release --lib --tests --features metal,accelerate` clean
- [x] `cargo fmt --all -- --check`
- [ ] Real quantized checkpoint load, to confirm normal loading is unaffected. Not run here: the release binaries are not built in this session and the reference MoE checkpoints are large. Left for the follow-up validation pass, which should cover `gpt-oss-20b-mxfp4` (the per-prefix case), `deepseek-v2-lite-4bit` (the shared `QuantizedMultiLinear` MLA path), and one of `qwen3-30b-a3b-4bit` / `mixtral-8x7b-4bit` / `nemotron-h-30b-4bit`.

The pre-existing `multimodal::host_preprocessor::tests::xla_loader_keeps_text_and_unqualified_vlm_image_capability_false` failure on `main` is tracked as #966 and is unrelated to this change.

## Notes

Two adjacent findings turned up during the trace that are **not** fixed here, because both are separate defects rather than this one. Both are filed rather than left as observations, as #975 and #976 respectively:

- `deepseek::ModelArgs` has no `quantization` field, only flat top-level `group_size` / `bits`. The three DeepSeek-OCR loaders in `loading/vlm_deepseekocr.rs` inject the root `quantization` object into `language_config` before deserializing, so serde drops it and `group_size()` / `bits()` silently fall back to 64/4 regardless of what the checkpoint declares. `DeepSeekModel::load` has the same shape. The contrast is `vlm_deepseek_vl2.rs`, which does the identical inheritance into `deepseek_v2::ModelArgs` and works because that config has the nested field.
- `mamba` / `mamba2` treat an embedding table with `.scales` but no `.biases` as non-quantized and fall through to `Embedding::new`, which would read packed uint32 data as a float table. Only reachable for a block-float embedding export, which no current mamba checkpoint ships.

Closes #958
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(core): an unvalidated config.json quantization block reaches MLX and terminates the process

1 participant