ci: pin wheel install-test deps to uv.lock (supply-chain) - #649
Conversation
|
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:
📝 WalkthroughWalkthroughChangesDeterministic wheel dependency installation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 @.github/workflows/ci.yaml:
- Around line 1092-1095: Update the uv export command in the wheel smoke-test
setup to include the services extra, using --all-extras or the targeted services
extra option. Keep the subsequent uv tool install of "${WHEEL}[services]"
unchanged so the generated constraints cover the same dependencies being
installed.
🪄 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: Enterprise
Run ID: 78feab1c-d2b5-4a6d-83eb-a19b3a1b795c
📒 Files selected for processing (1)
.github/workflows/ci.yaml
crookedstorm
left a comment
There was a problem hiding this comment.
Looks like an improvement to me.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
script/compile-wheel-locks.sh (1)
33-36: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGuard against multiple wheel matches.
head -1silently picks an arbitrary wheel ifWHEEL_DIRhas more than one match (stale build, leftover artifact). Only the "zero matches" case is guarded. A wrong pick means the committed lock is compiled from the wrong wheel's metadata.♻️ Add multiplicity check
np_wheel="$(find "${WHEEL_DIR}" -name 'nemo_platform-*.whl' | head -1)" pl_wheel="$(find "${WHEEL_DIR}" -name 'nemo_platform_plugin-*.whl' | head -1)" +np_count="$(find "${WHEEL_DIR}" -name 'nemo_platform-*.whl' | wc -l)" +pl_count="$(find "${WHEEL_DIR}" -name 'nemo_platform_plugin-*.whl' | wc -l)" +[[ "${np_count}" -le 1 ]] || { echo "multiple nemo_platform-*.whl in ${WHEEL_DIR}" >&2; exit 1; } +[[ "${pl_count}" -le 1 ]] || { echo "multiple nemo_platform_plugin-*.whl in ${WHEEL_DIR}" >&2; exit 1; } [[ -n "${np_wheel}" ]] || { echo "no nemo_platform-*.whl in ${WHEEL_DIR}" >&2; exit 1; }🤖 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 `@script/compile-wheel-locks.sh` around lines 33 - 36, Update the wheel discovery checks around np_wheel and pl_wheel to validate that each pattern matches exactly one file, rather than selecting the first result with head -1. Fail with an appropriate error when either wheel has zero or multiple matches, and only proceed to compile locks when both matches are unique..github/workflows/ci.yaml (1)
1090-1103: 🧹 Nitpick | 🔵 TrivialConsider a lock-freshness check.
--no-depsinstall trusts the committed lock matches the wheel's currentrequires-dist. Ifpyproject.tomldeps change without re-runningcompile-wheel-locks.sh, the smoke test won't catch missing/stale deps until a runtime failure. A CI check diffing wheel metadataRequires-Distagainst the lock's non-hash package names would catch drift early.Also applies to: 1119-1126
🤖 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 @.github/workflows/ci.yaml around lines 1090 - 1103, The wheel smoke-test workflow installs dependencies from a committed lock without verifying it matches the wheel metadata. Add a lock-freshness check before the --no-deps install in the wheel validation steps, extracting the wheel’s Requires-Dist entries and comparing normalized package names against the lock’s non-hash package names; fail CI on missing or stale dependencies, while preserving the existing pinned installation flow.
🤖 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.
Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 1090-1103: The wheel smoke-test workflow installs dependencies
from a committed lock without verifying it matches the wheel metadata. Add a
lock-freshness check before the --no-deps install in the wheel validation steps,
extracting the wheel’s Requires-Dist entries and comparing normalized package
names against the lock’s non-hash package names; fail CI on missing or stale
dependencies, while preserving the existing pinned installation flow.
In `@script/compile-wheel-locks.sh`:
- Around line 33-36: Update the wheel discovery checks around np_wheel and
pl_wheel to validate that each pattern matches exactly one file, rather than
selecting the first result with head -1. Fail with an appropriate error when
either wheel has zero or multiple matches, and only proceed to compile locks
when both matches are unique.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1a6b9a69-4037-4113-8724-b1c2ff409f75
📒 Files selected for processing (4)
.github/wheel-locks/nemo-platform-plugin.txt.github/wheel-locks/nemo-platform-services.txt.github/workflows/ci.yamlscript/compile-wheel-locks.sh
The wheel install-smoke-test installed the freshly built nemo-platform / nemo-platform-plugin wheels by resolving their whole dependency tree fresh from PyPI (latest-in-range) — non-reproducible and a supply-chain risk (the job then executes the installed code in the runner). It also pulled litellm 1.92.0, whose native/PyO3 build has no Python 3.14 wheel, breaking the py3.14 matrix. Pinning to uv.lock is not possible here: the built vendored-SDK wheel's closure requires newer deps than the workspace lock (e.g. pydantic>=2.13.3 vs the locked 2.12.5), and a full lock also pins packages that lack py3.14 wheels. Instead, partial-pin each wheel's DIRECT external deps to a committed constraints file (.github/wheel-constraints/, regenerated by script/compile-wheel-constraints.sh) and cap the litellm transitive <1.92; deep transitives resolve normally so they stay py3.14-compatible and cannot self-conflict. Install both wheels with `uv pip install --constraint` in a venv. Drop the litellm cap once litellm ships a Python 3.14 wheel. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
0a28805 to
d7c6062
Compare
Problem
The wheel install-smoke-test jobs (
nemo-platform wheel build + test,nemo-platform-plugin wheel build + test) install the freshly-built wheel with no dependency pinning:Neither uses
--constraints,uv.lock,--frozen, or hashes, so they resolve the entire transitive closure fresh from PyPI, latest-in-range. That's:This is what let
litellm==1.92.0slip in over the locked1.90.2and turn the py3.14 job red (1.92.0 added a Rust/PyO3 core that fails to build on Python 3.14). (see this CI link)Fix
Export the locked dependency set from
uv.lockand pass it as--constraintsto both installs:--all-packagespins the full external dependency universe of the workspace.--no-emit-workspace --no-emit-localdrop the local/workspace path & editable entries (which aren't installable as constraints), leaving only PyPI-pinned versions.uv.lock; a newer (potentially malicious) in-range release can no longer be pulled.Side effect: this also fixes the py3.14 wheel+test failure — the constraints force
litellm==1.90.2(pure-Python wheel, installs cleanly on 3.14).Verification
uv pip compilewith the exported constraints resolveslitellm==1.90.2; without it,1.92.0.[services]extra has no heavy/native deps (no torch/vllm/flashinfer), so pinning to the lock is safe across the Python matrix.Run actionlintin CI covers the shell.Follow-up (not in this PR)
Hash-pinning (
--require-hashes) is stronger defense-in-depth but needs the local wheel hashed too; version-pinning here closes the practical "auto-pull a newer release" vector first.🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit