[CI]Install clang in dokerfile for triton ascend#4409
[CI]Install clang in dokerfile for triton ascend#4409wangxiyuan merged 1 commit intovllm-project:mainfrom
Conversation
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
There was a problem hiding this comment.
Code Review
This pull request adds support for Triton on Ascend NPUs by introducing a new dependency triton-ascend and a script to install its required Ascend BiSheng toolkit. The changes update the Dockerfiles to run this installation script and modify dependency files. My review focuses on the new installation script tools/triton_ascend.sh, where I've identified critical issues related to environment setup within Docker, as well as opportunities to improve robustness and image size. The main issue is that the script attempts to modify ~/.bashrc, which is ineffective for the build process and will likely cause the installation of triton-ascend to fail. I've provided a revised script and instructions on how to correctly integrate it into the Docker build process.
| #!/bin/bash | ||
|
|
||
| BISHENG_DIR="/vllm-workspace/Ascend-BiSheng" | ||
| BISHENG_PACKAGE="Ascend-BiSheng-toolkit_aarch64.run" | ||
| BISHENG_URL="https://vllm-ascend.obs.cn-north-4.myhuaweicloud.com/vllm-ascend/${BISHENG_PACKAGE}" | ||
| BISHENG_INSTALLER_PATH="${BISHENG_DIR}/${BISHENG_PACKAGE}" | ||
| BISHENG_ENV_FILE="/usr/local/Ascend/8.3.RC1/bisheng_toolkit/set_env.sh" | ||
|
|
||
| mkdir -p "${BISHENG_DIR}" | ||
| wget -P "${BISHENG_DIR}" "${BISHENG_URL}" | ||
| chmod a+x "${BISHENG_INSTALLER_PATH}" | ||
| "${BISHENG_INSTALLER_PATH}" --install | ||
| echo "source ${BISHENG_ENV_FILE}" >> ~/.bashrc No newline at end of file |
There was a problem hiding this comment.
This script has several issues regarding correctness in a Docker context, image size optimization, and robustness.
-
Ineffective Environment Setup: The command
echo "source ${BISHENG_ENV_FILE}" >> ~/.bashrcwill not set up the environment for subsequent commands within the same DockerRUNlayer. The environment variables are needed for thepip installoftriton-ascend. This line should be removed, and the environment file should be sourced directly in theDockerfile. -
No Cleanup: The downloaded installer is not removed after installation, which unnecessarily increases the Docker image size. It's a best practice to clean up temporary files in the same
RUNlayer they are created. -
Hardcoded Path: The
BISHENG_ENV_FILEpath is hardcoded. This can be brittle if the toolkit version or installation path changes. While it may work now, consider making this more dynamic if possible. -
Error Handling: The script lacks
set -e, which means it might not fail immediately if a command likewgetor the installer fails, potentially leading to a broken build.
Here is a suggested revision that addresses these points. Note that you will also need to modify your Dockerfiles to source the environment file (/usr/local/Ascend/8.3.RC1/bisheng_toolkit/set_env.sh) after running this script.
| #!/bin/bash | |
| BISHENG_DIR="/vllm-workspace/Ascend-BiSheng" | |
| BISHENG_PACKAGE="Ascend-BiSheng-toolkit_aarch64.run" | |
| BISHENG_URL="https://vllm-ascend.obs.cn-north-4.myhuaweicloud.com/vllm-ascend/${BISHENG_PACKAGE}" | |
| BISHENG_INSTALLER_PATH="${BISHENG_DIR}/${BISHENG_PACKAGE}" | |
| BISHENG_ENV_FILE="/usr/local/Ascend/8.3.RC1/bisheng_toolkit/set_env.sh" | |
| mkdir -p "${BISHENG_DIR}" | |
| wget -P "${BISHENG_DIR}" "${BISHENG_URL}" | |
| chmod a+x "${BISHENG_INSTALLER_PATH}" | |
| "${BISHENG_INSTALLER_PATH}" --install | |
| echo "source ${BISHENG_ENV_FILE}" >> ~/.bashrc | |
| #!/bin/bash | |
| set -e | |
| BISHENG_DIR="/vllm-workspace/Ascend-BiSheng" | |
| BISHENG_PACKAGE="Ascend-BiSheng-toolkit_aarch64.run" | |
| BISHENG_URL="https://vllm-ascend.obs.cn-north-4.myhuaweicloud.com/vllm-ascend/${BISHENG_PACKAGE}" | |
| BISHENG_INSTALLER_PATH="${BISHENG_DIR}/${BISHENG_PACKAGE}" | |
| mkdir -p "${BISHENG_DIR}" | |
| wget -P "${BISHENG_DIR}" "${BISHENG_URL}" | |
| chmod a+x "${BISHENG_INSTALLER_PATH}" | |
| "${BISHENG_INSTALLER_PATH}" --install | |
| # Clean up the installer to reduce Docker image size | |
| rm -rf "${BISHENG_DIR}" |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
| source /usr/local/Ascend/ascend-toolkit/set_env.sh && \ | ||
| source /usr/local/Ascend/nnal/atb/set_env.sh && \ | ||
| export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/local/Ascend/ascend-toolkit/latest/`uname -i`-linux/devlib && \ | ||
| bash /vllm-workspace/vllm-ascend/tools/triton_ascend.sh && \ |
There was a problem hiding this comment.
Using bash .sh in a Dockerfile is not a good idea. For clarity, please write the contents of the .sh file directly in the Dockerfile.
There was a problem hiding this comment.
Thanks, I have revised it.
| source /usr/local/Ascend/ascend-toolkit/set_env.sh && \ | ||
| source /usr/local/Ascend/nnal/atb/set_env.sh && \ | ||
| export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/local/Ascend/ascend-toolkit/latest/`uname -i`-linux/devlib && \ | ||
| bash /vllm-workspace/vllm-ascend/tools/triton_ascend.sh && \ |
0dec6a4 to
c7a42f8
Compare
9874b9f to
883f351
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
8969c78 to
5deeeab
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
d7c50c3 to
83f7362
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
b533147 to
495df6d
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: Meihan-chen <jcccx.cmh@gmail.com>
What this PR does / why we need it?
Install clang in dokerfile for triton ascend
Does this PR introduce any user-facing change?
How was this patch tested?