model : load hparams.n_layer_nextn before n_layer() calls - #28159
Conversation
…l-org#28159 hoist ggml-org#28159 moved the n_layer_nextn read into load_hparams, so n_layer() excludes the NextN block by the time the base fills n_head_arr, n_head_kv_arr and n_ff_arr: the per-layer arrays stop before it and its entries stay zero. Harmless for the archs that only build trunk tensors, fatal as soon as a context reduces to that layer: the MTP context sized its KV cache from n_head_kv == 0 and died on "failed to allocate buffer for kv cache". The entries are copied from the last trunk layer, which is what a scalar key would have broadcast anyway. The local read of the key (lines 27 29) becomes a duplicate of the base and disappears with it.
|
Hmmm, this may have broken Step3.5 |
|
@CISC first: thank you for the hoist. It also removed the duplicated read in every NextN arch, this one included, and the local copy is gone from my branch as a result. What follows is a side effect of it, not a complaint; you could not see it from mainline, because no arch there exposes the case. After the hoist, n_layer() reads n_layer_all - n_layer_nextn from line 1226, so the three get_key_or_arr at lines 1287, 1288 and 1296 stop before the NextN block, and its entries stay zero. That is invisible for the archs that only build trunk tensors, since they never read the block's hparams. It is fatal as soon as a context reduces to that layer, which is what the glm5next MTP draft does: measured on my branch, on top of 9d81721, n_head_kv[3] = 1 against [4] = 0 (n_ff[3] = 192, [4] = 0, n_layer_all = 5, nextn = 1) — the context reduces to that single layer and sized its KV cache from n_head_kv == 0, dying on failed to allocate buffer for kv cache. So the question I could not settle from mainline alone: after the hoist, should the base broadcast the last value onto the NextN entries, or is it each arch's job to mirror it? A scalar key would have broadcast anyway, which is what my branch does now, in one small commit on top of yours — enough for this arch, but if you would rather the base own it, say so and the mirror is one delete away. |
|
Quick regression note: This appears to have broken model loading for GLM5-Next: As far as I can tell, this breaks ALL current gguf releases. |
|
@eauchs @Sciguy429 Yeah, but also Step3.5, I think we do indeed have to include full array, however there may be other breakages the other way, trying to figure it out now. |
|
This broke Qwen 3.8 27B MTP. |
Fixed with #28173 |
|
This does not fully cover the gemma4-assistant arch. Two things still break Gemma 4 MTP draft heads with this PR applied. 1. The assert added in 9d81721 fires before any of these loads are reached. llama-model.cpp:1227: GGML_ASSERT(hparams.n_layer_nextn < hparams.n_layer_all); Gemma 4 assistant/drafter models are standalone draft heads whose entire body is the nextn stack, so nextn_predict_layers == block_count by construction. All three heads I have tested are 4/4: They abort in load_hparams before reaching the array loads. This is also the condition the arch-specific assert removed in 9d81721 used to require (n_layer_nextn == n_layer_all). Relaxing to <= works and still catches nextn > block_count. 2. gemma4-assistant.cpp:7 also sizes an array by n_layer(), which is 0 here. With the assert relaxed but only the three loads in this PR changed, the E4B head gets past head_count_kv and then fails on: (The 26B-A4B head fails earlier, on head_count_kv, which this PR does fix.) Repro on master, no server config needed - load any Gemma 4 assistant head directly as the main model: Result with both changes plus this PR: --model-draft and --spec-draft-model work again. Tested on E4B and 26B-A4B, draft acceptance 0.54 to 0.86. |
|
@ZacharyReis Ah, nice catch, I'll make a separate fix for that. |
Am I reading correctly that this model was in fact broken before this change? IE, that its |
|
@CISC the answer is no. The array is exactly n_layer_all, not smaller, and the model was not broken before this change. I had Opus dump the per-layer array lengths from all three MTP heads:
Opus output below: The 26B carries 4 entries, equal to n_layer_all, not below it. Before 9d81721, n_layer_nextn was still 0 when the generic loader reached that read, because gemma4-assistant.cpp set it afterwards in load_arch_hparams. So n_layer() returned 4, matching the array. The hoist makes n_layer() evaluate to 4 - 4 = 0 at that same read, so the expected length collapses to 0 while the file still correctly carries 4. #28173 restores the length that was already in effect. Two things that may be useful, since they need these specific files to see: The scalar/array split is why my two nodes failed at different keys. E4B's head_count_kv is uniform, so it was written as a scalar and broadcasts to any length, including 0. It passes that read and dies later on sliding_window_pattern. The 12B and 26B have a differing last layer, so they were written as real arrays and die earlier on head_count_kv. That is @eauchs's "uniform values go out as a scalar key" showing up in practice. These files are also in the set #28173 repairs, not the set widening could break. eauchs's exposed set was files carrying trunk-length arrays; these carry n_layer_all-length arrays. I have been running the equivalent of #28173 + #28183 together since about 15:00 UTC on all three heads. Both nodes load and serve with MTP active, and draft acceptance is back in its normal band (E4B runs 0.54-0.65 over real traffic). #28183 as written matches what I patched locally, line for line. AI usage disclosure: yes. I used Claude Opus to diagnose the regression, dump the GGUF metadata above, and draft this comment. I reviewed it, I am running the patch, and the numbers are from my own machines. |
|
This commit also appear to broke mtp loading on gemma4 models: 53739] 0.03.126.460 I common_speculative_init_result: loading draft model '/mnt/storage/llm/Gemma-4-12B/mtp-gemma-4-12B-it.gguf' |
|
Sorry for the noise everyone, the two remaining PRs will put everything back to normal (but in a more maintainable state). :) |
This comment was marked as resolved.
This comment was marked as resolved.
68 upstream commits, 9 conflicted files. Brings qwen4exp fixes needed for Qwen3.8-Flash-Next: recurrent state rollback (ggml-org#28123), seq_cp/block position keying/mtmd input (ggml-org#27941), indexer head slicing (ggml-org#28023), and the central n_layer_nextn load (ggml-org#28159). Resolutions: - ggml-rpc.cpp: resolved entirely to the fork side again. Upstream's rpc_dispatcher/proto-6.0 port stays deferred; the fork keeps 5.1.3. Note that upstream ggml-org#26500 now implements the same foreign-endpoint skip in serialize_graph that the fork carries, so that one is a candidate to drop when the dispatcher port finally happens. - llama-graph.cpp: adopted upstream's fused ggml_swiglu_clamp for both the dense and MoE clamp paths and kept the fork's LLM_ARCH_GLM5NEXT in the condition. The fused kernel is min(gate,limit) / clamp(up,+-limit) / swiglu_split, i.e. bit-identical to the three-op sequence it replaces, and Vulkan implements it, so the boards get it too. Costs the ffn_*_clamped cb() trace names, same tradeoff upstream took for DSV4. - speculative.cpp: upstream ggml-org#27310 folded the DFlash encoder into the injection decode and deleted features_buf. The fork's non-finite feature scrub moves onto batch_inject.embd in place; the last_tap_nextn tap automerged. - dflash.cpp: kept the fork's Laguna kv_inp norm and deferred gated o_proj, layered on upstream's newly-populated wk_s/wv_s/wo_s scale arguments (previously NULL). The gated path latches wo_s alongside wo. - nemotron-h.cpp: dropped the now-duplicate NEXTN_PREDICT_LAYERS get_key, kept the fork's stricter assert and n_layer_nextn_per_head derivation. - llama-model-loader.cpp: kept the fork's rpc_preloaded skip ahead of upstream's use_mmap || lazy.has(cur) predicate. - tests: kept both sets of flash-attn cases; ported the fork's GLM5NEXT test_dsa_kpool/test_mtp onto upstream's int-verbosity logging convention. Windows CUDA build green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U3H5motr51eTWujztSXykc
KV restore batching (ggml-org#27991), kv-cells seq-scan early stop (ggml-org#28011), MOE fusion to specdec + multi-token (ggml-org#27621), mm_ids_helper templated fast path (ggml-org#27978), qwen4exp recurrent state rollback (ggml-org#28123), n_layer_nextn load order (ggml-org#28159), FA K/V XOR-swizzle smem tiles (ggml-org#25635), --lazy-mode -lzm (ggml-org#27837/ggml-org#27969). TQ3/TurboQuant stack and vitriol-* integration auto-merged clean; no conflicts. Experiment E1 of mining-experiment-master-plan-2026-09-01.
9 participants
Overview
Fixes inconsistent return values from
n_layer().Additional information
This would f.ex. break loading of models with
n_head_kvarrays <n_layer_all.Requirements