-
-
Notifications
You must be signed in to change notification settings - Fork 11.5k
[XPU] add missing dependency tblib for XPU CI #24639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Fanli Lin <[email protected]>
There was a problem hiding this 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
jikunshang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for fixing!
Signed-off-by: Fanli Lin <[email protected]>
Signed-off-by: Fanli Lin <[email protected]>
Signed-off-by: Fanli Lin <[email protected]>
Signed-off-by: Fanli Lin <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
Signed-off-by: Fanli Lin <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
Purpose
This PR adds the missing dependency introduced in #23795 to fix the following failure in XPU CI:
Test Result
All tests passed.