Repository navigation
convert/model/quantize: store Zamba2's per-layer MLP adapter instead of baking it (engine #1036 cause 2) - #218
Conversation
|
#218 ready for review at Engine ggml-org#1036 cause 2, the last 1.13 B params — and the shape of the problem is worth one line: the duplication was never in the What a reviewer should check first:
Numbers, all on one binary per comparison: 2.7B BF16 6,009,822,848 → 5,328,903,360 B, PPL 16.7937 → 16.795417 (0.0102 %), The 7B's -2.26 GB is computed from the shapes, not measured: its originals are not on disk and, unlike #203, this cannot be synthesised |
… it (engine ggml-org#1036 cause 2) The remaining cause of engine ggml-org#1036. #203 stopped storing the shared transformer block once per hybrid layer; this stops storing the per-layer MLP adapter baked into a full FFN matrix, which was the last 1.13 B params / 2.26 GB of the 7B. The duplication was never in the checkpoint: its `gate_up_proj.weight` appears exactly twice (one per shared block) and the per-layer differences live in `gate_up_proj_adapter_list.<visit>.<0|1>`. Our converter merged each layer's rank-128 pair into its own full [n_embd, 2*n_ff] copy. HF applies it as `gate_up_proj(x) + adapter(x)` with no LoRA scaling (modeling_zamba2.py:960-962), on the same normed FFN input and before the gate/up split. So: `gate_up_proj` joins the shared tails (written once per block, aliased by the loader through the mechanism #203 added), and each `gate_up_proj_adapter_list.<visit>.<0|1>` is written as `blk.<layer>.ffn_gate_up_adapter_a/_b.weight`. A new key `zamba2.ffn_gate_up_adapter_rank` both discriminates the two forms (absent/0 = the pre-adapter form) and sizes the tensors. In the graph the pair is applied as base + B*(A*x) through one defaulted parameter on build_ffn, so no other arch's call site changes; the loader, the saver and the arch test all handle both forms, and old GGUFs keep working as a code path rather than a compatibility shim. 2.7B, CPU, one binary for both forms: BF16 6,009,822,848 -> 5,328,903,360 B (-681 MB), tensors 520 -> 531 with ffn_up 9 -> 2 and 9+9 adapters, PPL 16.7937 -> 16.795417 (0.0102 %), KLD 0.000067 +- 0.000002 against the merged file's own logits with same top-p 99.510 %, calibration floor 0.000000, greedy identical. The residual is structural: the merged file has up @ down computed in f32 at conversion time and rounded to bf16, the adapter path applies the pair at run time. Quantized (what gets published): Q5_K_M merged 1994.93 MiB / PPL 17.0430, adapter 1773.11 MiB / PPL 16.9775 - 221.8 MiB smaller and 0.4 % better on the same window. test-llama-archs: zamba2 OK (0.00e+00) + roundtrip OK, with the synthetic model declaring the adapter form so the new tensors are exercised by the saver round-trip. Quantized files need one more thing, found by reading the HRX predicate before the device could say it: the adapter's B contraction has input_size = rank = 128, which the float weight path accepts (input_size >= 4 && % 4 == 0, #208) but the block formats do not (their block layout needs >= 256). So a default Q5_K_M quantized the pair, dropped that matmul on all 9 hybrid layers to the CPU - measured on gfx1151 as 9 of 3051 nodes off HRX and prefill 463.66 vs 589.47 tok/s (0.79x) - while every BF16 arm said "fully on HRX". llama-quant now keeps the pair at F16 by default for 31.8 MiB of a 1804.93 MiB file (explicit --tensor-type still wins, --pure is unchanged), and the resulting file is byte-identical to the one measured strict-clean on the device. Device arms (gfx1151, gpu-hold), BF16 both forms, same binary: GGML_HRX_STRICT=1 exit 0 on all four (prefill and decode, merged and adapter), pp512 441.98 -> 440.57 and tg64 22.87 -> 22.60 (0.997 / 0.988, i.e. no measurable cost), device PPL 19.3635 both.
9306550 to
cec0a5e
Compare
|
Head moved to |
|
Two claims in the description, verified rather than asserted (both
|
|
One acceptance claim now tested on shipped artifacts rather than on my own BF16 ref: "no published file breaks". The published quants
No |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at cec0a5e9.
Built and ran the test at this head (GGML_HRX=OFF, Release, thermal-run):
$ test-llama-archs
| zamba2 | AMD RYZEN AI MAX+ 395 w/ Radeon 8060S | Dense | OK (0.00e+00) | OK |
FAIL rows: 0 OK rows: 128
so the synthetic zamba2 model now really does exercise the adapter form (rank 8) and the saver round-trip of the two new
tensors — that is the part of this PR that a reader cannot check by eye, and it is green.
The arithmetic in the body re-derives exactly: 6,009,822,848 − 5,328,903,360 = 680,919,488 B (680.9 MB, 11.3 % of
the file), and the tensor count 520 → 531 is 520 − 7 (ffn_up 9→2) + 18 (9+9 adapters). Both are consistent with the
"13 copies of a 102 MB matrix where 2 would do" story.
The two structural claims hold in the code, not just in the prose:
- the graph change is an addition to the matmul result before the split and the activation, and it is defaulted
(ggml_tensor * up_delta = nullptr), so every other architecture's call site is untouched and the branch is dead for them
—zamba2.cpp:247-259passesbuild_lora_mm(ffn_gate_up_adapter_b, A*x)as that argument; - the saver's guard is the hparams rank (
zamba2_ffn_gate_up_adapter_rank > 0), and the loader sets that rank only from the
key and then requires the pair when it is set (zamba2.cpp:26,62-63,120-122), so a merged file cannot be written with
a key claiming an adapter it does not carry — the failure mode the comment names.
The new zamba2.ffn_gate_up_adapter_rank key is registered everywhere it has to be for a roundtrip to work (gguf-py
constants + writer + tensor enum, llama-arch.{h,cpp}, llama-hparams.h), and refusing a checkpoint with adapters but no
adapter_rank rather than guessing a shape is the right call.
Not re-measured here, stated rather than implied: the BF16/Q5_K_M sizes, the PPL pair and the KLD row, and the device
rows. Those need a Zamba2-2.7B checkpoint converted twice on this box plus an HRX window, and c31 has the device held
(TISSUES-ALL rule 1) — the NPU lock is also currently pinned by another lane's host-GEMM serve. The 6.7e-5 KLD and its
"structural, not a bug" explanation (f32 up @ down at conversion time vs the pair applied at run time) is sound reasoning and
the right order of magnitude to be below quant noise, which is what the Q5_K_M pair then shows.
Fixes ggml-org#1036 (cause 2), and the closing statement that old files keep working as a code path rather than a shim matches what I
read in the loader: ffn_up is required in the old form and optional in the new one.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at cec0a5e93 — I reproduced the two files this PR's whole argument rests on (the adapter-form container and the pre-change one are both on this box), and both load in one binary: byte-exact sizes, exact tensor counts, identical greedy text, PPL equal within the window's own noise.
The two artifacts, measured with gguf-py and stat (no conversion needed — the lane's own adapter-form file survives at ~/work/zarch/c2/z2-2.7b-adapter-bf16.gguf, and the pre-change container is the repo's reference ref/Zamba2-2.7B-instruct-bf16.gguf):
| merged (pre-change) | adapter (this PR's converter) | PR's claim | |
|---|---|---|---|
| file bytes | 6,009,822,848 | 5,328,903,360 | −681 MB / −680.9 measured ✔ |
| tensors | 520 | 531 | 520 → 531 ✔ |
ffn_up |
9 | 2 | 9 → 2 ✔ |
ffn_gate_up_adapter_a/_b |
0 | 9 + 9 | 9+9 ✔ |
zamba2.ffn_gate_up_adapter_rank |
absent | 128 | ✔ |
The 7B arithmetic in the body, recomputed from the checkpoint's own index (~/models/zyphra-orig/Zamba2-7B-Instruct@3146688c5286/model.safetensors.index.json, adapter_rank 128, use_shared_mlp_adapter true): gate_up_proj.weight appears exactly 2 times and gate_up_proj_adapter_list.<0..12>.<0|1> gives 13 pairs. With hidden 3584 × 2*ff 28672 = 102,760,448 params per copy: 11 extra copies = 1,130,364,928 params = 2.26 GB at bf16 ✔, and one rank-128 pair is 3584·128 + 128·28672 = 4,128,768 ≈ 4.13 M ✔. The one slip is prose: "13 copies of a 102 MB matrix" is 102.76 M params, i.e. 205 MB per copy in bf16 — the 1.13 B / 2.26 GB figures themselves are exact.
Both forms load in this head's binary (CPU, -ngl 0, --no-warmup, greedy):
adapter -> "The capital of France is Paris." 11.2 t/s
merged -> "The capital of France is Paris." 10.7 t/s (old files keep loading: the path is a code branch, not a shim)
Perplexity on the same 2×512 window, same binary: adapter 6.5991 ± 1.06426, merged 6.6027 ± 1.06509 — 0.055 % apart with overlapping spreads. That is the same conclusion as the PR's 16×512 pair (16.795417 vs 16.7937 = 0.0102 %), on a window small enough that I would not read it more finely; what it rules out is a wrong graph (a misplaced up_delta or a mis-shaped pair would move PPL by far more than 0.05 %).
The arch test, and the fix it exercises: test-llama-archs -a zamba2 → zamba2 | Dense | **OK (0.00e+00)** | roundtrip **OK**; full run 128 OK rows, 190 SKIP, 0 FAIL, rc 0. That is the adapter form (the fixture now declares rank 8), so the saver's round-trip of the two new tensors is covered rather than assumed. (A first run of this test I threw away was a bad build of my own — I had switched the source tree under a running ninja; the clean rebuild is what the numbers above come from.)
Quant rule: is_zamba2_mlp_adapter() keeps the pair at F16 with the reason in the comment (the B contraction's input_size = rank = 128 is under the block formats' 256 minimum), --pure and an explicit --tensor-type still win. Nothing in it changes another arch's default.
Not re-run, stated plainly: the 7B convert-to-7B numbers (that checkpoint's weights are not on this box — only its index), the Q5_K_M pair (1994.93 → 1773.11 MiB, PPL 17.0430 → 16.9775), and the HRX device arms (GGML_HRX_STRICT=1, pp512 441.98 → 440.57, tg64 22.87 → 22.60). The device rows need an HRX build and a GPU window, which tonight's c31 hold excludes; the quantized pair needs a 14 GB conversion I did not spend the queue's time on. The claim those support — "no silent CPU fallback, ~1 % cost" — is the author lane's, and it is the one thing a later window should spot-check.
|
Follow-up, form rather than code (my READY at
|
|
7B row for the adapter form (measured on strixhalo, CPU only) Zamba2-7B-Instruct @
Layout: Same model as the merged form, measured against the merged form's logits: Two builds gave the same numbers: #218's head and a build with a pending zamba2 loader fix. The result is not bit-identical, because |
…ck models with attention adapters load (engine ggml-org#1036) (#229) 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 Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
|
Caught by your review and fixed in the body rather than left standing: the 7B line said "13 copies of a 102 MB matrix" — 102.76 is the param count, so a bf16 copy is 205 MB — 13 copies → 2 saves 11 × 205 MB ≈ 2.26 GB, which is what the 1.13 B params / 2.26 GB figures already said. The slip was in the parenthetical only. |
convert/model: store Zamba2's per-layer MLP adapter instead of baking it (engine ggml-org#1036 cause 2)
Second and last cause of engine ggml-org#1036. #203 stopped storing the shared transformer block once per hybrid layer; this one stops storing
the per-layer MLP adapter baked into a full FFN matrix, which was the remaining 1.13 B params / 2.26 GB of the 7B.
What was actually duplicated
Not the checkpoint — its
gate_up_proj.weightalready appears exactly twice (one per shared block) and holds the per-layer differencesin
gate_up_proj_adapter_list.<visit>.<0|1>. Our converter merged each layer's rank-128 pair into its own full[n_embd, 2*n_ff]copy, so the 7B carried 13 copies of a 102.76 M-param matrix (205 MB each in bf16) where 2 would do, plus 13 pairs of 4.13 M params. (Corrected after review: this line first said "102 MB", which is the param count, not the bf16 byte size; the 1.13 B params / 2.26 GB figures were always exact.)
HF's math (
transformers/models/zamba2/modeling_zamba2.py:960-962):gate_up = gate_up_proj(x) + adapter(x), where the adapter isSequential(Linear(hidden, 128, bias=False), Linear(128, 2*inter, bias=False))— no LoRA alpha/rank scaling, applied to the same normedFFN input, added before the gate/up split.
Change (11 files)
conversion/zamba2.py:feed_forward.gate_up_proj.weightjoins the shared tails (written once, at the block's source layer, aliased bythe loader through convert/model: store Zamba2's shared transformer block once per block (engine #1036) #203's mechanism); each
gate_up_proj_adapter_list.<visit>.<0|1>is emitted atblk.<hybrid layer>.ffn_gate_up_adapter_a/_b.weight(<visit>is the hybrid visit index, which is the block mapping, so no configheuristic is involved). Attention adapters still merge as before — absent in these checkpoints, and unchanged if a future one has them. A
checkpoint with adapters but no
adapter_rankis refused rather than given a guessed shape.zamba2.ffn_gate_up_adapter_rank(u32): one key both discriminates the two forms (absent/0 = the pre-adapter form) andsizes the tensors, so the loader never has to infer a rank from a shape it does not trust. Registered in
gguf-py(Keys.Zamba2, writermethod, tensor enum + name),
llama-arch.{h,cpp},llama-hparams.h.src/models/zamba2.cpp):ffn_upis required per layer in the old form and optional in the new one, where every hybrid layerwithout a copy aliases the block's source — the same rule convert/model: store Zamba2's shared transformer block once per block (engine #1036) #203 uses for the other seven tails. The adapter pair is per layer and always
required when the key is set. Shapes are
{n_embd, rank}/{rank, 2*n_ff}, i.e. GGUF's [in, out] layout.build_ffngains one defaulted parameter,up_delta, added to the fused gate/up matmul result before the split and theactivation.
zamba2.cppcomputesB*(A*x)from the normed FFN input and passes it. Every other architecture's call site is untouched —the default is
nullptrand the added branch is dead for them.form while carrying baked weights. The new layer fields are tensors like any other and are picked up by the saver's existing loop.
tests/test-llama-archs.cpp: the synthetic zamba2 model now declares the adapter form (rank 8), so the arch test exercises the adapterpath and the saver round-trip of the new tensors; the published GGUFs remain the coverage for the merged form.
Old files keep working — the merged form is a code path, not a compatibility shim.
Evidence, 2.7B (CPU,
GGML_HRX=OFF, one binary for both forms)ffn_up× 9)ffn_up× 2, adapters 9+9)The residual 6.7e-5 is structural, not a bug: the merged file has
up @ downcomputed in f32 at conversion time and rounded to bf16, whilethe adapter path applies the pair at run time. Two orders of magnitude below the calibration noise of any quant arm.
test-llama-archs:zamba2 OK (0.00e+00)+ roundtrip OK, 128 OK rows, no FAIL row.Quantized, which is what gets published: Q5_K_M of both forms, same window — merged 1994.93 MiB / PPL 17.0430, adapter
1773.11 MiB / PPL 16.9775. So the new form is 221.8 MiB smaller and 0.4 % better; the pair is not damaged by quantization (BF16
16.7954 → Q5_K_M 16.9775 is ordinary Q5 noise).
Device evidence (gfx1151,
pi-zarch-c2-hrx, gpu-hold, 62 s on the token)The owner's requirement was that the adapter path runs on HRX with no silent CPU fallback, and that the throughput cost is reported
honestly because it decides the default.
GGML_HRX_STRICT=1STRICT aborts on the first node not placed on HRX, so four exit-0 runs is the claim in the form that cannot be mis-read; the dispatch log
shows the plain
common.mul_mat.f32_f32_{wmma,decode,batched}families claiming both low-rank contractions. There is no slowdown toreport: 0.3 % and 1.2 %, inside the run-to-run spread. Device PPL (4 chunks) 19.3635 both, identical to five digits.
Quantized files: the part the BF16 arms could not see
common_mul_mat_supported_plain_dense_input_size()(the float path #208 widened) acceptsinput_size >= 4 && % 4 == 0, while the blockformats keep
input_size >= 256for their own layout. The adapter's B contraction hasinput_size = rank = 128. So a default Q5_K_Mquantized the pair, and every hybrid layer's B matmul left the device:
GGML_HRX_STRICT=1"ffn_gate_up_adapter_b-6" is MUL_MAT f32 ne=[20480,2,1,1], 9 of 3051 nodes off (MUL_MAT x 9), 19 splitsSo
llama-quantnow keeps the pair at F16 by default (an explicit--tensor-typestill wins;--pureis unchanged), for 31.8 MiB of an1804.93 MiB file and a 27 % prefill gain. The file the new default produces is byte-identical to the one measured strict-clean above.
This is the one place where the change costs anything at all, and it costs less than the duplication it removes.
Not in this PR
GGUF — a low-rank pair is not recoverable from a merged matrix, and an SVD substitute would make the equivalence test meaningless. Computed
from the shapes instead: 11 × 205,520,896 B removed, 13 pairs × 8.59 MB added → net ≈ 2.15 GB (2.005 GiB), i.e. the issue's ~2.3 GiB is
the gross duplication. Asked for a re-fetch; will measure when the floor allows.
SHA256SUMS(
7d7c0794…,7a64f9b7…).