Conversation
Signed-off-by: Wei Du <wedu@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpgraded the vLLM Docker base image and added a new Python dependency; adjusted the vLLM server launcher to inject a Ray distributed-executor flag when running with multiple nodes unless an explicit backend flag is already provided. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Wei Du <wedu@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dockerfiles/Dockerfile.vllm (1)
2-2: Pinray[cgraph]to a specific version to avoid dependency drift.Line 2 installs Ray without a version pin, making rebuilds non-reproducible. Ray's transitive dependencies (particularly
aiohttp) can vary across releases, introducing unexpected conflicts.♻️ Proposed change
+ARG RAY_VERSION=2.55.0 -RUN pip install "ray[cgraph]" +RUN pip install --no-cache-dir "ray[cgraph]==${RAY_VERSION}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dockerfiles/Dockerfile.vllm` at line 2, The Dockerfile.vllm currently runs pip install "ray[cgraph]" without a version, which leads to non-reproducible builds; update the RUN pip install step that references "ray[cgraph]" to pin Ray to the project's tested version (e.g., add an explicit version specifier or use your constraints file) so transitive deps like aiohttp remain stable, and ensure you update any corresponding documentation/constraints (requirements.txt or constraints.txt) and CI images to match the pinned version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dockerfiles/Dockerfile.vllm`:
- Around line 1-5: The Dockerfile currently runs as root (base image FROM
vllm/vllm-openai:v0.18.1) and lacks a USER directive; add steps to create a
non-root user (e.g., create a group and user, set a home or workdir, chown any
needed directories after installing packages), switch to that user with a USER
<username> instruction before finishing the image, and ensure any runtime
writable paths (WORKDIR or volumes) are owned by that user so the container does
not default to root at runtime.
---
Nitpick comments:
In `@dockerfiles/Dockerfile.vllm`:
- Line 2: The Dockerfile.vllm currently runs pip install "ray[cgraph]" without a
version, which leads to non-reproducible builds; update the RUN pip install step
that references "ray[cgraph]" to pin Ray to the project's tested version (e.g.,
add an explicit version specifier or use your constraints file) so transitive
deps like aiohttp remain stable, and ensure you update any corresponding
documentation/constraints (requirements.txt or constraints.txt) and CI images to
match the pinned version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9abc8c5a-b5b4-43a3-bfa3-9ca13996b899
📒 Files selected for processing (1)
dockerfiles/Dockerfile.vllm
| FROM vllm/vllm-openai:v0.18.1 | ||
| RUN pip install "ray[cgraph]" | ||
| RUN pip install "vllm[audio]" | ||
| # Required by vLLM for Qwen-VL model family (runtime dependency, not directly imported) | ||
| RUN pip install qwen-vl-utils |
There was a problem hiding this comment.
Run the container as a non-root user.
No explicit USER is set, so runtime may default to root. Please set a non-root user to reduce container breakout impact.
🔒 Example hardening
FROM vllm/vllm-openai:v0.18.1
RUN pip install "ray[cgraph]"
RUN pip install "vllm[audio]"
# Required by vLLM for Qwen-VL model family (runtime dependency, not directly imported)
RUN pip install qwen-vl-utils
+RUN useradd --create-home --uid 10001 appuser
+USER appuser📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FROM vllm/vllm-openai:v0.18.1 | |
| RUN pip install "ray[cgraph]" | |
| RUN pip install "vllm[audio]" | |
| # Required by vLLM for Qwen-VL model family (runtime dependency, not directly imported) | |
| RUN pip install qwen-vl-utils | |
| FROM vllm/vllm-openai:v0.18.1 | |
| RUN pip install "ray[cgraph]" | |
| RUN pip install "vllm[audio]" | |
| # Required by vLLM for Qwen-VL model family (runtime dependency, not directly imported) | |
| RUN pip install qwen-vl-utils | |
| RUN useradd --create-home --uid 10001 appuser | |
| USER appuser |
🧰 Tools
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dockerfiles/Dockerfile.vllm` around lines 1 - 5, The Dockerfile currently
runs as root (base image FROM vllm/vllm-openai:v0.18.1) and lacks a USER
directive; add steps to create a non-root user (e.g., create a group and user,
set a home or workdir, chown any needed directories after installing packages),
switch to that user with a USER <username> instruction before finishing the
image, and ensure any runtime writable paths (WORKDIR or volumes) are owned by
that user so the container does not default to root at runtime.
Kipok
left a comment
Kipok
left a comment
There was a problem hiding this comment.
can you run gpu and slurm tests on this?
| args, unknown = parser.parse_known_args() | ||
|
|
||
| extra_arguments = join(unknown) | ||
| has_distributed_backend = any( |
There was a problem hiding this comment.
we shouldn't need this change I think - the user args will always override the default anyway. So we cna just check for multinode and if that's set we add the argument always
There was a problem hiding this comment.
sure, we can get rid of this
|
we could combine with upgrade to sglang and trtllm as well so that we do them all in one batch of tests |
Why I did not see sglang and trtllm docker files? you suggest to revise? _containers = { |
|
yes, as well as all configs |
2 participants
Summary by CodeRabbit
New Features
Chores