Skip to content

[trainer, cfg] fix: wire critic batching keys into TrainingWorkerConfig - #7465

Open
YeonwooSung wants to merge 1 commit into
verl-project:mainfrom
YeonwooSung:fix/7462-critic-batching-config
Open

[trainer, cfg] fix: wire critic batching keys into TrainingWorkerConfig#7465
YeonwooSung wants to merge 1 commit into
verl-project:mainfrom
YeonwooSung:fix/7462-critic-batching-config

Conversation

@YeonwooSung

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #7462.

The critic worker is a bare TrainingWorker and only reads batching knobs from engine_config. Trainers copied token lengths onto that engine and skipped use_dynamic_bsz / *_micro_batch_size_per_gpu. Combined with EngineConfig.use_dynamic_bsz defaulting to True, critic.use_dynamic_bsz=false was a no-op.

V1 (trainer.use_v1=true, the default) also wrote ppo_infer_max_token_len_per_gpu onto the train budget, so critic.ppo_max_token_len_per_gpu was ignored.

This PR:

  • Adds CriticConfig.apply_engine_batching() and uses it in v0, v1, and experimental/separation trainers.
  • Exposes ppo_infer_max_token_len_per_gpu / ppo_infer_micro_batch_size_per_gpu in critic YAML (defaulting to the documented forward / training knobs).
  • Adds CPU unit tests for the helper.

Checklist Before Starting

Not a duplicate of #7460 (that issue is critic.model.model_type being overwritten). Same construction site, different fields. No open PR already wires these batching keys.

Test

pytest tests/workers/config/test_critic_engine_batching_on_cpu.py -q

Local machine did not have the full verl test env (ray / torch not installed in the login Python). Ruff passed on the touched files. Please run the CPU test in CI.

API and Usage Example

# these now reach the critic engine
critic.use_dynamic_bsz=false
critic.ppo_micro_batch_size_per_gpu=4
critic.ppo_max_token_len_per_gpu=4096
critic.forward_max_token_len_per_gpu=2048

V1 no longer overwrites the train token budget with the infer value.

Design & Code Changes

TrainingWorker.train_batch reads engine_config.use_dynamic_bsz / max_token_len_per_gpu / micro_batch_size_per_gpu. Actor already assigns those from ActorConfig. The critic path now does the same via one helper so v0 / v1 / separation cannot drift again.

Checklist Before Submitting

  • Read the Contribute Guide.
  • Apply pre-commit checks: pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=always
  • Add / Update the documentation.
  • Add unit or end-to-end test(s) to the CI workflow to cover all the code. If not feasible, explain why: CPU unit tests added; they will run in the existing CPU unit-test workflow once ray/torch are available.
  • Once your PR is ready for CI, send a message in the ci-request channel in the verl Slack workspace.
  • If your PR is related to the recipe submodule, please also update the reference to the submodule commit via git submodule update --remote or cd recipe && git pull origin main.

AI assistance was used to locate the bug and draft the patch. A human author reviewed every changed line.

Fixes verl-project#7462

Co-authored-by: Grok <grok@x.ai>
Signed-off-by: YeonwooSung <neos960518@gmail.com>
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.

[trainer][critic] Critic batching keys never reach TrainingWorkerConfig; V1 also overwrites the train token budget

2 participants