Match Mobile300 custom trainer microstep semantics#862
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5175ddbb48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def test_train_run_sft_prime_rl_target_micro_steps_drops_partial_accumulation( | ||
| tmp_path: Path, monkeypatch | ||
| ) -> None: | ||
| """Custom-trainer max_steps counted micro-batches, not optimizer updates.""" |
There was a problem hiding this comment.
Name the guarded commit in this regression test
The repo-root AGENTS.md requires regression tests to name the PR or commit they guard. This newly added test is a regression guard for the microstep semantics, but the docstring only describes the behavior, so future maintainers lose the required traceability; please include the guarded PR/commit identifier in the docstring.
Useful? React with 👍 / 👎.
|
Users Simulation automation review (2026-06-30T12:45Z): ready for this simulation scope. Reviewed current head Evidence:
No model rollout artifacts were produced in this review because this PR only changes the trainer command-generation path, not BenchFlow task execution. For this scope, the relevant trainer-facing evidence is the manifest/test coverage showing I am applying |
Summary
Fixes the BenchFlow Prime-RL SFT compatibility profile for the env-0 Mobile300 / PR828 reproduction path by matching the old custom PEFT trainer's step semantics.
The historical custom trainer counted
max_steps=300as batch-size-1 micro-batches and only applied optimizer updates every 8 micro-batches. That means the saved adapter had 37 applied optimizer updates, with the final 4 micro-batches accumulated but not stepped. Prime-RLmax_stepscounts optimizer/trainer steps, so the previous--target-examples 300mapping generatedmax_steps=38and over-exposed the run relative to the source-of-truth custom trainer.Changes:
--target-micro-stepsto derive Prime-RLmax_stepsfrom custom-trainer micro-batch semantics.env0-mobile300-pr828compat profile to usetarget_micro_steps=300, producingmax_steps=37andscheduler.decay_steps=37.effective_train_examplesandunapplied_micro_stepsin the run manifest for auditability.--target-examplesbehavior unchanged: sample-exposure derivation still rounds up.Known caveat: this is a BenchFlow wrapper/profile fix only. It does not patch Prime-RL internals. Remaining exact-equivalence gaps, if performance still does not recover, are likely deeper token/loss/truncation semantics inside Prime-RL rather than
results.jsonlconversion or BenchFlow CLI command construction.Validation
uv run pytest tests/test_train_cli.py -q-> 27 passeduv run ruff format --check src/benchflow/training/backends/prime_rl.py src/benchflow/cli/train.py tests/test_train_cli.py-> 3 files already formatteduv run ruff check-> All checks passeduv run ty check src/benchflow/training/backends/prime_rl.py src/benchflow/cli/train.py-> All checks passeduv run bench train run sft --help-> exposes--target-micro-stepsuv run pytest -q-> 4704 passed, 14 skipped, 7 deselected