Repository navigation
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
thanks, as discussed yesterday, please link/close all the issues that asked for "dynamic sampling" |
…o-advantage # Conflicts: # tests/experimental/test_async_grpo_trainer.py
…o-advantage # Conflicts: # docs/source/async_grpo_trainer.md
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit aa84c8a. Configure here.
qgallouedec
left a comment
qgallouedec
left a comment
There was a problem hiding this comment.
Thanks, lgtm overall. A few things before merging:
reward_std == 0.0(L1224) misses some constant groups: with a constant reward of 0.1 and 3, 6, 7, 12 or 64 generations, numpy gives a std around 1e-17 🥲. Checking that all scored rewards are equal is exact.- As planned in #7841 and #7835, let's log GRPO's
frac_reward_zero_std(a fraction of groups) instead ofrollout/zero_advantage_groups_total. rewardnow covers every scored group, butreward_stdandrewards/<func>still come from the kept samples (L1259-L1260), soreward_stdreads higher than in GRPO. Can we compute them in the worker before the filter, likereward, and dropreward_kept?- The description still has the old metric names and says
rewardcovers trained groups only. "Closes #3708" also needs to be in the description
lewtun
left a comment
lewtun
left a comment
There was a problem hiding this comment.
Thanks for adding this @AmineDiro ! I left a few comments on top of Quentin's review.
Turbo nit on naming: we use "filtered" in the code (_filtered_groups, TestFilterZeroAdvantageGroups), "dropped" in the docs and "zero advantage" in the metric. Should we stick to just one wording consistently (e.g. dropped)?
| | `rollout/vllm_preemptions_total` | requests the server preempted during the logging window (vLLM's `num_preemptions_total`, summed over engines). Reported only: a weight sync resets the server's caches, which preempts every running request, so it is not a pressure signal | | ||
| | `rollout/zero_advantage_groups_total`, `reward_kept` | groups dropped because every rollout got the same reward, and the mean reward of the groups that were trained on. `reward` is the policy's reward over every scored group, dropped ones included, so it is the one to track | | ||
|
|
||
| A group whose rollouts all received the same reward (or none of which could be scored) has zero advantage everywhere, so the worker drops it instead of enqueuing it: it would cost a forward and backward and move nothing. The loop does not resample to replace a dropped group, so `batch/groups_per_step` goes down while the batch keeps its `samples_per_step` samples. A dropped group still counts toward an epoch, which stays one pass over the prompts. |
There was a problem hiding this comment.
Are you sure batch/groups_per_step goes down? IIUC each step still fills up to samples_per_step from the kept groups, and the metric counts trained groups
There was a problem hiding this comment.
You're right! I mainly asked claude to right this feature it seemed to be easy enough to one shot 🫠 🫠 🫠 Thanks for reviewiing I should looked at this closer.
Fixed
| self._rates["reward"][1] += int(scored_mask.sum()) | ||
| if not scored_mask.any() or reward_std == 0.0: | ||
| self._counters["rollout/zero_advantage_groups_total"] += 1 | ||
| self._filtered_groups.put_nowait(group.group_id) |
There was a problem hiding this comment.
IIUC a dropped group is counted as soon as it's scored, but a kept group only once it's trained?
An alternative would be to put the dropped group_id on rollout_buffer instead and have RolloutQueueDataset.__iter__ record it and continue. Then drops are counted in order and we don't need the filtered_groups queue at all.
FYI the new filtered_groups arg also breaks the _HarnessRolloutLoop(...) calls in test_openenv_tito.py. Not sure if the OpenEnv integration is considered critical since it's not tested in the CI :)
| self._rates["reward"][0] += float(np.nansum(rewards)) | ||
| self._rates["reward"][1] += int(scored_mask.sum()) | ||
| if not scored_mask.any() or reward_std == 0.0: | ||
| self._counters["rollout/zero_advantage_groups_total"] += 1 |
There was a problem hiding this comment.
FYI if we switch to frac_reward_zero_std as @qgallouedec suggested, GRPO doesn't count groups with zero or one scored completions as zero std, since nanstd returns NaN for them. We drop these groups here, so the metric would undercount the drops.
There was a problem hiding this comment.
Ok, I see. I kept the GRPO naming but defined it as the fraction of groups the worker actually drops which is slighttly different indeed:
The drop here checks if all scored rewards are equal, so a group with 1 scored completion, where nanstd would give NaN, is counted as well (something like [None, 1, None, None]). I think dropping it makes sense, since it also has zero advantage for all samples.
updated the docs 👍🏼
…o-advantage # Conflicts: # tests/experimental/test_async_grpo_trainer.py
The rollout worker now drops a group whose rollouts all got the same reward, or none of which could be scored, before it reaches the trainer. Every advantage in such a group is zero, so it costs a forward and backward and moves nothing. This is DAPO's dynamic sampling without resampling to a fixed group count.
The loop does not replace a dropped group, so
batch/groups_per_stepgoes down while the batch keepssamples_per_stepsamples. Filtered groups never reach the collator, so an epoch takes more prompts thanlen(train_dataset).Metrics:
rollout/groups_filtered_zero_advantagecounts dropped groups andreward_filteredis their mean reward.rewardcovers trained groups only, so the two are meant to be read together.Note
Medium Risk
Changes rollout queue protocol, epoch/checkpoint counting, and training batch composition; mis-handling dropped group IDs could skip prompts or skew metrics, but behavior is covered by new tests.
Overview
Async GRPO skips prompt groups with no learning signal (DAPO-style dynamic sampling): after scoring, the rollout worker drops groups where every scorable rollout got the same reward (zero advantage everywhere) instead of enqueueing
RolloutSamples. Dropped groups are signaled onrollout_bufferas a baregroup_idint; the trainer records them in_dropped_groupsso they still count as consumed fornum_train_epochs, checkpointrollout_state.jsonresume cursor, and epoch-stop logic—without running forward/backward on useless batches. Training steps still fillsamples_per_stepfrom kept groups, so heavy filtering means more generation per step.Metrics: per-sample trained reward is renamed to
reward_kept; aggregaterewardstill reflects all scored rollouts (including dropped groups);frac_reward_zero_stdtracks how often groups are filtered.num_generations < 2is now rejected at config validation.Reviewed by Cursor Bugbot for commit c882082. Bugbot is set up for automated code reviews on this repo. Configure here.