Skip to content

Support HSDP deferred DP-outer gradient reduction#5743

Merged
wujingyue merged 17 commits into
NVIDIA:mainfrom
Achyuthan-S:fsdp/hsdp-grad-reduce
Jul 20, 2026
Merged

Support HSDP deferred DP-outer gradient reduction#5743
wujingyue merged 17 commits into
NVIDIA:mainfrom
Achyuthan-S:fsdp/hsdp-grad-reduce

Conversation

@Achyuthan-S

@Achyuthan-S Achyuthan-S commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds HSDP DP-outer gradient synchronization to the experimental Megatron-FSDP path, building on the microbatch context from #5652. Partially addresses #5616.

  • Relax the main_grad/main_weight placement guard so main_grad may rest at a DP-outer-Partial accumulation state (Partial on an axis where main_weight is Replicate).
  • Gate the gradient reduction on is_last_microbatch: reduce-scatter DP-inner and accumulate into main_grad every backward; reduce DP-outer and install the sharded parameter gradients only on the last microbatch.
  • Compose the reduction one mesh axis at a time so a 2-D DP mesh can reduce-scatter DP-inner and then reduce DP-outer.
  • Thread is_last_microbatch from the FSDP context through post_backward.
  • Add a 2-D-mesh HSDP parity test driven via microbatch(...).

Scope and design notes (draft — feedback welcome)

  • HSDP only: parameters and optimizer stay DP-outer-replicated ([Replicate, Flat]); gradients rest [Partial, Flat]. HFSDP (optimizer sharded across DP-outer) is a follow-up.
  • The last-microbatch DP-outer reduction produces a per-step reduced-gradient buffer for the optimizer; a persistent/reused buffer could replace it.
  • The gradient placement's DP-outer Partial reduce op must match the collective op (AVG). Symmetric-memory reduction across more than one axis is not yet supported (raises NotImplementedError).
  • Single stream; overlapping DP-outer communication on a separate stream is orthogonal (see Overlap experimental FSDP communication #5416 / Add forward all-gather overlap #5513).

Testing

Run on a 2×2 mesh (4 ranks):

python -m torch.distributed.run --standalone --nproc-per-node 4 -m pytest -q
tests/unit_tests/distributed/mfsdp_v2/test_fully_shard.py

Covers HSDP parity against single-rank SGD for num_microbatches in {1, 3}; the existing 1-D FSDP path is unchanged.
cc @wujingyue @ahmadki

Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>
@Achyuthan-S
Achyuthan-S requested review from a team as code owners July 10, 2026 15:52
Copilot AI review requested due to automatic review settings July 10, 2026 15:52
@Achyuthan-S
Achyuthan-S requested a review from a team as a code owner July 10, 2026 15:52
@copy-pr-bot

copy-pr-bot Bot commented Jul 10, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

Copy link
Copy Markdown
Contributor

This PR has been automatically converted to draft because all PRs must start as drafts.

When you are ready for review, click Ready for Review to begin the review process. This will:

  1. Add the oncall reviewer (optional reviewer)
  2. Add required review teams based on your changes

See the contribution guide for more details.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the experimental Megatron-FSDP (“MFSDP v2”) path to support HSDP-style deferred DP-outer gradient synchronization, using the microbatch context to delay DP-outer reduction until the last microbatch.

Changes:

  • Allow main_grad to differ from main_weight placements in the HSDP-compatible case (grad Partial where weight Replicate) and introduce deferred DP-outer gradient reduction logic.
  • Thread is_last_microbatch through post_backward to gate the final DP-outer reduction and installation of sharded parameter grads.
  • Add a 2-D device-mesh parity test validating HSDP loss equivalence to single-rank SGD across multiple microbatch counts.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
tests/unit_tests/distributed/mfsdp_v2/test_fully_shard.py Adds an HSDP parity test on a 2-D DP mesh and introduces HSDP placement definitions.
megatron/core/distributed/fsdp/src/megatron_fsdp/experimental/parameter_group.py Implements deferred DP-outer gradient reduction/accumulation and relaxes gradient/weight placement constraints.
megatron/core/distributed/fsdp/src/megatron_fsdp/experimental/module.py Passes context.is_last_microbatch into gradient reduction to enable last-microbatch gating.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +408 to +416
if len(grad_placements) != len(weight_placements):
return False
for grad_placement, weight_placement in zip(grad_placements, weight_placements):
if grad_placement == weight_placement:
continue
if isinstance(grad_placement, Partial) and isinstance(weight_placement, Replicate):
continue
return False
return True

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restricted _grad_placements_reduce_to_weight to at most one deferred (Partial→Replicate) axis, matching the single-axis redistribute. Multi-axis deferral is a follow-up.

Comment on lines +361 to +365
if self._defers_grad_reduction:
self._accumulate_deferred_grad(partial_grad, partial_op, is_last_microbatch)
for parameter in self.unsharded_parameters:
parameter.grad = None
return

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_reduced_grad is now cleared at the first microbatch of each step, so the previous step's buffer is released after zero_grad().

…ad buffer

Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>

@wujingyue wujingyue left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Comment thread tests/unit_tests/distributed/mfsdp_v2/test_fully_shard.py
Comment thread tests/unit_tests/distributed/mfsdp_v2/test_fully_shard.py Outdated
Comment thread tests/unit_tests/distributed/mfsdp_v2/test_fully_shard.py Outdated
Comment thread megatron/core/distributed/fsdp/src/megatron_fsdp/experimental/parameter_group.py Outdated
Comment thread megatron/core/distributed/fsdp/src/megatron_fsdp/experimental/parameter_group.py Outdated
Comment thread megatron/core/distributed/fsdp/src/megatron_fsdp/experimental/parameter_group.py Outdated
Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>
@Achyuthan-S

Copy link
Copy Markdown
Contributor Author

Thanks for the PR!

Anytime @wujingyue , you can always tag me in the future for any other issues , I find them very interesting and love solving them .

Rest main_grad at main_weight placements so it backs .grad and reuses the zero_grad/set_to_none reset; reduce-scatter the sharded axes every backward and all-reduce the replicated DP-outer axis in place only on the last microbatch. Drop the per-group state and multi-axis reduction helper; cover both set_to_none modes in the HSDP parity test.

Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>
main_grad rests at its DP-outer-Partial gradient placement between microbatches and is all-reduced to main_weight's placements on the last microbatch, per HSDP semantics. Extend DBuffer.get_dtensor to represent Partial so main_grad backs .grad throughout, reusing the existing zero_grad/set_to_none accumulation. Cover both set_to_none modes in the HSDP parity test.

Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>

@wujingyue wujingyue left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall!

Comment thread megatron/core/distributed/fsdp/src/megatron_fsdp/experimental/dbuffer.py Outdated
Comment thread megatron/core/distributed/fsdp/src/megatron_fsdp/experimental/parameter_group.py Outdated
Comment thread megatron/core/distributed/fsdp/src/megatron_fsdp/experimental/parameter_group.py Outdated
@wujingyue
wujingyue marked this pull request as ready for review July 15, 2026 15:21
@svcnvidia-nemo-ci
svcnvidia-nemo-ci requested a review from a team July 15, 2026 15:21
@wujingyue

Copy link
Copy Markdown
Contributor

/ok to test

@copy-pr-bot

copy-pr-bot Bot commented Jul 15, 2026

Copy link
Copy Markdown

/ok to test

@wujingyue, there was an error processing your request: E1

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/

@wujingyue

Copy link
Copy Markdown
Contributor

/claude fix

Signed-off-by: svcnvidia-nemo-ci <svcnvidia-nemo-ci@nvidia.com>
@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🛠️ Claude fix commit 434a4d9d63b9 (attempt 1)

⚠️ This explanation is AI-generated and may be inaccurate; the exact commit is authoritative.

What changed
No edits were made. The pinned base merge already reports all conflicts resolved (no unmerged files; the only ’=======’ matches are Markdown underlines and reStructuredText table borders, not git conflict markers), and all 79 changed Python files compile cleanly.

Files changed by Claude

  • No additional file edits; the pinned base was merged.

Why
No repairable signal was present: the CI log directory /home/runner/work/_temp/claude-fix-ci/ is empty (no lint or unit failure logs), the steering file is empty, and git reported no remaining merge conflicts to resolve. With no observed conflict or terminal failure in scope, there was nothing to fix.

DCO
@Achyuthan-S, please fix any DCO failures on your commits before merge. DCO does not block this workflow.

View exact commit

Sanitized and posted by svcnvidia-nemo-ci.

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

/ok to test 434a4d9

@ericharper

Copy link
Copy Markdown
Contributor

/ok to test c7c7183

Filter on activity_type == 'kernel' so the profiler's gpu_user_annotation duplicates aren't double-counted, and format the boolean (per @wujingyue's suggestion).

Signed-off-by: Achyuthan Sivasankar <achyuthan.sivasankar@gmail.com>
auto-merge was automatically disabled July 17, 2026 21:15

Head branch was pushed to by a user without write access

@Achyuthan-S
Achyuthan-S force-pushed the fsdp/hsdp-grad-reduce branch from c7c7183 to 96764cd Compare July 17, 2026 21:15
@Achyuthan-S

Copy link
Copy Markdown
Contributor Author

@wujingyue @ericharper Signed off and black-formatted the activity_type change in 96764cd — DCO is green now. The force-push cleared CI authorization and disabled auto-merge, so it needs one more /ok to test 96764cd and auto-merge re-enabled (or a manual merge once green). Thanks!

@wujingyue

Copy link
Copy Markdown
Contributor

/claude fix

Signed-off-by: svcnvidia-nemo-ci <svcnvidia-nemo-ci@nvidia.com>
@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🛠️ Claude fix commit 89b5296dc5be (attempt 1)

⚠️ This explanation is AI-generated and may be inaccurate; the exact commit is authoritative.

What changed
No edits made. The pinned base merge in pr-head/ is already fully resolved ("All conflicts fixed", no unmerged files, no genuine conflict markers). All 27 changed Python files parse cleanly.

Files changed by Claude

  • No additional file edits; the pinned base was merged.

Why
No actionable conflict or failure was present: git reports all conflicts resolved with zero unmerged paths; the CI log directory /home/runner/work/_temp/claude-fix-ci/ is empty (no lint or unit-test failures to clear); and the steering file is empty. With no reported failure and no lint tooling available, and policy forbidding broadening the change, no local repair applies.

DCO
@Achyuthan-S, please fix any DCO failures on your commits before merge. DCO does not block this workflow.

View exact commit

Sanitized and posted by svcnvidia-nemo-ci.

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

/ok to test 89b5296

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

❌ Claude fix stopped because exact-SHA CI did not complete in time. View exact-SHA CI.

@wujingyue

Copy link
Copy Markdown
Contributor

/claude fix

Signed-off-by: svcnvidia-nemo-ci <svcnvidia-nemo-ci@nvidia.com>
@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🛠️ Claude fix commit 66b592a2cb3c (attempt 1)

⚠️ This explanation is AI-generated and may be inaccurate; the exact commit is authoritative.

What changed
Resolved the one merge conflict in tests/unit_tests/distributed/mfsdp_v2/test_fully_shard.py by keeping the PR (HEAD) side, which filters NCCL events via the shared _nccl_events() helper. The base side had inlined equivalent kernel-only filtering; that logic is already inside _nccl_events(), and HEAD matches the sibling block at lines 324-326 using the same helper. cuda_events stays defined for the downstream gemm_events block. Verified no markers remain and ast.parse passes.

Files changed by Claude

  • tests/unit_tests/distributed/mfsdp_v2/test_fully_shard.py

Why
A both-modified text conflict: PR HEAD refactored CUDA NCCL event filtering into a _nccl_events() helper, while the pinned base merge (b4ad280) independently inlined equivalent kernel-only filtering (activity_type == "kernel") with comments. Both express the same intent, so I kept HEAD's helper-based form for consistency with the rest of the file.

DCO
@Achyuthan-S, please fix any DCO failures on your commits before merge. DCO does not block this workflow.

View exact commit

Sanitized and posted by svcnvidia-nemo-ci.

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

/ok to test 66b592a

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

❌ Claude fix stopped because exact-SHA CI did not complete in time. View exact-SHA CI.

@wujingyue

Copy link
Copy Markdown
Contributor

/ok to test 66b592a

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🔄 Merge queue validation started!

You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/29783738040

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MFSDP v2 should support HSDP and HFSDP placements

8 participants