Qwen4exp correctness fixes - #27879
Conversation
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
Assisted-by: Codex
Assisted-by: Codex
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
|
@tarruda @ggerganov I re-checked - some fixes are fine, but some break CUDA - I also did a few more at #27941 if that helps |
be175ae to
a7fc7e4
Compare
|
@ggerganov done: #27944, also pushed a7fc7e4 to this branch which is something GPT 5.6 Sol found later |
Assisted-by: Codex
|
@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 |
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.
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.
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.
|
I think there is an issue here - with |
|
@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. |
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.
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.
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.
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.
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.
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.
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.
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.
|
Assuming #27941 and other recent PRs addressed all issues, so closing |
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:
Requirements