-
Notifications
You must be signed in to change notification settings - Fork 720
fix: resolve local dev container build issues #1269
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
|
👋 Hi t-ob! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
WalkthroughThe updates adjust build configurations by modifying feature flags in a shell script and reordering the installation of a Python package in a Dockerfile. No changes were made to public APIs or exported entities. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant DevContainer Script
participant Docker Build
Developer->>DevContainer Script: Trigger build
DevContainer Script->>Docker Build: Build with only 'mistralrs' feature
Docker Build->>Docker Build: Install 'maturin' as root
Docker Build->>Docker Build: Switch to non-root user
Docker Build->>Docker Build: Continue with remaining setup
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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)
301-302: Reorder maturin installation under root to fix permissions
Movinguv pip install maturinbeforeUSER $USERNAMEensures that the command runs with root privileges and avoids the previous permissions error.Consider pinning
maturinto a specific version for reproducible builds, for example:-RUN uv pip install maturin +RUN uv pip install maturin==<stable-version>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.devcontainer/post-create.sh(1 hunks)container/Dockerfile.vllm(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (1)
.devcontainer/post-create.sh (1)
52-52: Remove obsoletepythonfeature from Cargo build
Thepythonfeature no longer exists in the Rust packages, so dropping it from--features mistralrsaligns the script with the current codebase.Please verify that no other scripts, CI configurations, or documentation still reference the removed
pythonfeature and update them accordingly.
alec-flowers
left a comment
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.
Thanks for this!
|
Thanks @alec-flowers -- I don't believe I have permissions to merge, would you be able to on my behalf? |
Overview:
Fix for local dev container build. I ran into this today trying to follow the instructions in .devcontainer/README.md.
Details:
First, running
failed with a permissions error in Dockerfile.vllm. Moving the
pip installinvocation to before changing user seems to have resolved it.Second, it doesn't seem like there is a
pythonfeature any more? At least,cargo buildwas failing with:Removing
pythonfrom the feature list does seem to resolve, though I'm unsure if there are any knock-on effects.Where should the reviewer start?
container/Dockerfile.vllmSummary by CodeRabbit