[BugFix][v0.24.0][KV Pool] Guard batch_get_key_info before memcache backend init - #13309
Conversation
When the MemcacheBackend store is not yet initialized (lazy_init mode), batch_get_key_info hit `assert self.store is not None` and crashed the scheduler/worker. Return an empty list instead, matching the list[KeyInfo] return contract; callers treat an empty result as zero cache hits. Also call ensure_initialized() in batch_alloc so allocation triggers lazy init, and annotate the return type as list[Any]. (cherry picked from commit 476b7fb) Signed-off-by: tyy0829 <1455207791@qq.com>
Summary of ChangesHello, 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 addresses a runtime stability issue where the system would crash if key information was requested before the Memcache backend had completed its lazy initialization. By gracefully returning an empty list and ensuring the backend is initialized during allocation, the PR prevents unexpected failures and maintains consistent behavior for callers. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
Customization To customize the 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the MemcacheBackend to handle lazy initialization safely. It guards batch_get_key_info to return an empty list if called before the store is initialized, and ensures initialization is triggered during batch_alloc. The reviewer pointed out that the PR title and description do not conform to the repository's style guide and provided compliant templates.
| return self.store.batch_is_exist(keys) | ||
|
|
||
| def batch_get_key_info(self, keys: list[str]): | ||
| def batch_get_key_info(self, keys: list[str]) -> list[Any]: |
There was a problem hiding this comment.
The Pull Request title and description do not follow the repository's style guide.
Specifically:
- The PR title format should be
[Branch][Module][Action] Pull Request Title. The current title has[Action]before[Module]and is missing the branch prefix[releases/v0.24.0rc]. - The PR description is missing the required section headers (
### What this PR does / why we need it?,### Does this PR introduce _any_ user-facing change?, and### How was this patch tested?).
Please update them to match the repository style guide.
Suggested PR Title:
[releases/v0.24.0rc][KV Transfer][BugFix] Guard batch_get_key_info before memcache backend initSuggested PR Summary:
### What this PR does / why we need it?
When the MemcacheBackend store is not yet initialized (lazy_init mode), `batch_get_key_info` hits `assert self.store is not None` and crashes the scheduler/worker. This PR guards `batch_get_key_info` by returning an empty list when the store is not initialized, matching the `list[KeyInfo]` return contract (callers treat an empty result as zero cache hits).
Additionally, it calls `ensure_initialized()` in `batch_alloc` so that allocation triggers lazy initialization, and annotates the return type of `batch_get_key_info` as `list[Any]`.
Cherry-picked from commit 476b7fb6be099265db821a9339afe30f99e8b941.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Tested with existing unit tests and verified that the scheduler/worker no longer crashes during lazy initialization.References
- The Pull Request title and summary must follow the specific formats defined in the Repository Style Guide. (link)
There was a problem hiding this comment.
Code Review
Suggested PR Title:
[Ops][BugFix] Handle lazy initialization in MemcacheBackendSuggested PR Summary:
### What this PR does / why we need it?
This pull request updates `MemcacheBackend` to safely handle cases where methods are called before the store is fully initialized. Specifically, `batch_get_key_info` now returns an empty list if called before initialization when lazy init is enabled, and `batch_alloc` ensures the store is initialized before proceeding. Type hints were also added to `batch_get_key_info`.
I have no additional feedback to provide.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
No tests were added in this PR.
tyy0829
changed the title
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. Tip 💡 Consider Linking a Related Issue or RFCYour PR title contains the [BugFix] tag, indicating a bug fix or new feature. Linking a related issue or RFC in the PR description is strongly encouraged — it gives reviewers helpful context and speeds up the review. You can use any of these keywords:
🙏 Thanks for helping us keep the project well-organized! |
3 participants
fix(kv_pool): guard batch_get_key_info before memcache backend init
When the MemcacheBackend store is not yet initialized (lazy_init mode),
batch_get_key_info hit
assert self.store is not Noneand crashed thescheduler/worker. Return an empty list instead, matching the list[KeyInfo]
return contract; callers treat an empty result as zero cache hits.
Also call ensure_initialized() in batch_alloc so allocation triggers lazy
init, and annotate the return type as list[Any].
(cherry picked from commit 476b7fb)
Signed-off-by: tyy0829 1455207791@qq.com
Cherry-pick of #13307, rebased on
releases/v0.24.0rc.