Repository navigation
Add packed THD context parallelism for DSv4 - #7469
Conversation
Signed-off-by: Deyu Fu <deyuf@nvidia.com>
…260917 Signed-off-by: Deyu Fu <deyuf@nvidia.com>
Signed-off-by: Deyu Fu <deyuf@nvidia.com>
…260917 Signed-off-by: Deyu Fu <deyuf@nvidia.com>
Signed-off-by: Deyu Fu <deyuf@nvidia.com>
Signed-off-by: Deyu Fu <deyuf@nvidia.com>
Signed-off-by: Deyu Fu <deyuf@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test e17057c |
Signed-off-by: Deyu Fu <deyuf@nvidia.com>
|
/ok to test ef78927 |
Signed-off-by: Deyu Fu <deyuf@nvidia.com>
|
/ok to test 8e11d6b |
|
Hi @FDecaYed, a few notes from stacking #7705/#7708 on this:
|
| falls back to standard rolling behavior. | ||
| packed_seq_params (PackedSeqParams): Parameters for packed sequence processing. | ||
| If provided, respects sequence boundaries. | ||
| return_sum (bool): Whether to calculate and return the rolled tensor sum. |
There was a problem hiding this comment.
Documented cp_layout, fill_value and the contiguous-only shift/fill contract in 0f2ced4.
|
/review model=claude mode=strict |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
Summary
This is a strict review of the DSv4 packed-THD static context-parallel PR. I read all 17 changed files in the head tree. I also followed the paths that cross components:
- the hybrid attention, and the CSA compressor and indexer;
- the packed layout and the async all-gather/reduce-scatter (AG/RS) helpers;
- the MTP rolls, including the zigzag, CP1 and HSM paths;
- the config validation.
Findings: 0 CRITICAL · 0 IMPORTANT · 7 SUGGESTION (all inline)
- Four new args or branches that never take effect. The "every new arg has a real use path" check fails for each:
fill_value=Truein the zigzag MTPpadding_maskroll does nothing.compressed_baseandcompressed_is_sequence_majorinbuild_attention_indicesare never passed.- The packed branches of
_apply_unfused_ropecannot be reached. - The second operand of the guard in
compute_cp_indexer_topkis always false.
- Missing docs: the
roll_tensordocstring does not covercp_layoutorfill_value. This matches Phlip79's open request. - Lost diagnostic:
fused_csa_teacher_lsenow raises a genericValueError. The base raised aRuntimeErrorthat included the specific unsupported reason. - Untested path: every packed CP test fixes
apply_rope_fusion=True, so none runs the new unfused-RoPE code.
Risk: moderate. This is a large new CP surface. Neither the author nor I could run its GPU kernels or collectives. Reading the source, I found no correctness, process-group, CPU-GPU sync, CUDA-graph or backward-compatibility defect. Since nothing above is significant, the verdict is approve. Please check the suggestions before merging.
What I checked by reading the source (not by running it):
- Compressed-group ownership: each compressed group belongs to the rank that holds its last token. The 8-row halo always contains the ratio-4 overlap predecessor, so
is_first = group_id == 0is correct. - Row mapping:
seq_to_rank_rowmaps correctly into the capacity-padded, rank-major gathered buffer. Halo copies never enter the global KV domain. - Deferred reduce-scatter edges: they return each compressed-KV and indexer-K gradient exactly once. The gathered buffers are detached on the loss path.
- Loss scaling:
real_row_scalematchesloss_divisorfor the sparse loss, the dense loss and per-token loss. - Roll backward:
_RollLeftand_LeftBoundaryExchangesend boundary gradients back to the rank that owns the token. - Process groups: every P2P call and collective passes the explicit CP group.
- Host syncs: none were added. The code uses
torch._assert_async, and its boolean-mask scalar fills dispatch tomasked_fill. - Unchanged behavior: zigzag and CP1 MTP behave as before. For CP1, real and physical boundaries are the same tensor.
Not assessed: the PR discussion reports that bucketize(rows, cu[1:]) ignores the slice offset under torch.compile on some PyTorch builds. I cannot confirm this from source. _compiled_compactor and _compiled_attention_indices both use this pattern, so the 4-GPU test_dsv4_packed_cp.py run is what will show whether it holds here.
Review generated by claude
Adopt the full-prefix bucketize fix and metadata regressions from NVIDIA#7705. Simplify unused paths, document roll semantics, retain teacher diagnostics, and cover unfused RoPE. Explicitly reject packed deterministic_mode pending NVIDIA#7921. Co-authored-by: Yan Xu <yxu1@nvidia.com> Signed-off-by: Deyu Fu <deyuf@nvidia.com>
|
@Connor-XY Adopted #7705's boundary fix and metadata regressions in 0f2ced4, with co-author credit. Its scoped changes are now included here. Packed deterministic_mode now raises explicitly pending #7921, with #7708 as the shared/SBHD prerequisite. I kept the existing dependency pins in this PR; the deterministic follow-up can use main's newer runtime. New CI is running. |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/37944069001 |
What does this PR do?
DSv4 attention currently rejects CP > 1. This adds static context parallelism for packed THD inputs with
attention_cp_layout="contiguous"and TP=1, covering window attention, ratio-4 CSA, ratio-128 HCA, and MTP. Packed inputs requiredsa_kernel_backend="cudnn"; the existing SBHD native backend remains available.The implementation uses main's existing packing, process-group, RoPE, and static-spec interfaces. It preserves one dense indexer scoring call per CP-local query shard. Compact BF16/MXFP8 indexers and dynamic CP remain separate follow-ups; dependency versions are unchanged. This PR targets main independently of #6403.
Related work
Related to #5795 and #6404. Static CP behavior follows the existing dev CSA implementation, adapted to main's interfaces and pinned cuDNN Frontend.
Validation
git diff --check, and kernel determinism coverage checks. Local mypy still reports dependency/type-configuration errors.GPU validation targets:
Pre-checks
tools/autoformat.sh; blocking formatter/lint checks passed.