fix(deepseek-v4): DSA top-k kernel could not represent the real index_topk — remove the literal bounds (#505) - #542
Open
localai-bot wants to merge 3 commits into
Open
fix(deepseek-v4): DSA top-k kernel could not represent the real index_topk — remove the literal bounds (#505)#542localai-bot wants to merge 3 commits into
localai-bot wants to merge 3 commits into
Conversation
…_topk -- remove the literal bounds (#505) `DsaTopkKernel` sized two thread-local arrays by literal: bool chosen[512]; // indexed [0, n), n = candidate-window length int64_t picked[64]; // written [0, topk) `topk` is the caller's `index_topk` -- 512 on V4-Flash, 1024 on V4-Pro -- so `picked[64]` was 8x short for Flash and 16x short for Pro, and `chosen[512]` overflowed on any window wider than 512. The overflow branch is the `n > topk` path. Neither bound was asserted nor derived from the config, and every pre-existing device case ran at topk=3/nk=5, which is why it was invisible. MEASURED, not theoretical. On dgx.casa (GB10, sm_121a) the pre-fix kernel driven at Flash's OWN index_topk of 512 with a 600-wide window takes a device fault: what(): vt cuda: cudaStreamDestroy: an illegal memory access was encountered test_cuda_deepseek_v4.cpp:214: FATAL ERROR: test case CRASHED: SIGABRT [doctest] test cases: 6 | 5 passed | 1 failed | 17 skipped [doctest] Status: FAILURE! After the fix, the same tree and flags: 23 cases, 83913/83913 assertions, 0 skipped, Status SUCCESS. Both arms ran under `flock $HOME/gpu.lock`, each asserting its own kernel identity before building. THE FIX replaces the mask-plus-picks approach with a threshold formulation over the same total order the host reference sorts by (logit desc, then index asc -- a total order because candidate indices are distinct). Pass 1 walks the order downwards `topk` times to land on the topk-th best element; pass 2 scans the window once ascending and emits everything outranking-or-equal to it. Exactly `topk` elements qualify, already in ascending key order, so the O(topk^2) emit sort disappears along with the buffers. No per-thread scratch, no bound, no configurable limit. Pass 1 stays O(topk*n) as before, so this is strictly cheaper overall. Two defensive additions are NOT capacity limits: pass 1 stops if no strictly worse element exists and pass 2 carries `w < topk`, so a NaN row -- where every float comparison is false -- cannot write past its own row into the next one, which is the failure class this issue was about. Deliberately NOT done: asserting the bounds instead of removing them, which the issue itself proposed. A refusal would leave the device path unable to run the real index_topk at all, trading a latent overflow for a guaranteed refusal once the real-geometry DSA residual lands. Also deliberately not done: sharing a selection helper between kernel and host reference -- two independent implementations agreeing is the value of this gate, so the duplication stays and is recorded as a stop condition. Three new device cases, all against the independent `DsaTopkSelect` oracle: the real index_topk widths (65/80, 512/600, 1024/1200, asserting no -1 leaks and strictly ascending emit), tie-heavy rows at topk=128 (the tie-break that distinct random logits cannot exercise), and an offset window at ws=137 (the old code indexed its mask by `s - s0` and its picks by absolute `s`). Local algorithm equivalence before the GPU lock freed (scratch, ASan+UBSan): the new body vs an independent transcription of the oracle, 0 mismatched entries over 8 named shapes and a 4000-shape randomized sweep, half coarsely quantized to force tie density. Latency note recorded in the spec: this was latent, not shipped, because `dsa_dense = (be.gguf != nullptr)` (`deepseek_v4.cpp:668`) forces the indexer off on the real keep-quant path. Upstream has no equivalent cap -- selection is `ops.top_k_per_row_prefill` (`sparse_attn_indexer.py:488-497`) over a window built as `ke = row_start + (pos+1) // COMPRESS_RATIO` (`indexer.py:270-290`). CPU V4 suites unmoved: dsa 38, pro_variant 197, scaffold 62, all SUCCESS. Worth keeping from the RED log: it prints `assertions: 632 | 632 passed | 0 failed` beside `Status: FAILURE!`, because a crashed case contributes no failed assertion. A gate reading assertion counts alone would have called it clean. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
17 commits, no overlap with the three paths this branch touches. Re-gated after the merge, including a fresh device run of test_cuda_deepseek_v4 on dgx.casa. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
origin/main advanced 17 commits between the RED/GREEN pair and landing, so the device suite was rebuilt and re-run from the MERGED tree rather than trusting the pre-merge green: 23 cases, 83913/83913 assertions, 0 skipped, Status SUCCESS, on dgx.casa (GB10, sm_121a). The fast path is hard-verified in that run's OWN configure log -- `CUTLASS found ... enabling sm120a NVFP4 cutlass GEMM` and `FlashAttention-2 prefill/decode: ENABLED for arch(es) [121a]` -- and `--list-test-cases` confirms all 3 new cases are in the built binary, so the pass is neither a degraded-build nor an absent-test artifact. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #505. Spec:
.agents/specs/dsa-topk-bounds.md. Row:MODEL-TEXT-deepseek-v4-deepseek-v4-for-causal-lm.The defect
DsaTopkKernelsized two thread-local arrays by literal:topkis the caller'sindex_topk— 512 on V4-Flash, 1024 on V4-Pro — sopicked[64]was 8x short for Flash and 16x short for Pro, andchosen[512]overflowed on any window wider than 512. Neither bound was asserted nor derived from config. Every pre-existing device case ran attopk=3, nk=5, which is why it was invisible: that's the gate's shape, not the model's.Measured, not theoretical
On
dgx.casa(GB10, sm_121a), the pre-fix kernel driven at Flash's ownindex_topkof 512 with a 600-wide window faults on the device:Line 214 is the new
REAL index_topk widthscase. After the fix, same flags:23 cases | 23 passed | 0 failed | 0 skipped,83913/83913 assertions,Status: SUCCESS!.Both arms ran under
flock $HOME/gpu.lock, each asserting its own kernel identity before building — matching the declarationsbool chosen[512];/int64_t picked[64];, not the tokens, since the fixed kernel's comment cites both by name and a token grep reports the fixed tree as the old one.origin/mainthen advanced 17 commits, so the suite was rebuilt and re-run from the merged tree: 23/23, 83913 assertions, SUCCESS, withCUTLASS found … enabling sm120a NVFP4 cutlass GEMMandFlashAttention-2 … ENABLED for arch(es) [121a]hard-verified in that run's own configure log, and--list-test-casesconfirming all 3 new cases in the binary.The fix — two passes, no scratch
A threshold formulation over the same total order the host reference sorts by (logit desc, then index asc — total because candidate indices are distinct):
topktimes to land on the topk-th best element, the threshold;Exactly
topkelements qualify under a total order, and they come out already ascending, so theO(topk^2)emit sort disappears along with the buffers. No per-thread scratch, no bound, no configurable limit. Pass 1 staysO(topk*n), so this is strictly cheaper overall.Two defensive additions that are explicitly not capacity limits: pass 1 stops if no strictly-worse element exists, and pass 2 carries
w < topk, so a NaN row — where every float comparison is false — cannot write past its own row into the next one, which is the failure class this issue was about.Gate
Three new device cases, all against the independent
DsaTopkSelectoracle (std::stable_sort based — a genuinely separate implementation, so this isn't a shared-helper tautology):index_topkwidthspicked[64], then both shipped widths; also asserts no-1leaks and strictly ascending emitws=137s - s0and its picks by absolutesPlus a local pre-flight while the GPU lock was held by other jobs (scratch, ASan+UBSan): the new body vs an independent transcription of the oracle — 0 mismatched entries over 8 named shapes and a 4000-shape randomized sweep, half coarsely quantized to force tie density. And an ASan proof of the defect itself: the old body at
topk=512, nk=600reportsstack-buffer-overflow, WRITE of size 1on thechosen[]init loop.CPU V4 suites unmoved: dsa 38, pro_variant 197, scaffold 62, all SUCCESS.
scripts/agent-preflight.sh --stagedfully clean at the pushed SHA.Rejected alternatives
Asserting the bounds instead of removing them — which #505 itself proposed. A refusal would be honest but would leave the device DSA path unable to run the real
index_topkat all, trading a latent overflow for a guaranteed refusal the moment the real-geometry residual lands. The threshold form needs no bound, so there is nothing left to assert.Sharing a selection helper between kernel and host reference — would remove the duplication and make a CPU test trivial, but would make the gate prove only self-consistency. The duplication is deliberate and recorded as a stop condition.
Scope
Latent, not shipped:
dsa_dense = (be.gguf != nullptr)(deepseek_v4.cpp:668) forces the indexer off on the real keep-quant path. This change makes the kernel able to represent the real widths; it does not put the real path on it. The real-geometry DSA sparse path stays a named residual, as does the compressed-key-space candidate window — upstream builds it aske = row_start + (pos+1) // COMPRESS_RATIO(v1/attention/backends/mla/indexer.py:270-290) while our synthetic path useswe[t] = t+1over uncompressed keys. Spec §5 records that for whoever builds it out.Review note
Authored in a coordinating session on direct developer instruction, so it has not had an independent review pass. A fresh reviewer re-running the RED arm on dgx is still owed.
🤖 Generated with Claude Code