[dev] [DeepSeek-V4] Add compact BF16 and MXFP8 DSA indexer support - #5992
[dev] [DeepSeek-V4] Add compact BF16 and MXFP8 DSA indexer support#5992hxbai wants to merge 7 commits into
Conversation
|
/claude strict-review |
| if is_thd: | ||
| if mxfp8_workspace is not None: | ||
| cu_seqlens_q_scale_padded = mxfp8_workspace.cu_seqlens_q_scale_padded | ||
| cu_seqlens_k_scale_padded = mxfp8_workspace.cu_seqlens_k_scale_padded | ||
| assert cu_seqlens_q_scale_padded is not None | ||
| assert cu_seqlens_k_scale_padded is not None |
There was a problem hiding this comment.
[IMPORTANT Implementation] THD MXFP8 scale prefixes are not refreshed across CUDA-graph replays, unlike cand_batch_offsets.
What: For the THD compact path with a workspace, cu_seqlens_q_scale_padded / cu_seqlens_k_scale_padded are read verbatim from the workspace (lines 1286–1289), which was populated once during eager warmup from the warmup-time cu_seqlens_q (see prepare_thd_compact_indexer_workspace, line 557–558). But quantize_indexer_mxfp8 is then called with the live cu_seqlens=cu_seqlens_q (line 1300) together with these stale padded prefixes. Inside _pack_indexer_mxfp8_scale_thd_kernel, seq_start/seq_len come from the live cu_seqlens_ptr while scale_row_start comes from the stale cu_seqlens_scale_padded_ptr — the two must be mutually consistent for the byte placement to be correct.
Why it matters: Just above, _refresh_thd_compact_cand_batch_offsets (line 1242) re-derives cand_batch_offsets from the live cu_seqlens_q on every dispatch, and the docstring at prepare_thd_compact_indexer_workspace line 548 explicitly states "Sequence boundaries may change between graph capture and replay while tensor shapes and maxima remain static." If that contract holds, then when per-segment boundaries differ on replay, the scale prefixes go stale: scales land at wrong physical rows, and because the per-sequence padding (make_indexer_mxfp8_scale_cu_seqlens rounds each segment up independently) can change the padded total, the preallocated q_scale/k_scale buffer can even be undersized → out-of-bounds writes. The BF16 path is unaffected (no scales), which is why existing THD graph tests wouldn't catch it.
Fix: Refresh the padded scale prefixes from the live cu_seqlens_q/cu_seqlens_kv on each dispatch (writing into caller-owned workspace storage, mirroring _refresh_thd_compact_cand_batch_offsets), and size q_scale/k_scale for the geometry-wide worst case rather than the warmup boundaries. Alternatively, if the THD MXFP8 path is only ever entered with statically-padded cu_seqlens_q (constant across replays), assert that invariant here and document why the scale prefixes need no refresh while cand_batch_offsets does.
Review Summary — Compact BF16 & MXFP8 DSA indexer supportReviewed the full diff with focus on implementation correctness, TP/PP/SP/CP parallelism, CUDA-graph capture/replay, backward compatibility, Megatron Core process-group usage, and unused new identifiers. Findings by severityIMPORTANT (1)
Areas checked and found clean
Overall risk assessmentLow–moderate. The change is large but well-isolated behind the new |
Signed-off-by: Hongxiao Bai <hongxiaob@nvidia.com>
Signed-off-by: Hongxiao Bai <hongxiaob@nvidia.com>
Signed-off-by: Hongxiao Bai <hongxiaob@nvidia.com>
Signed-off-by: Hongxiao Bai <hongxiaob@nvidia.com>
Signed-off-by: Hongxiao Bai <hongxiaob@nvidia.com>
|
/claude strict-review |
There was a problem hiding this comment.
Review Summary — Compact BF16 & MXFP8 DSA Indexer (#5992)
I performed a strict correctness/distributed/performance/back-compat review of the six production files (indexer_quantization.py, csa.py, csa_cp_utils.py, csa_kernels.py, transformer_config.py, arguments.py) plus the accompanying tests.
Findings by severity
- CRITICAL: 0
- IMPORTANT: 0
- SUGGESTION: 0
No blocking issues found.
What was verified
- Dead code / unused args (mandatory check): every new parameter, workspace field, and return-tuple element is consumed.
compact_predict,deterministic,precision,return_softmax,use_compact,compact_workspace,indexer_precision, the_*_compact_indexer_workspaceslists, and_active_*slots all have live readers. - Autograd arity:
FusedCSAIndexerSparseAttnFromTopkFunc.forwardtakes 19 inputs;backwardreturns exactly 19 gradients (3 attn grads +None+ 3 indexer grads + 12None). Matches. - Variable return arity:
indexer_topkreturns a 2- or 3-tuple gated onreturn_softmax; all three production callers (csa.py:1990,csa.py:2431,csa_cp_utils.py:439) unpack the correct shape (the two non-CP callers omitreturn_softmax, defaulting toFalse). - CUDA-graph capture safety: workspaces are sized/prepared only during eager warmup (host syncs guarded by
is_current_stream_capturing()), validated during capture via metadata-onlymatches/validate(no.item()), and the compact kernel's output buffers are asserted to alias the caller-owned storage bydata_ptr(). - Buffer sizing: THD scale capacity (
indexer_mxfp8_thd_scale_capacity) and candidate buffer (max(cand_floats, total_q·max_seqlen_k)) are provable upper bounds over any in-shape sequence layout. - Triton scale packers: BSHD kernel masks source loads (
other=0) and bounds stores; THD kernel's grid + 512-byte tiling makes every store in-bounds by construction; the binary search overcu_seqlens_scale_paddedusesbit_length()steps. - Precision fallback: MXFP8 hard-errors when compact support is absent; BF16 warns and falls back to the dense forward → radix Top-K path, which returns dense scores so the sparse-loss branch can still gather
predict. - Config validation:
dsa_indexer_precisionMXFP8 gate correctly requirescudnnbackend, SM100+, 64 heads / head_dim 128, and sparse loss; cuDNN Frontend wrapper compatibility is probed viainspect.signature(import present). - Megatron Core process groups: no new direct
parallel_state.get_*_group()reads introduced. - Rename hygiene:
_build_cp_indexer_layout→build_cp_indexer_layoutwith no stale references anywhere in the tree.
The implementation is defensively coded and internally consistent. Nice work.
Signed-off-by: Hongxiao Bai <hongxiaob@nvidia.com>
What does this PR do?
Adds the SM100 compact cuDNN indexer forward + Top-K path for DeepSeek-V4 compressed sparse attention, including BF16 and MXFP8 execution, CUDA-graph-safe workspaces, and THD context-parallel support.
Rely on NVIDIA/cudnn-frontend#370
Key changes
--dsa-indexer-precision={bf16,mxfp8}and validates the MXFP8 hardware, geometry, loss, and dependency requirements.Issue tracking
For PRs from open-source community contributors:
Linked issue:
Contribution process
Pre-checks
Code review
Feel free to message or comment @NVIDIA/mcore-oncall to help accelerate your merge into main. The less complex your PR is, the faster it will be approved and merged!
All PRs start as draft. If you open a non-draft PR, it will be automatically converted to draft.
Step 1: Mark PR as "Ready for Review"
.github/CODEOWNERS.Final Review might get declined if these requirements are not fulfilled.
Step 2: Final Review
For PRs that change
megatron/core, once all expert reviewers have approved, theFinal Reviewlabel is applied automatically and final reviewers are assigned.For PRs outside
megatron/core, this step is skipped.Step 3: Approved
Once all required reviewers have approved, the
Approvedlabel is applied automatically.Merge
Any member of mcore-engineers will be able to merge your PR.