-
Notifications
You must be signed in to change notification settings - Fork 580
fix(rollout): train safely on incomplete groups #1563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 7 commits
b0dbd4c
6b11d22
509ab30
9392e20
677c92a
6aa093d
e8a483c
f7ec6b3
a6b354a
3a29737
31106b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,11 @@ class NormConfig: | |
| default=1, metadata={"help": "Group size for group-level normalization"} | ||
| ) | ||
|
|
||
| @property | ||
| def uses_group_statistics(self) -> bool: | ||
| """Whether normalization derives statistics from prompt groups.""" | ||
| return self.mean_level == "group" or self.std_level == "group" | ||
|
|
||
| def __post_init__(self): | ||
| """Validate normalization configuration.""" | ||
| valid_levels = {"batch", "group", None} | ||
|
|
@@ -1675,6 +1680,17 @@ class PPOActorConfig(TrainEngineConfig): | |
| default=None, metadata={"help": "Normalization configuration for advantages."} | ||
| ) | ||
|
|
||
| # Partial rollout groups | ||
| min_usable_group_size: int | None = field( | ||
| default=None, | ||
| metadata={ | ||
| "help": "Minimum usable rollout slots a prompt group must keep to stay " | ||
| "trainable when some slots fail or are filtered. None derives the " | ||
| "minimum from reward_norm/adv_norm: 2 when either uses group " | ||
| "statistics (1 for a singleton target group), else 1." | ||
| }, | ||
| ) | ||
|
|
||
| # KL Control | ||
| kl_ctl: float = field(default=0.1, metadata={"help": "KL divergence coefficient"}) | ||
| kl_estimator: str = field( | ||
|
|
@@ -1766,6 +1782,30 @@ class PPOActorConfig(TrainEngineConfig): | |
| metadata={"help": "Maximum number of new tokens to generate"}, | ||
| ) | ||
|
|
||
| def resolve_min_usable_group_size(self, target_group_size: int) -> int: | ||
| """Minimum usable rollout slots a group must keep to stay trainable. | ||
|
|
||
| An explicit ``min_usable_group_size`` wins. Otherwise group-relative | ||
| normalization needs at least two group members before partial groups | ||
| become a hazard; a singleton target group is complete by definition, | ||
| so it keeps the minimum of one. | ||
| """ | ||
| if self.min_usable_group_size is not None: | ||
| return self.min_usable_group_size | ||
| for normalization in (self.reward_norm, self.adv_norm): | ||
| if normalization is None: | ||
| continue | ||
| if isinstance(normalization, (dict, DictConfig)): | ||
| uses_group_statistics = ( | ||
| normalization.get("mean_level") == "group" | ||
| or normalization.get("std_level") == "group" | ||
| ) | ||
| else: | ||
| uses_group_statistics = normalization.uses_group_statistics | ||
| if uses_group_statistics: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a forced modification of the user's settings. If the user does not want to use this feature, they must obtain n_samples trajectories; otherwise, the behavior will be subtly altered unless it explicitly sets min_usable_group_size=n_samples.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The strict behavior keeps its existing knob: On the default path, main today does not obtain n_samples trajectories either — a partial group is kept with a warning ("using remaining results" in If you would rather have the default strict — only complete groups train unless the user opts in — I am happy to flip the derivation to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A more precise ledger, with one correction to my previous reply: the statistics fix for partial groups is not this PR — it already landed on main via #1454, which passes actual survivor counts into group normalization through So the choice is narrower than it looked:
I recommend 1 and have left the PR in that state; happy to switch to 2 if you prefer strict preservation. |
||
| return min(2, target_group_size) | ||
| return 1 | ||
|
|
||
| def should_compute_prox_logp(self) -> bool: | ||
| """Determine if forward pass is needed for proximal log-probabilities. | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add an assert to check that if
group normalizationis used,min_usable_group_sizemust be greater than 1 ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in f7ec6b3 —
PPOActorConfig.__post_init__now raises when an explicitmin_usable_group_sizeis below 2 whilereward_norm/adv_normuses group statistics (and rejects non-positive values generally). The derived default is unchanged.While re-checking the field's blast radius I also tightened two adjacent spots in 3a29737: the v2 rollout path never consumes the option, so an explicit setting there now fails fast instead of being silently ignored (matching
RolloutControllerV2's handling ofreward_normalization/drop_incomplete_group), and the slot-cardinality error plus docs now name the resolved minimum — not group normalization per se — as the trigger, since the two can diverge once the field is set explicitly.