Allocate main_grad on the reduce-scatter stream - #6187
Open
wujingyue wants to merge 1 commit into
Open
Conversation
FsdpParameterGroup allocated main_grad in __init__, which runs on whatever stream fully_shard() is called from -- normally the default stream. reduce_partial_gradients then rebinds main_grad from inside a reduce_scatter_stream context, dropping the previous buffer while that stream is still reading it as the reduction's input. The caching allocator binds a block to its allocation stream, so the dropped block became immediately reusable by default-stream allocations, and a later default-stream allocation could overwrite the in-flight reduction's input. The result was silently wrong gradients rather than a crash: measured against a single-rank baseline on 8 GPUs, individual parameters came out 12% to 125% off, varying run to run. It only surfaced when something perturbed timing enough to make the reuse collide, which is why the affected unit test passed most of the time. Materialize main_grad lazily from a property instead. The first access happens inside reduce_partial_gradients, so the buffer is allocated on the same stream that later frees it, and an assertion pins that invariant in place. This is the same allocate-and-release-on-one-stream rule that FsdpModule._reshard_parameter_groups already documents for unsharded parameter storage. Signed-off-by: Jingyue Wu <jingyuew@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. |
wujingyue
marked this pull request as ready for review
August 1, 2026 04:22
This was referenced Aug 1, 2026
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.
Root cause diagnosed by @Autumn1998 (Tong Liu) in #6137 (comment):
FsdpParameterGroupallocatedmain_gradin__init__, which runs on whatever streamfully_shard()is called from — normally the default stream.reduce_partial_gradientsthen rebinds it from inside areduce_scatter_streamcontext, dropping the old buffer while that stream is still reading it. The caching allocator binds a block to its allocation stream, so the dropped block became immediately reusable by default-stream allocations.Background on the allocator's stream semantics and this free-while-in-use hazard: FSDP & CUDACachingAllocator: an outsider newb perspective. That write-up frames
record_stream()as the mechanism for keeping a block alive across streams; this PR instead keeps allocation and free on a single stream, so the block only ever becomes reusable by allocations already ordered behind the reduce-scatter, andrecord_stream()is not needed.The result is silently wrong gradients, not a crash: 12%–125% off per parameter, varying run to run, present at the end of the first backward. It surfaced in #5956 as flaky
test_hsdp_losses_match_baselinefailures.Fix: materialize
main_gradlazily from a property, so the first access — insidereduce_partial_gradients— allocates it on the same stream that later frees it. An assertion pins that invariant. Same allocate-and-release-on-one-stream ruleFsdpModule._reshard_parameter_groupsalready documents for unsharded parameter storage.Verified on 8×A100: full
mfsdp_v2bucket goes from 5/5 failing to 0/4 under a timing perturbation that makes the reuse collide reliably.Also verified against #5956, the PR where this surfaced. That branch is flaky on its own, so no perturbation is needed — this is the real-world rate. Same 8×A100 node, arms alternated, full
mfsdp_v2bucket:The base-arm failures are the same signature as the original report —
test_hsdp_losses_match_baselinein varying combinations of[1-True](10),[3-True](4),[1-False](3),[3-False](3).