fix(rollout): require complete groups for group estimators - #48
Open
k21993 wants to merge 1 commit into
Open
Conversation
Group completeness was previously checked only when dynamic filtering was enabled. As a result, group-advantage estimators could compute advantages from fewer than n_samples_per_prompt usable rollouts when dynamic filtering was disabled. Require complete groups when training with a group estimator, while preserving the existing completeness requirement for dynamic filtering. Per-sample estimators without dynamic filtering and evaluation continue to retain usable partial groups. Preserve pre-filter score-stat collection before the completeness decision. Signed-off-by: Karthik Suresh <7954591+k21993@users.noreply.github.com>
Contributor
Author
|
Hi @hijkzzz, just wanted to follow up on this PR. Would appreciate your review and any feedback when you get a chance. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Group completeness (a group must keep all
n_samples_per_promptrollouts) was previouslychecked only when
--algo.dynamic_filtering_enablewas set. With it unset (the default), agroup-advantage estimator (
grpo/dr_grpo/reinforce_baseline/rloo) could computeadvantages from fewer than
n_samples_per_promptusable rollouts when a rollout was lost toan unrelated per-response drop (
vlm_truncation,logprob_misalign,no_action_tokens, ...),computing advantages from a partial group and changing the intended group statistics/baselines.
Require complete groups when training with a group estimator, while preserving the existing
completeness requirement whenever dynamic filtering is on (its uniform-vs-mixed decision is
itself group-based). Per-sample estimators without dynamic filtering, and eval (
_generate_batch,which never backfills), retain usable partial groups. Pre-filter score-stat collection stays
before the completeness decision so incomplete-but-scored groups still count toward the logged rate.
Test plan
tests/unit/test_samples_generator.py: incomplete group dropped when a group estimatorrequires it; dropped whenever dynamic filtering is on (even for a per-sample estimator, with
mixed scores that would otherwise pass the DAPO filter); kept for a per-sample estimator with
DF off; kept for eval; complete group kept; incomplete-but-scored group still recorded in score_stats.
python -m pytest -q: 196 passed, 2 failed; both failures (test_cp_thd_packing) reproduceunchanged on
origin/mainand are unrelated to this change.