feat(model): E2 DeepSeek family split + config fields (#109)#124
Conversation
Correct A1 interim debt that collapsed every DeepSeek HF model_type onto MODEL_DEEPSEEK_V4. Land the model_config_t surface E2 forward/sanitize needs, without enabling generate. - model_family_t: MODEL_DEEPSEEK_V3, MODEL_DEEPSEEK_V32, MODEL_DEEPSEEK_V4 - model_family_from_type: deepseek_v3 / deepseek_v3_2+deepseek_v32 / deepseek_v4 - Parse MLA, MoE extras, rope mscale_all_dim, V3.2 indexer, norm_topk_prob - n_routed_experts aliases into num_experts only when num_experts == 0 - weights_load rejects all three with honest family labels (no GGUF) - Gate still rejects all DeepSeek families as unsupported
Code review: feat(model): E2 DeepSeek family split + config fields (#124)Thorough review of the full PR against #109, decision 11, and SummaryThis is a clean, plan-faithful landing of the E2 prerequisite surface. Enum grain, mapping, parse fields, reject honesty, and test coverage all match decision 11 / R1-R10. No generate enablement, no sanitize/forward creep. I ran No blockers. A few lows/nits below; none need to gate merge unless you want the docs/test tighten-ups in-tree. Positive observations
Issues found1. (Low)
|
| field | mlxd absent | mlx-lm default |
|---|---|---|
n_group |
0 | 1 |
topk_group |
0 | 1 |
moe_layer_freq |
0 | 1 |
first_k_dense_replace |
0 | 0 (ok) |
R7 intentionally leaves these at 0 so unset is distinguishable - good for parse honesty. But #113 / fwd_moe_route_deepseek reject n_group < 1. A caller that treats the parsed struct as "ready to route" will fail closed on any config that omitted the keys (minimal fixtures, partial exports).
Suggestion: mirror the mscale comment on the MoE block, e.g.
int n_group; /* 0 = absent; mlx-lm default 1. NOT mamba_n_groups */
int topk_group; /* 0 = absent; mlx-lm default 1 */
int moe_layer_freq; /* 0 = absent; mlx-lm default 1 */No behavior change required in this PR.
2. (Low) DeepSeek-oriented defaults are applied globally
/* DeepSeek mlx-lm defaults (honored when keys absent; explicit values win). */
cfg->routed_scaling_factor = 1.0f;
cfg->norm_topk_prob = true;Per plan cycle 5c this lives in the universal non-zero-defaults block, so every family load (llama, qwen3, bert, ...) now reports norm_topk_prob == true and routed_scaling_factor == 1.0f even when the checkpoint has no MoE surface.
Functionally harmless today (nothing reads these outside future DeepSeek wiring), but it weakens "memset/load dump" as a signal of what the config actually is, and the zero-init test has to special-case "pre-default vs post-load".
Optional tighten (non-blocking): move the two assignments into the DeepSeek apply_family_defaults arm so non-DeepSeek configs keep zero/false. If you keep global defaults, the comment is fine - just know dumps will lie a little.
3. (Low) No coverage that explicit norm_topk_prob: false wins
Tests cover:
- default-when-absent ->
true(model_config_deepseek_v3_defaults) - explicit
truein V3/V32 fixtures
Missing: a fixture or inline case with "norm_topk_prob": false asserting the bool stays false. get_bool should honor it, but this is the one default that is not zero and is easy to regress (e.g. someone "helpfully" forces true for DeepSeek). Cheap add if you touch the file again.
4. (Nit) test_deepseek_config_fields_zero_init is now tautological
memset(&cfg, 0, sizeof(cfg));
assert(cfg.q_lora_rank == 0);
/* ... */Useful as the Cycle 5a RED compile probe for missing struct members; once the fields exist it only re-proves memset. Consider dropping it, or replacing with a post-load "non-DeepSeek family leaves DeepSeek fields at defaults/zero" assertion (which would also lock in finding 2's chosen policy).
5. (Nit) DeepSeek weights_load reject lives only in the GPU test binary
The early family reject runs before shard I/O / mlx graph work:
if (cfg->family == MODEL_DEEPSEEK_V3 ||
cfg->family == MODEL_DEEPSEEK_V32 ||
cfg->family == MODEL_DEEPSEEK_V4) {
/* ... snprintf ... */
return -1;
}Coverage is only in test_weights_gpu.c. Plan cycle 4 called this out as optional either-way. A CPU twin in test_weights.c (if link graph allows) would keep the decision-11 reject contract on make test without Metal. Not worth restructuring the Makefile for; flagging only because the GGUF debt lived in the GPU file and bit-rotted once already.
6. (Info) User-facing load still surfaces the generic gate string
engine_model_load order is config -> engine_model_check_supported -> weights_load. DeepSeek hits the gate default::
unsupported model family (only qwen3/llama/gemma4/qwen3_5/mistral dense supported)
The honest "deepseek_v3 MLA/MoE forward not yet implemented (Stage E2)" is only visible on direct weights_load (tests / future callers). This matches R9 and is correct for this PR. When #113 (or whichever issue enables generate) opens a gate arm, prefer carrying the family-named Stage-E2 message up to the gate so mlxd run / HTTP errors stay as honest as the weight path.
Spec / exit checklist cross-check
| Exit item | Status |
|---|---|
| V3 / V32 / V4 contiguous in enum | yes |
Mapping + unknown deepseek* |
yes + negative tests |
model_config_load family for all three + deepseek_v3_2 alias |
yes |
| Reject: three labels, no GGUF, no cross-family mislabel | yes (strncmp prefix) |
| MLA / MoE / mscale / indexer parse | yes |
n_routed_experts alias only when num_experts == 0 |
yes |
norm_topk_prob default true |
yes |
| Gate still rejects all DeepSeek | yes |
| No forward/sanitize/enablement creep | yes |
| CLAUDE.md verified | yes (no edit) |
Verdict
Approve with optional follow-ups on (1) absent-contract comments and (3) explicit norm_topk_prob: false test. Nothing here should block stacking #110/#111/#112 on top of this surface.
Nice work clearing the A1 interim collapse cleanly - this is the right grain for the rest of E2.
…s, CPU reject Scope norm_topk_prob / routed_scaling_factor mlx-lm defaults to the DeepSeek apply_family_defaults arm (absent-or-null only) so non-DeepSeek loads keep those fields at unset. Document 0=absent on MoE ints. Lock explicit norm_topk_prob:false and non-DeepSeek unset via tests; cover Stage-E2 weights_load reject on CPU make test. Plan: doc/plans/fix-124-5020997508.md Review: #124 (comment)
|
Thanks for the thorough optional follow-ups - landed in cde9d58. Plan trail: Disposition
Notes on scope choices
Verification
|
Summary
Implements #109 (E2 family split correction / decision 11 + DeepSeek config fields). Parent: #105.
Corrects A1 interim debt that mapped every DeepSeek HF
model_typeontoMODEL_DEEPSEEK_V4, and lands themodel_config_tsurface later E2 work needs. Generate is not enabled for any DeepSeek family.Decision 11 mapping
model_typemodel_family_tdeepseek_v3MODEL_DEEPSEEK_V3deepseek_v3_2MODEL_DEEPSEEK_V32deepseek_v32MODEL_DEEPSEEK_V32deepseek_v4MODEL_DEEPSEEK_V4deepseek*MODEL_FAMILY_UNKNOWN(no prefix match)HF audit snapshot (raw
config.json)model_typemlx-community/DeepSeek-V3-4bitdeepseek_v3rope_scalingwithmscale_all_dim=1.0; no indexermlx-community/DeepSeek-V3.2-4bitdeepseek_v32index_head_dim=128,index_n_heads=64,index_topk=2048mlx-community/DeepSeek-V4-Flash-4bitdeepseek_v4q_lora_rank,qk_rope_head_dim, indexer, MoE counts; missingkv_lora_rank/qk_nope_head_dim/v_head_dim/n_group/topk_group/moe_layer_freq/first_k_dense_replace. V4-only keys deferred to #115.Sample values:
New
model_config_tfieldsq_lora_rank,kv_lora_rank,qk_rope_head_dim,qk_nope_head_dim,v_head_dimn_routed_experts,n_shared_experts,routed_scaling_factor(default 1.0),moe_layer_freq,first_k_dense_replace,n_group,topk_group,norm_topk_prob(default true)rope_scaling_mscale_all_dim(0 = absent)index_head_dim,index_n_heads,index_topkn_routed_experts > 0andnum_experts == 0, setnum_experts = n_routed_experts(DeepSeek HF omitsnum_experts; do not clobber explicit value)Reject path
weights_loadrejects all three families with honest labels:Clears stale test debt that still expected a
"GGUF"substring while production already said Stage-E MLA.Explicit non-goals (this PR)
engine_model_check_supportedenablement for any DeepSeek familyweights_expected_names/ sanitize (E2: DeepSeek weight sanitization + weights_expected_names #112)o_lora_rank,compress_ratios,hc_*) (E2: V4 characterization + forward (DeepSeek-V4-Flash-4bit) #115)CLAUDE.mdsupported-families text already matched decision 11 - verified, no edit.Test plan
make test(44 passed)./tests/test_weights_gpu(DeepSeek three-family reject + existing suite)Closes #109