Skip to content

Conversation

@jiridanek
Copy link
Member

@jiridanek jiridanek commented Nov 7, 2025

Carrying on with

Description

Reformat inline bash commands into HEREDOC blocks, so they can be later extracted to separate script files.

How Has This Been Tested?

Self checklist (all need to be checked):

  • Ensure that you have run make test (gmake on macOS) before asking for review
  • Changes to everything except Dockerfile.konflux files should be done in odh/notebooks and automatically synced to rhds/notebooks. For Konflux-specific changes, modify Dockerfile.konflux files directly in rhds/notebooks as these require special attention in the downstream repository and flow to the upcoming RHOAI release.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Summary by CodeRabbit

  • Refactor
    • Consolidated and standardized shell execution in multiple runtime build scripts to improve error handling, consistency, and build robustness.
    • Isolated architecture-specific build steps into clearer, self-contained sequences and unified package/wheel handling flows.
    • No changes to runtime behavior, public interfaces, or user-facing functionality; end-user experience remains unchanged.

@openshift-ci openshift-ci bot requested review from atheo89 and dibryant November 7, 2025 14:06
@github-actions github-actions bot added the review-requested GitHub Bot creates notification on #pr-review-ai-ide-team slack channel label Nov 7, 2025
@openshift-ci openshift-ci bot added the size/xl label Nov 7, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 7, 2025

Walkthrough

Convert multiple Dockerfile RUN chains into /bin/bash heredoc blocks with strict shell options (set -Eeuxo pipefail), reorganizing architecture-specific build sequences and consolidating installs, cleanup, and permission fixes within those heredoc contexts.

Changes

Cohort / File(s) Summary
Datascience runtime
runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu
Replaced many inline RUN chains with /bin/bash <<'EOF' heredocs, added set -Eeuxo pipefail, moved arch-specific flows (s390x, ppc64le, ...) into isolated heredocs, consolidated wheel build/install and env handling, and introduced explicit temp wheel directories.
Minimal runtime
runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu
Replaced chained RUNs with heredocs that compute ARCH, conditionally extend package lists for s390x/ppc64le, run dnf installs, and perform pip installs and permission fixes under strict shell options.
PyTorch runtimes
runtimes/pytorch+llmcompressor/ubi9-python-3.12/Dockerfile.cuda, runtimes/pytorch/ubi9-python-3.12/Dockerfile.cuda
Converted dnf and pip RUN sequences into /bin/bash heredocs with set -Eeuxo pipefail, added echo lines, and preserved package installs, pip installs, chmod and fix-permissions steps inside the heredocs.
ROCm runtimes
runtimes/rocm-pytorch/ubi9-python-3.12/Dockerfile.rocm, runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm
Wrapped prior RUN sequences in heredocs, added strict shell options and explicit echo/logging, preserved dnf installs, de-vendoring, pip installs, cleanup and permission fixes within the heredoc blocks.
TensorFlow runtime
runtimes/tensorflow/ubi9-python-3.12/Dockerfile.cuda
Replaced chained RUN commands with /bin/bash heredocs (with set -Eeuxo pipefail), moved OS package installs and uv pip install plus permission fixes into the heredoc structure.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Repetitive pattern across many Dockerfiles reduces per-file complexity but review should verify heredoc quoting, variable expansion, and preserved conditional branches.
  • Focus review on: runtimes/datascience/.../Dockerfile.cpu (dense arch-specific logic) and any heredoc sections that build/install wheels or modify permissions.

Possibly related PRs

Suggested labels

size/l

Suggested reviewers

  • atheo89
  • dibryant
  • ide-developer

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description includes a problem statement, references previous related PRs, and explains the intent to reformat bash into HEREDOCs. However, none of the self-checklist items are marked complete, testing details are missing, and the PR has an unresolved Docker build failure with LD_LIBRARY_PATH unbound variable. Mark completed checklist items with [x], provide detailed testing instructions and environment details, and resolve the Docker build failure caused by unbound LD_LIBRARY_PATH variable in the HEREDOC blocks before merging.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main refactoring effort: wrapping multiple RUN commands with bash for improved readability, consistency, and error handling across runtime Dockerfiles.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8658da0 and 4acfcc5.

📒 Files selected for processing (7)
  • runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu (6 hunks)
  • runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu (2 hunks)
  • runtimes/pytorch+llmcompressor/ubi9-python-3.12/Dockerfile.cuda (2 hunks)
  • runtimes/pytorch/ubi9-python-3.12/Dockerfile.cuda (2 hunks)
  • runtimes/rocm-pytorch/ubi9-python-3.12/Dockerfile.rocm (2 hunks)
  • runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm (2 hunks)
  • runtimes/tensorflow/ubi9-python-3.12/Dockerfile.cuda (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • runtimes/pytorch/ubi9-python-3.12/Dockerfile.cuda
  • runtimes/rocm-pytorch/ubi9-python-3.12/Dockerfile.rocm
  • runtimes/tensorflow/ubi9-python-3.12/Dockerfile.cuda
⏰ 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). (17)
  • GitHub Check: Red Hat Konflux / odh-pipeline-runtime-pytorch-rocm-py312-ubi9-on-pull-request
  • GitHub Check: Red Hat Konflux / odh-pipeline-runtime-minimal-cpu-py312-ubi9-on-pull-request
  • GitHub Check: Red Hat Konflux / odh-pipeline-runtime-pytorch-llmcompressor-cuda-py312-ubi9-on-pull-request
  • GitHub Check: Red Hat Konflux / odh-pipeline-runtime-pytorch-cuda-py312-ubi9-on-pull-request
  • GitHub Check: Red Hat Konflux / odh-pipeline-runtime-datascience-cpu-py312-ubi9-on-pull-request
  • GitHub Check: Red Hat Konflux / odh-pipeline-runtime-tensorflow-rocm-py312-ubi9-on-pull-request
  • GitHub Check: Red Hat Konflux / odh-pipeline-runtime-tensorflow-cuda-py312-ubi9-on-pull-request
  • GitHub Check: build (runtime-cuda-tensorflow-ubi9-python-3.12, 3.12, linux/arm64, false) / build
  • GitHub Check: build (rocm-runtime-tensorflow-ubi9-python-3.12, 3.12, linux/amd64, false) / build
  • GitHub Check: build (runtime-cuda-pytorch-ubi9-python-3.12, 3.12, linux/amd64, false) / build
  • GitHub Check: build (runtime-minimal-ubi9-python-3.12, 3.12, linux/s390x, false) / build
  • GitHub Check: build (runtime-datascience-ubi9-python-3.12, 3.12, linux/amd64, false) / build
  • GitHub Check: build (rocm-runtime-pytorch-ubi9-python-3.12, 3.12, linux/amd64, false) / build
  • GitHub Check: build (runtime-datascience-ubi9-python-3.12, 3.12, linux/s390x, false) / build
  • GitHub Check: build (runtime-cuda-tensorflow-ubi9-python-3.12, 3.12, linux/amd64, false) / build
  • GitHub Check: build (runtime-cuda-pytorch-llmcompressor-ubi9-python-3.12, 3.12, linux/amd64, false) / build
  • GitHub Check: build (runtime-minimal-ubi9-python-3.12, 3.12, linux/amd64, false) / build
🔇 Additional comments (16)
runtimes/pytorch+llmcompressor/ubi9-python-3.12/Dockerfile.cuda (2)

39-44: Good heredoc conversion with proper error handling.

The refactor correctly converts the sequential RUN commands into a single heredoc block with set -Eeuxo pipefail for improved consistency and safety. The single-quoted 'EOF' delimiter ensures no accidental variable expansion.


88-97: Good heredoc conversion; verify no unbound variables in uv pip install context.

This conversion improves readability by consolidating related operations (install message, package install, permissions fixes). The set -u option will catch unbound variables if any are used during the uv pip install execution or in scripts it invokes. Ensure that no build dependencies or environment setup in upstream stages rely on undefined variables.

The PR description notes an "unbound variable" error (LD_LIBRARY_PATH) in a heredoc elsewhere in the PR. Please verify that:

  1. This file builds successfully with docker build or the project's build system.
  2. All converted heredocs in the full PR have been tested and pass without unbound variable errors.
  3. The checklist items (especially "Run make test" and "manually test and verify the changes work") have been completed before requesting review.
runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm (2)

37-42: LGTM - Consistent heredoc conversion with proper error handling.

The conversion from chained RUN commands to a single /bin/bash <<'EOF' heredoc with set -Eeuxo pipefail is clean and maintains the original logic while improving consistency.


87-97: LGTM - Well-structured Python package installation in heredoc.

The consolidation of the Python package installation, comments, and permission fixes into a single heredoc block improves readability and ensures all operations execute under unified error handling.

runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu (9)

41-58: LGTM - Architecture-aware package installation with proper error handling.

The architecture-specific package selection using conditional logic within a single heredoc block is well-structured and maintains the PR's goal of improving consistency.


60-72: LGTM - Clean ppc64le profile setup.

The ppc64le environment profile is properly initialized and includes necessary build configuration variables.


75-93: LGTM - s390x Rust setup with proper scoping.

The Rust installation and environment setup are correctly isolated within the architecture conditional and properly create the necessary profile scripts.


225-236: LGTM - ppc64le OpenBLAS build properly isolated.

The architecture-conditional OpenBLAS build correctly sources the toolchain profile and maintains the dummy directory fallback for non-ppc64le targets.


252-267: LGTM - ppc64le ONNX build with proper environment handling.

The ONNX build for ppc64le properly sources the toolchain and sets required CMake flags within the heredoc context.


283-330: ✓ ppc64le arrow build correctly initializes LD_LIBRARY_PATH.

Unlike the s390x section (lines 143–207), this ppc64le arrow build correctly initializes LD_LIBRARY_PATH at line 289 before attempting the conditional append at line 291. This approach is sound.


358-371: LGTM - Architecture-specific wheel installation with safe cleanup.

The conditional installation of pre-built wheels from upstream builder stages is properly scoped and includes appropriate error checking and cleanup logic.


376-384: LGTM - s390x wheel installation with proper scoping.

The s390x wheel installation from the build stage is correctly isolated within an architecture conditional.


392-415: ✓ Python package installation correctly handles architecture-specific environment setup.

The runtime Python package installation properly initializes LD_LIBRARY_PATH for ppc64le (line 397) and s390x (implicitly, since wheels are pre-built) before using the variable in expansions. This section is safe from the unbound variable issue.

runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu (3)

37-48: Architecture detection and conditional package installation look correct.

The ARCH variable is properly defined before use (line 39), and the conditional logic that augments PACKAGES for s390x/ppc64le is well-scoped. The transition to HEREDOC with set -Eeuxo pipefail improves error handling.


92-101: uv pip install refactoring to HEREDOC is sound.

The commands are straightforward: pip installation, permissions fix, and cleanup. No undefined variable references detected in this segment.


28-48: LD_LIBRARY_PATH handling is already properly protected; the review comment's diagnosis cannot be verified.

After comprehensive verification of the codebase, the specific concern raised about an unbound LD_LIBRARY_PATH variable is not supported by evidence:

  • All references to LD_LIBRARY_PATH across the codebase use safe expansion syntax (${LD_LIBRARY_PATH:+...}), which prevents set -u errors
  • The file shown in the review (runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu) contains no LD_LIBRARY_PATH references
  • The code snippet in lines 28–48 is correct and contains no problematic variable references

While other unprotected variables exist (e.g., $TARGETARCH, $NVIDIA_GPGKEY_SUM), these are declared as ARG or ENV at the Dockerfile level before use and are outside the scope of this review comment's specific claim.

If a Docker build failure occurs, it would likely involve variables other than LD_LIBRARY_PATH, and the actual error message should be consulted to identify the true cause.

Likely an incorrect or invalid review comment.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci bot added size/xl and removed size/xl labels Nov 7, 2025
@jiridanek
Copy link
Member Author

jiridanek commented Nov 7, 2025

https://github.com/opendatahub-io/notebooks/actions/runs/19170782151/job/54802600249?pr=2656#step:26:1550

+ cd arrow
+ export ARROW_HOME=/usr/local
+ ARROW_HOME=/usr/local
/bin/bash: line 15: LD_LIBRARY_PATH: unbound variable
Error: building at STEP "RUN --mount=type=cache,target=/root/.cache/pip --mount=type=cache,target=/root/.cache/dnf /bin/bash <<'EOF'": while running runtime: exit status 1

@openshift-ci openshift-ci bot added size/xl and removed size/xl labels Nov 7, 2025
…with bash for improved readability, consistency, and error handling (opendatahub-io#2656)
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4658997 and 8658da0.

📒 Files selected for processing (1)
  • runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu (6 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). (17)
  • GitHub Check: build (runtime-cuda-tensorflow-ubi9-python-3.12, 3.12, linux/arm64, false) / build
  • GitHub Check: build (runtime-minimal-ubi9-python-3.12, 3.12, linux/amd64, false) / build
  • GitHub Check: build (runtime-cuda-pytorch-ubi9-python-3.12, 3.12, linux/amd64, false) / build
  • GitHub Check: build (rocm-runtime-tensorflow-ubi9-python-3.12, 3.12, linux/amd64, false) / build
  • GitHub Check: build (rocm-runtime-pytorch-ubi9-python-3.12, 3.12, linux/amd64, false) / build
  • GitHub Check: build (runtime-cuda-tensorflow-ubi9-python-3.12, 3.12, linux/amd64, false) / build
  • GitHub Check: build (runtime-minimal-ubi9-python-3.12, 3.12, linux/s390x, false) / build
  • GitHub Check: build (runtime-datascience-ubi9-python-3.12, 3.12, linux/amd64, false) / build
  • GitHub Check: build (runtime-datascience-ubi9-python-3.12, 3.12, linux/s390x, false) / build
  • GitHub Check: build (runtime-cuda-pytorch-llmcompressor-ubi9-python-3.12, 3.12, linux/amd64, false) / build
  • GitHub Check: Red Hat Konflux / odh-pipeline-runtime-datascience-cpu-py312-ubi9-on-pull-request
  • GitHub Check: Red Hat Konflux / odh-pipeline-runtime-pytorch-cuda-py312-ubi9-on-pull-request
  • GitHub Check: Red Hat Konflux / odh-pipeline-runtime-minimal-cpu-py312-ubi9-on-pull-request
  • GitHub Check: Red Hat Konflux / odh-pipeline-runtime-pytorch-rocm-py312-ubi9-on-pull-request
  • GitHub Check: Red Hat Konflux / odh-pipeline-runtime-tensorflow-cuda-py312-ubi9-on-pull-request
  • GitHub Check: Red Hat Konflux / odh-pipeline-runtime-tensorflow-rocm-py312-ubi9-on-pull-request
  • GitHub Check: Red Hat Konflux / odh-pipeline-runtime-pytorch-llmcompressor-cuda-py312-ubi9-on-pull-request
🔇 Additional comments (1)
runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu (1)

283-330: Verify arrow-builder (ppc64le) pattern for similar set -u issues.

Line 291 in the arrow-builder stage also uses conditional expansion on LD_LIBRARY_PATH: LD_LIBRARY_PATH=$(pwd)/dist/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}. This assignment pattern differs from line 159 (which uses direct export). Ensure the ppc64le build path properly initializes or sources LD_LIBRARY_PATH before line 286 to avoid the same unbound-variable error.

…ding empty entry to LD_LIBRARY_PATH (opendatahub-io#2656)

```
+ cd arrow
+ export ARROW_HOME=/usr/local
+ ARROW_HOME=/usr/local
/bin/bash: line 15: LD_LIBRARY_PATH: unbound variable
Error: building at STEP "RUN --mount=type=cache,target=/root/.cache/pip --mount=type=cache,target=/root/.cache/dnf /bin/bash <<'EOF'": while running runtime: exit status 1
```
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 7, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ide-developer
Once this PR has been reviewed and has the lgtm label, please assign jstourac for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 7, 2025

@jiridanek: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/runtime-cuda-pt-ubi9-python-3-12-pr-image-mirror 4acfcc5 link true /test runtime-cuda-pt-ubi9-python-3-12-pr-image-mirror
ci/prow/rocm-runtime-pt-ubi9-python-3-12-pr-image-mirror 4acfcc5 link true /test rocm-runtime-pt-ubi9-python-3-12-pr-image-mirror
ci/prow/runtime-cuda-tf-ubi9-python-3-12-pr-image-mirror 4acfcc5 link true /test runtime-cuda-tf-ubi9-python-3-12-pr-image-mirror
ci/prow/runtime-ubi9-python-3-12-pr-image-mirror 4acfcc5 link true /test runtime-ubi9-python-3-12-pr-image-mirror
ci/prow/runtime-ds-ubi9-python-3-12-pr-image-mirror 4acfcc5 link true /test runtime-ds-ubi9-python-3-12-pr-image-mirror
ci/prow/images 4acfcc5 link true /test images

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@jiridanek jiridanek merged commit fa24b68 into opendatahub-io:main Nov 7, 2025
26 of 36 checks passed
jiridanek added a commit that referenced this pull request Nov 7, 2025
…with bash for improved readability, consistency, and error handling (#2656)
@jiridanek jiridanek deleted the jd_heredoc5 branch November 7, 2025 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm review-requested GitHub Bot creates notification on #pr-review-ai-ide-team slack channel size/xl

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants