Skip to content

fix: preserve BSHD layout in PPO CP advantages#1752

Open
guapisolo wants to merge 1 commit into
fix/ppo-cp-terminal-rewardfrom
fix/ppo-bshd-cp-advantages
Open

fix: preserve BSHD layout in PPO CP advantages#1752
guapisolo wants to merge 1 commit into
fix/ppo-cp-terminal-rewardfrom
fix/ppo-bshd-cp-advantages

Conversation

@guapisolo

Copy link
Copy Markdown
Collaborator

Summary

Preserve BSHD token ownership while computing PPO advantages with context parallelism.

Symptom & Reproduction

  • Symptom: BSHD PPO with CP > 1 returned response-aligned outputs in THD token order.
  • Reproduction: pytest -q -p no:cacheprovider tests/fast/backends/training_utils/test_ppo_cp_advantages.py::test_ppo_bshd_cp_uses_padded_layout_metadata

Root Cause

  1. compute_advantages did not forward qkv_format or max_seq_lens.
  2. get_advantages_and_returns_batch used THD defaults for gather and re-slice.

Fix

Propagate max_seq_lens from rollout data and pair it with args.qkv_format throughout PPO GAE. Both CP gather and re-slice now call the existing layout-aware helpers with identical per-sample metadata, while THD and CP=1 keep their previous behavior.

Verification

  • New / updated test: test_ppo_bshd_cp_uses_padded_layout_metadata compares reconstructed CP=2 BSHD outputs with CP=1 baselines.
  • Existing test suite: tests/fast/backends/training_utils/test_ppo_cp_advantages.py passes all three cases.

Review Focus

  • Scrutinize metadata propagation from compute_advantages_and_returns to get_advantages_and_returns_batch.
  • Scrutinize per-sample max_seq_lens pairing in both CP gather and re-slice.

PPO GAE dropped qkv_format and per-sample max sequence lengths before CP gather and slicing, so BSHD shards were interpreted with THD offsets.

Propagate the layout metadata through advantage computation and add a two-rank regression against a CP=1 baseline.
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

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