Skip to content

feat(vllm): support NeMo-RL NCCL weight updates - #11034

Merged
jthomson04 merged 4 commits into
mainfrom
jthomson04/nemo-rl-nomx
Jun 29, 2026
Merged

feat(vllm): support NeMo-RL NCCL weight updates#11034
jthomson04 merged 4 commits into
mainfrom
jthomson04/nemo-rl-nomx

Conversation

@jthomson04

@jthomson04 jthomson04 commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Overview:

Details:

Where should the reviewer start?

Related Issues

⚠️ This section is required. Choose one path below and delete the other.

🔗 This PR is linked to an issue:

  • Closes #XXXX

🚫 This PR is NOT linked to an issue:

  • Confirmed — no related issue

Summary by CodeRabbit

  • New Features

    • Weight updates can now be applied without pausing the worker when explicitly allowed.
    • Cache reset behavior is now configurable, letting updates optionally preserve existing prefix/KV cache state.
  • Bug Fixes

    • Added stricter validation for new update options to prevent invalid requests.
    • Improved update handling so only the intended control fields are passed through during distributed updates.
  • Tests

    • Added coverage for unpaused weight updates and default-safe error handling.

@jthomson04
jthomson04 requested review from a team as code owners June 28, 2026 21:53
@copy-pr-bot

copy-pr-bot Bot commented Jun 28, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@jthomson04
jthomson04 temporarily deployed to external_collaborator June 28, 2026 21:53 — with GitHub Actions Inactive
@github-actions github-actions Bot added the feat label Jun 28, 2026
@jthomson04
jthomson04 requested a review from biswapanda June 28, 2026 21:53
@github-actions github-actions Bot added the backend::vllm Relates to the vllm backend label Jun 28, 2026
@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

update_weights_from_distributed gains two boolean flags: allow_unpaused (default False) and reset_prefix_cache (default True). The pause guard is relaxed when allow_unpaused is True, both flags are stripped from RPC kwargs, and cache invalidation is now conditional. Two new unit tests cover async RL semantics and safe-default behavior.

Changes

Weight Update Flag Controls

Layer / File(s) Summary
Handler: new flags, pause guard, and conditional cache reset
components/src/dynamo/vllm/handlers.py
Parses and boolean-validates allow_unpaused and reset_prefix_cache; allows weight updates without pause when allow_unpaused is True; excludes both fields from rpc_kwargs; calls reset_prefix_cache() only when reset_prefix_cache is True.
Unit tests for new flag semantics
components/src/dynamo/vllm/tests/test_vllm_worker_handler.py
Adds test_distributed_update_can_match_async_rl_semantics (verifies allow_unpaused path skips cache reset) and test_distributed_update_preserves_safe_defaults (verifies empty payload error and paused-path cache reset).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is only the template skeleton and omits the required content in Overview, Details, reviewer start, and issue linkage. Fill in the required sections with a real summary, implementation details, reviewer guidance, and a valid linked issue or confirmed no-issue checkbox.
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: NeMo-RL NCCL weight updates in vllm.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@components/src/dynamo/vllm/handlers.py`:
- Line 1644: The pause check in BaseWorkerHandler is using defensive getattr for
a known attribute, which can hide initialization regressions. Update the
condition in the handler logic to reference self._paused directly, and only
introduce a class-level default if __new__-based test doubles truly require it.
Keep the change scoped to the BaseWorkerHandler pause/allow_unpaused path and
follow the known-attribute guidance from the Python guidelines.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 99dfd0d4-12de-4c0a-8032-3936570e821f

📥 Commits

Reviewing files that changed from the base of the PR and between d672016 and f6d44eb.

📒 Files selected for processing (2)
  • components/src/dynamo/vllm/handlers.py
  • components/src/dynamo/vllm/tests/test_vllm_worker_handler.py

Comment thread components/src/dynamo/vllm/handlers.py Outdated
@datadog-official

This comment has been minimized.

@jthomson04
jthomson04 temporarily deployed to external_collaborator June 28, 2026 23:31 — with GitHub Actions Inactive
Comment thread components/src/dynamo/vllm/handlers.py
@jthomson04

Copy link
Copy Markdown
Contributor Author

/ok to test 071684f

@jthomson04
jthomson04 temporarily deployed to external_collaborator June 29, 2026 02:21 — with GitHub Actions Inactive
@jthomson04

Copy link
Copy Markdown
Contributor Author

/ok to test 8db1596

Comment thread components/src/dynamo/vllm/handlers.py Outdated

@biswapanda biswapanda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

jthomson04 and others added 4 commits June 29, 2026 15:22
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jothomson@nvidia.com>
@jthomson04
jthomson04 force-pushed the jthomson04/nemo-rl-nomx branch from 03c0dda to 5ebd0d2 Compare June 29, 2026 22:22
@jthomson04

Copy link
Copy Markdown
Contributor Author

/ok to test 5ebd0d2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend::vllm Relates to the vllm backend feat size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants