[dev] [DeepSeek-v4] Context Parallel support - #5087
Conversation
|
/claude strict-review |
There was a problem hiding this comment.
Code Review Summary
PR: [dev] [DeepSeek-v4] Context Parallel support
Files reviewed: 15 (8003 additions, 116 deletions)
Findings: CRITICAL: 0, IMPORTANT: 3, SUGGESTION: 4
Overview
This PR adds context parallel (CP) support for the DeepSeek-v4 (DSv4) hybrid attention architecture. The implementation introduces:
- CP partition modes —
contiguous(default) andtwo_chunk(load-balanced for long sequences) - Boundary hidden exchange — P2P communication of window rows between CP ranks via
_LeftBoundaryExchangeautograd function - CP-aware compressor/indexer — All-gather of compressed KV, repacking from rank-major to sequence-major order, and CP-aware top-K indexer scoring
- CuTeDSL layout kernels — Fused RoPE, KV packing, compressor input preparation, and attention index building for CP layouts
- Data schedule integration — DSv4-specific CP row partitioning in
get_cp_slice_for_thd
The architecture is well-structured: csa_cp_utils.py provides the MCore-facing API layer, csa_cp_layout_kernels.py contains the CuTe device code, and the existing csa.py / deepseek_v4_hybrid_attention.py files are extended with CP dispatch logic. Process groups are passed through via ProcessGroupCollection or explicit cp_group parameters, following the CLAUDE.md guidance.
Most Impactful Findings
-
Inconsistent
getattrusage indata_schedule.py— Theexperimental_attention_variantattribute is accessed withgetattr(config, ..., None)in one place but directly asconfig.experimental_attention_variantin another. The direct access will raiseAttributeErrorifconfigdoesn't have the attribute. -
Missing padding-row exclusion in
FusedIndexerSparseAttnFromTopkFunc— The new CP-specific autograd function lackscu_seqlens_q_unpaddedsupport for excluding CUDA-graph padding rows from the indexer loss. The existing non-CP counterpart (FusedIndexerSparseAttnFunc) has this support. This could cause slight training loss pollution when CP is used with CUDA graph capture and max-padded packing. -
Dual indexer-loss implementations — The unfused CP path (
_unfused_indexer_sparse_attn_from_topk) reimplements the target/predict KL loss computation independently from the shared helpers indsa_kernels.py. Two independent implementations of the same math increase maintenance risk.
Risk Assessment
Medium risk. The core CP communication patterns (P2P boundary exchange, all-gather compressed KV) are correctly implemented with proper autograd backward support. The RoPE position mapping, KV packing, and index lowering kernels include appropriate validation. The new code is well-contained behind cp_size > 1 guards and does not alter single-rank behavior. The two-chunk partition mode adds complexity but is backed by comprehensive unit tests (1900+ lines of new test code).
The main risks are: (a) the padding-row exclusion gap for CUDA graph compatibility, and (b) the dual loss implementations potentially diverging over time.
Signed-off-by: kunlunl <kunlunl@nvidia.com>
55ee782 to
b83d57e
Compare
|
/ok to test b83d57e |
Signed-off-by: kunlunl <kunlunl@nvidia.com>
|
/ok to test d4832db |
|
/ok to test e1198f6 |
@xlsunstar Thanks for flagging this and for suggesting a fix. |
Signed-off-by: kunlunl <kunlunl@nvidia.com>
chtruong814
left a comment
There was a problem hiding this comment.
If possible, some of the dependency updates should be handled in pyproject toml and uv lock. I'd like @balasaajay and @ko3n1g to weigh in on bumping to the cudnn frontend commit.
| COPY megatron/core/package_info.py /workspace/megatron/core/ | ||
| ARG IMAGE_TYPE=dev | ||
| ARG CUDNN_FRONTEND_COMMIT=0a14b7181d129d30e7bad34b8c3ed0a0c995e23d | ||
| ARG FLASH_MLA_COMMIT=b7643bd54521f563b839b98289b5cd048c062ba2 |
There was a problem hiding this comment.
I think flash_mla is already using the commit you want
https://github.com/NVIDIA/Megatron-LM/blob/main/pyproject.toml#L228
There was a problem hiding this comment.
Oh this PR is going into dev branch. I would just cherry pick this change from main branch
https://github.com/NVIDIA/Megatron-LM/pull/5448/changes
There was a problem hiding this comment.
Thanks for pointing this out. I mirrored the change from #5448 on the dev branch, pyproject.toml now tracks FlashMLA nv_dev, and uv.lock resolves it to b7643bd. I also removed the manual FlashMLA installation from the Dockerfile.
| uv pip install --no-build-isolation \ | ||
| "flash-mla @ git+https://github.com/deepseek-ai/FlashMLA.git@${FLASH_MLA_COMMIT}" \ | ||
| "nvidia-cudnn-frontend[cutedsl] @ git+https://github.com/NVIDIA/cudnn-frontend.git@${CUDNN_FRONTEND_COMMIT}" \ | ||
| "torch-c-dlpack-ext==0.1.5" |
There was a problem hiding this comment.
Please use the pyproject.toml and uv.lock to manage dependencies if possible
There was a problem hiding this comment.
Done. FlashMLA and cuDNN Frontend are now declared in pyproject.toml, with the exact revisions and the CuTe DSL / DLPack transitive dependencies captured in uv.lock. The Dockerfile now installs this dependency chain through uv sync --locked; the separate manual installation has been removed.
| COPY megatron/core/__init__.py /workspace/megatron/core/ | ||
| COPY megatron/core/package_info.py /workspace/megatron/core/ | ||
| ARG IMAGE_TYPE=dev | ||
| ARG CUDNN_FRONTEND_COMMIT=0a14b7181d129d30e7bad34b8c3ed0a0c995e23d |
There was a problem hiding this comment.
@kunlunl Do we know when cudnn-frontend will include this in a release? Does this need to be included in next 26.08 Megatron-Bridge container release? Is the code backwards compatible to handle if cudnn frontend is not at this commit?
@balasaajay @ko3n1g any concerns?
There was a problem hiding this comment.
@balasaajay @ko3n1g I didn't realize this was going into dev branch. I don't think much of a concern on the specific cudnn frontend used for dev branch, right? Main recommendation would still be to move what we can to pyproject toml and uv lock.
There was a problem hiding this comment.
Thanks for checking. Since this PR targets the dev branch, I pinned the required cuDNN Frontend commit in pyproject.toml / uv.lock for the dev environment.
Older cuDNN Frontend versions remain usable for paths that do not require the new CP offset API. When ratio-4 fused CP is enabled, TransformerConfig now checks that the required wrappers expose q_causal_offsets and raises a clear initialization error if they do not, with the option to disable DSA kernel fusion.
| RUN uv pip install --reinstall --no-deps \ | ||
| nvidia-cutlass-dsl==4.5.0 \ | ||
| nvidia-cutlass-dsl-libs-base==4.5.0 \ | ||
| nvidia-cutlass-dsl-libs-cu13==4.5.0 |
There was a problem hiding this comment.
Why do we need to reinstall? Can we handle this in pyproject toml and uv lock?
There was a problem hiding this comment.
@balasaajay @ko3n1g btw, looks like we had to do this in MBridge container though I can't recall why.
NVIDIA-NeMo/Megatron-Bridge@9e06ad9
There was a problem hiding this comment.
The explicit reinstall is no longer needed and has been removed. nvidia-cudnn-frontend[cutedsl] now resolves the required CuTe DSL 4.5.0 packages through uv.lock.
Signed-off-by: kunlunl <kunlunl@nvidia.com>
|
/ok to test 3599a37 |
yuzhongw-nvidia
left a comment
There was a problem hiding this comment.
LGTM! Thanks for your incredible work!
Signed-off-by: kunlunl <kunlunl@nvidia.com>
|
/ok to test 1543950 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/28663237378 |
What does this PR do ?
Add CP support for DS v4
Depends on #5011 (Merged)
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.