Skip to content

Conversation

@faaany
Copy link
Contributor

@faaany faaany commented Sep 11, 2025

Purpose

This PR adds the missing dependency introduced in #23795 to fix the following failure in XPU CI:

image

Test Result

All tests passed.

@mergify mergify bot added the ci/build label Sep 11, 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 addresses a CI failure on XPU by adding the missing tblib dependency. While the change is effective, it installs the dependency directly within the test script. My review suggests moving this dependency into a dedicated requirements file to improve maintainability and build consistency. This is a more robust long-term solution for dependency management.

bash -c '
set -e
echo $ZE_AFFINITY_MASK
pip install tblib==3.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

high

For better dependency management and to ensure a consistent build environment, it's recommended to add this dependency to a requirements file instead of installing it directly in the CI script. This helps in centralizing dependency management and leverages Docker's layer caching, potentially speeding up the CI process.

Please consider adding tblib==3.1.0 to the appropriate requirements file (e.g., requirements/xpu.txt as used in docker/Dockerfile.xpu) and removing this line from the script.

Copy link
Contributor Author

@faaany faaany Sep 11, 2025

Choose a reason for hiding this comment

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

tblib is a test-only dependency. So adding to "xpu.txt" might not be appropriate. But adding an extra "xpu-test.txt" file is also unnecessary for just one dependency. So for now, I think install it directly in the CI script is the most straightforward way.

@faaany
Copy link
Contributor Author

faaany commented Sep 11, 2025

cc @jikunshang @yma11 @rogerxfeng8

Copy link
Collaborator

@jikunshang jikunshang left a comment

Choose a reason for hiding this comment

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

thanks for fixing!

@jikunshang jikunshang added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 11, 2025
@jikunshang jikunshang enabled auto-merge (squash) September 11, 2025 08:51
@jikunshang jikunshang merged commit 0cd72a7 into vllm-project:main Sep 11, 2025
16 checks passed
skyloevil pushed a commit to skyloevil/vllm that referenced this pull request Sep 13, 2025
dsxsteven pushed a commit to dsxsteven/vllm_splitPR that referenced this pull request Sep 15, 2025
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/build 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.

2 participants