feat(container): libfabric2.5.1 for EFA and TRTLLM - #10047
Conversation
Signed-off-by: Erez Zarum <erezz@amazon.com>
|
👋 Hi erezzarum! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
feat
external-contribution
|
/ok to test f143f4d |
WalkthroughThe PR updates the AWS EFA Dockerfile template with three configuration changes: narrowed post-installer cleanup scope, a new libfabric directory overlay strategy with pkgconfig rewriting when version conditions are met, and simplified TRTLLM NIXL plugin setup using direct environment variable configuration instead of arch-specific logic. ChangesAWS EFA and NIXL Configuration
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🧹 Nitpick comments (1)
container/templates/aws.Dockerfile (1)
81-89: ⚡ Quick winUpdate the TRTLLM note to match the new flow.
The block comment above still says this stage clears
LD_PRELOADand exposes a stable/opt/nvidia/nvda_nixl/pluginsalias, but Lines 84-89 now setLD_PRELOADtolibnixl.soand pointNIXL_PLUGIN_DIRatlib64/pluginsdirectly. Please sync the comment so it does not mislead the next change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@container/templates/aws.Dockerfile` around lines 81 - 89, The comment above the RUN/ENV block is out of sync: it still claims this stage clears LD_PRELOAD and exposes a stable /opt/nvidia/nvda_nixl/plugins alias, but the Dockerfile now sets LD_PRELOAD to /opt/nvidia/nvda_nixl/lib64/libnixl.so and NIXL_PLUGIN_DIR to /opt/nvidia/nvda_nixl/lib64/plugins; update that note to describe the new flow (explicitly mention LD_PRELOAD is set to libnixl.so and NIXL_PLUGIN_DIR points at lib64/plugins rather than a cleared LD_PRELOAD or a plugins alias) so future readers see accurate behavior for LD_PRELOAD and NIXL_PLUGIN_DIR.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@container/templates/aws.Dockerfile`:
- Line 39: The cleanup command is removing /etc/ld.so.conf.d/aws-ofi-nccl.conf
but targeting the wrong plugin directory (/opt/amazon/aws-ofi-nccl); update the
Dockerfile cleanup to remove the correct installed path (/opt/aws-ofi-nccl)
instead of /opt/amazon/aws-ofi-nccl so both the plugin directory and the .conf
file are deleted (adjust the rm -rf target from "/opt/amazon/aws-ofi-nccl" to
"/opt/aws-ofi-nccl" while keeping "/etc/ld.so.conf.d/aws-ofi-nccl.conf").
- Around line 58-62: Do not delete the installer-owned EFA tree; instead of "rm
-rf /opt/amazon/efa && cp -Pfr /tmp/libfabric_build /opt/amazon/efa", copy/merge
the overlay into the existing prefix (e.g., use rsync -a or cp -a
--no-dereference to merge from /tmp/libfabric_build into /opt/amazon/efa) so
existing files owned by the installer are preserved; keep the sed edit on
/opt/amazon/efa/lib/pkgconfig/libfabric.pc but only update prefix there, ensure
the ldconfig file (/etc/ld.so.conf.d/000_efa.conf) contains both
/opt/amazon/efa/lib and /opt/amazon/efa/lib64 (append rather than overwrite),
and do not remove /etc/ld.so.conf.d/efa.conf unconditionally—preserve or merge
it instead.
---
Nitpick comments:
In `@container/templates/aws.Dockerfile`:
- Around line 81-89: The comment above the RUN/ENV block is out of sync: it
still claims this stage clears LD_PRELOAD and exposes a stable
/opt/nvidia/nvda_nixl/plugins alias, but the Dockerfile now sets LD_PRELOAD to
/opt/nvidia/nvda_nixl/lib64/libnixl.so and NIXL_PLUGIN_DIR to
/opt/nvidia/nvda_nixl/lib64/plugins; update that note to describe the new flow
(explicitly mention LD_PRELOAD is set to libnixl.so and NIXL_PLUGIN_DIR points
at lib64/plugins rather than a cleared LD_PRELOAD or a plugins alias) so future
readers see accurate behavior for LD_PRELOAD and NIXL_PLUGIN_DIR.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3221241-866f-405c-a49d-db36cfcc5205
📒 Files selected for processing (1)
container/templates/aws.Dockerfile
| # before re-running ldconfig. | ||
| rm -rf /opt/amazon/aws-ofi-nccl /opt/amazon/ofi-nccl \ | ||
| /etc/ld.so.conf.d/aws-ofi-nccl.conf && \ | ||
| rm -rf /opt/amazon/aws-ofi-nccl /etc/ld.so.conf.d/aws-ofi-nccl.conf && \ |
There was a problem hiding this comment.
Fix the aws-ofi-nccl cleanup path.
The EFA images elsewhere in this repo use /opt/aws-ofi-nccl, so this currently removes the .conf file but leaves the installed plugin directory behind.
Suggested fix
- rm -rf /opt/amazon/aws-ofi-nccl /etc/ld.so.conf.d/aws-ofi-nccl.conf && \
+ rm -rf /opt/aws-ofi-nccl /etc/ld.so.conf.d/aws-ofi-nccl.conf && \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@container/templates/aws.Dockerfile` at line 39, The cleanup command is
removing /etc/ld.so.conf.d/aws-ofi-nccl.conf but targeting the wrong plugin
directory (/opt/amazon/aws-ofi-nccl); update the Dockerfile cleanup to remove
the correct installed path (/opt/aws-ofi-nccl) instead of
/opt/amazon/aws-ofi-nccl so both the plugin directory and the .conf file are
deleted (adjust the rm -rf target from "/opt/amazon/aws-ofi-nccl" to
"/opt/aws-ofi-nccl" while keeping "/etc/ld.so.conf.d/aws-ofi-nccl.conf").
There was a problem hiding this comment.
We never use /opt/aws-ofi-nccl can you confirm this?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all references to aws-ofi-nccl paths in the repository
rg -n "aws-ofi-nccl" --type=dockerfile
rg -n "ofi-nccl" --type=dockerfile
# Also search in shell scripts and other files
rg -n "aws-ofi-nccl" -g "*.sh" -g "*.yml" -g "*.yaml" -g "*.json" -g "*.py" -g "*.txt" -g "*.md" 2>/dev/null || true
rg -rn "opt/aws-ofi-nccl" 2>/dev/null | head -30
rg -rn "opt/amazon/aws-ofi-nccl" 2>/dev/null | head -30
rg -rn "opt/amazon/ofi-nccl" 2>/dev/null | head -30Length of output: 3490
@erezzarum You are correct — my suggestion was wrong. The path /opt/aws-ofi-nccl does exist in the repo, but only in an unrelated recipe (recipes/glm-5-nvfp4/sglang/disagg/efa/) and docs — it has no connection to this Dockerfile.
Looking at the diff more carefully, the current code (rm -rf /opt/amazon/aws-ofi-nccl) makes sense in context: the pre-PR code removed both /opt/amazon/aws-ofi-nccl and /opt/amazon/ofi-nccl (plus the conf file) to fully disable aws-ofi-nccl. This PR intentionally drops the /opt/amazon/ofi-nccl removal so the EFA installer's aws-ofi-nccl is preserved, while still cleaning up the duplicate /opt/amazon/aws-ofi-nccl path. That's consistent with the PR goal of "switching to the EFA installer aws-ofi-nccl."
My original review comment is invalid. Apologies for the noise!
✏️ Learnings added
Learnt from: erezzarum
Repo: ai-dynamo/dynamo PR: 10047
File: container/templates/aws.Dockerfile:39-39
Timestamp: 2026-05-27T16:26:36.525Z
Learning: In the ai-dynamo/dynamo repository, the EFA installer installs aws-ofi-nccl primarily at `/opt/amazon/ofi-nccl` (without the `aws-` prefix). The path `/opt/amazon/aws-ofi-nccl` is a secondary/duplicate location. The path `/opt/aws-ofi-nccl` is used only in unrelated recipes (e.g., `recipes/glm-5-nvfp4/sglang/disagg/efa/`) and is not related to `container/templates/aws.Dockerfile`. Do not confuse these paths when reviewing EFA-related Dockerfile cleanup commands.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: yuanwu2017
Repo: ai-dynamo/dynamo PR: 7889
File: container/templates/dev.Dockerfile:221-225
Timestamp: 2026-04-04T09:52:48.051Z
Learning: For CPU-target build Dockerfiles in ai-dynamo/dynamo (e.g., templates used for CPU device builds), treat hard-coded `x86_64-linux-gnu` multiarch triplets in `NIXL_LIB_DIR` and `NIXL_PLUGIN_DIR` paths as intentional. Do not raise a multi-arch portability/architecture issue for these paths when the target is explicitly x86_64-only for CPU builds.
Signed-off-by: Erez Zarum <erezz@amazon.com>
Signed-off-by: Erez Zarum <erezz@amazon.com> Signed-off-by: Jie Hao <jihao@nvidia.com>
2 participants
Overview:
This PR fixes issues with PR #9727
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit