Repository navigation
model/zamba2: find the stored shared block by attn_output, so one-block models with attention adapters load (engine #1036) - #229
Conversation
…ck models with attention adapters load (engine ggml-org#1036) The loader finds the layers that hold the stored shared block by `attn_q`. With `use_shared_attention_adapter` the converter writes q/k/v (adapter merged) in every hybrid layer, so every hybrid layer counts as a source. With num_mem_blocks = 1 (the Zamba2-1.2B-v2 shape) the alias loop then has nothing to fill: attn_output, ffn_norm, ffn_down and post_attention_norm stay null in all hybrid layers but the first, and the graph aborts in ggml_mul_mat. The loader now finds sources by `attn_output`, and fills q/k/v from the source only when the layer does not store its own. tests/test-zamba2-convert.py builds small random Zamba2 checkpoints (1, 2, 3 shared blocks; 1 block with attention adapters), converts them, and checks that every hybrid layer resolves the weights HF uses (atol 1e-5). Controls: swapping two stored blocks must give mismatches, and moving the one block away from the first hybrid layer must be refused by the loader. With `--bin` it also runs each GGUF through llama-perplexity on the CPU. #218 head (c2 build): FAILED 1, blocks1-attn exit -6 (abort); the other 11 checks pass this change: PASS, 12/12 Zamba2-7B-Instruct (2 blocks, no attention adapters) is unchanged: PPL 8.499977, KLD 0.000095 vs the merged form's logits on both builds. Assisted-by: Claude Opus 5.5
|
ready for review at 3c6fada |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 3c6fada49 — I reproduced the abort on the unfixed loader and watched this patch remove it, with the controls biting and the real 2.7B container numerically unchanged. One finding about the new test's environment, which cost me 20 minutes and will cost the next person the same.
The defect, reproduced — base binary at 765fa1c4c (the #214 merge, i.e. the unfixed attn_q rule), this PR's own test, the author's venv (~/venvs/quark013/bin/python, transformers 4.57.6):
ok blocks1: loads and evaluates on the CPU (exit 0)
FAIL blocks1-attn: loads and evaluates on the CPU (exit -6) <- the abort the body describes
ok blocks2 / blocks3: loads and evaluates (exit 0)
ok controls: block moved to blk.3 refused (loader model + exit -6); blocks swapped -> 40 mismatches
FAILED: 1
That is the author's "unfixed loader" table to the row, on a binary I built here.
The fix, on the same test and the same venv, head 3c6fada49:
ok blocks1-attn: … sources [1], ffn_up x1 attn_q x4 attn_output x1 adapters 4+4 rank 4, 112 tensors
ok blocks1-attn: loads and evaluates on the CPU (exit 0)
… all 12 arms ok … PASS
Note the row that matters: attn_q x4 with attn_output x1 — q/k/v per layer (attention adapters merged) and one stored wo. Sourcing on wq therefore counted every hybrid layer as a block source, which is exactly why the alias loop had nothing left to fill for num_mem_blocks = 1; sourcing on wo and filling q/k/v only when a layer lacks its own is the right correction, and the if (layer.wq == nullptr) guard is what keeps the per-layer merged q/k/v intact.
Controls are not decorative: swapping two stored blocks produces 40 mismatches in both the 2-block and 3-block cases, and moving the single block off the first hybrid layer is refused twice — by the test's own loader model and by the real loader (exit -6). So the compare would notice a wrong alias map, and the loader genuinely asserts instead of loading wrong weights.
Numerically inert for the real container, checked rather than taken: the 2.7B adapter-form file (~/work/zarch/c2/z2-2.7b-adapter-bf16.gguf) on the fixed binary gives PPL 6.5991 ± 1.06426 on my 2×512 window — the same value #218 was verified at, to the digit — and The capital of France is Paris. as before. test-llama-archs -a zamba2 is also still OK (0.00e+00) + roundtrip OK.
The finding — this test is silently version-dependent, and the failure looks like this PR's fault. With transformers 5.17 (~/venvs/d3ref/bin/python) the --bin leg fails on both binaries, base and head, identically:
FAIL blocks1/blocks1-attn/blocks2/blocks3: loads and evaluates on the CPU (exit 1)
check_tensor_dims: tensor 'blk.0.ssm_in.weight' has wrong shape; expected 32, 164, got 32, 168, 1, 1
The tiny synthetic config's ssm_in width is computed differently by the converter's expectations under 5.17 (164 vs 168 = the case RANK), so every GGUF the test builds is refused before the thing under test runs. The test only checks that the modules import (exit 77 on ImportError), not that the fixture is calibrated for the installed version, so a reviewer or CI box with a newer transformers gets five FAILs on a healthy patch. Two lines would fix it: an upper bound on transformers (or a version-aware SKIP, in the same shape as the 77), and a line in the docstring naming the version the f32 fixture was calibrated against — the test is now the gate for this code path, so it should not be the one that fails confusingly.
Not re-run, stated plainly: the 7B real-model KLD pair (PPL 8.499977 / KLD 0.000095 on both binaries) — the 7B adapter-form GGUF is not the one I have here; I used the 2.7B container above, which makes the same point (identical PPL to the pre-patch build). Refs #1036 and not closing it is correct: this is cause 1's one-block shape, and the issue's other causes stay open.
Refs 1bit-MONSTER/engine#1036 (cause 1 follow-up: the
num_mem_blocks = 1shape with attention adapters). Scoped; the issue stays open.The defect
The zamba2 loader (from #203) finds the layers that hold the stored shared block by
attn_q. Withuse_shared_attention_adapter, the converter writes q/k/v (adapter merged) in every hybrid layer, so every hybrid layer counts as a source. Withnum_mem_blocks = 1(the Zamba2-1.2B-v2 shape), the alias loop then has nothing to fill.attn_output,ffn_norm,ffn_downandpost_attention_normstay null in every hybrid layer after the first, and the graph aborts:No 2-block model hits this: every model measured so far has 2 blocks and no attention adapters.
The change
src/models/zamba2.cpp(+8/-5): find sources byattn_output. Fill q/k/v from the source only when the layer does not store its own.tests/test-zamba2-convert.py(new):Zamba2ForCausalLMcheckpoints and converts them withconvert_hf_to_gguf.py --outtype f32.gate_up = base + W1 @ W0), atol 1e-5.--bin DIRalso runs each GGUF throughllama-perplexityon the CPU.Proof (strixhalo, CPU only,
~/venvs/quark013/bin/python)Real model, unchanged by this patch: Zamba2-7B-Instruct adapter-form BF16 (sha256
58b26e9a...), measured against the merged form's logits:Not in this PR
docs/zyphra.mdsizes) come in a separate engine PR, from measured numbers only.🤖 Generated with Claude Code