fix(diffusion): rank-symmetric DMD2 all-reduce and deferred FSDP2 grad sync - #3679
Open
akoumpa wants to merge 1 commit into
Open
fix(diffusion): rank-symmetric DMD2 all-reduce and deferred FSDP2 grad sync#3679akoumpa wants to merge 1 commit into
akoumpa wants to merge 1 commit into
Conversation
…FSDP2 grad sync in DMD2 Two independent distributed issues in the DMD2 objective. 1. `_synchronize_discriminator_gradients` skipped parameters whose `.grad` was `None`, which makes the *number* of collectives rank-dependent. NCCL matches collectives by call order, so a single rank taking a different branch hangs the whole job with no diagnostic -- no timeout message and no rank identification. The replicated discriminator uses every parameter on every forward today, so there is no reachable trigger from the shipped recipes, but the guard buys nothing: materializing a zero gradient costs one allocation and keeps every rank issuing the same collectives in the same order. 2. The microbatch loop never deferred FSDP2's gradient reduce-scatter to the final microbatch. It relies on `prepare_for_grad_accumulation` / `prepare_for_final_backward`, which are gated on a method only the MoE mixin defines -- so both are no-ops for a diffusers transformer and `fsdp.defer_fsdp_grad_sync` had no effect on this path. Measured on a sharded Wan transformer: 20 `reduce_scatter_tensor` calls for 4 microbatches where 5 are needed, i.e. a full reduce-scatter set per microbatch instead of only the last. This is a performance issue, not a correctness one -- reduce-scatter of a sum equals the sum of reduce-scatters -- and it is invisible at the shipped configs, whose `global_batch_size // (local_batch_size * dp_size)` is 1. It becomes real as soon as `global_batch_size` is raised. The non-DMD2 branch of this same recipe already handles it via `get_sync_ctx`; the DMD2 branch now does too. The test fixtures gain `defer_fsdp_grad_sync`, which the real `TrainDiffusionRecipe` sets but the partial stand-ins did not. Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>
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.
Two independent distributed issues in the DMD2 objective.
Stacked on #3298 —
step_distillation.pydoes not exist onmain, so this targetsthat branch rather than
main.1. Rank-dependent collective count in the discriminator all-reduce
_synchronize_discriminator_gradientsskipped parameters whose.gradwasNone:NCCL matches collectives by call order, so one rank taking a different branch hangs
the whole job with no diagnostic — no timeout message, no rank identification.
The replicated discriminator uses every parameter on every forward today, so there is
no reachable trigger from the shipped recipes — this is a latent hazard rather than
a live bug. But the guard buys nothing: materializing a zero gradient costs one
allocation and keeps every rank issuing the same collectives in the same order.
Verified: the collective count is now constant at 4 whether or not a gradient is
None; before the change it was 3 vs 4 across ranks.2. FSDP2 gradient sync was never deferred to the final microbatch
The microbatch loop relies on
prepare_for_grad_accumulation/prepare_for_final_backward, which are gated on a method only the MoE mixin defines —so both are no-ops for a diffusers transformer, and
fsdp.defer_fsdp_grad_synchadno effect on this path.
Measured on a sharded Wan transformer: 20
reduce_scatter_tensorcalls for 4microbatches where 5 are needed — a full reduce-scatter set per microbatch instead of
only on the last.
This is a performance issue, not a correctness one (reduce-scatter of a sum equals the
sum of reduce-scatters), and it is invisible at the shipped configs, whose
global_batch_size // (local_batch_size * dp_size)is 1. It becomes real as soon asglobal_batch_sizeis raised.The non-DMD2 branch of this same recipe already handles it via
get_sync_ctx; the DMD2branch now does too.
Testing
tests/unit_tests/recipes/test_diffusion_step_distillation.py: 42 passedrecipes,datasets/diffusion,_diffusers,flow_matching): 1721passed; the 5
test_base_recipe.pyfailures are pre-existing and reproduce on aclean
maincheckoutruff check/ruff format --checkcleanThe test fixtures gain
defer_fsdp_grad_sync, which the realTrainDiffusionRecipesets but the partial stand-ins did not — added there rather than defensively
getattr-ing it in production, since that would mask the same class of bug later.