Fixes #2556: Assertion when remote backend is enabled without local CPU backend#2557
Fixes #2556: Assertion when remote backend is enabled without local CPU backend#2557hickeyma merged 8 commits intoLMCache:devfrom
Conversation
Summary of ChangesHello @hlin99, 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 critical issues related to the setup and operation of storage backends, specifically when a remote backend is enabled without a local CPU backend. It introduces a necessary assertion to enforce the dependency of remote backends on a local CPU buffer, preventing runtime errors. Additionally, it corrects the initialization of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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. Changelog
Activity
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 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 counter productive. 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. 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
|
There was a problem hiding this comment.
Code Review
This pull request addresses two issues when using a remote backend. First, it adds an assertion to ensure a local CPU backend is available when a remote backend is configured. Second, it fixes an issue with SafeLocalCPUBackend by initializing config and metadata attributes.
The changes are in the right direction, but I've identified a couple of issues. The new assertion is too strict and would break schedulers, which can validly use a remote backend without a local CPU backend. I've suggested making this assertion conditional on the worker role. Additionally, the initialization of SafeLocalCPUBackend is incomplete and will cause an AttributeError because it's missing the layerwise attribute. I've provided a suggestion to fix this.
maobaolong
left a comment
There was a problem hiding this comment.
@hlin99 LGTM. thanks for this fix.
|
Hi @sammshen could u take 5mins to take a look? This is a small safety check. Thx! |
The scheduler never reaches this code path, so the `metadata.role != "scheduler"` condition is unnecessary. Remove the redundant role check and always assert the presence of a local CPU backend when remote_url is set. Signed-off-by: Tony Lin <tony.lin@intel.com>
|
hi @maobaolong @sammshen @hickeyma just resolved the conflicts and rebased to the latest... could you help manually merge this PR if no more comment? auto-merge seems does not work...... thanks. |
…local CPU backend (LMCache#2557) Remove redundant role check for remote backend CPU assertion The scheduler never reaches this code path, so the `metadata.role != "scheduler"` condition is unnecessary. Remove the redundant role check and always assert the presence of a local CPU backend when remote_url is set. Signed-off-by: Tony Lin <tony.lin@intel.com>
…local CPU backend (LMCache#2557) Remove redundant role check for remote backend CPU assertion The scheduler never reaches this code path, so the `metadata.role != "scheduler"` condition is unnecessary. Remove the redundant role check and always assert the presence of a local CPU backend when remote_url is set. Signed-off-by: Tony Lin <tony.lin@intel.com> Signed-off-by: Ofer Kiselov Nahman <ofer.kiselovnahman@weka.io>
…local CPU backend (LMCache#2557) Remove redundant role check for remote backend CPU assertion The scheduler never reaches this code path, so the `metadata.role != "scheduler"` condition is unnecessary. Remove the redundant role check and always assert the presence of a local CPU backend when remote_url is set. Signed-off-by: Tony Lin <tony.lin@intel.com>
…local CPU backend (LMCache#2557) Remove redundant role check for remote backend CPU assertion The scheduler never reaches this code path, so the `metadata.role != "scheduler"` condition is unnecessary. Remove the redundant role check and always assert the presence of a local CPU backend when remote_url is set. Signed-off-by: Tony Lin <tony.lin@intel.com>
…local CPU backend (LMCache#2557) Remove redundant role check for remote backend CPU assertion The scheduler never reaches this code path, so the `metadata.role != "scheduler"` condition is unnecessary. Remove the redundant role check and always assert the presence of a local CPU backend when remote_url is set. Signed-off-by: Tony Lin <tony.lin@intel.com> Signed-off-by: shaoxiawjc <wjc2800@163.com>
…local CPU backend (LMCache#2557) Remove redundant role check for remote backend CPU assertion The scheduler never reaches this code path, so the `metadata.role != "scheduler"` condition is unnecessary. Remove the redundant role check and always assert the presence of a local CPU backend when remote_url is set. Signed-off-by: Tony Lin <tony.lin@intel.com> Signed-off-by: Aaron Wu <aaron.wu@dell.com>
…local CPU backend (LMCache#2557) Remove redundant role check for remote backend CPU assertion The scheduler never reaches this code path, so the `metadata.role != "scheduler"` condition is unnecessary. Remove the redundant role check and always assert the presence of a local CPU backend when remote_url is set. Signed-off-by: Tony Lin <tony.lin@intel.com>
…local CPU backend (LMCache#2557) Remove redundant role check for remote backend CPU assertion The scheduler never reaches this code path, so the `metadata.role != "scheduler"` condition is unnecessary. Remove the redundant role check and always assert the presence of a local CPU backend when remote_url is set. Signed-off-by: Tony Lin <tony.lin@intel.com>
#2556 includes two underneath issues.
SafeLocalCPUBackend config and metadata are missing, which throw exception when connector is initialized
refer to code https://github.com/LMCache/LMCache/blob/dev/lmcache/v1/storage_backend/connector/lm_connector.py#L45
Local CPU backend is must have for remote backend, otherwise remote backend won't work properly. so raise an assertion during creating backend is a decent handling way
refer to code: https://github.com/LMCache/LMCache/blob/dev/lmcache/v1/storage_backend/storage_manager.py#L327