-
Notifications
You must be signed in to change notification settings - Fork 677
chore: updating to use pinned precompiled version #2489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe AMD64 branch of container/deps/vllm/install_vllm.sh removes the VLLM_USE_PRECOMPILED flag logic, adds VLLM_PRECOMPILED_WHEEL_LOCATION (unused in shown commands), and simplifies installation to direct uv pip install calls for editable and non-editable modes. The ARM64 path is unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as install_vllm.sh (AMD64)
participant Pip as uv pip
Note over Script: Previous flow
User->>Script: Run installer
Script->>Script: Check VLLM_USE_PRECOMPILED
alt Precompiled enabled
Script->>Pip: Install using precompiled wheel
else
Script->>Pip: Install from source with torch-backend
end
Note over Script: New flow
User->>Script: Run installer
Script->>Script: Set VLLM_PRECOMPILED_WHEEL_LOCATION (unused)
Script->>Pip: Install (editable or non-editable) with torch-backend
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
container/deps/vllm/install_vllm.sh (2)
153-157: Use --torch-backend only for source builds; it’s irrelevant for wheel installsThe flag is parsed by vLLM’s build system (per prior learnings) and has no effect when installing a prebuilt wheel. The proposed change above scopes the flag correctly.
82-93: Help text defaults drifted from actual defaults — sync to avoid confusionSeveral help strings don’t match the values set earlier in the script (e.g., VLLM_REF, INSTALLATION_DIR, DEEPGEMM_REF). Recommend referencing the variables directly to keep them in sync.
Apply this diff to align help text with the actual defaults:
@@ - echo " --vllm-ref REF Git reference to checkout (default: f4135232b9a8c4845f8961fb1cd17581c56ae2ce)" + echo " --vllm-ref REF Git reference to checkout (default: ${VLLM_REF})" @@ - echo " --installation-dir DIR Directory to install vllm (default: /tmp/vllm)" + echo " --installation-dir DIR Directory to install vllm (default: ${INSTALLATION_DIR})" @@ - echo " --deepgemm-ref REF Git reference for DeepGEMM (default: 1876566)" + echo " --deepgemm-ref REF Git reference for DeepGEMM (default: ${DEEPGEMM_REF})" @@ - echo " --torch-backend BACKEND Torch backend to use (default: cu128)" + echo " --torch-backend BACKEND Torch backend to use (default: ${TORCH_BACKEND})"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
container/deps/vllm/install_vllm.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: zaristei
PR: ai-dynamo/dynamo#2020
File: container/deps/vllm/install_vllm.sh:115-118
Timestamp: 2025-07-21T00:10:56.947Z
Learning: Graceful fallback for PyTorch wheel installation is broken on ARM architecture, so immediate exit on pinned version failure is preferred over fallback mechanisms in container/deps/vllm/install_vllm.sh for ARM64.
Learnt from: ptarasiewiczNV
PR: ai-dynamo/dynamo#2027
File: container/deps/vllm/install_vllm.sh:0-0
Timestamp: 2025-07-22T10:22:28.972Z
Learning: The `--torch-backend=auto` flag works with vLLM installations via uv pip install, even though it's not a standard pip option. This flag is processed by vLLM's build system during installation to automatically match PyTorch distribution with container CUDA versions.
📚 Learning: 2025-07-21T00:10:56.947Z
Learnt from: zaristei
PR: ai-dynamo/dynamo#2020
File: container/deps/vllm/install_vllm.sh:115-118
Timestamp: 2025-07-21T00:10:56.947Z
Learning: Graceful fallback for PyTorch wheel installation is broken on ARM architecture, so immediate exit on pinned version failure is preferred over fallback mechanisms in container/deps/vllm/install_vllm.sh for ARM64.
Applied to files:
container/deps/vllm/install_vllm.sh
📚 Learning: 2025-07-22T10:22:28.972Z
Learnt from: ptarasiewiczNV
PR: ai-dynamo/dynamo#2027
File: container/deps/vllm/install_vllm.sh:0-0
Timestamp: 2025-07-22T10:22:28.972Z
Learning: The `--torch-backend=auto` flag works with vLLM installations via uv pip install, even though it's not a standard pip option. This flag is processed by vLLM's build system during installation to automatically match PyTorch distribution with container CUDA versions.
Applied to files:
container/deps/vllm/install_vllm.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (1)
container/deps/vllm/install_vllm.sh (1)
154-156: Using --torch-backend with uv pip install is valid for vLLM buildsPer previous learnings, passing this flag is correctly handled by vLLM’s build system. Keep it for source builds.
|
/ok to test 92bb00b |
|
/ok to test 8882917 |
|
/ok to test 8882917 |
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Overview:
Pins the precompile version for vllm to the same as used for source installation.
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit