fix(docker): correctly handle VLLM_USE_PRECOMPILED truthiness #21063
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
![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 improves the handling of the VLLM_USE_PRECOMPILED flag by ensuring that only '1' or 'true' are interpreted as true values. Additionally, the Dockerfile has been updated to correctly pass this argument during the wheel build process. The changes enhance the correctness and clarity of the build process.
docker/Dockerfile
Outdated
| @@ -235,6 +226,7 @@ RUN --mount=type=cache,target=/root/.cache/uv \ | |||
| && export SCCACHE_S3_NO_CREDENTIALS=${SCCACHE_S3_NO_CREDENTIALS} \ | |||
| && export SCCACHE_IDLE_TIMEOUT=0 \ | |||
| && export CMAKE_BUILD_TYPE=Release \ | |||
| && export VLLM_USE_PRECOMPILED=${VLLM_USE_PRECOMPILED} \ | |||
There was a problem hiding this comment.
To prevent word splitting and unexpected behavior, it's recommended to quote variable expansions in shell scripts. This ensures the value is treated as a single string, especially if VLLM_USE_PRECOMPILED contains spaces.
&& export VLLM_USE_PRECOMPILED="${VLLM_USE_PRECOMPILED}" \
docker/Dockerfile
Outdated
| @@ -248,6 +240,7 @@ RUN --mount=type=cache,target=/root/.cache/ccache \ | |||
| # Clean any existing CMake artifacts | |||
| rm -rf .deps && \ | |||
| mkdir -p .deps && \ | |||
| export VLLM_USE_PRECOMPILED=${VLLM_USE_PRECOMPILED} && \ | |||
There was a problem hiding this comment.
To prevent word splitting and unexpected behavior, it's recommended to quote variable expansions in shell scripts. This ensures the value is treated as a single string, especially if VLLM_USE_PRECOMPILED contains spaces.
export VLLM_USE_PRECOMPILED="${VLLM_USE_PRECOMPILED}" && \
There was a problem hiding this comment.
Fair enough! Accounted for in part by the trimming in envs.py, but, updated.
dougbtv
force-pushed
the
use-precompiled-truthiness
branch
14 times, most recently
from
573c8db to
2ac5db2
Compare
Main goal is in the context of CI, in order to not build wheels when unnecessary, and speed up CI builds overall. - added VLLM_DOCKER_BUILD_CONTEXT to envs to skip git + unzip logic in setup.py - normalized VLLM_USE_PRECOMPILED, treat only "1" or "true" as true - setup.py now copies contextually-named precompiled wheel into dist/ during docker builds. - smoother precompiled wheel flow, overall, in docker Signed-off-by: dougbtv <[email protected]>
dougbtv
force-pushed
the
use-precompiled-truthiness
branch
from
2ac5db2 to
96dd5d6
Compare
|
closing in support of a new PR with better descriptions. |
Action: treat only "1" or "true" as true; avoid baking ENV, use ARG default=""
This is in follow up to: #20943
And relates to ci-infra PR: vllm-project/ci-infra#125
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.Purpose
Previously, setting VLLM_USE_PRECOMPILED=0 or VLLM_USE_PRECOMPILED=false would be interpreted as "true", and only an empty because of the way
envs.pyhandled parsing VLLM_USE_PRECOMPILED as a bool. This change favors false-y values and only sets true if set to1,trueorTrue(regardless of surrounding whitespace).This is essential for CI jobs where this argument needs to be set to conditionally build the wheels.
Test Plan
Should produce the same images with the same build arguments today, provided someone wasn't abusing
Test Result
[ ] Will follow up
(Optional) Documentation Update
This matches existing documentation which uses
=1for truthiness.