fix: Make lmcache dependency optional for PdConnector#4319
Conversation
Signed-off-by: Olga Andreeva <oandreeva@nvidia.com>
There was a problem hiding this comment.
Pull Request Overview
This PR makes the lmcache dependency optional by gracefully handling cases where the library is not installed. The changes ensure the code continues to function with DynamoConnector when lmcache is unavailable, while maintaining full compatibility when it is present.
Key Changes:
- Wrapped
LMCacheConnectorV1import in a try-except block, setting it toNoneon import failure - Modified type validation to dynamically build allowed connector types based on availability
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Olga Andreeva <124622579+oandreeva-nv@users.noreply.github.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/kvbm/python/kvbm/vllm_integration/connector/pd_connector.py (1)
50-59: Fix misleading error message when LMCache is unavailable.The error message on lines 57-58 is static and always mentions "LMCacheConnector" as an option, even when
LMCacheConnectorV1failed to import. This misleads users into thinking LMCache is supported when it's not available.Apply this diff to make the error message reflect the actual available types:
# Build allowed types for first connector allowed_first_types = [DynamoConnector] if LMCacheConnectorV1 is not None: allowed_first_types.append(LMCacheConnectorV1) if not isinstance(self._connectors[0], tuple(allowed_first_types)): + type_names = " or ".join(t.__name__ for t in allowed_first_types) raise TypeError( - f"Expected first connector to be DynamoConnector or LMCacheConnector, " + f"Expected first connector to be {type_names}, " f"got {type(self._connectors[0]).__name__}" )
🧹 Nitpick comments (1)
lib/kvbm/python/kvbm/vllm_integration/connector/pd_connector.py (1)
35-41: Consider clarifying that LMCache support is optional.The docstring could be more precise by indicating that LMCache is an optional dependency. This helps users understand the requirements.
Apply this diff to clarify the optional nature of LMCache:
""" A wrapper for using KV offloading Connectors (e.g. KVBM or LMCache) and NIXL Connector for PD disaggregated serving. The current logic is: - - The first connector must be KVBM or LMCache and would be used by prefill worker to offload and onboard KV blocks. + - The first connector must be KVBM (DynamoConnector) or LMCache (if available) and would be used by prefill worker to offload and onboard KV blocks. - The second connector must be NIXL and will be used by decode worker to get KV blocks from prefill worker. """
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/kvbm/python/kvbm/vllm_integration/connector/pd_connector.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: oandreeva-nv
Repo: ai-dynamo/dynamo PR: 2989
File: lib/llm/src/block_manager/distributed/transfer.rs:6-6
Timestamp: 2025-09-18T21:47:44.143Z
Learning: For PR ai-dynamo/dynamo#2989, the ConnectorTransferBatcher architectural issues will be addressed in a follow-up PR by removing the duplicate batching logic and integrating distributed transfers with the existing TransferBatcher + LocalTransferManager pipeline, rather than adding bounded concurrency primitives like Semaphore.
🧬 Code graph analysis (1)
lib/kvbm/python/kvbm/vllm_integration/connector/pd_connector.py (1)
lib/kvbm/python/kvbm/vllm_integration/connector/dynamo_connector.py (1)
DynamoConnector(42-127)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (2)
lib/kvbm/python/kvbm/vllm_integration/connector/pd_connector.py (2)
14-20: LGTM! Good pattern for optional dependencies.The try-except block correctly handles the optional LMCache dependency by setting
LMCacheConnectorV1toNoneon import failure. The# type: ignorecomment is appropriate for suppressing type checker warnings about the None assignment.
14-20: No issues found—the optional import is properly implemented and safe.The codebase verification confirms that
LMCacheConnectorV1is only used internally withinpd_connector.py(lines 52–53) with a defensive check (if LMCacheConnectorV1 is not None:), and is not exported from the module's public API. No other code depends on it being available, and no dynamic references or type hint issues exist. The optional import handling is correct.
Signed-off-by: Olga Andreeva <oandreeva@nvidia.com>
|
/ok to test ae60cf2 |
Signed-off-by: Olga Andreeva <oandreeva@nvidia.com>
|
/ok to test 0cf8143 |
Signed-off-by: Olga Andreeva <oandreeva@nvidia.com> Signed-off-by: Olga Andreeva <124622579+oandreeva-nv@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Daiyaan <darfeen@nvidia.com>
Signed-off-by: Olga Andreeva <oandreeva@nvidia.com> Signed-off-by: Olga Andreeva <124622579+oandreeva-nv@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Olga Andreeva <oandreeva@nvidia.com>
Signed-off-by: Olga Andreeva <oandreeva@nvidia.com> Signed-off-by: Olga Andreeva <124622579+oandreeva-nv@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Overview:
resolves DIS-1041
I wrapped the LMCacheConnectorV1 dependency so the code works even when
lmcacheis not present. Here's what I changed:This approach ensures that:
lmcacheis installed, the code works exactly as beforelmcacheis not installed, the code still works but only accepts DynamoConnector as the first connectorDetails:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit