Skip to content

update nemo-rl#972

Merged
Kipok merged 1 commit intomainfrom
wedu/update-nemo-rl
Oct 19, 2025
Merged

update nemo-rl#972
Kipok merged 1 commit intomainfrom
wedu/update-nemo-rl

Conversation

@wedu-nvidia
Copy link
Collaborator

@wedu-nvidia wedu-nvidia commented Oct 18, 2025

Update to latest commit or nemo-rl

Summary by CodeRabbit

  • Chores

    • Updated NeMo-RL dependency to a newer version.
  • New Features

    • Added support for configuring chat template options in training configurations for GRPO and SFT models.

Signed-off-by: Wei Du <wedu@nvidia.com>
@wedu-nvidia wedu-nvidia requested a review from Kipok October 18, 2025 01:39
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 18, 2025

Walkthrough

Updated the NeMo-RL Docker build commit reference and added a new optional chat_template_kwargs configuration field to two YAML config files under policy.tokenizer to support passing kwargs to chat templates.

Changes

Cohort / File(s) Summary
Docker dependency update
dockerfiles/Dockerfile.nemo-rl
Bumped NEMO_RL_COMMIT from 838475ff... to 85eeb8d0... to checkout a different repository state
Chat template configuration
nemo_skills/training/nemo_rl/configs/grpo.yaml, nemo_skills/training/nemo_rl/configs/sft.yaml
Added chat_template_kwargs: null field under policy.tokenizer in both files to enable passing optional kwargs to the chat template handler

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A commit update hops through the build,
Chat templates now wear kwargs as their guild,
Two configs dressed alike, side by side,
Simple changes with nothing to hide! 🌟

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "update nemo-rl" directly relates to the primary change in the changeset, which is updating the NEMO_RL_COMMIT hash in the Dockerfile.nemo-rl file to a newer commit. The supporting configuration changes (adding chat_template_kwargs fields) are complementary modifications that enable functionality in the updated NeMo-RL version. The title is concise, clear, and specific enough that a teammate reviewing the history would understand this PR concerns updating the NeMo-RL dependency. While the title is brief, it accurately summarizes the main change without being misleading or overly vague.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch wedu/update-nemo-rl

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8b18f39 and 22b02d5.

📒 Files selected for processing (3)
  • dockerfiles/Dockerfile.nemo-rl (1 hunks)
  • nemo_skills/training/nemo_rl/configs/grpo.yaml (1 hunks)
  • nemo_skills/training/nemo_rl/configs/sft.yaml (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: unit-tests
  • GitHub Check: pre-commit
🔇 Additional comments (3)
nemo_skills/training/nemo_rl/configs/sft.yaml (1)

31-31: Config addition is backward compatible and well-documented.

The new chat_template_kwargs field defaults to null and aligns with the parallel addition in grpo.yaml, enabling optional customization of chat template behavior without breaking existing configurations.

nemo_skills/training/nemo_rl/configs/grpo.yaml (1)

46-46: Config addition consistent across both YAML files.

The chat_template_kwargs: null addition mirrors the change in sft.yaml, ensuring uniform tokenizer configuration across SFT and GRPO training modes.

Please verify that the training code properly reads and handles this new optional configuration field. You may search the codebase for where chat_template_kwargs is consumed to confirm it's properly integrated.

dockerfiles/Dockerfile.nemo-rl (1)

52-52: Commit exists and is recent, but manual verification of feature alignment is needed.

The new commit hash 85eeb8d059b0249cace427dd5dec9573107be224 has been verified to exist in the NVIDIA-NeMo/RL repository. It is a recent fix (October 17, 2025) by a NVIDIA maintainer addressing "more robust fp8 rollout metric check (#1307)".

However, the commit's focus on fp8 metrics does not directly indicate it contains the specific features or fixes (e.g., chat_template_kwargs support in config files) that justify the accompanying changes in this PR. Confirm that this commit addresses the underlying dependencies required by the config updates in sft.yaml and grpo.yaml before merging.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Kipok Kipok merged commit ac0e2e7 into main Oct 19, 2025
7 checks passed
@Kipok Kipok deleted the wedu/update-nemo-rl branch October 19, 2025 15:30
SeanNaren pushed a commit to SeanNaren/NeMo-Skills that referenced this pull request Oct 22, 2025
Signed-off-by: Wei Du <wedu@nvidia.com>
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
dgtm777 pushed a commit that referenced this pull request Oct 29, 2025
Signed-off-by: Wei Du <wedu@nvidia.com>
@coderabbitai coderabbitai bot mentioned this pull request Jan 15, 2026
dgtm777 pushed a commit that referenced this pull request Mar 18, 2026
Signed-off-by: Wei Du <wedu@nvidia.com>
Signed-off-by: dgitman <dgitman@nvidia.com>
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.

2 participants