Skip to content

Qwen4exp correctness fixes - #27879

Closed
tarruda wants to merge 10 commits into
ggml-org:masterfrom
tarruda:qwen4exp-correctness-fixes
Closed

Qwen4exp correctness fixes#27879
tarruda wants to merge 10 commits into
ggml-org:masterfrom
tarruda:qwen4exp-correctness-fixes

Conversation

@tarruda

@tarruda tarruda commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Overview

Fix issues found by GTP 5.6 Sol by comparing llama.cpp implementation of Qwen4Exp against transformers and vllm.

Additional information

I cannot judge if these are correct or even make sense, so I'm opening as a draft/reference that maintainers can use as they want.

Here's a summary provided by GPT 5.6 Sol. Each item in this list corresponds to one commit:

  • Fix sparse-attention block selection. QSA blocks are now formed in each sequence’s logical token order, with only the incomplete tail kept unconditionally visible. Pooled keys also use the first token’s full M-RoPE position. Previously, long-context, unified-cache, multi-sequence, and multimodal inference could select the wrong history blocks, causing silent logit drift, quality loss, and cross-sequence interference.
  • Support independent PLE embedding widths. PLE key/value projections are now sized from the concatenated n-gram embedding width rather than assuming it equals the main hidden width. The official model does not satisfy that assumption, so the old implementation could reject a correctly converted GGUF with tensor-shape errors or construct incorrect projections.
  • Validate Qwen4Exp metadata during loading. GDN, hyper-connection, QSA, compression, and PLE invariants are checked before graph construction. Malformed or incompatible GGUFs now produce clear load errors instead of later assertions, out-of-bounds fixed-array copies, invalid tensor shapes, or possible crashes. This does not change inference for a valid official model.
  • Update the QSA indexer cache during sequence copies. Indexer keys are treated as raw, unrotated data and are copied or updated alongside the attention and recurrent caches. Previously, sequence forks, prompt sharing, cache shifts, or non-unified sequence copies could leave stale indexer keys or incorrectly apply RoPE to them, making child-sequence logits diverge from an equivalent independent decode.
  • Enable recurrent-state rollback. Qwen4Exp already had the required rollback implementation, but its architecture capability flag was missing, so n_rs_seq was silently disabled. This primarily affects speculative decoding: rejected draft tokens need the GDN and PLE recurrent state restored exactly. Ordinary non- speculative generation is unaffected.
  • Reject tensor-split mode. Qwen4Exp does not currently implement the state and cache semantics required by -sm tensor. Master incorrectly allowed the mode. It now fails cleanly during model loading instead of reaching unsupported device placement, graph failures, or potentially invalid results. Normal layer offloading remains available

Requirements

Build QSA blocks per sequence in token order and select complete blocks
before expanding them to cache cells. Keep only the incomplete tail
unconditionally visible and rotate pooled keys with the first token's
full M-RoPE position.

This prevents unified-cache sequences from sharing pooled indexer keys
and avoids replacing padded tail entries with extra history tokens.

Synthetic Qwen4 architecture, exact mask, F16 and Q8_0 state,
sequence-copy, Metal, and AddressSanitizer checks pass.

Assisted-by: Codex
Size the PLE key and value projections from the concatenated n-gram
embedding instead of assuming it matches the model hidden width.
Validate the head count before narrowing it to the stored type.

Add a synthetic PLE model with a 64-wide embedding and a 256-wide
hidden state, then verify inference and model roundtrip.

Assisted-by: Codex
Reject invalid GDN, hyper-connection, QSA, and PLE dimensions during
model loading instead of aborting later while building the graph.
Validate PLE array lengths before copying them into fixed storage.

The released configuration and synthetic Qwen4 architecture tests pass.

Assisted-by: Codex
Treat cached indexer keys as unrotated data and apply pending cache
updates alongside the attention and recurrent state. This copies indexer
data during non-unified cross-stream sequence copies without applying
RoPE shifts to raw keys.

Assisted-by: Codex
@github-actions github-actions Bot added model Model specific testing Everything test related ggml changes relating to the ggml tensor library for machine learning Apple Metal https://en.wikipedia.org/wiki/Metal_(API) labels Aug 28, 2026
Normalize GDN queries and keys with rsqrt(sum(x^2) + eps), matching
the reference instead of ggml_l2_norm's max(sqrt(sum), eps)
convention.

Assisted-by: Codex

@ggerganov ggerganov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tarruda Could you extract just the ggml-metal 2 line changes in a separate PR? If possible with a test in test-backend-ops that reproduces the overflow.

@danielhanchen

Copy link
Copy Markdown
Contributor

@tarruda @ggerganov I re-checked - some fixes are fine, but some break CUDA - I also did a few more at #27941 if that helps

@tarruda

tarruda commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

@ggerganov done: #27944, also pushed a7fc7e4 to this branch which is something GPT 5.6 Sol found later

@tarruda

tarruda commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

@danielhanchen dfc7932 should address the CUDA issue.

Correct me if I'm wrong, but it seems #27941 is just doing a subset of the things that this PR does but with an alternative QSA implementation, plus the CUDA fix that I just covered with dfc7932

mihailescu2m added a commit to mihailescu2m/llama.cpp that referenced this pull request Aug 29, 2026
Metal requires the length passed to setThreadgroupMemoryLength: to be 16-byte
aligned. An unaligned length is undefined behaviour and shows up as corrupt
kernel results rather than an error.

Cherry-picked from upstream PR ggml-org#27879.
mihailescu2m added a commit to mihailescu2m/llama.cpp that referenced this pull request Aug 29, 2026
Treat cached indexer keys as unrotated data and apply pending cache updates
alongside the attention and recurrent state. This copies indexer data during
non-unified cross-stream sequence copies without applying RoPE shifts to the
raw keys.

Cherry-picked from upstream PR ggml-org#27879.
dzannotti added a commit to dzannotti/strix-halo-llama.cpp that referenced this pull request Aug 30, 2026
  ggml-org#25144  speculative: fix MTP draft crash on vision inputs
  ggml-org#27879  Qwen4exp correctness fixes
  ggml-org#27897  speculative: fix combined draft-mtp + external draft (-md) init crash
  ggml-org#27973  vulkan: fuse GATED_DELTA_NET state write into recurrent cache

27836 (qwen4exp NextN/MTP draft head) is left out: it overlaps 27879 in
qwen4exp.cpp and cannot apply alongside it. 27879 is the correctness set, and we
already have a working MTP head, so it wins the conflict.

25144 and 27897 both cover configurations we run -- mmproj alongside MTP, and -md
with --spec-type draft-mtp. 27973 is the Gated DeltaNet path, 36 of Flash-Next's
48 layers.
@ggerganov

Copy link
Copy Markdown
Member

I think there is an issue here - with -ub 2048 it tries to allocate 128GB compute buffer. Overall the compute buffer is extremely large with this branch. Haven't looked in details, but building separate graphs for the streams is incorrect. The streams should be batched instead.

@tarruda

tarruda commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

@ggerganov ashamed to say I didn't test this branch in isolation, but only as a subset of the changes of this branch.

A bit more context: that other branch contains a whole lot more than just fixing qwen4exp to match transformers reference. With this PR, I've tried to get the agent to isolate only the fixes to match the transformers implementation without any of the graph/metal optimizations. Clearly it missed important pieces.

Merged master and added a fix on top, and the issue is fixed. As always, feel free to ignore or just pick what you need into separate PRs. I can also do it if it will make reviewing/merging easier.

mihailescu2m added a commit to mihailescu2m/llama.cpp that referenced this pull request Aug 30, 2026
Metal requires the length passed to setThreadgroupMemoryLength: to be 16-byte
aligned. An unaligned length is undefined behaviour and shows up as corrupt
kernel results rather than an error.

Cherry-picked from upstream PR ggml-org#27879.
mihailescu2m added a commit to mihailescu2m/llama.cpp that referenced this pull request Aug 30, 2026
Treat cached indexer keys as unrotated data and apply pending cache updates
alongside the attention and recurrent state. This copies indexer data during
non-unified cross-stream sequence copies without applying RoPE shifts to the
raw keys.

Cherry-picked from upstream PR ggml-org#27879.
@danielhanchen

Copy link
Copy Markdown
Contributor

@tarruda Sorry on the delay - yes some fixes I repurposed and rewrote since it broke CUDA - I also added 2 more as well to #27941

dzannotti added a commit to dzannotti/strix-halo-llama.cpp that referenced this pull request Aug 30, 2026
Squashed so the set reverts as one on rebase.

Takes the line descending from the merged ggml-org#27742: ggml-org#27836 (NextN/MTP draft head)
and ggml-org#27941 (danielhanchen's follow-up fixes), plus ggml-org#27977 and fifteen others.
Drops ggml-org#27879 (third-party correctness fixes) -- it conflicts with ggml-org#27836 in
qwen4exp.cpp, and MTP is not optional here.

ggml-org#27836 supplies everything the MTP head needs: it reads nextn_predict_layers,
loads the nextn block behind ml.load_mtp, and adds the MTP graph. Earlier local
patches for those are dropped as redundant.

Still ours: the two --ngram-on-disk bugs from halo-box#8, which live in
code these PRs do not touch. ple_head_offsets was applied twice, and can_reuse
dereferenced rows -- null whenever the table is on disk.
dzannotti added a commit to dzannotti/strix-halo-llama.cpp that referenced this pull request Aug 30, 2026
Squashed so the set reverts as one on rebase.

Takes the line descending from the merged ggml-org#27742: ggml-org#27836 (NextN/MTP draft head)
and ggml-org#27941 (danielhanchen's follow-up fixes), plus ggml-org#27977 and fifteen others.
Drops ggml-org#27879 (third-party correctness fixes) -- it conflicts with ggml-org#27836 in
qwen4exp.cpp, and MTP is not optional here.

ggml-org#27836 supplies everything the MTP head needs: it reads nextn_predict_layers,
loads the nextn block behind ml.load_mtp, and adds the MTP graph. Earlier local
patches for those are dropped as redundant.

Still ours: the two --ngram-on-disk bugs from halo-box#8, which live in
code these PRs do not touch. ple_head_offsets was applied twice, and can_reuse
dereferenced rows -- null whenever the table is on disk.
dzannotti added a commit to dzannotti/strix-halo-llama.cpp that referenced this pull request Aug 30, 2026
Squashed so the set reverts as one on rebase.

Takes the line descending from the merged ggml-org#27742: ggml-org#27836 (NextN/MTP draft head)
and ggml-org#27941 (danielhanchen's follow-up fixes), plus ggml-org#27977 and fifteen others.
Drops ggml-org#27879 (third-party correctness fixes) -- it conflicts with ggml-org#27836 in
qwen4exp.cpp, and MTP is not optional here.

ggml-org#27836 supplies everything the MTP head needs: it reads nextn_predict_layers,
loads the nextn block behind ml.load_mtp, and adds the MTP graph. Earlier local
patches for those are dropped as redundant.

Still ours: the two --ngram-on-disk bugs from halo-box#8, which live in
code these PRs do not touch. ple_head_offsets was applied twice, and can_reuse
dereferenced rows -- null whenever the table is on disk.
dzannotti added a commit to dzannotti/strix-halo-llama.cpp that referenced this pull request Aug 30, 2026
Squashed so the set reverts as one on rebase.

Takes the line descending from the merged ggml-org#27742: ggml-org#27836 (NextN/MTP draft head)
and ggml-org#27941 (danielhanchen's follow-up fixes), plus ggml-org#27977 and fifteen others.
Drops ggml-org#27879 (third-party correctness fixes) -- it conflicts with ggml-org#27836 in
qwen4exp.cpp, and MTP is not optional here.

ggml-org#27836 supplies everything the MTP head needs: it reads nextn_predict_layers,
loads the nextn block behind ml.load_mtp, and adds the MTP graph. Earlier local
patches for those are dropped as redundant.

Still ours: the two --ngram-on-disk bugs from halo-box#8, which live in
code these PRs do not touch. ple_head_offsets was applied twice, and can_reuse
dereferenced rows -- null whenever the table is on disk.
dzannotti added a commit to dzannotti/strix-halo-llama.cpp that referenced this pull request Aug 30, 2026
Squashed so the set reverts as one on rebase.

Takes the line descending from the merged ggml-org#27742: ggml-org#27836 (NextN/MTP draft head)
and ggml-org#27941 (danielhanchen's follow-up fixes), plus ggml-org#27977 and fifteen others.
Drops ggml-org#27879 (third-party correctness fixes) -- it conflicts with ggml-org#27836 in
qwen4exp.cpp, and MTP is not optional here.

ggml-org#27836 supplies everything the MTP head needs: it reads nextn_predict_layers,
loads the nextn block behind ml.load_mtp, and adds the MTP graph. Earlier local
patches for those are dropped as redundant.

Still ours: the two --ngram-on-disk bugs from halo-box#8, which live in
code these PRs do not touch. ple_head_offsets was applied twice, and can_reuse
dereferenced rows -- null whenever the table is on disk.
dzannotti added a commit to dzannotti/strix-halo-llama.cpp that referenced this pull request Aug 30, 2026
Squashed so the set reverts as one on rebase.

Takes the line descending from the merged ggml-org#27742: ggml-org#27836 (NextN/MTP draft head)
and ggml-org#27941 (danielhanchen's follow-up fixes), plus ggml-org#27977 and fifteen others.
Drops ggml-org#27879 (third-party correctness fixes) -- it conflicts with ggml-org#27836 in
qwen4exp.cpp, and MTP is not optional here.

ggml-org#27836 supplies everything the MTP head needs: it reads nextn_predict_layers,
loads the nextn block behind ml.load_mtp, and adds the MTP graph. Earlier local
patches for those are dropped as redundant.

Still ours: the two --ngram-on-disk bugs from halo-box#8, which live in
code these PRs do not touch. ple_head_offsets was applied twice, and can_reuse
dereferenced rows -- null whenever the table is on disk.
@tarruda

tarruda commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Assuming #27941 and other recent PRs addressed all issues, so closing

@tarruda tarruda closed this Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Apple Metal https://en.wikipedia.org/wiki/Metal_(API) ggml changes relating to the ggml tensor library for machine learning model Model specific testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants