RHAIENG-2042: chore(base-images): use AIPCC-style scripting instead of scl base to add s390x architecture to odh-base-image-cpu-py312-c9s
#2690
Conversation
github-actions
bot
added
the
review-requested
WalkthroughEnables linux/s390x in Tekton pipelines; replaces sclorg base with centos:stream9 for the Python 3.12 CPU image; adds CNB user, environment wiring, LD_PRELOAD and permission handling in the Dockerfile; introduces package-install and permission utility scripts; and adds a README helper to compare package lists. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
base-images/utils/fix-permissions (1)
5-8: Clarify env expectations and clean up meta-comments
- The
# added line/# modified linecomments are patch-artifacts rather than documentation; they’ll be confusing to future readers. Consider removing them or replacing them with a brief explanation of why ownership is forced toCNB_USER_ID/CNB_GROUP_ID.- With
${CNB_USER_ID:?}/${CNB_GROUP_ID:?}, the script will still exit non‑zero if those env vars are unset, even though the header says “Allow this script to fail without failing a build.” If the intent is “hard fail on misconfiguration, soft fail on chmod/chown errors,” this is fine; otherwise you may want to drop the:?guards or add a short comment clarifying that CNB_* must always be set in your images.Also applies to: 13-21, 23-31
base-images/utils/aipcc.sh (2)
4-7: ARCH is currently unused; either use it for conditionals or drop it
ARCH=${TARGETARCH}is never referenced, which is what ShellCheck is warning about. Given this PR’s focus on multi-arch support, it might be useful soon for arch-conditional package selections; otherwise it’s better to remove it to keep the script clean and silence SC2034.-ARCH=${TARGETARCH} +ARCH=${TARGETARCH} # reserved for future arch-conditional logicor, if you don’t expect to use it:
-ARCH=${TARGETARCH}
8-107: Double-check package lists and venv setup behavior across architecturesTwo things worth validating here:
Package lists vs s390x availability
The
PKGSandSCL_PACKAGESarrays are large and tuned to x86_64 today. On s390x (and potentially other arches), some of these packages may be missing or named differently, which would causednf installto fail before the image build completes. Since you already haveARCH=${TARGETARCH}available, consider:
- Guarding clearly non-portable or optional packages with simple arch checks (e.g.
[[ "$ARCH" != "s390x" ]] && PKGS+=(...)).- Or at least running a quick build/test on s390x to identify any failures and annotating/removing those entries.
Ensuring pip upgrades run inside the venv
install_python_venvdoes:"${PYTHON}" -m venv "${VIRTUAL_ENV}" "${PYTHON}" -m pip install --force-reinstall --upgrade \ --index-url https://pypi.org/simple/ \ pip setuptools wheelThis relies on two assumptions:
- The venv creates a
python${PYTHON_VERSION}binary in${VIRTUAL_ENV}/bin.${APP_ROOT}/binappears inPATHbefore/usr/bin, sopython${PYTHON_VERSION}now resolves to the venv binary.That’s likely true in this Dockerfile, but a small change in PATH or venv layout could accidentally send pip to the system interpreter.
To make the intent explicit and architecture‑agnostic, you could drive pip through the venv’s python directly:
function install_python_venv() {
- "${PYTHON}" -m venv "${VIRTUAL_ENV}"
- "${PYTHON}" -m pip install --force-reinstall --upgrade \
- "${PYTHON}" -m venv "${VIRTUAL_ENV}"
- "${VIRTUAL_ENV}/bin/python" -m pip install --force-reinstall --upgrade
--index-url https://pypi.org/simple/
pip setuptools wheel
}This guarantees the upgrades land in the venv regardless of PATH or arch. Also applies to: 111-327, 346-354, 356-376 </blockquote></details> <details> <summary>base-images/cpu/c9s-python-3.12/Dockerfile.cpu (2)</summary><blockquote> `49-56`: **Consider adding `-l` to useradd to address Hadolint DL3046** Hadolint is warning that `useradd` with a high UID and no `-l` can grow `/var/log/lastlog` unnecessarily. Since this is a container-only user, you can safely avoid creating lastlog/utmp entries: ```diff -RUN useradd -u ${CNB_USER_ID} -g ${CNB_GROUP_ID} -d ${HOME} -K HOME_MODE=0770 -K UMASK=0007 -m -s /bin/bash -c "Default Application User" default \ +RUN useradd -l -u ${CNB_USER_ID} -g ${CNB_GROUP_ID} -d ${HOME} -K HOME_MODE=0770 -K UMASK=0007 -m -s /bin/bash -c "Default Application User" default \ && mkdir -m 770 "${HOME}/.cache"This keeps the image leaner and should satisfy the linter without changing runtime behavior.
31-47: Verify architecture-specific paths (MPI_HOME, jemalloc) on s390xA couple of paths are assumed to be stable across all arches:
MPI_HOME=/usr/lib64/openmpiLD_PRELOAD=/usr/lib64/libjemalloc.so.2Given you’re enabling
linux/s390xbuilds, it’s worth double-checking that:
- OpenMPI installs into
/usr/lib64/openmpion s390x as well.- The
jemallocpackage provides/usr/lib64/libjemalloc.so.2on all target arches; otherwise the image will fail to start Python processes due to a brokenLD_PRELOAD.If any of these differ per arch, wiring them through
TARGETARCH(or guarding per-arch inaipcc.sh) will make the image more robust.Also applies to: 69-77
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.tekton/odh-base-image-cpu-py312-c9s-pull-request.yaml(1 hunks).tekton/odh-base-image-cpu-py312-c9s-push.yaml(1 hunks)base-images/cpu/c9s-python-3.12/Dockerfile.cpu(1 hunks)base-images/cpu/c9s-python-3.12/README.md(1 hunks)base-images/utils/aipcc.sh(2 hunks)base-images/utils/fix-permissions(1 hunks)base-images/utils/rpm-file-permissions(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Code static analysis
base-images/cpu/c9s-python-3.12/Dockerfile.cpu
[warning] 54-54: Hadolint DL3046: 'useradd' without flag '-l' and high UID will result in excessively large Image.
🪛 Shellcheck (0.11.0)
base-images/utils/aipcc.sh
[warning] 4-4: ARCH appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / odh-base-image-cpu-py312-c9s-on-pull-request
🔇 Additional comments (3)
.tekton/odh-base-image-cpu-py312-c9s-pull-request.yaml (1)
37-42: s390x enablement in PR pipeline LGTMAdding
linux/s390xhere is consistent with the existing multi-arch list and the PR goal; no further Tekton changes are needed in this file..tekton/odh-base-image-cpu-py312-c9s-push.yaml (1)
35-40: Push pipeline platform list is consistent with PR pipelineIncluding
linux/s390xalongside the other three architectures is coherent with the multi-arch strategy and mirrors the PR PipelineRun config.base-images/utils/rpm-file-permissions (1)
1-23: Upstream rpm-file-permissions script looks good as-isThis matches the upstream sclorg implementation closely and is nicely scoped to
filesystempaths underCHECK_DIRS; no issues from a correctness or safety standpoint in this image-build context.
jiridanek
changed the title
s390x architecture to odh-base-image-cpu-py312-c9ss390x architecture to odh-base-image-cpu-py312-c9s
jiridanek
force-pushed
the
jd_basec9sibmz
branch
from
b725e0c to
c662f99
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
base-images/utils/aipcc.sh (3)
4-4: Unused ARCH variable.The
ARCHvariable is assigned fromTARGETARCHbut never used in the script. If this is intended for future multi-architecture logic, consider either using it now or removing it to avoid confusion.Apply this diff if the variable is not needed:
-ARCH=${TARGETARCH} -
108-328: Verify necessity of SCL packages and track removal.This function installs 150+ packages as a temporary workaround. While the TODO comment on line 372 notes eventual removal, this represents significant technical debt:
- The large package set increases image size and attack surface
- Many packages may not be necessary for the target workloads
- The hack defeats the purpose of moving away from the SCL base
Consider:
- Profiling which SCL packages are actually used by downstream builds
- Creating a tracking issue for incremental removal
- Documenting which packages are required and why
Would you like me to open an issue to track the removal of unnecessary SCL packages?
361-364: Consider security update timing.Security updates are applied before installing packages. Since
dnf installwill pull the latest available versions anyway, this early security update step may be redundant. Consider either:
- Moving security updates after package installation to catch any security issues in newly installed packages
- Removing this step if packages are already being installed at their latest versions
base-images/utils/fix-permissions (1)
5-6: Consider the implications of always-succeeding behavior.The script uses
set +eto allow failures without breaking the build. While this provides resilience during image construction, it could mask real permission issues that would cause runtime failures.Consider logging more detailed diagnostics when operations fail, or optionally allowing strict mode via an environment variable for debugging purposes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.tekton/odh-base-image-cpu-py312-c9s-pull-request.yaml(1 hunks).tekton/odh-base-image-cpu-py312-c9s-push.yaml(1 hunks)base-images/cpu/c9s-python-3.12/Dockerfile.cpu(1 hunks)base-images/cpu/c9s-python-3.12/README.md(1 hunks)base-images/utils/aipcc.sh(2 hunks)base-images/utils/fix-permissions(1 hunks)base-images/utils/rpm-file-permissions(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- base-images/utils/rpm-file-permissions
- base-images/cpu/c9s-python-3.12/README.md
🧰 Additional context used
🪛 Shellcheck (0.11.0)
base-images/utils/aipcc.sh
[warning] 4-4: ARCH appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / odh-base-image-cpu-py312-c9s-on-pull-request
🔇 Additional comments (14)
base-images/utils/aipcc.sh (4)
8-106: LGTM: Comprehensive package installation.The function is well-structured with clear categorization of packages. The use of
${PYTHON:?}ensures the script will fail fast if PYTHON is not set.
338-344: LGTM: CRB enablement.Correctly enables the CentOS CodeReady Builder equivalent, which is necessary for accessing additional development packages.
346-354: LGTM: Python venv setup.The function correctly creates a Python virtual environment and ensures pip tooling is up-to-date. Using explicit
--index-urland--force-reinstallensures reproducible builds.
356-378: LGTM: Well-structured main flow with proper cleanup.The main function orchestrates the setup steps logically:
- Repository configuration (CSB, EPEL) first
- Package installation
- Validation (libzmq check)
- Python environment setup
- Cleanup (EPEL removal)
The EPEL install/uninstall pattern correctly ensures build-time dependencies don't remain in the final image.
.tekton/odh-base-image-cpu-py312-c9s-pull-request.yaml (1)
37-42: LGTM: s390x platform enabled for PR builds.This change correctly adds
linux/s390xto the build platforms for pull request validation, aligning with the PR objective to support s390x architecture..tekton/odh-base-image-cpu-py312-c9s-push.yaml (1)
35-40: LGTM: s390x platform enabled for push builds.This change correctly adds
linux/s390xto the build platforms for main branch pushes, consistent with the PR pipeline configuration.base-images/utils/fix-permissions (1)
24-29: LGTM: Permission fixing logic.The find commands correctly:
- Update ownership to CNB_USER_ID and CNB_GROUP_ID
- Ensure group read/write permissions
- Propagate execute permissions from user to group
- Ensure directories are group-executable
The use of
${CNB_USER_ID:?}ensures the script fails if these variables aren't set.base-images/cpu/c9s-python-3.12/Dockerfile.cpu (7)
3-10: LGTM: Base image switch enables s390x support.Switching from
quay.io/sclorg/python-312-c9stoquay.io/centos/centos:stream9is the correct approach to enable s390x architecture support, as the sclorg base lacks s390x builds.
12-24: LGTM: Comprehensive metadata.The image metadata follows Container Application Generic Labels standards and provides clear versioning and description information.
26-46: LGTM: Well-structured environment configuration.The environment setup is comprehensive and follows container best practices:
- Python virtual environment configuration
- Locale settings (en_US.UTF-8)
- Proper PATH configuration including MPI
- Disabling pip caching for smaller image size
- Setting Python encoding and unbuffered output
48-56: LGTM: CNB user setup follows OpenShift patterns.The user configuration correctly implements OpenShift's arbitrary UID support:
- Primary group is 0 (root) to support any UID with GID 0
- Directory permissions (0770) allow group write access
- HOME_MODE and UMASK ensure consistent permissions
This pattern is standard for OpenShift/Kubernetes compatibility.
61-70: LGTM: Efficient build with proper permission handling.The build steps correctly:
- Use BuildKit cache mounts for dnf to improve build performance
- Execute aipcc.sh to install packages and setup Python
- Apply permission fixes to APP_ROOT with
-Pflag (don't follow symlinks, contradicting fix-permissions default)- Normalize RPM file permissions
The use of
--mount=type=cachefor dnf is a best practice for faster rebuilds.
76-78: Verify LD_PRELOAD doesn't break other libraries.The jemalloc LD_PRELOAD is a known workaround for PyArrow's TLS allocation issue. While this is a legitimate fix, LD_PRELOAD affects all dynamically linked executables in the container and could potentially cause issues with other libraries.
Ensure this has been tested with the full workload, particularly:
- Other Python packages that use native extensions
- System utilities that might be affected by the memory allocator change
- Multi-threaded applications
73-74: LGTM: Proper user context and workspace.Correctly sets the working directory to APP_ROOT and switches to the CNB user for runtime operations, following the principle of least privilege.
| if ! [ -e "$1" ] ; then | ||
| echo "ERROR: File or directory $1 does not exist." >&2 | ||
| # We still want to end successfully | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Non-existent target handling could hide configuration errors.
When the target path doesn't exist, the script logs an error but exits successfully. This could mask configuration bugs (wrong paths in Dockerfile) that would be better caught at build time.
Consider whether this should be a hard error in some contexts. At minimum, ensure the error message is prominent enough to be noticed in build logs.
🤖 Prompt for AI Agents
In base-images/utils/fix-permissions around lines 17-21, the script currently
logs a missing-target message then exits 0 which can hide Dockerfile/path
errors; change the behavior to fail loudly: return non-zero (exit 1) when the
target does not exist, and make this controllable via an environment flag (e.g.
FAIL_ON_MISSING=true/false) so callers can opt into tolerant behavior; also
ensure the error message is written to stderr and is clearly prefixed so it
stands out in build logs.
There was a problem hiding this comment.
@coderabbitai create github issue for this, note that the script is simply somewhat modified https://github.com/sclorg/s2i-python-container/blob/master/3.13-minimal/root/usr/bin/fix-permissions
why this behavior was implemented in s2i/scl, I don't know
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for fix-permissions script behavior
gh issue create \
--repo opendatahub-io/notebooks \
--title "fix-permissions script exits successfully on missing target, potentially masking errors" \
--assignee jiridanek \
--body "## Context
The \`fix-permissions\` script at \`base-images/utils/fix-permissions\` currently logs an error but exits with status 0 when the target path doesn't exist (lines 17-21):
\`\`\`bash
if ! [ -e \"\$1\" ] ; then
echo \"ERROR: File or directory \$1 does not exist.\" >&2
# We still want to end successfully
exit 0
fi
\`\`\`
This behavior could mask configuration bugs (e.g., incorrect paths in Dockerfile) that would be better caught at build time.
## Background
The script is a modified version of the upstream s2i/scl script:
https://github.com/sclorg/s2i-python-container/blob/master/3.13-minimal/root/usr/bin/fix-permissions
The rationale for this behavior in the upstream script is unclear.
## Potential improvements
Consider one or more of the following:
- Make the script fail loudly (exit 1) when the target doesn't exist
- Make this behavior configurable via an environment flag (e.g., \`FAIL_ON_MISSING=true/false\`)
- Ensure the error message is prominent enough to be noticed in build logs
- Document the rationale for the current behavior if it should be preserved
## Related
- PR: https://github.com/opendatahub-io/notebooks/pull/2690
- Review comment: https://github.com/opendatahub-io/notebooks/pull/2690#discussion_r2543682042
- Requested by: @jiridanek"
Length of output: 55
@jiridanek, I've created issue #2691 to track this concern about the fix-permissions script behavior. The issue includes:
- Context about the exit-0-on-missing-target behavior
- A link to the upstream s2i/scl script you mentioned
- Potential improvement options
- Backlinks to this PR and review comment
You can find it here: #2691
…e-image-cpu-py312-c9s ``` [2/3] STEP 1/10: FROM quay.io/opendatahub/odh-base-image-cpu-py312-c9s:latest AS cpu-base Trying to pull quay.io/opendatahub/odh-base-image-cpu-py312-c9s:latest... Error: creating build container: unable to copy from source docker://quay.io/opendatahub/odh-base-image-cpu-py312-c9s:latest: choosing an image from manifest list docker://quay.io/opendatahub/odh-base-image-cpu-py312-c9s:latest: no image found in manifest list for architecture "s390x", variant "", OS "linux" ```
jiridanek
force-pushed
the
jd_basec9sibmz
branch
from
c662f99 to
8aa7d5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
base-images/utils/fix-permissions (1)
5-6: CNB_USER_ID / CNB_GROUP_ID now hard‑require env; header comment is slightly misleadingUsing
${CNB_USER_ID:?}and${CNB_GROUP_ID:?}in thefindlines means the script will abort with a non‑zero status if those variables are unset or empty, regardless ofset +eand the finalexit 0. That’s probably what you want for image builds (failing fast when the CNB IDs are missing), but it does contradict the “Allow this script to fail without failing a build” / “Always end successfully” comments.If the intent is “best‑effort fix unless the environment is misconfigured”, consider either:
- Making that intent explicit in the comments (e.g., “Script is best‑effort, but fails hard if CNB_USER_ID/CNB_GROUP_ID are missing”), or
- Replacing the
:?expansions with an explicit check up front that logs a clear error and exits 1 if the IDs are missing.While here, you might also optionally quote
"$SYMLINK_OPT"and"${CHECK_OWNER}"in thefindcalls to avoid accidental word‑splitting if those ever contain spaces.Also applies to: 23-32
base-images/utils/aipcc.sh (1)
8-106: Consider arch‑conditional handling for packages that may not exist on all architectures
install_packagesandinstall_scl_packagesunconditionally install a large set of RPMs, andmain()always calls both. Given the PR’s goal of adding s390x support, any package missing on a particular architecture will causednf install(and thus the whole script) to fail.You already capture
ARCHfromTARGETARCH; it might be worth using it to:
- Gate known arch‑specific or low‑value packages (e.g., GPU/GUI/HTTPD/NodeJS stacks) when they’re not needed or not available on s390x, or
- Split the lists into common vs. arch‑specific groups and only install the latter conditionally.
That would make the script more robust across architectures while still keeping parity with the SCL image where it matters.
Also applies to: 111-128, 356-378
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.tekton/odh-base-image-cpu-py312-c9s-pull-request.yaml(1 hunks).tekton/odh-base-image-cpu-py312-c9s-push.yaml(1 hunks)base-images/cpu/c9s-python-3.12/Dockerfile.cpu(1 hunks)base-images/cpu/c9s-python-3.12/README.md(1 hunks)base-images/utils/aipcc.sh(2 hunks)base-images/utils/fix-permissions(1 hunks)base-images/utils/rpm-file-permissions(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .tekton/odh-base-image-cpu-py312-c9s-pull-request.yaml
- base-images/utils/rpm-file-permissions
- base-images/cpu/c9s-python-3.12/Dockerfile.cpu
🧰 Additional context used
🪛 Shellcheck (0.11.0)
base-images/utils/aipcc.sh
[warning] 4-4: ARCH appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / odh-base-image-cpu-py312-c9s-on-pull-request
🔇 Additional comments (2)
base-images/cpu/c9s-python-3.12/README.md (1)
5-36: Helper script documentation looks consistent and self‑containedThe podman/rpm commands and the small Python helper give a clear, reproducible recipe for regenerating
SCL_PACKAGESfrom the two images. For the stated purpose in this README (one‑off list generation for this PR), the approach is reasonable and doesn’t need changes..tekton/odh-base-image-cpu-py312-c9s-push.yaml (1)
40-40: Enable s390x architecture support — verification complete, no issues found.All verification items confirm the change is correct and complete:
✅ Dockerfile readiness: Confirmed. The Dockerfile uses
quay.io/centos/centos:stream9as the base image, which supports s390x. No architecture-specific constraints found.✅ Consistency across pipelines: Confirmed. Both
.tekton/odh-base-image-cpu-py312-c9s-push.yamland.tekton/odh-base-image-cpu-py312-c9s-pull-request.yamlhave identical build-platforms configurations with s390x enabled.✅ Build platforms: All four architectures are properly configured (x86_64, arm64, ppc64le, s390x) in both pipeline files.
The change aligns with the PR objective and is ready for merge.
| ARCH=${TARGETARCH} | ||
|
|
||
| DNF_OPTS=(-y --nodocs --setopt=install_weak_deps=False --setopt=keepcache=True --setopt=max_parallel_downloads=10) | ||
|
|
There was a problem hiding this comment.
ARCH/TARGETARCH are unused but still enforced by set -u
ARCH=${TARGETARCH} combined with set -u means this script will fail immediately if TARGETARCH is not set, even though ARCH is never used afterwards (as ShellCheck notes).
If you don’t need arch‑specific behavior here yet, you can safely drop the assignment to avoid an unnecessary environment requirement. If you do plan to branch on architecture (e.g., to gate certain packages), consider actually using ARCH in the relevant install functions instead of leaving it unused.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 4-4: ARCH appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
In base-images/utils/aipcc.sh around lines 4 to 7, the script assigns
ARCH=${TARGETARCH} which, combined with set -u, causes an immediate failure if
TARGETARCH is unset even though ARCH is never used; remove that assignment to
avoid requiring TARGETARCH, or if architecture-specific behavior is intended,
actually use ARCH in install/branching logic (e.g., reference ARCH in package
selection or conditional blocks) and ensure TARGETARCH has a safe default or is
checked before use so the script no longer fails under set -u.
There was a problem hiding this comment.
@jiridanek, thanks for clarifying! Since TARGETARCH is set in the Dockerfile, the set -u concern is resolved. The unused ARCH variable is just a minor linting note—if you're planning to use it for architecture-specific logic later or keeping it for consistency with AIPCC patterns, feel free to leave it as is.
| # create Python virtual env and update pip inside the venv | ||
| function install_python_venv() { | ||
| # install venv with bundled pip (no --upgrade-deps) | ||
| "${PYTHON}" -m venv "${VIRTUAL_ENV}" | ||
|
|
||
| "${PYTHON}" -m pip install --force-reinstall --upgrade \ | ||
| --index-url https://pypi.org/simple/ \ | ||
| pip setuptools wheel | ||
| } |
There was a problem hiding this comment.
Ensure pip/setuptools/wheel are upgraded inside the venv, not in the system env
In install_python_venv, the same ${PYTHON} is used both to create the venv and to run -m pip. Unless PYTHON already points at the interpreter inside ${VIRTUAL_ENV}, this will upgrade pip/setuptools/wheel for the global interpreter, not for the newly created venv.
To guarantee you’re operating on the venv, it’s safer to drive pip via the venv’s own python:
function install_python_venv() {
# install venv with bundled pip (no --upgrade-deps)
- "${PYTHON}" -m venv "${VIRTUAL_ENV}"
-
- "${PYTHON}" -m pip install --force-reinstall --upgrade \
+ "${PYTHON}" -m venv "${VIRTUAL_ENV}"
+
+ "${VIRTUAL_ENV}/bin/python" -m pip install --force-reinstall --upgrade \
--index-url https://pypi.org/simple/ \
pip setuptools wheel
}This keeps the venv self‑contained and avoids surprising the base interpreter.
📝 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.
| # create Python virtual env and update pip inside the venv | |
| function install_python_venv() { | |
| # install venv with bundled pip (no --upgrade-deps) | |
| "${PYTHON}" -m venv "${VIRTUAL_ENV}" | |
| "${PYTHON}" -m pip install --force-reinstall --upgrade \ | |
| --index-url https://pypi.org/simple/ \ | |
| pip setuptools wheel | |
| } | |
| # create Python virtual env and update pip inside the venv | |
| function install_python_venv() { | |
| # install venv with bundled pip (no --upgrade-deps) | |
| "${PYTHON}" -m venv "${VIRTUAL_ENV}" | |
| "${VIRTUAL_ENV}/bin/python" -m pip install --force-reinstall --upgrade \ | |
| --index-url https://pypi.org/simple/ \ | |
| pip setuptools wheel | |
| } |
🤖 Prompt for AI Agents
In base-images/utils/aipcc.sh around lines 346 to 354, the script creates the
venv with "${PYTHON}" but then calls pip with the same "${PYTHON}", which may
upgrade the global interpreter; change the pip invocation to use the venv's own
Python interpreter (e.g. "${VIRTUAL_ENV}/bin/python" on Unix or the equivalent
Scripts\python.exe on Windows) when running "-m pip install --upgrade pip
setuptools wheel" so the upgrades happen inside the newly created virtualenv and
not the system Python.
There was a problem hiding this comment.
create github issue; the current implementation is what aipcc does
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for pip upgrade concern in install_python_venv()
gh issue create \
--repo opendatahub-io/notebooks \
--title "install_python_venv() may upgrade system Python instead of venv" \
--body "## Context
In \`base-images/utils/aipcc.sh\`, the \`install_python_venv()\` function creates a virtual environment but then uses \`\${PYTHON}\` to upgrade pip/setuptools/wheel:
\`\`\`bash
function install_python_venv() {
# install venv with bundled pip (no --upgrade-deps)
\"\${PYTHON}\" -m venv \"\${VIRTUAL_ENV}\"
\"\${PYTHON}\" -m pip install --force-reinstall --upgrade \\
--index-url https://pypi.org/simple/ \\
pip setuptools wheel
}
\`\`\`
## Issue
Using \`\${PYTHON}\` for the pip upgrade may upgrade the system/base Python interpreter instead of the venv's Python, unless \`PYTHON\` already points to the venv interpreter.
## Note
Per @jiridanek: \"the current implementation is what aipcc does\" - this follows the existing AIPCC pattern.
## Backlinks
- PR: #2690
- Comment: https://github.com/opendatahub-io/notebooks/pull/2690#discussion_r2543745529
- Requested by: @jiridanek" \
--assignee jiridanek
Length of output: 55
@jiridanek, I've created issue #2692 to track the install_python_venv() pip upgrade concern. The issue documents that this follows the existing AIPCC pattern and includes backlinks to this PR and comment.
🐰✨
/>_/)
(='.'=)
(")_(")
[Task done!]
|
@jiridanek: The following test 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ide-developer 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 |
2 participants
https://issues.redhat.com/browse/RHAIENG-2042
The problem is that https://quay.io/repository/sclorg/python-312-c9s?tab=tags does not provide s390x build, and therefore, we were unable to create our odh-base-image-cpu-py312-c9s
To fix this, we need to build from raw centos:stream9 and install python on top of that.
Description
How Has This Been Tested?
Self checklist (all need to be checked):
make test(gmakeon macOS) before asking for reviewDockerfile.konfluxfiles should be done inodh/notebooksand automatically synced torhds/notebooks. For Konflux-specific changes, modifyDockerfile.konfluxfiles directly inrhds/notebooksas these require special attention in the downstream repository and flow to the upcoming RHOAI release.Merge criteria:
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.