Skip to content

feat(rollout): add min_valid_group_size to drop under-filled rollout groups - #1416

Closed
EazyReal wants to merge 1 commit into
areal-project:mainfrom
EazyReal:oss/partial-group-min-valid
Closed

feat(rollout): add min_valid_group_size to drop under-filled rollout groups#1416
EazyReal wants to merge 1 commit into
areal-project:mainfrom
EazyReal:oss/partial-group-min-valid

Conversation

@EazyReal

@EazyReal EazyReal commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Description

Adds min_valid_group_size for grouped rollouts. When a grouped rollout has too few non-None survivors, GroupedRolloutWorkflow can now drop the whole group instead of training on a partial group.

The default is 1, so existing behavior is unchanged: any non-empty partial group is kept. Set the value to gconfig.n_samples to require full groups. Values outside [1, group_size] raise instead of being clamped. The config is plumbed through both grouped rollout wrap sites, CLI reference docs are regenerated, and the Chinese rollout workflow reference documents the drop-policy behavior and YAML usage.

Current main already includes the variable-size group normalization layer from #1454, so this PR is now focused only on the optional drop policy.

Related Issue

Part of #1419.

Validation

  • uv run python docs/generate_cli_docs.py
  • uv run pytest -q tests/test_grouped_rollout_min_valid.py tests/test_reward_norm_variable_group.py (15 passed)
  • uv run ruff check areal/api/cli_args.py areal/infra/remote_inf_engine.py tests/test_grouped_rollout_min_valid.py
  • uv run ruff format --check areal/api/cli_args.py areal/infra/remote_inf_engine.py tests/test_grouped_rollout_min_valid.py
  • git diff --check origin/main...HEAD

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for dropping rollout groups that fall below a minimum size threshold (min_valid_group_size) and enables group-level normalization to handle partial or unequal groups correctly by passing actual group sizes. The review feedback suggests validating that all elements in group_sizes are positive integers, logging a warning when min_valid_group_size is silently clamped, and adding a defensive check for empty input data in compute_advantages.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread areal/utils/data.py Outdated
Comment thread areal/infra/remote_inf_engine.py Outdated
Comment thread areal/trainer/ppo/actor.py Outdated
@EazyReal
EazyReal force-pushed the oss/partial-group-min-valid branch from a26ede8 to 90f07e5 Compare June 16, 2026 07:50
@EazyReal

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed:

  • min_valid_group_size silent clamp — addressed, slightly stronger than suggested: a threshold above group_size is an impossible configuration, so GroupedRolloutWorkflow now raises (must be between 1 and group_size) instead of clamping. This fails fast and matches the group_size < 1 validation right above it.
  • Validate group_sizes are positive — done in the stacked fix commit (raises if any element is < 1).

Respectfully holding off on:

  • Empty-data guard in compute_advantages — pre-existing (identical to the prior batched_call path), not reachable in the training loop (prepare_batch blocks for a full batch), and the same pattern applies to compute_logp/compute_values; better as a separate hardening than a one-off guard here.

@EazyReal

Copy link
Copy Markdown
Contributor Author

@garrett4wade @nuzant @sitabulaixizawaluduo @rchardx @fishcrap @HwVanICI @guozhihao-224 Could I get a quick review on this rollout-group filtering PR? It drops under-filled groups before they skew group-based training.

@EazyReal
EazyReal force-pushed the oss/partial-group-min-valid branch from 60040c8 to 0d8d4ea Compare July 7, 2026 17:22
@EazyReal

EazyReal commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Rebuilt on current main; checks are green. @sitabulaixizawaluduo if you have bandwidth, would appreciate a review on the focused min_valid_group_size rollout drop-policy change.

Allow grouped rollouts to reject under-filled groups while keeping partial groups as the default. Document the configuration boundary and cover wrapper plumbing.

Signed-off-by: EazyReal <8047065+EazyReal@users.noreply.github.com>
@EazyReal
EazyReal force-pushed the oss/partial-group-min-valid branch from 0d8d4ea to 8777cb6 Compare July 21, 2026 05:48
Comment thread areal/api/cli_args.py
@EazyReal EazyReal closed this Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants