[KVConnector]: Add kv_connector_class_name to resolve name conflict with built-in connectors#38314
[KVConnector]: Add kv_connector_class_name to resolve name conflict with built-in connectors#38314maobaolong wants to merge 3 commits intovllm-project:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances configuration handling and model support across several components. It introduces kv_connector_class_name for explicit connector class selection and updates the AutoConfig registration logic to handle custom configurations more securely. Several model-specific config classes were refactored to call super().__init__ after attribute assignment to ensure proper initialization. Additionally, DeepSeek-VL2 received compatibility updates for Transformers v5, and a fix was added for Step3.5 to handle inconsistent layer type lengths. I have no feedback to provide.
|
Hi @maobaolong, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
1 similar comment
|
Hi @maobaolong, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
There was a problem hiding this comment.
Why are these changes being made?
There was a problem hiding this comment.
Sorry, these changes are introduced by accident, revert it.
…ith built-in connectors Signed-off-by: baoloongmao <baoloongmao@tencent.com>
Signed-off-by: baoloongmao <baoloongmao@tencent.com>
Signed-off-by: baoloongmao <baoloongmao@tencent.com>
d0e42a7 to
406e31e
Compare
|
@chaunceyjiang Thanks for the remind of the unrelated code changes, revert those code which introduced by accident while rebase code to upstream. PTAL. |
|
/cc @NickLucche @ApostaC PTAL. |
ApostaC
left a comment
There was a problem hiding this comment.
Looks good to me in general.
@NickLucche Some more context: this PR helps people to do offline debug and pre-prod testing before the changes have been merged into vLLM upstream. Can you take a quick look as well and leave your thoughts?
|
I think the current "check local registry, fallback to remote import" is confusing. Then, we don't need the extra |
@orozery Thanks for your review and I also think that PR is better #38301 |
|
@ApostaC @orozery @chaunceyjiang #38301 has been merged, thanks for your kindly review and suggestion. BTW, how to deal this PR? close or merge too? |
|
I think we can close it for now. If we have similar needs in the future, we can open it |
Purpose
When using an external KV connector via kv_connector_module_path, the kv_connector field is also used as the class name to look up in the loaded module. This causes a conflict if the external class shares the same name as a built-in registered connector (e.g., LMCacheConnectorV1) — the factory matches the registry first and never reaches the dynamic loading path.
This PR adds an optional kv_connector_class_name field to KVTransferConfig. When set, it is used as the actual class name to load from the external module, while kv_connector acts as a logical alias.
{ "kv_connector": "MyLMCacheConnector", "kv_role": "kv_both", "kv_connector_module_path": "lmcache.integration.vllm.lmcache_connector_v1", "kv_connector_class_name": "LMCacheConnectorV1" }Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.