Skip to content

Conversation

SahilJain314
Copy link
Contributor

I noticed that the loss_type of the GRPO loss was impacting how metrics were calculated. Changed all losses to always accept both global token and sequence counts so that metrics can be calculated based on whichever they should be. Also fixed a merge artifact that was causing sampling_importance_weights to be miscalculated.

…ther or

Signed-off-by: Sahil Jain <sahilj@nvidia.com>
Signed-off-by: Sahil Jain <sahilj@nvidia.com>
@SahilJain314 SahilJain314 requested review from terrykong and ashors1 May 13, 2025 20:06
Signed-off-by: Sahil Jain <sahilj@nvidia.com>
@SahilJain314 SahilJain314 changed the title fix: Fixed sampling_importance_weights metric calculation and made all grpo metrics token-level feat: Fixed sampling_importance_weights metric calculation and made all grpo metrics token-level May 13, 2025
@SahilJain314 SahilJain314 added CI:L0 Run doctests and unit tests and removed CI:L0 Run doctests and unit tests labels May 13, 2025
@SahilJain314 SahilJain314 changed the title feat: Fixed sampling_importance_weights metric calculation and made all grpo metrics token-level feat: Fixed metric calculation and made all grpo metrics token-level May 13, 2025
Signed-off-by: Sahil Jain <sahilj@nvidia.com>
@SahilJain314 SahilJain314 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels May 14, 2025
Signed-off-by: Sahil Jain <sahilj@nvidia.com>
@SahilJain314 SahilJain314 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels May 14, 2025
ashors1
ashors1 previously approved these changes May 14, 2025
Signed-off-by: Sahil Jain <sahilj@nvidia.com>
parthchadha
parthchadha previously approved these changes May 14, 2025
…seq counts

Signed-off-by: Sahil Jain <sahilj@nvidia.com>
terrykong
terrykong previously approved these changes May 14, 2025
@terrykong terrykong added this pull request to the merge queue May 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 15, 2025
Signed-off-by: Sahil Jain <sahilj@nvidia.com>
@SahilJain314 SahilJain314 added this pull request to the merge queue May 15, 2025
Merged via the queue into main with commit b960794 May 15, 2025
13 checks passed
@SahilJain314 SahilJain314 deleted the sahilj/entropy_log_fix branch May 15, 2025 01:44
YzjiaoNvd pushed a commit to YzjiaoNvd/NeMo-RL that referenced this pull request Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants