Skip to content

fix: avoid duplicate gradient norm reduction in pure DDP - #290

Open
taking-lying-flat wants to merge 1 commit into
modelscope:mainfrom
taking-lying-flat:fix/ddp-grad-norm
Open

taking-lying-flat wants to merge 1 commit into
modelscope:mainfrom
taking-lying-flat:fix/ddp-grad-norm

Conversation

@taking-lying-flat

Copy link
Copy Markdown

PR type

  • Bug Fix
  • New Feature
  • Document Updates
  • More Models or Datasets Support

PR information

Pure DDP backward synchronizes complete gradients on every rank, but passing _dp_group into gradient clipping sums their squared norms again. With two ranks, a normalized gradient of 0.8 is reported as 1.131371 and incorrectly clipped to 0.707106 when max_grad_norm=1.

Use the existing local clipping path when the model is wrapped in DistributedDataParallel and its mesh contains only data parallelism. Keep the DP group for token-count gathering and retain gradient normalization, scaler handling, and training-state updates. Other parallel configurations keep their existing norm-reduction group.

Experiment results

Local validation with PyTorch 2.13.0 and CPU/Gloo, using an out-of-tree harness with real DDP backward and the TransformersModel.clip_grad_norm method body:

  • Reproduced the original two-rank error against the base commit.
  • Passed 36 DDP cases across 1, 2, and 4 ranks, covering L1/L2/infinity norms, gradients above and below the clipping threshold, unequal token counts, gradient accumulation, and SGD updates.
  • Verified skipped accumulation steps, explicit Tensor-shard reductions, DTensor shard norms/clipping, and preservation of hybrid/EP group arguments.
  • uvx pre-commit run --all-files passed.

For the two-rank 0.8 case, the corrected norm and clipped gradient are both 0.800000.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant