Skip to content

perf(train): release consumed grads at the optimizer-step boundary - #460

Merged
CjhHa1 merged 3 commits into
Tencent-Hunyuan:mainfrom
Yuanjia-Lee:perf/release-grads
Sep 17, 2026
Merged

CjhHa1 merged 3 commits into
Tencent-Hunyuan:mainfrom
Yuanjia-Lee:perf/release-grads

Conversation

@Yuanjia-Lee

Copy link
Copy Markdown
Contributor

Summary

optimizer_step() zeroed grads only on the skip path; committed updates left
them alive. The colocate loops then carry them through the whole cycle:
offload()/onload() move params, grads, and optimizer state together, and
the next batch's anchor prep runs with the previous update's grads on GPU.
Nothing reads .grad between updates: the only two callers (stack/base.py,
unified_model_stack.py) go straight to metric aggregation, and accumulation
never spans an optimizer step.

A/B over a full FSDP2 train/offload/rollout/onload/anchor cycle, arms
differing only in the release line: parameters, AdamW state, and both
updates' losses are bit-identical. The baseline arm alone still moves the
previous update's grads through offload and onload; with the release,
onload's allocated delta drops from params+grads to params only. On the 4B
full-FT AR recipe the sharded fp32 grads reach 2 GB/rank, crossing PCIe
twice per rollout.

The release is the skip path's own zero_grad(set_to_none=True), placed
after the EMA/count consumers. Accumulation windows are unaffected:
non-stepping parts run zero_grad=False, the stepping update zeroes at
entry. Recorded in unirl/train/readme.md Gotchas.

Related Issue

Related to #429: move count there, payload size here.

Test Plan

  • SKIP=no-commit-to-branch pre-commit run --files unirl/train/backend/base_backend.py unirl/train/readme.md --show-diff-on-failure
    → all hooks pass.
  • Distributed FSDP2 harness on one 4090 node (two ranks): backward → real
    optimizer_step → real fsdp_offload → real fsdp_onload → no-grad
    anchor forward → next update. Param shards and AdamW state match
    bit-for-bit (max diff 0.0), losses equal.

Compatibility / Risk

  • Grads free earlier; the between-updates footprint drops by the local grad
    shard. Clipping and deferred sync are unchanged: grads are fully
    reduce-scattered before optimizer.step().

Reviewer Notes

Checklist

  • I reviewed the changed code and removed unrelated/generated artifacts.
  • I updated tests, docs, and configs where needed, or explained why not.

optimizer_step() zeroed grads only on the skip path; committed updates left
them alive across offload()/onload(), which move "params + grads + optimizer"
as one train state, and across the next batch's anchor prep. Release them
after the commit so dead tensors stop riding the rollout boundary.
Accumulation windows are unaffected: non-stepping parts run zero_grad=False
and the stepping update still zeroes at entry.
@github-actions github-actions Bot added the need review Ready and waiting for review label Sep 14, 2026

@CjhHa1 CjhHa1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions Bot added approved Approved by reviewer and removed need review Ready and waiting for review labels Sep 17, 2026
@CjhHa1

CjhHa1 commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

@Yuanjia-Lee I've shorten the docstrings as the method itself is worth adding

@CjhHa1
CjhHa1 merged commit b137861 into Tencent-Hunyuan:main Sep 17, 2026
6 checks passed
@github-actions github-actions Bot removed the approved Approved by reviewer label Sep 17, 2026
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.

2 participants