-
Notifications
You must be signed in to change notification settings - Fork 492
Update the build_wheel CI script to test that built wheels are installable #1457
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update the build_wheel CI script to test that built wheels are installable #1457
Conversation
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…olkit into david-post-build-wheel-test Signed-off-by: David Gardner <dagardner@nvidia.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds CI tooling to install and test multiple Python versions (3.11–3.13) per job, expands wheel build script to move and test wheels across those Python versions using per-version venvs, and pins Changes
Sequence DiagramsequenceDiagram
participant BuildScript as Build Script
participant PythonMgr as UV Python Manager
participant WheelDir as Wheels Directory
participant VEnv as VirtualEnv/Test Runner
BuildScript->>BuildScript: Build wheels
BuildScript->>WheelDir: Move wheels to WHEELS_BASE_DIR
BuildScript->>PythonMgr: install_python_versions()
PythonMgr->>PythonMgr: detect current Python (major.minor)
PythonMgr->>PythonMgr: for each supported version: uv python find --managed-python
PythonMgr->>PythonMgr: if missing -> UV_PYTHON_DOWNLOADS="manual" uv python install --managed-python <version>
BuildScript->>BuildScript: for each wheel
BuildScript->>BuildScript: for each supported Python version
BuildScript->>VEnv: create uv venv for version
BuildScript->>VEnv: install wheel from WheelDir
VEnv->>VEnv: python -c "import nat"
VEnv->>VEnv: run nat --version
VEnv-->>BuildScript: return success or error
BuildScript->>VEnv: cleanup venv and temp dir
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ci/scripts/github/build_wheel.sh`:
- Around line 99-103: Adjust the indentation of the conditional block that
checks IMPORT_TEST_RESULT to match the surrounding 8-space indentation style:
locate the if/then/fi block referencing IMPORT_TEST_RESULT, rapids-logger, echo
"${PYTHON_IMPORT_OUT}", and exit ${IMPORT_TEST_RESULT} (the lines using
variables whl and pyver) and change the current 7-space indent to 8 spaces so
the block aligns visually with the surrounding code.
In `@packages/nvidia_nat_weave/pyproject.toml`:
- Around line 30-33: The dependencies list in pyproject.toml contains a syntax
error: add a missing comma after the dependency string "blis~=1.3" so it is
properly separated from the next entry "fickling>=0.1.7,<1.0.0"; update the
dependencies array to include that comma and re-parse the TOML to confirm it
loads correctly.
🧹 Nitpick comments (1)
ci/scripts/common.sh (1)
227-251: LGTM! The function correctly provisions multiple Python versions.The logic for checking existing installations and conditionally downloading is well-structured. The
--managed-pythonflag usage is appropriately documented.Minor shellcheck note (SC2155): Line 229 combines declaration and assignment which can mask return values. This is low risk here since
awkis unlikely to fail on valid input, but separating them is a defensive practice:local current_python_version current_python_version=$(echo ${PYTHON_VERSION} | awk '{split($0, a, "."); print a[1]"."a[2]}')
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
mnajafian-nv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with minor fixes suggested inline
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Co-authored-by: mnajafian-nv <mnajafian@nvidia.com> Signed-off-by: David Gardner <96306125+dagardner-nv@users.noreply.github.com>
…/NeMo-Agent-Toolkit into david-post-build-wheel-test Signed-off-by: David Gardner <dagardner@nvidia.com>
Co-authored-by: mnajafian-nv <mnajafian@nvidia.com> Signed-off-by: David Gardner <96306125+dagardner-nv@users.noreply.github.com>
…/NeMo-Agent-Toolkit into david-post-build-wheel-test Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ci/scripts/github/build_wheel.sh`:
- Around line 56-62: When flattening wheels the current loop (variables
BUILT_WHEELS, MOVED_WHEELS, iterating for whl and computing dest_wheel_name then
mv) can silently overwrite if two wheels share the same basename; modify the
loop to check if dest_wheel_name already exists before moving, and if it does
log a clear error and exit non-zero (fail fast) instead of overwriting; on
success continue to mv and append to MOVED_WHEELS as before.
♻️ Duplicate comments (1)
ci/scripts/github/build_wheel.sh (1)
65-119: Ensure temp venv cleanup on failures (trap-based cleanup).
If a test fails mid-loop,TEMP_INSTALL_LOCATIONcan linger; a trap keeps CI clean.🧹 Suggested cleanup hook
TEMP_INSTALL_LOCATION="${WORKSPACE_TMP}/wheel_test_env" install_python_versions + +cleanup_test_env() { + if [[ -n "${TEMP_INSTALL_LOCATION}" && -d "${TEMP_INSTALL_LOCATION}" ]]; then + deactivate 2>/dev/null || true + rm -rf "${TEMP_INSTALL_LOCATION}" 2>/dev/null || true + fi +} +trap cleanup_test_env EXIT
🧹 Nitpick comments (1)
ci/scripts/common.sh (1)
227-251: Avoid masking return status when initializingcurrent_python_version.
Shellcheck SC2155 recommends declaring and assigning separately; also makes the flow clearer. Line 229.♻️ Proposed tweak
- local current_python_version=$(echo ${PYTHON_VERSION} | awk '{split($0, a, "."); print a[1]"."a[2]}') + local current_python_version + current_python_version=$(echo "${PYTHON_VERSION}" | awk '{split($0, a, "."); print a[1]"."a[2]}')
…olkit into david-post-build-wheel-test Signed-off-by: David Gardner <dagardner@nvidia.com>
|
/merge |
Description
By Submitting this PR I confirm:
Summary by CodeRabbit
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.