Skip to content

Conversation

@NickLucche
Copy link
Collaborator

@NickLucche NickLucche commented Sep 22, 2025

I mistakenly deleted a hook call in some earlier PR of mine, surely breaking some OOT logic.
Thanks @sdavidbd for the ping and sorry!

I also added some new tests to make sure the CI is better equipped to stop me next time :)

Signed-off-by: NickLucche <nlucches@redhat.com>
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 correctly fixes a bug where clear_connector_metadata was not being called, which could lead to state being leaked between steps. The fix re-introduces the call in the finally block of the _get_kv_connector_output context manager, ensuring proper cleanup. The addition of a new unit test, test_kv_connector_mixin_clears_metadata, is excellent as it specifically verifies that the metadata is cleared, preventing future regressions. The changes are well-implemented and improve the robustness of the KV connector functionality.

Copy link
Collaborator

@ApostaC ApostaC left a comment

Choose a reason for hiding this comment

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

LGTM!

@njhill njhill added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 22, 2025
@DarkLight1337 DarkLight1337 merged commit 0901970 into vllm-project:main Sep 23, 2025
55 of 56 checks passed
@NickLucche NickLucche deleted the fix-clear-metadata branch September 23, 2025 09:26
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
Signed-off-by: NickLucche <nlucches@redhat.com>
charlifu pushed a commit to ROCm/vllm that referenced this pull request Sep 25, 2025
Signed-off-by: NickLucche <nlucches@redhat.com>
Signed-off-by: charlifu <charlifu@amd.com>
yewentao256 pushed a commit that referenced this pull request Oct 3, 2025
Signed-off-by: NickLucche <nlucches@redhat.com>
Signed-off-by: yewentao256 <zhyanwentao@126.com>
gjc0824 pushed a commit to gjc0824/vllm that referenced this pull request Oct 10, 2025
Signed-off-by: NickLucche <nlucches@redhat.com>
Signed-off-by: gaojc <1055866782@qq.com>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
Signed-off-by: NickLucche <nlucches@redhat.com>
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
choprahetarth pushed a commit to Tandemn-Labs/vllm that referenced this pull request Oct 11, 2025
Signed-off-by: NickLucche <nlucches@redhat.com>
lywa1998 pushed a commit to lywa1998/vllm that referenced this pull request Oct 20, 2025
Signed-off-by: NickLucche <nlucches@redhat.com>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 24, 2025
Signed-off-by: NickLucche <nlucches@redhat.com>
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kv-connector ready ONLY add when PR is ready to merge/full CI is needed v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants