[Fix]: add missing device attribute to ChunkCache#11493
[Fix]: add missing device attribute to ChunkCache#11493zhyncs merged 3 commits intosgl-project:mainfrom
Conversation
Summary of ChangesHello @leavelet, 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 resolves a critical issue by adding a missing Highlights
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
|
|
@cctry please review |
There was a problem hiding this comment.
Code Review
This pull request adds a missing device attribute to the ChunkCache class. The implementation is sound, but I've identified a potential issue regarding the nullability of token_to_kv_pool_allocator. The current code defensively checks if this allocator exists, but other parts of the class do not, which could lead to runtime errors. My review comment suggests simplifying the code to align with the existing type hints, which would also resolve this inconsistency.
| if self.token_to_kv_pool_allocator: | ||
| self.device = self.token_to_kv_pool_allocator.device | ||
| else: | ||
| self.device = torch.device("cpu") |
There was a problem hiding this comment.
This logic implies that token_to_kv_pool_allocator can be None. However, the type hint in the __init__ signature (BaseTokenToKVPoolAllocator) suggests it's non-nullable. This is inconsistent.
If token_to_kv_pool_allocator can be None, then cache_finished_req will raise an AttributeError at line 59, as it accesses self.token_to_kv_pool_allocator.free(...) without a check. Please ensure all usages of token_to_kv_pool_allocator are safe if it can be None, and consider updating the type hint to Optional[BaseTokenToKVPoolAllocator].
If token_to_kv_pool_allocator is never None (as the type hint suggests), this conditional logic is unnecessary and can be simplified.
self.device = self.token_to_kv_pool_allocator.device|
Sorry to bring in this bug. I am wondering when |
Should we just change it to self.device = self.token_to_kv_pool_allocator.device? It should never be None anyway. Sorry about the mistake |
|
hi @leavelet , Could you help add a CI to cover this issue? It needs --disable-radix-cache and page_size > 1. |
Motivation
Fix #11492 by adding device attribute to 'ChunkCache'.
Modifications
'ChunkCache' will inherit device from its token_to_kv_pool_allocator.
Accuracy Tests
Benchmarking and Profiling
Checklist