[Chore][Revert] MP adapter signature shim from #3100#3111
[Chore][Revert] MP adapter signature shim from #3100#3111ApostaC merged 1 commit intoLMCache:devfrom
Conversation
PR LMCache#3100 bundled two unrelated changes: the CI sitecustomize/SCM fix and a backward-compat shim on LMCacheMPScheduler/WorkerAdapter that accepted the old positional (world_size, kv_rank, vllm_block_size) call form used by the vllm-bundled lmcache_mp_connector. This reverts only the adapter shim (restores the pre-LMCache#3100 signature requiring parallel_strategy). The CI sitecustomize and SETUPTOOLS_SCM_PRETEND_VERSION changes from LMCache#3100 are kept. Callers on an un-updated vllm that still pass the old positional args will break until the vllm side is updated to pass parallel_strategy. Signed-off-by: Samuel Shen <slshen@uchciago.edu>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 18fbeab. Configure here.
| tp_size: int = 1, | ||
| parallel_strategy: Optional[ParallelStrategy] = None, | ||
| vllm_block_size: int, | ||
| parallel_strategy: ParallelStrategy, |
There was a problem hiding this comment.
In-repo connector still uses old adapter call convention
High Severity
The signature of LMCacheMPSchedulerAdapter and LMCacheMPWorkerAdapter was reverted to require (vllm_block_size, parallel_strategy), but the in-repo caller lmcache_mp_connector_0180.py (create_scheduler_adapter / create_worker_adapter) still passes (world_size, kv_rank, block_size) positionally. This causes kv_rank (an int) to land on parallel_strategy (expects ParallelStrategy), and block_size to collide with the keyword mq_timeout, producing a TypeError at runtime. The connector file needs to be updated to construct a ParallelStrategy and pass the new signature.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 18fbeab. Configure here.
There was a problem hiding this comment.
Code Review
This pull request refactors the LMCacheMPSchedulerAdapter and LMCacheMPWorkerAdapter classes by removing legacy parameters and making vllm_block_size and parallel_strategy mandatory. The review identifies that these changes break existing internal integrations in lmcache_mp_connector_0180.py and that the LMCacheMPWorkerAdapter constructor lacks a required docstring per the project's style guide.
| def __init__( | ||
| self, | ||
| server_url: str, | ||
| context: zmq.Context, | ||
| model_name: str, | ||
| world_size: int = 1, | ||
| kv_rank: int = 0, | ||
| vllm_block_size: int = 16, | ||
| tp_size: int = 1, | ||
| parallel_strategy: Optional[ParallelStrategy] = None, | ||
| vllm_block_size: int, | ||
| parallel_strategy: ParallelStrategy, | ||
| mq_timeout: float = DEFAULT_MQ_TIMEOUT, | ||
| heartbeat_interval: float = DEFAULT_HEARTBEAT_INTERVAL, | ||
| ): |
There was a problem hiding this comment.
This change breaks the internal integration code in lmcache/integration/vllm/lmcache_mp_connector_0180.py. The create_scheduler_adapter function (lines 123-133) still passes arguments using the old signature, which will now cause a TypeError because it attempts to pass an integer (kv_rank) to the parallel_strategy parameter. Since this connector is part of the repository, it should be updated in this PR to maintain a working state.
| def __init__( | ||
| self, | ||
| server_url: str, | ||
| context: zmq.Context, | ||
| model_name: str, | ||
| world_size: int = 1, | ||
| kv_rank: int = 0, | ||
| vllm_block_size: int = 16, | ||
| tp_size: int = 1, | ||
| parallel_strategy: Optional[ParallelStrategy] = None, | ||
| vllm_block_size: int, | ||
| parallel_strategy: ParallelStrategy, | ||
| mq_timeout: float = DEFAULT_MQ_TIMEOUT, | ||
| heartbeat_interval: float = DEFAULT_HEARTBEAT_INTERVAL, | ||
| ): |
There was a problem hiding this comment.
This change breaks create_worker_adapter in lmcache/integration/vllm/lmcache_mp_connector_0180.py (lines 148-157) because the positional arguments no longer match the new signature. Additionally, per the repository style guide (line 26), this public method should include a docstring describing its arguments.
def __init__(
self,
server_url: str,
context: zmq.Context,
model_name: str,
vllm_block_size: int,
parallel_strategy: ParallelStrategy,
mq_timeout: float = DEFAULT_MQ_TIMEOUT,
heartbeat_interval: float = DEFAULT_HEARTBEAT_INTERVAL,
):
"""
Args:
server_url: The server URL for the LMCache message queue
context: The ZMQ context
model_name: The model name used for LMCache keys
vllm_block_size: The block size used in vLLM
parallel_strategy:
The parallel strategy, which includes use_mla,
kv_world_size, kv_worker_id and so on
mq_timeout: Timeout in seconds for message queue requests.
heartbeat_interval: Interval in seconds between heartbeat pings.
"""References
- All new public functions have docstrings (what, args, return, exceptions) (link)
PR #3100 bundled two unrelated changes: the CI sitecustomize/SCM fix and a backward-compat shim on LMCacheMPScheduler/WorkerAdapter that accepted the old positional (world_size, kv_rank, vllm_block_size) call form used by the vllm-bundled lmcache_mp_connector.
This reverts only the adapter shim (restores the pre-#3100 signature requiring parallel_strategy). The CI sitecustomize and SETUPTOOLS_SCM_PRETEND_VERSION changes from #3100 are kept.
Callers on an un-updated vllm that still pass the old positional args will break until the vllm side is updated to pass parallel_strategy.
What this PR does / why we need it:
Special notes for your reviewers:
If applicable:
Note
Medium Risk
Breaking change to
LMCacheMPSchedulerAdapter/LMCacheMPWorkerAdapterconstructor signatures; older vLLM callers using positional(world_size, kv_rank, vllm_block_size)will fail until updated.Overview
Restores the pre-#3100 API for vLLM multiprocess adapters by removing the backward-compat constructor shim on
LMCacheMPSchedulerAdapterandLMCacheMPWorkerAdapter.Both constructors now require
vllm_block_sizeand a non-optionalparallel_strategy, and no longer accept/derive values fromworld_size,kv_rank, ortp_size(including dropping related typing/imports).Reviewed by Cursor Bugbot for commit 18fbeab. Bugbot is set up for automated code reviews on this repo. Configure here.