[Bugfix][KVConnector] Fix MultiConnector bypassing compat shim for legacy child connectors#40702
[Bugfix][KVConnector] Fix MultiConnector bypassing compat shim for legacy child connectors#40702KrxGu wants to merge 1 commit intovllm-project:mainfrom
Conversation
KrxGu
requested review from
ApostaC,
NickLucche,
orozery and
xuechendi
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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 addresses issue #40690 by ensuring that MultiConnector uses the KVConnectorFactory to instantiate child connectors. This change allows legacy connectors with two-argument constructors to function correctly as children within a MultiConnector. Additionally, regression tests have been added to verify compatibility for both old-style and new-style connectors. I have no feedback to provide.
There was a problem hiding this comment.
Pull request overview
Fixes MultiConnector child instantiation to use the same backwards-compat constructor shim as top-level KV connector creation, preventing legacy 2-arg connectors from crashing when used as children.
Changes:
- Route
MultiConnectorchild creation throughKVConnectorFactory.create_connector(...)instead of calling the child class directly. - Add regression tests covering legacy (2-arg) and modern (3-arg) child connectors inside
MultiConnector.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
vllm/distributed/kv_transfer/kv_connector/v1/multi_connector.py |
Uses the factory for child connector creation so compat probing applies uniformly. |
tests/v1/kv_connector/unit/test_backwards_compatibility.py |
Adds regression tests ensuring MultiConnector handles both legacy and new-style children correctly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ld connectors MultiConnector was calling connector_cls(config, role, kv_cache_config) directly, so any child connector still using the old 2-arg __init__(vllm_config, role) would crash with TypeError even though the same connector works fine at the top level (where it goes through the factory compat path). Route child construction through KVConnectorFactory.create_connector instead. The import was already there; no new dependencies added. Fixes vllm-project#40690 Signed-off-by: KrxGu <krishom70@gmail.com>
KrxGu
force-pushed
the
fix/40690-multiconnector-compat-shim
branch
from
9affd97 to
2a946dc
Compare
|
This PR is now ready for review and fixes the bug in question. LGTM!! |
What's the problem?
MultiConnector.__init__instantiates child connectors by callingconnector_cls(config, role, kv_cache_config)directly. This means any child connector that still uses the pre-#27887 two-argument__init__(self, vllm_config, role)crashes with:The same connector works fine at the top level because top-level construction goes through
KVConnectorFactory.create_connector, which probes forkv_cache_configsupport and dispatches accordingly.MultiConnectorskipped that path entirely.Fix
Replace the direct
connector_cls(...)call withKVConnectorFactory.create_connector(temp_config, role, kv_cache_config)so children get the same compat detection as top-level connectors.KVConnectorFactorywas already imported in multi_connector.py - no new dependencies.Tests
Added two regression tests to test_backwards_compatibility.py:
test_multiconnector_old_style_child_instantiation- a legacy 2-arg connector works as aMultiConnectorchild (raisedTypeErrorbefore this fix)test_multiconnector_new_style_child_unaffected- a modern 3-arg child still receiveskv_cache_configcorrectlyNotes
Creating v1 connector...log line per child is expected and informational.create_connectoris a no-op here because--kv-transfer-configdisables the hybrid KV cache manager automatically.Fixes #40690