Skip to content

Conversation

@sdavidbd
Copy link
Contributor

@sdavidbd sdavidbd commented May 14, 2025

This PR introduces a new mechanism to simplify the integration of external KV connectors in vLLM's V1 prefill-decode (P/D) disaggregation pipeline.

📌 Motivation

Currently, external packages implementing KVConnectorBase_V1 must define a connector class and upstream it into the vLLM codebase, even if the class simply delegates to an external implementation. This causes unnecessary boilerplate and maintenance overhead for both vendors and vLLM maintainers.

✅ What this PR adds

  • A new config field:
kv_connector_module_path: Optional[str]

This allows users to specify the Python module path from which the connector class should be dynamically loaded.

  • Updated KVConnectorFactory logic:
    If a kv_connector is not found in the internal registry, the factory attempts to load the class from the given kv_connector_module_path via importlib.

🔧 Usage Example

This enables setups like the following, without requiring any code to be added to the vLLM repo:

KVTransferConfig(
    kv_connector="MyVendorConnector",
    kv_role="kv_both",
    kv_connector_module_path="myvendor.vllm.my_kv_connector"
)

✅ Benefits

  • Avoids duplicating boilerplate classes that only wrap vendor code.
  • No need to upstream vendor-specific connectors into vLLM.

@github-actions
Copy link

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

@maobaolong
Copy link
Contributor

@sdavidbd This looks much more clear

Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sdavidbd, this looks much better to me too.

Would be good to get thoughts from @KuntaiDu and/or @ApostaC too if possible.

vllm/config.py Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there currently appears to be a distinction between the kv connector name and the class name, perhaps this should be the fully-qualified class name rather than just the module path?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe another explicit kv_connector_class_name to keep thing safe?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment! I was actually hoping we could avoid maintaining a distinction between kv_connector and the actual class name.

In fact, I think we should consider renaming the kv_connector field to kv_connector_class_name (to reflect its intended use more clearly) in V1, where I believe we should move away from the current registration-based approach and instead rely entirely on dynamic loading via configuration.

Right now, the primary use of the kv_connector field is to look up a registered connector class, but this mechanism becomes redundant if we support fully dynamic loading. Also, currently, the only case where the kv_connector value is different from the class name is in the V0 SimpleConnector - where kv_connector is used more as an internal behavior flag. Even in that case, I’d argue that kv_connector_extra_config would have been a better fit.

Please let me know what you think, I'd be happy to help drive this cleanup if there's agreement on the direction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdavidbd I agree with all of that! But I wasn't involved when the original implementation was done so I'm not sure if there's some other rationale for it being done this way. Thought it would be good for the original authors confirm that it's fine to remove this distinction.

@robertgshaw2-redhat robertgshaw2-redhat enabled auto-merge (squash) May 14, 2025 22:12
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label May 14, 2025
auto-merge was automatically disabled May 15, 2025 20:16

Head branch was pushed to by a user without write access

@sdavidbd sdavidbd force-pushed the feature/simplify-external-kv-connector-integration branch 2 times, most recently from b7f56b7 to 3b85dd4 Compare May 16, 2025 07:03
David Ben-David added 2 commits May 16, 2025 15:37
Added a new 'kv_connector_module_path' field to KVTransferConfig.
If a connector name is not found in the internal registry, the factory
will now attempt to load it dynamically using the provided module path

Signed-off-by: David Ben-David <davidb@pliops.com>
Signed-off-by: David Ben-David <davidb@pliops.com>
@sdavidbd sdavidbd force-pushed the feature/simplify-external-kv-connector-integration branch from 3b85dd4 to 0d2f835 Compare May 16, 2025 12:39
@houseroad houseroad enabled auto-merge (squash) May 17, 2025 05:31
@houseroad houseroad merged commit 3e0d435 into vllm-project:main May 17, 2025
69 checks passed
zzzyq pushed a commit to zzzyq/vllm that referenced this pull request May 24, 2025
…tions (vllm-project#18142)

Signed-off-by: David Ben-David <davidb@pliops.com>
Co-authored-by: David Ben-David <davidb@pliops.com>
Signed-off-by: Yuqi Zhang <yuqizhang@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants