Repository navigation
[dev] Share the fused compact DSA indexer workspace across CSA layers - #8104
Conversation
Signed-off-by: HaochenYuan <haocheny@nvidia.com>
|
/review model=claude mode=strict |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
Strict review: share the fused compact DSA indexer workspace across CSA layers
Findings: CRITICAL: 0, IMPORTANT: 0, SUGGESTION: 2 (both posted inline)
Risk: low. Sharing one workspace across layers looks safe. I traced the workspace from the point it is written to every point it is read:
- Quantization buffers: the MXFP8 quantization destinations and packed scales are overwritten on every dispatch.
- Per-dispatch refresh:
cand_batch_offsetsand the MXFP8 scale offsets (cu_seqlens_*_scale_padded) are rewritten in place on every dispatch (fused_sparse_attention.py L1875-L1877, L1925-L1930). - Scratch and outputs: candidate scratch and the compact outputs are allocated per dispatch, and a check confirms the kernel wrote into those per-dispatch buffers (L540-L566, L1974-L1990).
- Backward: the sparse-attention backward saves
q,kv,topk_idxs,outandlse, never the workspace. - Ordering: with the local CUDA graph implementation, each graph's warm-up runs right before its own capture (cuda_graphs.py L930-L980). Changing who owns the "active" workspace therefore does not change which workspace a capture sees.
So a later layer overwriting the shared buffers is the same reuse that eager mode already does.
The new config field is read in _compact_indexer_workspace_store, so it is not dead code. Its --no-/--disable- CLI flags are generated automatically by ArgumentGroupFactory (argument_utils.py L199-L211, arguments.py L2861). The golden-config entry is updated.
Suggestions (inline):
- BSHD capture path: it still accepts only the active workspace, while the THD path now scans every warmed-up workspace. The full-recompute geometry alternation described in the new THD comment applies to BSHD unchanged. This already existed before the PR and is not caused by sharing. Mirroring the THD scan is a short change, and the patch is in the inline comment.
- Stale comments in
_get_thd_compact_indexer_workspace: two unchanged comments now contradict the code. The docstring says capture reuses only the active workspace. Another comment says each attention module owns its storage independently, which is no longer true with the default sharing.
Limitations:
- The skill's
style-guide.mdis not in the base tree (/context/base/style-guide.mdand/context/base/skills/style-guide.mdare both missing), so no style-guide audit was done. - I could not search the whole repo for code that assigns to the now read-only
_*_compact_indexer_workspacesattributes. I spot-checkedcuda_graphs.py,cp_balanced_indexer.pyand the DSv4 CP CUDA graph tests and found no such assignments. - None of the suggested code has been run.
Review generated by claude
| wrapper, avoiding an extra permutation during graph capture. | ||
| """ | ||
| precision = self.config.dsa_indexer_precision | ||
| self._compact_indexer_workspace_store(q.device) |
There was a problem hiding this comment.
[SUGGESTION Consistency] The BSHD capture path still accepts only the active workspace, while THD now scans every warmed-up workspace.
Failure. The new THD comment (L2359-L2362) says that under full activation recompute, the no-grad forward and its recompute alternate between two geometries. The BSHD path alternates the same way:
- No-grad forward:
forwardgoes to_forward_fused_indexer_inference, which asks for a workspace withreturn_softmax=False(L2607). - Recompute with grad: it goes to
_forward_fused_indexer_training, which usesreturn_softmax=indexer_loss_coeff > 0 and sparse_loss(L2674-L2680).
BSHDCompactIndexerWorkspace.matches_shape compares return_softmax. However, the capture branch (L2284-L2292) only tries self._active_bshd_compact_indexer_workspace.
Trigger: warm up with return_softmax=False, then warm up with True, then capture the False geometry. The capture raises ValueError: BSHD compact CUDA graph capture requires an eagerly prepared compact workspace..., even though a matching workspace is already in _bshd_compact_indexer_workspaces. With sharing on (the default), the active slot now holds the last warm-up done by any CSA layer on the device. The base code already checked only the active workspace, so this is not a regression from sharing. It is the same failure the PR fixes for THD, left in place for BSHD.
Correction. Replace the capturing branch of _get_bshd_compact_indexer_workspace with the same candidate scan the THD path uses:
capturing = torch.cuda.is_current_stream_capturing()
workspace = self._active_bshd_compact_indexer_workspace
if capturing:
# As in the THD path: the last warm-up may have prepared the other recompute
# geometry. Matching is shape-only (no host synchronization).
candidates = [workspace] if workspace is not None else []
candidates += [w for w in self._bshd_compact_indexer_workspaces if w is not workspace]
for candidate in candidates:
if candidate.matches_shape(**match_kwargs):
self._active_bshd_compact_indexer_workspace = candidate
return candidate
raise ValueError(
"BSHD compact CUDA graph capture requires an eagerly prepared compact "
"workspace for the active static geometry. Run eager warmup before capture."
)Invariant: during capture, return any eagerly prepared workspace whose static geometry matches. Never prepare one during capture, because preparing needs a host sync.
Preservation. The existing test test_bshd_compact_workspace_prepared_in_warmup_and_reused_in_capture (test_attention_variant_csa.py L900-L953) should still pass. Warm-up sees an empty list and prepares once; capture has exactly one candidate. So workspace.matches_shape.assert_called_once() and _bshd_compact_indexer_workspaces == [workspace] still hold. A capture with no matching warm-up still raises ValueError.
Acceptance (proposed test, not run).
- Setup: set
dsa_compact_indexer_workspace_sharing=Falseand patchbshd_compact_indexer_availableto returnTrue. - Patch
prepare_bshd_compact_indexer_workspacewithside_effect=[ws_f, ws_t]. These are MagicMocks whosematches_shapereturnskwargs["return_softmax"] is Falseandis Truerespectively. - Patch
is_current_stream_capturingwithside_effect=[False, False, True]. - Steps: call with
return_softmax=False, thenTrue, then capture withFalse. - Expected: the third call returns
ws_fandpreparewas called twice. Today the third call raisesValueError.
| clearly. Unsupported devices/frontends retain the non-compact path. | ||
| """ | ||
| precision = self.config.dsa_indexer_precision | ||
| self._compact_indexer_workspace_store(q.device) |
There was a problem hiding this comment.
[SUGGESTION Comments] Two unchanged comments in this function now contradict the new behavior:
- Docstring (L2331-L2334): it says "Capture reuses the active workspace". Capture now tries the active workspace first and then every warmed-up workspace in the slot (L2363-L2385).
- Comment at L2344-L2346: it says "...just as each attention module owns storage independently of the other layers". With
dsa_compact_indexer_workspace_sharing=True(the new default), modules no longer own their storage independently. Only the separation between head and tail slots remains.
A wrong comment here will mislead the next person debugging a capture failure. Suggested replacement for both:
"""Return persistent compact storage for a warmed-up THD graph geometry.
Workspace sizing synchronizes with the host, so it is performed only
during eager CUDA-graph warmup. Capture reuses the active workspace or any
other warmed-up workspace of the same slot whose static geometry matches;
when none matches (warmup skipped or static shapes changed), capture must
fail clearly. Unsupported devices/frontends retain the non-compact path.
"""
...
# Head and tail have different causal offsets and may have different
# geometry. Keep their quantization/candidate metadata in separate slots;
# each slot is shared by every CSA layer bound to the same workspace store.Acceptance (static check): the docstring and comment should match the capture branch at L2358-L2389 and the binding logic in _compact_indexer_workspace_store. Behavior does not change.
Signed-off-by: HaochenYuan <haocheny@nvidia.com>
Graph slot memory protects only tensors saved for backward. With dynamic-CP graph banks, a replay of another bank can overwrite tensors that a forward kept in ctx attributes before the matching backward runs. The CSA sparse attention functions, the cuDNN sparse attention functions, FusedDSAIndexerLoss and the fused MLA RoPE in-place, concat and KV-split functions now pass that metadata through save_for_backward. Dynamic-CP capture walks several CP banks in sequence. It relies on CSA capture reusing any warmed-up compact-indexer workspace that matches the geometry (NVIDIA#8104); the tests cover this for BSHD and THD. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tailai Ma <tailaim@nvidia.com>
Graph slot memory protects only tensors saved for backward. With dynamic-CP graph banks, a replay of another bank can overwrite tensors that a forward kept in ctx attributes before the matching backward runs. The CSA sparse attention functions, the cuDNN sparse attention functions, FusedDSAIndexerLoss and the fused MLA RoPE in-place, concat and KV-split functions now pass that metadata through save_for_backward. Dynamic-CP capture walks several CP banks in sequence. It relies on CSA capture reusing any warmed-up compact-indexer workspace that matches the geometry (NVIDIA#8104); the tests cover this for BSHD and THD. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tailai Ma <tailaim@nvidia.com>
Graph slot memory protects only tensors saved for backward. With dynamic-CP graph banks, a replay of another bank can overwrite tensors that a forward kept in ctx attributes before the matching backward runs. The CSA sparse attention functions, the cuDNN sparse attention functions, FusedDSAIndexerLoss and the fused MLA RoPE in-place, concat and KV-split functions now pass that metadata through save_for_backward. Dynamic-CP capture walks several CP banks in sequence. It relies on CSA capture reusing any warmed-up compact-indexer workspace that matches the geometry (NVIDIA#8104); the tests cover this for BSHD and THD. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tailai Ma <tailaim@nvidia.com>
Graph slot memory protects only tensors saved for backward. With dynamic-CP graph banks, a replay of another bank can overwrite tensors that a forward kept in ctx attributes before the matching backward runs. The CSA sparse attention functions, the cuDNN sparse attention functions, FusedDSAIndexerLoss and the fused MLA RoPE in-place, concat and KV-split functions now pass that metadata through save_for_backward. Dynamic-CP capture walks several CP banks in sequence. It relies on CSA capture reusing any warmed-up compact-indexer workspace that matches the geometry (NVIDIA#8104); the tests cover this for BSHD and THD. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tailai Ma <tailaim@nvidia.com>
What does this PR do?
Under CUDA graphs the fused compact DSA indexer (
--dsa-kernel-backend cudnn,--dsa-indexer-precision mxfp8) keeps a persistent workspace per static geometry (MXFP8 q/k quantization destinations, packed scales, candidate offsets) that must be prepared outside capture. Every CSA layer owned its own copy: about 7 GiB per rank on DSv4 at 16K tokens per rank.TransformerConfignow share one workspace per geometry and balanced-indexer slot (_CompactIndexerWorkspaceStore, keyed on the config). The layers of a graph run serially on one stream and nothing in the workspace outlives the indexer dispatch that fills it, so sharing only removes duplicates.dsa_compact_indexer_workspace_sharing(defaultTrue;--no-dsa-compact-indexer-workspace-sharingrestores per-layer workspaces), golden-config entry, doc paragraph incuda_graph.md.test_csa_compact_indexer_workspace_sharing.py) and a contract test in the CSA test class that a workspace prepared by one layer serves the capture of another.Validation: GB200, 4 ranks: CSA test files 118 passed. DSv4-flash 1M on 128 GPUs with chunk-wise graphs: last-stage NVML memory 173.8 → 149.4 GiB together with linear CE (#5258 validation).
Issue tracking
For PRs from open-source community contributors:
Linked issue:
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.