Skip to content

fix: append to hf_overrides rather than overwriting#1413

Merged
terrykong merged 4 commits intomainfrom
ashors/fix-fp8-hf-override
Oct 25, 2025
Merged

fix: append to hf_overrides rather than overwriting#1413
terrykong merged 4 commits intomainfrom
ashors/fix-fp8-hf-override

Conversation

@ashors1
Copy link
Copy Markdown
Contributor

@ashors1 ashors1 commented Oct 22, 2025

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

  • ...

Summary by CodeRabbit

Bug Fixes

  • Improved configuration override handling to ensure overrides are properly initialized and merged correctly when used with generation models.

Signed-off-by: ashors1 <ashors@nvidia.com>
@ashors1 ashors1 requested a review from a team as a code owner October 22, 2025 21:36
@ashors1 ashors1 requested a review from guyueh1 October 22, 2025 21:36
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 22, 2025

📝 Walkthrough

Walkthrough

Modified hf_overrides initialization logic in _patch_vllm_sampler to explicitly ensure the dictionary key exists before merging configuration overrides, replacing direct assignment with a two-step approach: key initialization followed by update operation.

Changes

Cohort / File(s) Summary
vLLM sampler patching
nemo_rl/models/generation/vllm/vllm_worker.py
Changed hf_overrides handling from direct assignment to explicit key initialization followed by merge; ensures dictionary presence before applying config overrides

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly describes the primary change to the patch sampling logic by appending to hf_overrides instead of overwriting it, which directly reflects the main code update in vllm_worker.py without unnecessary detail or noise.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Test Results For Major Changes ✅ Passed The change in vllm_worker.py merely adjusts how the hf_overrides dictionary is initialized and merged to prevent overwriting user-specified overrides, which is a minor bug fix rather than a new feature, breaking change, or performance-sensitive update, so formal test results or performance benchmarks are not required for this PR.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ashors/fix-fp8-hf-override

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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3a69c21 and bb676ab.

📒 Files selected for processing (1)
  • nemo_rl/models/generation/vllm/vllm_worker.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

**/*.py: Follow the Google Python Style Guide for all Python code
Target Python 3.12+ for all Python code in NeMo-RL
Indent Python code with 4 spaces; do not use tabs
Python filenames should be snake_case (e.g., some_file.py)
Class names should be PascalCase
Function and method names should be snake_case
Local variable names should be snake_case; if starting with a number, prefix with k (e.g., k_99th_percentile)
Global variables should be UPPER_SNAKE_CASE and prefixed with G_ (e.g., G_MY_GLOBAL)
Constants should be UPPER_SNAKE_CASE
Avoid shadowing variables declared in an outer scope
Initialize all externally visible members of a class in the constructor
For public interfaces used outside a file, prefer docstrings over comments
Use comments mainly for code within a function or interfaces local to a file
Commented-out code must include a nearby comment explaining usage and why it is commented out; otherwise remove before merging
Use Google-style docstrings for classes and functions (Sphinx-parseable)
Avoid using reflection when functionality can be easily achieved without it
Limit except clauses to the smallest specific set of exceptions possible
For duck-typing via try/except, keep the try body minimal and use else for main logic
Add the NVIDIA copyright header (with current year) at the top of all Python files, excluding tests/ and test-only scripts

Files:

  • nemo_rl/models/generation/vllm/vllm_worker.py
nemo_rl/**/*.py

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

nemo_rl/**/*.py: Do not set non-None configuration defaults in code; YAML is the single source of truth for defaults
Access required config attributes directly (e.g., policy_cfg["precision"]) and assume presence; do not introduce hidden defaults
Express configuration optionality via TypedDict using typing.NotRequired
When adding a new config key to a TypedDict subclass, document the key’s purpose, valid values/types, and recommended default in code
For any class or function decorated with @ray.remote, add '# pragma: no cover' on the class/def line (and on remote functions)

Files:

  • nemo_rl/models/generation/vllm/vllm_worker.py

Comment thread nemo_rl/models/generation/vllm/vllm_worker.py Outdated
guyueh1
guyueh1 previously approved these changes Oct 22, 2025
Copy link
Copy Markdown
Contributor

@guyueh1 guyueh1 left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: ashors1 <ashors@nvidia.com>
terrykong
terrykong previously approved these changes Oct 22, 2025
@terrykong terrykong added CI:L1 Run doctests, unit tests, and functional tests r0.4.0 and removed CI:L1 Run doctests, unit tests, and functional tests labels Oct 22, 2025
@terrykong terrykong added the CI:L1 Run doctests, unit tests, and functional tests label Oct 22, 2025
@terrykong terrykong enabled auto-merge (squash) October 22, 2025 21:55
Signed-off-by: ashors1 <ashors@nvidia.com>
terrykong
terrykong previously approved these changes Oct 23, 2025
@terrykong
Copy link
Copy Markdown
Collaborator

@ashors1 could you resolve the linter error?

Signed-off-by: ashors1 <ashors@nvidia.com>
@terrykong terrykong added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels Oct 24, 2025
@terrykong terrykong merged commit 79269af into main Oct 25, 2025
93 of 106 checks passed
@terrykong terrykong deleted the ashors/fix-fp8-hf-override branch October 25, 2025 16:35
terrykong pushed a commit that referenced this pull request Nov 1, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
terrykong pushed a commit that referenced this pull request Nov 1, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
terrykong pushed a commit that referenced this pull request Nov 1, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
terrykong pushed a commit that referenced this pull request Nov 2, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
terrykong pushed a commit that referenced this pull request Nov 2, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
Signed-off-by: Terry Kong <terryk@nvidia.com>
terrykong pushed a commit that referenced this pull request Nov 2, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
terrykong pushed a commit that referenced this pull request Nov 2, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
terrykong added a commit that referenced this pull request Nov 2, 2025
….4.0` (#1460)

Signed-off-by: Terry Kong <terryk@nvidia.com>
Signed-off-by: ashors1 <ashors@nvidia.com>
Co-authored-by: Terry Kong <terrycurtiskong@gmail.com>
Co-authored-by: Terry Kong <terryk@nvidia.com>
Co-authored-by: Anna Shors <ashors@nvidia.com>
lbliii pushed a commit that referenced this pull request Nov 3, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
Signed-off-by: Lawrence Lane <llane@nvidia.com>
PrinsYin pushed a commit to PrinsYin/RL that referenced this pull request Nov 30, 2025
yuanhangsu1986 pushed a commit to yuanhangsu1986/RL-Nemontron-Edge-Omni that referenced this pull request Feb 21, 2026
Signed-off-by: ashors1 <ashors@nvidia.com>
Signed-off-by: yuanhangs <yuanhangs@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:L1 Run doctests, unit tests, and functional tests r0.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants