[rhoai-2.25] fix(build): derive native ONNX/pyarrow versions from pylock.toml#2519
Conversation
📝 WalkthroughWalkthroughThe pull request adds lockfile-based dependency version resolution to native builds, updates setup scripts and Dockerfiles, adds resolver validation, tightens a shell-analysis rule with regression tests, and introduces a quiet/noisy pytest subtest decorator. ChangesPylock-driven native dependency builds
Semgrep dangerous-command detection
Pytest subtest utility
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 (4)
tests/unit/scripts/test_pylock_version.py (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd descriptive messages to single
assertstatements.As per coding guidelines, plain
assertstatements should include descriptive messages for single assertions. For example,assert result.stdout.strip() == "1.22.0"should beassert result.stdout.strip() == "1.22.0", f"expected onnx ppc64le version, got {result.stdout.strip()}".Also applies to: 19-19, 30-30, 47-47, 68-68, 82-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/scripts/test_pylock_version.py` at line 15, Add descriptive failure messages to each single assert in test_pylock_version.py, including the assertions around pylock_version.format_version and the other referenced test cases. Keep each assertion’s existing condition unchanged and make each message identify the expected result and the actual value.Source: Coding guidelines
scripts/pylock_version.py (1)
88-88: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
fnmatch.fnmatchcasetreats?and[]as special characters.When
expectedends with.*,fnmatchis used for wildcard matching. Besides*, fnmatch also treats?and[seq]as glob patterns. If a marker expected value contains?or[, it would be interpreted as a glob pattern rather than a literal. For version strings this is unlikely, but a simpler check likeactual.startswith(expected[:-2])would be more precise for the.*wildcard case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/pylock_version.py` at line 88, Update the version comparison logic in pylock_version.py so the expected-ending-`.*` case uses a literal prefix check via actual.startswith(expected[:-2]) instead of fnmatch.fnmatchcase. Preserve exact equality for expected values without the `.*` suffix, ensuring characters such as `?` and `[` remain literal.jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh (2)
51-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuote
${TORCH_TAG}in thegit clonecommand.The variable
${TORCH_TAG}is unquoted on line 54. While the value originates frompylock_version.pyand is expected to be a clean tag likev2.7.1, quoting it prevents globbing/word splitting if the script ever returns an unexpected value.♻️ Suggested fix
- git clone --recursive https://github.com/pytorch/pytorch.git -b ${TORCH_TAG} + git clone --recursive https://github.com/pytorch/pytorch.git -b "${TORCH_TAG}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh` around lines 51 - 54, Quote the ${TORCH_TAG} argument in the git clone command within the devel environment setup script, preserving the existing recursive clone and repository branch selection.
101-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuote command substitution on the
accelerateinstall line.The
$(python3 ./pylock_version.py accelerate --platform x86_64)substitution is unquoted. Quote it to prevent word splitting.♻️ Suggested fix
- uv pip install --refresh ${WHEELS_DIR}/*.whl accelerate==$(python3 ./pylock_version.py accelerate --platform x86_64) + uv pip install --refresh ${WHEELS_DIR}/*.whl "accelerate==$(python3 ./pylock_version.py accelerate --platform x86_64)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh` at line 101, Quote the command substitution used for the accelerate version in the install command so its output is passed as a single argument. Update only the accelerate version expression on the uv pip install line, preserving the existing package and platform arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu`:
- Around line 64-73: Update the version lookups in the ppc64le profile to
request formatted values from pylock_version.py: use the git-tag format for ONNX
and apache-arrow-branch for PyArrow. Adjust the downstream onnx-builder and
arrow-builder references, plus the corresponding s390x builder lookups, to
consume these formatted values without manually prepending v or apache-arrow-.
Preserve the existing version variables and platform-specific behavior.
---
Nitpick comments:
In `@jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh`:
- Around line 51-54: Quote the ${TORCH_TAG} argument in the git clone command
within the devel environment setup script, preserving the existing recursive
clone and repository branch selection.
- Line 101: Quote the command substitution used for the accelerate version in
the install command so its output is passed as a single argument. Update only
the accelerate version expression on the uv pip install line, preserving the
existing package and platform arguments.
In `@scripts/pylock_version.py`:
- Line 88: Update the version comparison logic in pylock_version.py so the
expected-ending-`.*` case uses a literal prefix check via
actual.startswith(expected[:-2]) instead of fnmatch.fnmatchcase. Preserve exact
equality for expected values without the `.*` suffix, ensuring characters such
as `?` and `[` remain literal.
In `@tests/unit/scripts/test_pylock_version.py`:
- Line 15: Add descriptive failure messages to each single assert in
test_pylock_version.py, including the assertions around
pylock_version.format_version and the other referenced test cases. Keep each
assertion’s existing condition unchanged and make each message identify the
expected result and the actual value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 885da1da-c633-4460-b966-3211a0a29c60
📒 Files selected for processing (14)
codeserver/ubi9-python-3.12/Dockerfile.cpucodeserver/ubi9-python-3.12/Dockerfile.konflux.cpucodeserver/ubi9-python-3.12/devel_env_setup.shjupyter/datascience/ubi9-python-3.12/Dockerfile.cpujupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpujupyter/trustyai/ubi9-python-3.12/Dockerfile.cpujupyter/trustyai/ubi9-python-3.12/Dockerfile.konflux.cpujupyter/trustyai/ubi9-python-3.12/devel_env_setup.shruntimes/datascience/ubi9-python-3.12/Dockerfile.cpuruntimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpuscripts/pylock_version.pytests/conftest.pytests/test_native_build_versions.pytests/unit/scripts/test_pylock_version.py
|
/build-konflux |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/pylock_version.py (1)
174-197: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTuple
metavarwithnargs="+"causes a crash when displaying help, and displays arguments in the wrong order.The test case "explicit-pylock" (line 40-48 in
test_pylock_version.py) confirms the actual argument contract isPYLOCK PACKAGE, butmetavar=("PACKAGE", "PYLOCK")displays them in reverse order.Additionally, calling
--helpcrashes withValueError: too many values to unpack (expected 1)when argparse tries to format the metavar for the help message. While the crash occurs at help-formatting time (not atadd_argument()time), it makes the help unavailable in practice.🛡️ Proposed fix
parser.add_argument( "positional", nargs="+", - metavar=("PACKAGE", "PYLOCK"), + metavar="PACKAGE_OR_PYLOCK_PACKAGE", help="PACKAGE, or PYLOCK PACKAGE when pylock.toml is not in the cwd", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/pylock_version.py` around lines 174 - 197, Update the positional argument definition in _parse_args so nargs="+" uses a single safe metavar rather than a two-item tuple, preventing argparse help formatting from crashing. Ensure the help text reflects the actual accepted forms, especially the two-argument PYLOCK PACKAGE order, while preserving the existing argument parsing behavior.
🧹 Nitpick comments (1)
tests/unit/scripts/test_pylock_version.py (1)
101-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
subtestsfor the per-marker loop.
test_native_build_pylocks_have_parseable_markersiterates over every marker in a lockfile and stops at the first failure, hiding any other bad markers in the same run. As per coding guidelines,tests/**/*.pyshould Usesubtestsfrom pytest-subtests plugin for iterating over multiple items with per-item assertions.♻️ Proposed refactor using `subtests`
-def test_native_build_pylocks_have_parseable_markers(pylock_version, pylock: Path) -> None: +def test_native_build_pylocks_have_parseable_markers(pylock_version, subtests, pylock: Path) -> None: doc = tomllib.loads(pylock.read_text()) markers = {entry["marker"] for entry in doc.get("packages", []) if entry.get("marker")} assert markers, f"expected markers in {pylock}" env = pylock_version.marker_env(python_minor="3.12", platform_machine="x86_64") for marker in markers: - pylock_version.evaluate_marker(marker, env) + with subtests.test(marker=marker): + pylock_version.evaluate_marker(marker, env)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/scripts/test_pylock_version.py` around lines 101 - 107, Update test_native_build_pylocks_have_parseable_markers to use the pytest-subtests fixture for the per-marker loop, wrapping each marker evaluation in its own named subtest so all markers are evaluated and failures are reported independently.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/pylock_version.py`:
- Around line 34-48: Update _ALLOWED_NAMES to match the keys actually populated
by marker_env(): either add os_name, platform_release, platform_version, and
extra to marker_env() with correct values, or remove them from _ALLOWED_NAMES so
_eval_marker() rejects unsupported markers with ValueError instead of evaluating
empty defaults.
---
Outside diff comments:
In `@scripts/pylock_version.py`:
- Around line 174-197: Update the positional argument definition in _parse_args
so nargs="+" uses a single safe metavar rather than a two-item tuple, preventing
argparse help formatting from crashing. Ensure the help text reflects the actual
accepted forms, especially the two-argument PYLOCK PACKAGE order, while
preserving the existing argument parsing behavior.
---
Nitpick comments:
In `@tests/unit/scripts/test_pylock_version.py`:
- Around line 101-107: Update test_native_build_pylocks_have_parseable_markers
to use the pytest-subtests fixture for the per-marker loop, wrapping each marker
evaluation in its own named subtest so all markers are evaluated and failures
are reported independently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6403627d-11b8-48c4-9153-f9e9fbcb227a
📒 Files selected for processing (8)
jupyter/datascience/ubi9-python-3.12/Dockerfile.cpujupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpujupyter/trustyai/ubi9-python-3.12/devel_env_setup.shruntimes/datascience/ubi9-python-3.12/Dockerfile.cpuruntimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpuscripts/pylock_version.pytests/test_native_build_versions.pytests/unit/scripts/test_pylock_version.py
🚧 Files skipped from review as they are similar to previous changes (4)
- jupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu
- jupyter/datascience/ubi9-python-3.12/Dockerfile.cpu
- runtimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu
- runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu
jiridanek
changed the title
|
/build-konflux |
|
/hold this reveals additional build problems, https://redhat-internal.slack.com/archives/C0BD6LZUEGJ/p1783859157800919 |
|
/unhold 2.25.10 development is now open |
622ceb8 to
76a32de
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_native_build_versions.py (1)
26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
subtestsfor the Dockerfile loop. Wrap each Dockerfile/pattern/package check in a namedsubtests.test(...)scope so failures point to the specific item instead of the whole loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_native_build_versions.py` around lines 26 - 33, Update the Dockerfile validation loop in the test to wrap each dockerfile, forbidden-pattern, and package assertion in a named subtests.test(...) scope, using the relevant Dockerfile and item names so failures identify the specific check while preserving the existing assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_native_build_versions.py`:
- Around line 31-33: Update the regex assertion in the test loop over onnx and
pyarrow so it matches the pylock_version.py resolver invocation together with
the target package as its argument, rather than allowing unrelated later
mentions via re.DOTALL. Keep the assertion tied to each package and preserve the
existing failure message.
- Around line 16-17: Assign Allure metadata to every affected test: add the
missing allure import, then add matching `@allure.description`() and
`@allure.issue`(...) decorators to tests/test_native_build_versions.py lines 16-17
and 36-37, and tests/unit/scripts/test_pylock_version.py lines 54, 65, 70, 80,
85, 108, and 119-125. Preserve the existing test behavior and use the matching
issue links for each test.
---
Nitpick comments:
In `@tests/test_native_build_versions.py`:
- Around line 26-33: Update the Dockerfile validation loop in the test to wrap
each dockerfile, forbidden-pattern, and package assertion in a named
subtests.test(...) scope, using the relevant Dockerfile and item names so
failures identify the specific check while preserving the existing assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3887ae48-93cb-4a14-8645-c723bc32fe46
📒 Files selected for processing (11)
jupyter/datascience/ubi9-python-3.12/Dockerfile.cpujupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpujupyter/trustyai/ubi9-python-3.12/Dockerfile.cpujupyter/trustyai/ubi9-python-3.12/Dockerfile.konflux.cpujupyter/trustyai/ubi9-python-3.12/devel_env_setup.shruntimes/datascience/ubi9-python-3.12/Dockerfile.cpuruntimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpuscripts/pylock_version.pytests/conftest.pytests/test_native_build_versions.pytests/unit/scripts/test_pylock_version.py
🚧 Files skipped from review as they are similar to previous changes (7)
- jupyter/trustyai/ubi9-python-3.12/Dockerfile.konflux.cpu
- tests/conftest.py
- jupyter/datascience/ubi9-python-3.12/Dockerfile.cpu
- runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu
- jupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu
- runtimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu
- scripts/pylock_version.py
|
@coderabbitai Code Review: PR #2519 — [rhoai-2.25] fix(build): derive native ONNX/pyarrow versions from pylock.toml Overview Replaces hardcoded ARG ONNX_VERSION=v1.20.1 / ARG PYARROW_VERSION=17.0.0 across 8 Dockerfiles (4 ODH .cpu + 4 Konflux .konflux.cpu) with a new stdlib-only scripts/pylock_version.py that reads pinned versions from each workbench's pylock.toml. Also adds a PEP 508 marker evaluator, ONNX 1.22.0 11 files changed, +465 / -51. Positives
Issues
devel_env_setup.sh:138 and :171:
devel_env_setup.sh:155:
runtimes/datascience/.../Dockerfile.cpu:197-208: The heredoc switched from <<'PROFILE_EOF' (literal) to <<PROFILE_EOF (expanding) to inject ${ONNX_VERSION} and ${PYARROW_VERSION}. The $ escapes on LD_LIBRARY_PATH and PATH are correct. However, adding any new $VAR line to this profile script in the future will
tests/test_native_build_versions.py:33:
@pytest.fixture(scope="session")
tests/test_native_build_versions.py:40:
scripts/pylock_version.py:49:
metavar=("PACKAGE", "PYLOCK"), Security
Summary Well-structured change that solves a real version-drift problem. The marker evaluator is solid, the test coverage is good, and the Dockerfile changes are mechanically correct. The main actionable items are:
|
jiridanek
left a comment
jiridanek
left a comment
There was a problem hiding this comment.
Nice change — single source of truth for native build versions is a real improvement. The marker evaluator is solid and the ast.parse approach is safe.
Main actionable items in the inline comments:
- Comment the
--platform x86_64inside ppc64le blocks — correct but looks like a bug to future readers - Drop
re.DOTALLfrom the test regex — currently matches across unrelated lines - Fix metavar order in
_parse_args—--helpshows args backwards - Quote remaining
${PYARROW_VERSION}/${PILLOW_VERSION}indevel_env_setup.shfor consistency with theTORCH_TAGfix
| # Torch | ||
| cd ${CURDIR} | ||
| TORCH_VERSION=$(grep -A1 '"torch"' pylock.toml | grep -Eo '\b[0-9\.]+\b') | ||
| TORCH_VERSION=$(python3 ./pylock_version.py torch --platform x86_64) |
There was a problem hiding this comment.
This resolves the x86_64 marker version inside a ppc64le block. It's correct — torch only has x86_64 markers in the pylock (no ppc64le wheel exists, it's built from source) — but a future reader will think it's a copy-paste bug.
A one-line comment would prevent a well-intentioned "fix":
# torch is pinned under x86_64 markers; we build the same version from source on ppc64le
TORCH_VERSION=$(python3 ./pylock_version.py torch --platform x86_64)Same applies to the accelerate call on line 101.
There was a problem hiding this comment.
@jiridanek, agreed. Resolving against the x86_64 marker is intentional for the ppc64le source-build path, and both Torch and Accelerate calls should document that rationale to avoid a misleading future “fix.”
✏️ Learnings added
Learnt from: jiridanek
Repo: red-hat-data-services/notebooks PR: 2519
File: jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh:51-51
Timestamp: 2026-07-21T16:31:44.220Z
Learning: In `jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh`, the ppc64le native-wheel build intentionally resolves Torch and Accelerate versions with `scripts/pylock_version.py --platform x86_64`: Torch has no ppc64le wheel lock entry because it is built from source on ppc64le, and the x86_64 marker selects the canonical pinned version. Future reviews should not treat these x86_64 platform arguments as copy-paste errors; explanatory comments at both call sites are appropriate.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Addressed in e9e299c — added comments above the torch and accelerate --platform x86_64 lookups explaining that the pylock pins use x86_64 markers while we build/install the same version on ppc64le.
| cd ${CURDIR} | ||
| # Pyarrow | ||
| PYARROW_VERSION=$(grep -A1 '"pyarrow"' pylock.toml | grep -Eo '\b[0-9\.]+\b') | ||
| PYARROW_VERSION=$(python3 ./pylock_version.py pyarrow --platform ppc64le) |
There was a problem hiding this comment.
This PR already fixed quoting for TORCH_TAG (nice), but PYARROW_VERSION here is still unquoted. Same for PILLOW_VERSION on line 90. Word splitting on a version string is unlikely but free to prevent:
git clone --recursive https://github.com/apache/arrow.git -b "apache-arrow-${PYARROW_VERSION}"There was a problem hiding this comment.
@jiridanek Agreed—good catch. Both the PyArrow and Pillow git clone -b arguments should be quoted, matching the existing TORCH_TAG handling:
git clone --recursive https://github.com/apache/arrow.git -b "apache-arrow-${PYARROW_VERSION}"
git clone --recursive https://github.com/python-pillow/Pillow.git -b "${PILLOW_VERSION}"✏️ Learnings added
Learnt from: jiridanek
Repo: red-hat-data-services/notebooks PR: 2519
File: jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh:67-67
Timestamp: 2026-07-21T16:31:52.445Z
Learning: In `jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh`, quote dynamically expanded version/tag arguments passed to shell commands (including `git clone -b`) to prevent word splitting and preserve consistency with the quoted `TORCH_TAG` usage.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Addressed in e9e299c — quoted apache-arrow-${PYARROW_VERSION} and "${PILLOW_VERSION}" in the git clone -b args.
| if [ "$TARGETARCH" = "ppc64le" ]; then | ||
| ONNX_VERSION=$(python3 /tmp/pylock_version.py /tmp/pylock.toml onnx --platform ppc64le) | ||
| PYARROW_VERSION=$(python3 /tmp/pylock_version.py /tmp/pylock.toml pyarrow --platform ppc64le) | ||
| cat > /etc/profile.d/ppc64le.sh <<PROFILE_EOF |
There was a problem hiding this comment.
The heredoc switched from <<'PROFILE_EOF' (literal, no expansion) to <<PROFILE_EOF (expanding) so ${ONNX_VERSION} and ${PYARROW_VERSION} are injected at build time. The \$ escapes on LD_LIBRARY_PATH and PATH are correct.
However, this is a subtle trap: adding any new $VAR line to this profile script in the future will silently expand at build time instead of at login time. Consider a comment at the heredoc marker:
# NB: unquoted heredoc — escape $ for vars that must expand at login
cat > /etc/profile.d/ppc64le.sh <<PROFILE_EOFThere was a problem hiding this comment.
@jiridanek, agreed. The proposed comment clearly documents the intentional unquoted-heredoc behavior and the need to escape variables that must remain runtime-expanded. This would be a worthwhile maintainability safeguard.
✏️ Learnings added
Learnt from: jiridanek
Repo: red-hat-data-services/notebooks PR: 2519
File: runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu:67-67
Timestamp: 2026-07-21T16:31:48.430Z
Learning: In `runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu`, the unquoted `PROFILE_EOF` heredoc that generates `/etc/profile.d/ppc64le.sh` intentionally expands the resolved ONNX and PyArrow versions during the image build. Any variables intended to expand when the profile is sourced must be escaped as `\$VAR`; document this behavior at the heredoc marker when modifying the block.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Addressed in e9e299c — added # NB: unquoted heredoc — escape $ for vars that must expand at login above the PROFILE_EOF heredoc in both runtime datascience Dockerfiles.
| assert "pylock_version.py" in text, f"{dockerfile.name} should copy pylock_version.py" | ||
| for package in ("onnx", "pyarrow"): | ||
| lookup = re.compile(rf"pylock_version\.py.*\b{package}\b", re.DOTALL) | ||
| assert lookup.search(text), f"{dockerfile.name} should resolve {package} via pylock_version.py" |
There was a problem hiding this comment.
re.DOTALL makes . match newlines, so this succeeds if pylock_version.py appears on line 10 and onnx appears on line 200 in any unrelated context (e.g., a pip install onnx comment). Drop re.DOTALL to make the assertion meaningful:
lookup = re.compile(rf"pylock_version\.py\s+(?:\S+\s+)?{package}\b")There was a problem hiding this comment.
@jiridanek, agreed. re.DOTALL makes this test susceptible to unrelated later occurrences of onnx or pyarrow, so it does not prove that the resolver is invoked for that package.
- Update
tests/test_native_build_versions.pyto replace the DOTALL-based lookup with the constrained same-command pattern:lookup = re.compile(rf"pylock_version\.py\s+(?:\S+\s+)?{package}\b")
- This retains support for both supported CLI forms—
pylock_version.py PACKAGEandpylock_version.py PYLOCK PACKAGE—while preventing cross-line unrelated matches.
✏️ Learnings added
Learnt from: jiridanek
Repo: red-hat-data-services/notebooks PR: 2519
File: tests/test_native_build_versions.py:33-33
Timestamp: 2026-07-21T16:31:54.516Z
Learning: In `tests/test_native_build_versions.py`, Dockerfile assertions that verify `scripts/pylock_version.py` resolves a package must bind the resolver path and package argument to the same command context. Do not use a `re.DOTALL` pattern such as `pylock_version\.py.*\b{package}\b`, because it can match unrelated later Dockerfile content; use a constrained pattern that allows the optional pylock path and then requires the package argument.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Failed to handle agent chat message. Please try again.
There was a problem hiding this comment.
Addressed in e9e299c — dropped re.DOTALL and tightened the pattern to \bpylock_version\.py\s+(?:\S+\s+)?{package}\b so it only matches the resolver invocation.
| def test_pylock_pins_resolve_for_native_arches(image_dir: Path, pylock_version) -> None: | ||
| pylock = image_dir / "pylock.toml" | ||
| locked_version = pylock_version.locked_version | ||
| onnx_ppc64le = locked_version(pylock, "onnx", platform_machine="ppc64le") |
There was a problem hiding this comment.
This enforces that ppc64le and s390x pin the same pyarrow version. Is this an intended contract or an incidental coincidence? If they ever diverge intentionally (e.g., s390x needs a patched version), this test breaks. Consider removing this assertion or adding a comment explaining the invariant.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '=== test outline and relevant lines ==='
ast-grep outline tests/test_native_build_versions.py --items all || true
sed -n '1,120p' tests/test_native_build_versions.py
printf '%s\n' '=== PyArrow lock entries and platform markers ==='
for pylock in \
jupyter/datascience/ubi9-python-3.12/pylock.toml \
runtimes/datascience/ubi9-python-3.12/pylock.toml
do
printf '\n--- %s ---\n' "$pylock"
rg -n -i -C 5 'name\s*=\s*"pyarrow"|marker|version\s*=' "$pylock"
done
printf '%s\n' '=== Other references to the cross-architecture assertion ==='
rg -n -C 3 'pyarrow_(ppc64le|s390x)|ppc64le.*s390x|s390x.*ppc64le' \
tests scripts jupyter runtimesLength of output: 201226
@jiridanek, agreed. The lockfile currently uses one pyarrow 17.0.0 entry whose marker covers both architectures, but that is current data rather than an established cross-architecture contract. Since this PR adds platform-marker-aware resolution, the equality assertion would wrongly reject a future intentional s390x-specific pin.
I support removing the pyarrow_ppc64le == pyarrow_s390x assertion and retaining independent resolution/non-empty checks for each architecture.
There was a problem hiding this comment.
Addressed in e9e299c — removed the pyarrow_ppc64le == pyarrow_s390x equality check. The test now only asserts that each arch resolves a non-empty pin independently.
| f"package {package!r} not found in {pylock_path} " | ||
| f"for platform_machine={platform_machine!r} python={resolved_python!r}" | ||
| ) | ||
| return packages[package]["version"] |
There was a problem hiding this comment.
Minor: the metavar tuple says ("PACKAGE", "PYLOCK") but the actual positional order is PYLOCK PACKAGE (pylock first when two args are given, per the match at line 178). This makes --help show them backwards:
positional arguments:
PACKAGE ...
PYLOCK ...
Should be metavar=("PYLOCK", "PACKAGE") or metavar=("ARG",).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline scripts/pylock_version.py --items all --type function --match '_parse_args|main'
nl -ba scripts/pylock_version.py | sed -n '165,215p'Length of output: 696
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '172,215p' scripts/pylock_version.pyLength of output: 1720
@jiridanek, confirmed. scripts/pylock_version.py parses the two-argument form as PYLOCK PACKAGE, but Line 177 declares the metavars in reverse order. Updating it to the following will make --help consistent with the actual CLI contract:
metavar=("PYLOCK", "PACKAGE")There was a problem hiding this comment.
Addressed in e9e299c — switched to metavar="ARG" so --help no longer shows the positionals backwards. The help text still documents the PACKAGE / PYLOCK PACKAGE contract.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline scripts/pylock_version.py --items all
ast-grep outline tests/test_native_build_versions.py --items all
ast-grep outline tests/conftest.py --items all
for f in \
jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh \
runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu \
tests/test_native_build_versions.py \
tests/conftest.py \
scripts/pylock_version.py
do
echo "===== $f ====="
nl -ba "$f" | sed -n '1,260p'
done
echo "===== pylock package entries and architecture markers ====="
rg -n -A10 -B2 'name = "(torch|accelerate|pyarrow|onnx|pillow)"|marker' \
jupyter/trustyai/ubi9-python-3.12/pylock.toml \
jupyter/datascience/ubi9-python-3.12/pylock.toml \
runtimes/datascience/ubi9-python-3.12/pylock.tomlLength of output: 2654 🏁 Script executed: #!/bin/bash
set -euo pipefail
print_numbered() {
local file="$1"
echo "===== $file ====="
sed -n '=' "$file" | sed 'N;s/\n/\t/'
}
for f in \
jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh \
runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu \
tests/test_native_build_versions.py \
tests/conftest.py \
scripts/pylock_version.py
do
print_numbered "$f"
done
echo "===== pylock package entries and architecture markers ====="
rg -n -A12 -B2 'name = "(torch|accelerate|pyarrow|onnx|pillow)"|marker' \
jupyter/trustyai/ubi9-python-3.12/pylock.toml \
jupyter/datascience/ubi9-python-3.12/pylock.toml \
runtimes/datascience/ubi9-python-3.12/pylock.tomlLength of output: 201214 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_range() {
local file="$1"
local range="$2"
echo "===== $file:$range ====="
sed -n "${range}p" "$file" | awk '{ printf "%6d %s\n", NR, $0 }'
}
show_range jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh '125,180'
show_range runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu '185,215'
show_range tests/test_native_build_versions.py '1,80'
show_range tests/conftest.py '1,60'
show_range scripts/pylock_version.py '40,215'
echo '===== relevant TrustyAI package tables ====='
awk '
/^\[\[packages\]\]$/ {
if (block ~ /^name = "(torch|accelerate|pyarrow|pillow)"$/m) print block
block = ""
}
{ block = block $0 ORS }
END { if (block ~ /^name = "(torch|accelerate|pyarrow|pillow)"$/m) print block }
' jupyter/trustyai/ubi9-python-3.12/pylock.tomlLength of output: 201343 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '===== recovered cited source ranges ====='
sed -n '1,320p' /tmp/coderabbit-shell-logs/shell-output-GItlpj
echo '===== TrustyAI package marker summaries ====='
awk '
/^\[\[packages\]\]$/ { name = version = marker = "" }
/^name = "(torch|accelerate|pyarrow|pillow)"$/ { name = $0 }
/^version = / { if (name != "") version = $0 }
/^marker = / {
if (name != "") {
marker = $0
print name ORS version ORS marker ORS ""
}
}
' jupyter/trustyai/ubi9-python-3.12/pylock.tomlLength of output: 22509 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '===== TrustyAI native-build constructs ====='
rg -n -C3 -- \
'pylock_version\.py|TORCH_VERSION|PYARROW_VERSION|PILLOW_VERSION|WHEELS_DIR|git clone|uname -m|--platform x86_64' \
jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh || true
echo '===== runtime profile heredoc and deferred expansions ====='
rg -n -C5 -- \
'PROFILE_EOF|ppc64le\.sh|LD_LIBRARY_PATH|PYTHONPATH|(^|[^\\])\$\{?[A-Za-z_][A-Za-z0-9_]*\}?' \
runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu
echo '===== matching Konflux runtime profile construction, if any ====='
rg -n -C5 -- \
'PROFILE_EOF|ppc64le\.sh|LD_LIBRARY_PATH|PYTHONPATH' \
runtimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu || trueLength of output: 17048
The unquoted I’d treat the PyArrow cross-architecture equality assertion and the implicit Python 3.12 fallback as lower-priority policy decisions. The ✏️ Learnings added
|
Document intentional x86_64 marker lookups on ppc64le, quote pyarrow and pillow git refs, note the expanding ppc64le profile heredoc, tighten Dockerfile pylock lookup regexes, stop requiring identical pyarrow pins across arches, and fix CLI metavar display for --help. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the open review items in e9e299c:
Skipped CodeRabbit’s Allure metadata suggestion for these unit/script tests (Allure is used in container e2e tests). @coderabbitai recheck |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --format=fuller --stat --find-renames e9e299c87
git diff e9e299c87^ e9e299c87 -- \
jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh \
runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu \
runtimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu \
tests/test_native_build_versions.py \
scripts/pylock_version.py
echo '===== TrustyAI ppc64le native-build block ====='
nl -ba jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh | sed -n '125,180p'
echo '===== Runtime Datascience profile heredocs ====='
rg -n -C5 -- 'PROFILE_EOF|ppc64le\.sh|LD_LIBRARY_PATH|PYTHONPATH' \
runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu \
runtimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu
echo '===== Native version test ====='
nl -ba tests/test_native_build_versions.py | sed -n '1,100p'
echo '===== CLI argument declaration ====='
rg -n -C5 -- 'add_argument|metavar|nargs' scripts/pylock_version.pyLength of output: 8474
Skipping Allure metadata is reasonable for these unit/script tests when that convention applies only to container E2E coverage. The focused source-range command was unavailable because ✅ Action performedReview finished.
|
|
Addressed the open review items on #2519 in Fixed
Skipped (with reply)
Already resolved earlier
Replies posted on each thread + a summary comment asking |
|
Tip For best results, initiate chat on the files or code changes.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_native_build_versions.py (1)
32-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrevent newline-spanning resolver matches.
Removing
re.DOTALLis insufficient because\s+still matches newlines, sopylock_version.pyon one line andonnxorpyarrowlater can satisfy this assertion. Use[ \t]+(and a non-newline optional argument pattern), or explicitly model shell continuations.python - <<'PY' import re pattern = re.compile(r"\bpylock_version\.py\s+(?:\S+\s+)?onnx\b") assert pattern.search("pylock_version.py\nunrelated\nonnx") PY🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_native_build_versions.py` around lines 32 - 34, Update the resolver regex in the test assertion around lookup so whitespace separators cannot cross lines: replace \s+ with [ \t]+ and constrain the optional argument to non-newline characters, or explicitly support shell continuations if required. Preserve matching valid same-line resolver invocations for each package while preventing unrelated lines from satisfying the assertion.
🧹 Nitpick comments (1)
tests/test_native_build_versions.py (1)
31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
subtestsfor per-package assertions.This loop performs independent assertions for
onnxandpyarrow; wrap each iteration insubtests.test(...)so failures identify the package while allowing the remaining checks to run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_native_build_versions.py` around lines 31 - 34, Update the package loop in the test to wrap each independent assertion in subtests.test(...) using the current package as the identifier, so failures report whether onnx or pyarrow failed while allowing the other assertion to run.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/test_native_build_versions.py`:
- Around line 32-34: Update the resolver regex in the test assertion around
lookup so whitespace separators cannot cross lines: replace \s+ with [ \t]+ and
constrain the optional argument to non-newline characters, or explicitly support
shell continuations if required. Preserve matching valid same-line resolver
invocations for each package while preventing unrelated lines from satisfying
the assertion.
---
Nitpick comments:
In `@tests/test_native_build_versions.py`:
- Around line 31-34: Update the package loop in the test to wrap each
independent assertion in subtests.test(...) using the current package as the
identifier, so failures report whether onnx or pyarrow failed while allowing the
other assertion to run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 19df70e4-ac42-411b-aada-7bc1e4dd608d
📒 Files selected for processing (5)
jupyter/trustyai/ubi9-python-3.12/devel_env_setup.shruntimes/datascience/ubi9-python-3.12/Dockerfile.cpuruntimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpuscripts/pylock_version.pytests/test_native_build_versions.py
🚧 Files skipped from review as they are similar to previous changes (4)
- jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh
- runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu
- runtimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu
- scripts/pylock_version.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_native_build_versions.py`:
- Around line 42-44: Add descriptive failure messages to the three
locked_version assertions in the native build version tests, identifying both
the package and target architecture (ppc64le or s390x) for each assertion. Keep
the existing validation calls unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 696f7ebd-1dff-43a5-8847-00767126f8ad
📒 Files selected for processing (5)
jupyter/trustyai/ubi9-python-3.12/devel_env_setup.shruntimes/datascience/ubi9-python-3.12/Dockerfile.cpuruntimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpuscripts/pylock_version.pytests/test_native_build_versions.py
🚧 Files skipped from review as they are similar to previous changes (2)
- runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu
- runtimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu
|
/build-konflux |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@semgrep.yaml`:
- Around line 1871-1873: Update the pattern-regex in the dangerous-command rule
so variable matches cannot cross shell command separators, especially &&
and ||, while preserving quote-aware handling and existing word-boundary
behavior. Ensure the regex treats these separators as command boundaries rather
than part of the matched command, and add regression coverage for commands such
as rm "$FILE" && echo $OTHER and the equivalent || case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3e1624f1-97fc-4e39-b4b4-3b2b8ae20026
📒 Files selected for processing (3)
jupyter/trustyai/ubi9-python-3.12/devel_env_setup.shsemgrep.yamltests/test_native_build_versions.py
|
/build-konflux |
Konflux — #2519 @
|
| Outcome | Pipelines |
|---|---|
| Pass | runtime-minimal, runtime-pytorch-cuda, runtime-pytorch-llmcompressor, runtime-tensorflow-cuda, codeserver |
| Fail (this PR) | jupyter-datascience, runtime-datascience |
| Fail (unrelated) | jupyter-minimal / pytorch / tensorflow / trustyai (cpu+cuda+rocm) |
| Timeout 4h | runtime-pytorch-rocm, runtime-tensorflow-rocm |
Datascience (real, PR-owned): ppc64le onnx-builder → pip install -r requirements-min.txt builds numpy==1.26.0 from sdist → No BLAS library detected! / openblas found: NO. Same on jupyter + runtime. That is exactly what #2534 fixes.
Everything else: install_pdf_deps.sh can’t fetch TexLive from ftp.math.utah.edu — curl 28 (timeout) or 60 (SSL issuer). Not caused by the pylock/ONNX changes. ROCm runtimes hit the 4h PipelineRun limit.
Konflux — #2534 @ 94f1b11df
No PipelineRuns for the current tip (KubeArchive + live empty). Last /build-konflux was before the restack; tip never got a Konflux rebuild.
GHA subscription builds that did run fail the same TexLive curl path on amd64 (install_pdf_deps.sh exit 28); ppc64le/s390x jobs hit ~6h timeouts — not a signal on openblas yet.
Bottom line: The only stack-owned Konflux break on #2519 is datascience onnx/numpy needing OpenBLAS on ppc64le. Comment /build-konflux on #2534 (and optionally re-run datascience-only on #2519 after merge) to confirm the fix; ignore/retrigger the TexLive and ROCm timeout noise.
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
429c811 to
8834a43
Compare
|
New changes are detected. LGTM label has been removed. |
|
@coderabbitai review PR comments and whether they have been satisfactorily addressed |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="red-hat-data-services/notebooks"
pr="2519"
echo "===== PR `#2519` metadata ====="
gh pr view "$pr" --repo "$repo" --json number,state,isDraft,headRefName,headRefOid,baseRefName,mergeable,reviews,comments \
--jq '{number,state,isDraft,headRefName,headRefOid,baseRefName,mergeable,reviews:[.reviews[]|{author:.author.login,state,submittedAt,body}],comments:[.comments[]|{author:.author.login,createdAt,body}]}'
echo "===== PR `#2519` review comments ====="
gh api --paginate "repos/$repo/pulls/$pr/comments?per_page=100" \
--jq '.[] | {id, user:.user.login, path, line, original_line, created_at, body, in_reply_to_id}'
echo "===== PR `#2519` review threads and resolution state ====="
gh api graphql -F owner="red-hat-data-services" -F name="notebooks" -F number="$pr" -f query='
query($owner:String!, $name:String!, $number:Int!) {
repository(owner:$owner, name:$name) {
pullRequest(number:$number) {
reviewThreads(first:100) {
nodes {
isResolved
isOutdated
path
line
comments(first:20) {
nodes { author { login } body createdAt }
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | {isResolved,isOutdated,path,line,comments:[.comments.nodes[]|{author:.author.login,body,createdAt}]}'
echo "===== PR `#2534` status ====="
gh pr view 2534 --repo "$repo" --json number,state,headRefName,headRefOid,mergeable,statusCheckRollup,comments \
--jq '{number,state,headRefName,headRefOid,mergeable,statusCheckRollup:[.statusCheckRollup[]?|{name,conclusion,status}],comments:[.comments[]|{author:.author.login,createdAt,body}]}'
echo "===== relevant files at PR `#2519` head ====="
for file in \
jupyter/datascience/ubi9-python-3.12/Dockerfile.cpu \
jupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu \
runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu \
runtimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu \
jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh \
tests/test_native_build_versions.py \
scripts/pylock_version.py
do
echo "===== $file ====="
rg -n -C3 -- 'requirements-min|openblas|OPENBLAS|onnx-builder|ONNX_VERSION|pylock_version|PROFILE_EOF|platform x86_64|metavar|DOTALL' "$file" || true
done
echo "===== commits affecting ppc64le ONNX requirements in `#2519` checkout ====="
git log --oneline -G 'requirements-min\.txt|openblas|OPENBLAS' -- \
jupyter/datascience/ubi9-python-3.12/Dockerfile.cpu \
jupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu \
runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu \
runtimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpuLength of output: 196281
Source-review comments: satisfactorily addressedThe previously raised functional and maintainability items are present in the current source:
Thread statusFour user-created inline threads are still unresolved in GitHub’s UI, although their requested changes are in the source and the replies document them:
These can be manually resolved for review hygiene; I consider their content addressed. Konflux/OpenBLAS resultThe ppc64le ONNX failure is not fixed on
Conclusion: all review-comment feedback on |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
jupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu (1)
148-154: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrip local-version suffixes before constructing Git refs.
The resolver can return PEP 440 versions containing
+..., but these values are used directly inapache-arrow-*branches andv*tags. Strip the local suffix before forming both refs; current pins may be plain releases, but otherwise valid lock entries would fail checkout.Proposed fix
PYARROW_VERSION=$(python3 /tmp/pylock_version.py /tmp/pylock.toml pyarrow --platform "$TARGETARCH") +PYARROW_VERSION="${PYARROW_VERSION%%+*}" ARROW_BRANCH="apache-arrow-${PYARROW_VERSION}" ONNX_VERSION=$(python3 /tmp/pylock_version.py /tmp/pylock.toml onnx --platform ppc64le) +ONNX_VERSION="${ONNX_VERSION%%+*}" ONNX_TAG="v${ONNX_VERSION}"Also applies to: 226-231
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu` around lines 148 - 154, Update the version values used to construct the Apache Arrow branch and tag refs in the PyArrow and ONNX checkout flows, stripping any PEP 440 local-version suffix beginning with “+” first. Apply the same normalization in both the visible PyArrow flow and the corresponding ONNX flow so refs use only the base release version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jupyter/trustyai/ubi9-python-3.12/devel_env_setup.sh`:
- Line 43: Update the LD_LIBRARY_PATH export in devel_env_setup.sh to safely
handle an unset variable under set -u by using an unset-safe expansion, while
preserving the existing library directories and any preexisting value.
In `@tests/test_native_build_versions.py`:
- Around line 26-34: Update the test function in
tests/test_native_build_versions.py to use subtests for each Dockerfile,
forbidden-pattern assertion, pylock_version.py presence check, and
package-resolution check. Keep the existing validation logic and messages, but
scope each check with descriptive subtest contexts so failures in one Dockerfile
or package do not prevent the remaining checks from running.
In `@tests/unit/scripts/test_pylock_version.py`:
- Line 62: Update the plain assertions in
tests/unit/scripts/test_pylock_version.py at lines 62, 82, 87-96, and 127-128 to
include concise descriptive messages: identify the CLI invocation with
unexpected output, the expected os_name marker result, each marker expression
that evaluated unexpectedly, and the package/platform with its invalid resolved
version.
- Around line 16-23: Extend _DOCKERFILE_PIN_CASES with TrustyAI entries for
pillow on ppc64le and accelerate on x86_64, using the existing
jupyter/trustyai/ubi9-python-3.12/pylock.toml path so both devel_env_setup.sh
resolver lookups are covered.
---
Nitpick comments:
In `@jupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu`:
- Around line 148-154: Update the version values used to construct the Apache
Arrow branch and tag refs in the PyArrow and ONNX checkout flows, stripping any
PEP 440 local-version suffix beginning with “+” first. Apply the same
normalization in both the visible PyArrow flow and the corresponding ONNX flow
so refs use only the base release version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f7951670-b672-48de-b628-650031c94e16
📒 Files selected for processing (13)
jupyter/datascience/ubi9-python-3.12/Dockerfile.cpujupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpujupyter/trustyai/ubi9-python-3.12/Dockerfile.cpujupyter/trustyai/ubi9-python-3.12/Dockerfile.konflux.cpujupyter/trustyai/ubi9-python-3.12/devel_env_setup.shruntimes/datascience/ubi9-python-3.12/Dockerfile.cpuruntimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpuscripts/pylock_version.pysemgrep.yamltests/conftest.pytests/test_native_build_versions.pytests/unit/scripts/test_pylock_version.pytests/unit/test_semgrep_shell_unquoted.py
🚧 Files skipped from review as they are similar to previous changes (8)
- jupyter/trustyai/ubi9-python-3.12/Dockerfile.cpu
- jupyter/trustyai/ubi9-python-3.12/Dockerfile.konflux.cpu
- semgrep.yaml
- tests/conftest.py
- jupyter/datascience/ubi9-python-3.12/Dockerfile.cpu
- runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu
- scripts/pylock_version.py
- runtimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu
Read pinned package versions from pylock.toml with marker-aware platform filtering using only the Python stdlib (tomllib + ast). Co-authored-by: Cursor <cursoragent@cursor.com>
Wire datascience Dockerfiles to pylock_version.py, switch ONNX 1.22 ppc64le builds to requirements-min.txt, and cover Dockerfile pin lookups. Co-authored-by: Cursor <cursoragent@cursor.com>
Use pylock_version.py for torch/pyarrow/pillow/accelerate pins and quote shell expansions so Semgrep shell-unquoted-var checks stay clean. Co-authored-by: Cursor <cursoragent@cursor.com>
Require word boundaries for dangerous commands and stop argument spans at |, ||, ;, &, and && so matches stay on one logical command. Co-authored-by: Cursor <cursoragent@cursor.com>
8834a43 to
36c7a55
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/unit/scripts/test_pylock_version.py (1)
119-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun marker validation as subtests.
One invalid marker stops validation of every remaining marker. Use the existing
with_subtestshelper so failures identify each marker independently.Proposed change
+from typing import TYPE_CHECKING + import pytest +from tests.with_subtests import with_subtests + +if TYPE_CHECKING: + from pytest import Subtests + ... -def test_native_build_pylocks_have_parseable_markers(pylock_version, pylock: Path) -> None: +def test_native_build_pylocks_have_parseable_markers( + pylock_version, pylock: Path, subtests: Subtests +) -> None: ... - for marker in markers: + `@with_subtests`(subtests, markers, msg="marker") + def _(marker: str) -> None: pylock_version.evaluate_marker(marker, env)As per coding guidelines, “Use
subtestsfrom pytest-subtests plugin for iterating over multiple items with per-item assertions.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/scripts/test_pylock_version.py` around lines 119 - 120, Update the marker iteration around pylock_version.evaluate_marker to wrap each marker validation in the existing with_subtests helper, so every marker runs independently and failures identify the specific marker without stopping subsequent validation.Source: Coding guidelines
tests/unit/test_subtests.py (3)
1-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlace the unit test under the mirrored source path.
This test targets
tests/with_subtests.py, but is located attests/unit/test_subtests.py, which does not follow the required mirrored source layout. Move it to the repository’s corresponding mirrored path and retain the source-aligned test name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_subtests.py` around lines 1 - 7, Move the unit test for with_subtests from tests/unit/test_subtests.py to the repository’s mirrored path corresponding to tests/with_subtests.py, preserving the source-aligned test_subtests.py filename and its existing test contents.Source: Coding guidelines
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd descriptive assertion messages.
The single assertions omit messages required by the test guidelines. Include the mode and expected item list so failures identify whether callback execution or final collection failed.
Also applies to: 30-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_subtests.py` around lines 19 - 21, Update the assertions in the subtest checks, including the assertion near the final collection validation and the corresponding assertion around the additional referenced lines, to include descriptive failure messages containing the relevant mode and expected item list. Preserve the existing assertion conditions while making failures distinguish callback execution from final collection validation.Source: Coding guidelines
13-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover the failure path, not only successful items.
These tests would pass even if
quiet=Truefailed to create a subtest for an exception or stopped before later items. Add a regression test that fails one item and verifies subsequent items execute and the failure is attributed to that item.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_subtests.py` around lines 13 - 32, Add a regression test alongside test_with_subtests_quiet_runs_all_items that makes one input raise or fail, then verifies later inputs still execute and the resulting failure is attributed to the failing item’s subtest. Exercise the quiet=True path through with_subtests and preserve the existing successful-item coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/with_subtests.py`:
- Around line 41-46: Update the exception handling around f(item) in the subtest
execution flow to explicitly catch pytest.fail.Exception while continuing to let
KeyboardInterrupt and SystemExit propagate. Ensure failing items are re-raised
inside subtests so later items still execute, and add a regression test covering
one failing item followed by passing items.
---
Nitpick comments:
In `@tests/unit/scripts/test_pylock_version.py`:
- Around line 119-120: Update the marker iteration around
pylock_version.evaluate_marker to wrap each marker validation in the existing
with_subtests helper, so every marker runs independently and failures identify
the specific marker without stopping subsequent validation.
In `@tests/unit/test_subtests.py`:
- Around line 1-7: Move the unit test for with_subtests from
tests/unit/test_subtests.py to the repository’s mirrored path corresponding to
tests/with_subtests.py, preserving the source-aligned test_subtests.py filename
and its existing test contents.
- Around line 19-21: Update the assertions in the subtest checks, including the
assertion near the final collection validation and the corresponding assertion
around the additional referenced lines, to include descriptive failure messages
containing the relevant mode and expected item list. Preserve the existing
assertion conditions while making failures distinguish callback execution from
final collection validation.
- Around line 13-32: Add a regression test alongside
test_with_subtests_quiet_runs_all_items that makes one input raise or fail, then
verifies later inputs still execute and the resulting failure is attributed to
the failing item’s subtest. Exercise the quiet=True path through with_subtests
and preserve the existing successful-item coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c872fcab-d255-4ae7-b748-f6f37cb2cb07
📒 Files selected for processing (16)
jupyter/datascience/ubi9-python-3.12/Dockerfile.cpujupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpujupyter/trustyai/ubi9-python-3.12/Dockerfile.cpujupyter/trustyai/ubi9-python-3.12/Dockerfile.konflux.cpujupyter/trustyai/ubi9-python-3.12/devel_env_setup.shpytest.iniruntimes/datascience/ubi9-python-3.12/Dockerfile.cpuruntimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpuscripts/pylock_version.pysemgrep.yamltests/conftest.pytests/test_native_build_versions.pytests/unit/scripts/test_pylock_version.pytests/unit/test_semgrep_shell_unquoted.pytests/unit/test_subtests.pytests/with_subtests.py
🚧 Files skipped from review as they are similar to previous changes (9)
- jupyter/trustyai/ubi9-python-3.12/Dockerfile.cpu
- jupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu
- jupyter/trustyai/ubi9-python-3.12/Dockerfile.konflux.cpu
- tests/conftest.py
- semgrep.yaml
- jupyter/datascience/ubi9-python-3.12/Dockerfile.cpu
- runtimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu
- runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu
- scripts/pylock_version.py
Introduce a decorator that runs a multi-statement body as one subtest per item, with quiet mode that only opens a subtest context on failure. Use it for native Dockerfile/pylock pin coverage and pin subtests verbosity to 0 by default. Co-authored-by: Cursor <cursoragent@cursor.com>
36c7a55 to
cba986d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/test_subtests.py (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd descriptive messages to bare assertions.
The
tests/**/*.pyguidelines require descriptive messages for singleassertstatements. Add assertion messages throughout these tests so failures identify the failing item or captured output.Suggested change
- assert u < 3 + assert u < 3, f"unexpected item: {u}" ... - assert seen == [0, 1, 2] + assert seen == [0, 1, 2], f"items seen: {seen}" ... - assert seen == [0, 1, 2] + assert seen == [0, 1, 2], f"items seen: {seen}" ... - assert "Failed: intentional quiet-mode fail" in out + assert "Failed: intentional quiet-mode fail" in out, out - assert "SUBFAILED(item=0)" in out + assert "SUBFAILED(item=0)" in out, out - assert "assert seen ==" not in out + assert "assert seen ==" not in out, out - assert "assert [0] ==" not in out + assert "assert [0] ==" not in out, outAs per coding guidelines, single assertions in
tests/**/*.pymust include descriptive messages.Also applies to: 24-24, 33-33, 35-35, 55-55, 63-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_subtests.py` at line 22, Add descriptive failure messages to every bare assertion in tests/unit/test_subtests.py, including the assertions around u, captured output, and the items referenced at the specified locations. Ensure each message identifies the failing value or expected condition while preserving the existing assertion logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/scripts/test_pylock_version.py`:
- Around line 119-120: Update the marker iteration in the test to use the
pytest-subtests fixture, wrapping each pylock_version.evaluate_marker(marker,
env) call in subtests.test(...) with the current marker identified, so every
marker is validated independently and later failures remain visible.
In `@tests/unit/test_subtests.py`:
- Line 7: Update the exception handler in with_subtests to use tuple syntax when
catching Exception and pytest.fail.Exception, replacing the invalid
comma-separated form. Preserve the existing handling behavior so the helper
imports and runs under Python 3.14.
---
Nitpick comments:
In `@tests/unit/test_subtests.py`:
- Line 22: Add descriptive failure messages to every bare assertion in
tests/unit/test_subtests.py, including the assertions around u, captured output,
and the items referenced at the specified locations. Ensure each message
identifies the failing value or expected condition while preserving the existing
assertion logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 08759ca3-5b85-4e80-9af7-620076a0eff7
📒 Files selected for processing (16)
jupyter/datascience/ubi9-python-3.12/Dockerfile.cpujupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpujupyter/trustyai/ubi9-python-3.12/Dockerfile.cpujupyter/trustyai/ubi9-python-3.12/Dockerfile.konflux.cpujupyter/trustyai/ubi9-python-3.12/devel_env_setup.shpytest.iniruntimes/datascience/ubi9-python-3.12/Dockerfile.cpuruntimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpuscripts/pylock_version.pysemgrep.yamltests/conftest.pytests/test_native_build_versions.pytests/unit/scripts/test_pylock_version.pytests/unit/test_semgrep_shell_unquoted.pytests/unit/test_subtests.pytests/with_subtests.py
🚧 Files skipped from review as they are similar to previous changes (11)
- jupyter/trustyai/ubi9-python-3.12/Dockerfile.cpu
- pytest.ini
- jupyter/trustyai/ubi9-python-3.12/Dockerfile.konflux.cpu
- jupyter/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu
- runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu
- tests/with_subtests.py
- tests/conftest.py
- jupyter/datascience/ubi9-python-3.12/Dockerfile.cpu
- semgrep.yaml
- runtimes/datascience/ubi9-python-3.12/Dockerfile.konflux.cpu
- scripts/pylock_version.py
| for marker in markers: | ||
| pylock_version.evaluate_marker(marker, env) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Run each marker validation as a subtest.
One invalid marker aborts the loop and hides later failures. Wrap each evaluation in subtests.test(...).
Suggested fix
-def test_native_build_pylocks_have_parseable_markers(pylock_version, pylock: Path) -> None:
+def test_native_build_pylocks_have_parseable_markers(pylock_version, pylock: Path, subtests) -> None:
...
env = pylock_version.marker_env(python_minor="3.12", platform_machine="x86_64")
for marker in markers:
- pylock_version.evaluate_marker(marker, env)
+ with subtests.test(msg="marker", marker=marker):
+ pylock_version.evaluate_marker(marker, env)As per coding guidelines, “Use subtests from pytest-subtests plugin for iterating over multiple items with per-item assertions.”
📝 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.
| for marker in markers: | |
| pylock_version.evaluate_marker(marker, env) | |
| def test_native_build_pylocks_have_parseable_markers(pylock_version, pylock: Path, subtests) -> None: | |
| ... | |
| env = pylock_version.marker_env(python_minor="3.12", platform_machine="x86_64") | |
| for marker in markers: | |
| with subtests.test(msg="marker", marker=marker): | |
| pylock_version.evaluate_marker(marker, env) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/scripts/test_pylock_version.py` around lines 119 - 120, Update the
marker iteration in the test to use the pytest-subtests fixture, wrapping each
pylock_version.evaluate_marker(marker, env) call in subtests.test(...) with the
current marker identified, so every marker is validated independently and later
failures remain visible.
Source: Coding guidelines
2 participants
Description
Replace hardcoded
ARG ONNX_VERSION/ARG PYARROW_VERSIONwith a stdlib-onlyscripts/pylock_version.pyhelper that reads pinned versions from each workbenchpylock.tomlusing PEP 508-style platform markers.Scope (images touched):
jupyter/datascience,runtimes/datascience— ppc64le ONNX + pyarrow native builds; s390x pyarrowjupyter/trustyai—devel_env_setup.shppc64le wheel builds (torch/pyarrow/pillow/accelerate)scripts/pylock_version.py:tomllib+astmarker evaluator); runs in UBIpython-312beforeuvis installedv${ONNX_VERSION},apache-arrow-${PYARROW_VERSION},${TORCH_VERSION%%+*}for git tags)ast.parse— supports nestedand/or/not,intuples,.*wildcards; rejects unsupported operators loudlyAlso in this PR:
requirements.txt— ppc64leonnx-builderusesrequirements-min.txtdevel_env_setup.shshell quoting + unset-safeLD_LIBRARY_PATHunderset -ushell-unquoted-var-in-dangerous-cmdhardened (word boundaries; stop at|,||,;,&,&&)tests/with_subtests.pydecorator for iterable-driven subtests (quieter alternative to manualfor/with subtests.test) — tracks opendatahub-io/notebooks#4140Commits (5):
feat(scripts): add stdlib pylock_version helperfix(build): derive native ONNX/pyarrow versions from pylock.tomlfix(trustyai): resolve pins via pylock and quote shell varsfix(semgrep): harden shell-unquoted-var-in-dangerous-cmdtest: add with_subtests helper for iterable static checksHow Has This Been Tested?
uv run pytestontests/unit/scripts/test_pylock_version.py,tests/test_native_build_versions.py,tests/unit/test_subtests.py,tests/unit/test_semgrep_shell_unquoted.pypython3.12 -S scripts/pylock_version.pysmoke test on host and inregistry.access.redhat.com/ubi9/python-312:latestuv run prek --all-files(ruff green; pre-existing pyright noise on unrelatedruntime_test.py)Self checklist:
Dockerfile.konflux.*underrhds/notebooksas required for rhoai-2.25Merge criteria
Out of scope / follow-ups
openblas-develinonnx-builderfor ppc64le numpy sdist (stacked after this merges)with_subtestsacross the suite — ODH #4140uv pip install)Summary by CodeRabbit
pylock_versiontooling and use it during Docker builds and development setup (with a configurableDATASCIENCE_SOURCE_CODEbuild argument).