fix(container): use released NIXL for SGLang EFA - #12106
Conversation
| export PIP_CACHE_DIR=/root/.cache/pip && \ | ||
| pip install --break-system-packages --no-deps "distro==1.9.0" | ||
|
|
||
| {% if make_efa == true and device == "cuda" and target == "runtime" %} |
There was a problem hiding this comment.
No need for this, we should just introduce ability to upgrade NIXL wheels, it should not be specific to EFA or SGLang.
I'm also not sure why install_nixl_from_wheel is a case just for EFA? i see this in the vLLM container and it's not related to SGLang nor EFA, can you explain more about this?
There was a problem hiding this comment.
I’m trying to keep this PR narrowly scoped to the know broken image, which is the SGLang EFA image to reduce impact radius.
Your suggestion makes sense for long term in main branch.
| RUN --mount=type=cache,target=/root/.cache/pip,sharing=locked \ | ||
| set -eux; \ | ||
| export PIP_CACHE_DIR=/root/.cache/pip; \ | ||
| pip install --break-system-packages --force-reinstall --no-deps --only-binary=:all: \ |
There was a problem hiding this comment.
This should be introduced as a general argument to force update wheel files to a nixl version.
It should not be related just to EFA and should be applied on the vLLM and SGLang container runtime.
When building for EFA we override this argument with the updated nixl version.
Why? hard coding this will create issues when the upstream framework container images will catch up on NIXL versions, we should make it dynamic and configurable.
There was a problem hiding this comment.
This is to make the PR scope narrow for this specific release.
4373245 to
dabe508
Compare
dabe508 to
b37c3bf
Compare
b37c3bf to
e2a2371
Compare
|
LGTM, pending pypi release for NIXL 1.3.2 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| - name: libfabric | ||
| # AWS fork: shipped by EFA installer 1.47.0 as libfabric1-aws_2.4.0amzn1.0 | ||
| # (Ubuntu 24.04 DEB). The `amzn1.0` suffix indicates an AWS patch set on | ||
| # top of upstream libfabric 2.4.0; preserved here for OSRB traceability. | ||
| version: 2.4.0amzn1.0 | ||
| version: 2.4.0amzn5.0 | ||
| license: BSD-2-Clause | ||
| source: https://github.com/ofiwg/libfabric | ||
| source: https://github.com/aws/libfabric | ||
| images: | ||
| - vllm-runtime-efa | ||
| - trtllm-runtime-efa | ||
| - sglang-runtime-efa | ||
|
|
||
| - name: aws-ofi-nccl | ||
| # Shipped by EFA installer 1.47.0 as libnccl-ofi_1.18.0-1 (Ubuntu 24.04 DEB). | ||
| version: 1.18.0 | ||
| version: 1.20.0 | ||
| license: Apache-2.0 | ||
| source: https://github.com/aws/aws-ofi-nccl | ||
| images: | ||
| - vllm-runtime-efa | ||
| - trtllm-runtime-efa | ||
| - sglang-runtime-efa |
There was a problem hiding this comment.
🔍 Compliance version attributions rely on audit job for verification
The native package attributions were updated for EFA 1.49.0: libfabric bumped to 2.4.0amzn5.0 with source changed to github.com/aws/libfabric, and aws-ofi-nccl to 1.20.0 (container/compliance/native_packages.yaml:38-47), plus sglang-runtime-efa added to both image filters. These exact upstream version strings could not be verified from within the repo; per the file's own note the authoritative check is the <framework>-compliance-audit syft scan. If the EFA 1.49.0 installer actually ships different versions, the NOTICES attribution would be inaccurate. Recommend confirming against the 1.49.0 installer's bundled DEB versions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ENV EFA_VERSION="${EFA_VERSION}" | ||
|
|
There was a problem hiding this comment.
🔍 aws.Dockerfile libfabric overlay removal changes EFA behavior for all frameworks
This PR (via the underlying #12113 cherry-pick) removes the wheel_builder libfabric overlay logic that conditionally overwrote the EFA-bundled libfabric when older than NIXL_LIBFABRIC_REF (container/templates/aws.Dockerfile:44-67 deleted), and stops purging /opt/amazon/aws-ofi-nccl. This means vllm-runtime-efa and trtllm-runtime-efa now ship the EFA-bundled libfabric/aws-ofi-nccl unconditionally rather than the wheel_builder-built version. nixl_libfabric_ref was also downgraded v2.5.1 -> v2.4.0 in context.yaml but is still consumed by wheel_builder (container/templates/wheel_builder.Dockerfile:437). Behavior change is presumably intentional with the newer EFA installer, but the runtime libfabric version now shipped by vllm/trtllm EFA images differs from before — worth confirming those images still function with the EFA-stock libfabric.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment has been minimized.
This comment has been minimized.
|
Please retarget to |
Signed-off-by: Erez Zarum <erezz@amazon.com> Signed-off-by: Jie Hao <jihao@nvidia.com>
Signed-off-by: Jie Hao <jihao@nvidia.com>
e2a2371 to
c7ae914
Compare
Both were labelled 'Targeted fix: v1.3.1' but shipped no fix in that release. v1.3.1 carried one functional change (#12106, the EFA KV-transfer stall); the branch has no KVBM commit, and it still pins TensorRT-LLM 1.3.0rc19 while that issue's fix requires rc21. main is on rc23, so v1.4.0 is where both land. Signed-off-by: Dan Gil <dagil@nvidia.com>
4 participants
Summary
nixl/nixl-cu13wheels in the SGLang CUDA EFA runtime image throughNIXL_WHEEL_VERSION=1.3.2Notes
container/templates/sglang_runtime.Dockerfile.Validation
python3 container/render.py --framework sglang --device cuda --target runtime --cuda-version 13.0 --make-efa --platform linux/amd64python3 container/render.py --framework sglang --device cuda --target runtime --cuda-version 13.0 --make-efa --platform linux/arm64git diff --checkgit commit -shook was retried with--no-verify; the localpytest-marker-reporthook fails before checking changed files because this machine runs it under a Python that cannot evaluatestr | Noneintests/conftest.py