Add gradient accumulation for training (single-GPU/DDP/FSDP2)#859
Add gradient accumulation for training (single-GPU/DDP/FSDP2)#859eldarkurtic wants to merge 1 commit into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdded a validated gradient-accumulation CLI option, wired it into ChangesGradient accumulation training
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merge Protections🔴 1 of 1 protections blocking · waiting on 👀 reviews
🔴 Require approval from approved reviewers listWaiting for any of
This rule is failing.All pull requests must have at least one approving review from a member of the approved reviewers list before merging.
|
|
This pull request has merge conflicts that must be resolved before it can be |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/speculators/train/trainer.py`:
- Around line 191-197: Update the training-state persistence and resume
validation around the trainer’s accumulation setup: store
gradient_accumulation_steps in training_state.json, read it when resuming, and
reject checkpoints whose stored value differs from the configured value. Treat
missing accumulation metadata in legacy state as 1, while preserving the
existing zero-optimizer-step validation.
- Around line 506-519: Update checkpoint cadence calculations in the training
loop around _optimizer_steps_per_epoch, deriving step_interval from
optimizer-step counts rather than raw microbatch counts. Compare the
optimizer-step position using local_step // accum in the checkpoint modulo
logic, while preserving boundary alignment and the existing end-of-epoch guard.
- Around line 125-132: Update _optimizer_steps_per_epoch to validate that accum
is at least 1 before performing floor division, raising the trainer API’s
appropriate validation error for non-positive values. Add a direct TrainerConfig
test covering gradient_accumulation_steps=0 and a negative value, while
preserving the existing drop-remainder behavior for valid accumulation values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d25c7b4e-c115-47c0-a052-1c0c4805ac84
📒 Files selected for processing (6)
docs/cli/train.mdscripts/train.pysrc/speculators/train/trainer.pytests/unit/train/test_cli_args.pytests/unit/train/test_gradient_accumulation.pytests/unit/train/test_trainer_scheduler.py
cab6621 to
20c22f5
Compare
|
The quality checks have failed. Please run |
20c22f5 to
e93d88e
Compare
e93d88e to
c571e41
Compare
|
Hi @eldarkurtic #841 just landed, quick heads up The new config CLI (#841) is in The gist:
# src/speculators/train/config/schema.py — in TrainerArgs
gradient_accumulation_steps: int = Field(
default=1,
description="Accumulate gradients over N microbatches before each optimizer step ...",
)
@field_validator("gradient_accumulation_steps")
@classmethod
def _validate_gradient_accumulation_steps(cls, v: int) -> int:
if v < 1:
raise ValueError("--gradient-accumulation-steps must be a positive integer (>= 1)")
return v
Easiest path: I already rebased + migrated your commit (authorship kept). Grab the patch below and: git fetch origin
git checkout -B add-gradient-accumulation origin/main
git am pr859-new-cli.patch
git push --force-with-lease origin add-gradient-accumulationTested on the rebased result CLI, scheduler, grad-accum, resume, and schema tests all pass. 👍 Shout if you'd rather I just push it up for you. Full patch —
|
|
This pull request has merge conflicts that must be resolved before it can be |
Introduce a `--gradient-accumulation-steps` / `TrainerConfig.gradient_accumulation_steps` knob (default 1) that accumulates gradients over N microbatches before each optimizer step, growing the effective batch size without extra per-microbatch memory. - Loss is scaled by 1/N; the optimizer, gradient clip, and LR scheduler run once per accumulation window (macro-step counting for global_step, log_freq, checkpoint cadence, and scheduler total steps). - Trailing microbatches that don't fill a full window are dropped each epoch; `_optimizer_steps_per_epoch` is the single source of truth for this policy. - `_maybe_no_sync` skips the DDP all-reduce on non-boundary microbatches; the single-GPU and FSDP2 paths fall back to a no-op context and stay correct because gradients accumulate additively into `.grad`. - Trainer raises early when N exceeds the batches-per-epoch (would never step). - accum=1 is behavior-identical to the previous one-step-per-batch loop. Verified numerically equivalent to the mean of per-microbatch gradients on single-GPU, 8-GPU DDP, and 4-GPU FSDP2 (fully_shard), plus unit tests for the scheduler math, no_sync behavior, cadence, and CLI wiring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Eldar Kurtic <8884008+eldarkurtic@users.noreply.github.com>
c571e41 to
ea39e9e
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
Summary
Adds a
--gradient-accumulation-steps/TrainerConfig.gradient_accumulation_stepsknob (default1) that accumulates gradients over N microbatches before each optimizer step, growing the effective batch size (per-step batch × N) without extra per-microbatch memory. Matches the convention used by peer speculative-decoding frameworks (DeepSpec, TorchSpec, SpecForge): loss scaled by1/N, optimizer + LR scheduler stepped once per accumulation window, counting in optimizer (macro) steps.Behavior
global_step,--log-freq,--checkpoint-freq(when< 1), and the LR scheduler total all advance once per accumulation window (an accum=N run does1/Nthe scheduler steps per epoch)._optimizer_steps_per_epochis the single source of truth for this policy (used by the scheduler math, the__init__guard, and the loop boundary)._maybe_no_sync: on DDP, skips the gradient all-reduce on non-boundary microbatches and syncs the accumulated gradient on the boundary. Single-GPU (raw module) and FSDP2 (fully_shard, DTensor params) fall back to a no-op context and stay correct because gradients accumulate additively into.grad.Trainerraises early if N exceeds the batches-per-epoch (which would otherwise run zero optimizer steps).accum=1is behavior-identical to the previous one-step-per-batch loop (regression-safe).Verification
Numerically confirmed equivalent to the mean of per-microbatch gradients (accumulated grad vs. reference) on real hardware:
pythontorchrun×8fully_shard)torchrun×4All three exercise the real
Trainer.train_epoch → _accumulate_and_step → _maybe_no_syncpath. DDP/FSDP2 use different data per rank, so cross-rank averaging composes with accumulation.Unit tests added: scheduler-step math (accum, remainder, accum=1 regression), numerical grad-equivalence on a toy model,
_maybe_no_syncbehavior (single-process DDP), the accum-too-large guard, and CLI parsing/rejection.Test plan
python -m pytest tests/unit/train/test_gradient_accumulation.py tests/unit/train/test_trainer_scheduler.py tests/unit/train/test_cli_args.py— passing.make quality(ruff + format + mypy) clean on changed files.🤖 Generated with Claude Code