Skip to content

fix(ppo): reject implicit partial group normalization - #1415

Open
EazyReal wants to merge 1 commit into
areal-project:mainfrom
EazyReal:oss/partial-group-investigation
Open

fix(ppo): reject implicit partial group normalization#1415
EazyReal wants to merge 1 commit into
areal-project:mainfrom
EazyReal:oss/partial-group-investigation

Conversation

@EazyReal

@EazyReal EazyReal commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Description

Current main already includes variable-size group normalization through #1454. This PR keeps the remaining invariant: when group-level Normalization is called without explicit group_sizes, it is using fixed-size groups, so the batch size must be divisible by group_size.

Before this guard, a direct fixed-stride call with a partial tail could leave tail rows with zero-filled normalization state. The PPO rollout path now passes TrajBatchMeta.traj_group_sizes, so this is a small boundary hardening for direct callers and future refactors.

Superseded Context

The original broader #1415 head was 3e3ab94b257acf1a77dc5a9fdecf7cfea2a6f7f1. That broader variable-group normalization change was superseded by #1454, merged to main as bbc10f0e95bf350d71169efcb2facb9756f1238b (PR commit 375cc12a4b91da50614aa9d2211ab9995475c942). This PR is now only the remaining non-duplicative guard.

Validation

  • uv run pytest -q tests/test_reward_norm_variable_group.py tests/test_adv_norm_config.py (214 passed)
  • uv run ruff check areal/utils/data.py tests/test_reward_norm_variable_group.py
  • uv run ruff format --check areal/utils/data.py tests/test_reward_norm_variable_group.py
  • git diff --check origin/main...HEAD

Related Issue

Part of #1419.

@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 group-level normalization with unequal or partial groups by passing actual per-group row counts (group_sizes) through the PPO actor and normalization utilities. It replaces batched_call with explicit batch concatenation and splitting to recover trajectory group sizes, updates the normalization logic to resolve dynamic group boundaries, and adds comprehensive unit tests. The review feedback suggests adding a guard for empty input data in compute_advantages to prevent potential crashes, and validating that all elements in group_sizes are strictly positive to avoid runtime errors during tensor expansion.

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/trainer/ppo/actor.py Outdated
Comment thread areal/utils/data.py Outdated
@EazyReal
EazyReal force-pushed the oss/partial-group-investigation branch from 35b7b7b to 730acf9 Compare June 16, 2026 07:44
@EazyReal

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed:

  • Validate group_sizes are positive — done: _resolve_group_bounds now raises if any element is < 1, alongside the existing sum check.

Respectfully holding off on:

  • Empty-data guard in compute_advantages — this is pre-existing behavior (the previous batched_call(self._compute_advantages, data) had the identical empty-input path), it isn't reachable in the training loop (prepare_batch blocks until a full batch of accepted results), and the same assumption applies to compute_logp/compute_values. Guarding only this method would be inconsistent; if empty-batch handling is wanted it belongs in a separate, deliberate change. Happy to add it if you'd prefer.

@EazyReal
EazyReal force-pushed the oss/partial-group-investigation branch 3 times, most recently from e461748 to 3e3ab94 Compare June 30, 2026 08:39
@EazyReal

Copy link
Copy Markdown
Contributor Author

@garrett4wade @sitabulaixizawaluduo @rchardx @fishcrap Could I get a quick review on this partial-group PPO normalization fix? It uses actual group sizes for under-filled rollout groups.

@EazyReal
EazyReal force-pushed the oss/partial-group-investigation branch from 3e3ab94 to f947377 Compare July 7, 2026 17:26
@EazyReal EazyReal changed the title fix(ppo): group-normalize by actual group sizes for partial groups fix(ppo): reject implicit partial group normalization Jul 7, 2026
@EazyReal

EazyReal commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Retargeted this to the remaining non-duplicative guard on current main. @sitabulaixizawaluduo if you have bandwidth, would appreciate a review on the fixed-stride group-normalization boundary check.

Group-level normalization without explicit group_sizes uses fixed-size groups. Reject non-divisible batch sizes instead of leaving a partial tail with zero-filled normalization state.

Signed-off-by: EazyReal <8047065+EazyReal@users.noreply.github.com>
@EazyReal
EazyReal force-pushed the oss/partial-group-investigation branch from f947377 to a831901 Compare July 21, 2026 05:59
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity within the last 14 days.

Please add a comment or push new commits to keep it active.

Thank you for your contribution!

@EazyReal

Copy link
Copy Markdown
Contributor Author

The hazard this guards is still live on main (2ee4904): with group_sizes=None and a batch not divisible by group_size, Normalization._build_group_slices covers only bs // group_size groups and __call__ returns the uncovered tail as x/(0+eps) — reproduced on main: bs=3, group_size=2 silently gives the orphan row an advantage of ~2e5. #1454 added the explicit group_sizes branch but left this fallback unguarded. This PR is the minimal fix — a 5-line divisibility guard plus a regression test, mirroring the invariant train_controller.py already enforces one layer up. One note: open #1563 currently carries the same hunk; whichever lands first, the other drops it. @sitabulaixizawaluduo could you take a look?

@github-actions github-actions Bot removed the stale label Aug 12, 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.

1 participant