Skip to content

fix(vllm): propagate cache salt in text mode - #11082

Merged
biswapanda merged 1 commit into
ai-dynamo:mainfrom
weizhoublue:fix/vllm-text-cache-salt
Jul 3, 2026
Merged

fix(vllm): propagate cache salt in text mode#11082
biswapanda merged 1 commit into
ai-dynamo:mainfrom
weizhoublue:fix/vllm-text-cache-salt

Conversation

@weizhoublue

@weizhoublue weizhoublue commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Overview:

propagate cache salt in text mode

Details:

In the --use-vllm-tokenizer scenario, an OpenAI-compatible v1/chat/completions request may carry nvext.cache_salt, but the current text-mode path loses that value before calling vLLM.

The fix extracts nvext.cache_salt from the HTTP request-derived Python request dict and writes it into the prompt object passed to the vLLM engine:

  Client HTTP body
    {"messages": [...], "nvext": {"cache_salt": "tenant-a"}}
          ↓
  Dynamo frontend / runtime converts it into a Python request dict
          ↓
  DecodeWorkerHandler._generate_text_mode()
          ↓
  Builds vLLM internal prompt object: TokensPrompt/TextPrompt
          ↓
  Copies request.nvext.cache_salt into prompt["cache_salt"]
          ↓
  engine_client.generate(prompt, sampling_params, ...)

It translates Dynamo’s OpenAI extension field, nvext.cache_salt, into vLLM prompt metadata, cache_salt, so vLLM can apply it when handling KV cache keys.

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

  • Bug Fixes
    • Improved text-generation handling so cache-salt settings are applied earlier in the request flow, helping ensure consistent generation behavior.
  • Tests
    • Added coverage to verify cache-salt values are preserved through text-mode generation.

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

copy-pr-bot Bot commented Jun 30, 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 temporarily deployed to external_collaborator June 30, 2026 07:07 — with GitHub Actions Inactive
@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi weizhoublue! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions Bot added fix external-contribution Pull request is from an external contributor backend::vllm Relates to the vllm backend labels Jun 30, 2026
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

_generate_text_mode in DecodeWorkerHandler now calls _apply_nvext_cache_salt(request, prompt) immediately after constructing the vLLM prompt, before sampling parameters are built. A new async unit test verifies that a request carrying nvext.cache_salt results in that value being present on the prompt object.

Changes

nvext cache salt in text mode

Layer / File(s) Summary
Cache salt application and test
components/src/dynamo/vllm/handlers.py, components/src/dynamo/vllm/tests/test_vllm_unit.py
_generate_text_mode calls _apply_nvext_cache_salt after prompt construction; new test_generate_text_mode_applies_nvext_cache_salt stubs the handler and asserts captured["prompt"]["cache_salt"] matches the request value.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning Overview and details are present, but the required Related Issues section still contains placeholders and the reviewer-start section is empty. Fill in the required issue linkage with a real number or confirm no related issue, and add a brief note on where reviewers should start.
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% 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 is concise and accurately summarizes the main change: propagating cache salt in text mode.
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.

🧹 Nitpick comments (1)
components/src/dynamo/vllm/tests/test_vllm_unit.py (1)

1236-1236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Hoist DecodeWorkerHandler to module scope.

Line 1236 adds a non-optional import inside the test body. Move it to the top-level imports so the dependency stays explicit and consistent with the repo’s Python test rules.

As per coding guidelines, "Keep imports at the top of the file; always flag import statements inside function bodies" and as per path instructions, "Keep all imports at the top of each file (avoid imports inside functions/classes)."

🤖 Prompt for 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.

In `@components/src/dynamo/vllm/tests/test_vllm_unit.py` at line 1236, The test
currently imports DecodeWorkerHandler inside the test body, which violates the
repo’s import rules. Move the dynamo.vllm.handlers import to the module-level
import section in test_vllm_unit.py so all dependencies are explicit at the top
of the file, and keep the test logic unchanged.

Sources: Coding guidelines, Path instructions

🤖 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.

Nitpick comments:
In `@components/src/dynamo/vllm/tests/test_vllm_unit.py`:
- Line 1236: The test currently imports DecodeWorkerHandler inside the test
body, which violates the repo’s import rules. Move the dynamo.vllm.handlers
import to the module-level import section in test_vllm_unit.py so all
dependencies are explicit at the top of the file, and keep the test logic
unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8cacf4c3-dc08-4ece-b599-40f1be17e081

📥 Commits

Reviewing files that changed from the base of the PR and between 61865e9 and 2840b7c.

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

@rmccorm4

rmccorm4 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Thanks for the contribution @weizhoublue, tagging @biswapanda to help review

@biswapanda

Copy link
Copy Markdown
Contributor

/ok to test 2840b7c

@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

@biswapanda
biswapanda merged commit b9c5bd7 into ai-dynamo:main Jul 3, 2026
94 checks passed
@github-actions
github-actions Bot deleted the fix/vllm-text-cache-salt branch August 4, 2026 11:37
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 external-contribution Pull request is from an external contributor fix size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants