feat(grpo): stream rollout batches by prompt group#3000
Conversation
b913406 to
e02c941
Compare
|
/ok to test e02c941 |
bfd380f to
5c060b1
Compare
96fe56a to
fa88050
Compare
|
/ok to test fa88050 |
fa88050 to
9924919
Compare
|
/ok to test 9924919 |
487463b to
2ba9898
Compare
9924919 to
2d61051
Compare
|
/ok to test 2d61051 |
Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
2d61051 to
c8d51f6
Compare
|
/ok to test c8d51f6 |
Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
…ts_clean Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com> # Conflicts: # nemo_rl/algorithms/grpo.py
|
/ok to test 4fa8108 |
terrykong
left a comment
There was a problem hiding this comment.
TL;DR — what / why / how
What: Async GRPO previously generated rollouts prompt-group by prompt-group but consumed them batch by batch — nothing reached the replay buffer until every prompt in a batch finished, so one slow prompt held all completed groups hostage. This PR streams results back as each prompt group completes, so finished groups become trainable immediately and the slowest prompt only delays itself.
Why: Throughput. The trainer eats from the replay buffer while the collector fills it; head-of-line blocking on the slowest prompt starves the buffer and inflates step time — worst for NeMo-Gym workloads with heavy tail latency (agentic/SWE tasks).
How (bottom-up):
- Gym actor —
run_rolloutsbecomes an async generator yielding(row_index, result, timing)per completed task, consumed via Raynum_returns="streaming". - Rollout assembly —
_NemoGymStreamAccumulatorbuckets streamed rows byrow_index // num_generationsand emits each complete prompt group in restored input order, fail-louding on duplicate/out-of-range/mixed rows.run_nemo_gym_rollout_syncdrains the stream so sync GRPO/PPO/distillation keep whole-batch semantics (parity tests unchanged). - Collector — one batched worker per reserved target (replacing thread-per-group + semaphore) runs an asyncio loop in
_collect_rollout_batch: streams groups, enqueues each concurrently with bounded backoff, computes teacher logprobs off-loop, and retries failed Gym streams up to 3× without re-enqueuing accepted groups. - Identity — a monotonic
_ng_task_indexis stamped per prompt, checkpointed torollouts.pt, and restored asmax(saved counter, max buffered index + 1), so retries and restores never reuse a task identity.
Also riding along: NaN generation-logprobs now raise instead of silently passing through (rollout_max_attempts_to_avoid_lp_nan removed), and W&B full_result tables get their own opt-in flag (logger.wandb.log_nemo_gym_full_result_tables).
Team review (5 agents: RL-guidelines, async-loop audit, independent bug scan, test review, adversarial verification) with extra scrutiny on the async/streaming semantics.
Overall: the async machinery held up very well under a dedicated 9-point concurrency audit. Verified with evidence: group ordering/identity (duplicate/out-of-range/mixed-agent rows all rejected), retry semantics (no group enqueued twice, no silent loss, failures raise), monotonic _ng_task_index checkpointing (restore takes max(rollouts.pt, max buffered index + 1)), event-loop hygiene (teacher logprobs via asyncio.to_thread + per-teacher locks; all push tasks gathered), correct Ray streaming-generator usage, and the efficiency-timer thread-seconds vs wall-clock split. Both parity tests binding the new implementations to the legacy ones are byte-identical to main, and replay_buffer.py is untouched. 37 unit tests from this PR pass locally; the vLLM/nemo_gym-marked E2E parity tests were left to CI.
The inline comments cover: the W&B full_result logging behavior change and a ruff-format failure (also failing the PR's Lint check in CI).
Two informational notes (no action needed):
NemoGymis@ray.remote(max_restarts=-1, max_task_retries=-1); on actor restart Ray may replay already-yielded stream rows. This is safely converted into a clean batch retry by the accumulator's duplicate-row rejection — a one-line code comment noting the interaction would help future readers.timing/rollout/*metrics now ride on one group per batched worker instead of every group; the driver merge averages only over present keys, so values shift from per-prompt-group to per-worker-batch semantics without dilution.
Generated by Claude Code
|
@yfw generally lgtm (also thanks @bxyu-nvidia for the change). two extremely minor comments |
Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
…ts_clean # Conflicts: # nemo_rl/experience/rollouts.py
|
/ok to test 07601e9 |
|
/ok to test 5c602b3 |
What does this PR do ?
Stream NeMo-Gym rollout results back to NeMo RL as they complete, assemble them into prompt groups, and enqueue each complete group without waiting for the slowest prompt in the batch.
Thanks to @bxyu-nvidia for writing the initial version of this change.
What changed
falseso after this change, we will no longer log these results to wandb by default. To enable logging of these results to match the previous behavior, set this flag totrue.Issues
List issues that this PR closes (syntax):
Usage
# Add a code snippet demonstrating how to use thisBefore your PR is "Ready for review"
Pre checks:
Additional Information