Skip to content

fix: fix single controller grad_norm#3282

Open
yuki-97 wants to merge 1 commit into
mainfrom
yukih/fix-sc-grad
Open

fix: fix single controller grad_norm#3282
yuki-97 wants to merge 1 commit into
mainfrom
yukih/fix-sc-grad

Conversation

@yuki-97

@yuki-97 yuki-97 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Bug

SC (Single Controller) split-API path had grad_norm at ~1/2 of the sync path from step 2 onward on the Megatron backend; reward regressed correspondingly.

Fix

Add torch.cuda.synchronize() between finish_grad_sync() and optimizer.step() in _finish_train_step_body, so the DP reduce dispatched on the comm stream completes before optimizer.step reads main_grad.

Test Results

  • baseline (old code path)
    • yellow: didn't enable validate and checkpointing to keep the same test settings.
    • purple: with val_period=10, which will reset vLLM's prefix cache every 10 steps.
  • blue: before this PR, grad_norm is ~1/2x smaller and reward is a bit lower.
  • red: after this PR.

Note: Only the purple one will reset vLLM's prefix cache every 10 steps, other runs didn't reset for the whole run.

image image

Signed-off-by: Yuki Huang <yukih@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 19, 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.

@yuki-97 yuki-97 added the CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) label Jul 19, 2026
@yuki-97

yuki-97 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test c2feb15

@yuki-97
yuki-97 marked this pull request as ready for review July 19, 2026 09:17
@yuki-97
yuki-97 requested review from a team as code owners July 19, 2026 09:17
@yuki-97
yuki-97 requested a review from terrykong July 19, 2026 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant