install RPMs for all the build architectures to ensure the same versions of openssl and other RPMs packages are for all the architectures - #71
Conversation
Add konflux dockerfiles
Add konflux dockerfile
Add required label to satify konflux conforma checks
Signed-off-by: Varad Ahirwadkar <varad.ahirwadkar1@ibm.com>
Signed-off-by: Varad Ahirwadkar <varad.ahirwadkar1@ibm.com>
Sync guardrails HF runtime changes with Dockerfile.konflux.hf
Update Konflux dockerfiles for new directory structure
sync: Pin urllib3 to 2.6.2
Signed-off-by: Nishidha Panpaliya <nishidha.panpaliya@partner.ibm.com>
Fixed Power/Z build
…s3.2 Cherrypick: Add multithreaded prometheus setup to Konflux dockerfile for HF detector
Sync sentencepiece versions between upstream main and downstream rhoai-3.2 branches
Reviewer's GuideAdds Konflux/Tekton multi-arch build PipelineRun definitions and documentation for guardrails-detectors, aligning push and PR builds (including rhoai-3.2) with konflux-central and preparing Dockerfiles for built-in and HuggingFace runtime images so all architectures share consistent RPM (e.g., OpenSSL) versions. Sequence diagram for pull request triggered Konflux build for built-in detectorsequenceDiagram
actor Developer
participant GitHub
participant PipelinesAsCode
participant Tekton as TektonController
participant KonfluxPipeline as "multi-arch-container-build"
participant Quay as "quay.io"
Developer->>GitHub: Open pull request to target_branch
Developer->>GitHub: Comment /build-konflux built-in-detector
GitHub-->>PipelinesAsCode: Webhook pull_request comment event
PipelinesAsCode->>PipelinesAsCode: Match on-comment ^/build-konflux built-in-detector
PipelinesAsCode->>Tekton: Create PipelineRun odh-built-in-detector-on-pull-request
Tekton->>KonfluxPipeline: Resolve pipelineRef via git resolver
KonfluxPipeline->>KonfluxPipeline: Run multi-arch build using Dockerfile.konflux.builtIn
KonfluxPipeline->>Quay: Push image quay.io/rhoai/pull-request-pipelines:odh-built-in-detector-<revision>
KonfluxPipeline->>Quay: Tag pr-<pull_request_number>-into-<target_branch>
Quay-->>Developer: Image available for validation (temporary, expires-after 5d)
Flow diagram for Tekton configuration sync from konflux-central to component .tekton directoryflowchart LR
A[konflux-central repo
path pipelineruns/guardrails-detectors/.tekton] --> B[Automation sync process]
B --> C[guardrails-detectors repo
.tekton directory]
subgraph EditProcess[How to update Tekton pipelines]
C1[Clone konflux-central] --> C2[Checkout rhoai-3.2 branch]
C2 --> C3[Edit Tekton YAML under pipelineruns/guardrails-detectors/.tekton]
C3 --> C4[Commit and push to rhoai-3.2]
C4 --> B
end
C -.->|files overwritten on next sync| CNote[Do not edit .tekton directly in component repo]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a .tekton README, five Tekton PipelineRun manifests for PR- and push-triggered multi-architecture builds referencing a remote pipeline via a git resolver and git-auth workspaces, and two multi-stage Dockerfiles for built-in and HuggingFace detector images. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (Git)
participant Konflux as konflux-central (resolver repo)
participant Tekton as Tekton Controller
participant Pipeline as PipelineRun
participant Tasks as Pipeline Tasks
participant Registry as Image Registry
Dev->>Konflux: push or open PR (repo changes)
Konflux->>Tekton: git-resolver supplies pipelineRef & pipeline manifest
Tekton->>Pipeline: create PipelineRun (params, workspaces, SA)
Pipeline->>Tasks: execute build tasks (multi-arch build)
Tasks->>Registry: push built images (tags, platforms)
Tekton->>Dev: update status/notifications
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
.tekton/odh-built-in-detector-v3-2-push.yaml,metadata.creationTimestamp:is present with no value, which is unusual for a template and should either be removed or populated correctly to avoid YAML parsing or tooling issues. - The
build-platformsentries includelinux-m2xlarge/arm64alongsidelinux/x86_64,linux/ppc64le, etc.; double-check that this nonstandardlinux-m2xlarge/arm64identifier is intentional and supported by the target pipeline tooling, and align the platform strings across the new PipelineRuns if possible.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `.tekton/odh-built-in-detector-v3-2-push.yaml`, `metadata.creationTimestamp:` is present with no value, which is unusual for a template and should either be removed or populated correctly to avoid YAML parsing or tooling issues.
- The `build-platforms` entries include `linux-m2xlarge/arm64` alongside `linux/x86_64`, `linux/ppc64le`, etc.; double-check that this nonstandard `linux-m2xlarge/arm64` identifier is intentional and supported by the target pipeline tooling, and align the platform strings across the new PipelineRuns if possible.
## Individual Comments
### Comment 1
<location> `.tekton/odh-built-in-detector-v3-2-push.yaml:15` </location>
<code_context>
+ event == "push"
+ && target_branch == "rhoai-3.2"
+ && ( "detectors/**".pathChanged() || ".tekton/odh-built-in-detector-v3-2-push.yaml".pathChanged() )
+ creationTimestamp:
+ labels:
+ appstudio.openshift.io/application: rhoai-v3-2
</code_context>
<issue_to_address>
**issue (bug_risk):** Including an empty creationTimestamp in metadata is unnecessary and may conflict with server-managed fields.
`metadata.creationTimestamp` is a server-populated field and shouldn’t be included in manifests. Keeping it, even empty, can trigger validation/admission failures on some clusters. Please remove this field and let the API server set it automatically.
</issue_to_address>
### Comment 2
<location> `.tekton/odh-guardrails-detector-huggingface-runtime-v3-2-push.yaml:49-53` </location>
<code_context>
+ value: false
+ - name: build-image-index
+ value: true
+ - name: build-platforms
+ value:
+ - linux/x86_64
+ - linux/ppc64le
+ - linux-m2xlarge/arm64
+ - linux/s390x
+ - name: image-expires-after
</code_context>
<issue_to_address>
**question (bug_risk):** The platform string `linux-m2xlarge/arm64` looks inconsistent with standard buildx OS/arch notation.
Other platforms use the `os/arch` format (`linux/x86_64`, `linux/ppc64le`, `linux/s390x`), but this one is `linux-m2xlarge/arm64`. Unless Konflux explicitly requires this custom value, it’s likely meant to be `linux/arm64` (or similar). If it is intentional, please document it or centralize the definition, since this value is duplicated in other PipelineRuns.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| event == "push" | ||
| && target_branch == "rhoai-3.2" | ||
| && ( "detectors/**".pathChanged() || ".tekton/odh-built-in-detector-v3-2-push.yaml".pathChanged() ) | ||
| creationTimestamp: |
There was a problem hiding this comment.
issue (bug_risk): Including an empty creationTimestamp in metadata is unnecessary and may conflict with server-managed fields.
metadata.creationTimestamp is a server-populated field and shouldn’t be included in manifests. Keeping it, even empty, can trigger validation/admission failures on some clusters. Please remove this field and let the API server set it automatically.
| - name: build-platforms | ||
| value: | ||
| - linux/x86_64 | ||
| - linux/ppc64le | ||
| - linux-m2xlarge/arm64 |
There was a problem hiding this comment.
question (bug_risk): The platform string linux-m2xlarge/arm64 looks inconsistent with standard buildx OS/arch notation.
Other platforms use the os/arch format (linux/x86_64, linux/ppc64le, linux/s390x), but this one is linux-m2xlarge/arm64. Unless Konflux explicitly requires this custom value, it’s likely meant to be linux/arm64 (or similar). If it is intentional, please document it or centralize the definition, since this value is duplicated in other PipelineRuns.
…ons of openssl and other RPMs packages are for all the architectures
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @detectors/Dockerfile.konflux.hf:
- Around line 26-27: Fix the typo in the Dockerfile comment: change the comment
"Buildinf torch for ppc64le" to "Building torch for ppc64le" (the line
immediately above the FROM base as torch-builder instruction).
- Around line 86-95: The RUN shell block has a trailing space after the line
with "source /opt/rh/gcc-toolset-13/enable
&& \" which leaves characters after the line-continuation backslash and can
break shell parsing; remove the trailing whitespace so the backslash is the last
character on that line (or join that clause onto the previous/next line), and
while editing consider replacing the inline cmake-from-source sequence (the
curl/./bootstrap/make/make install steps in the same RUN) with a packaged or
prebuilt cmake binary to reduce build time and complexity (references: the RUN
block, the "source /opt/rh/gcc-toolset-13/enable" line and the cmake
download/build commands).
- Around line 5-24: The install of openssl-devel is only present in the
ppc64le/s390x branch, causing inconsistent OpenSSL RPM versions across
architectures; update the Dockerfile RUN blocks to install openssl-devel for all
architectures by adding openssl-devel to the main microdnf install invocation
(the first RUN microdnf install -y ... line) and ensure it's also present in the
architecture-specific microdnf install lists (the branches that check TARGETARCH
for ppc64le and s390x) so every code path that runs microdnf install includes
openssl-devel.
- Around line 91-92: There are two conflicting cmake installs: the pip step "pip
install 'cmake<4'" and the source build of "cmake-3.31.0"
(curl/tar/bootstrap/make). Either replace the source-build block with
installation of the RHEL AppStream cmake RPM (if cmake 3.20.2 is sufficient) to
speed builds and support ppc64le/s390x, or if 3.31.0 is strictly required, keep
the source-build but add a clear comment above the curl/tar line explaining why
3.31.0 is necessary and update/remove the pip "cmake<4" line to avoid ambiguity
(ensure only one cmake provider is installed and tested).
🧹 Nitpick comments (4)
detectors/Dockerfile.konflux.builtIn (1)
28-32: Consider using a heredoc or copying a script file for maintainability.Creating the wrapper script via multiple
echocommands works but is harder to maintain. A heredoc or copying a pre-existing script file would be cleaner. However, this is a minor concern given the backwards compatibility rationale.♻️ Optional: Use heredoc syntax
# for backwards compatibility with existing k8s deployment configs -RUN mkdir /app/bin &&\ - echo '#!/bin/bash' > /app/bin/regex-detector &&\ - echo "uvicorn app:app --workers 4 --host 0.0.0.0 --port 8080 --log-config /app/detectors/common/log_conf.yaml" >> /app/bin/regex-detector &&\ - chmod +x /app/bin/regex-detector +RUN mkdir /app/bin && \ + cat <<'EOF' > /app/bin/regex-detector +#!/bin/bash +uvicorn app:app --workers 4 --host 0.0.0.0 --port 8080 --log-config /app/detectors/common/log_conf.yaml +EOF + chmod +x /app/bin/regex-detectordetectors/Dockerfile.konflux.hf (1)
14-16: Redundantwheelinstallation.
wheelis already installed on line 6 (pip install --upgrade --no-cache-dir pip wheel). The second installation on line 16 is redundant.♻️ Remove redundant wheel installation
elif [ "$TARGETARCH" = "s390x" ]; then \ microdnf install -y --nodocs \ git gcc-toolset-13 make wget unzip rust cargo \ gcc-gfortran openblas-devel pkgconfig && \ - pip install --upgrade --no-cache-dir 'cmake<4' \ - wheel ; \ + pip install --upgrade --no-cache-dir 'cmake<4' ; \ fi && \.tekton/odh-built-in-detector-pull-request.yaml (1)
19-51: Consider adding pipeline timeouts for consistency.Unlike the push pipeline (
odh-guardrails-detector-huggingface-runtime-v3-2-push.yamllines 22-24), this PR pipeline doesn't define timeouts. Long-running builds for multi-arch (especially ppc64le/s390x with source builds of torch/cmake) could hang indefinitely.♻️ Add timeouts
spec: + timeouts: + pipeline: 8h + tasks: 4h params: - name: git-url.tekton/odh-built-in-detector-v3-2-push.yaml (1)
51-65: Consider standardizing template variable syntax.There's inconsistent whitespace in template variables throughout the file:
{{revision}}(no spaces) on lines 27, 30, 36{{ target_branch }}(with spaces) on line 57{{ git_auth_secret }}(with spaces) on line 65While Pipelines as Code typically tolerates both styles, standardizing on one format improves maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.tekton/README.md.tekton/odh-built-in-detector-pull-request.yaml.tekton/odh-built-in-detector-v3-2-push.yaml.tekton/odh-guardrails-detector-huggingface-runtime-pull-request.yaml.tekton/odh-guardrails-detector-huggingface-runtime-v3-2-push.yamldetectors/Dockerfile.konflux.builtIndetectors/Dockerfile.konflux.hf
🔇 Additional comments (12)
.tekton/README.md (1)
1-39: LGTM! Clear documentation for the auto-sync workflow.The README effectively communicates the workflow for modifying Tekton files through the konflux-central repository, with actionable step-by-step instructions.
detectors/Dockerfile.konflux.builtIn (1)
1-6: LGTM! Base stage with proper package updates.The base stage correctly runs
microdnf update -ybefore installing packages, which aligns with the PR objective of ensuring consistent package versions across architectures.detectors/Dockerfile.konflux.hf (1)
99-122: LGTM! Final stage with proper app configuration and labels.The application setup, Prometheus multiprocess directory configuration, and comprehensive container labels are well-defined.
.tekton/odh-built-in-detector-pull-request.yaml (1)
42-47: LGTM! Multi-arch build platforms properly configured.The build platforms cover all target architectures (x86_64, ppc64le, arm64, s390x), which aligns with the PR objective of ensuring consistent package versions across all build architectures.
.tekton/odh-guardrails-detector-huggingface-runtime-v3-2-push.yaml (3)
11-14: CEL expression well-crafted for selective triggering.The on-cel-expression correctly filters for push events to the
rhoai-3.2branch while avoiding unnecessary triggers when only other.tekton/files change. This prevents redundant pipeline runs.
22-24: LGTM! Appropriate timeouts for complex multi-arch builds.The 8-hour pipeline timeout and 4-hour task timeout are reasonable given the source builds of torch and OpenBLAS required for ppc64le/s390x architectures.
49-54: LGTM! Build platforms consistent with PR pipeline.The multi-arch build platforms (x86_64, ppc64le, arm64, s390x) are consistent with the other pipeline manifests and the Dockerfile's architecture-specific logic.
.tekton/odh-built-in-detector-v3-2-push.yaml (2)
1-21: LGTM - Well-structured push pipeline metadata.The CEL expression correctly triggers on pushes to
rhoai-3.2with appropriate path filtering. The annotations and labels follow the expected Konflux/App Studio conventions.
23-50: LGTM - Multi-arch build parameters are correctly configured.The build platforms cover all required architectures (x86_64, arm64, ppc64le, s390x), and the
linux-m2xlarge/arm64format is the correct Konflux convention for specifying the machine type for ARM builds..tekton/odh-guardrails-detector-huggingface-runtime-pull-request.yaml (3)
1-18: LGTM - Well-configured PR workflow.Good use of PR-specific features:
cancel-in-progress: "true"prevents redundant builds on rapid commitson-commenttrigger allows manual rebuilds via/build-konflux guardrails-detector-hf-runtimemax-keep-runs: "3"provides appropriate cleanup
42-49: LGTM - Multi-arch builds with appropriate PR image expiration.The 5-day expiration for PR images is appropriate to avoid accumulating stale images. All required architectures are covered.
Minor note: The platform ordering differs from the push pipeline (this file has ppc64le before arm64). This is functionally equivalent but you may want to align ordering across manifests for consistency.
50-65: LGTM - Pipeline reference and workspace configuration are correct.The git resolver correctly points to konflux-central for the multi-arch-container-build pipeline, and the git-auth workspace is properly configured.
Same minor template syntax inconsistency noted in the other file applies here as well.
| # Buildinf torch for ppc64le | ||
| FROM base as torch-builder |
There was a problem hiding this comment.
Typo in comment.
"Buildinf" should be "Building".
📝 Fix typo
-# Buildinf torch for ppc64le
+# Building torch for ppc64le📝 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.
| # Buildinf torch for ppc64le | |
| FROM base as torch-builder | |
| # Building torch for ppc64le | |
| FROM base as torch-builder |
🤖 Prompt for AI Agents
In @detectors/Dockerfile.konflux.hf around lines 26 - 27, Fix the typo in the
Dockerfile comment: change the comment "Buildinf torch for ppc64le" to "Building
torch for ppc64le" (the line immediately above the FROM base as torch-builder
instruction).
| RUN export CURDIR=$(pwd) && \ | ||
| if [ "$TARGETARCH" = "ppc64le" ] || [ "$TARGETARCH" = "s390x" ]; then \ | ||
| source /opt/rh/gcc-toolset-13/enable && \ | ||
| microdnf install tar openssl-devel libatomic gcc-toolset-13-libatomic-devel -y && \ | ||
| export OPENSSL_ROOT_DIR=/usr && \ | ||
| curl -L https://cmake.org/files/v3.31/cmake-3.31.0.tar.gz > /tmp/cmake-3.31.0.tar.gz && \ | ||
| cd /tmp/ && tar -xzf /tmp/cmake-3.31.0.tar.gz -C /tmp && cd /tmp/cmake-3.31.0 && ./bootstrap && make && make install && cd .. && rm -rf cmake-3.31.0 && \ | ||
| cd $CURDIR; \ | ||
| fi && \ | ||
| pip install --no-cache-dir -r requirements.txt |
There was a problem hiding this comment.
Trailing whitespace on line 88 may cause build failures.
Line 88 appears to have trailing whitespace after the backslash (&& \). In shell, trailing whitespace after a backslash continuation can cause parsing errors. Additionally, the inline cmake build from source is complex and adds significant build time.
🔧 Fix trailing whitespace
RUN export CURDIR=$(pwd) && \
if [ "$TARGETARCH" = "ppc64le" ] || [ "$TARGETARCH" = "s390x" ]; then \
- source /opt/rh/gcc-toolset-13/enable && \
+ source /opt/rh/gcc-toolset-13/enable && \
microdnf install tar openssl-devel libatomic gcc-toolset-13-libatomic-devel -y && \📝 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.
| RUN export CURDIR=$(pwd) && \ | |
| if [ "$TARGETARCH" = "ppc64le" ] || [ "$TARGETARCH" = "s390x" ]; then \ | |
| source /opt/rh/gcc-toolset-13/enable && \ | |
| microdnf install tar openssl-devel libatomic gcc-toolset-13-libatomic-devel -y && \ | |
| export OPENSSL_ROOT_DIR=/usr && \ | |
| curl -L https://cmake.org/files/v3.31/cmake-3.31.0.tar.gz > /tmp/cmake-3.31.0.tar.gz && \ | |
| cd /tmp/ && tar -xzf /tmp/cmake-3.31.0.tar.gz -C /tmp && cd /tmp/cmake-3.31.0 && ./bootstrap && make && make install && cd .. && rm -rf cmake-3.31.0 && \ | |
| cd $CURDIR; \ | |
| fi && \ | |
| pip install --no-cache-dir -r requirements.txt | |
| RUN export CURDIR=$(pwd) && \ | |
| if [ "$TARGETARCH" = "ppc64le" ] || [ "$TARGETARCH" = "s390x" ]; then \ | |
| source /opt/rh/gcc-toolset-13/enable && \ | |
| microdnf install tar openssl-devel libatomic gcc-toolset-13-libatomic-devel -y && \ | |
| export OPENSSL_ROOT_DIR=/usr && \ | |
| curl -L https://cmake.org/files/v3.31/cmake-3.31.0.tar.gz > /tmp/cmake-3.31.0.tar.gz && \ | |
| cd /tmp/ && tar -xzf /tmp/cmake-3.31.0.tar.gz -C /tmp && cd /tmp/cmake-3.31.0 && ./bootstrap && make && make install && cd .. && rm -rf cmake-3.31.0 && \ | |
| cd $CURDIR; \ | |
| fi && \ | |
| pip install --no-cache-dir -r requirements.txt |
🤖 Prompt for AI Agents
In @detectors/Dockerfile.konflux.hf around lines 86 - 95, The RUN shell block
has a trailing space after the line with "source /opt/rh/gcc-toolset-13/enable
&& \" which leaves characters after the line-continuation backslash and can
break shell parsing; remove the trailing whitespace so the backslash is the last
character on that line (or join that clause onto the previous/next line), and
while editing consider replacing the inline cmake-from-source sequence (the
curl/./bootstrap/make/make install steps in the same RUN) with a packaged or
prebuilt cmake binary to reduce build time and complexity (references: the RUN
block, the "source /opt/rh/gcc-toolset-13/enable" line and the cmake
download/build commands).
| curl -L https://cmake.org/files/v3.31/cmake-3.31.0.tar.gz > /tmp/cmake-3.31.0.tar.gz && \ | ||
| cd /tmp/ && tar -xzf /tmp/cmake-3.31.0.tar.gz -C /tmp && cd /tmp/cmake-3.31.0 && ./bootstrap && make && make install && cd .. && rm -rf cmake-3.31.0 && \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n detectors/Dockerfile.konflux.hf | head -120Repository: trustyai-explainability/guardrails-detectors
Length of output: 6859
🏁 Script executed:
# Check what cmake package versions are typically available in RHEL 9 repos
rg "cmake" detectors/ -i --type-list | head -20Repository: trustyai-explainability/guardrails-detectors
Length of output: 477
🌐 Web query:
RHEL 9 cmake package availability ppc64le s390x architecture
💡 Result:
Yes — RHEL 9 ships CMake (3.20.2 in base RHEL‑9) and the RPM builds for ppc64le and s390x are available in the EL9 AppStream repositories. [1][2]
Sources:
- RHEL 9 release notes (CMake 3.20.2 in RHEL 9). [1]
- EL9 AppStream package listings showing cmake RPMs for ppc64le and s390x (cmake-3.20.2 and later). [2]
Clarify cmake version requirement and consider using RHEL package.
The Dockerfile uses two different cmake installations: pip install 'cmake<4' (Python wrapper, lines 10/15) and a source build of cmake 3.31.0 (lines 91-92). RHEL 9 AppStream repositories provide prebuilt cmake 3.20.2 RPMs for both ppc64le and s390x architectures.
If cmake 3.20.2 is sufficient for the build, use the RHEL package instead of building from source to reduce build time. If cmake 3.31.0 is specifically required, add a comment explaining why the source build and this particular version are necessary.
🤖 Prompt for AI Agents
In @detectors/Dockerfile.konflux.hf around lines 91 - 92, There are two
conflicting cmake installs: the pip step "pip install 'cmake<4'" and the source
build of "cmake-3.31.0" (curl/tar/bootstrap/make). Either replace the
source-build block with installation of the RHEL AppStream cmake RPM (if cmake
3.20.2 is sufficient) to speed builds and support ppc64le/s390x, or if 3.31.0 is
strictly required, keep the source-build but add a clear comment above the
curl/tar line explaining why 3.31.0 is necessary and update/remove the pip
"cmake<4" line to avoid ambiguity (ensure only one cmake provider is installed
and tested).
…_ensure_same_rpm_packages_are_installed_for_all_arches install RPMs for all the build architectures to ensure the same versi…
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @detectors/Dockerfile.konflux.hf:
- Around line 87-89: The RUN line unconditionally sources
/opt/rh/gcc-toolset-13/enable and installs gcc-toolset-13-libatomic-devel which
only exists on ppc64le/s390x; make the sourcing and that package installation
conditional on the architecture: detect the arch (e.g., via uname -m or
$TARGETARCH), only source /opt/rh/gcc-toolset-13/enable when it exists, and
include gcc-toolset-13-libatomic-devel in the microdnf install list only for
those architectures; leave the other packages (tar, openssl-devel, libatomic,
microdnf/gcc) installed unconditionally so non-ppc/s390 builds succeed.
♻️ Duplicate comments (2)
detectors/Dockerfile.konflux.hf (2)
26-27: Typo in comment."Buildinf" should be "Building".
91-98: Trailing whitespace and cmake concerns already flagged.Line 88 has trailing whitespace after the backslash. The cmake version inconsistency (pip-installed cmake vs source-built 3.31.0) was also previously noted.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
detectors/Dockerfile.konflux.hf
🔇 Additional comments (7)
detectors/Dockerfile.konflux.hf (7)
1-19: Base stage setup looks correct for architecture-specific dependencies.The conditional package installation for ppc64le and s390x provides the necessary build tools. Note that
gcc-toolset-13is intentionally installed only for ppc64le/s390x where it's needed for building PyTorch and OpenBLAS from source.
21-24: LGTM!Correctly installs prebuilt PyTorch wheels for x86_64/arm64 while deferring to the source build for ppc64le/s390x.
37-45: LGTM!The PyTorch source build correctly enables gcc-toolset-13 and uses appropriate build parameters for ppc64le/s390x.
47-64: LGTM!OpenBLAS source build is correctly scoped to ppc64le only with appropriate Power9 target flags.
66-80: LGTM!Efficient use of bind mounts for wheel installation and correct architecture gating for OpenBLAS.
100-115: LGTM!Final image setup is clean with appropriate directory structure, environment configuration, and uvicorn runtime command.
119-125: LGTM!Image labels follow Red Hat OpenShift conventions with appropriate component identification and license reference.
| # install RPMs for all the build architectures to ensure the same versions of openssl and other RPMs packages are for all the architectures | ||
| RUN source /opt/rh/gcc-toolset-13/enable && \ | ||
| microdnf install tar openssl-devel libatomic gcc-toolset-13-libatomic-devel -y |
There was a problem hiding this comment.
Critical: Unconditional gcc-toolset-13 sourcing will fail on x86_64/arm64.
Line 88 sources /opt/rh/gcc-toolset-13/enable unconditionally, but gcc-toolset-13 is only installed for ppc64le (line 9) and s390x (line 13). This will cause build failures on x86_64 and arm64 architectures where the file doesn't exist.
Additionally, gcc-toolset-13-libatomic-devel on line 89 may not be installable on architectures where gcc-toolset-13 repo isn't enabled.
🐛 Proposed fix: Make gcc-toolset-13 sourcing conditional
# install RPMs for all the build architectures to ensure the same versions of openssl and other RPMs packages are for all the architectures
-RUN source /opt/rh/gcc-toolset-13/enable && \
- microdnf install tar openssl-devel libatomic gcc-toolset-13-libatomic-devel -y
-
+RUN if [ -f /opt/rh/gcc-toolset-13/enable ]; then \
+ source /opt/rh/gcc-toolset-13/enable; \
+ fi && \
+ if [ "$TARGETARCH" = "ppc64le" ] || [ "$TARGETARCH" = "s390x" ]; then \
+ microdnf install tar openssl-devel libatomic gcc-toolset-13-libatomic-devel -y; \
+ else \
+ microdnf install tar openssl-devel libatomic -y; \
+ fi📝 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.
| # install RPMs for all the build architectures to ensure the same versions of openssl and other RPMs packages are for all the architectures | |
| RUN source /opt/rh/gcc-toolset-13/enable && \ | |
| microdnf install tar openssl-devel libatomic gcc-toolset-13-libatomic-devel -y | |
| # install RPMs for all the build architectures to ensure the same versions of openssl and other RPMs packages are for all the architectures | |
| RUN if [ -f /opt/rh/gcc-toolset-13/enable ]; then \ | |
| source /opt/rh/gcc-toolset-13/enable; \ | |
| fi && \ | |
| if [ "$TARGETARCH" = "ppc64le" ] || [ "$TARGETARCH" = "s390x" ]; then \ | |
| microdnf install tar openssl-devel libatomic gcc-toolset-13-libatomic-devel -y; \ | |
| else \ | |
| microdnf install tar openssl-devel libatomic -y; \ | |
| fi |
🤖 Prompt for AI Agents
In @detectors/Dockerfile.konflux.hf around lines 87 - 89, The RUN line
unconditionally sources /opt/rh/gcc-toolset-13/enable and installs
gcc-toolset-13-libatomic-devel which only exists on ppc64le/s390x; make the
sourcing and that package installation conditional on the architecture: detect
the arch (e.g., via uname -m or $TARGETARCH), only source
/opt/rh/gcc-toolset-13/enable when it exists, and include
gcc-toolset-13-libatomic-devel in the microdnf install list only for those
architectures; leave the other packages (tar, openssl-devel, libatomic,
microdnf/gcc) installed unconditionally so non-ppc/s390 builds succeed.
…me versions of openssl and other RPMs packages are for all the architectures" This reverts commit b98b878.
…oai-3.2_ensure_same_rpm_packages_are_installed_for_all_arches Revert "install RPMs for all the build architectures to ensure the sa…
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@detectors/Dockerfile.konflux.hf`:
- Line 35: The ENV line using "$HOME" may not resolve at build time; replace the
use of $HOME in the ENV declaration (the ENV PATH line) with an explicit
absolute path (e.g., /root/.cargo/bin) or set a deterministic HOME earlier, so
PATH is defined using a concrete directory rather than "$HOME" which can be
empty during Docker build.
♻️ Duplicate comments (3)
detectors/Dockerfile.konflux.hf (3)
26-27: Typo in comment."Buildinf" should be "Building".
📝 Fix typo
-# Buildinf torch for ppc64le +# Building torch for ppc64le
91-92: Clarify cmake version requirement and consider using RHEL package.Two different cmake installations exist:
pip install 'cmake<4'(lines 10/15) and a source build of cmake 3.31.0. If cmake 3.20.2 (available in RHEL 9 repos) is sufficient, use the RPM package to reduce build time. If 3.31.0 is specifically required, add a comment explaining why.
86-95: Trailing whitespace on line 88 may cause build failures.Line 88 appears to have trailing whitespace after the backslash (
&& \). In shell, trailing whitespace after a backslash continuation can break parsing.🔧 Fix trailing whitespace
RUN export CURDIR=$(pwd) && \ if [ "$TARGETARCH" = "ppc64le" ] || [ "$TARGETARCH" = "s390x" ]; then \ - source /opt/rh/gcc-toolset-13/enable && \ + source /opt/rh/gcc-toolset-13/enable && \ microdnf install tar openssl-devel libatomic gcc-toolset-13-libatomic-devel -y && \
🧹 Nitpick comments (1)
detectors/Dockerfile.konflux.hf (1)
114-114: Remove or document the commented-out gunicorn command.This appears to be dead code or a debug artifact. If it's an alternative configuration, consider documenting when it should be used. Otherwise, remove it to keep the Dockerfile clean.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
detectors/Dockerfile.konflux.hf
🔇 Additional comments (6)
detectors/Dockerfile.konflux.hf (6)
1-19: LGTM!The base stage setup correctly handles architecture-conditional package installation with appropriate cleanup. The Python version verification at the end is a good sanity check.
22-24: LGTM!Correct conditional logic to install prebuilt PyTorch wheels only for architectures that have them available (x86_64/arm64).
47-64: LGTM!The OpenBLAS builder stage correctly handles the ppc64le-specific source build with appropriate compiler flags. The s390x architecture uses the system
openblas-develpackage instead (installed in base stage).
66-80: LGTM!The builder stage correctly uses bind mounts for efficient wheel installation and handles architecture-specific OpenBLAS installation. The pattern is appropriate for multi-arch builds.
99-112: LGTM!The final stage setup is well-structured with proper working directory, application file copying, and Prometheus multiproc directory configuration. The uvicorn command with log configuration is appropriate.
116-122: LGTM!The image labels are comprehensive and follow Red Hat container image conventions with appropriate component names, descriptions, and license terms.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| ARG TARGETARCH | ||
|
|
||
| ENV TORCH_VERSION=2.6.0 | ||
| ENV PATH="$HOME/.cargo/bin:$PATH" |
There was a problem hiding this comment.
$HOME may not resolve correctly in ENV instruction.
In Docker's ENV instruction, $HOME may be empty or undefined since it's evaluated at build time before any shell context. Use an explicit path instead.
🔧 Proposed fix
-ENV PATH="$HOME/.cargo/bin:$PATH"
+ENV PATH="/root/.cargo/bin:$PATH"🤖 Prompt for AI Agents
In `@detectors/Dockerfile.konflux.hf` at line 35, The ENV line using "$HOME" may
not resolve at build time; replace the use of $HOME in the ENV declaration (the
ENV PATH line) with an explicit absolute path (e.g., /root/.cargo/bin) or set a
deterministic HOME earlier, so PATH is defined using a concrete directory rather
than "$HOME" which can be empty during Docker build.
11 participants
Summary by Sourcery
Add Konflux multi-architecture build pipelines and Dockerfiles for guardrails detectors and document the Tekton configuration sync process.
New Features:
CI:
Documentation:
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.