RHOAIENG-18582: Post ODH Elyra release (v4.3.0) #2555
Conversation
|
Skipping CI for Draft Pull Request. |
WalkthroughRemoves bootstrapper.py from multiple runtime subtrees and updates several Dockerfiles to COPY utils with --chown=1001, download an external Elyra Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
the
review-requested
| # Copy Elyra dependencies for air-gapped enviroment | ||
| COPY ${DATASCIENCE_SOURCE_CODE}/utils ./utils/ | ||
| # Download Elyra boostrapper.py | ||
| RUN curl -fL https://raw.githubusercontent.com/opendatahub-io/elyra/refs/heads/main/elyra/kfp/bootstrapper.py \ |
There was a problem hiding this comment.
Are we sure that this will work in disconnected envs?
There was a problem hiding this comment.
Hello @atheo89 I thought that it would work because this would be done during the image build time. The idea is now that Notebook images don't need a custom bootstrapper.py we could download it from Elyra and use the new system property we introduced: ELYRA_INSTALL_PACKAGES to prevent Elyra from trying to download packages - this new flag should do that
There was a problem hiding this comment.
Can we curl that file from a fixed tag (associated with the Elyra version in the image) instead of main?
EDIT: nvm, I saw the other comment below.
| # Copy Elyra dependencies for air-gapped enviroment | ||
| COPY ${TENSORFLOW_SOURCE_CODE}/utils ./utils/ | ||
| # Download Elyra boostrapper.py | ||
| RUN curl -fL https://raw.githubusercontent.com/opendatahub-io/elyra/refs/tags/v4.3.0/elyra/kfp/bootstrapper.py \ |
There was a problem hiding this comment.
isn't this bootstrapper installed together with the odh-elyra package? Can't we grab it from python's site-packages? And if it is not installed, can't we install it that way?
There was a problem hiding this comment.
that's a great suggestion, it should be with the elyra package, it would be better to copy it from there - I will check and update the Dockerfile accordingly
There was a problem hiding this comment.
@jiridanek I found the local bootstrapper.py and copy it to utils on script setup-elyra.sh - would it be the best place to do it?
Thanks!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
jupyter/datascience/ubi9-python-3.12/pylock.toml(1 hunks)jupyter/datascience/ubi9-python-3.12/pyproject.toml(1 hunks)jupyter/datascience/ubi9-python-3.12/setup-elyra.sh(2 hunks)jupyter/pytorch+llmcompressor/ubi9-python-3.12/pylock.toml(1 hunks)jupyter/pytorch+llmcompressor/ubi9-python-3.12/pyproject.toml(1 hunks)jupyter/pytorch/ubi9-python-3.12/pylock.toml(1 hunks)jupyter/pytorch/ubi9-python-3.12/pyproject.toml(1 hunks)jupyter/rocm/pytorch/ubi9-python-3.12/pylock.toml(1 hunks)jupyter/rocm/pytorch/ubi9-python-3.12/pyproject.toml(1 hunks)jupyter/rocm/tensorflow/ubi9-python-3.12/pylock.toml(1 hunks)jupyter/rocm/tensorflow/ubi9-python-3.12/pyproject.toml(1 hunks)jupyter/tensorflow/ubi9-python-3.12/pylock.toml(1 hunks)jupyter/tensorflow/ubi9-python-3.12/pyproject.toml(1 hunks)jupyter/trustyai/ubi9-python-3.12/pylock.toml(1 hunks)jupyter/trustyai/ubi9-python-3.12/pyproject.toml(1 hunks)runtimes/datascience/ubi9-python-3.12/utils/bootstrapper.py(0 hunks)runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu(1 hunks)runtimes/minimal/ubi9-python-3.12/utils/bootstrapper.py(0 hunks)runtimes/pytorch+llmcompressor/ubi9-python-3.12/utils/bootstrapper.py(0 hunks)runtimes/pytorch/ubi9-python-3.12/utils/bootstrapper.py(0 hunks)runtimes/rocm-pytorch/ubi9-python-3.12/utils/bootstrapper.py(0 hunks)runtimes/rocm-tensorflow/ubi9-python-3.12/utils/bootstrapper.py(0 hunks)runtimes/tensorflow/ubi9-python-3.12/utils/bootstrapper.py(0 hunks)
💤 Files with no reviewable changes (7)
- runtimes/rocm-tensorflow/ubi9-python-3.12/utils/bootstrapper.py
- runtimes/tensorflow/ubi9-python-3.12/utils/bootstrapper.py
- runtimes/pytorch+llmcompressor/ubi9-python-3.12/utils/bootstrapper.py
- runtimes/minimal/ubi9-python-3.12/utils/bootstrapper.py
- runtimes/datascience/ubi9-python-3.12/utils/bootstrapper.py
- runtimes/rocm-pytorch/ubi9-python-3.12/utils/bootstrapper.py
- runtimes/pytorch/ubi9-python-3.12/utils/bootstrapper.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1875
File: runtimes/pytorch/ubi9-python-3.12/pylock.toml:0-0
Timestamp: 2025-09-12T23:14:23.293Z
Learning: Issue #3325 in elyra-ai/elyra repository documents the urllib3 2.x compatibility blocker affecting jupyter/* images in opendatahub-io/notebooks. The dependency chain odh-elyra==4.2.3 → appengine-python-standard==1.1.10 → urllib3>=1.26.2,<2 prevents upgrading from urllib3=1.26.20 to 2.5.0 in images that include elyra, explaining why only codeserver/* and runtimes/* images achieved the 2.5.0 upgrade in PR #1875.
📚 Learning: 2025-09-12T23:14:23.293Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1875
File: runtimes/pytorch/ubi9-python-3.12/pylock.toml:0-0
Timestamp: 2025-09-12T23:14:23.293Z
Learning: Issue #3325 in elyra-ai/elyra repository documents the urllib3 2.x compatibility blocker affecting jupyter/* images in opendatahub-io/notebooks. The dependency chain odh-elyra==4.2.3 → appengine-python-standard==1.1.10 → urllib3>=1.26.2,<2 prevents upgrading from urllib3=1.26.20 to 2.5.0 in images that include elyra, explaining why only codeserver/* and runtimes/* images achieved the 2.5.0 upgrade in PR #1875.
Applied to files:
jupyter/trustyai/ubi9-python-3.12/pyproject.tomljupyter/trustyai/ubi9-python-3.12/pylock.tomljupyter/tensorflow/ubi9-python-3.12/pylock.tomljupyter/datascience/ubi9-python-3.12/pylock.tomljupyter/pytorch+llmcompressor/ubi9-python-3.12/pylock.tomljupyter/tensorflow/ubi9-python-3.12/pyproject.tomljupyter/datascience/ubi9-python-3.12/pyproject.tomljupyter/pytorch/ubi9-python-3.12/pyproject.tomljupyter/pytorch+llmcompressor/ubi9-python-3.12/pyproject.toml
📚 Learning: 2025-07-23T16:18:42.922Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#0
File: :0-0
Timestamp: 2025-07-23T16:18:42.922Z
Learning: The TensorFlow ROCm Python 3.12 compatibility issue in opendatahub-io/notebooks PR #1259 was caused by using tensorflow-rocm==2.14.0.600 in Pipfile.lock which lacks Python 3.12 wheels, while the Pipfile specifies tensorflow_rocm=~=2.18.1. The solution requires updating Pipfile sources to include https://repo.radeon.com/rocm/manylinux/rocm-rel-6.4/ repository which contains tensorflow_rocm-2.18.1-cp312-cp312-manylinux_2_28_x86_64.whl and regenerating Pipfile.lock using the piplock-refresh GitHub Action.
Applied to files:
jupyter/rocm/tensorflow/ubi9-python-3.12/pylock.toml
📚 Learning: 2025-08-29T12:20:28.311Z
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1875
File: runtimes/pytorch+llmcompressor/ubi9-python-3.12/Pipfile:70-70
Timestamp: 2025-08-29T12:20:28.311Z
Learning: jiridanek requested GitHub issue creation for urllib3 2.x compatibility blocked by appengine-python-standard dependency during PR #1875 review. Issue #3325 was created on elyra-ai/elyra repository explaining the dependency conflict chain (odh-elyra==4.2.3 → appengine-python-standard==1.1.10 → urllib3>=1.26.2,<2) that blocks updating urllib3 to 2.5.0 for CVE security fixes across notebook runtime environments.
Applied to files:
jupyter/tensorflow/ubi9-python-3.12/pyproject.tomljupyter/datascience/ubi9-python-3.12/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). (14)
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-pytorch-llmcompressor-cuda-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-tensorflow-cuda-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-workbench-jupyter-tensorflow-rocm-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-tensorflow-cuda-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-datascience-cpu-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-trustyai-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-rocm-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-pytorch-rocm-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-pytorch-cuda-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-pipeline-runtime-minimal-cpu-py312-ubi9-on-pull-request
- GitHub Check: Generate job matrix
🔇 Additional comments (6)
jupyter/pytorch/ubi9-python-3.12/pylock.toml (1)
2595-2599: Confirmed odh-elyra 4.3.1 artifacts on PyPI
sdist and wheel are live with matching SHA256 hashes; safe to merge.jupyter/rocm/tensorflow/ubi9-python-3.12/pylock.toml (1)
2511-2513: Confirm Elyra target version.This lockfile now pins
odh-elyrato 4.3.1, while the PR description frames the update as 4.3.0. Please double-check that 4.3.1 is the intended release (and already available for the build pipeline) before merging.jupyter/rocm/pytorch/ubi9-python-3.12/pylock.toml (1)
2459-2461: Lockfile entry looks consistent with 4.3.1.Version, URLs, timestamps, and hashes follow the established pattern for odh-elyra updates. 👍
jupyter/datascience/ubi9-python-3.12/pyproject.toml (1)
30-30: Dependency bump matches Elyra 4.3.x rollout.Thanks for updating the datascience image to pin
odh-elyra==4.3.1; this keeps it aligned with the new bootstrapper flow introduced elsewhere in the PR. No further action from my side.jupyter/pytorch+llmcompressor/ubi9-python-3.12/pyproject.toml (1)
46-46: Good to see odh-elyra unified at 4.3.1.The pytorch+llmcompressor image now tracks the same Elyra release as the rest of the stack, which will simplify maintenance once 4.3.x ships. Looks solid.
jupyter/pytorch+llmcompressor/ubi9-python-3.12/pylock.toml (1)
2706-2708: Confirm odh-elyra bump target, lock consistency, and urllib3 compatibility
Lock pins 4.3.1 but PR title says 4.3.0—confirm intended version, ensure all pylock.toml files use 4.3.1, and verify that appengine-python-standard no longer enforces urllib3<2.
| RUN curl -fL https://raw.githubusercontent.com/opendatahub-io/elyra/refs/tags/v4.3.0/elyra/kfp/bootstrapper.py \ | ||
| -o ./utils/bootstrapper.py |
There was a problem hiding this comment.
Align bootstrapper version with Elyra bump and avoid network-dependent builds.
We now pull bootstrapper.py at build time from Elyra tag v4.3.0, but the rest of this PR upgrades ODH Elyra to 4.3.1. Shipping a 4.3.0 bootstrapper alongside 4.3.1 bits is a mismatch that can regress the fixes we’re picking up. In addition, downloading from GitHub during the build defeats disconnected/air-gapped builds—the very scenario the new ELYRA_INSTALL_PACKAGES=false knob is supposed to support. Please vendor the correct 4.3.1 bootstrapper (or fetch it from a pre-staged artifact inside the image) so the Docker build stays reproducible and works without external network access.
🤖 Prompt for AI Agents
In runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu around lines 77-78, the
Dockerfile downloads bootstrapper.py from Elyra tag v4.3.0 at build time which
mismatches the repo bump to v4.3.1 and prevents air-gapped/disconnected builds;
replace the curl step with a reproducible, network-free approach by vendoring
the v4.3.1 bootstrapper into the build context (or copying it from a pre-staged
artifact inside the image/build context), update the Dockerfile to ADD/COPY that
local file into ./utils/bootstrapper.py instead of curling, and ensure the
vendored file version matches Elyra v4.3.1 (optionally add a checksum or comment
noting the source and tag).
There was a problem hiding this comment.
what if we do
cp /opt/app-root/lib/python3.12/site-packages/elyra/kfp/bootstrapper.py ...
There was a problem hiding this comment.
@jiridanek, that's an excellent solution! 🎯
Copying bootstrapper.py from the installed Elyra package would indeed solve both issues:
- ✅ Version alignment: Automatically uses the correct 4.3.1 version
- ✅ Air-gapped builds: No network dependency during build
However, there's an ordering consideration in the Dockerfile. Currently:
- Line 75:
COPY ${MINIMAL_SOURCE_CODE}/utils ./utils/ - Line 77-78:
curlbootstrapper.py (problematic) - Line 80:
uv pip install(this installs odh-elyra and creates site-packages)
To implement your approach, the copy would need to happen after the uv pip install step, since that's when the Elyra package gets installed to site-packages. Something like:
# Copy Elyra dependencies for air-gapped environment
COPY ${MINIMAL_SOURCE_CODE}/utils ./utils/
RUN echo "Installing softwares and packages" && \
uv pip install --strict --no-deps --no-cache ... && \
# Copy bootstrapper from installed Elyra package
cp /opt/app-root/lib/python3.12/site-packages/elyra/kfp/bootstrapper.py ./utils/bootstrapper.py && \
chmod -R g+w /opt/app-root/lib/python3.12/site-packages && \
fix-permissions /opt/app-root -PThis approach is cleaner, more maintainable, and naturally stays in sync with the Elyra version specified in pylock.toml. 👍
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of the Dockerfile
cat runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu | grep -A 15 "Copy Elyra dependencies"
Length of output: 775
🏁 Script executed:
#!/bin/bash
# Look for any bootstrapper.py references in the file
grep -n "bootstrapper" runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the entire RUN command that installs packages to see if cp was added
sed -n '/^RUN echo "Installing softwares and packages"/,/fix-permissions/p' runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu
Length of output: 652
🏁 Script executed:
#!/bin/bash
# Check if bootstrapper.py exists in the minimal utils directory
ls -la runtimes/minimal/ubi9-python-3.12/utils/
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Check if bootstrapper.py was deleted or is present
fd -t f "bootstrapper.py" runtimes/minimal/ubi9-python-3.12/
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the git status of the minimal runtime to see what files changed
git diff --name-status origin/main...HEAD -- runtimes/minimal/ubi9-python-3.12/ | head -20
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if minimal runtime has any setup scripts
ls -la runtimes/minimal/ubi9-python-3.12/ | grep -E "setup|sh$"
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the datascience runtime for comparison
ls -la runtimes/datascience/ubi9-python-3.12/utils/
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Look for any Elyra-related setup scripts in minimal runtime
fd -t f -e sh . runtimes/minimal/ubi9-python-3.12/
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the PR changes to understand what was modified
git diff --stat HEAD~1 HEAD -- runtimes/minimal/
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the datascience image to see if it has setup-elyra.sh
fd -t f "setup-elyra.sh" jupyter/datascience/ubi9-python-3.12/
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Look at the setup-elyra.sh to confirm the cp approach
cat jupyter/datascience/ubi9-python-3.12/setup-elyra.sh | grep -A 3 -B 3 "bootstrapper"
Length of output: 261
🏁 Script executed:
#!/bin/bash
# Check what Dockerfile references the minimal runtime
rg -l "FROM.*runtime-minimal" --type dockerfile 2>/dev/null || rg -l "runtime-minimal" jupyter/
Length of output: 0
@jiridanek, perfect! ✅ The changes look great:
-
Version alignment issue resolved: The problematic
curldownloading v4.3.0 has been removed from the Dockerfile. -
Air-gapped build support maintained: The
setup-elyra.shscript now implements exactly what you suggested:cp /opt/app-root/lib/python3.12/site-packages/elyra/kfp/bootstrapper.py /opt/app-root/bin/utils/
-
Clean architecture: The minimal runtime serves as a base without Elyra-specific setup, and derived images (like datascience) handle the bootstrapper copy in their setup scripts after package installation.
This approach ensures the bootstrapper version always matches the installed odh-elyra package (4.3.1) and works perfectly in disconnected environments. The solution is maintainable and self-documenting with the comment acknowledging ELYRA_INSTALL_PACKAGES=false requirement. Nice work! 🎯
🧠 Learnings used
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1333
File: runtimes/rocm-pytorch/ubi9-python-3.12/utils/bootstrapper.py:1-769
Timestamp: 2025-07-08T19:35:49.482Z
Learning: jiridanek requested GitHub issue creation for bootstrapper code duplication problem in runtimes/rocm-pytorch/ubi9-python-3.12/utils/bootstrapper.py during PR #1333 review. After an initial failed attempt, issue #1349 was successfully created with comprehensive problem description covering maintenance overhead and consistency risks from duplicate implementations across 5 Python 3.12 runtime environments, four solution options (symlinks, import-based, template-based, direct shared import) with pros/cons analysis, clear acceptance criteria for consolidation and maintainability, step-by-step implementation guidance, and proper context linking, continuing the established pattern of systematic code quality improvements through detailed issue tracking.
Learnt from: jiridanek
PR: opendatahub-io/notebooks#1333
File: runtimes/pytorch/ubi9-python-3.12/utils/bootstrapper.py:619-626
Timestamp: 2025-07-08T19:33:14.340Z
Learning: jiridanek requested GitHub issue creation for Python 3.12 version check bug in bootstrapper.py during PR #1333 review. Issue #1348 was created with comprehensive problem description covering version check exclusion affecting all Python 3.12 runtime images, detailed impact analysis of bootstrapper execution failures, clear solution with code examples, affected files list including all 6 runtime bootstrapper copies, acceptance criteria for testing and verification, implementation notes about code duplication and upstream reporting, and proper context linking, continuing the established pattern of systematic code quality improvements through detailed issue tracking.
Learnt from: jiridanek
PR: opendatahub-io/notebooks#2215
File: runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu:0-0
Timestamp: 2025-09-05T12:10:50.856Z
Learning: jiridanek requested GitHub issue creation for Dockerfile environment variable refactoring during PR #2215 review. Issue #2311 was created addressing build-only variables (OPENBLAS_VERSION, ONNX_VERSION, GRPC_PYTHON_BUILD_SYSTEM_OPENSSL) being unnecessarily written to /etc/profile.d/ppc64le.sh in runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu, causing variable duplication across stages, unreliable sourcing in non-login build contexts, and violation of DRY principles. The issue includes comprehensive problem description covering affected lines 30-37, detailed impact analysis of build reliability and maintenance overhead, three solution options with centralized ARG/ENV approach as recommended, clear acceptance criteria for version centralization and build-only variable cleanup, and specific implementation guidance with code examples, assigned to jiridanek, continuing the established pattern of systematic infrastructure improvements through detailed issue tracking.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
elyra package is not on runtime image, so copying bootstrapper.py from python packages would not be possible as the package is not there.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
jiridanek
left a comment
jiridanek
left a comment
There was a problem hiding this comment.
@jesuino please also update elyra version in manifests, it is causing tests to fail
[checking the `notebook-python-dependencies` array] (pyproject=PosixPath('/home/runner/work/notebooks/notebooks/jupyter/pytorch+llmcompressor/ubi9-python-3.12/pyproject.toml')) SUBFAIL tests/test_main.py::test_image_pyprojects - AssertionError: Odh-Elyra: manifest /home/runner/work/notebooks/notebooks/manifests/base/jupyter-pytorch-llmcompressor-imagestream.yaml declares 4.2, but pylock.toml pins 4.3.1
assert (4, 3) == (4, 2)
https://github.com/opendatahub-io/notebooks/actions/runs/18386078682/job/52384729336?pr=2555
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
runtimes/tensorflow/ubi9-python-3.12/Dockerfile.cuda (1)
69-72: Network-dependent build defeats stated air-gapped environment support.Same concern as other runtime Dockerfiles: downloading bootstrapper.py via
curlat build time breaks reproducibility and air-gapped builds. Vendor the file locally instead for offline-capable, reproducible images.runtimes/rocm-pytorch/ubi9-python-3.12/Dockerfile.rocm (1)
65-68: Network-dependent build defeats stated air-gapped environment support.Same concern as other runtime Dockerfiles: downloading bootstrapper.py via
curlat build time breaks reproducibility and air-gapped builds. Vendor the file locally instead for offline-capable, reproducible images.runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu (1)
73-76: Network-dependent build defeats stated air-gapped environment support.Same concern as other runtime Dockerfiles: downloading bootstrapper.py via
curlat build time breaks reproducibility and air-gapped builds. Vendor the file locally instead for offline-capable, reproducible images.runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu (1)
248-251: Network-dependent build defeats stated air-gapped environment support.Same concern as other runtime Dockerfiles: downloading bootstrapper.py via
curlat build time breaks reproducibility and air-gapped builds. Vendor the file locally instead for offline-capable, reproducible images.runtimes/pytorch+llmcompressor/ubi9-python-3.12/Dockerfile.cuda (1)
67-70: Network-dependent build defeats stated air-gapped environment support.Same concern as other runtime Dockerfiles: downloading bootstrapper.py via
curlat build time breaks reproducibility and air-gapped builds. Vendor the file locally instead for offline-capable, reproducible images.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
manifests/base/commit-latest.env(1 hunks)runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu(1 hunks)runtimes/datascience/ubi9-python-3.12/utils/bootstrapper.py(0 hunks)runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu(1 hunks)runtimes/minimal/ubi9-python-3.12/utils/bootstrapper.py(0 hunks)runtimes/pytorch+llmcompressor/ubi9-python-3.12/Dockerfile.cuda(1 hunks)runtimes/pytorch+llmcompressor/ubi9-python-3.12/utils/bootstrapper.py(0 hunks)runtimes/pytorch/ubi9-python-3.12/Dockerfile.cuda(1 hunks)runtimes/pytorch/ubi9-python-3.12/utils/bootstrapper.py(0 hunks)runtimes/rocm-pytorch/ubi9-python-3.12/Dockerfile.rocm(1 hunks)runtimes/rocm-pytorch/ubi9-python-3.12/utils/bootstrapper.py(0 hunks)runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm(1 hunks)runtimes/rocm-tensorflow/ubi9-python-3.12/utils/bootstrapper.py(0 hunks)runtimes/tensorflow/ubi9-python-3.12/Dockerfile.cuda(1 hunks)runtimes/tensorflow/ubi9-python-3.12/utils/bootstrapper.py(0 hunks)
💤 Files with no reviewable changes (7)
- runtimes/rocm-pytorch/ubi9-python-3.12/utils/bootstrapper.py
- runtimes/minimal/ubi9-python-3.12/utils/bootstrapper.py
- runtimes/tensorflow/ubi9-python-3.12/utils/bootstrapper.py
- runtimes/pytorch+llmcompressor/ubi9-python-3.12/utils/bootstrapper.py
- runtimes/rocm-tensorflow/ubi9-python-3.12/utils/bootstrapper.py
- runtimes/pytorch/ubi9-python-3.12/utils/bootstrapper.py
- runtimes/datascience/ubi9-python-3.12/utils/bootstrapper.py
⏰ 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). (9)
- GitHub Check: build (cuda-rstudio-rhel9-python-3.12, 3.12, linux/amd64, true) / build
- GitHub Check: build (rstudio-rhel9-python-3.12, 3.12, linux/amd64, true) / build
- 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-cuda-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-minimal-cpu-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-pytorch-rocm-py312-ubi9-on-pull-request
🔇 Additional comments (3)
runtimes/pytorch/ubi9-python-3.12/Dockerfile.cuda (1)
67-70: Review comment references mechanisms and quotes not present in the repository.The review correctly identifies that the curl download contradicts the "air-gapped environment" comment, but makes factual errors about this codebase:
- The quote "must be commented out in airgapped images..." does not exist in this repository (searched entire codebase)
ELYRA_INSTALL_PACKAGESmechanism does not exist in this repository (zero references found)- The bootstrap file is not present anywhere in the repository to examine
What is accurate: The curl command on lines 69-70 is unconditional and always downloads from GitHub, which does contradict the "air-gapped environment" comment on line 66. This pattern exists identically across 7+ Dockerfiles (pytorch, tensorflow, rocm-pytorch, rocm-tensorflow, minimal, pytorch+llmcompressor, datascience).
The recommendation to vendor
bootstrapper.pyis valid, but the specific premises attributed to this codebase are incorrect.Likely an incorrect or invalid review comment.
manifests/base/commit-latest.env (2)
1-8: Confirm selective scope of commit updates.The commit update to
0df60b1applies only toodh-workbench-jupyter-*images, whileodh-pipeline-runtime-*images (lines 1-7) andodh-workbench-codeserver-*(line 8) remain at their previous commits.Please confirm this selective scope is intentional and that the Elyra bootstrapper changes do not apply to pipeline runtime or codeserver workbenches.
9-18: Verify inconsistent commit update for tensorflow variants — confirmed as real.The inconsistency is genuine and reflects actual differences in the container images. The
update-commit-latest-env.pyscript automatically extractsvcs-reflabels from container images, so Line 16 (tensorflow-cudaatec4a477) and Line 17 (tensorflow-rocmat0df60b1) indicate these workbench images were built from different commits.Since both variants are referenced identically in
manifests/base/kustomization.yamlpatches, they should typically be kept in sync. Please verify whetherodh-workbench-jupyter-tensorflow-cudawas intentionally left at the older commit or if its rebuild is pending.
| # Copy Elyra dependencies for air-gapped enviroment | ||
| COPY ${TENSORFLOW_SOURCE_CODE}/utils ./utils/ | ||
| COPY --chown=1001 ${TENSORFLOW_SOURCE_CODE}/utils ./utils/ | ||
| # Download Elyra Bootstrapper | ||
| RUN curl -L https://raw.githubusercontent.com/opendatahub-io/elyra/refs/tags/v4.3.1/elyra/kfp/bootstrapper.py \ | ||
| -o ./utils/bootstrapper.py |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify air-gapped environment support.
Line 64's comment states "Elyra dependencies for air-gapped environment," yet the implementation downloads bootstrapper.py from GitHub at build time, which requires network access. The PR objectives mention supporting air-gapped environments via ELYRA_INSTALL_PACKAGES=false, but this implementation contradicts that goal.
Verify whether:
- Air-gapped support is intended to work differently (e.g., pre-cached layers, image registry mirrors).
- The comment on line 64 is misleading and should be updated.
- This curl step should be conditional or skipped in air-gapped scenarios.
🏁 Script executed:
#!/bin/bash
# View the full Dockerfile context around lines 64-68
cat "runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm" | head -80 | tail -30Length of output: 2131
🏁 Script executed:
#!/bin/bash
# Search for ELYRA_INSTALL_PACKAGES usage in the codebase
rg -n -C3 "ELYRA_INSTALL_PACKAGES" --type dockerfileLength of output: 34
🏁 Script executed:
#!/bin/bash
# Check for conditional logic patterns (if statements, ARG usage) in this Dockerfile
rg -n "^(RUN if|ARG|ENV)" "runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm" -A2Length of output: 193
🏁 Script executed:
#!/bin/bash
# Search for other curl commands in the same Dockerfile to see if there's a pattern
rg -n "curl" "runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm" -B2 -A2Length of output: 530
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or logic related to air-gapped environments in this Dockerfile
rg -n -i "air.gapped|offline|cache|registry" "runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm" -B1 -A1Length of output: 1389
🏁 Script executed:
#!/bin/bash
# Search for ELYRA_INSTALL_PACKAGES across entire codebase (without file type restriction)
rg -n "ELYRA_INSTALL_PACKAGES" .Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check what's in the utils directory referenced by TENSORFLOW_SOURCE_CODE
find . -path "*rocm-tensorflow/ubi9-python-3.12/utils*" -type fLength of output: 131
🏁 Script executed:
#!/bin/bash
# Look for bootstrapper.py file to see if it's already present
find . -name "bootstrapper.py" -type fLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check the full Dockerfile to see if there's a parent/base Dockerfile or imports
head -50 "runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm"Length of output: 1970
🏁 Script executed:
#!/bin/bash
# Search for "air-gapped" references across the entire codebase to understand implementation strategy
rg -n -i "air.gapped|air gapped" . -B2 -A2Length of output: 3678
Remove misleading air-gapped comment or make bootstrapper download conditional.
Line 64's comment claims "air-gapped environment" support, but the utils/ directory contains only pip.conf and requirements-elyra.txt (no bootstrapper.py). The immediate curl to download bootstrapper.py from GitHub at build time (lines 67–68) is unconditional and will fail in air-gapped environments. Additionally, ELYRA_INSTALL_PACKAGES referenced in PR objectives does not exist in the codebase.
Either:
- Pre-cache
bootstrapper.pyinutils/so the curl step can be removed, or - Make the curl conditional based on network availability/environment flag, or
- Update the comment to reflect actual behavior (not air-gapped).
Note: This pattern repeats identically across 8 Dockerfiles in the repository.
🤖 Prompt for AI Agents
In runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm around lines 64–68
the comment claims support for an "air-gapped environment" but the Dockerfile
unconditionally CURLs bootstrapper.py from GitHub (which will fail offline) and
references a non-existent ELYRA_INSTALL_PACKAGES objective; fix by either (A)
pre-populating utils/bootstrapper.py in the build context and remove the curl
step so the image truly supports air-gapped builds, or (B) make the download
conditional using a build ARG or environment flag (e.g., only run curl when a
NETWORK_FETCH=true/ARG is set) and fall back to the existing utils/ files, or
(C) if you cannot change build inputs, update the comment to remove the
air-gapped claim; also apply the same chosen fix consistently across the other 7
Dockerfiles and remove or document the nonexistent ELYRA_INSTALL_PACKAGES
reference.
jesuino
force-pushed
the
RHOAIENG-18582
branch
from
b490ab0 to
38a02b0
Compare
|
Hello, I had to force push my changes. After a few discussions, we found that:
Also, notice that I have to force push my changes, but I kept a backup branch with the old changes. |
harshad16
left a comment
harshad16
left a comment
There was a problem hiding this comment.
great work, to pivot to getting the bootstrapper.py from direct elyra.
didnt find where we are setting the env var: ELYRA_INSTALL_PACKAGES
shared a suggestion
| RUN curl -L https://raw.githubusercontent.com/opendatahub-io/elyra/refs/tags/v4.3.1/elyra/kfp/bootstrapper.py \ | ||
| -o ./utils/bootstrapper.py |
There was a problem hiding this comment.
Lets also set the env var ELYRA_INSTALL_PACKAGES false in the dockerfile too,
so it is set in the runtime image.
ENV ELYRA_INSTALL_PACKAGES="false"
There was a problem hiding this comment.
Done, thanks for the suggestion! As we discussed, setting this by default true in boostrapper may affect other users, so it is better to set this in runtime images.
| COPY ${DATASCIENCE_SOURCE_CODE}/utils ./utils/ | ||
| COPY --chown=1001 ${DATASCIENCE_SOURCE_CODE}/utils ./utils/ | ||
| # Download Elyra Bootstrapper | ||
| RUN curl -L https://raw.githubusercontent.com/opendatahub-io/elyra/refs/tags/v4.3.1/elyra/kfp/bootstrapper.py \ |
There was a problem hiding this comment.
Can we read the version dynamically from the installed odh-elyra package instead of hardcoding 4.3.1? Otherwise, we'll have to remember to update this version every time we update the package.
There was a problem hiding this comment.
The only problem is that I don't have this information in runtime images because Elyra is not installed there, only the runtime image dependencies are installed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
runtimes/tensorflow/ubi9-python-3.12/Dockerfile.cuda (1)
78-79: Consider parameterizing the Elyra version.The Elyra version is hardcoded to
v4.3.1. This creates maintenance burden if the version needs to be updated; the Dockerfile must be edited manually. Consider using a build ARG to make versioning more flexible and reduce coupling to a specific release.Apply this diff to parameterize the Elyra version:
ARG TARGETARCH +ARG ELYRA_VERSION=v4.3.1 ARG BASE_IMAGE # External image alias for UBI repository configuration @@ -75,7 +76,7 @@ WORKDIR /opt/app-root/bin COPY ${TENSORFLOW_SOURCE_CODE}/pylock.toml ./ # Copy Elyra dependencies for air-gapped enviroment COPY --chown=1001 ${TENSORFLOW_SOURCE_CODE}/utils ./utils/ # Download Elyra Bootstrapper -RUN curl -L https://raw.githubusercontent.com/opendatahub-io/elyra/refs/tags/v4.3.1/elyra/kfp/bootstrapper.py \ +RUN curl -fL https://raw.githubusercontent.com/opendatahub-io/elyra/refs/tags/${ELYRA_VERSION}/elyra/kfp/bootstrapper.py \ -o ./utils/bootstrapper.py
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu(1 hunks)runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu(1 hunks)runtimes/pytorch+llmcompressor/ubi9-python-3.12/Dockerfile.cuda(1 hunks)runtimes/pytorch/ubi9-python-3.12/Dockerfile.cuda(1 hunks)runtimes/rocm-pytorch/ubi9-python-3.12/Dockerfile.rocm(1 hunks)runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm(1 hunks)runtimes/tensorflow/ubi9-python-3.12/Dockerfile.cuda(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- runtimes/pytorch/ubi9-python-3.12/Dockerfile.cuda
- runtimes/rocm-pytorch/ubi9-python-3.12/Dockerfile.rocm
- runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu
- runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm
- runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu
⏰ 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). (16)
- 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-minimal-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-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-pytorch-llmcompressor-cuda-py312-ubi9-on-pull-request
- GitHub Check: build (runtime-cuda-pytorch-llmcompressor-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 (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-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
- GitHub Check: build (runtime-minimal-ubi9-python-3.12, 3.12, linux/s390x, false) / build
🔇 Additional comments (5)
runtimes/tensorflow/ubi9-python-3.12/Dockerfile.cuda (2)
76-76: Ownership preservation via--chownis appropriate.Setting
--chown=1001ensures the copied utils directory and its contents have proper ownership for the non-root user, which aligns with the container security context established at line 40.
81-81: Environment variable for air-gapped environments is appropriate.Setting
ELYRA_INSTALL_PACKAGES="false"prevents Elyra from attempting to install or re-install dependencies at runtime, which is essential for air-gapped environments where package downloads may not be available.runtimes/pytorch+llmcompressor/ubi9-python-3.12/Dockerfile.cuda (3)
73-73: Correct ownership preservation for copied utils directory.The
--chown=1001flag ensures the copied files have proper ownership for the default user (UID 1001), which is good practice for permission management in containerized environments.
78-78: Correct ENV variable for air-gapped environments.Setting
ELYRA_INSTALL_PACKAGES="false"correctly prevents Elyra from attempting to reinstall dependencies, which is essential for air-gapped environments where package repositories are unavailable.
75-76: Verify supply-chain risk mitigation for bootstrapper.py download.The concern is valid: lines 75–76 download executable code from GitHub without file integrity verification. This codebase uses
--verify-hashesfor pip packages (line 83), indicating your organization prioritizes verification.For this bootstrapper download, confirm your security standards:
- If checksum verification is required: obtain the SHA256 from Elyra's release artifacts (verify against published sources) and add verification steps
- If HTTPS + GitHub's release tag are sufficient: document this decision in the Dockerfile comment
No published checksum for
bootstrapper.pysource files was found in public Elyra releases; you may need to compute it from the source or contact the Elyra team.
runtimes/pytorch+llmcompressor/ubi9-python-3.12/Dockerfile.cuda
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm (1)
72-77: Implementation is consistent with the rocm-pytorch file and correct.Version alignment (v4.3.1), curl error handling (
-fL), ownership preservation, and environment variable all match the intended pattern.Minor note: The comment on line 71 says "air-gapped environment," but the curl step requires network access during build. Based on past PR discussions, air-gapped support is intended to work through pre-cached builds or registry mirrors, and
ELYRA_INSTALL_PACKAGES=falseprevents re-installation at runtime. If clarity is needed, consider clarifying the comment to distinguish between build-time network requirements and runtime air-gapped behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu(1 hunks)runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu(1 hunks)runtimes/pytorch+llmcompressor/ubi9-python-3.12/Dockerfile.cuda(1 hunks)runtimes/pytorch/ubi9-python-3.12/Dockerfile.cuda(1 hunks)runtimes/rocm-pytorch/ubi9-python-3.12/Dockerfile.rocm(1 hunks)runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm(1 hunks)runtimes/tensorflow/ubi9-python-3.12/Dockerfile.cuda(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- runtimes/pytorch/ubi9-python-3.12/Dockerfile.cuda
- runtimes/tensorflow/ubi9-python-3.12/Dockerfile.cuda
- runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu
- runtimes/pytorch+llmcompressor/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). (12)
- GitHub Check: pytest-tests
- GitHub Check: check-generated-code
- GitHub Check: code-static-analysis
- GitHub Check: Trivy scan (fs)
- GitHub Check: Generate job matrix
- GitHub Check: Red Hat Konflux / odh-pipeline-runtime-minimal-cpu-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-pytorch-llmcompressor-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-pytorch-cuda-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-pipeline-runtime-pytorch-rocm-py312-ubi9-on-pull-request
🔇 Additional comments (2)
runtimes/rocm-pytorch/ubi9-python-3.12/Dockerfile.rocm (1)
71-76: Implementation looks good—bootstrapper provisioning is consistent and correct.All changes follow the intended pattern: copy utils with explicit ownership, download the correct Elyra v4.3.1 bootstrapper via curl (with
-fLfor error handling), and set the environment variable to disable re-installation.runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu (1)
77-84: Consistent and correct implementation across all three runtime Dockerfiles.The pattern is sound: copy utils with ownership preservation, fetch the correct Elyra v4.3.1 bootstrapper with robust error handling, and configure the environment to disable re-installation. All three files (rocm-pytorch, rocm-tensorflow, minimal) follow the same approach.
|
@jesuino: The following tests failed, say
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. |
|
I did test this with my local KFP installation and it seems to work. I see the following in logs: 
Notice that it skips the
This is good to merge! |
8 participants
Description
This is a preparation for Elyra 4.3.0 release. It is a working in progress because we need Elyra to be released. Elyra 4.3.0 should be released from main branch, hence a few environment properties changes are required in order to avoid breaking the images that use Elyra.
This PR include the following changes:
bootstrapper.pyremoving the need of having a copy hereELYRA_INSTALL_PACKAGES=false) so it will work on air-gapped environmentsHow Has This Been Tested?
We are waiting images to be available so we can make Elyra sanity checks.
Summary by CodeRabbit
Refactor
Chore
Configuration