Skip to content

fix(openai): preserve multi-turn step rewards and raw baselines in group normalization - #1674

Open
hsusul wants to merge 4 commits into
areal-project:mainfrom
hsusul:fix/normalize-group-rewards-multi-turn
Open

hsusul wants to merge 4 commits into
areal-project:mainfrom
hsusul:fix/normalize-group-rewards-multi-turn

Conversation

@hsusul

@hsusul hsusul commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Description

In multi-turn agent rollouts (e.g. tool-using agents exported with style='individual'), an episode contains multiple interactions across successive turns (e.g., tool-call turns with intermediate step rewards, followed by a final answer turn with an outcome reward).

Previously, normalize_group_rewards in areal/experimental/openai/types.py computed group statistics from the last interaction of each rollout (last_id = next(reversed(result))), but then looped over every interaction in the episode:

for result, normalized_reward in zip(results, normalized_rewards):
    for interaction in result.values():
        interaction.original_reward = interaction.reward
        interaction.reward = normalized_reward

This caused two critical issues:

  1. Destruction of Step-Level Rewards in Multi-Turn Episodes:
    Every intermediate interaction in the episode (e.g., Turn 1 penalty of -0.2) was completely overwritten by the terminal turn's normalized outcome reward (e.g., +1.0). Intermediate turns lost their distinct reward attribution and were falsely assigned the final answer's normalized score.
  2. Overwriting Existing Raw Baselines:
    interaction.original_reward was unconditionally assigned interaction.reward, destroying any previously recorded raw reward baselines (e.g. from prior reward discounting or step-level evaluation).

This PR fixes these issues:

  1. Targeted Terminal Normalization: In multi-turn rollouts (len(result) > 1), only the terminal interaction (cid == last_id) that provided the group outcome score is updated with the group-normalized reward. Intermediate interactions preserve their distinct step-level rewards and attribution.
  2. Preserve Existing original_reward: interaction.original_reward is only set if it was not already populated.
  3. Cache Synchronization: Ensures updated rewards and original rewards are cleanly reflected in interaction._cache.
  4. Exact Backward Compatibility: Single-turn rollouts (len(result) == 1) or concat-mode rollouts maintain 100% numerical and behavioral equivalence with baseline.

Verification Plan

  • Unit test suite in tests/experimental/openai/test_normalize_group_rewards.py:
    • test_multi_turn_preserves_intermediate_step_rewards: verifies intermediate step rewards are preserved while terminal turn is normalized.
    • test_existing_original_reward_not_overwritten: verifies pre-existing raw reward baselines are retained.
    • test_single_turn_exact_parity: verifies numerical identity on single-turn groups.
    • test_empty_or_incomplete_group_handling: verifies handling of missing/None rewards.
  • Ran uv run pytest tests/experimental/openai/test_normalize_group_rewards.py tests/experimental/openai/test_cache.py (16 passed).
  • Passed all 16 pre-commit hooks (ruff check, ruff format, check-yaml, etc.).

- Use lazy evaluation in testing model path dictionaries to avoid eager downloads on test collection
- Safely handle PackageNotFoundError in version check helper functions
…oup normalization

- In multi-turn rollouts (len(result) > 1), only normalize the terminal interaction that supplied the rollout outcome reward, preserving intermediate step-level rewards
- Prevent overwriting pre-existing interaction.original_reward baselines
- Synchronize normalized values and original rewards to interaction._cache
- Add unit tests verifying intermediate step-level reward preservation, original_reward retention, and single-turn parity
@Le8r0nJames

Copy link
Copy Markdown
Collaborator

Could you clarify the intended training objective here? In the v2 export path, reward discounting runs before normalization, so earlier interaction rewards may already represent returns that include future rewards, rather than independent step scores.
For outcome-only multi-turn rollouts, this change leaves earlier turns with unnormalized returns while only the last turn receives the group-normalized reward. That changes the existing training semantics even when no step rewards are present.
Could we preserve the existing behavior for outcome-only rollouts and introduce an explicit normalization policy for step rewards, with tests covering both cases?

Default to the historical broadcast behavior so outcome-only rollouts keep
their existing training semantics, and add an opt-in "terminal" policy for
rollouts that carry genuine step-level rewards.
@hsusul
hsusul force-pushed the fix/normalize-group-rewards-multi-turn branch from 079a737 to 12ccd72 Compare September 11, 2026 23:35
@hsusul

hsusul commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

You're right, and thanks for catching it. After apply_reward_discount the earlier interactions hold discounted returns, not independent step scores, so unconditionally exempting them from normalization changed the objective for outcome-only rollouts.

I've made the policy explicit instead. normalize_group_rewards now takes step_reward_normalization, defaulting to "broadcast" — byte-for-byte the previous behavior, with every interaction receiving the group-normalized rollout reward. "terminal" is opt-in and only normalizes the last interaction, for setups where intermediate turns carry real step rewards. Group statistics are unchanged in both cases (one scalar per rollout, from the terminal interaction). Tests cover both paths, and no caller passes the non-default yet, so this PR is now behavior-preserving on every existing path.

The one remaining behavior change is original_reward: it's now only populated when still unset, so a raw baseline recorded earlier in the pipeline survives. That's a no-op on the current single-call path. Happy to split it out if you'd rather keep this PR purely additive.

Comment thread areal/utils/testing_utils.py Outdated
Comment on lines +86 to +92
class _LazyModelDict(dict):
"""Dictionary that lazily resolves model paths upon access."""

def __init__(self, specs: dict[str, tuple[str, str]]):
super().__init__()
self._specs = specs

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The unrelated changes to testing_utils.py and pkg_version.py are still present here.
Please review all related PRs and remove these changes wherever they remain, rather than addressing them one PR at a time. Please also check each PR's full diff for scope and correctness before requesting another review.

These files are not part of group-reward normalization
and already landed on main via areal-project#1568.
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