Skip to content

feat: update PYTHONPATH in runtime image #2982

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

feat: update PYTHONPATH in runtime image #2982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions container/Dockerfile.sglang
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ COPY components/backends/sglang /workspace/components/backends/sglang
# Copy attribution files
COPY ATTRIBUTION* LICENSE /workspace/

ENV PYTHONPATH=/workspace/examples/sglang/utils:$PYTHONPATH
ENV PYTHONPATH=/workspace/examples/sglang/utils:/workspace/benchmarks/profiler:$PYTHONPATH
Copy link
Contributor

@nv-anants nv-anants Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as vllm -- benchmarks is installed in L240, why do we need this?


ENTRYPOINT ["/opt/nvidia/nvidia_entrypoint.sh"]
CMD []
Expand Down Expand Up @@ -320,7 +320,7 @@ RUN uv pip install maturin[patchelf]

# 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
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:$PYTHONPATH

Optionally 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.

Suggested change
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).


ENTRYPOINT ["/opt/nvidia/nvidia_entrypoint.sh"]
CMD []
8 changes: 5 additions & 3 deletions container/Dockerfile.vllm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,10 @@ RUN --mount=type=bind,source=./container/launch_message.txt,target=/workspace/la
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
Copy link
Contributor

@nv-anants nv-anants Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

benchmarks is already installed in runtime image, see L270 -- why do we need to set python path here?

Copy link
Contributor Author

@hhzhang16 hhzhang16 Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@nv-anants nv-anants Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we include all the helper functions in benchmarks module itself? ideally we should not be setting pythonpath anywhere

Copy link
Contributor

@nv-anants nv-anants Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@hhzhang16 hhzhang16 Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@nv-anants nv-anants Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure yeah, that one looks good to me

# 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/components/metrics/src:/workspace/components/frontend/src:/workspace/components/planner/src:/workspace/components/backends/mocker/src:/workspace/components/backends/trtllm/src:/workspace/components/backends/vllm/src:/workspace/components/backends/sglang/src:/workspace/components/backends/llama_cpp/src:/workspace/benchmarks/profiler:$PYTHONPATH

ENTRYPOINT ["/opt/nvidia/nvidia_entrypoint.sh"]
CMD []

Expand Down Expand Up @@ -516,9 +520,7 @@ COPY --from=runtime ${VIRTUAL_ENV} ${VIRTUAL_ENV}
# so we can use maturin develop
RUN uv pip install maturin[patchelf]

# 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
Copy link
Contributor

@nv-anants nv-anants Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please dont set python path in runtime image -- everything should be used from venv install there. dev image uses python path for dev purposes


ENTRYPOINT ["/opt/nvidia/nvidia_entrypoint.sh"]
CMD []
Loading