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 •
edited
Loading

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

tarruda added 6 commits August 28, 2026 10:15
@tarruda
qwen4exp: fix sparse-attention block selection
c07204d 
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
@tarruda
qwen4exp: support independent PLE embedding widths
fce561d 
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
@tarruda
qwen4exp: validate model metadata
14b71e5 
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
@tarruda
qwen4exp: update indexer cache after sequence copies
6ef2436 
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
@tarruda
qwen4exp: enable recurrent state rollback
edb6dec 
Assisted-by: Codex
@tarruda
qwen4exp: disable tensor split
8427625 
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
@tarruda
qwen4exp: fix GDN QK normalization
a7fc7e4 
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 reviewed Aug 29, 2026
View reviewed changes

@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 danielhanchen mentioned this pull request Aug 29, 2026
Closed
@danielhanchen

danielhanchen commented Aug 29, 2026

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 mentioned this pull request Aug 29, 2026
Closed
@tarruda
tarruda force-pushed the qwen4exp-correctness-fixes branch from be175ae to a7fc7e4 Compare August 29, 2026 09:31
@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
@mihailescu2m
metal: align dynamic threadgroup memory to 16 bytes
8ee9dc3 
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
@mihailescu2m
llama: update the qwen4exp indexer cache after sequence copies
f2ad87e 
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.
@Green-Sky Green-Sky mentioned this pull request Aug 29, 2026
Closed
@ggerganov

ggerganov commented Aug 30, 2026

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
@mihailescu2m
metal: align dynamic threadgroup memory to 16 bytes
ec3c29d 
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
@mihailescu2m
llama: update the qwen4exp indexer cache after sequence copies
fe7a714 
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

danielhanchen commented Aug 30, 2026

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

@ovidiu-morar ovidiu-morar mentioned this pull request Aug 31, 2026
Open
@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

Reviewers

@ggerganov ggerganov ggerganov left review comments
@JohannesGaessler JohannesGaessler Awaiting requested review from JohannesGaessler JohannesGaessler will be requested when the pull request is marked ready for review JohannesGaessler is a code owner
@CISC CISC Awaiting requested review from CISC CISC will be requested when the pull request is marked ready for review CISC is a code owner

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tarruda @danielhanchen @ggerganov