Skip to content

[CI] Fix test_shared_storage_connector_hashes#25748

Merged
DarkLight1337 merged 4 commits intovllm-project:mainfrom
chaunceyjiang:fix_test
Sep 26, 2025
Merged

[CI] Fix test_shared_storage_connector_hashes#25748
DarkLight1337 merged 4 commits intovllm-project:mainfrom
chaunceyjiang:fix_test

Conversation

@chaunceyjiang
Copy link
Collaborator

@chaunceyjiang chaunceyjiang commented Sep 26, 2025

Purpose

FIX #25745

Test Plan

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

@mergify mergify bot added the v1 label Sep 26, 2025
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 aims to fix a CI issue. It introduces a pytest fixture to ensure KV transfer groups are shut down after tests, preventing state leakage between modules. However, it also disables a test case for the 'ray' backend and hardcodes a user-specific model path, which will cause issues. My review focuses on making the test suite more robust and portable by addressing these two points.

@mergify mergify bot added the kv-connector label Sep 26, 2025
@chaunceyjiang chaunceyjiang added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 26, 2025
Copy link
Collaborator

@NickLucche NickLucche left a comment

Choose a reason for hiding this comment

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

Awesome job spotting this @chaunceyjiang !

Also tbh I don't have a strong opinion on this invariant https://github.com/vllm-project/vllm/blob/main/vllm/distributed/kv_transfer/kv_connector/v1/metrics.py#L54, it was just a quick way to prevent misuse.
We could also remove that line if this leads to unclear tests behavior or even worse if there are cases where VLLM_ENABLE_V1_MULTIPROCESSING=0 can be broken in a real scenario.. though I believe that should not be the case.

Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
@DarkLight1337 DarkLight1337 merged commit 2827b3f into vllm-project:main Sep 26, 2025
19 checks passed
@DarkLight1337
Copy link
Member

Merging to unblock CI

pdasigi pushed a commit to pdasigi/vllm that referenced this pull request Oct 2, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
yewentao256 pushed a commit that referenced this pull request Oct 3, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: yewentao256 <zhyanwentao@126.com>
choprahetarth pushed a commit to Tandemn-Labs/vllm that referenced this pull request Oct 11, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
lywa1998 pushed a commit to lywa1998/vllm that referenced this pull request Oct 20, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
alhridoy pushed a commit to alhridoy/vllm that referenced this pull request Oct 24, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
rtourgeman pushed a commit to rtourgeman/vllm that referenced this pull request Nov 10, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.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.

[CI Failure]: tests/v1/kv_connector/unit/test_shared_storage_connector.py::test_shared_storage_connector_hashes

3 participants