Skip to content

fix: place PPO terminal rewards on the owning CP rank - #2375

Open
MrCapricornLiu wants to merge 1 commit into
THUDM:mainfrom
MrCapricornLiu:lch/fix-ppo-terminal-reward-cp
Open

MrCapricornLiu wants to merge 1 commit into
THUDM:mainfrom
MrCapricornLiu:lch/fix-ppo-terminal-reward-cp

Conversation

@MrCapricornLiu

Copy link
Copy Markdown

PPO currently adds the scalar reward to CP rank 0. With zigzag context-parallel slicing, padding can put the final response prediction on another rank. For example, total length 9 and response length 3 at CP=2 leave rank 0 with no response predictions, so indexing its last reward raises IndexError. If rank 0 owns earlier response predictions, the reward is instead attached to the wrong time step.

Use the existing CP token offsets to find the shard containing the final response token. CP=1 retains the existing behavior, and the per-token KL reward remains unchanged.

Validation: the added CPU/Gloo regression runs the real advantage/return calculation at CP=1, 2 and 4, comparing five sequence layouts with an independent unsharded GAE recurrence. The original code fails the CP=2/4 cases; the updated test module passes all 4 tests. The CP utility, discounted-return and CP whitening modules also pass (56 tests). Changed-file pre-commit hooks pass.

The distributed tests use the repository's MPU stub for rank/group metadata and real Gloo collectives. They do not run a Megatron model, NCCL, or a full PPO training job.

OMP_NUM_THREADS=1 PYTHONPATH=.:tests pytest -q tests/test_ppo_kl_metric.py
OMP_NUM_THREADS=1 PYTHONPATH=.:tests pytest -q tests/test_cp_utils.py tests/test_discounted_returns.py tests/test_advantage_whiten_cp.py

This branch has not been deployed

No deployments
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