fix: Add common and test dependencies to sglang runtime build #2279
Conversation
nv-tusharma
requested review from
rmccorm4,
tanmayv25,
richardhuo-nv,
ptarasiewiczNV,
ishandhanani,
alec-flowers,
nnshah1 and
a team
as code owners
WalkthroughTwo new RUN commands were added to the runtime image stage of Changes
Sequence Diagram(s)Not applicable; no new feature or control flow changes were introduced. Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
container/Dockerfile.sglang (1)
452-456: Collapse the two-step install, mark mounts read-only, and reuse theuvcacheInstalling the common requirements in a dedicated RUN layer and then repeating for the test requirements immediately afterwards costs one extra layer and forfeits cache reuse.
Minor but free wins:
- Mount both requirements files read-only (
ro=true) – they never need write-access.- Re-use the
uvwheel cache as done earlier (--mount=type=cache,target=/root/.cache/uv).- Combine both requirements into a single
uv pip installinvocation to save a layer and speed up future rebuilds.-# Common dependencies -# TODO: Remove extra install and use pyproject.toml to define all dependencies -RUN --mount=type=bind,source=./container/deps/requirements.txt,target=/tmp/requirements.txt \ - uv pip install --requirement /tmp/requirements.txt +RUN --mount=type=bind,source=./container/deps/requirements.txt,target=/tmp/requirements.txt,ro=true \ + --mount=type=bind,source=./container/deps/requirements.test.txt,target=/tmp/requirements.test.txt,ro=true \ + --mount=type=cache,target=/root/.cache/uv \ + uv pip install \ + --requirement /tmp/requirements.txt \ + --requirement /tmp/requirements.test.txtThis leaves the TODO in place while shaving image size and build time.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
container/Dockerfile.sglang(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nnshah1
PR: ai-dynamo/dynamo#1444
File: tests/fault_tolerance/scenarios.py:57-57
Timestamp: 2025-07-01T15:39:56.789Z
Learning: The fault tolerance tests in tests/fault_tolerance/ are designed to run only in the mounted container environment, so hardcoded absolute paths with `/workspace/` prefix are intentional and should not be changed to relative paths.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (1)
container/Dockerfile.sglang (1)
458-460: Re-evaluate shipping test-only dependencies in the runtime imageThe test requirements inflate the production runtime image and broaden the attack surface.
If these deps are strictly for CI, consider moving the install to theci_minimumor a dedicated test stage instead of the final runtime stage.
Otherwise document the production need so future maintainers don’t accidentally strip them.
|
/ok to test 6bed4d8 |
Overview:
Add common, test dependencies to the sglang runtime container so that it follows suite with other backend images.
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit