Repository navigation
fix(docker): install gRPC proto and servicer for vLLM images - #843
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical dependency issue in vLLM Docker images by ensuring that gRPC support is properly installed. It addresses the problem where vLLM instances running in Docker could not utilize gRPC for communication, making the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
vLLM doesn't include gRPC support by default. The smg-grpc-proto and smg-grpc-servicer packages (published to PyPI) provide the gRPC server that wraps vLLM's Python API for SMG gateway communication. Previously, gRPC was missing from both cases: - Base image (vllm/vllm-openai) has no gRPC - Source install (install-vllm.sh) uses --no-deps without [grpc] extra The install runs after both install-smg.sh and install-vllm.sh (if used) so it works regardless of whether vLLM came from the base image or source. SGLang and TRT-LLM have native gRPC support and don't need this. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
b5ade0b to
842c7f8
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly adds the necessary gRPC dependencies to the vLLM Docker image, fixing an issue where gRPC mode would fail. The change is isolated to the engine.Dockerfile and conditionally installs smg-grpc-proto and smg-grpc-servicer[vllm] only when the engine is vllm. My feedback includes a minor suggestion to improve the shell syntax in the RUN command for better readability and adherence to common Dockerfile practices.
| fi \ | ||
| && if [ "${ENGINE}" = "vllm" ]; then \ | ||
| pip install --no-cache-dir smg-grpc-proto "smg-grpc-servicer[vllm]"; \ | ||
| fi |
There was a problem hiding this comment.
While functionally correct, the shell syntax here can be improved for readability and to follow common Dockerfile conventions. The fi \ && if... structure is unusual, and the trailing backslash on the pip install line is unnecessary. A cleaner way to chain these commands is to place && \ at the end of the line before the continuation.
fi && \
if [ "${ENGINE}" = "vllm" ]; then \
pip install --no-cache-dir smg-grpc-proto "smg-grpc-servicer[vllm]"; \
fi
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5ade0be5a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| bash /tmp/scripts/install-${ENGINE}.sh /opt/engine-src; \ | ||
| fi \ | ||
| && if [ "${ENGINE}" = "vllm" ]; then \ | ||
| pip install --no-cache-dir smg-grpc-proto "smg-grpc-servicer[vllm]"; \ |
There was a problem hiding this comment.
This pip install pulls smg-grpc-proto and smg-grpc-servicer from PyPI instead of from the already-cloned /opt/smg-src tree, so the gRPC layer is no longer tied to the selected SMG_COMMIT. If someone builds with an unreleased SHA/branch (or any commit where gRPC code changed before publishing new wheels), the image can mix new SMG core code with older published gRPC packages and break --connection-mode grpc at runtime due to protocol/API skew.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker/engine.Dockerfile`:
- Around line 68-70: The pip install line that runs when the shell variable
ENGINE equals "vllm" installs smg-grpc-proto and smg-grpc-servicer[vllm] without
version constraints; change the Dockerfile's install invocation (the conditional
block that checks ENGINE) to pin those packages to specific versions or at least
minimum versions (e.g., smg-grpc-proto==<VERSION> and
smg-grpc-servicer[vllm]==<VERSION> or using >=) to ensure reproducible builds
and compatibility.
- Line 72: The Dockerfile’s ENTRYPOINT ["smg"] lacks runtime hardening: add a
HEALTHCHECK that runs a lightweight probe (e.g., curl or the binary’s built-in
health endpoint) to allow orchestrators to detect unhealthy containers, and
create/switch to a non-root user before ENTRYPOINT (e.g., add a user/group,
chown the smg binary and any necessary dirs, then set USER) so the container
does not run as root; ensure the smg executable permissions and owned
directories are adjusted so the non-root user can execute ENTRYPOINT ["smg"] and
perform required I/O.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 160ad2d1-fcee-4c7d-9bf6-a3edcfce513b
📒 Files selected for processing (1)
docker/engine.Dockerfile
1 participant
Summary
Installs
smg-grpc-protoandsmg-grpc-servicer[vllm]in vLLM Docker images so gRPC mode works out of the box.Problem
vLLM doesn't include gRPC support by default. The Docker image was built without the gRPC servicer, so running with
--connection-mode grpcwould fail.Both build paths were affected:
vllm/vllm-openai:v0.17.0): no gRPC includedENGINE_REPOset):install-vllm.shuses--no-depswithout[grpc]extraWhat changed
pip installstep that runs only for vLLM images, installingsmg-grpc-protoandsmg-grpc-servicer[vllm]from PyPI after engine installation.SGLang and TRT-LLM have native gRPC and are not affected.
Test plan
docker build --build-arg BASE_IMAGE_REF=vllm/vllm-openai:v0.17.0 --build-arg ENGINE=vllm ...smg-grpc-serviceris importable inside the container--connection-mode grpcworks with vLLM backendSummary by CodeRabbit