Repository navigation
fix(mfsdp): refresh V2 compute weights after optimizer step - #6336
Merged
wujingyue merged 2 commits intoAug 11, 2026
Merged
Conversation
|
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. |
wujingyue
force-pushed
the
agent/mfsdp-v2-native-weight-sync
branch
2 times, most recently
from
August 7, 2026 04:23
89e7063 to
1252bd6
Compare
wujingyue
marked this pull request as ready for review
August 7, 2026 04:26
Contributor
Author
|
/ok to test 1252bd6 |
skyw
approved these changes
Aug 7, 2026
| for parameter in self.get_parameters() | ||
| if (parameter_group := get_containing_parameter_group(parameter)) is not None | ||
| } | ||
| for parameter_group in parameter_groups: |
Contributor
There was a problem hiding this comment.
Nit: single letter "s" difference is hard to spot and easily miss. May consider better name.
wujingyue
force-pushed
the
agent/mfsdp-v2-native-weight-sync
branch
from
August 7, 2026 17:48
1252bd6 to
54d9aec
Compare
wujingyue
force-pushed
the
agent/mfsdp-v2-native-weight-sync
branch
from
August 7, 2026 17:52
54d9aec to
ad73e1b
Compare
wujingyue
force-pushed
the
agent/mfsdp-v2-native-weight-sync
branch
from
August 7, 2026 17:55
ad73e1b to
bb85dbd
Compare
shanmugamr1992
approved these changes
Aug 10, 2026
wujingyue
force-pushed
the
agent/mfsdp-v2-native-weight-sync
branch
2 times, most recently
from
August 11, 2026 00:25
1e21d17 to
847ee78
Compare
Signed-off-by: Jingyue Wu <jingyuew@nvidia.com>
Extract sync_model_weights_from_main_weights() so the checkpoint load path and the fully_shard_optimizer() step post-hook refresh compute weights through one implementation instead of two. The post-hook previously collected parameter groups into a set and iterated it. FsdpParameterGroup is @DataClass(frozen=True, eq=False), so it hashes by id() and set iteration order varies per process. sync_model_weight_from_main_weight() issues a collective whenever main_weight and model_weight are distinct buffers, so ranks could issue the per-group collectives in different orders and deadlock. Reusing the shared helper fixes that: it iterates the parameters, whose order is identical on every rank, and uses a set only to skip groups already synced. Reproduced with an HFSDP configuration (optimizer [Flat, Flat], parameter [Replicate, Flat]) over three parameter groups on 4 GPUs, which hung until this change; it now completes in ~9s. Signed-off-by: Jingyue Wu <jingyuew@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
wujingyue
force-pushed
the
agent/mfsdp-v2-native-weight-sync
branch
from
August 11, 2026 00:33
847ee78 to
296a795
Compare
wujingyue
added a commit
to Achyuthan-S/Megatron-LM
that referenced
this pull request
Aug 11, 2026
HFSDP's optimizer placement [Flat, Flat] differs from the parameter placement [Replicate, Flat], so main_weight and model_weight are distinct buffers. The test drove a plain torch.optim.SGD, which never refreshes the compute weight, so every forward re-read the initial weights: across all five steps the loss and gradient were bit-identical while the optimizer weight advanced. HSDP passes unwrapped because its optimizer and parameter placements match, so the two buffers alias and the sync is a no-op. Wrap the optimizer with fully_shard_optimizer() so its step post-hook refreshes the compute weight. With that, HFSDP matches the single-rank baseline exactly (absolute difference 0.0 at every step). Drop the temporary race-repro probe and the magnitude diagnostic. The failure was deterministic rather than a race, so neither is needed. Depends on NVIDIA#6336, which routes that post-hook through sync_model_weights_from_main_weights(); without it the hook iterates a set whose order varies per rank and the collectives deadlock. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jingyue Wu <jingyuew@nvidia.com>
Contributor
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/31514574131 |
This was referenced Aug 27, 2026
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FullyShardedOptimizer's native post-step copy hook.sync_model_weights_from_main_weights()so the checkpoint-load path and thefully_shard_optimizer()step post-hook refresh compute weights through one implementation instead of two.Root cause
#5949 moved main-weight to model-weight synchronization from pre-forward to optimizer post-step.
FullyShardedOptimizerinherited the post-step extension point but left its implementation as a no-op, so optimizer weights updated while subsequent forwards continued to use stale compute weights.The existing three-step integration test used a 1% relative tolerance. The resulting loss drift remained below that threshold, so the test passed despite the missing refresh.
Also fixes a rank-order deadlock in
fully_shard_optimizer()The experimental
step_post_hookcollected parameter groups into asetand iterated it:FsdpParameterGroupis@dataclass(frozen=True, eq=False), so it hashes byid()and set iteration order varies per process.sync_model_weight_from_main_weight()issues a collective whenevermain_weightandmodel_weightare distinct buffers, so ranks could issue the per-group collectives in different orders and hang.Routing the hook through the shared helper fixes this as a consequence of the refactor rather than as a special case: the helper iterates the parameters, whose order is identical on every rank, and uses a set only to skip groups already synced.
The bug is latent on
maintoday because it needs both conditions at once — two or more parameter groups andmain_weight is not model_weight. No in-tree test combines them: configurations whose optimizer and parameter placements match (e.g. HSDP) alias the two buffers, so the sync early-returns without a collective.Validation
uv run python -m torch.distributed.run --nproc_per_node=2 -m pytest -q tests/unit_tests/distributed/mfsdp_v2/test_mcore_adapter.py::TestMcoreAdapter::test_build_train_and_steptests/unit_tests/distributed/mfsdp_v2/test_fully_shard.pyon 4x H100: 32 passed. The one failure,test_overlaps_communication_and_compute[default], reproduces identically on unmodifiedmain— it is a launch-bound profiler assertion, unrelated to this change.[Flat, Flat], parameter[Replicate, Flat]) over three parameter groups on 4 GPUs hung indefinitely before this change and completes in ~9s after it.ruff checkandblack --checkon edited filesRelated to #5949 and #6325.