Skip to content

fix(vllm): Propagate nvext.cache_salt in Unified vLLM Prompt Construction - #11218

Closed
weizhoublue wants to merge 1 commit into
ai-dynamo:mainfrom
weizhoublue:fix/vllm-unified-cache-salt
Closed

fix(vllm): Propagate nvext.cache_salt in Unified vLLM Prompt Construction#11218
weizhoublue wants to merge 1 commit into
ai-dynamo:mainfrom
weizhoublue:fix/vllm-unified-cache-salt

Conversation

@weizhoublue

@weizhoublue weizhoublue commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Overview:

Propagate nvext.cache_salt in Unified vLLM Prompt Construction

Details:

A similar nvext.cache_salt issue in PR #11082 exists in a different path components/src/dynamo/vllm/llm_engine.py.

Client sends OpenAI chat/completions request with nvext: {"cache_salt": "tenant-a"} through vLLM unified backend. Dynamo preprocesses request into token IDs plus extra_args.nvext.cache_salt, then components/src/dynamo/vllm/llm_engine.py builds fresh TokensPrompt without cache_salt

In PR #11082, components/src/dynamo/vllm/handlers.py is handler-based vLLM path , where handler creates TokensPrompt or TextPrompt from OpenAI-style text-mode input.

Regarding the PR, in a different path, components/src/dynamo/vllm/llm_engine.py is unified backend path. It receives already-preprocessed GenerateRequest with token_ids and extra_args, then creates its own TokensPrompt. Fix in handlers.py does not cover llm_engine.py, because unified backend bypasses _generate_text_mode.

Both paths construct vLLM prompt objects independently, so both need to propagate nvext.cache_salt at their own prompt-construction point.

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

Open in Devin Review

Summary by CodeRabbit

  • Bug Fixes
    • Generation requests now preserve cache-salt metadata, helping ensure more consistent behavior for tenant-specific prompts.
    • Added coverage for this request path to verify the cache-salt value is applied during generation.

Signed-off-by: weizhoublue <weizhou.lan@daocloud.io>
@weizhoublue
weizhoublue requested review from a team as code owners July 4, 2026 02:30
@copy-pr-bot

copy-pr-bot Bot commented Jul 4, 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.

@weizhoublue
weizhoublue had a problem deploying to external_collaborator July 4, 2026 02:30 — with GitHub Actions Failure
@weizhoublue
weizhoublue temporarily deployed to external_collaborator July 4, 2026 02:30 — with GitHub Actions Inactive
@github-actions github-actions Bot added fix backend::vllm Relates to the vllm backend labels Jul 4, 2026
@datadog-official

Copy link
Copy Markdown

Pipelines

⚠️ Warnings

🚦 1 Pipeline job failed

PR Reminder Full CI Comment Bot | pr_reminder   View in Datadog   GitHub Actions

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 2bbcab9 | Docs | Give us feedback!

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The vLLM engine's generate() method now applies an nvext cache salt to the constructed prompt via a new _apply_nvext_cache_salt helper imported from handlers, invoked immediately after prompt construction. A corresponding unit test validates that the cache_salt value from extra_args is applied to the prompt.

Changes

NVExt Cache Salt Application

Layer / File(s) Summary
Apply cache salt in generate() and unit test
components/src/dynamo/vllm/llm_engine.py, components/src/dynamo/vllm/tests/test_vllm_unit.py
Imports and calls _apply_nvext_cache_salt(request, prompt) after building the TokensPrompt in generate(), and adds a unit test asserting prompt["cache_salt"] reflects the nvext.cache_salt value passed via extra_args.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Related Issues: None specified

Related PRs: None specified

Suggested labels: vllm, testing

Suggested reviewers: None specified

Poem
A salt so small, a cache so wise,
Slipped into prompts before they rise,
A rabbit tests with tenant-a's name,
Ensuring the salt lands right the same.
Hop, commit, and merge with pride! 🐇🧂

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Title check ✅ Passed The title clearly matches the main change: propagating nvext.cache_salt in unified vLLM prompt construction.
Description check ✅ Passed Overview and details are substantive, and the no-related-issue choice is filled; the reviewer-start section is left blank.

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

@devin-ai-integration devin-ai-integration 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.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@weizhoublue

weizhoublue commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

@biswapanda hi , please help check whether this PR makes sense
I think I missed the fix for another independent path.
Thank you

@waynehacking8

Copy link
Copy Markdown
Contributor

Took a look at this one. The gap is real: on main the unified engine is the only prompt-build path that skips the salt (llm_engine.py:368 builds TokensPrompt bare, while all three handlers.py call sites go through _apply_nvext_cache_salt from #11082). Ran your new unit test with the repo code first on sys.path: KeyError on main, passes on this branch, and vllm's input preprocessing does consume prompt["cache_salt"] (vllm/inputs/preprocess.py). I didn't run a live two-salt E2E, but the unit-level A/B plus reusing the helper that already ships for the other paths looks correct to me.

@rmccorm4

Copy link
Copy Markdown
Contributor

Hi @weizhoublue, thanks for the contribution. Can you help fix the merge conflict?

@rmccorm4 rmccorm4 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 other than merge conflict

@weizhoublue

Copy link
Copy Markdown
Contributor Author

LGTM other than merge conflict

The conflict was caused by other merged PR #8197 which also aimed to fix cache_salt issue
So I close this PR

@github-actions
github-actions Bot deleted the fix/vllm-unified-cache-salt branch August 9, 2026 10:03
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 fix size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants