-
Notifications
You must be signed in to change notification settings - Fork 444
fix: update nixl build and keep wheels dir in vllm container #1544
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
WalkthroughThis change simplifies the Dockerfile for the vllm container by removing architecture-specific build steps and configuration flags for building and installing the NIXL component. All architectures now use the same Meson build commands, and the cleanup step for the wheelhouse directory is omitted after wheel installation. Changes
Sequence Diagram(s)sequenceDiagram
participant Docker as Docker Build
participant Meson as Meson Build
participant Python as Python Wheel Install
Docker->>Meson: Run meson setup (no arch-specific flags)
Meson-->>Docker: Build NIXL native code
Docker->>Python: Build and install NIXL Python wheel
Python-->>Docker: Wheel installed (wheelhouse remains)
Possibly related PRs
Poem
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 0
🧹 Nitpick comments (1)
container/Dockerfile.vllm (1)
523-523
: Review broad binary symlinks—limit to needed executables
Symlinking all files from the venv into/usr/local/bin
may overshadow system binaries or include unintended tools. Consider restricting links to only required CLI executables to avoid conflicts.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
container/Dockerfile.vllm
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pre-commit
🔇 Additional comments (2)
container/Dockerfile.vllm (2)
123-128
: Unify NIXL build commands—verify ARM build compatibility
The removal of architecture‐specific-Dgds_path
flags simplifies the Meson build. Please ensure this unified setup succeeds on both x86_64 and aarch64 and that GPU Direct Storage support is correctly detected without explicit paths.
155-155
: Consolidated Python wheel build—validate output
Usinguv build
to produce the NIXL wheel in/workspace/wheels/nixl
is a clean approach. Confirm that the wheel is correctly tagged for all target platforms and Python versions (e.g., manylinux compatibility on ARM).
Apply changes from #1544 to other dockfiles
Overview:
closes: OPS-272, OPS-273
Summary by CodeRabbit