feat: update PYTHONPATH in runtime image #2982
Conversation
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
hhzhang16
requested review from
a team,
alec-flowers,
ishandhanani,
nnshah1,
ptarasiewiczNV,
richardhuo-nv,
rmccorm4 and
tanmayv25
as code owners
WalkthroughThe Dockerfiles adjust PYTHONPATH entries. For sglang, /workspace/benchmarks/profiler is added to PYTHONPATH in both base and development stages. For vLLM, PYTHONPATH is centralized in the framework stage with multiple component paths (including benchmarks/profiler) and is inherited by later stages, removing per-stage duplication. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Build as Docker Build
participant Framework as Framework Stage
participant Runtime as Runtime Stage
participant Dev as Development Stage
Build->>Framework: Build framework image
Note over Framework: Set ENV PYTHONPATH with components/* and /workspace/benchmarks/profiler
Framework-->>Runtime: Inherit ENV PYTHONPATH
Runtime-->>Dev: Inherit ENV PYTHONPATH (no redefinition)
Note over Dev: Uses inherited PYTHONPATH during development
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
container/Dockerfile.sglang(2 hunks)container/Dockerfile.vllm(2 hunks)
⏰ 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 - dynamo
🔇 Additional comments (3)
container/Dockerfile.sglang (1)
250-250: PYTHONPATH update in runtime looks good.Profiler is now discoverable at runtime; ordering preserves project paths ahead of inherited ones.
container/Dockerfile.vllm (2)
523-523: Dev inherits PYTHONPATH from runtime — nice removal of duplication.Keeps stages consistent and reduces drift.
289-292: Manual verification required: confirm PYTHONPATH includes benchmarks/profiler in runtime imagesThe smoke‐test script couldn’t run (no
dockeravailable), so please manually verify in container/Dockerfile.vllm (lines 289–292) and in container/Dockerfile.sglang that theENV PYTHONPATHsetting indeed prepends/workspace/benchmarks/profiler(while preserving$PYTHONPATH) before the--target=runtimestage.
| # Make sure to sync this with the one specified on README.md. | ||
| # This is a generic PYTHONPATH which works for all the frameworks, so some paths may not be relevant for this particular framework. | ||
| ENV PYTHONPATH=${WORKSPACE_DIR}/components/metrics/src:${WORKSPACE_DIR}/components/frontend/src:${WORKSPACE_DIR}/components/planner/src:${WORKSPACE_DIR}/components/backends/mocker/src:${WORKSPACE_DIR}/components/backends/trtllm/src:${WORKSPACE_DIR}/components/backends/vllm/src:${WORKSPACE_DIR}/components/backends/sglang/src:${WORKSPACE_DIR}/components/backends/llama_cpp/src | ||
| ENV PYTHONPATH=${WORKSPACE_DIR}/components/metrics/src:${WORKSPACE_DIR}/components/frontend/src:${WORKSPACE_DIR}/components/planner/src:${WORKSPACE_DIR}/components/backends/mocker/src:${WORKSPACE_DIR}/components/backends/trtllm/src:${WORKSPACE_DIR}/components/backends/vllm/src:${WORKSPACE_DIR}/components/backends/sglang/src:${WORKSPACE_DIR}/components/backends/llama_cpp/src:${WORKSPACE_DIR}/benchmarks/profiler |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t clobber inherited PYTHONPATH in dev stage; append $PYTHONPATH.
This overwrites the runtime PYTHONPATH (drops /workspace/examples/sglang/utils and any upstream additions). Append $PYTHONPATH for parity with vLLM and to avoid surprises.
Apply this diff:
-ENV PYTHONPATH=${WORKSPACE_DIR}/components/metrics/src:${WORKSPACE_DIR}/components/frontend/src:${WORKSPACE_DIR}/components/planner/src:${WORKSPACE_DIR}/components/backends/mocker/src:${WORKSPACE_DIR}/components/backends/trtllm/src:${WORKSPACE_DIR}/components/backends/vllm/src:${WORKSPACE_DIR}/components/backends/sglang/src:${WORKSPACE_DIR}/components/backends/llama_cpp/src:${WORKSPACE_DIR}/benchmarks/profiler
+ENV PYTHONPATH=${WORKSPACE_DIR}/components/metrics/src:${WORKSPACE_DIR}/components/frontend/src:${WORKSPACE_DIR}/components/planner/src:${WORKSPACE_DIR}/components/backends/mocker/src:${WORKSPACE_DIR}/components/backends/trtllm/src:${WORKSPACE_DIR}/components/backends/vllm/src:${WORKSPACE_DIR}/components/backends/sglang/src:${WORKSPACE_DIR}/components/backends/llama_cpp/src:${WORKSPACE_DIR}/benchmarks/profiler:$PYTHONPATHOptionally also include ${WORKSPACE_DIR}/examples/sglang/utils if those imports are expected during dev.
📝 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.
| ENV PYTHONPATH=${WORKSPACE_DIR}/components/metrics/src:${WORKSPACE_DIR}/components/frontend/src:${WORKSPACE_DIR}/components/planner/src:${WORKSPACE_DIR}/components/backends/mocker/src:${WORKSPACE_DIR}/components/backends/trtllm/src:${WORKSPACE_DIR}/components/backends/vllm/src:${WORKSPACE_DIR}/components/backends/sglang/src:${WORKSPACE_DIR}/components/backends/llama_cpp/src:${WORKSPACE_DIR}/benchmarks/profiler | |
| ENV PYTHONPATH=${WORKSPACE_DIR}/components/metrics/src:${WORKSPACE_DIR}/components/frontend/src:${WORKSPACE_DIR}/components/planner/src:${WORKSPACE_DIR}/components/backends/mocker/src:${WORKSPACE_DIR}/components/backends/trtllm/src:${WORKSPACE_DIR}/components/backends/vllm/src:${WORKSPACE_DIR}/components/backends/sglang/src:${WORKSPACE_DIR}/components/backends/llama_cpp/src:${WORKSPACE_DIR}/benchmarks/profiler:$PYTHONPATH |
🤖 Prompt for AI Agents
In container/Dockerfile.sglang around line 323, the ENV PYTHONPATH currently
overwrites any inherited PYTHONPATH; change it to append the existing
$PYTHONPATH (and optionally include ${WORKSPACE_DIR}/examples/sglang/utils) so
upstream/runtime additions like /workspace/examples/sglang/utils are preserved —
update the ENV to add your listed component paths then include :$PYTHONPATH at
the end (also add ${WORKSPACE_DIR}/examples/sglang/utils into the list if those
imports are needed).
|
Thats great it is working for vLLM and SGLang, was trtllm tested? I don't see such an update (or |
| echo "cat ~/.launch_screen" >> ~/.bashrc && \ | ||
| echo "source $VIRTUAL_ENV/bin/activate" >> ~/.bashrc | ||
|
|
||
| # Set PYTHONPATH to include all component paths and benchmarks/profiler for utils module access |
There was a problem hiding this comment.
benchmarks is already installed in runtime image, see L270 -- why do we need to set python path here?
There was a problem hiding this comment.
Benchmarks is called as a module and requires helper functions in other files; as an alternative, I can have the profiling script set PYTHONPATH within the yaml file @nv-anants
There was a problem hiding this comment.
can we include all the helper functions in benchmarks module itself? ideally we should not be setting pythonpath anywhere
There was a problem hiding this comment.
from config here seems like only data_gen dir is included in benchmarks install, adding profile here might solve it as well? https://github.com/ai-dynamo/dynamo/blob/main/benchmarks/pyproject.toml#L63
There was a problem hiding this comment.
I think https://github.com/ai-dynamo/dynamo/pull/2857/files has the fix you're referring to, but it doesn't appear to be in the release branch; this could also be why images I built locally (which includes this MR's changes) work properly without the need to set PYTHONPATH. I just built a runtime image locally (undoing the changes in this MR, but with !2857's changes in main) and the import issue reported in bug is resolved. Could we just cherry pick !2857 to the release branch? cc @tedzhouhk
There was a problem hiding this comment.
sure yeah, that one looks good to me
| # Make sure to sync this with the one specified on README.md. | ||
| # This is a generic PYTHONPATH which works for all the frameworks, so some paths may not be relevant for this particular framework. | ||
| ENV PYTHONPATH=${WORKSPACE_DIR}/components/metrics/src:${WORKSPACE_DIR}/components/frontend/src:${WORKSPACE_DIR}/components/planner/src:${WORKSPACE_DIR}/components/backends/mocker/src:${WORKSPACE_DIR}/components/backends/trtllm/src:${WORKSPACE_DIR}/components/backends/vllm/src:${WORKSPACE_DIR}/components/backends/sglang/src:${WORKSPACE_DIR}/components/backends/llama_cpp/src | ||
| # PYTHONPATH is inherited from the runtime stage |
There was a problem hiding this comment.
please dont set python path in runtime image -- everything should be used from venv install there. dev image uses python path for dev purposes
| COPY ATTRIBUTION* LICENSE /workspace/ | ||
|
|
||
| ENV PYTHONPATH=/workspace/examples/sglang/utils:$PYTHONPATH | ||
| ENV PYTHONPATH=/workspace/examples/sglang/utils:/workspace/benchmarks/profiler:$PYTHONPATH |
There was a problem hiding this comment.
same as vllm -- benchmarks is installed in L240, why do we need this?
Overview:
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Refactor
Chores