Skip to content

fix: recognize n_layer / n_embd and multi_query in KV estimation - #957

Merged
inureyes merged 1 commit into
mainfrom
fix/issue-927-kv-arch-openai-aliases
Jul 30, 2026
Merged

fix: recognize n_layer / n_embd and multi_query in KV estimation#957
inureyes merged 1 commit into
mainfrom
fix/issue-927-kv-arch-openai-aliases

Conversation

@inureyes

Copy link
Copy Markdown
Member

Summary

mlxcel inspect reported KV cache: 0 bytes (unavailable (config.json missing architecture fields)) and Activation: 0 bytes for GPT-2 and GPT-BigCode, because the estimators never aliased the OpenAI-era n_layer and n_embd field names. On the server path the same None propagated through paged_block_bytes to src/server/model_worker.rs, leaving the paged pool with no --kv-cache-budget auto admission ceiling. This teaches the estimators both spellings, adds the missing multi_query branch that the alias fix alone would have gotten wrong, and consolidates the five duplicated alias lists behind one module.

The trap this avoids

Adding the two aliases on their own would have been worse than the zero it replaced. Every other family expresses grouped attention through a numeric num_key_value_heads, but GPT-BigCode signals it with the boolean multi_query: true, meaning exactly one shared kv head. The kv-head resolution read only numeric fields and otherwise fell back to num_kv_heads = num_heads, while GptBigCodeArgs::n_kv_heads (src/models/gpt_bigcode.rs) returns if multi_query { 1 } else { n_head } and the runtime caches accordingly.

kv heads used Total at ctx 8192, fp16
Before (no estimate) 0 bytes
Alias-only "fix" 16 1,610,612,736 (1.50 GiB)
This PR 1 100,663,296 (96 MiB)

The over-reservation scales with n_head, so it is 48x for a StarCoder-sized checkpoint, not 16x. resolve_num_kv_heads now honors the boolean, ordered so an explicit numeric field still wins when a config carries both; multi_query: false and an absent flag both keep the existing MHA behavior.

The duplication

The same alias lists were copied into five independent .or_else() chains, which is exactly why the gap reproduced in every one of them, and why a fix scoped to kv_arch.rs alone would have left inspect still printing Activation: 0 bytes. Rather than adding a sixth copy, new src/execution/config_fields.rs owns the lists and the kv-head resolution once, and all five call sites read from it. Legacy spellings are appended last in each list, so a config that already resolved cannot resolve differently.

What changed

  • src/execution/config_fields.rs (new): the alias constants (LAYER_COUNT_KEYS, HIDDEN_SIZE_KEYS, NUM_HEADS_KEYS, NUM_KV_HEADS_KEYS, HEAD_DIM_KEYS, INTERMEDIATE_SIZE_KEYS), the text_config / get_u64 / get_f64 helpers, and resolve_num_kv_heads, which implements the numeric-then-boolean-then-MHA precedence.
  • src/execution/kv_arch.rs: classify and attn_dims (and the MLA head-count lookup) read the shared constants; attn_dims resolves kv heads through resolve_num_kv_heads.
  • src/execution/memory_estimate.rs: activation_dims_from_path and kv_cache_params_from_path both read the shared constants; the latter also drops its duplicated kv-head fallback in favor of the shared resolution, so it cannot disagree with the classifier.
  • src/execution/kv_cache_advisor.rs: read_head_dim reads the shared constants, making good on the promise its own doc comment already made. Its None-when-head-count-absent semantics are unchanged, so the permissive Turbo default is untouched for configs that genuinely lack a head count.
  • src/execution/quant_advisor.rs: estimate_params_from_config was a fifth copy of the same lists; it now reads the shared constants and honors multi_query in its K/V projection term.
  • src/execution/mod.rs: declares the new module.

Before and after, real checkpoints

Model KV before KV after Activation before Activation after
models/gpt2 0 (unavailable) 301,989,888 0 7,964,834
models/gpt_bigcode-santacoder 0 (unavailable) 100,663,296 0 21,070,080

Both now report KvSource::Config with standard attention (N layers, full context), and both produce a non-empty KV-cache-mode advice vector (3 entries) where they previously produced none.

No regression

The three families the issue named as regression targets are byte-identical, confirmed against the real checkpoints:

Model KV at ctx 8192 fp16
models/pythia-1b 1,073,741,824 (unchanged)
models/helium-1-preview-2b-4bit 2,013,265,920 (unchanged)
models/ling-lite-1.5 469,762,048 (unchanged, verified config-only via estimate_kv_arch, no weights loaded)

Test plan

  • cargo check --release --lib --tests --features metal,accelerate clean; only the two warnings already on main in src/multimodal/host_preprocessor.rs and host_preprocessor_export.rs.
  • cargo clippy --release --lib --tests --features metal,accelerate produces nothing new; same two pre-existing warnings.
  • cargo test --release --lib --features metal,accelerate execution:: passes 112, up from 93, with 19 new cases.
  • cargo test --release --lib --features metal,accelerate models::gpt passes 78.
  • cargo test --release --lib --features metal,accelerate budget passes 79 and quant passes 141.
  • cargo fmt --all -- --check clean.
  • Baseline reproduced with the pre-change binary: mlxcel inspect on models/gpt2 and models/gpt_bigcode-santacoder both showed 0 KV and 0 activation, matching the issue exactly.
  • Post-change figures measured against the real checkpoints through estimate_total_memory and advise_kv_cache_modes, the same entry points mlxcel inspect calls. The release binary itself was not rebuilt here (that is the orchestrator's step), so the arithmetic is additionally pinned in unit tests using the verbatim config fields from both checkpoints.

New coverage: GPT-2 style naming classifying as standard MHA, GPT-BigCode multi_query: true yielding one kv head, multi_query: false degenerating to n_head, the 48x StarCoder-sized case, numeric fields outranking the boolean, a non-boolean multi_query value being ignored, the degenerate multi_query: true with n_head: 0, kv_cache_params_from_path agreeing with the classifier, paged_block_bytes resolving instead of returning None, the advisor returning advice for both families, and the three no-regression figures.

Closes #927

KV cache and activation estimation did not accept the OpenAI-era config field names that GPT-2 and GPT-BigCode use. The layer-count lookup in `classify` aliased `num_hidden_layers` / `n_layers` / `num_layers` but not `n_layer`, and the hidden-size lookup in `attn_dims` aliased `hidden_size` / `d_model` / `dim` / `model_dim` but not `n_embd`. Both families therefore short-circuited to `KvSource::Unavailable`, so `mlxcel inspect` printed `KV cache: 0 bytes (unavailable (config.json missing architecture fields))` and `Activation: 0 bytes`, and on the server path the `None` propagated through `paged_block_bytes` and left the paged pool with no `--kv-cache-budget auto` admission ceiling.

Adding those two aliases alone would have been worse than the zero it replaced. Every other family expresses grouped attention through a numeric `num_key_value_heads`, but GPT-BigCode signals it with the boolean `multi_query: true`, meaning exactly one shared kv head, and the kv-head resolution read only numeric fields before falling back to `num_kv_heads = num_heads`. An alias-only change would have claimed `n_head` kv heads where `GptBigCodeArgs::n_kv_heads` caches one: 1.50 GiB against a correct 96 MiB at ctx 8192 fp16 for santacoder, and 48x for a StarCoder-sized checkpoint. `resolve_num_kv_heads` now honors the boolean, ordered so an explicit numeric field still wins when a config carries both, and `multi_query: false` or an absent flag keeps the MHA fallback.

The same alias lists were copied into five independent `.or_else()` chains, which is why the gap reproduced in all of them and why a fix scoped to `kv_arch.rs` would have left `inspect` still printing a zero activation reserve. New `src/execution/config_fields.rs` owns the lists and the kv-head resolution once; `kv_arch::classify` / `attn_dims`, `memory_estimate::activation_dims_from_path` / `kv_cache_params_from_path`, `kv_cache_advisor::read_head_dim`, and `quant_advisor::estimate_params_from_config` all read from it. Legacy spellings are appended last in each list, so a config that already resolved cannot resolve differently.

Verified against real checkpoints. `models/gpt2` moves from 0 to 301,989,888 KV bytes plus a 7,964,834-byte activation reserve, `models/gpt_bigcode-santacoder` from 0 to 100,663,296 plus 21,070,080, both now sourced from the config and both emitting a non-empty KV-cache-mode advice vector. The three families checked for regression are byte-identical: `models/pythia-1b` 1,073,741,824, `models/helium-1-preview-2b-4bit` 2,013,265,920, `models/ling-lite-1.5` 469,762,048.

Closes #927
@inureyes inureyes added status:review Under review type:bug Bug fixes, error corrections, or issue resolutions priority:low Low priority area:core mlxcel-core: MLX FFI, primitives, KV cache, layers status:done Completed and removed status:review Under review labels Jul 28, 2026
@inureyes
inureyes merged commit 94782d8 into main Jul 30, 2026
5 checks passed
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 priority:low Low 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: KV estimation misses n_layer / n_embd for GPT-2 and GPT-BigCode

1 participant