feat(ppo): support async ppo - #3410
Conversation
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
…ed-clean Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com> # Conflicts: # nemo_rl/algorithms/async_utils/replay_buffer.py # nemo_rl/algorithms/async_utils/trajectory_collector.py # tests/unit/algorithms/test_async_utils.py
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
|
/ok to test b8776c9 |
Signed-off-by: bg51717 <biguo@nvidia.com> # Conflicts: # nemo_rl/algorithms/async_utils/trajectory_collector.py # nemo_rl/algorithms/grpo.py
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
yuki-97
left a comment
There was a problem hiding this comment.
Thanks @bg51717 — a lot of careful work here, and the warmup lead/age design in particular is subtler than it looks and holds up. Picking up async_ppo_train and async_utils/, the part I deferred on the earlier pass.
Two things I traced end to end, both clean:
-
Critic-warmup → normal-training transition. Walked the version/lead/window schedule across the frontier with a warmup config where lead and buffer age actually diverge. No data discard and no deadlock: consumption age never exceeds buffer age (exactly equal at the two frontier targets), the wakeup path can't miss a version bump, peak occupancy
(warmup_age+1)·Nstays under the2·warmup_age·Ncapacity, and the one dangerous shape — evicting rows for a target the collector can no longer regenerate, since past the frontier its window is only[v+1]— never triggers. The<=instep <= warmup_rollout_frontieris what guarantees that last one, andtest_async_ppo_consumes_frozen_policy_rollout_at_safe_warmup_frontieralready pins it. -
Policy / value GPU residency. Matches sync
ppo_trainstage for stage — value forward, logprobs, the per-epoch value and policy training pairs, the refit offload, and both checkpoint saves. Every load is paired, both models are on CPU at step boundaries, and the conditional skip of the post-refit offload is correct because the policy is never loaded during warmup.
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com> # Conflicts: # nemo_rl/algorithms/single_controller.py # nemo_rl/algorithms/single_controller_utils/setup.py # nemo_rl/environments/nemo_gym.py
Signed-off-by: bg51717 <biguo@nvidia.com>
|
/ok to test bac4a8c |
yuki-97
left a comment
There was a problem hiding this comment.
thanks for the updates, overall LGTM, just some nits and one nightly coverage.
Signed-off-by: bg51717 <biguo@nvidia.com> # Conflicts: # nemo_rl/algorithms/grpo.py
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>
|
/ok to test 6e0362d |
terrykong
left a comment
There was a problem hiding this comment.
Deep review by a team of 6 agents (RL/config, bug-finding, tests, design, comment audit) plus an adversarial pass that dropped most candidate findings. Everything below survived that pass and was re-verified against 6e0362d after your latest push.
First — this is a large, careful piece of work, and the review it has already had is excellent. I checked every thread marked fixed in <sha> and they all genuinely hold. Your three recent commits also resolved several things I had queued up, so I dropped them: the #3611 carry-over in should_use_async_rollouts (correct now — asserts and returns True), the TODO(#2625) rationale, the in_flight_weight_updates exemplar/docs default, the max_trajectory_age_steps > 1 warning, and the avg_trajectory_age nightly bound — where you also added the companion ["40"] <= 1.0 assertion and pointed the AutoModel nightly at a real warmup config. That last one closes the coverage gap that mattered most.
Two items are worth your attention before merge.
1. drop_incomplete_targets_on_restore — suggest removing it. Making the parameter required was the right call, but I think the option itself should go. true deletes restored rollouts that false keeps, and both branches then train on the same number of prompt groups, so the deletion only costs dataset coverage and regeneration compute — what it keeps is a strict subset of what the other branch keeps. It also cannot be a safety measure, because it leaves complete restored steps untouched. main and async GRPO both behave as false, and nothing sets the key, so every async PPO run today takes the deleting branch. Walkthrough with figures: https://terrykong.github.io/gh-pages-poc/terryk/pr-3410-resume-drops-batch.html — and if you have a case where deleting beats gap-filling, please say so and I will withdraw it.
2. The name of warmup_max_trajectory_age_steps. It reads as a staleness bound, but I verified it cannot affect staleness at all — it is a concurrency knob. Renaming is free right now (0 references on main) and a breaking change after merge.
main, so one more rebase is needed — but the specific #3611 hazard I was going to flag is already handled.
Verification notes: ruff, ruff-format, taplo and minimize-check pass. pyrefly could not run here (the uv lockfile is linux-only; this is macOS) — an environment limit, not a finding. No test was executed (no GPU, no runnable env), so please treat the test suggestions as reviewed-by-reading only. GPU-hour figures were measured with tools/launch's own formula, not estimated.
Generated by Claude Code
Signed-off-by: bg51717 <biguo@nvidia.com> # Conflicts: # nemo_rl/algorithms/grpo.py # nemo_rl/algorithms/single_controller.py # tests/unit/algorithms/test_grpo.py # tests/unit/test_recipes_and_test_suites.py
Signed-off-by: bg51717 <biguo@nvidia.com>
|
/ok to test 9182c74 |
What does this PR do?
Based on #3185.
Adds asynchronous PPO on top of non-colocated PPO, overlapping rollout generation on dedicated vLLM GPUs with policy and value-model training on the training cluster.
This is the asynchronous follow-up to #3262. The existing synchronous colocated and non-colocated PPO paths remain supported.
Design
The main changes are:
max_trajectory_age_steps.warmup_max_trajectory_age_steps, while keeping the effective policy-update age within the normal window once policy training starts.max_num_stepsis reached so asynchronous progress is controlled by learner steps rather than dataloader exhaustion.PPOConfigfrom the legacyTypedDictrepresentation to a PydanticBaseModel, preserving existing defaults and extra-field compatibility while aligning PPO configuration access with GRPO.Usage
Enable asynchronous PPO together with non-colocated asynchronous vLLM generation and importance-sampling correction:
max_trajectory_age_steps=1is the recommended default. When critic warmup is enabled,warmup_max_trajectory_age_stepscan be increased to let the frozen policy bank more rollouts before policy updates begin.Before your PR is "Ready for review"
Pre checks:
Test
The following test matrix passed:
Unit tests covering PPO asynchronous configuration and validation, trajectory-age windows, critic-warmup rollout banking, replay-buffer restore, dataloader cycling, weight-update coordination, failure propagation, and cleanup.
Async GRPO replay-buffer checkpoint compatibility tests passed after the shared asynchronous utility changes.
Synchronous colocated and non-colocated PPO functional regression tests passed on both AutoModel and Megatron.
AutoModel and Megatron asynchronous PPO functional tests passed, covering critic warmup, replay-buffer population, consistent critic-loss sanity checks, policy/value checkpointing, restore, and continued training after resume.
AutoModel (green, with
policy_training_start_step=5andwarmup_max_trajectory_age_steps=2) and Megatron (red, without critic warmup) 40-step Async PPO nightly runs both passed. correctness gates for reward, validation accuracy and train/generation logprob mismatch.A matched 40-step comparison between synchronous non-colocated Megatron PPO(blue) and Async Megatron PPO(yellow) used the same two-node training/generation split and showed no obvious regression in reward, validation accuracy, or train/generation logprob correctness.