[Misc] Make EP kernels install script support uv #25785
Conversation
Signed-off-by: Lucas Wilkinson <[email protected]>
![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 introduces support for configurable pip commands, like uv pip, in the install_python_libraries.sh script by using a PIP_CMD environment variable. This is a great enhancement for flexibility. The changes also modernize the pip install commands by using the --no-build-isolation flag.
My review focuses on improving the robustness of how the PIP_CMD variable is used. The current implementation with unquoted variables can fail if the command path contains spaces. I've suggested using eval to handle such cases correctly, making the script more reliable for different user environments.
|
|
||
| # install dependencies if not installed | ||
| pip3 install cmake torch ninja | ||
| $PIP_CMD install cmake torch ninja |
There was a problem hiding this comment.
Using an unquoted variable $PIP_CMD can lead to unexpected behavior due to word splitting, especially if the command path contains spaces. While it works for the intended uv pip case, it's not robust. A more reliable way to handle a command stored in a string is to use eval. This will correctly handle commands with arguments and paths with spaces (if quoted within the string).
For example, if a user sets PIP_CMD="'/path with spaces/pip' --some-arg", eval will handle it correctly, whereas the current implementation would fail.
| $PIP_CMD install cmake torch ninja | |
| eval "$PIP_CMD install cmake torch ninja" |
| # see https://github.com/pypa/pip/issues/9955#issuecomment-838065925 | ||
| # PIP_NO_BUILD_ISOLATION=0 disables build isolation | ||
| PIP_NO_BUILD_ISOLATION=0 pip install -vvv -e . | ||
| $PIP_CMD install --no-build-isolation -vvv -e . |
There was a problem hiding this comment.
Similar to the previous comment, using an unquoted $PIP_CMD here is not robust. Using eval will ensure that commands with multiple parts or paths with spaces are handled correctly.
| $PIP_CMD install --no-build-isolation -vvv -e . | |
| eval "$PIP_CMD install --no-build-isolation -vvv -e ." |
| cd DeepEP | ||
| export NVSHMEM_DIR=$WORKSPACE/nvshmem_install | ||
| PIP_NO_BUILD_ISOLATION=0 pip install -vvv -e . | ||
| $PIP_CMD install --no-build-isolation -vvv -e . |
There was a problem hiding this comment.
Similar to the previous comments, using an unquoted $PIP_CMD here is not robust. Using eval will ensure that commands with multiple parts or paths with spaces are handled correctly.
| $PIP_CMD install --no-build-isolation -vvv -e . | |
| eval "$PIP_CMD install --no-build-isolation -vvv -e ." |
make the
tools/ep_kernels/install_python_libraries.shsupport alternatives topip3/pipnamelyuv pip, e.g.TORCH_CUDA_ARCH_LIST="10.0a" PIP_CMD="uv pip" bash install_python_libraries.sh