Repository navigation
Conversation
The engine images inherited smg-grpc-servicer from whatever the base image (e.g. lmsysorg/sglang:v0.5.10) happened to preinstall from PyPI at its own build time, rather than from the SMG source tree being baked into the image. When sglang v0.5.10 refactored srt/utils into a subpackage and moved get_zmq_socket into srt/utils/network, the stale bundled servicer broke at import time with "cannot import name 'get_zmq_socket' from 'sglang.srt.utils'", crash-looping the engine pod despite v1.4.1 source already containing the fix. CI never caught this because scripts/ci_install_sglang.sh installs the servicer from source (`uv pip install -e grpc_servicer/`), so CI and the published image were testing two different versions of the same package. Fix: install smg-grpc-proto and smg-grpc-servicer from the cloned source tree in install-smg.sh, using --force-reinstall to override any stale version present in the base image. Drop the now-redundant vLLM-only PyPI install from the Dockerfile. Add an import smoke test at build time so an upstream engine refactor that breaks our servicer fails the image build immediately instead of surfacing as a CrashLoopBackOff in production. Signed-off-by: key4ng <rukeyang@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe PR removes a vLLM-specific pip install step from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Code Review
This pull request updates the Docker build and installation scripts to install smg-grpc-proto and smg-grpc-servicer directly from source, ensuring the container image remains synchronized with the repository. Additionally, a smoke-test step was added to the Dockerfile to verify successful imports of engine-specific servicers. Feedback suggests using python3 instead of python in the smoke-test to ensure compatibility across different base images.
The smoke test added alongside the source install cannot run at docker
build time: importing smg_grpc_servicer.sglang.servicer transitively
loads sglang's sgl_kernel, which dlopens libcuda.so.1 and requires an
NVIDIA driver. Build runners don't have GPUs, so it always fails:
ImportError: libcuda.so.1: cannot open shared object file
ModuleNotFoundError: No module named 'common_ops'
The source-install fix is confirmed working by the same failure — the
build got past request_manager.py (where the original get_zmq_socket
ImportError would have surfaced if the stale base-image servicer were
still active) and only failed deeper in the chain when sgl_kernel tried
to load. That's the bug this PR fixes, and removing the unrunnable
smoke test leaves the actual fix intact.
A proper defense-in-depth smoke test needs to run against the built
image on a GPU runner, which is a separate workflow change.
Signed-off-by: key4ng <rukeyang@gmail.com>
Description
Problem
Engine images (
ghcr.io/lightseekorg/smg:*-sglang-v0.5.10, etc.) shipped a stalesmg_grpc_servicerinherited from whatever the base image (lmsysorg/sglang:v0.5.10) happened to preinstall from PyPI at its own build time — not the version in the SMG source tree being baked into the image.When sglang v0.5.10 refactored
srt/utils.pyinto thesrt/utils/subpackage and movedget_zmq_socketintosrt/utils/network.pywithout re-exporting it at package level, the bundled stale servicer broke at import time:This crash-looped engine pods in production despite the
v1.4.1source tree already containing the correct import (commit 5321bce, released assmg-grpc-servicer 0.5.2in #1078).Why CI didn't catch it:
scripts/ci_install_sglang.shinstalls the servicer from source (uv pip install -e grpc_servicer/), so CI ran against the fixed code. The published Docker image ran against a stale PyPI snapshot baked into the base image. CI and the image were testing two different versions of the same package.Solution
Eliminate the PyPI path entirely for engine images. Always install both
smg-grpc-protoandsmg-grpc-servicerfrom the cloned source tree, using--force-reinstallto override anything the base image preinstalled. This makes the image a single source of truth that tracks the repo.Changes
scripts/installation/install-smg.sh: after installing thesmgwheel frombindings/python, alsopip install --no-cache-dir --force-reinstallcrates/grpc_client/pythonandgrpc_servicerfrom the cloned source. No extras activated — engines are already in the base image and we don't want to reinstallvllm/sglang.docker/engine.Dockerfile: drop the now-redundantENGINE=vllm-onlypip install smg-grpc-servicer[vllm]block (covered uniformly by the source install).Test Plan
Reproduction before this PR (from a crashing pod running
ghcr.io/lightseekorg/smg:1.4.1-sglang-v0.5.10):After this PR,
install-smg.shreinstallssmg-grpc-servicerfrom${SMG_SRC}/grpc_servicer(which contains the fixedfrom sglang.srt.utils.network import get_zmq_socket) with--force-reinstall, overriding the base-image version.Confirmation that the fix is actually live in the built image comes from the
release-sglang-docker.ymlPR dry-run on this branch: the initial build attempt (commit f091a01) successfully installed from source and got pastrequest_manager.py:38— the line the originalget_zmq_socketImportError surfaced at — and only failed deeper in the chain atsgl_kernelloading (which requires libcuda.so.1 and a real GPU driver, unavailable on a build runner). If the stale base-image servicer had still been active, the failure would have matched the production traceback exactly. That it didn't is proof--force-reinstallfrom source is in effect.Note on defense-in-depth
I initially added a
RUN python -c "from smg_grpc_servicer.sglang.servicer import ..."smoke test to the Dockerfile as defense-in-depth, but reverted it (commit 644980f) once CI proved it unrunnable: the servicer's import chain transitively loads sglang'ssgl_kernel, whichdlopenslibcuda.so.1at module init time and requires an NVIDIA driver that doesn't exist on CPU-only docker build runners. The appropriate place for a full import smoke test is a post-build job inrelease-sglang-docker.ymlthat runs the built image on a GPU runner — that's a separate workflow change and out of scope for this PR.Local verification
bash -n scripts/installation/install-smg.sh— syntax OKshellcheck scripts/installation/install-smg.sh— cleanpre-commit run --files docker/engine.Dockerfile scripts/installation/install-smg.sh— all hooks passcargo +nightly fmt --all -- --check— clean (Rust workspace unperturbed; no Rust files in diff)Checklist
cargo +nightly fmtpasses (no-op — zero Rust changes)cargo clippy --all-targets --all-features -- -D warnings(no-op — zero Rust changes)Summary by CodeRabbit