fix: place PPO terminal rewards on the owning CP rank - #2375
Open
MrCapricornLiu wants to merge 1 commit into
Open
MrCapricornLiu wants to merge 1 commit into
MrCapricornLiu wants to merge 1 commit into
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.