Skip to content

refactor(loader): gate batched-prefill MLA exclusion on capability, not model_type#285

Open
rsafier wants to merge 1 commit into
fix/generic-nvfp4-moe-gatefrom
refactor/mla-guard-capability
Open

refactor(loader): gate batched-prefill MLA exclusion on capability, not model_type#285
rsafier wants to merge 1 commit into
fix/generic-nvfp4-moe-gatefrom
refactor/mla-guard-capability

Conversation

@rsafier

@rsafier rsafier commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Re-opened replacement for #261. #261 was merged into its base feature branch by mistake on 2026-07-09 and reverted; the base branch has since been restored and restacked onto main. GitHub cannot reopen a merged PR, so this is a fresh PR carrying the identical commit (verified patch-identical via git range-diff). Marked ready for review — the original's "draft" wording was stale.

Stacked on #260 (base fix/generic-nvfp4-moe-gate). Unit tests green; the e2e co-dispatch smoke is still outstanding.

What

check_kernel_batched_eligible() blocked batched-prefill attention for MLA models via config.model_type == "mistral" — a hardcoded proxy the original author flagged as conservative ("mistral is the only MLA model in Atlas today"). This keys it on the actual capability: config.kv_lora_rank > 0 (== AttentionType::Mla).

Behavior preservation

kv_lora_rank > 0 == {mistral, deepseek_v4}; every other model_type has kv_lora_rank == 0is_mla=false → unchanged. deepseek_v4 is the only newly-matched model, but it is already rejected one check later by head_dim > 256 (head_dim=512) — so its outcome (not batched-eligible) is identical. Net: bit-identical on every shipped model, plus it future-proofs any MLA checkpoint shipped with head_dim <= 256. batch_kernel_tests pass (12/12) — they directly exercise the converted predicate (MLA reject, head_dim reject, non-MLA accept).

Why this is the only additional conversion

This PR is the output of an exhaustive adversarial pass over every model_type string gate in the tree (stacking on #260, which already did the two qwen35/load_layers.rs MoE gates). The analysis found this is the only other safe architecture-generic conversion. In particular it rejected several tempting ones:

  • MTP allowlist (preflight.rs MTP_SUPPORTED_MODEL_TYPES) → must NOT become capabilities().has_mtp: has_mtp (mtp_num_hidden_layers>0) is broader than the allowlist and is true for deepseek_v4 / minimax_m2 / step3p7 — converting would re-enable exactly the unconsumable per-module-MTP loader path (MiniMax) the guard exists to reject.
  • is_qwen35* predicates → loader sub-dispatch (protected; no capability expresses "use dense vs VL loader").
  • gemma4 skip_lm_head_quantization / softcap, minimax/step3p7 setup, the dispatch.rs model_type→kernel-dir rewrites → genuinely checkpoint/family-specific, kept.

No kernels move; no loader dispatch changes. The refactor is deliberately minimal — the value is as much the documented boundary of what stays model_type-gated as the one conversion itself.

@rsafier rsafier marked this pull request as ready for review July 10, 2026 00:48
@SeedSource

Copy link
Copy Markdown
Collaborator

[BETA] Automated PR review — SeedSource AI pipeline

Advisory only, not a merge gate. AI-generated (internal risk-triage, Atlas serve gate, GLM-5.2 review), attributed per Atlas convention.

Triage

  • Risk: low - serve-test-required (triage advisory): False - path-heuristic (authority): True
  • Rationale: Refactoring MLA eligibility check from string model type to boolean capability flag; no quantization or serving changes.
  • Serving-path files: crates/spark-model/src/model/trait_impl/prefill_b/batch_kernel/eligible.rs, crates/spark-model/src/model/trait_impl/prefill_b/batch_kernel_tests.rs

Serve gate

  • Build: Nones - Qwen3.6-35B-A3B-FP8 on GB10 node1
  • Top-k KL vs main: None - decode delta None%
  • Coherence probe: None (note: our test_coherence.py is flaky here; KL is the reliable serve signal)

GLM-5.2 code review

Correctness: Sound. kv_lora_rank > 0 is the canonical MLA marker and correctly supersedes the brittle model_type == "mistral" string match. The comment accurately notes deepseek_v4 was already caught by the subsequent head_dim > 256 guard, so no behavioral regression on shipped models. Parameter type change from &str to bool is clean and all call sites are updated.

Risk: Low. The refactor is narrowly scoped to one eligibility gate. The only latent risk: if a future MLA model has kv_lora_rank > 0 and head_dim <= 256, it would now be rejected earlier (correct) rather than slipping through the string check (bug). That's a strict improvement.

Quality concerns:

  1. The comment block (lines 160-165) is verbose for a hot-path file. Trim to 2 lines.
  2. No test exercises a non-mistral MLA model (the stated motivation). Add one with is_mla=true, head_dim=128 to lock in the fix.
  3. Tests pass false for non-MLA cases but never assert the kv_lora_rank derivation logic itself—consider a unit test on the caller side.

Verdict: Approve with the test gap addressed.

Pipeline: build+serve on GB10, GLM-5.2 via NVIDIA NIM, internal risk-triage classifier. External diff treated as untrusted input. Public repo.

@rsafier rsafier force-pushed the fix/generic-nvfp4-moe-gate branch from cb79d49 to fdc6bba Compare July 12, 2026 22:46
@rsafier rsafier force-pushed the refactor/mla-guard-capability branch from dbec4e6 to 63ca929 Compare July 12, 2026 23:30
@rsafier rsafier force-pushed the fix/generic-nvfp4-moe-gate branch from fdc6bba to 6a48210 Compare July 13, 2026 00:43
…ot model_type

check_kernel_batched_eligible() blocked batched-prefill attention for MLA models
via `config.model_type == "mistral"` — a hardcoded proxy the author flagged as
conservative ("mistral is the only MLA model in Atlas today"). Key it on the
actual MLA capability instead: `config.kv_lora_rank > 0` (== AttentionType::Mla).

- signature: `model_type: &str` -> `is_mla: bool`; wrapper passes
  `self.config.kv_lora_rank > 0` (the exact derivation of
  capabilities().attention_type == Mla; read directly to avoid rebuilding the
  full ModelCapabilities struct on this per-prefill-batch hot path).
- tests updated to pass the bool.

Behavior-preserving on every shipped model: kv_lora_rank>0 == {mistral,
deepseek_v4}; all other model_types have kv_lora_rank=0 -> is_mla=false ->
unchanged. deepseek_v4 is the only newly-matched model, but it is ALREADY
rejected one check later by `head_dim > 256` (head_dim=512), so its outcome
(not batched-eligible) is identical. Also future-proofs any MLA checkpoint
shipped with head_dim<=256. Unit tests (batch_kernel_tests) pass.

Stacked on #260 (fix/generic-nvfp4-moe-gate). NOTE: an exhaustive adversarial
pass over the OTHER model_type gates found this is the ONLY additional safe
architecture-generic conversion — notably the MTP allowlist (preflight.rs) must
NOT be converted to has_mtp (broader than intent: would re-enable the
MiniMax/DeepSeek per-module-MTP path the guard rejects). See PR description.
@rsafier rsafier force-pushed the refactor/mla-guard-capability branch from 63ca929 to 9343daf Compare July 13, 2026 00:44
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.

2 participants