refactor: replace install-common.sh with declarative artifact system - #470
refactor: replace install-common.sh with declarative artifact system#470eoinfennessy wants to merge 6 commits into
Conversation
Make the upstream build's file download mechanism consistent with what is done for downstream Konflux hermetic builds. Replaces install-common.sh (which used runtime tools like hf download, docling-tools, and tiktoken) with a shared declarative artifact system: - artifacts.lock.yaml: declares all ML model artifacts with download URLs and SHA256 checksums, pinned to specific revisions for reproducibility - fetch_artifacts.py: async Python downloader with checksum verification - copy-artifacts.sh: copies fetched artifacts into correct cache layouts, used by both standard and Konflux builds Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughContainer build logic now swaps Estimated code review effort: 4 (Complex) | ~45 minutes Changes
Security notes: CWE-22 path traversal checks are added in 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
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
🧹 Nitpick comments (1)
distribution/copy-artifacts.sh (1)
23-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not hardcode the tiktoken cache key in a second place.
9b5ad71b2ce5302211f9c61530b329a4922fc6a4is derived from the URL indistribution/artifacts.lock.yaml. If that URL changes, the build still succeeds but runtime lookup misses the file. Put the cache key in the lockfile or derive it from the same URL during copy.🤖 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 `@distribution/copy-artifacts.sh` around lines 23 - 25, The tiktoken cache key is hardcoded in copy-artifacts logic and can drift from the source URL in the lockfile. Update the artifact copy step in distribution/copy-artifacts.sh so it does not duplicate the key manually; instead, read the cache key from distribution/artifacts.lock.yaml or derive it from the same download URL used there, keeping the tiktoken copy in sync with the lockfile.
🤖 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 `@distribution/copy-artifacts.sh`:
- Around line 30-41: The Hugging Face cache location in copy-artifacts.sh is
tied to ${HOME}/.cache/huggingface/hub, so the prefetched snapshot can be missed
when HOME changes between build and runtime. Update the cache root handling in
copy-artifacts.sh to place the HF cache under ${APP_ROOT} and make the image use
the same location by exporting HF_HOME/TRANSFORMERS_CACHE alongside the existing
env vars in the Containerfile, keeping the snapshot_dir and refs/main setup
aligned with that shared path.
In `@distribution/fetch_artifacts.py`:
- Around line 54-64: The artifact download flow in fetch_artifacts.py is
vulnerable to path traversal because download_artifact is called with output_dir
/ a["filename"] without validating the lock entry. Add validation before
building the destination path so each filename is strictly relative, reject
absolute paths and any path containing .. segments, and verify the resolved
target stays under output_dir. Apply this check in the artifact iteration that
prepares the tasks list, using the existing download_artifact and output_dir
handling as the integration points.
- Around line 1-6: The script header in fetch_artifacts.py leaves httpx and
pyyaml unpinned, so lock those dependencies in the script metadata or move the
script to the repo’s locked dependency path. Also update the artifact-writing
logic in fetch_artifacts.py where output_dir is joined with a["filename"] to
normalize the manifest-provided path and reject any absolute path or traversal
attempt (for example, any path that escapes the staging directory) before
writing files.
---
Nitpick comments:
In `@distribution/copy-artifacts.sh`:
- Around line 23-25: The tiktoken cache key is hardcoded in copy-artifacts logic
and can drift from the source URL in the lockfile. Update the artifact copy step
in distribution/copy-artifacts.sh so it does not duplicate the key manually;
instead, read the cache key from distribution/artifacts.lock.yaml or derive it
from the same download URL used there, keeping the tiktoken copy in sync with
the lockfile.
🪄 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: c60d2cab-a723-4885-9213-e7467bf050a2
📒 Files selected for processing (7)
ContainerfileContainerfile.indistribution/artifacts.lock.yamldistribution/constraints.txtdistribution/copy-artifacts.shdistribution/fetch_artifacts.pydistribution/install-common.sh
💤 Files with no reviewable changes (1)
- distribution/install-common.sh
The HuggingFace hub cache was placed under ${HOME} during build, but the
container runs as user 1001 with a different HOME, so SentenceTransformer
could not find the prefetched embedding model at runtime.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Validate artifact filenames before downloading to reject absolute paths and directory traversal attempts. Pin httpx and pyyaml version bounds in the PEP 723 script metadata. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Compute the SHA-1 hash at copy time so the cache filename stays in sync with the download URL rather than being a separately maintained constant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The opencv-python<0 constraint in constraints.txt silently caused the resolver to drop rapidocr and downgrade docling. Replace it with an explicit uninstall/install step in the Containerfile. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Containerfile (1)
19-23: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winENV var used by
copy-artifacts.shis set after the step that consumes it.
DOCLING_ARTIFACTS_PATHis only defined viaENVon Line 22, butcopy-artifacts.shis invoked on Line 20, before thatENVinstruction takes effect. Per the upstream script logic (distribution/copy-artifacts.sh:8-57),DOCLING_ARTIFACTS_PATH="${DOCLING_ARTIFACTS_PATH:-${APP_ROOT}/.cache/docling/models}"— since the env var is unset at execution time, the script silently falls back to its hardcoded default. That default happens to match the value on Line 22 today, but this is fragile: this is a latent misconfiguration bug (CWE-665, incorrect initialization) — any future change to theENVvalue or the script default will silently diverge with no build failure to flag it. Move theENV DOCLING_ARTIFACTS_PATHdeclaration above Line 19 so the value is authoritative and actually consumed by the script, rather than relying on coincidental default parity.🔒 Proposed fix
+ENV DOCLING_ARTIFACTS_PATH="${APP_ROOT}/.cache/docling/models" RUN uv run ${APP_ROOT}/fetch_artifacts.py ${APP_ROOT}/artifacts.lock.yaml /tmp/artifacts \ && ${APP_ROOT}/copy-artifacts.sh /tmp/artifacts \ && rm -rf /tmp/artifacts -ENV DOCLING_ARTIFACTS_PATH="${APP_ROOT}/.cache/docling/models" ENV HF_HOME="${APP_ROOT}/.cache/huggingface"The line-range-change-details confirm ENV DOCLING_ARTIFACTS_PATH is introduced after the fetch/copy RUN step
🤖 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 `@Containerfile` around lines 19 - 23, The `DOCLING_ARTIFACTS_PATH` environment variable is defined too late for `copy-artifacts.sh` to use it during the `RUN` step. Move the `ENV DOCLING_ARTIFACTS_PATH` declaration before the `RUN` instruction so `copy-artifacts.sh` sees the intended value instead of falling back to its internal default; keep `HF_HOME` where appropriate, and verify the build step still uses `fetch_artifacts.py` plus `copy-artifacts.sh` with the updated initialization order.
🧹 Nitpick comments (1)
Containerfile (1)
19-21: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueNo layer cache cleanup for
uvafter artifact fetch.
uv runwill populateuv's package/download cache during execution; nothing here scrubs it (unlike the explicitrm -rf /tmp/artifacts). Combined with any transient pip/uv cache from Line 12-14, this can quietly bloat the final image layer. ConsiderUV_NO_CACHE=1or an explicit cache purge in the sameRUN.🤖 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 `@Containerfile` around lines 19 - 21, The artifact fetch RUN step leaves uv’s cache behind, which can bloat the final image layer. Update the Containerfile’s artifact-fetch command to disable uv caching during `uv run` (for example via UV_NO_CACHE) or explicitly remove uv’s cache in the same RUN block, alongside the existing cleanup after `copy-artifacts.sh`.
🤖 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 `@Containerfile`:
- Around line 12-14: The OpenCV replacement step in the Containerfile is
bypassing the existing constraint file and leaving opencv-python-headless
unpinned, which breaks build reproducibility. Update the RUN block that installs
opencv-python-headless so it uses the same ${APP_ROOT}/constraints.txt
constraint as the other uv pip installs and specify an explicit version pin for
opencv-python-headless. Keep the change localized to the dependency swap
sequence around the uv pip install/uninstall commands.
---
Outside diff comments:
In `@Containerfile`:
- Around line 19-23: The `DOCLING_ARTIFACTS_PATH` environment variable is
defined too late for `copy-artifacts.sh` to use it during the `RUN` step. Move
the `ENV DOCLING_ARTIFACTS_PATH` declaration before the `RUN` instruction so
`copy-artifacts.sh` sees the intended value instead of falling back to its
internal default; keep `HF_HOME` where appropriate, and verify the build step
still uses `fetch_artifacts.py` plus `copy-artifacts.sh` with the updated
initialization order.
---
Nitpick comments:
In `@Containerfile`:
- Around line 19-21: The artifact fetch RUN step leaves uv’s cache behind, which
can bloat the final image layer. Update the Containerfile’s artifact-fetch
command to disable uv caching during `uv run` (for example via UV_NO_CACHE) or
explicitly remove uv’s cache in the same RUN block, alongside the existing
cleanup after `copy-artifacts.sh`.
🪄 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: bd22bf96-7881-4cec-a04b-8d996a4c4de7
📒 Files selected for processing (2)
ContainerfileContainerfile.in
🚧 Files skipped from review as they are similar to previous changes (1)
- Containerfile.in
Capture the exact version from uv pip show before uninstalling, so the headless variant matches what the resolver chose for rapidocr. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Superseded by #473 (pushed to upstream branch for CI). |
…473) ## Summary - Replaces `install-common.sh` (which used runtime tools like `hf download`, `docling-tools`, `tiktoken`) with a shared declarative artifact system consistent with how downstream Konflux hermetic builds fetch artifacts - Adds `artifacts.lock.yaml` declaring all ML model artifacts with download URLs and SHA256 checksums, pinned to specific revisions - Adds `fetch_artifacts.py` (async downloader with checksum verification) and `copy-artifacts.sh` (copies artifacts into correct cache layouts for both standard and Konflux builds) - Fixes opencv-python swap: the previous `opencv-python<0` constraint silently dropped rapidocr and downgraded docling; replaced with a post-install swap pinned to the resolved version - Fixes HF cache location to use `APP_ROOT` instead of `HOME` (build-time vs runtime user mismatch) - Adds path traversal validation to `fetch_artifacts.py` - Derives tiktoken cache key from URL instead of hardcoding Supersedes #470 (pushed to upstream branch for CI). ## Test plan - [x] Build container image locally and verify all 27 artifacts are fetched and checksums verified - [x] Verify tiktoken loads from cache and encodes correctly (`cl100k_base`) - [x] Verify granite embedding model loads from HF cache and produces meaningful 768-dim embeddings (semantic similarity check) - [x] Verify docling `DocumentConverter` initializes and processes a PDF end-to-end (layout analysis, RapidOCR, text extraction all using prefetched models) - [x] Verify `opencv-python` is absent and `opencv-python-headless` is installed at the same version (`4.13.0.92`) - [x] Run smoke tests against the built image (requires vLLM + PostgreSQL — covered by CI) 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit * **New Features** * Container builds now fetch and cache required model and tokenizer artifacts from a pinned lockfile, improving consistency across environments. * Added support for a Hugging Face cache location and artifact path configuration during image builds. * **Bug Fixes** * Replaced the standard OpenCV package with the headless variant to prevent runtime failures caused by missing system graphics libraries. * Artifact downloads are now verified with checksums and copied into the image safely. Approved-by: derekhiggins Approved-by: Artemon-line
1 participant
Summary
install-common.sh(which used runtime tools likehf download,docling-tools,tiktoken) with a shared declarative artifact system consistent with how downstream Konflux hermetic builds fetch artifactsartifacts.lock.yamldeclaring all ML model artifacts with download URLs and SHA256 checksums, pinned to specific revisionsfetch_artifacts.py(async downloader with checksum verification) andcopy-artifacts.sh(copies artifacts into correct cache layouts for both standard and Konflux builds)Test plan
podman build -t ogx-core .and verify all artifacts are fetched and placed correctly🤖 Generated with Claude Code
Summary by CodeRabbit