[LLM] Bump to nixl==0.4.1#55671
Conversation
Signed-off-by: Linkun <github@lkchen.net>
Signed-off-by: Linkun <github@lkchen.net>
Signed-off-by: Linkun <github@lkchen.net>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily bumps the nixl dependency to version 0.4.1 and UCX to 1.19.0. It also introduces support for darwin/aarch64 for the uv utility, which required changes to Bazel configurations and CI scripts. The dependency files have been updated accordingly. The changes are well-structured and address the goal of the pull request. I have a couple of suggestions to improve the robustness of the build process and simplify one of the shell scripts.
| actual = select({ | ||
| "//:linux_x86_64": "@uv_x86_64-linux//:file", | ||
| "//:darwin_aarch64": "@uv_aarch64-darwin//:file", | ||
| "//conditions:default": "@uv_x86_64-linux//:file", | ||
| }), |
There was a problem hiding this comment.
The use of //conditions:default to fall back to the linux_x86_64 binary might cause confusing runtime errors on unsupported platforms (e.g., linux_aarch64). When an unsupported platform is used, it will attempt to use the x86_64 binary and fail. It would be more robust to have the build fail explicitly during the analysis phase for unsupported platforms. Consider removing the default case, which will cause Bazel to error out if no condition matches, providing a clearer error to the user.
actual = select({
"//:linux_x86_64": "@uv_x86_64-linux//:file",
"//:darwin_aarch64": "@uv_aarch64-darwin//:file",
}),
There was a problem hiding this comment.
I agree we can scrap the default and error out if an supported platform is used
There was a problem hiding this comment.
we do check and throw an error for unsupported platforms in raydepsets but I don't see the point in using a default here?
| cp python/requirements_compiled.txt /tmp/ray-deps/requirements_compiled.txt | ||
| sed -i '/^--extra-index-url /d' /tmp/ray-deps/requirements_compiled.txt | ||
| sed -i '/^--find-links /d' /tmp/ray-deps/requirements_compiled.txt | ||
| sed -e '/^--extra-index-url /d' -e '/^--find-links /d' /tmp/ray-deps/requirements_compiled.txt > /tmp/ray-deps/requirements_compiled.txt.tmp | ||
| mv /tmp/ray-deps/requirements_compiled.txt.tmp /tmp/ray-deps/requirements_compiled.txt |
There was a problem hiding this comment.
These three lines can be simplified into a single sed command that reads from the source file and writes to the destination file. This avoids an intermediate copy and a temporary file, making the script cleaner and more efficient.
| cp python/requirements_compiled.txt /tmp/ray-deps/requirements_compiled.txt | |
| sed -i '/^--extra-index-url /d' /tmp/ray-deps/requirements_compiled.txt | |
| sed -i '/^--find-links /d' /tmp/ray-deps/requirements_compiled.txt | |
| sed -e '/^--extra-index-url /d' -e '/^--find-links /d' /tmp/ray-deps/requirements_compiled.txt > /tmp/ray-deps/requirements_compiled.txt.tmp | |
| mv /tmp/ray-deps/requirements_compiled.txt.tmp /tmp/ray-deps/requirements_compiled.txt | |
| sed -e '/^--extra-index-url /d' -e '/^--find-links /d' python/requirements_compiled.txt > /tmp/ray-deps/requirements_compiled.txt |
aslonnie
left a comment
aslonnie
left a comment
There was a problem hiding this comment.
@elliot-barn could you help review?
| sed -e '/^--extra-index-url /d' -e '/^--find-links /d' /tmp/ray-deps/requirements_compiled.txt > /tmp/ray-deps/requirements_compiled.txt.tmp | ||
| mv /tmp/ray-deps/requirements_compiled.txt.tmp /tmp/ray-deps/requirements_compiled.txt |
ci/raydepsets/rayllm.depsets.yaml
Outdated
| - --extra-index-url https://download.pytorch.org/whl/${CUDA_CODE} | ||
| append_flags: | ||
| - --python-version=3.11 | ||
| - --python-platform=x86_64-manylinux_2_28 |
There was a problem hiding this comment.
we use manylinux2014, not 2_28
elliot-barn
left a comment
elliot-barn
left a comment
There was a problem hiding this comment.
ran locally on my mac and looks good! I would just address Lonnie's comments, remove the uv_file alias default and ill approve
Signed-off-by: Linkun <github@lkchen.net>
|
@elliot-barn All comments have been resolved in #55664 , I've merged that branch. |
aslonnie
left a comment
aslonnie
left a comment
There was a problem hiding this comment.
this is just llm related dependency changes now. llm team can (and should) review it internally.
Signed-off-by: Linkun <github@lkchen.net>
Signed-off-by: Linkun <github@lkchen.net> Signed-off-by: ljstrnadiii <ljstrnadiii@gmail.com>
Signed-off-by: Linkun <github@lkchen.net>
Signed-off-by: Linkun <github@lkchen.net> Signed-off-by: jugalshah291 <shah.jugal291@gmail.com>
Signed-off-by: Linkun <github@lkchen.net> Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
Signed-off-by: Linkun <github@lkchen.net>
4 participants
Why are these changes needed?
Use latest nixl, which should've resolved some perf. issues according to slack message.
Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.Based on #55664