refactor: generate lock files with hashes for midstream and downstream builds - #484
Conversation
…m builds Replace the unpinned requirements.txt with two lock files produced by `uv pip compile --generate-hashes`: one for midstream (PyPI, with --torch-backend=cpu) and one for downstream (RHAI index, for Konflux hermetic builds). This ensures reproducible installs with hash verification on both build paths. Key changes: - build.py generates requirements-lock.txt (midstream) and requirements-lock-konflux.txt (downstream) based on build.env config - Use UV_TORCH_BACKEND=cpu + UV_CONFIG_FILE=/dev/null in Containerfile to avoid multi-index hash incompleteness (uv#14000) - Add build/run.sh wrapper to run build.py inside the base image container for consistent Linux-native resolution - Move pymilvus/milvus-lite constraints to distribution/constraints.txt - Update CI workflows to use the new lock files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedAn error occurred during the review process. Please try again later. 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 replaces the old Changes
Estimated code review effort: 4 (Complex) | ~60 minutes Related issues: None referenced. Related PRs: None referenced. Suggested labels: supply-chain, ci-cd, dependencies, security-review Suggested reviewers: build and container maintainers Poem: 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
build/build.py (1)
65-88: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSource installs still need branch refs (CWE-20) —
.github/workflows/test-upstream-in-showroom.ymlpassesOGX_VERSION: mainintobuild/run.sh, butbuild/build.py:65-88now queriesgit ls-remote --tagsonly, so that path will fail to resolve. Either include heads here or stop feeding branch refs into source-install mode.🤖 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 `@build/build.py` around lines 65 - 88, The ref resolver in _resolve_ref_to_sha only checks tags, so branch refs like main will not resolve during source installs. Update _resolve_ref_to_sha to query both tags and heads (or otherwise handle branch refs) before returning the SHA, and keep the annotated-tag dereference behavior intact. Make sure the change still works for callers such as build/run.sh that may pass OGX_VERSION as a branch name..github/workflows/test-upstream-in-showroom.yml (1)
37-49: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass
OGX_VERSIONintobuild/run.sh.podman rundrops the step env, sobuild/build.pyfalls back tobuild/build.env(OGX_VERSION=v1.1.3+rhaiv.0) instead ofmain. Add-e OGX_VERSIONor an env file, or this workflow builds the wrong ref. (CWE-20)🤖 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/test-upstream-in-showroom.yml around lines 37 - 49, The Build image step is relying on a workflow env var that is not reaching build/run.sh, so build/build.py falls back to build/build.env instead of using main. Update the Build image command to explicitly pass OGX_VERSION into the build/run.sh invocation (or an equivalent env-file mechanism) and keep the existing OGX_VERSION assignment in the workflow so the build uses the intended ref; use the build/run.sh and build/build.py flow as the key places to verify the variable is propagated..github/workflows/redhat-distro-container.yml (1)
114-118: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winForward
OGX_VERSIONintobuild/run.sh../build/run.shstartsbuild/build.pywithout-e, so the step-levelOGX_VERSION: ${{ env.OGX_COMMIT_SHA }}never reachesos.getenv("OGX_VERSION"). The scheduled/manual path will silently fall back tobuild.envand build the wrong ogx revision.🤖 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/redhat-distro-container.yml around lines 114 - 118, The scheduled/manual workflow step sets OGX_VERSION, but build/run.sh does not pass that environment variable through to build/build.py, so the arbitrary commit override is lost. Update build/run.sh so it forwards OGX_VERSION into the Python build invocation (for example by exporting/passing the env through), and ensure the workflow step that uses OGX_COMMIT_SHA continues to reach the code path in build/build.py that reads os.getenv("OGX_VERSION").
🧹 Nitpick comments (2)
build/build.py (1)
174-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
uv venvcreation bypasses the shared_run()error-reporting helper.The venv-creation
subprocess.runcall (Lines 177-182) doesn't go through_run(), so a failure here won't print stdout/stderr like every other invocation in this file, making failures harder to diagnose in CI logs.♻️ Proposed fix for consistent error diagnostics
- subprocess.run( - ["uv", "venv", "--python", sys.executable, str(venv_path)], - check=True, - capture_output=True, - text=True, - ) + _run(["uv", "venv", "--python", sys.executable, str(venv_path)])🤖 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 `@build/build.py` around lines 174 - 202, The `uv venv` creation in `build/build.py` bypasses the shared `_run()` helper, so failures won’t include stdout/stderr like the rest of the build commands. Update the venv setup inside the temporary-directory block to use `_run()` (or apply the same error-reporting behavior as `_run`) for the `uv venv` invocation, keeping the existing `build` flow and `cmd` construction intact.build/run.sh (1)
18-22: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUnpinned
ruamel.yamlfetched fresh on every container run.
uv run --with ruamel.yamlresolves and installs the latestruamel.yamlfrom the network on every invocation, with no version pin or hash verification — undermining the reproducibility this PR otherwise aims for.♻️ Pin the version
- uv run --with ruamel.yaml build/build.py + uv run --with ruamel.yaml==0.18.10 build/build.py🤖 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 `@build/run.sh` around lines 18 - 22, The build/run.sh container invocation is pulling ruamel.yaml without a fixed version, so update the uv run --with ruamel.yaml usage to a pinned, reproducible dependency reference. Use the existing runtime command path in the run invocation and replace the unversioned package spec with a version-pinned form (ideally with hash verification if supported) so the build is deterministic across runs.
🤖 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 @.pre-commit-config.yaml:
- Around line 47-53: The pkg-gen hook in the pre-commit configuration is too
mandatory because always_run makes ./build/run.sh execute on every commit and
block contributors without podman or docker. Update the pre-commit entry for
pkg-gen so it is optional or removed from local hooks, and move the Distribution
Build step to CI instead; keep the hook settings around pass_filenames and
require_serial aligned with the new behavior. Use the pkg-gen hook entry in
.pre-commit-config and the ./build/run.sh script reference to locate the change.
In `@build/build.py`:
- Around line 374-378: The temporary requirements file creation in
_write_temp_requirements is unsafe because it uses a fixed name under
tempfile.gettempdir() and write_text can follow a pre-existing symlink. Change
this helper to create a unique secure temp file using mkstemp() or
NamedTemporaryFile(delete=False), write the contents through that handle, and
ensure the caller around _compile_lockfile() removes the file afterward in a
cleanup step.
In `@build/run.sh`:
- Line 6: The build image reference in run.sh is mutable, so pin the IMAGE value
to a specific digest instead of using the latest tag. Update the IMAGE
assignment in run.sh to the digest-pinned
quay.io/opendatahub/odh-midstream-python-base-3-12 reference, and make sure
Containerfile and Containerfile.in use the same pinned image if they participate
in the same build path.
---
Outside diff comments:
In @.github/workflows/redhat-distro-container.yml:
- Around line 114-118: The scheduled/manual workflow step sets OGX_VERSION, but
build/run.sh does not pass that environment variable through to build/build.py,
so the arbitrary commit override is lost. Update build/run.sh so it forwards
OGX_VERSION into the Python build invocation (for example by exporting/passing
the env through), and ensure the workflow step that uses OGX_COMMIT_SHA
continues to reach the code path in build/build.py that reads
os.getenv("OGX_VERSION").
In @.github/workflows/test-upstream-in-showroom.yml:
- Around line 37-49: The Build image step is relying on a workflow env var that
is not reaching build/run.sh, so build/build.py falls back to build/build.env
instead of using main. Update the Build image command to explicitly pass
OGX_VERSION into the build/run.sh invocation (or an equivalent env-file
mechanism) and keep the existing OGX_VERSION assignment in the workflow so the
build uses the intended ref; use the build/run.sh and build/build.py flow as the
key places to verify the variable is propagated.
In `@build/build.py`:
- Around line 65-88: The ref resolver in _resolve_ref_to_sha only checks tags,
so branch refs like main will not resolve during source installs. Update
_resolve_ref_to_sha to query both tags and heads (or otherwise handle branch
refs) before returning the SHA, and keep the annotated-tag dereference behavior
intact. Make sure the change still works for callers such as build/run.sh that
may pass OGX_VERSION as a branch name.
---
Nitpick comments:
In `@build/build.py`:
- Around line 174-202: The `uv venv` creation in `build/build.py` bypasses the
shared `_run()` helper, so failures won’t include stdout/stderr like the rest of
the build commands. Update the venv setup inside the temporary-directory block
to use `_run()` (or apply the same error-reporting behavior as `_run`) for the
`uv venv` invocation, keeping the existing `build` flow and `cmd` construction
intact.
In `@build/run.sh`:
- Around line 18-22: The build/run.sh container invocation is pulling
ruamel.yaml without a fixed version, so update the uv run --with ruamel.yaml
usage to a pinned, reproducible dependency reference. Use the existing runtime
command path in the run invocation and replace the unversioned package spec with
a version-pinned form (ideally with hash verification if supported) so the build
is deterministic across runs.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2f6e89dd-4e2c-4dd2-a3de-71bfd0ea67c9
📒 Files selected for processing (13)
.github/workflows/create-or-update-release-branch.yml.github/workflows/redhat-distro-container.yml.github/workflows/test-pr-in-showroom.yml.github/workflows/test-upstream-in-showroom.yml.pre-commit-config.yamlContainerfileContainerfile.inbuild/build.envbuild/build.pybuild/run.shdistribution/constraints.txtdistribution/requirements-lock-konflux.txtdistribution/requirements-lock.txt
Replace hand-rolled _load_env parser and scattered os.getenv calls with a pydantic-settings BuildConfig class. This gives all three config variables (OGX_VERSION, OGX_INSTALL_FROM_SOURCE, RHAI_INDEX_URL) consistent env-var-over-file precedence, automatic type coercion, and validation at construction time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3eb76d8 to
f62db04
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 `@build/run.sh`:
- Around line 18-20: The current build permission handling in run.sh is too
broad because the chmod -R a+w on distribution and Containerfile makes those
paths world-writable. Update the build flow around the podman/docker run
invocation so the container uid matches the host uid with --userns=keep-id for
podman, removing the need for the chmod in that path; if the docker fallback
still needs writable outputs, replace the global write grant with a narrower
group-based approach tied to an explicit group instead of a+w.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c976e113-8f16-4ca8-be4b-3a93d7e442ae
📒 Files selected for processing (1)
build/run.sh
Use --userns=keep-id for podman and --user for docker instead of chmod a+w on build outputs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
This pull request has merge conflicts that must be resolved before it can be merged. @eoinfennessy please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
Lost during rebase; needed for nightly builds that pass branch names (e.g. main) as OGX_VERSION. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
build/run.sh (1)
21-26: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCI env-var overrides silently dropped inside the container.
exec "$runtime" run --rm "$user_flag" ...forwards no-e/--env-fileinto the container.build/build.py'sBuildConfig(pydantic-settings) readsogx_versionand other fields from process env orbuild/build.env. Workflows such as.github/workflows/test-upstream-in-showroom.ymlsetOGX_VERSIONat the step level before invoking./build/run.sh— but that variable never crosses the container boundary, sobuild.pywill always resolve tobuild.env's value, not the CI-supplied override. This silently breaks per-branch/nightly builds that depend on dynamicOGX_VERSION(orRHAI_INDEX_URL) values, and can hard-fail ifogx_versionhas no fallback default inbuild.env.This matches the still-open past review question ("Do we need to pass OGX_VERSION in here is provided (by CI job?)") which was never addressed in code.
🔧 Proposed fix: forward relevant env vars into the container
exec "$runtime" run --rm \ "$user_flag" \ + -e OGX_VERSION \ + -e OGX_INSTALL_FROM_SOURCE \ + -e RHAI_INDEX_URL \ -v "$REPO_ROOT:/workspace:z" \ -w /workspace \ "$IMAGE" \ uv run --with ruamel.yaml --with pydantic-settings build/build.py#!/bin/bash # Verify BuildConfig's required fields and whether build.env supplies defaults cat -n build/build.env rg -n 'ogx_version|OGX_VERSION|env_file|env_prefix' build/build.py🤖 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 `@build/run.sh` around lines 21 - 26, The container launch in run.sh is not forwarding CI-set environment overrides, so BuildConfig in build/build.py cannot see values like OGX_VERSION or RHAI_INDEX_URL from the host process. Update the exec "$runtime" run --rm invocation to pass through the relevant env vars into the container (or an env file) so pydantic-settings in BuildConfig can resolve overrides before falling back to build.env. Use the existing build/build.py and BuildConfig symbols as the reference point when wiring the env propagation.
🤖 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.
Duplicate comments:
In `@build/run.sh`:
- Around line 21-26: The container launch in run.sh is not forwarding CI-set
environment overrides, so BuildConfig in build/build.py cannot see values like
OGX_VERSION or RHAI_INDEX_URL from the host process. Update the exec "$runtime"
run --rm invocation to pass through the relevant env vars into the container (or
an env file) so pydantic-settings in BuildConfig can resolve overrides before
falling back to build.env. Use the existing build/build.py and BuildConfig
symbols as the reference point when wiring the env propagation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d67df4f1-f191-4123-9914-40c97b540ad7
📒 Files selected for processing (4)
.github/workflows/redhat-distro-container.ymlbuild/build.pybuild/run.shdistribution/requirements-lock.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/redhat-distro-container.yml
- build/build.py
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The merge from main left both install methods (lockfile + old requirements.txt with constraints). Consolidate into a single RUN layer that installs from the lockfile and swaps opencv-python for the headless variant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 `@build/run.sh`:
- Around line 21-27: The env forwarding loop in run.sh can skip the final entry
in build.env when the file has no trailing newline. Update the while read logic
that builds env_flags so the last KEY=VALUE line is still processed even on EOF
without newline, and keep the existing checks in the loop that reference key and
env_flags.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8dee30d9-4987-4c9d-8ec8-d8cf44593c8a
📒 Files selected for processing (4)
ContainerfileContainerfile.inbuild/run.shdistribution/requirements-lock.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- Containerfile.in
- Containerfile
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Tick the box to add this pull request to the merge queue (same as
|
3 participants
Summary
requirements.txtwith two lock files produced byuv pip compile --generate-hashes:requirements-lock.txt(midstream, PyPI) andrequirements-lock-konflux.txt(downstream, RHAI index for Konflux hermetic builds)UV_TORCH_BACKEND=cpu+UV_CONFIG_FILE=/dev/nullin Containerfile to avoid multi-index hash incompleteness (uv#14000)build/run.shwrapper to runbuild.pyinside the base image container for consistent Linux-native resolutionpymilvus/milvus-liteconstraints fromPINNED_DEPENDENCIESinbuild.pytodistribution/constraints.txtpydantic-settingsWhy
Downstream builds will use Hermeto for pre-fetching pip packages. Hermeto pip pre-fetch requires a fully resolved lockfile.
Test plan
uv run pre-commit run --all-filespasses cleanly (idempotent — second run produces no diff)podman build -t ogx-core .succeeds using the midstream lock file./build/run.shinstead ofuv run ... build/build.py🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit
build/run.shentrypoint.build/run.sh.