Skip to content

Conversation

@tzulingk
Copy link
Contributor

@tzulingk tzulingk commented Dec 2, 2025

Overview:

Fix vllm finder module path. Note this is a helper Dockerfile for WideEP FT project. We need this to test the vllm implementation before submitting PR for views

Details:

Fix vllm finder module path.

Where should the reviewer start?

tests/fault_tolerance/deploy/container/Dockerfile.local_vllm

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

DIS-1123

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved vLLM module path resolution in container environments for enhanced compatibility and runtime stability.
  • Chores

    • Refined container build configuration for streamlined dependency setup.

✏️ Tip: You can customize this high-level summary in your review settings.

Signed-off-by: tzulingk@nvidia.com <tzulingk@nvidia.com>
@github-actions github-actions bot added the fix label Dec 2, 2025
@tzulingk tzulingk enabled auto-merge (squash) December 2, 2025 03:46
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 2, 2025

Walkthrough

The Dockerfile for local vLLM deployment has been updated to replace a broad .pth file search remediation with a targeted approach that specifically updates vLLM finder module paths, changing directory references from /vllm-workspace to /opt/vllm.

Changes

Cohort / File(s) Change Summary
vLLM Dockerfile path configuration
tests/fault_tolerance/deploy/container/Dockerfile.local_vllm
Replaced generic .pth file search with targeted fix for vLLM-specific __editable___vllm*_finder.py files, updating path references from /vllm-workspace to /opt/vllm

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Container configuration change with straightforward path replacement
  • Single file modification with clear, targeted scope

Poem

🐰 Paths were wandering through the workspace deep,
Now vLLM's finder modules leap and creep,
To /opt/vllm, they find their way,
No broad searches needed today!

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: fixing the vllm finder module path in the Dockerfile.
Description check ✅ Passed The description covers all required template sections with adequate detail about the fix, its purpose, and file location.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
tests/fault_tolerance/deploy/container/Dockerfile.local_vllm (1)

104-113: Consider documenting the scope of path remediation.

The RUN block now remediates paths for three packages (pplx_kernels, DeepEP, vLLM). While targeted, adding an inline comment clarifying why only these packages require remediation would improve maintainability for future developers.

For example, you could add a comment at the start of the block:

# Fix the .pth files to point to the correct location for pplx_kernels and DeepEP
+# Note: Only pplx_kernels, DeepEP, and vLLM modules have hardcoded /vllm-workspace paths
 RUN if [ -f ${VIRTUAL_ENV}/lib/python${PYTHON_VERSION}/site-packages/__editable__.pplx_kernels-0.0.1.pth ]; then \
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5708b70 and e4f73a2.

📒 Files selected for processing (1)
  • tests/fault_tolerance/deploy/container/Dockerfile.local_vllm (1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 4323
File: components/src/dynamo/vllm/main.py:197-205
Timestamp: 2025-11-14T01:09:35.244Z
Learning: In components/src/dynamo/vllm/main.py, keivenchang considers temporary directory cleanup from tempfile.mkdtemp() to be LOW PRIORITY for production because containerized deployment patterns automatically clean up temp directories when containers are destroyed, mitigating resource leak concerns.
Learnt from: nnshah1
Repo: ai-dynamo/dynamo PR: 2489
File: container/deps/vllm/install_vllm.sh:151-152
Timestamp: 2025-08-18T16:52:15.659Z
Learning: The VLLM_PRECOMPILED_WHEEL_LOCATION environment variable, when exported, automatically triggers vLLM's build system to use the precompiled wheel instead of building from source, even when using standard `uv pip install .` commands in container/deps/vllm/install_vllm.sh.
Learnt from: nnshah1
Repo: ai-dynamo/dynamo PR: 2489
File: container/deps/vllm/install_vllm.sh:151-152
Timestamp: 2025-08-18T16:52:15.659Z
Learning: The VLLM_PRECOMPILED_WHEEL_LOCATION environment variable is an official vLLM environment variable that, when exported, automatically triggers vLLM's build system to use the specified precompiled wheel instead of building from source. This works even with standard `uv pip install .` commands without requiring explicit reference to the variable in the install command. The vLLM build system internally detects and uses this environment variable.
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2797
File: container/Dockerfile:437-449
Timestamp: 2025-08-30T20:43:49.632Z
Learning: In the dynamo project's devcontainer setup, the team prioritizes consistency across framework-specific Dockerfiles (like container/Dockerfile, container/Dockerfile.vllm, etc.) by mirroring their structure, even when individual optimizations might be possible, to maintain uniformity in the development environment setup.
📚 Learning: 2025-07-01T15:39:56.789Z
Learnt from: nnshah1
Repo: ai-dynamo/dynamo PR: 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.

Applied to files:

  • tests/fault_tolerance/deploy/container/Dockerfile.local_vllm
📚 Learning: 2025-08-30T20:43:49.632Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2797
File: container/Dockerfile:437-449
Timestamp: 2025-08-30T20:43:49.632Z
Learning: In the dynamo project's devcontainer setup, the team prioritizes consistency across framework-specific Dockerfiles (like container/Dockerfile, container/Dockerfile.vllm, etc.) by mirroring their structure, even when individual optimizations might be possible, to maintain uniformity in the development environment setup.

Applied to files:

  • tests/fault_tolerance/deploy/container/Dockerfile.local_vllm
📚 Learning: 2025-11-14T01:09:35.244Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 4323
File: components/src/dynamo/vllm/main.py:197-205
Timestamp: 2025-11-14T01:09:35.244Z
Learning: In components/src/dynamo/vllm/main.py, keivenchang considers temporary directory cleanup from tempfile.mkdtemp() to be LOW PRIORITY for production because containerized deployment patterns automatically clean up temp directories when containers are destroyed, mitigating resource leak concerns.

Applied to files:

  • tests/fault_tolerance/deploy/container/Dockerfile.local_vllm
📚 Learning: 2025-09-25T00:54:01.369Z
Learnt from: kthui
Repo: ai-dynamo/dynamo PR: 3193
File: tests/fault_tolerance/cancellation/test_trtllm.py:4-204
Timestamp: 2025-09-25T00:54:01.369Z
Learning: The fault tolerance tests in tests/fault_tolerance/cancellation/ run in a controlled container environment where files written to /workspace are automatically cleaned up after test completion, and tests execute sequentially without concurrency concerns, so temporary file management for config files is not necessary.

Applied to files:

  • tests/fault_tolerance/deploy/container/Dockerfile.local_vllm
📚 Learning: 2025-08-18T16:52:15.659Z
Learnt from: nnshah1
Repo: ai-dynamo/dynamo PR: 2489
File: container/deps/vllm/install_vllm.sh:151-152
Timestamp: 2025-08-18T16:52:15.659Z
Learning: The VLLM_PRECOMPILED_WHEEL_LOCATION environment variable, when exported, automatically triggers vLLM's build system to use the precompiled wheel instead of building from source, even when using standard `uv pip install .` commands in container/deps/vllm/install_vllm.sh.

Applied to files:

  • tests/fault_tolerance/deploy/container/Dockerfile.local_vllm
📚 Learning: 2025-05-28T22:54:46.875Z
Learnt from: grahamking
Repo: ai-dynamo/dynamo PR: 1177
File: container/Dockerfile.vllm:102-105
Timestamp: 2025-05-28T22:54:46.875Z
Learning: In Dockerfiles, when appending to environment variables that may not exist in the base image, Docker validation will fail if you reference undefined variables with ${VARIABLE} syntax. In such cases, setting the environment variable directly (e.g., ENV CPATH=/usr/include) rather than appending is the appropriate approach.

Applied to files:

  • tests/fault_tolerance/deploy/container/Dockerfile.local_vllm
📚 Learning: 2025-08-18T16:52:15.659Z
Learnt from: nnshah1
Repo: ai-dynamo/dynamo PR: 2489
File: container/deps/vllm/install_vllm.sh:151-152
Timestamp: 2025-08-18T16:52:15.659Z
Learning: The VLLM_PRECOMPILED_WHEEL_LOCATION environment variable is an official vLLM environment variable that, when exported, automatically triggers vLLM's build system to use the specified precompiled wheel instead of building from source. This works even with standard `uv pip install .` commands without requiring explicit reference to the variable in the install command. The vLLM build system internally detects and uses this environment variable.

Applied to files:

  • tests/fault_tolerance/deploy/container/Dockerfile.local_vllm
⏰ 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). (5)
  • GitHub Check: sglang (amd64)
  • GitHub Check: trtllm (amd64)
  • GitHub Check: vllm (amd64)
  • GitHub Check: operator (amd64)
  • GitHub Check: Build and Test - dynamo
🔇 Additional comments (1)
tests/fault_tolerance/deploy/container/Dockerfile.local_vllm (1)

112-113: Syntax and pattern are correct; verify at runtime that vLLM finder modules exist.

The sed command follows the identical pattern successfully established for the DeepEP finder modules on line 111, with correct escaping and syntax. The find pattern __editable___vllm*_finder.py is consistent with Python's editable installation naming conventions and matches the equivalent DeepEP pattern. Path mapping is correct: /vllm-workspace (line 98 COPY source) → /opt/vllm (line 98 COPY destination).

However, verification during local testing is recommended to confirm that vLLM's build system generates finder modules matching this pattern and that the sed replacements are applied correctly.

Signed-off-by: tzulingk@nvidia.com <tzulingk@nvidia.com>
@tzulingk tzulingk merged commit ca264be into main Dec 2, 2025
29 of 32 checks passed
@tzulingk tzulingk deleted the tzulingk/docker_local_vllm branch December 2, 2025 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants