[rl] Validate SkyRL role plan group/batch consistency - #8722
Conversation
n_samples_per_prompt < 2 leaves every rollout group with zero variance (no GRPO advantage signal), and a train_batch_size not divisible by the group size splits rollout groups across batch boundaries. Both fail silently deep in training; catch them at plan construction instead, following the existing SkyRLRetentionPolicy validation pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7ff524cca
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| n_samples_per_prompt: int | ||
|
|
||
| def __post_init__(self) -> None: | ||
| if self.n_samples_per_prompt < 2: |
There was a problem hiding this comment.
SkyRLRolePlan does not know the trainer.algorithm.advantage_estimator selected by SkyRLSpec.config_yaml, so this unconditional check also rejects configurations using a non-group-relative estimator, where one rollout per prompt is valid. Perform this validation only after the algorithm is known, or encode the estimator in the validated plan.
Useful? React with 👍 / 👎.
| f"n_samples_per_prompt must be at least 2 so each rollout group has " | ||
| f"within-group variance for the GRPO advantage; got {self.n_samples_per_prompt}" | ||
| ) | ||
| if self.train_batch_size % self.n_samples_per_prompt != 0: |
There was a problem hiding this comment.
train_batch_size counts prompts selected for a rollout batch, while n_samples_per_prompt controls how many completions are generated for each selected prompt. For example, 18 prompts with four samples still produces 18 complete four-member groups; no group crosses a batch boundary. This modulus check therefore rejects valid SkyRL configurations without protecting the GRPO grouping invariant.
Useful? React with 👍 / 👎.
| def test_skyrl_role_plan_requires_within_group_sampling() -> None: | ||
| """A single sample per prompt leaves every group with zero variance, hence no GRPO advantage.""" | ||
|
|
||
| assert _role_plan().n_samples_per_prompt == 4 |
There was a problem hiding this comment.
This assertion only confirms that _role_plan() returns the literal 4 assigned inside that helper, so it would fail on an unrelated fixture update while providing no coverage of the new validation; the following pytest.raises already exercises the behavior under test. Remove this assertion to avoid pinning an incidental fixture value.
AGENTS.md reference: AGENTS.md:L203-L208
Useful? React with 👍 / 👎.
1 participant
[rl] Validate SkyRL role plan group/batch consistency
Summary
__post_init__validation toSkyRLRolePlan, following the existingSkyRLRetentionPolicyvalidation pattern.n_samples_per_prompt >= 2— a group of one has zero within-group variance, so every GRPO advantage is zero and the run silently learns nothing.train_batch_size % n_samples_per_prompt == 0— a batch boundary slicing through a rollout group corrupts the group-relative advantage.Why
Both misconfigurations currently fail silently (or surface much later as mysteriously flat rewards / zero-advantage steps), which is hard to diagnose from cluster logs. Catching them at plan-construction time turns a multi-hour debugging session into an immediate, actionable error.
The group-size invariant follows directly from the GRPO objective: the advantage is computed within each rollout group, so a degenerate group (single sample) or a group split across batch boundaries invalidates the estimator rather than crashing the run.
Testing
test_skyrl_role_plan_requires_within_group_sampling— rejectsn_samples_per_prompt=1test_skyrl_role_plan_requires_whole_rollout_groups_per_batch— rejectstrain_batch_size=18with groups of 4iceball_microrole plan (batch 16 / groups 4) still validtest_skyrl_step_fingerprint_...'sdataclasses.replace(..., train_batch_size=32)(mini 16 < train 32) still valid — the mini-vs-train relation is intentionally not validated, sincemini < trainis legitimate under multi-epoch iteration over a batch