fix: add capi to pip and make it fallback #1904
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alec-flowers
requested review from
a team,
GuanLuo,
PeaBrane,
biswapanda,
grahamking,
hhzhang16,
hutm,
ishandhanani,
jthomson04,
julienmancuso,
kkranen,
mohammedabdulwahhab,
nnshah1,
paulhendricks,
piotrm-nvidia,
ptarasiewiczNV,
rmccorm4,
ryanolson,
tanmayv25,
tedzhouhk and
tmonty12
as code owners
Contributor
WalkthroughA new utility function, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant vllm_inc.py
participant dynamo.sdk.lib.utils
participant OS
User->>vllm_inc.py: Start vLLM worker
vllm_inc.py->>dynamo.sdk.lib.utils: get_capi_library_path()
dynamo.sdk.lib.utils->>OS: Check VLLM_KV_CAPI_PATH env var
alt Env var set
dynamo.sdk.lib.utils-->>vllm_inc.py: Return env var path
else Env var not set
dynamo.sdk.lib.utils->>OS: Locate default library path
dynamo.sdk.lib.utils-->>vllm_inc.py: Return default path
end
vllm_inc.py->>OS: Set VLLM_KV_CAPI_PATH env var
Poem
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
deploy/sdk/src/dynamo/sdk/__init__.py(2 hunks)deploy/sdk/src/dynamo/sdk/lib/utils.py(1 hunks)hatch_build.py(1 hunks)launch/dynamo-run/src/subprocess/vllm_inc.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
launch/dynamo-run/src/subprocess/vllm_inc.py (2)
Learnt from: nnshah1
PR: ai-dynamo/dynamo#1444
File: tests/fault_tolerance/utils/metrics.py:30-32
Timestamp: 2025-07-01T13:55:03.940Z
Learning: The `@dynamo_worker()` decorator in the dynamo codebase returns a wrapper that automatically injects the `runtime` parameter before calling the wrapped function. This means callers only need to provide the non-runtime parameters, while the decorator handles injecting the runtime argument automatically. For example, a function with signature `async def get_metrics(runtime, log_dir)` decorated with `@dynamo_worker()` can be called as `get_metrics(log_dir)` because the decorator wrapper injects the runtime parameter.
Learnt from: GuanLuo
PR: ai-dynamo/dynamo#1371
File: examples/llm/benchmarks/vllm_multinode_setup.sh:18-25
Timestamp: 2025-06-05T01:46:15.509Z
Learning: In multi-node setups with head/worker architecture, the head node typically doesn't need environment variables pointing to its own services (like NATS_SERVER, ETCD_ENDPOINTS) because local processes can access them via localhost. Only worker nodes need these environment variables to connect to the head node's external IP address.
🧬 Code Graph Analysis (2)
deploy/sdk/src/dynamo/sdk/__init__.py (1)
deploy/sdk/src/dynamo/sdk/lib/utils.py (1)
get_capi_library_path(137-159)
launch/dynamo-run/src/subprocess/vllm_inc.py (1)
deploy/sdk/src/dynamo/sdk/lib/utils.py (1)
get_capi_library_path(137-159)
⏰ 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: Mirror Repository to GitLab
- GitHub Check: Build and Test - vllm
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (.)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
🔇 Additional comments (3)
hatch_build.py (1)
31-31: LGTM! Ensures library is included in the wheel.The mapping correctly includes the shared library in the build output at the expected location that aligns with the utility function's fallback path.
deploy/sdk/src/dynamo/sdk/__init__.py (1)
26-26: LGTM! Properly exposes the new utility function.The import and export follow standard Python patterns for making the new functionality available as part of the public API.
Also applies to: 43-43
launch/dynamo-run/src/subprocess/vllm_inc.py (1)
39-39: LGTM! Import follows existing patterns.The import is correctly placed and follows the file's import organization.
ishandhanani
approved these changes
Jul 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview:
Fixes: https://nvbugspro.nvidia.com/bug/5385756
Summary by CodeRabbit