Skip to content

Conversation

NickLucche
Copy link
Collaborator

@NickLucche NickLucche commented Sep 22, 2025

This PR completes the KVConnector V0 deprecation we started here #21785.
It mostly moves stuff from v1/kv_connector to kv_connector to clarify there is no other "vX" connector (since removed in the above PR) .
Takes care of v1-related naming + warning as well (eg checking v1 is enabled).

List of changes:

  • KVConnectorBase_V1->KVConnectorBase
  • KVConnectorBaseType = KVConnectorBase_V1
  • vllm/distributed/kv_transfer/v1/kv_connector/->vllm/distributed/kv_transfer/kv_connector/
  • tests/v1/kv_connector->tests/kv_connector
  • separate KV Connector Test CI job
  • extra: updating codeowners reference + auto-label rule for kvconnectors
  • +minor renaming

cc @lk-chen

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request completes the deprecation of KVConnector V0 by refactoring the file structure and class names to remove the 'v1' differentiation. The changes primarily involve moving files from v1/kv_connector to kv_connector, renaming KVConnectorBase_V1 to KVConnectorBase, and updating all related paths and imports across the codebase.

My review has identified a critical issue where a file was copied instead of moved, leading to code duplication. I've also pointed out a now-redundant function that should be removed to finalize the cleanup. Apart from these points, the refactoring looks consistent and well-executed.

@markmc
Copy link
Member

markmc commented Sep 22, 2025

lgtm apart from the 2 helpful gemini comments

Splitting a 2 minute set of unit tests out in its own job seems a bit excessive, no?

@hmellor hmellor moved this to In Progress in V0 Deprecation Sep 22, 2025
@NickLucche
Copy link
Collaborator Author

NickLucche commented Sep 22, 2025

Thanks for reviewing!

Splitting a 2 minute set of unit tests out in its own job seems a bit excessive, no?

Yeah I was hoping to run the nixl_integrations one too and start a separate namespace with the hope of getting more tests in.. let me revert that

Signed-off-by: NickLucche <[email protected]>
@markmc markmc added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 22, 2025
@njhill
Copy link
Member

njhill commented Sep 22, 2025

@NickLucche I think we need to keep the V1 interface classes in their current locations as aliases for backwards compatibility.

See for example what was done with AsyncLLMEngine.

@njhill
Copy link
Member

njhill commented Sep 22, 2025

I'm actually not sure we should rush to move the V1 classes yet. We can clear out all of the v0 things but should keep the v1 naming/structure for a while (e.g. a release or two at least). I think removing v0 and moving v1 all at the same time has potential to introduce more confusion.

Copy link
Collaborator

@WoosukKwon WoosukKwon left a comment

Choose a reason for hiding this comment

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

+1 with @njhill

@NickLucche
Copy link
Collaborator Author

I think removing v0 and moving v1 all at the same time has potential to introduce more confusion.

Sounds good, will keep things like this. We can go back to cleaning up the codebase at a later time.

Copy link

mergify bot commented Sep 22, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @NickLucche.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/build documentation Improvements or additions to documentation kv-connector needs-rebase ready ONLY add when PR is ready to merge/full CI is needed tpu Related to Google TPUs v1
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants