Skip to content

fix(utils): share the Saver frequency decision across models in a step - #1802

Merged
sitabulaixizawaluduo merged 1 commit into
areal-project:mainfrom
MohammadHijjawi97:fix/saver-shared-frequency-check
Oct 19, 2026
Merged

sitabulaixizawaluduo merged 1 commit into
areal-project:mainfrom
MohammadHijjawi97:fix/saver-shared-frequency-check

Conversation

@MohammadHijjawi97

Copy link
Copy Markdown
Contributor

Description

PPOTrainer._save_hf calls Saver.save for the actor and then for the critic in the same training step. Each call ran the shared EpochStepTimeFreqCtl.check, so the step and epoch counters advanced twice per step, and the time window was reset by the first call. As a result, when a critic is used:

  • saver.freq_steps=2 (or freq_epochs=2): the critic is saved every step (or epoch) and the actor is never saved.
  • saver.freq_secs=N: the first call resets the window, so the actor is saved and the critic is not.

This PR makes the frequency decision once per global step in Saver and reuses it for the other models saved in that step. Trainers that save a single model behave exactly as before, and the saved freq_ctl state format is unchanged.

Reproduction (freq_steps=2, actor + critic saved each step, 6 steps):

before: actor saves at steps: []               critic saves at steps: ['0', '1', '2', '3', '4', '5']
after:  actor saves at steps: ['1', '3', '5']  critic saves at steps: ['1', '3', '5']

Related Issue

None; no existing issue or PR covers it.

Type of Change

  • 🐛 Bug fix
  • ✨ New feature
  • 💥 Breaking change
  • 📝 Documentation update
  • ♻️ Refactoring
  • ⚡ Performance improvement
  • ✅ Test coverage improvement

Checklist

  • I have read the Contributing Guide
  • Pre-commit hooks pass (pre-commit run --all-files)
  • Relevant tests pass; new tests added for new functionality
  • Documentation updated (if applicable; built with ./docs/build_all.sh)
  • Branch is up to date with main
  • Self-reviewed via /review-pr command
  • This PR was created by a coding agent via /create-pr
  • This PR is a breaking change

Additional Context

Added test_save_applies_frequency_once_per_step_for_all_models to tests/test_saver.py, parametrized over freq_steps=2 and freq_epochs=2. It fails on main (the actor is never saved) and passes with this change. All of tests/test_saver.py passes (6 tests). pre-commit run --files on the two changed files passes (ruff lint/format, SPDX header). These are CPU-only unit tests; I did not run a full PPO training job.

PPOTrainer._save_hf calls Saver.save for the actor and then for the
critic in the same training step. Each call ran the shared
EpochStepTimeFreqCtl.check, so the step/epoch counters advanced twice
per step and the time window was reset by the first call. With
freq_steps=2 (or freq_epochs=2) the critic was saved every step or
epoch and the actor was never saved; with freq_secs the critic was
never saved.

Make the frequency decision once per global step and reuse it for the
other models saved in that step. Trainers with a single model are
unaffected.

Signed-off-by: Mohammad Hijjawi <mohammad.hijjawi1997@gmail.com>
@sitabulaixizawaluduo
sitabulaixizawaluduo merged commit 3b93679 into areal-project:main Oct 19, 2026
6 checks passed
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.

3 participants