build: add --build-infinia to bundle the Infinia DDN plugin into the whl#1941
build: add --build-infinia to bundle the Infinia DDN plugin into the whl#1941NirWolfer wants to merge 1 commit into
Conversation
|
👋 Hi NirWolfer! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
/build |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds opt-in Infinia DDN library staging to container builds and packages the staged libraries in manylinux wheels. It also ignores the staging directory and updates six Jenkins matrix configurations to CI image tag ChangesInfinia wheel build
CI image tag refresh
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant build-container.sh
participant INFINIA_LIBS_IMAGE
participant Dockerfile.manylinux
build-container.sh->>INFINIA_LIBS_IMAGE: pull and extract /infinia/$ARCH/
build-container.sh->>Dockerfile.manylinux: pass BUILD_INFINIA=true
Dockerfile.manylinux->>Dockerfile.manylinux: copy libraries into /opt/ddn/red
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: All matrix branches of Root cause: In Implicated commit: Not definitively isolated from history, but the matrix restructuring/rename in File: Suggested fix: Ensure Related: PR #1941 (triggering PR); likely regression from PR #1839 "CI: rename container images". No existing issue found for
|
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: "Compiling NIXL Docker Image" stage failed (exit 125) — Root cause: Implicated commit: Most likely File: The CI pipeline/image-name template that defines Suggested fix: Ensure the UCX-version (or matrix-axis) variable that fills the image-name suffix is set before Related: #1839 (container image rename), #1868 (UCX v1.22.x bump)
|
|
🤖 CI Triage Agent — TL;DR: The Docker/podman build failed with Full analysisSummary: Root cause: The Implicated commit: File: The pipeline step that exports Suggested fix: Ensure the UCX-version matrix variable is set before Related: PR #1941 (under test); CI image rename PR #1839; UCX bump PR #1868.
|
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: Implicated commit: Most likely File: The Jenkins pipeline/matrix definition that exports Suggested fix: Ensure the matrix suffix / Related: PR #1941 (this build); likely regression from #1839 (CI: rename container images) and #1868 (UCX bump). No existing issue found for this exact error.
|
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: In the Implicated commit: unknown — the offending File: Suggested fix:
Related: none found (searched "PR_IMAGE invalid reference format nixl-ci-dl-gpu-test"; the triggering PR is #1941, which is unrelated to the tag-construction logic). |
NirWolfer
force-pushed
the
infinia-wheel-build
branch
from
a9abcb5 to
6c4c8f5
Compare
|
/build |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.ci/jenkins/lib/build-wheel-matrix.yaml:
- Around line 285-309: Update the Build Wheel (Infinia) command invoking
contrib/build-container.sh to pass a distinct image tag via --tag, separate from
the default nixl:${VERSION} reference. Keep the existing build arguments
unchanged and ensure the later sanity-image selection cannot choose this Infinia
image accidentally.
- Around line 285-294: Add Harbor authentication to the “Build Wheel (Infinia)”
matrix path, alongside the existing Artifactory credential setup, so its docker
pull from harbor.mellanox.com succeeds. Reuse the repository’s established
Harbor credential/login mechanism and scope the change only to the Infinia wheel
build.
In `@contrib/Dockerfile.manylinux`:
- Around line 375-381: Update the BUILD_INFINIA branch in the Dockerfile RUN
command to verify that /workspace/nixl/infinia-libs contains files before
copying it to /opt/ddn/red. Fail the build when the source tree is empty, while
preserving the existing package installation and staging behavior for non-empty
trees.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 44668112-f42b-44ac-bcf8-4de4c85f4f50
📒 Files selected for processing (10)
.ci/docs/ci-overview.md.ci/jenkins/lib/build-matrix.yaml.ci/jenkins/lib/build-wheel-matrix.yaml.ci/jenkins/lib/test-dl-ep-matrix.yaml.ci/jenkins/lib/test-dl-matrix.yaml.ci/jenkins/lib/test-matrix.yaml.ci/jenkins/lib/test-sanitizer-matrix.yaml.gitignorecontrib/Dockerfile.manylinuxcontrib/build-container.sh
|
🤖 CI Triage Agent — TL;DR: The SGLang sanity test (stage 615) failed because HuggingFace returned HTTP Full analysisSummary: Root cause: In Implicated commit: unknown (not a source regression — external HF rate limit) File: Suggested fix:
Related: none found Side note: no secrets need quoting here; the SSH key values were already redacted as |
NirWolfer
force-pushed
the
infinia-wheel-build
branch
from
6c4c8f5 to
69f858c
Compare
|
/build |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
contrib/Dockerfile.manylinux (1)
375-381: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the Infinia staging copy with a non-empty check (unresolved from a prior review pass).
cp -a /workspace/nixl/infinia-libs/. /opt/ddn/red/succeeds even when the source directory exists but is empty, so a failed/partial stage flows into the Meson build with no error until the wheel silently lackslibplugin_INFINIA.so. This was already flagged on this exact block in a previous review round and remains unaddressed.🛡️ Suggested fix
RUN if [ "$BUILD_INFINIA" = "true" ]; then \ dnf install -y libuuid-devel && \ mkdir -p /opt/ddn/red && \ - cp -a /workspace/nixl/infinia-libs/. /opt/ddn/red/; \ + if [ -z "$(ls -A /workspace/nixl/infinia-libs 2>/dev/null)" ]; then \ + echo "ERROR: infinia-libs staging directory is empty" >&2 && exit 1; \ + fi && \ + cp -a /workspace/nixl/infinia-libs/. /opt/ddn/red/; \ fi🤖 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 `@contrib/Dockerfile.manylinux` around lines 375 - 381, Update the BUILD_INFINIA block to validate that /workspace/nixl/infinia-libs contains at least one staged file before running the cp command. Fail the Docker build when the directory is missing or empty, while preserving the existing package installation and destination setup for valid stages.
🤖 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 `@contrib/build-container.sh`:
- Around line 314-317: Move the INFINIA_LIBS_DIR initialization, removal,
creation, and EXIT trap into the existing BUILD_INFINIA=true guard in
contrib/build-container.sh. Ensure non-Infinia builds do not touch the staging
directory or install this trap, while preserving the current setup and cleanup
behavior for Infinia builds.
---
Duplicate comments:
In `@contrib/Dockerfile.manylinux`:
- Around line 375-381: Update the BUILD_INFINIA block to validate that
/workspace/nixl/infinia-libs contains at least one staged file before running
the cp command. Fail the Docker build when the directory is missing or empty,
while preserving the existing package installation and destination setup for
valid stages.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 554a7b07-6a52-4f48-b0d6-f6093f5c9122
📒 Files selected for processing (9)
.ci/jenkins/lib/build-matrix.yaml.ci/jenkins/lib/build-wheel-matrix.yaml.ci/jenkins/lib/test-dl-ep-matrix.yaml.ci/jenkins/lib/test-dl-matrix.yaml.ci/jenkins/lib/test-matrix.yaml.ci/jenkins/lib/test-sanitizer-matrix.yaml.gitignorecontrib/Dockerfile.manylinuxcontrib/build-container.sh
|
🤖 CI Triage Agent — TL;DR: The pipeline failed in the "Allocate DL EP Environment" stage because Full analysisSummary: Stage "Allocate DL EP Environment" (node 245) failed after ~60 min with Root cause: The GB200 Slurm partition ( Implicated commit: unknown — not a code regression; infrastructure/scheduler capacity. File: N/A (CI/Slurm allocation step in the Suggested fix: Re-run the build once GB200 nodes are available; this is a transient capacity failure. To reduce false failures: (1) add automatic retry/backoff on the Related: none found (no code issue to search against). |
Mirror the UCX spcx wiring for the Infinia DDN plugin (#1941): a BUILD_INFINIA bool (default off) plus an optional INFINIA_IMAGE override, passed through to build-container.sh --build-infinia only when enabled. Off by default keeps nightly and poller-triggered builds unchanged.
NirWolfer
force-pushed
the
infinia-wheel-build
branch
from
69f858c to
0d144d4
Compare
|
/build |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@contrib/build-container.sh`:
- Around line 303-334: Validate that staged Infinia libraries are non-empty
before continuing: in contrib/build-container.sh lines 303-334, check
INFINIA_LIBS_DIR immediately after docker cp and fail via error with the
architecture and image details when empty; in contrib/Dockerfile.manylinux lines
375-381, add a guard around the BUILD_INFINIA copy block that fails when
/workspace/nixl/infinia-libs is empty before copying.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: b83692b6-1d3e-411e-a2fd-91f24a74e638
📒 Files selected for processing (9)
.ci/jenkins/lib/build-matrix.yaml.ci/jenkins/lib/build-wheel-matrix.yaml.ci/jenkins/lib/test-dl-ep-matrix.yaml.ci/jenkins/lib/test-dl-matrix.yaml.ci/jenkins/lib/test-matrix.yaml.ci/jenkins/lib/test-sanitizer-matrix.yaml.gitignorecontrib/Dockerfile.manylinuxcontrib/build-container.sh
NirWolfer
force-pushed
the
infinia-wheel-build
branch
from
0d144d4 to
f4d4d6a
Compare
|
/build |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@contrib/Dockerfile.manylinux`:
- Around line 381-383: Update the wheel-stage command sequence near the
libuuid-devel installation to run dnf clean all and remove /var/cache/dnf after
the conditional install, before the staged libraries are copied into
/opt/ddn/red.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: febcad87-cbdb-4c90-bc81-5e917f4e1135
📒 Files selected for processing (9)
.ci/jenkins/lib/build-matrix.yaml.ci/jenkins/lib/build-wheel-matrix.yaml.ci/jenkins/lib/test-dl-ep-matrix.yaml.ci/jenkins/lib/test-dl-matrix.yaml.ci/jenkins/lib/test-matrix.yaml.ci/jenkins/lib/test-sanitizer-matrix.yaml.gitignorecontrib/Dockerfile.manylinuxcontrib/build-container.sh
Alexey-Rivkin
left a comment
Alexey-Rivkin
left a comment
There was a problem hiding this comment.
Pls update the PR body
|
🤖 CI Triage Agent — TL;DR: Both the vLLM (#593) and SGLang (#552) sanity stages failed at the model-prefetch step because HuggingFace returned HTTP 429 "Too Many Requests" (rate-limited the CI egress IP 216.228.125.129) — an unauthenticated Full analysisSummary: The Root cause: In Implicated commit: unknown — not caused by a code change; the failure is external HF rate-limiting. The prefetch logic lives in File: Suggested fix:
Related: none found. |
| COPY . /workspace/nixl | ||
| WORKDIR /workspace/nixl | ||
|
|
||
| # Opt-in: place the pre-staged DDN Infinia libs (from infinia-libs/ in the build |
There was a problem hiding this comment.
This seems to diverge from https://github.com/ai-dynamo/nixl/pull/1832/changes where libred_client seems to need liburing.so.2 and libldap.so.2 plus jansson/libjose/xxhash from @nv-nmailhot
Also how does the rest of the build use this library if there is no LD_LIBRARY_PATH adjustment?
There was a problem hiding this comment.
What is the difference here? the only dep that was failing the build was libuuid which i added to the installation
Local verification shows that the infinia so file lands in the whl at the end of the build
…wheel Add an opt-in --build-infinia flag to contrib/build-container.sh that pulls the DDN Infinia libs from harbor.mellanox.com (reachable only from the internal network) on the host, stages them flat into the build context, and passes BUILD_INFINIA=true to the manylinux wheel build. Dockerfile.manylinux installs libuuid-devel and copies the staged libs to /opt/ddn/red before meson runs so libplugin_INFINIA.so is built into the wheel; libred_client/libred_async remain excluded by auditwheel. Both sides fail fast if the extraction is empty (wrong arch / misconfigured image) rather than silently shipping a wheel without the plugin. No harbor reference exists in the Dockerfile, so external/default builds (BUILD_INFINIA=false) are byte-for-byte unaffected: no filesystem writes, no trap, no plugin. CI wiring is intentionally deferred; when added it will fold into the normal Build Wheel step rather than run as a separate step. CI_IMAGE_TAG is now auto-derived by cidemo-init.sh from the CI-file commit SHA, so the Dockerfile.manylinux change needs no manual matrix-YAML tag bump. Signed-off-by: NirWolfer <nwolfer@nvidia.com>
NirWolfer
force-pushed
the
infinia-wheel-build
branch
from
f4d4d6a to
f43b5e2
Compare
|
/build |
|
🤖 CI Triage Agent — TL;DR: All Docker-image build stages failed because Full analysisSummary: Every "Compiling NIXL Docker Image for DL" stage (nodes 220, 210) and the ucx build stages (176, 177) failed at Dockerfile STEP 15/17 during Root cause: apt could not connect to the Ubuntu ports mirror: Implicated commit: None — this is infrastructure, not a code change. The failing File: Suggested fix: Retry the build once the CI network/egress to Related: none |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Stage 621 "Build sanity image" (building Root cause: The Ubuntu Implicated commit: b594eb0 (NirWolfer, "ci: add vLLM/SGLang NIXL disaggregation sanity testing", #1777) added the File: contrib/Dockerfile.vllm:22-25 Suggested fix: This is a transient mirror-connectivity failure — retry the build first. To harden against recurrence: (1) point Related: PR #1777 (introduced the apt step); none found for the mirror outage specifically. |
|
🤖 CI Triage Agent — TL;DR: The "Run CPP tests" stage failed because UCX could not initialize CUDA on test node Full analysisSummary: Both parallel "Run CPP tests" stages (nodes 272 ucx-master and 294 ucx-v1.22.x) exited with code 42 with 67–69 UCX gtests reported failed, and one test genuinely FAILED. Root cause: On node Implicated commit: unknown (not a code regression; environmental CUDA-init failure on the test host). The warning path in File: Suggested fix: Treat this as a node/infra issue: (1) verify the CUDA driver/ Related: none |
|
🤖 CI Triage Agent — TL;DR: The Docker image build failed because Full analysisSummary: Stages "build_helper_dl/aarch64/ucx-master" (124) and "Compiling NIXL Docker Image for DL" (150) failed at Root cause: During Implicated commit: none — not caused by 97b9bb6 (PR #1941); this is an environmental/infra failure. File: Suggested fix: No code change required — re-run the build; it is already green on retry (stage 168). To reduce future flakiness, harden the apt step against transient mirror outages: force IPv4 and add retries, e.g. Related: none |
|
🤖 CI Triage Agent — TL;DR: The wheel built fine; the "Build sanity image" stage failed because Full analysisSummary: Stage 504 "Build sanity image" ( Root cause: During the Docker build, apt could not fetch the Ubuntu package indices from Implicated commit: Not a code regression. The File: Suggested fix:
Related: PR #1941 (this build); apt step added in PR #1777. No existing issue found for the ports.ubuntu.com connectivity failure. |
4 participants
What?
Add an opt-in --build-infinia flag to contrib/build-container.sh that pulls the DDN Infinia libs from harbor.mellanox.com (reachable only from the internal network) on the host, stages them flat into the build context, and passes BUILD_INFINIA=true to the manylinux wheel build. Dockerfile.manylinux copies the staged libs to /opt/ddn/red before meson runs so libplugin_INFINIA.so is built into the wheel; libred_client/libred_async remain excluded by auditwheel. No harbor reference exists in the Dockerfile, so external/default builds (BUILD_INFINIA=false) are byte-for-byte unaffected
Why?
needed for the release
How?
It is optional, but for complex PRs, please provide information about the design,
architecture, approach, etc.
Summary by CodeRabbit