fix: Add ENV variables in docker files for NIXL Plugins #1490
Conversation
Signed-off-by: Murali Andoorveedu <[email protected]>
Signed-off-by: Murali Andoorveedu <[email protected]>
Signed-off-by: Murali Andoorveedu <[email protected]>
andoorve
requested review from
a team,
alec-flowers,
ishandhanani,
nnshah1,
ptarasiewiczNV,
richardhuo-nv,
rmccorm4 and
tanmayv25
as code owners
|
👋 Hi andoorve! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
bot
added
the
external-contribution
andoorve
changed the title
WalkthroughEnvironment variables for NIXL plugin discovery have been added to three Dockerfiles. The changes set Changes
Sequence Diagram(s)sequenceDiagram
participant Dockerfile
participant ContainerEnv
participant NIXL Runtime
Dockerfile->>ContainerEnv: Set LD_LIBRARY_PATH with NIXL and UCX paths
Dockerfile->>ContainerEnv: Set NIXL_PLUGIN_DIR to plugins directory
ContainerEnv->>NIXL Runtime: Provide environment variables at runtime
NIXL Runtime->>NIXL Runtime: Discover plugins using environment variables
Possibly related PRs
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
container/Dockerfile.tensorrt_llm (1)
330-333: Parameterize NIXL plugin path using ARCH_ALTHard-coding
x86_64prevents building for other architectures (e.g., aarch64). Use the existingARCH_ALTARG to generate the correct library and plugin paths.Apply this diff:
- # Set environment variables for NIXL to find the right plugin - ENV LD_LIBRARY_PATH="/usr/local/nixl/lib/x86_64-linux-gnu:/usr/local/nixl/lib/x86_64-linux-gnu/plugins:/usr/local/ucx/lib:$LD_LIBRARY_PATH" - ENV NIXL_PLUGIN_DIR="/usr/local/nixl/lib/x86_64-linux-gnu/plugins" + # Set environment variables for NIXL to find the right plugin + ENV LD_LIBRARY_PATH="/usr/local/nixl/lib/${ARCH_ALT}-linux-gnu:/usr/local/nixl/lib/${ARCH_ALT}-linux-gnu/plugins:/usr/local/ucx/lib:$LD_LIBRARY_PATH" + ENV NIXL_PLUGIN_DIR="/usr/local/nixl/lib/${ARCH_ALT}-linux-gnu/plugins"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
container/Dockerfile.sglang(1 hunks)container/Dockerfile.tensorrt_llm(1 hunks)container/Dockerfile.vllm(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
| # Set environment variables for NIXL to find the right plugin | ||
| ENV LD_LIBRARY_PATH="/usr/local/nixl/lib/x86_64-linux-gnu:/usr/local/nixl/lib/x86_64-linux-gnu/plugins:/usr/local/ucx/lib:$LD_LIBRARY_PATH" | ||
| ENV NIXL_PLUGIN_DIR="/usr/local/nixl/lib/x86_64-linux-gnu/plugins" | ||
|
|
There was a problem hiding this comment.
Ensure NIXL/UCX assets are copied in the runtime stage
You’ve set LD_LIBRARY_PATH and NIXL_PLUGIN_DIR here, but the runtime image never copies /usr/local/ucx or /usr/local/nixl from the base stage. Without those directories, plugin discovery will fail. Also, avoid hard-coding x86_64.
Suggested patch:
@@ FROM ${RUNTIME_IMAGE}:${RUNTIME_IMAGE_TAG} AS runtime
+ # Bring in UCX and NIXL from the base image for plugin support
+ COPY --from=base /usr/local/ucx /usr/local/ucx
+ COPY --from=base /usr/local/nixl /usr/local/nixl
# Set environment variables for NIXL to find the right plugin
- ENV LD_LIBRARY_PATH="/usr/local/nixl/lib/x86_64-linux-gnu:/usr/local/nixl/lib/x86_64-linux-gnu/plugins:/usr/local/ucx/lib:$LD_LIBRARY_PATH"
- ENV NIXL_PLUGIN_DIR="/usr/local/nixl/lib/x86_64-linux-gnu/plugins"
+ ARG ARCH_ALT
+ ENV LD_LIBRARY_PATH="/usr/local/nixl/lib/${ARCH_ALT}-linux-gnu:/usr/local/nixl/lib/${ARCH_ALT}-linux-gnu/plugins:/usr/local/ucx/lib:$LD_LIBRARY_PATH"
+ ENV NIXL_PLUGIN_DIR="/usr/local/nixl/lib/${ARCH_ALT}-linux-gnu/plugins"🤖 Prompt for AI Agents
In container/Dockerfile.sglang around lines 400 to 403, the environment
variables LD_LIBRARY_PATH and NIXL_PLUGIN_DIR are set to paths under
/usr/local/nixl and /usr/local/ucx, but these directories are not copied into
the runtime stage from the base stage. To fix this, update the Dockerfile to
copy the /usr/local/nixl and /usr/local/ucx directories from the base stage into
the runtime stage. Additionally, replace the hard-coded architecture segment
"x86_64-linux-gnu" with a dynamic variable or a more generic path to avoid
architecture-specific hardcoding.
| ENV LD_LIBRARY_PATH="/usr/local/nixl/lib/x86_64-linux-gnu:/usr/local/nixl/lib/x86_64-linux-gnu/plugins:/usr/local/ucx/lib:$LD_LIBRARY_PATH" | ||
| ENV NIXL_PLUGIN_DIR="/usr/local/nixl/lib/x86_64-linux-gnu/plugins" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate static override of plugin env
The block at lines 533–535 restates LD_LIBRARY_PATH and NIXL_PLUGIN_DIR with a hard-coded x86_64 path, overriding the earlier parameterized values. Remove these lines to avoid conflicts:
- # Set environment variables for NIXL to find the right plugin
- ENV LD_LIBRARY_PATH="/usr/local/nixl/lib/x86_64-linux-gnu:/usr/local/nixl/lib/x86_64-linux-gnu/plugins:/usr/local/ucx/lib:$LD_LIBRARY_PATH"
- ENV NIXL_PLUGIN_DIR="/usr/local/nixl/lib/x86_64-linux-gnu/plugins"📝 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 LD_LIBRARY_PATH="/usr/local/nixl/lib/x86_64-linux-gnu:/usr/local/nixl/lib/x86_64-linux-gnu/plugins:/usr/local/ucx/lib:$LD_LIBRARY_PATH" | |
| ENV NIXL_PLUGIN_DIR="/usr/local/nixl/lib/x86_64-linux-gnu/plugins" |
🤖 Prompt for AI Agents
In container/Dockerfile.vllm around lines 533 to 535, remove the two ENV lines
that set LD_LIBRARY_PATH and NIXL_PLUGIN_DIR with hard-coded x86_64-linux-gnu
paths, as they duplicate and override earlier parameterized environment variable
settings, causing conflicts.
|
This is already in, I just missed it |
1 participant
Overview:
Adds the correct env variables for pointers to the nixl plugins.
Summary by CodeRabbit