[BugFix] Honor enable_caching in connector-delayed kvcache load case
#19435
Conversation
Co-authored-by: hidva <w@hidva.com> Signed-off-by: Nick Hill <nhill@redhat.com>
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
There was a problem hiding this comment.
Summary of Changes
Hello @njhill, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request fixes a bug identified in a specific KV cache loading flow where the system would attempt to cache blocks regardless of the enable_caching configuration. The change ensures that caching logic is now correctly gated by the enable_caching flag, preventing unintended caching behavior when it is disabled. A clarifying comment has also been added in the scheduler code.
Highlights
- Bug Fix: Addresses a bug where KV cache blocks were being cached even when
enable_cachingwas set toFalse, specifically in the connector-delayed KV cache load scenario. - Conditional Caching: Introduces a check for the
enable_cachingflag within thecache_blocksmethod of theKVCacheManagerto ensure caching only occurs when explicitly enabled. - Code Clarity: Adds a comment in the scheduler to clarify that the call to
cache_blocksrespects theenable_cachingsetting.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configureGemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the bug where enable_caching was not honored in the connector-delayed KV cache load case. The changes ensure that block caching is skipped if enable_caching is false.
Specifically:
- In
vllm/v1/core/kv_cache_manager.py, thecache_blocksmethod now appropriately checks theself.enable_cachingflag before attempting to cache blocks. The docstring has also been updated to reflect this conditional behavior. - In
vllm/v1/core/sched/scheduler.py, a comment has been added to clarify that the call tokv_cache_manager.cache_blockswill respect the caching flag.
The fix is straightforward and improves the correctness of the caching mechanism. The changes are clear and well-targeted.
|
@heheda12345 I'm actually not sure :) but it seemed pretty clear that it shouldn't be done here given it wouldn't have been called if the kvcache load wasn't delayed. |
|
Can you have a try on what will happen if we enable such kv cache load but disable prefix cache? |
|
We (@simpx ) encountered the following error stack while testing our async load KVConnector: |
|
@simpx Does this pr fix your problem? |
Yes. I haven't encountered any new issues with this PR in my stress testing |
vllm-project#19435) Signed-off-by: Nick Hill <nhill@redhat.com> Signed-off-by: minpeter <kali2005611@gmail.com>
vllm-project#19435) Signed-off-by: Nick Hill <nhill@redhat.com>
vllm-project#19435) Signed-off-by: Nick Hill <nhill@redhat.com>
vllm-project#19435) Signed-off-by: Nick Hill <nhill@redhat.com> Signed-off-by: avigny <47987522+avigny@users.noreply.github.com>
vllm-project#19435) Signed-off-by: Nick Hill <nhill@redhat.com>
Thanks to @hidva for catching this.