Repository navigation
Conversation
b62cdc0 to
03132fa
Compare
|
/review |
03132fa to
ba04da6
Compare
|
/ok to test ba04da6 |
1 similar comment
|
/ok to test ba04da6 |
|
/review |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
Static review completed for all five changed files, including the full-fusion and split indexer callers. No tests were executed.
1. The fully fused attention path ignores FP8 scoring
Failure: With dsa_kernel_backend="cudnn", dsa_indexer_scoring_precision="fp8", zero indexer loss, and supported absorbed-MLA shapes, DSAttention.forward attempts full fusion before the newly updated split top-k hook. That route reaches FusedIndexerSparseAttnFunc.forward, which calls _indexer_topk_bshd without scoring_precision. It therefore scores in BF16 and returns before the FP8-aware split path runs. The advertised precision switch silently has no effect on this supported route.
Correction: Either thread the precision through the full-fusion wrapper and autograd contract, including its backward return arity, or deliberately use the already precision-aware split selector for FP8. The smallest correction is this insertion immediately after _assert_supported_indexer_scoring(use_relu) in the cuDNN full-fusion hook:
if getattr(config, "dsa_indexer_scoring_precision", "bf16") == "fp8":
return NoneThis lets DSAttention continue through the split FP8 selector while retaining fused sparse attention afterward. It also preserves compatibility with existing test config stubs that lack the new field.
Preservation and acceptance: Add a proposed DSAttention integration regression using attention_backend=auto, no index sharing/KPool, causal absorbed MLA with value width 512, sequence length 128, and top-k 64. Spy on scoring and assert that FP8 configuration reaches FP8 scoring, including with rope_fp32=True; exercise both plain causal and packed inputs. The BF16 configuration must still use full fusion. Retain split-attention backward handling of invalid slots and supplied lengths: the existing sanitization regression must continue to pass. Suggested code requires author verification.
2. Global-row scoring does not honor BF16 precision for FP32 carriers
Failure: With dsa_indexer_rope_fp32=True and BF16 scoring, _indexer_forward rounds q/k to BF16 at the scoring boundary. However, the global-row helper leaves FP32 carriers unrounded. Thus explicit-varlen or chunked scoring uses a different operand precision. A minimal helper-only example with one head/dimension, q=k=1.001, weight 1, ratio 1, scale 1, and one valid key returns approximately 1.002001; BF16 scoring must return 1.0, because both operands round to 1.
Correction: Replace the current elif scoring_precision != "bf16" branch in that helper with:
elif scoring_precision == "bf16":
q_chunk_bshd = q_chunk_bshd.to(torch.bfloat16)
if k_bshd.dtype != torch.bfloat16:
k_bshd = k_bshd.to(torch.bfloat16)
k_bdk = None
else:
raise ValueError(
f"Unsupported DSA indexer scoring precision {scoring_precision!r}; "
"expected 'bf16' or 'fp8'."
)Invalidating k_bdk for an FP32 key is necessary: the caller currently caches the unrounded key. The invariant is BF16-rounded scoring operands regardless of preprocessing carrier dtype or chunking.
Preservation and acceptance: Proposed tests should assert 1.0 for the example both without a cache and with a cache built from the original FP32 key. Already-BF16 inputs must retain their reusable cache and unchanged scores; FP8 must retain its E4M3 branch. Preserve global causal offsets and distinct rows with repeated varlen ends: the existing repeated-ends regression must still produce indices [[1,0],[0,1]], lengths [2,2], and scores [[2,1],[-1,-2]]. Suggested code requires author verification.
3. Add the required registered bit-exact replay coverage
Failure: This adds a new cuDNN FP8 kernel dispatch, but no determinism test or manifest update accompanies it. The added scoring tests compare implementations once or allow 98% top-k overlap; neither checks repeated byte-identical results. This is required by the kernel testing contract. Independently, the changed DSA preprocessing source is already registered to a determinism test that this PR does not update, which violates the coverage gate's test-update check.
Correction and acceptance: Add SM90+/dependency-guarded replay cases to the registered determinism test module, and register the cuDNN scoring source in the manifest. Using the same (1,128,32,128) q and (1,128,1,128) k setup as the added scoring tests, the scoring replay core can be:
for precision in ("bf16", "fp8"):
assert_replays_bit_exact(
lambda qq, kk, ww: kern._indexer_forward(
qq, kk, ww, ratio=1, sm_scale=1.0, scoring_precision=precision
),
(q.float(), k.float(), w),
replays=3,
backward=False,
contention=True,
what=f"DSA indexer {precision}",
)Also replay top-k outputs and the new FP32 preprocessing route, asserting that q/k remain FP32 while weights remain BF16. Retain BF16/default replay cases and the existing zero-centered-gamma behavior. Expected outcomes are byte-identical replay outputs and a passing manifest/coverage check—not merely approximate reference agreement. This is proposed coverage, not evidence of executed tests.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
dd63550 to
a7dcf09
Compare
|
/ok to test a7dcf09 |
@RayenTian, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test a7dcf09 |
a7dcf09 to
e16c82a
Compare
|
/ok to test e16c82a |
e16c82a to
c1eb741
Compare
Add dsa_indexer_rope_fp32 (indexer query/key LayerNorm -> RoPE stays in FP32, no intermediate BF16 round-trip) and dsa_indexer_scoring_precision (bf16|fp8, E4M3 operands with UE8M0 per-vector scales). Route all cuDNN indexer scoring through a single _indexer_forward helper and thread the precision through the split top-k paths instead of patching the cuDNN namespace at runtime. The global-row score path rounds FP32 carriers to BF16 to match the dense path. Defaults preserve existing behavior. Signed-off-by: ruit <ruit@nvidia.com>
DSAttention's full-fusion route (FusedIndexerSparseAttnFunc) scored the indexer in BF16 regardless of dsa_indexer_scoring_precision, so FP8 silently had no effect there. Thread scoring_precision through run_fused_dsa_attention, fused_indexer_sparse_attn, and the autograd function (forward signature plus backward return arity). Add a cuDNN FP8 full-fusion integration test exercising forward and backward. Signed-off-by: ruit <ruit@nvidia.com>
Register dsa_cudnn_kernels.py in the kernel determinism manifest and add bit-exact replay cases for the BF16/FP8 _indexer_forward scoring choke point and the FP32-carrier index-score path, satisfying the coverage gate. Signed-off-by: ruit <ruit@nvidia.com>
c1eb741 to
3f7e796
Compare
|
/ok to test 3f7e796 |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
LGTM for this light follow-up. Verified the earlier findings against the current source:
- Full fusion now forwards scoring precision through the wrapper and autograd contract to indexer selection, with matching backward return arity.
- Global-row BF16 scoring now rounds FP32 carriers and invalidates the unrounded key cache; the added regression covers cached and uncached inputs.
- The cuDNN scoring dispatch now has BF16/FP8 bit-exact replay coverage and a corresponding manifest entry.
No unresolved prior findings or eligible new regressions identified. Review was limited to the prior findings, supplied fix evidence, and directly affected paths; the previously reviewed tree was unavailable for an exact inter-review comparison. Static inspection only—tests were not executed.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a findings-only review. Request a full review with /review reset=true.
What does this PR do?
Adds two orthogonal, opt-in precision options for the cuDNN DSA (DeepSeek Sparse Attention) lightning indexer. Both default to the existing behavior, so models that don't set them are unaffected.
dsa_indexer_rope_fp32(bool, defaultFalse) — keeps the indexer query/key preprocessing (key LayerNorm → RoPE) in FP32 end-to-end, with no intermediate BF16 round-trip before the scoring matmul.dsa_indexer_scoring_precision("bf16" | "fp8", default"bf16") — runs the cuDNN indexer scoring matmul in BF16 (unchanged) or FP8 (E4M3 operands with UE8M0 per-vector scales)."fp8"requiresdsa_kernel_backend="cudnn"anddsa_indexer_loss_coeff=0; SM90+ is enforced by the existing cuDNN backend dependency check.Motivation
In RL post-training (e.g. NeMo-RL) that uses vLLM as the rollout/generation backend, the indexer's top-k selection must match between training and generation. The top-k boundary is sensitive to the precision of both the q/k preprocessing and the scoring matmul:
That precision gap flips some top-k picks near the boundary, so training recomputes log-probs over a different key set than generation actually selected — a train/inference mismatch that hurts RL convergence.
These two options make that precision configurable so the training side can reproduce the FP8 inference indexer's top-k selection bit-for-bit. The switches are orthogonal:
dsa_indexer_rope_fp32removes the BF16 round-trip in preprocessing,dsa_indexer_scoring_precisioncontrols the scoring matmul precision. Megatron Core only exposes the mechanism (both default off); enabling them for a given model/runtime is a downstream concern — e.g. NeMo-RL turns them on for its vLLM rollout path.Implementation
_indexer_forwardchoke point indsa_cudnn_kernels.py, which dispatches onscoring_precision(BF16 cast vs. E4M3 quantization +precision="fp8"/q_scale/k_scale).scoring_precisionis threaded through every top-k path (including the pure-torch global-row path) rather than monkey-patching the cuDNN namespace at runtime.forward_before_topkkeeps q/k in FP32 underdsa_indexer_rope_fp32."fp8"to the cuDNN backend with zero indexer loss.rope_fp32) is byte-identical to before — covered by a regression test.Test
End-to-end (training)
Split Path
Validated both options on a real MoE + MLA + DSA training path, using a toy GLM-5 (
glm_moe_dsa) model built through Megatron-Bridge:index_topk=256,seq_length=2048(>index_topk, so the sparse top-k truncation is actually exercised), random weights, mock data, 2×H100 (TP=1 / PP=1 / EP=2).bf16scoring, norope_fp32;dsa_indexer_rope_fp32=true;dsa_indexer_scoring_precision=fp8+dsa_indexer_loss_coeff=0(cuDNN backend).dsa_indexer_rope_fp32=true+dsa_indexer_scoring_precision=fp8+dsa_indexer_loss_coeff=0(cuDNN backend).dsa_kernel_backend,scoring_precision,rope_fp32,loss_coeff);fp8scoring path is actually hit at runtime — the E4M3 quantization/round-trip of q/k runs inside the cuDNN indexer top-k path (verified with temporary instrumentation on the scoring choke points, both the single-chunk_indexer_forwardand the chunked global-row path);rope_fp32, the indexer q/k carry FP32 through preprocessing;fp8vs.bf16on identical inputs, while the default path is unchanged;Fused Path
Validated the same two options on the fully fused cuDNN DSA path (
run_fused_dsa_attention→FusedIndexerSparseAttnFunc, which fuses indexer scoring + FlashMLA sparse attention + the optional indexer loss into one autograd Function), again on a toy GLM-5 (glm_moe_dsa) through Megatron-Bridge:kv_lora_rank=512(key width 576) anddsa_indexer_n_heads=32; 2×H100 (TP=1 / PP=1 / EP=2), mock data, 200 steps.bf16vs.fp8indexer scoring, both withdsa_indexer_loss_coeff=0on the cuDNN backend, differing only bydsa_indexer_scoring_precision.run_fused_dsa_attentionreturns a result andFusedIndexerSparseAttnFunc.forwardruns on every rank — confirmed with temporary instrumentation;scoring_precisionis threaded all the way into the fused forward (showsbf16/fp8respectively);fp8vs.bf16on identical inputs (different top-k index checksums), confirming the E4M3 quantization is effective on this path;{32, 64}(bf16 additionally allows16);dsa_indexer_n_heads=16runs in bf16 but asserts under fp8 (qhead_per_kv_head must be one of (32, 64)).End-to-end (RL)
Undergoing...
Issue tracking
Linked issue: N/A — incremental DSA indexer precision option.
Contribution process
Pre-checks
jit_fuser/torch.compile, CUDA extension, TE or external-library dispatch, or a scatter/index accumulation), I have added or updated its bit-exact determinism test and registered it intests/unit_tests/determinism/kernels/manifest.py(guide)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.