Skip to content

Conversation

parthchadha
Copy link
Contributor

@parthchadha parthchadha commented May 5, 2025

Add division by temperature in training model to reflect that training and generation are computing the same thing

logprob error with this change:
Screenshot 2025-05-05 at 9 44 17 AM

Without this change:
Screenshot 2025-05-05 at 9 55 58 AM

What does this PR do ?

Add a one line overview of what this PR aims to accomplish.

Issues

List issues that this PR closes (syntax):

Usage

  • You can potentially add a usage example below
# Add a code snippet demonstrating how to use this 

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you run the unit tests and functional tests locally? Visit our Testing Guide for how to run tests
  • Did you add or update any necessary documentation? Visit our Document Development Guide for how to write, build and test the docs.

Additional Information

  • ...

@parthchadha parthchadha requested a review from terrykong May 5, 2025 16:44
@parthchadha parthchadha added the CI:L0 Run doctests and unit tests label May 5, 2025
…aining and generation are computing the same thing

Signed-off-by: Parth Chadha <pchadha@nvidia.com>
@parthchadha parthchadha force-pushed the pchadha/training-temp-fix branch from c0ed20d to 34c97e5 Compare May 5, 2025 17:01
@parthchadha parthchadha added CI:L0 Run doctests and unit tests and removed CI:L0 Run doctests and unit tests labels May 5, 2025
SahilJain314
SahilJain314 previously approved these changes May 5, 2025
Copy link
Contributor

@SahilJain314 SahilJain314 left a comment

Choose a reason for hiding this comment

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

doubled checked that tests use the temperature parameter. LGTM

@parthchadha parthchadha enabled auto-merge May 5, 2025 17:47
Signed-off-by: Parth Chadha <pchadha@nvidia.com>
@parthchadha parthchadha added CI:L0 Run doctests and unit tests and removed CI:L0 Run doctests and unit tests labels May 5, 2025
terrykong
terrykong previously approved these changes May 5, 2025
@parthchadha parthchadha added this pull request to the merge queue May 5, 2025
SahilJain314
SahilJain314 previously approved these changes May 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 5, 2025
@parthchadha parthchadha added this pull request to the merge queue May 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 5, 2025
…ot need generation

Signed-off-by: Parth Chadha <pchadha@nvidia.com>
@parthchadha parthchadha dismissed stale reviews from SahilJain314 and terrykong via 75a7582 May 5, 2025 23:24
@parthchadha parthchadha added CI:L0 Run doctests and unit tests and removed CI:L0 Run doctests and unit tests labels May 5, 2025
@parthchadha parthchadha added this pull request to the merge queue May 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 6, 2025
@parthchadha parthchadha added this pull request to the merge queue May 6, 2025
Merged via the queue into main with commit 4e02da0 May 6, 2025
21 checks passed
@parthchadha parthchadha deleted the pchadha/training-temp-fix branch May 6, 2025 18:20
YzjiaoNvd pushed a commit to YzjiaoNvd/NeMo-RL that referenced this pull request Jun 10, 2025
Signed-off-by: Parth Chadha <pchadha@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI:L0 Run doctests and unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants