ci: split Dockerfile.manylinux into wheel_base/wheel stages to cache deps - #1870
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. 🚀 |
54ac0af to
4ea9a09
Compare
4ea9a09 to
2e3495e
Compare
2e3495e to
895bcc1
Compare
895bcc1 to
00000f5
Compare
|
👀 Investigating |
00000f5 to
aae135d
Compare
aae135d to
5f975f2
Compare
5f975f2 to
2946f7d
Compare
|
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:
📝 WalkthroughWalkthroughThis PR adds a cached ChangesWheel base image support
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/build |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.ci/docs/build-wheel-matrix-ci.md (1)
46-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale Manylinux axis from this section.
manylinux_2_28is fixed in.ci/jenkins/lib/build-wheel-matrix.yaml, so the docs should list only the Python and architecture axes here.🤖 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 @.ci/docs/build-wheel-matrix-ci.md around lines 46 - 58, The Matrix Axes section still lists Manylinux Versions even though manylinux_2_28 is fixed in build-wheel-matrix.yaml. Update this documentation block to describe only the Python Versions and Architectures axes, and remove the stale Manylinux axis from the build-wheel-matrix-ci.md section.
🤖 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/cidemo-init.sh:
- Around line 56-62: The wheel_dockerfile_changed detection repeats the same
three-way git diff pattern already used for files_changed, so factor that logic
into a shared helper and reuse it here. Add or extend a helper in cidemo-init.sh
that checks HEAD~1..HEAD, working tree, and staged changes for a given path,
then call it for WHEEL_DOCKERFILE instead of duplicating the grep chain. Keep
the existing wheel_dockerfile_changed flag assignment and echo behavior, but
make the change through the shared helper so future file-gating updates stay
consistent.
In `@contrib/build-container.sh`:
- Around line 168-175: The option names in build-container.sh are too easy to
mix up because --wheel-base and --wheel-base-image control different values in
separate argument cases. Rename the manylinux tag option handled alongside
WHL_BASE to a more distinct name such as --wheel-platform, and update the
related parser branches and any callers that reference the current name so the
argument handling around WHEEL_BASE_IMAGE and WHL_BASE is unambiguous.
---
Outside diff comments:
In @.ci/docs/build-wheel-matrix-ci.md:
- Around line 46-58: The Matrix Axes section still lists Manylinux Versions even
though manylinux_2_28 is fixed in build-wheel-matrix.yaml. Update this
documentation block to describe only the Python Versions and Architectures axes,
and remove the stale Manylinux axis from the build-wheel-matrix-ci.md section.
🪄 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: c641ee91-2db0-4771-b293-209365ebd236
📒 Files selected for processing (6)
.ci/cidemo-init.sh.ci/docs/build-wheel-matrix-ci.md.ci/docs/ci-overview.md.ci/jenkins/lib/build-wheel-matrix.yamlcontrib/Dockerfile.manylinuxcontrib/build-container.sh
|
/ok to test a42637a |
Split Dockerfile.manylinux into two build stages to avoid rebuilding all dependencies on every PR: - wheel_base: all dep compilation (hwloc, OpenSSL, Abseil, gRPC, AWS/Azure SDKs, Rust, DOCA, libfabric, UCX, Python env). Built and pushed to Artifactory by ci-demo when the Dockerfile changes. - wheel: starts FROM wheel_base and only runs the NIXL build + wheel creation steps. In CI, build-container.sh accepts --wheel-base-image which sets --build-arg wheel_base=<url> and --target wheel, pulling the cached deps image instead of rebuilding from scratch. Local docker builds are unaffected: the wheel_base ARG defaults to the local stage name so docker build ./contrib/Dockerfile.manylinux builds the full image as before. The nightly is also untouched as it does not pass --wheel-base-image. Also: - Removes the manylinux matrix axis from build-wheel-matrix.yaml (was single-valued 2_28, now hardcoded where needed). - Adds contrib/Dockerfile.manylinux to cidemo-init.sh CI_FILES and build-wheel-matrix.yaml to the CI_IMAGE_TAG enforcement, so changes to the wheel deps image require bumping tags in all matrix files. - Bumps CI_IMAGE_TAG to 20260701-1 across all matrix YAML files. Signed-off-by: Nir Wolfson <nwolfer@nvidia.com> Signed-off-by: NirWolfer <nwolfer@nvidia.com>
a42637a to
2a918b5
Compare
|
/build |
## What?
Rename the local `BUILD_TARGET` in `contrib/build-container.sh` to
`DOCKER_BUILD_TARGET`.
## Why?
The container job exports a `BUILD_TARGET` param (nixl/nixlbench), which
leaked into the `docker build` line via `${BUILD_TARGET:-}` as a stray
positional arg. Every nixl-target run has failed since #1870 with
`accepts at most 1 arg(s), received 2`.
## How?
Rename the local var so it no longer collides with the param. The
`--wheel-base-image` path still passes `--target wheel`.
### Note
This highlights the need for the per-PR container check
#1863
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved container builds when a wheel base image is configured.
* Prevented build parameters from being unintentionally passed to
Docker, resulting in more reliable build behavior.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
…deps (ai-dynamo#1870) ## What? Add a wheel_base Docker build target covering all dependency compilation (hwloc, OpenSSL, Abseil, gRPC, AWS/Azure SDKs, Rust, DOCA, libfabric, UCX, etc.). The wheel stage starts FROM wheel_base and only runs the NIXL build and wheel creation steps. In CI the wheel_base image is built and pushed to Artifactory by ci-demo when Dockerfile.manylinux changes (via the new runs_on_dockers entry). The build-wheel pipeline then passes --wheel-base-image to build-container.sh which pulls the cached image and runs --target wheel, skipping the expensive dep compilation on every PR. Local docker builds are unaffected: wheel_base ARG defaults to the local stage name so docker build ./contrib/Dockerfile.manylinux builds the full image as before. Also removes the now-redundant manylinux matrix axis (was single-valued 2_28, hardcoded in the image name and wheel-base arg), and wires cidemo-init.sh to enforce a CI_IMAGE_TAG bump in build-wheel-matrix.yaml whenever Dockerfile.manylinux changes. ## Why? Lower nixl-ci-build-wheel pipeline times, and to justify adding ai-dynamo#1777 later on which adds time to this pipeline <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Wheel builds now use a single cached wheel-base (manylinux_2_28) and support selecting a prebuilt dependencies image, alongside optional Torch version selection. * The wheel build matrix is simplified to vary only by Python version and architecture for more consistent builds. * **Bug Fixes** * CI validation now more reliably enforces required image tag updates for both standard CI changes and wheel Dockerfile changes, with clearer guidance when updates are missing. * **Documentation** * Updated CI docs to reflect the new cached wheel-base flow and how to refresh it via the wheel cache image tag. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Nir Wolfson <nwolfer@nvidia.com> Signed-off-by: NirWolfer <nwolfer@nvidia.com>
What?
Add a wheel_base Docker build target covering all dependency compilation (hwloc, OpenSSL, Abseil, gRPC, AWS/Azure SDKs, Rust, DOCA, libfabric, UCX, etc.). The wheel stage starts FROM wheel_base and only runs the NIXL build and wheel creation steps.
In CI the wheel_base image is built and pushed to Artifactory by ci-demo when Dockerfile.manylinux changes (via the new runs_on_dockers entry). The build-wheel pipeline then passes --wheel-base-image to build-container.sh which pulls the cached image and runs --target wheel, skipping the expensive dep compilation on every PR.
Local docker builds are unaffected: wheel_base ARG defaults to the local stage name so docker build ./contrib/Dockerfile.manylinux builds the full image as before.
Also removes the now-redundant manylinux matrix axis (was single-valued 2_28, hardcoded in the image name and wheel-base arg), and wires cidemo-init.sh to enforce a CI_IMAGE_TAG bump in build-wheel-matrix.yaml whenever Dockerfile.manylinux changes.
Why?
Lower nixl-ci-build-wheel pipeline times, and to justify adding #1777 later on which adds time to this pipeline
Summary by CodeRabbit