build: Add vllm arm precompiled wheel env variable#1970
build: Add vllm arm precompiled wheel env variable#1970terrykong merged 2 commits intoNVIDIA-NeMo:mainfrom
Conversation
Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
📝 WalkthroughWalkthroughThe change modifies the VLLM wheel configuration script to conditionally select the appropriate precompiled wheel URL based on system architecture. If the system is aarch64, it uses the aarch64 wheel URL; otherwise, it defaults to the x86_64 wheel URL. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/build-custom-vllm.sh (1)
56-57:⚠️ Potential issue | 🟡 MinorUnset of
UV_PROJECT_ENVIRONMENTwill fail underset -euif it's not already set.Line 56 reads
$UV_PROJECT_ENVIRONMENTwithout a default. If it's unset,set -u(fromset -eou pipefailon line 16) will cause an immediate error. Similarly, line 93 re-exports it without guarding.This is pre-existing, not introduced by this PR, but worth noting since it's on the execution path.
🛡️ Proposed fix
-OLD_UV_PROJECT_ENVIRONMENT=$UV_PROJECT_ENVIRONMENT +OLD_UV_PROJECT_ENVIRONMENT="${UV_PROJECT_ENVIRONMENT:-}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/build-custom-vllm.sh` around lines 56 - 57, The script reads and unsets UV_PROJECT_ENVIRONMENT unsafely under set -u; change the assignment to use a safe parameter expansion (e.g., assign OLD_UV_PROJECT_ENVIRONMENT using ${UV_PROJECT_ENVIRONMENT-} so it yields empty if unset) and when restoring, guard the re-export/unset by checking the saved value (use [ -n "${OLD_UV_PROJECT_ENVIRONMENT-}" ] to export UV_PROJECT_ENVIRONMENT back, otherwise ensure it remains unset) so neither OLD_UV_PROJECT_ENVIRONMENT nor re-exporting UV_PROJECT_ENVIRONMENT will trigger set -u failures.
🧹 Nitpick comments (1)
tools/build-custom-vllm.sh (1)
27-31: Consider allowing an environment variable override forVLLM_PRECOMPILED_WHEEL_LOCATION.The PR objective mentions "introducing a configurable environment variable," but this code unconditionally overwrites any externally-set value. Wrapping in a guard would let users point to a custom wheel (e.g., a local mirror or a different version) without editing the script.
♻️ Proposed refactor
-if [[ "$(uname -m)" == "aarch64" ]]; then - VLLM_PRECOMPILED_WHEEL_LOCATION="https://github.com/vllm-project/vllm/releases/download/v0.13.0/vllm-0.13.0-cp38-abi3-manylinux_2_31_aarch64.whl" -else - VLLM_PRECOMPILED_WHEEL_LOCATION="https://github.com/vllm-project/vllm/releases/download/v0.13.0/vllm-0.13.0-cp38-abi3-manylinux_2_31_x86_64.whl" -fi +if [[ -z "${VLLM_PRECOMPILED_WHEEL_LOCATION:-}" ]]; then + if [[ "$(uname -m)" == "aarch64" ]]; then + VLLM_PRECOMPILED_WHEEL_LOCATION="https://github.com/vllm-project/vllm/releases/download/v0.13.0/vllm-0.13.0-cp38-abi3-manylinux_2_31_aarch64.whl" + else + VLLM_PRECOMPILED_WHEEL_LOCATION="https://github.com/vllm-project/vllm/releases/download/v0.13.0/vllm-0.13.0-cp38-abi3-manylinux_2_31_x86_64.whl" + fi +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/build-custom-vllm.sh` around lines 27 - 31, The script currently always overwrites VLLM_PRECOMPILED_WHEEL_LOCATION in the arch-detection if/else block; change it to respect an externally provided value by only assigning the default wheel URL when VLLM_PRECOMPILED_WHEEL_LOCATION is empty/undefined (i.e., keep the existing if [[ "$(uname -m)" == "aarch64" ]] { ... } else { ... } logic but wrap or replace assignments so they only set a default when no env value exists), referencing VLLM_PRECOMPILED_WHEEL_LOCATION and the arch-detection if block so users can override with a custom URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tools/build-custom-vllm.sh`:
- Around line 56-57: The script reads and unsets UV_PROJECT_ENVIRONMENT unsafely
under set -u; change the assignment to use a safe parameter expansion (e.g.,
assign OLD_UV_PROJECT_ENVIRONMENT using ${UV_PROJECT_ENVIRONMENT-} so it yields
empty if unset) and when restoring, guard the re-export/unset by checking the
saved value (use [ -n "${OLD_UV_PROJECT_ENVIRONMENT-}" ] to export
UV_PROJECT_ENVIRONMENT back, otherwise ensure it remains unset) so neither
OLD_UV_PROJECT_ENVIRONMENT nor re-exporting UV_PROJECT_ENVIRONMENT will trigger
set -u failures.
---
Nitpick comments:
In `@tools/build-custom-vllm.sh`:
- Around line 27-31: The script currently always overwrites
VLLM_PRECOMPILED_WHEEL_LOCATION in the arch-detection if/else block; change it
to respect an externally provided value by only assigning the default wheel URL
when VLLM_PRECOMPILED_WHEEL_LOCATION is empty/undefined (i.e., keep the existing
if [[ "$(uname -m)" == "aarch64" ]] { ... } else { ... } logic but wrap or
replace assignments so they only set a default when no env value exists),
referencing VLLM_PRECOMPILED_WHEEL_LOCATION and the arch-detection if block so
users can override with a custom URL.
Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
What does this PR do ?
Adding vllm arm precompiled wheel environment variable to build on ARM machines
Issues
Fixes #1952
Usage
# Add a code snippet demonstrating how to use thisBefore your PR is "Ready for review"
Pre checks:
Additional Information
Summary by CodeRabbit