LCORE-858 konflux: hermetic build #748
Conversation
WalkthroughThis PR introduces hermetic build support by adding RPM dependency prefetching, configuring container build isolation, and defining hermetic Python dependency variants. New Tekton pipeline parameters enable source image builds and dependency prefetch inputs, while new configuration files define Red Hat UBI repository structures and locked RPM dependencies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 0
🧹 Nitpick comments (1)
.tekton/lightspeed-stack-pull-request.yaml (1)
30-30: Replace TODO comments with tracked issues.Informal TODO comments should be converted to tracked issues or linked via ADR/documentation. Line 30 references platform expansion; line 34 mentions coordinating changes with the on-push pipeline.
These items would be easier to track and communicate if converted to:
- GitHub issues with acceptance criteria
- An ADR (Architecture Decision Record) documenting the multi-platform strategy
- Comments in docs linking to the implementation plan
For now, consider adding issue numbers or links to clarify ownership and timeline.
Also applies to: 34-34
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (3)
.tekton/lightspeed-stack-pull-request.yaml (3)
35-40: Verify hermetic build enablement and coordinate on-push pipeline updates.This change activates hermetic (network-isolated) builds alongside source-image generation and dependency prefetching. These are significant behavioral changes that require:
- Verify the hermetic build works end-to-end: Confirm all build steps (especially custom ones in your Containerfile) remain compatible with network isolation.
- Confirm on-push pipeline sync: The TODO on line 34 indicates the on-push pipeline also needs these parameter additions. Ensure both pipelines are kept in sync to maintain consistent behavior across PR and merge workflows.
- Test the prefetch configuration: The
prefetch-inputJSON (line 38) pre-downloads pip dependencies. Verify this works with your project's dependency structure and any custom pip configurations.Run the following checks if available:
- Trigger a test build on this PR branch and verify the build completes successfully with hermetic mode enabled.
- Inspect the on-push pipeline configuration to confirm it also needs updating (see
.tekton/lightspeed-stack-on-push.yamlor similar).- Validate that all build-time dependencies are captured by the prefetch configuration.
1-40: PR requires completion before merge: checklist items outstanding.Per the PR objectives, this PR is marked WIP and the following checklist items remain unchecked:
- Self-review performed
- PR passed pre-merge test jobs
- Core feature tests added (if applicable)
The Testing section contains no concrete test steps or results. Before this can be merged:
- Complete self-review, documenting any concerns or design rationale.
- Run the full pre-merge test suite and provide results.
- Add or update tests to cover the hermetic build behavior if not already present.
- Update the on-push pipeline to maintain consistency (as noted in the TODO).
Please confirm:
- Have pre-merge tests been executed? If so, share the results.
- Are there existing tests for the build pipeline configuration? Do they need updates?
- Is the on-push pipeline update being handled in this PR or a follow-up?
38-38: JSON syntax is valid; manual verification of task compatibility required.The prefetch-input JSON at line 38 is syntactically valid and properly passed to the
prefetch-dependencies-oci-tatask via theinputparameter (line 186). Path"."correctly resolves to the cloned project root since the task runs afterclone-repository. However, verify that the remote task bundle (version 0.2) accepts the string value"allow_binary": "true"as configured—consult the task documentation or test execution to confirm compatibility.
raptorsun
force-pushed
the
hermetic
branch
2 times, most recently
from
279ac6d to
57b1036
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- rpms.in.yaml
- ubi.repo
- rpms.lock.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Use uv as the package manager; prefix commands with uv run
Applied to files:
requirements.hermetic.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (3)
requirements.hermetic.txt (1)
1-1: Upgrade uv to 0.9.7 after verifying hermetic build reproducibility.uv 0.8.15 is outdated; uv 0.9.7 was released October 30, 2025. However, the default
--python-platform linuxtarget changed from manylinux_2_17 to manylinux_2_28, which affects wheel resolution. Since the project pins a custom PyTorch CPU wheel index and uses a lockedrequirements.txtgenerated viauv pip compile, upgrading to 0.9.7 may causerequirements.txtto diverge, breaking hermetic reproducibility. Regeneraterequirements.txtwith uv 0.9.7 and verify the lock file is identical or intentionally updated before committing the upgrade..tekton/lightspeed-stack-pull-request.yaml (2)
37-38: Theallow_binary: truesetting is standard Cachi2/Hermeto practice and does not conflict with hermetic builds.The
allow_binaryparameter in Cachi2 controls whether the prefetcher will accept binary distributions (wheels) for Python packages instead of forcing source distributions. This setting operates during the prefetch phase—precisely where dependencies are cached before the hermetic build phase begins. Hermetic builds emphasize no external network access during the build itself, not during prefetching. Pre-fetching binary wheels (when available) is a standard optimization and does not weaken hermetic guarantees; the wheels are vendored and available for reproducible builds.The pipeline includes security scanning tasks (clair-scan, sast-snyk-check) that can audit the final artifacts, and the setting is intentional and documented. No action required.
35-40: All specified parameters are properly integrated and configuration files exist.The verification confirms:
Parameters are defined in the pipeline spec: build-source-image (lines 91-94), prefetch-input (lines 82-85), hermetic (lines 78-81) are all declared as string parameters with appropriate defaults.
Parameters are passed to downstream tasks:
prefetch-inputis correctly passed to the prefetch-dependencies task (line 186 as "input") and buildah-remote-oci-ta task (line 225 as "PREFETCH_INPUT")hermeticis passed to buildah-remote-oci-ta (line 223 as "HERMETIC")build-source-imageis used in the source-build-oci-ta task's when condition (line 320)Task references are correct: prefetch-dependencies-oci-ta:0.2, buildah-remote-oci-ta:0.6, and source-build-oci-ta:0.3 are all referenced at the specified lines.
Configuration files exist: rpms.in.yaml, rpms.lock.yaml, and ubi.repo are present in the repository root.
prefetch-input JSON is well-formed with valid fields (type, path, allow_binary).
Note: The prefetch-input JSON schema validation against the external Konflux task bundle schema cannot be performed locally; verify this against the official Konflux CI task catalog documentation if needed.
| value: quay.io/redhat-user-workloads/lightspeed-core-tenant/lightspeed-stack:on-pr-{{revision}} | ||
| - name: image-expires-after | ||
| value: 5d | ||
| # todo: add arm64. refer to https://konflux.pages.redhat.com/docs/users/getting-started/multi-platform-builds.html#arm64-2 |
There was a problem hiding this comment.
Incomplete TODOs block mergeability.
Lines 30 and 34 contain TODO comments that indicate incomplete work:
- Line 30: ARM64 multi-platform support not yet added
- Line 34: on-push pipeline not updated to match PR pipeline changes
These must be resolved or tracked as separate issues before merge.
Create follow-up issues or PRs to address:
- Add ARM64 platform support (refer to the linked Konflux documentation)
- Apply the same hermetic/prefetch changes to
.tekton/lightspeed-stack-on-push.yaml
Also applies to: 34-34
🤖 Prompt for AI Agents
In .tekton/lightspeed-stack-pull-request.yaml around lines 30 and 34, there are
TODO comments left for adding ARM64 multi-platform support and updating the
on-push pipeline to match PR-pipeline hermetic/prefetch changes; resolve by
either implementing ARM64 build support per the Konflux multi-platform docs (add
arm64 entries, multi-platform build config and any necessary image/platform
matrix) and by applying the same hermetic/prefetch changes to
.tekton/lightspeed-stack-on-push.yaml, or open two follow-up issues/PRs that
clearly track: (1) adding ARM64 support with a link to the Konflux guidance, and
(2) updating the on-push pipeline to mirror the PR pipeline changes — reference
the new issues/PR numbers in these TODOs and remove or replace the inline TODO
comments so the file is mergeable.
be8d77a to
96fbb5e
Compare
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)
pyproject.toml (1)
1-195: PR is marked "[WIP] Do Not Merge!" — address before review.The PR objectives indicate this is marked as work-in-progress with incomplete author checklists (self-review, pre-merge tests, feature tests all unchecked). This should be finalized before requesting substantive code review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks)pyproject.toml(1 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- rpms.lock.yaml
- .tekton/lightspeed-stack-pull-request.yaml
- rpms.in.yaml
- requirements.hermetic.txt
- ubi.repo
🧰 Additional context used
📓 Path-based instructions (1)
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
pyproject.toml: ALWAYS check pyproject.toml for existing dependencies before adding new ones
ALWAYS verify current library versions in pyproject.toml rather than assuming versions
Prefer reading supported Python versions and tool configs from pyproject.toml
Files:
pyproject.toml
🧠 Learnings (1)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to pyproject.toml : ALWAYS check pyproject.toml for existing dependencies before adding new ones
Applied to files:
pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
raptorsun
force-pushed
the
hermetic
branch
2 times, most recently
from
deee0f1 to
3acd44b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
.tekton/lightspeed-stack-pull-request.yaml (1)
30-30: Resolve TODOs before merge.The TODO comments at lines 30 and 34 were previously flagged as merge blockers. These must be addressed by either:
- Implementing the ARM64 support and updating the on-push pipeline, or
- Creating tracked issues and updating the TODOs with issue references.
Also applies to: 34-34
pyproject.toml (1)
208-209: Reconsider pdm in hermetic group—already flagged in previous review.The hermetic group includes both uv==0.8.15 and pdm>=2.21.0. A previous review comment identified that pdm is already the project's build backend and conflicts with the hermetic tooling approach that uses uv. Including both package managers in the same group creates confusion about which tool is intended for hermetic builds.
Based on the Containerfile (line 30) which uses
uv sync, the hermetic group should only include uv, not pdm. Remove pdm from this group.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks)Containerfile(1 hunks)pyproject.toml(2 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- requirements.hermetic.txt
- ubi.repo
🧰 Additional context used
📓 Path-based instructions (1)
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
pyproject.toml: ALWAYS check pyproject.toml for existing dependencies before adding new ones
ALWAYS verify current library versions in pyproject.toml rather than assuming versions
Prefer reading supported Python versions and tool configs from pyproject.toml
Files:
pyproject.toml
🧠 Learnings (2)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to pyproject.toml : ALWAYS check pyproject.toml for existing dependencies before adding new ones
Applied to files:
pyproject.toml
📚 Learning: 2025-08-18T10:57:39.266Z
Learnt from: matysek
Repo: lightspeed-core/lightspeed-stack PR: 292
File: pyproject.toml:59-59
Timestamp: 2025-08-18T10:57:39.266Z
Learning: In the lightspeed-stack project, transitive dependencies like faiss-cpu are intentionally pinned as top-level dependencies to maintain better control over the dependency graph and avoid version conflicts when bundling ML/LLM tooling packages.
Applied to files:
pyproject.toml
🪛 GitHub Actions: E2E Tests
Containerfile
[error] 29-33: RUN ... uv sync --locked --no-dev --group llslibdev; failed: The lockfile at uv.lock needs to be updated. To update, run 'uv lock'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: build-pr
🔇 Additional comments (2)
.tekton/lightspeed-stack-pull-request.yaml (1)
35-40: LGTM: Hermetic build parameters correctly configured.The three new parameters (build-source-image, prefetch-input, hermetic) are properly structured for Konflux hermetic builds. The prefetch-input correctly specifies both rpm and pip dependency types, with allow_binary enabled for pip packages.
rpms.lock.yaml (1)
1-40: Lockfile structure is correct but may be incomplete.The lockfile is well-formed and includes oniguruma (jq's dependency) and jq for both architectures. However, gcc and patch declared in rpms.in.yaml are not present here. This was flagged in the rpms.in.yaml review.
raptorsun
force-pushed
the
hermetic
branch
7 times, most recently
from
d46f456 to
5ebdc69
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Containerfile (1)
28-33: Consider extracting the common command to reduce duplication.Both branches execute the same
uv synccommand. You can simplify the conditional to only source the cachi2 environment when present, then run the command once:-# Source cachi2 environment for hermetic builds if available, otherwise use normal installation -RUN if [ -f /cachi2/cachi2.env ]; then \ - . /cachi2/cachi2.env && uv sync --locked --no-dev --group llslibdev; \ - else \ - uv sync --locked --no-dev --group llslibdev; \ - fi +# Source cachi2 environment for hermetic builds if available +RUN if [ -f /cachi2/cachi2.env ]; then . /cachi2/cachi2.env; fi && \ + uv sync --locked --no-dev --group llslibdevThis reduces duplication and makes future command changes easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks)Containerfile(1 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- rpms.in.yaml
- .tekton/lightspeed-stack-pull-request.yaml
- rpms.lock.yaml
- requirements.hermetic.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (1)
ubi.repo (1)
17-17: The ubi.repo SRPM URLs are correctly configured.The verification confirms that UBI9 source RPM repositories require the
$basearchvariable in the URL path. Testing shows:
- URLs with
$basearchreturn HTTP 302 (valid redirects)- URLs without
$basearchreturn HTTP 404 (not found)Red Hat's CDN organizes source packages with architecture-specific URL paths, making the current configuration correct. No changes are needed.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.tekton/lightspeed-stack-pull-request.yaml (1)
30-30: Unresolved TODOs block mergeability — duplicate of past review.Lines 30 and 34 contain TODO comments that remain unresolved:
- Line 30: ARM64 multi-platform support not yet added
- Line 34: on-push pipeline not updated to match PR pipeline changes
These were flagged in the previous review as blocking issues. Either resolve them or create and reference follow-up tracking issues before merging.
Also applies to: 34-34
🧹 Nitpick comments (1)
pyproject.toml (1)
178-221: Eliminate duplication: llslibdev-hermetic mirrors entire llslibdev group.Lines 178–221 duplicate the entire
llslibdevgroup (lines 130–177) verbatim. This creates a maintenance burden: any change tollslibdevmust be manually synchronized tollslibdev-hermetic. The comment at line 179 acknowledges this ("the same as llslibdev, just using default index"), but the actual difference in index routing is handled separately at lines 95–98.Explore one of these alternatives:
Option 1: Use group composition (if uv supports it)
llslibdev-hermetic = [ { include-group = "llslibdev" }, ]This inherits all dependencies from
llslibdevwhile allowing the torch source scoping to apply per-group routing.Option 2: Keep duplication with a maintenance warning
Add a prominent comment at line 178 explicitly warning that changes tollslibdev(lines 130–177) must be mirrored tollslibdev-hermetic, and clarify that torch variant routing is controlled by the source scoping at lines 95–98, not by group differences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks)Containerfile(1 hunks)pyproject.toml(2 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- rpms.in.yaml
- requirements.hermetic.txt
- ubi.repo
- rpms.lock.yaml
- Containerfile
🧰 Additional context used
📓 Path-based instructions (1)
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
pyproject.toml: ALWAYS check pyproject.toml for existing dependencies before adding new ones
ALWAYS verify current library versions in pyproject.toml rather than assuming versions
Prefer reading supported Python versions and tool configs from pyproject.toml
Files:
pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (2)
pyproject.toml (2)
95-98: Verify torch variant routing for llslibdev-hermetic.Line 96 routes
llslibdevgroup topytorch-cpu(CPU variant), but line 97 routesllslibdev-hermetictopypi-default, which defaults to the GPU variant on PyPI. The past review suggested both groups should use the CPU variant for consistency. Clarify whether the GPU variant is intentional for hermetic builds, or whether line 97 should also referencepytorch-cpu.
222-225: The review comment is based on an incorrect premise and should be dismissed.The review claims that
requirements.hermetic.txt specifies uv==0.8.15 as the hermetic tool, not pdm, but the actual file contents show thatrequirements.hermetic.txtcontains bothuv==0.8.15andpdm>=2.21.0. Therefore, includingpdm>=2.21.0in thetool-hermeticgroup is correct and maintains consistency with the hermetic requirements file. Having pdm as both the build-backend and in the hermetic tooling group is intentional for ensuring reproducible builds.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pyproject.toml (2)
178-221: Consider group composition to reduce duplication.The
llslibdev-hermeticgroup duplicates all 44 dependencies fromllslibdev. While this works, any future changes tollslibdevmust be manually synchronized. If your tooling supports it, consider using group composition/inclusion to reference the base group and avoid the maintenance burden.
95-98: Torch variant mismatch between groups—hermetic will use GPU instead of CPU.The torch source configuration directs
llslibdevto thepytorch-cpuindex but routesllslibdev-hermetictopypi-default, which defaults to the GPU variant. This creates an unintended hardware dependency difference between the two groups that should otherwise be equivalent.Apply this diff to align both groups to the CPU variant:
torch = [ { index = "pytorch-cpu", group = "llslibdev" }, - { index = "pypi-default", group = "llslibdev-hermetic" } + { index = "pytorch-cpu", group = "llslibdev-hermetic" } ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks)Containerfile(1 hunks)pyproject.toml(2 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- rpms.in.yaml
- requirements.hermetic.txt
- ubi.repo
- rpms.lock.yaml
🧰 Additional context used
📓 Path-based instructions (1)
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
pyproject.toml: ALWAYS check pyproject.toml for existing dependencies before adding new ones
ALWAYS verify current library versions in pyproject.toml rather than assuming versions
Prefer reading supported Python versions and tool configs from pyproject.toml
Files:
pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (5)
Containerfile (1)
28-33: LGTM! Clean hermetic build conditional.The conditional correctly switches between hermetic and standard builds by detecting the cachi2 environment file and selecting the appropriate dependency group. Both branches maintain lockfile integrity with the
--lockedflag..tekton/lightspeed-stack-pull-request.yaml (2)
30-30: TODOs are tracked but need resolution before merge.The TODO comments for ARM64 support (line 30) and on-push pipeline updates (line 34) were flagged in previous reviews as blocking mergeability. Since this PR is still marked WIP, these are acceptable for now, but ensure they're addressed or converted to tracked issues before removing the WIP status.
Also applies to: 34-34
35-40: LGTM! Hermetic build parameters are correctly configured.The new parameters enable hermetic builds with appropriate prefetch configuration for both RPM and Python dependencies. The
allow_binary: truesetting for pip is appropriate for this Python project.pyproject.toml (2)
89-93: LGTM! PyPI default index correctly configured.The explicit pypi-default index is properly configured for hermetic builds that need to use the standard PyPI repository.
99-105: LGTM! Conflict declaration prevents mixing incompatible groups.The mutual exclusion between
llslibdevandllslibdev-hermeticis correctly configured to prevent simultaneous installation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pyproject.toml (1)
222-225: Remove pdm from tool-hermetic; it is unused in hermetic builds.Previous review verified that pdm is not invoked during hermetic builds (no references in Containerfile, Tekton pipeline, or build scripts). The hermetic context uses only uv. Since pdm's sole role is the project's build backend (
pdm.backendinbuild-systemat line 247), it does not belong intool-hermetic. Remove line 224 ("pdm>=2.21.0") to keep only the uv entry.Apply this diff:
tool-hermetic = [ "uv==0.8.15", - "pdm>=2.21.0", ]
🧹 Nitpick comments (1)
pyproject.toml (1)
178-225: Eliminate duplication between llslibdev and llslibdev-hermetic groups; document synchronization requirement.Lines 178–221 duplicate all 44 lines of the llslibdev group verbatim (lines 130–177), then append build tools in lines 222–225. This creates a maintenance burden: any dependency update to llslibdev must be manually mirrored to llslibdev-hermetic.
If uv's dependency group syntax supports group composition (e.g.,
include-group), refactor to reference the base group and only override/append differing entries. Otherwise, add a prominent comment warning maintainers to mirror llslibdev changes to llslibdev-hermetic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks)Containerfile(1 hunks)pyproject.toml(2 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- rpms.in.yaml
- .tekton/lightspeed-stack-pull-request.yaml
- rpms.lock.yaml
- ubi.repo
- requirements.hermetic.txt
- Containerfile
🧰 Additional context used
📓 Path-based instructions (1)
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
pyproject.toml: ALWAYS check pyproject.toml for existing dependencies before adding new ones
ALWAYS verify current library versions in pyproject.toml rather than assuming versions
Prefer reading supported Python versions and tool configs from pyproject.toml
Files:
pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (1)
pyproject.toml (1)
90-98: Clarify intentionality of torch variant split across indices.Past review (marked resolved in fadad34) recommended extending CPU variant to both llslibdev and llslibdev-hermetic. However, the current code specifies
pytorch-cpufor llslibdev butpypi-defaultfor llslibdev-hermetic, causing hermetic builds to resolve torch from the default GPU-first PyPI index—contradicting the stated intent.Is this split intentional (perhaps hermetic builds require GPU torch despite non-hermetic using CPU)? If so, add an explanatory comment. If not, apply the suggested fix to use
pytorch-cpufor both groups.
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
raptorsun
force-pushed
the
hermetic
branch
2 times, most recently
from
5f4c673 to
90686ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
pyproject.toml (3)
95-98: Torch variant mismatch—llslibdev-hermetic should use pytorch-cpu, not pypi-default.The torch configuration (lines 95–98) specifies
pytorch-cpuindex forllslibdev(CPU-only, smaller) butpypi-defaultforllslibdev-hermetic(GPU variant, much larger). This contradicts the intent that hermetic builds should match the dev environment. A past review flagged this same issue and marked it addressed, but the code still shows the inconsistency.Apply this diff to align both groups on the CPU variant:
torch = [ { index = "pytorch-cpu", group = "llslibdev" }, - { index = "pypi-default", group = "llslibdev-hermetic" } + { index = "pytorch-cpu", group = "llslibdev-hermetic" } ]Verify: Was commit fadad34 actually merged to this branch, and if so, why does the torch variant still differ?
178-221: Eliminate duplication—llslibdev-hermetic copies entire llslibdev group.Lines 178–221 duplicate all 44 lines of the
llslibdevgroup verbatim, creating maintenance risk: future changes tollslibdevmust be manually mirrored to avoid divergence. A past review flagged this same maintenance burden and marked it addressed, but the full duplication remains.If your uv version supports group composition, refactor to reference the existing group:
llslibdev-hermetic = [ { include-group = "llslibdev" }, "torch==2.7.1", # re-pin here if needed to override index scoping ]Otherwise, add a prominent comment warning that changes to
llslibdevmust be manually synced tollslibdev-hermetic, and clarify that the torch GPU/CPU variant is determined by the index scoping (line 97), not by duplicate pins here.
222-226: Remove pdm and pdm-backend from tool-hermetic—not used in hermetic builds.The
tool-hermeticdependency group includespdm>=2.21.0andpdm-backend==2.4.5, but hermetic builds use onlyuv(line 223). Since pdm is already the project's build backend (specified in[build-system]line 248), it does not belong in this group. A past review flagged this and marked it addressed, but both entries remain.Apply this diff to remove the unused build-system entries:
tool-hermetic = [ "uv==0.8.15", - "pdm>=2.21.0", - "pdm-backend==2.4.5", ]
🧹 Nitpick comments (1)
.tekton/lightspeed-stack-pull-request.yaml (1)
30-30: TODO comments block merge—resolve or track as separate issues.Lines 30 and 34 contain incomplete work markers that prevent merging:
- Line 30: ARM64 multi-platform support not yet implemented
- Line 34: On-push pipeline not synchronized with PR pipeline changes
Given that the PR is marked [WIP] and the author checklist is unchecked, these are documented known gaps. Before merge, either implement both features or open two follow-up issues with explicit references and remove/replace the TODO comments so the file is merge-ready.
I can help generate the follow-up issue templates or implement ARM64 support per the Konflux multi-platform documentation if you'd like.
Also applies to: 34-34
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks)Containerfile(1 hunks)pyproject.toml(2 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- rpms.lock.yaml
- ubi.repo
- requirements.hermetic.txt
- Containerfile
🧰 Additional context used
📓 Path-based instructions (1)
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
pyproject.toml: ALWAYS check pyproject.toml for existing dependencies before adding new ones
ALWAYS verify current library versions in pyproject.toml rather than assuming versions
Prefer reading supported Python versions and tool configs from pyproject.toml
Files:
pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (2)
.tekton/lightspeed-stack-pull-request.yaml (1)
35-40: Parameter additions are properly integrated.The three new parameters (
build-source-image,prefetch-input,hermetic) align with pipeline spec definitions and are correctly forwarded to dependent tasks (buildah-remote, source-build, etc.). The prefetch-input JSON structure matches the prefetch-dependencies task expectations.rpms.in.yaml (1)
1-4: Structure and architecture alignment look correct.The RPM configuration correctly declares all required packages (gcc, jq, patch), references the ubi.repo repository, and specifies both target architectures. This aligns with the Tekton prefetch-input configuration and the build-platforms specified in the PipelineRun.
raptorsun
force-pushed
the
hermetic
branch
4 times, most recently
from
d3c6138 to
9a447a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
Containerfile (1)
28-28: Remove unused requirements.txt from COPY statement.This issue was previously flagged:
requirements.txtis copied but not used in either the hermetic or standard build paths. Both invokeuv syncwithpyproject.toml. Remove${LSC_SOURCE_DIR}/requirements.txtfrom the COPY statement.pyproject.toml (1)
222-226: Remove pdm and pdm-backend from tool-hermetic group.A previous review comment identified that pdm is not invoked during hermetic builds and requested its removal from this group. Verification confirmed no pdm usage in the Containerfile, pipeline, or build scripts. The hermetic build uses only uv. Since pdm's role is limited to being the build backend (
[build-system]), it should not be in this dependency group.Apply this diff:
tool-hermetic = [ "uv==0.8.15", - "pdm>=2.21.0", - "pdm-backend==2.4.5", ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks)Containerfile(1 hunks)pyproject.toml(2 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .tekton/lightspeed-stack-pull-request.yaml
- ubi.repo
- rpms.in.yaml
- requirements.hermetic.txt
- rpms.lock.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (1)
pyproject.toml (1)
89-105: Incomplete hermetic torch configuration—intentional staging for WIP?The torch source for
llslibdev-hermetic(line 97), thepypi-defaultindex (lines 90-93), and the conflicts block (lines 99-105) are all commented out. This is consistent with thellslibdev-hermeticdependency group also being commented out (lines 178-221), but it leaves hermetic build support incomplete. Given the PR is marked[WIP]and "Do Not Merge," this may be intentional staging.If hermetic builds are intended to be testable in this PR, uncomment these blocks. Otherwise, document that hermetic configuration is staged but not yet active.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pyproject.toml (1)
222-226: Remove pdm from tool-hermetic; it is unused in hermetic builds.The
tool-hermeticgroup includes"pdm>=2.21.0"(line 224), but verification from past review confirms pdm is not invoked during hermetic builds. No pdm references appear in the Containerfile, Tekton pipeline, or build scripts. Since pdm's sole role is as the project's build backend (specified in[build-system]line 247), it does not belong in the hermetic tooling group.Additionally, the version constraint
2.21.0is outdated; the latest stable release is 2.26.1 (October 2025).Apply this diff to remove pdm and keep only uv for hermetic builds:
tool-hermetic = [ "uv==0.8.15", - "pdm>=2.21.0", "pdm-backend==2.4.5", ]Verify whether
pdm-backend==2.4.5is also needed in hermetic builds or should be removed as well.
🧹 Nitpick comments (1)
pyproject.toml (1)
178-221: Duplication of llslibdev group remains; consider refactoring for maintainability.The
llslibdev-hermeticgroup duplicates all 43 lines ofllslibdevverbatim before adding hermetic tools. This creates a maintenance burden: any future change tollslibdev(dependencies, versions, comments) must be manually mirrored tollslibdev-hermeticto keep them in sync.Past review suggested using group composition (e.g.,
include-group) if the tool supports it, or at minimum documenting the duplication risk with a prominent comment. The current implementation has neither.If uv supports dependency group composition, refactor to:
llslibdev-hermetic = [ { include-group = "llslibdev" }, "uv==0.8.15", ]If composition is not supported, add a comment warning that changes to
llslibdevmust be mirrored here to prevent divergence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks)Containerfile(1 hunks)pyproject.toml(2 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Containerfile
- .tekton/lightspeed-stack-pull-request.yaml
- ubi.repo
- requirements.hermetic.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (3)
rpms.in.yaml (1)
1-4: ✅ RPM configuration is complete and consistent with lockfile.All three declared packages (gcc, jq, patch) are now present in rpms.lock.yaml for both x86_64 and aarch64 architectures with resolved NEVRA entries. The reference to
./ubi.repois valid, and the architecture list matches the lockfile structure.rpms.lock.yaml (1)
1-355: ✅ Lockfile structure and content are valid.The lockfile includes all declared packages with proper NEVRA entries, sha256 checksums, and architecture-specific URLs for both aarch64 and x86_64. Repository IDs (ubi-9-baseos-rpms, ubi-9-appstream-rpms) align with the ubi.repo configuration.
pyproject.toml (1)
95-98: Torch source still uses PyPI default for hermetic group (GPU variant mismatch).Line 97 specifies
index = "pypi-default"forllslibdev-hermetic, which resolves torch to the PyPI GPU variant. This contradicts the CPU-only variant used byllslibdev(line 96:pytorch-cpu), causing the hermetic build to pull a significantly larger, architecture-incompatible torch package.Past review flagged this issue as "Addressed in commit f35507a" but the fix was not applied to the current code.
Apply this diff to align hermetic with non-hermetic torch resolution:
torch = [ { index = "pytorch-cpu", group = "llslibdev" }, - { index = "pypi-default", group = "llslibdev-hermetic" } + { index = "pytorch-cpu", group = "llslibdev-hermetic" } ]
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
pyproject.toml (3)
178-221: Duplication remains between llslibdev groups—consider composition.The llslibdev-hermetic group duplicates 43 of 44 lines from llslibdev, differing only in torch's platform marker (line 212). Past review flagged this maintenance burden: changes to llslibdev must be manually mirrored. While marked as addressed in a previous commit, the duplication persists.
If UV supports group composition, consider:
llslibdev-hermetic = [ { include-group = "llslibdev" }, # Override torch with platform marker "torch==2.7.1; sys_platform == 'linux'", ]Otherwise, add a prominent comment warning that llslibdev changes must be mirrored here.
Based on learnings
95-98: Torch variant mismatch persists—hermetic group uses GPU variant.This issue remains from past review: llslibdev uses
pytorch-cpuindex (CPU-only), while llslibdev-hermetic usespypi-default(GPU variant, significantly larger). This contradicts the stated intent that hermetic should mirror llslibdev except for build tools.Apply this diff to use CPU variant for both:
torch = [ { index = "pytorch-cpu", group = "llslibdev" }, - { index = "pypi-default", group = "llslibdev-hermetic" } + { index = "pytorch-cpu", group = "llslibdev-hermetic" } ]Based on learnings
222-226: Remove pdm from tool-hermetic—not used in hermetic builds.Past verification confirmed pdm is not invoked during hermetic builds. The hermetic workflow uses only uv. Since pdm's role is solely as the build backend (in
[build-system]), it does not belong in the tool-hermetic dependency group.Apply this diff:
tool-hermetic = [ "uv==0.8.15", - "pdm>=2.21.0", - "pdm-backend==2.4.5", ]Based on learnings
Containerfile (2)
22-23: Remove unused pdm installation—hermetic builds use only uv.This conditional pdm installation remains from a previous review. Verification shows hermetic builds invoke only
uv sync(line 36), and no pdm commands are used elsewhere. Installing pdm here adds unnecessary dependencies and image bloat.Apply this diff:
-# Install pdm and pdm-backend for hermetic builds -RUN if [ -f /cachi2/cachi2.env ]; then pip3.12 install "pdm>=2.21.0" "pdm-backend"; fi -Based on learnings
28-28: Remove unused requirements.txt from COPY statement.This file is copied but never used in the build. Past review confirmed neither the hermetic build (
uv sync --group llslibdev-hermetic) nor the standard build (uv sync --group llslibdev) references requirements.txt. It adds unnecessary bloat.Apply this diff:
-COPY ${LSC_SOURCE_DIR}/pyproject.toml ${LSC_SOURCE_DIR}/LICENSE ${LSC_SOURCE_DIR}/README.md ${LSC_SOURCE_DIR}/uv.lock ${LSC_SOURCE_DIR}/requirements.txt ./ +COPY ${LSC_SOURCE_DIR}/pyproject.toml ${LSC_SOURCE_DIR}/LICENSE ${LSC_SOURCE_DIR}/README.md ${LSC_SOURCE_DIR}/uv.lock ./Based on learnings
requirements.hermetic.txt (1)
1-3: Remove pdm and pdm-backend—only uv is used in hermetic builds.Past review confirmed pdm is not invoked during hermetic builds and serves only as the build backend. The hermetic workflow uses uv exclusively. Including these packages is inconsistent with hermetic tooling requirements.
Apply this diff:
uv==0.8.15 -pdm>=2.21.0 -pdm-backendBased on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks)Containerfile(1 hunks)pyproject.toml(3 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- rpms.in.yaml
- .tekton/lightspeed-stack-pull-request.yaml
- ubi.repo
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (4)
pyproject.toml (2)
90-93: LGTM: pypi-default index added for hermetic group.The explicit pypi-default index enables hermetic builds to use PyPI packages while the standard build uses pytorch-cpu index. The
explicit = trueflag ensures packages must explicitly reference this index.
99-105: LGTM: UV conflicts prevent mixing hermetic and non-hermetic groups.The conflict declaration correctly prevents simultaneous installation of llslibdev and llslibdev-hermetic groups, enforcing clean separation between standard and hermetic builds.
Containerfile (1)
31-39: Hermetic build conditional verified as correct.The uv.lock lockfile exists and contains the llslibdev-hermetic group, confirming that past critical issues (missing hermetic group and outdated lockfile) have been resolved. The conditional logic correctly routes to the hermetic path with proper environment sourcing and Python version specification.
rpms.lock.yaml (1)
1-355: Lockfile is current and valid.All three packages specified in rpms.in.yaml are present in rpms.lock.yaml for both architectures:
- gcc (lines 18, 197)
- jq (lines 137, 309)
- patch (lines 74, 246)
Additional packages are transitive dependencies, which is correct for a hermetic build lockfile. All packages include required metadata (URL, checksum, size, EVR, source RPM). The lockfile is properly formatted and up-to-date with the input specification.
pyproject.toml
Outdated
| "RAG" | ||
| ] | ||
| requires-python = ">=3.12,<3.14" | ||
| requires-python = ">=3.12,<3.13" |
There was a problem hiding this comment.
🧩 Analysis chain
Python version restricted to 3.12.x—verify intentional for hermetic builds.
The Python requirement is narrowed from a broader range to >=3.12,<3.13, restricting to Python 3.12.x only. This is typical for hermetic builds requiring specific versions, but verify this doesn't break compatibility for users on different Python versions.
Verify this restriction is documented and intentional:
🏁 Script executed:
#!/bin/bash
# Description: Check if Python version restriction is documented
echo "=== Check README for Python version requirements ==="
rg -n "python.*3\.12|requires.*python" README.md -i -A2 -B2 || echo "Not documented in README"
echo -e "\n=== Check if CI uses Python 3.12 ==="
rg -n "python.*3\.12|python-version.*3\.12" .github/workflows/ .tekton/ -A1 -B1 || echo "Not found in CI config"Length of output: 2664
Python version restriction breaks documented 3.13 support.
The change restricts to >=3.12,<3.13 (3.12.x only), but the README documents support for both Python 3.12 and 3.13, and CI tests run on both versions. Update to >=3.12,<3.14 to maintain advertised compatibility, or clarify documentation if 3.13 support is being dropped.
🤖 Prompt for AI Agents
In pyproject.toml around line 21, the Python version constraint was narrowed to
">=3.12,<3.13" which contradicts README and CI that claim support for Python
3.13; update the constraint to ">=3.12,<3.14" to allow both 3.12 and 3.13, or if
3.13 is intentionally dropped, update README and CI to reflect the stricter
requirement — make the change consistent across pyproject.toml, README, and CI
matrix.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
requirements.hermetic.txt (1)
2-3: Remove pdm and pdm-backend—they are build-time dependencies, not hermetic runtime tools.As previously flagged, pdm serves only as the project's build backend (
[build-system]in pyproject.toml) and is not invoked during hermetic builds. The hermetic workflow uses uv and pip. Including these entries is inconsistent with hermetic tooling requirements. Based on learnings.Apply this diff:
uv==0.8.15 -pdm>=2.21.0 -pdm-backend pip==24.2pyproject.toml (2)
221-225: Remove pdm and pdm-backend from tool-hermetic group.As previously flagged with verification, pdm is not invoked during hermetic builds and serves only as the project's build backend. The hermetic workflow uses only uv. Including pdm and pdm-backend here is inconsistent with hermetic tooling requirements. Based on learnings.
Apply this diff:
tool-hermetic = [ "uv==0.8.15", - "pdm>=2.21.0", - "pdm-backend==2.4.5", ]
96-99: Critical: Torch variant mismatch regressed—hermetic group uses GPU variant.The llslibdev-hermetic group resolves torch from
pypi-default(GPU variant), while llslibdev usespytorch-cpu(CPU variant). This was previously flagged and marked as addressed in commit f35507a, but the issue persists or has regressed. The hermetic build will pull a much larger GPU torch package, contradicting the stated intent (line 178 comment: "same as llslibdev").Apply this diff to align both groups to CPU variant:
torch = [ { index = "pytorch-cpu", group = "llslibdev" }, - { index = "pypi-default", group = "llslibdev-hermetic" } + { index = "pytorch-cpu", group = "llslibdev-hermetic" } ]Containerfile (1)
22-23: Remove unused pdm installation—hermetic builds use only uv and pip.As previously flagged, the hermetic build path (line 36) invokes only
uv venvandpip install. No pdm commands appear in the hermetic workflow. Since pdm's role is limited to being the build backend (specified in pyproject.toml[build-system]), installing it here adds unnecessary dependencies and image bloat. Based on learnings.Remove these lines:
-# Install pdm and pdm-backend for hermetic builds -RUN if [ -f /cachi2/cachi2.env ]; then pip3.12 install "pdm>=2.21.0" "pdm-backend"; fi -
🧹 Nitpick comments (1)
pyproject.toml (1)
177-220: Refactor: llslibdev-hermetic duplicates entire llslibdev group—maintenance burden.The llslibdev-hermetic group duplicates all 43 lines from llslibdev, creating a synchronization burden. This issue was previously flagged and marked as addressed in commit be34f46, but the duplication persists or has regressed. Any change to llslibdev requires manual mirroring to llslibdev-hermetic.
If uv supports group composition, consider:
llslibdev-hermetic = [ { include-group = "llslibdev" }, # hermetic-specific overrides if needed ]If composition is unavailable, add a prominent comment documenting the synchronization requirement and clarifying that torch variant selection is controlled by
[tool.uv.sources](lines 96-99), not by differences in this group definition.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks)Containerfile(1 hunks)pyproject.toml(2 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- rpms.in.yaml
- ubi.repo
- rpms.lock.yaml
- .tekton/lightspeed-stack-pull-request.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (5)
requirements.hermetic.txt (1)
4-4: LGTM—pip pin aligns with hermetic build usage.The hermetic build path in Containerfile (line 36) invokes
pip install, so pinningpip==24.2ensures reproducible builds.pyproject.toml (2)
91-94: LGTM—explicit PyPI index supports hermetic source management.Adding the
pypi-defaultindex as explicit aligns with hermetic build requirements to control package sources precisely.
100-106: LGTM—conflict declaration prevents mixing hermetic and non-hermetic groups.The conflict ensures that llslibdev and llslibdev-hermetic cannot be used simultaneously, which is appropriate given their different resolution strategies.
Containerfile (2)
28-28: Clarification: requirements.txt is used in hermetic build path.Contrary to a previous review comment,
requirements.txtis actively used in the hermetic build path (line 36):pip install -r requirements.txt. The file serves a valid purpose and should remain in the COPY statement.
31-39: The original review comment is based on a misunderstanding of the architecture.The requirements.txt file is generated using
uv pip compile pyproject.toml ... --generate-hashes --group llslibdev-hermetic, making it the compiled output of the llslibdev-hermetic group, not a competing approach. The Containerfile correctly implements the documented hermetic build strategy: line 36 uses pip install from the hashed requirements.txt for hermetic builds, while line 38 falls back to uv sync with the llslibdev group for standard builds. This is the intended design—define dependencies in pyproject.toml, compile them to requirements.txt with hashes for hermetic isolation, and maintain a fallback path using the standard dependency group.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
Containerfile (1)
22-23: Remove unused pdm installation—hermetic builds use only uv and pip.Past reviews confirmed that the hermetic build path (line 36) invokes only
pipanduv, neverpdm. This conditional installation adds unnecessary dependencies and image bloat.Apply this diff:
-# Install pdm and pdm-backend for hermetic builds -RUN if [ -f /cachi2/cachi2.env ]; then pip3.12 install "pdm>=2.21.0" "pdm-backend"; fi -pyproject.toml (1)
96-99: Critical: Torch variant mismatch—hermetic group uses default GPU variant.This configuration creates an inconsistency between the two groups:
llslibdev: resolves torch frompytorch-cpuindex (CPU-only, smaller)llslibdev-hermetic: resolves torch frompypi-defaultindex (GPU variant from PyPI, much larger)This contradicts the intent stated at line 178 that the hermetic group is "the same as llslibdev". The hermetic build will pull a different, larger torch package.
Apply this diff to use the CPU variant for both groups:
torch = [ { index = "pytorch-cpu", group = "llslibdev" }, - { index = "pypi-default", group = "llslibdev-hermetic" } + { index = "pytorch-cpu", group = "llslibdev-hermetic" } ]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks).tekton/lightspeed-stack-push.yaml(1 hunks)Containerfile(1 hunks)pyproject.toml(2 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- rpms.in.yaml
- ubi.repo
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (6)
rpms.lock.yaml (1)
1-355: LGTM—machine-generated RPM lockfile for hermetic builds.This lockfile provides pinned RPM dependencies for both aarch64 and x86_64 architectures. The format is correct and the content is appropriate for the hermetic build workflow introduced in this PR.
pyproject.toml (1)
91-94: LGTM—explicit PyPI index for hermetic group.Adding an explicit
pypi-defaultindex allows granular control over which dependency groups use which package sources, supporting the hermetic build workflow..tekton/lightspeed-stack-push.yaml (2)
31-36: LGTM—hermetic build parameters added correctly.The addition of
build-source-image,prefetch-input, andhermeticparameters properly enables hermetic builds in the push pipeline. Theprefetch-inputcorrectly references bothrequirements.txtandrequirements.hermetic.txt, and the RPM prefetch configuration aligns with the newrpms.lock.yamllockfile.
27-27: Resolve TODO or create tracking issue for ARM64 support.The TODO comment indicates ARM64 multi-platform support is planned but not yet implemented. This blocks the pipeline from building ARM64 images.
Either implement ARM64 support now (following the linked Konflux documentation), or create a tracking issue and reference it in this comment. Since the PR is marked "Do Not Merge!", this TODO must be resolved before the feature is ready.
.tekton/lightspeed-stack-pull-request.yaml (2)
35-40: LGTM—hermetic build parameters configured correctly.The addition of
build-source-image,prefetch-input, andhermeticparameters enables hermetic builds in the PR pipeline. The configuration correctly references both requirements files and aligns with the hermetic build workflow.
30-34: Resolve TODOs before removing WIP status.Two incomplete tasks are documented:
- Line 30: ARM64 multi-platform support not yet added
- Line 34: On-push pipeline needs the same hermetic changes (though line 27 of push.yaml shows this has been partially addressed)
Since the PR is marked "Do Not Merge!", these TODOs should be resolved or converted to tracked issues before finalizing. Line 34's TODO about the on-push pipeline appears outdated since
.tekton/lightspeed-stack-push.yamlalready includes the hermetic parameters. Consider removing this TODO or clarifying what additional work is needed.
| llslibdev-hermetic = [ | ||
| # the same as llslibdev, just using default index. | ||
| "matplotlib>=3.10.0", | ||
| "pillow>=11.1.0", | ||
| "pandas>=2.2.3", | ||
| "scikit-learn>=1.5.2", | ||
| "psycopg2-binary>=2.9.10", | ||
| # API eval: inline::meta-reference | ||
| "tree_sitter>=0.24.0", | ||
| "pythainlp>=3.0.10", | ||
| "langdetect>=1.0.9", | ||
| "emoji>=2.1.0", | ||
| "nltk>=3.8.1", | ||
| # API inference: remote::gemini | ||
| "litellm>=1.75.5.post1", | ||
| # API inference: inline::sentence-transformers | ||
| "sentence-transformers>=5.0.0", | ||
| # API vector_io: inline::faiss | ||
| "faiss-cpu>=1.11.0", | ||
| # API scoring: inline::basic | ||
| "requests>=2.32.4", | ||
| # API datasetio: inline::localfs | ||
| "aiosqlite>=0.21.0", | ||
| # API datasetio: remote::huggingface | ||
| "datasets>=3.6.0", | ||
| # API telemetry: inline::meta-reference | ||
| "opentelemetry-sdk>=1.34.1", | ||
| "opentelemetry-exporter-otlp>=1.34.1", | ||
| # API tool_runtime: inline::rag-runtime | ||
| "transformers>=4.34.0", | ||
| "numpy==2.2.6", | ||
| # API tool_runtime: remote::model-context-protocol | ||
| "mcp>=1.9.4", | ||
| # API post_training: inline::huggingface | ||
| "torch==2.7.1; sys_platform == 'linux'", | ||
| "trl>=0.18.2", | ||
| "peft>=0.15.2", | ||
| # Other | ||
| "autoevals>=0.0.129", | ||
| "fire>=0.7.0", | ||
| "opentelemetry-instrumentation>=0.55b0", | ||
| "blobfile>=3.0.0", | ||
| "psutil>=7.0.0", | ||
| ] |
There was a problem hiding this comment.
Inconsistent torch platform marker—creates behavioral divergence.
The llslibdev-hermetic group at line 211 specifies torch==2.7.1; sys_platform == 'linux', but the regular llslibdev group at line 167 has no platform marker. This means:
llslibdev: torch installs on all platformsllslibdev-hermetic: torch installs only on Linux
This contradicts the comment at line 178 stating the groups are "the same". If the Linux restriction is intentional for hermetic builds, document why. Otherwise, remove the platform marker for consistency.
Apply this diff if the platform restriction is unintentional:
- "torch==2.7.1; sys_platform == 'linux'",
+ "torch==2.7.1",🤖 Prompt for AI Agents
In pyproject.toml around lines 177 to 220 the llslibdev-hermetic extras list
contains a platform marker on the torch entry ("torch==2.7.1; sys_platform ==
'linux'") while the llslibdev group (around line 167) has torch with no marker,
causing inconsistent behavior; if the Linux-only restriction is unintentional,
remove the platform marker so both groups specify torch identically (or if
intentional, update the comment to document why hermetic excludes non-Linux),
ensuring both extras are kept semantically the same.
pyproject.toml
Outdated
| tool-hermetic = [ | ||
| "uv==0.8.15", | ||
| "pdm>=2.21.0", | ||
| "pdm-backend==2.4.5", | ||
| ] |
There was a problem hiding this comment.
Critical: Remove pdm and pdm-backend from tool-hermetic group.
Past reviews confirmed that pdm is not invoked during hermetic builds. The Containerfile hermetic path (line 36) uses only uv sync, and no pdm commands appear in the build process. Since pdm's sole role is as the project's build backend (specified in [build-system]), it does not belong in the hermetic tooling group.
Apply this diff to remove the unused pdm entries:
tool-hermetic = [
"uv==0.8.15",
- "pdm>=2.21.0",
- "pdm-backend==2.4.5",
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tool-hermetic = [ | |
| "uv==0.8.15", | |
| "pdm>=2.21.0", | |
| "pdm-backend==2.4.5", | |
| ] | |
| tool-hermetic = [ | |
| "uv==0.8.15", | |
| ] |
🤖 Prompt for AI Agents
In pyproject.toml at lines 221 to 225, the tool-hermetic dependency group
includes pdm and pdm-backend entries that are not used during hermetic builds.
The Containerfile hermetic path only uses uv sync and contains no pdm commands.
Since pdm is only needed as the project's build backend in the [build-system]
section, remove the "pdm>=2.21.0" and "pdm-backend==2.4.5" lines from the
tool-hermetic group, leaving only the uv==0.8.15 entry.
f3aa2a8 to
5c49876
Compare
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.tekton/lightspeed-stack-pull-request.yaml (2)
30-30: Duplicate: TODO for ARM64 support already flagged.This TODO for adding ARM64 multi-platform support was already raised in previous reviews. Since the PR is marked WIP "Do Not Merge!", this is expected, but ensure a follow-up issue is created to track ARM64 implementation.
34-34: Duplicate: TODO for on-push pipeline already flagged.This TODO for updating the on-push pipeline to match PR pipeline changes was already raised in previous reviews. Since the PR is marked WIP "Do Not Merge!", this is expected, but ensure this is tracked as follow-up work.
Containerfile (1)
28-36: Critical: Hermetic build doesn't use llslibdev-hermetic group—creates drift risk.The hermetic build path (line 33) installs from requirements.txt using pip, while pyproject.toml defines a dedicated
llslibdev-hermeticdependency group. This creates two separate dependency specifications that can drift out of sync. Although a past review comment was marked "✅ Addressed in commit f3aa2a8," the current code still usespip install -r requirements.txtinstead of the recommendeduv sync --group llslibdev-hermeticapproach.Update the hermetic path to use the llslibdev-hermetic group from pyproject.toml:
RUN if [ -f /cachi2/cachi2.env ]; then \ - . /cachi2/cachi2.env && uv venv --seed --no-index --find-links ${PIP_FIND_LINKS} && . .venv/bin/activate && pip install --no-index --find-links ${PIP_FIND_LINKS} -r requirements.txt; \ + . /cachi2/cachi2.env && uv sync --locked --no-index --find-links ${PIP_FIND_LINKS} --no-dev --group llslibdev-hermetic; \ else \ uv sync --locked --no-dev --group llslibdev; \ fiThis ensures both build paths use pyproject.toml as the single source of truth for dependencies.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks).tekton/lightspeed-stack-push.yaml(1 hunks)Containerfile(1 hunks)pyproject.toml(2 hunks)requirements.hermetic.in(1 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements.hermetic.in
🚧 Files skipped from review as they are similar to previous changes (5)
- .tekton/lightspeed-stack-push.yaml
- pyproject.toml
- requirements.hermetic.txt
- rpms.lock.yaml
- ubi.repo
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (1)
rpms.in.yaml (1)
1-4: LGTM! Configuration structure is correct.The rpms.in.yaml input file correctly declares the packages (gcc, jq, patch), content origin (./ubi.repo), and target architectures (x86_64, aarch64) for hermetic RPM builds. The structure is valid and appropriate for the hermetic build workflow.
| - name: build-source-image | ||
| value: 'true' | ||
| - name: prefetch-input | ||
| value: '[{"type": "rpm", "path": "."}, {"type": "pip", "path": ".", "allow_binary": "true", "requirements_files": ["requirements.txt", "requirements.hermetic.txt"]}]' | ||
| - name: hermetic | ||
| value: 'true' |
There was a problem hiding this comment.
🧩 Analysis chain
Verify requirements.hermetic.txt is included in Containerfile and properly used.
The prefetch-input parameter references both "requirements.txt" and "requirements.hermetic.txt" in the pip requirements_files array (line 38), but the Containerfile hermetic build path (line 33) only installs from requirements.txt. If requirements.hermetic.txt contains additional hermetic-specific dependencies, they may not be installed.
Run the following to check if requirements.hermetic.txt exists and is used:
🏁 Script executed:
#!/bin/bash
# Check if requirements.hermetic.txt exists and its usage
echo "=== Check if requirements.hermetic.txt exists ==="
if [ -f requirements.hermetic.txt ]; then
echo "✓ requirements.hermetic.txt exists"
echo "First 10 lines:"
head -10 requirements.hermetic.txt
else
echo "✗ requirements.hermetic.txt not found"
fi
echo -e "\n=== Check Containerfile for requirements.hermetic.txt usage ==="
rg -n "requirements\.hermetic\.txt" ContainerfileLength of output: 806
🏁 Script executed:
#!/bin/bash
# Examine Containerfile for pip/requirements usage and build logic
echo "=== Containerfile pip install and requirements handling ==="
rg -n -A3 -B3 "requirements\.txt|pip install" Containerfile
echo -e "\n=== Check Containerfile structure for hermetic build ==="
cat ContainerfileLength of output: 4420
Add requirements.hermetic.txt to Containerfile and use it in the hermetic build path.
Verification confirms the issue: requirements.hermetic.txt exists with pinned dependencies (autogenerated by uv), but the Containerfile never references it. Line 25 only copies requirements.txt, and line 33 (hermetic build path) installs from requirements.txt, not requirements.hermetic.txt. This defeats the purpose of prefetching hermetic-specific dependencies in the Tekton task.
Fix required:
- Add
${LSC_SOURCE_DIR}/requirements.hermetic.txtto the COPY statement (line 25) - Update line 33 to install from requirements.hermetic.txt instead of requirements.txt for reproducible hermetic builds
🤖 Prompt for AI Agents
.tekton/lightspeed-stack-pull-request.yaml (and the referenced Containerfile
around lines 25 and 33): the Containerfile currently only copies and installs
requirements.txt which means the hermetic build path cannot use the pre-fetched
requirements.hermetic.txt; update the COPY at line ~25 to also copy
${LSC_SOURCE_DIR}/requirements.hermetic.txt into the image (same destination as
requirements.txt), and change the hermetic install step at line ~33 to
pip-install from requirements.hermetic.txt instead of requirements.txt so the
hermetic build uses the pinned, reproducible dependency list.
| # (avoid accidental inclusion of local directories or env files or credentials) | ||
| COPY ${LSC_SOURCE_DIR}/src ./src | ||
| COPY ${LSC_SOURCE_DIR}/pyproject.toml ${LSC_SOURCE_DIR}/LICENSE ${LSC_SOURCE_DIR}/README.md ${LSC_SOURCE_DIR}/uv.lock ./ | ||
| COPY ${LSC_SOURCE_DIR}/pyproject.toml ${LSC_SOURCE_DIR}/LICENSE ${LSC_SOURCE_DIR}/README.md ${LSC_SOURCE_DIR}/uv.lock ${LSC_SOURCE_DIR}/requirements.txt ./ |
There was a problem hiding this comment.
🧩 Analysis chain
Verify requirements.txt is properly generated and kept in sync.
The hermetic build path relies on requirements.txt (line 33), but a past review comment noted that the Makefile still uses an outdated pdm export command to generate this file, even though the repo has migrated to uv. This creates a risk of drift between the checked-in requirements.txt and the actual generation method.
Run the following to verify requirements.txt generation is using uv:
🏁 Script executed:
#!/bin/bash
# Check how requirements.txt is generated
echo "=== Check Makefile requirements.txt target ==="
rg -n "requirements\.txt" Makefile -A 2 -B 2
echo -e "\n=== Check header of requirements.txt for generation method ==="
head -5 requirements.txtLength of output: 769
Update Makefile to use uv for requirements.txt generation.
The Makefile still uses pdm export to generate requirements.txt (line 44), but the actual file was generated using uv pip compile (as shown in its header). The Makefile has a "TODO uv migration" comment but hasn't been updated. This creates a critical maintenance issue: running make requirements.txt would regenerate the file using the outdated pdm method, causing potential drift from pyproject.toml. Additionally, the methods use different dependency groups (pdm: --no-extras --without evaluation vs. uv: --group llslibdev-hermetic), which could result in different locked dependencies.
Update line 44 in the Makefile to:
uv pip compile pyproject.toml -o requirements.txt --generate-hashes --group llslibdev-hermetic
🤖 Prompt for AI Agents
In Makefile around line 44, the rule still uses "pdm export" to regenerate
requirements.txt which is inconsistent with the actual file generated by uv;
replace the pdm export invocation with the uv command used to generate the file
so the Makefile runs: uv pip compile pyproject.toml -o requirements.txt
--generate-hashes --group llslibdev-hermetic, ensuring the TODO uv migration is
resolved and the requirements.txt will be recreated with the same tool and
dependency group as the committed lockfile.
|
/retest |
1 similar comment
|
/retest |
Signed-off-by: Haoyu Sun <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
Containerfile (1)
25-36: Hermetic pip prefetch is only partially used—Containerfile ignores requirements.hermetic.txtThe Tekton
prefetch-inputis configured to prefetch bothrequirements.txtandrequirements.hermetic.txt, but the Containerfile only copiesrequirements.txtand the hermetic path installs from that file only. Any hermetic‑specific pins inrequirements.hermetic.txtwon’t be installed, which defeats the purpose of prefetching that file.Consider:
- Extending the COPY to include
${LSC_SOURCE_DIR}/requirements.hermetic.txt, and- Installing from
requirements.hermetic.txt(or from both files, if that’s the intended combined environment) in the hermetic path.This mirrors the earlier review feedback and still needs resolution.
.tekton/lightspeed-stack-pull-request.yaml (2)
30-35: Clean up TODOs: arm64 follow-up and stale on-push noteThe arm64 TODO is fine as a reminder but should be backed by a follow‑up issue before merge; the “todo: change on-push pipeline,too” comment is now stale since
.tekton/lightspeed-stack-push.yamlalready has matching hermetic/prefetch params and should be removed or updated to reference any remaining gap.This was raised earlier and still needs cleanup.
35-40: Prefetching requirements.hermetic.txt without using it in the buildThe
prefetch-inputparameter asks Cachi2 to prefetch dependencies for bothrequirements.txtandrequirements.hermetic.txt, but the Containerfile’s hermetic path still installs only fromrequirements.txt. That means anything present only inrequirements.hermetic.txtis fetched but never installed, undermining the hermetic configuration.Align the Containerfile with this param by copying and installing from
requirements.hermetic.txt(or adjust the prefetch-input to match the actual install behavior).This is the same issue noted in the earlier review and remains unresolved.
🧹 Nitpick comments (2)
.tekton/lightspeed-stack-push.yaml (1)
27-36: Hermetic/prefetch params wiring looks correct; track the arm64 TODO
build-source-image,prefetch-input, andhermeticare correctly passed into the pipeline spec and used by the tasks; the TODO about adding arm64 is fine for WIP, but should be implemented or tracked in a follow-up before relying on multi-arch builds.rpms.lock.yaml (1)
1-355: RPM lockfile looks complete and aligned with rpms.in.yamlBoth
aarch64andx86_64sections include pinned entries forgcc,jq,patchand their required toolchain dependencies from the expected UBI repos, matching thepackages/archesdeclared inrpms.in.yaml. Treat this file as generated and avoid manual edits; regenerate it when updating UBI or compiler/tooling requirements.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks).tekton/lightspeed-stack-push.yaml(1 hunks)Containerfile(1 hunks)pyproject.toml(2 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- requirements.hermetic.txt
- ubi.repo
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (7)
rpms.in.yaml (1)
1-4: rpms.in.yaml is consistent and matches lockfile/arch expectations
packagesandarchesalign with entries inrpms.lock.yaml, andcontentOrigin.repofilespoints at the expected UBI repo file; config looks good as a lockfile input.Containerfile (1)
14-18: Confirm gcc dnf install works with hermetic/rpm-prefetch configurationWith
hermetic: 'true'and RPM prefetch configured (viarpms.in.yaml/rpms.lock.yaml), thisdnfinstall still targets the public UBI repos. Please double‑check that, in your Konflux setup, hermetic builds either (a) allow this access via a prefetched local mirror, or (b) transparently rewrite these repos to the prefetched content; otherwise this step may fail once network isolation is enforced and might need to be switched to use the prefetched RPM repo.pyproject.toml (5)
91-94: Approve: New pypi-default index added.This index enables the torch source configuration to route different groups to different PyTorch variants.
21-21: LGTM: Python version constraint allows both 3.12 and 3.13.The constraint
>=3.12,<3.14correctly supports both Python 3.12 and 3.13, aligning with documented support and CI test matrix.
100-106: Syntax is valid and correctly formatted.UV supports a [tool.uv] conflicts setting where you can declare group-level conflict matrices using { group = "name" } entries. The conflicts definition in your pyproject.toml matches the documented format and correctly prevents co-installation of the llslibdev and llslibdev-hermetic groups.
177-220: The core claim in this review is incorrect.The two groups are not duplicated verbatim as claimed.
llslibdev-hermeticincludes an additional dependency not present inllslibdev:litellm>=1.75.5.post1(marked for "API inference: remote::gemini"). This represents an intentional functional difference between the groups, not accidental duplication.While the torch platform marker difference (line 167 vs. line 211) is real and should be clarified, the review's primary concern about "43 lines duplicated verbatim creating maintenance burden" mischaracterizes the actual relationship between these groups.
The groups serve different purposes and contain different dependencies by design. The referenced commit be34f46 also cannot be found in the git history.
Likely an incorrect or invalid review comment.
96-99: The review comment is invalid because it cites a non-existent commit and mischaracterizes an intentional configuration change.The cited commit
f35507adoes not exist in the repository history. The actual torch configuration change was introduced in commit76e5ee1(konflux: hermetic build, Oct 31, 2025) and is documented inline in pyproject.toml with the comment: "the same as llslibdev, just using default index." The hermetic dependency group deliberately usespypi-defaultto resolve the GPU variant, as documented in the pyproject.toml file. This is a recent, intentional architectural decision, not a regression requiring correction.Likely an incorrect or invalid review comment.
|
/retest |
2 similar comments
|
/retest |
|
/retest |
1 participant
Do Not Merge!
Description
activate hermetic build
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit