chore(rhoai-3.3): cherry-pick developer tooling QoL from main#2055
Open
chore(rhoai-3.3): cherry-pick developer tooling QoL from main#2055
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPins uv to 0.10.6 via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 7
🧹 Nitpick comments (2)
uv (1)
34-40: Add explicit error whenuvis not installed.Right now missing
uvfails implicitly in fast/slow path. A direct precheck gives a clearer actionable message.Proposed robustness patch
+# Prerequisite: this wrapper still needs a bootstrap `uv` command available. +if ! command -v uv >/dev/null 2>&1; then + echo "error: uv is not installed; install uv first, then rerun ./uv" >&2 + exit 127 +fi + # Fast path: use the system uv directly if it already matches the pinned version if current=$(uv --version 2>/dev/null) && [[ "$current" == "uv $version" || "$current" == "uv $version "* ]]; then exec uv "$@" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@uv` around lines 34 - 40, Add an explicit precheck that the `uv` binary exists and is executable before attempting the fast/slow path: test for `command -v uv >/dev/null 2>&1` (or `if ! command -v uv >/dev/null 2>&1; then ...`) and when missing call `printf`/`echo` to emit a clear error like "uv is not installed; please install uv or ensure it's on PATH" and exit with non-zero status; keep this check before the `current=$(uv --version ...)` line so the subsequent `current` assignment and the `exec uv "$@"` / `exec uv tool run "uv@${version}" "$@"` paths assume `uv` is available.scripts/pylocks_generator.py (1)
68-68: SynchronizeUV_MIN_VERSIONwith the actualuvrequirement.
UV_MIN_VERSIONis set to(0, 4, 0), butuv.tomlpins the version to0.10.6, and the lock-generation logic below (lines 250–253) depends on features introduced in0.10.5. UpdateUV_MIN_VERSIONto(0, 10, 5)to reflect the actual minimum version required by the script, or dynamically read it fromuv.tomlto prevent future drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pylocks_generator.py` at line 68, Update the UV_MIN_VERSION constant to match the actual minimum uv requirement used by the script: change UV_MIN_VERSION from (0, 4, 0) to (0, 10, 5) (or implement logic to parse and use the pinned version from uv.toml) so the lock-generation logic that relies on features introduced in 0.10.5 works correctly; specifically modify the UV_MIN_VERSION symbol in pylocks_generator.py or add code to read uv.toml and set UV_MIN_VERSION dynamically before the lock-generation checks that reference it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/piplock-renewal.yaml:
- Around line 60-65: The workflow currently checks out the user-selected branch
(ref: ${{ env.BRANCH }}) with token: ${{ secrets.GH_ACCESS_TOKEN }} and
persist-credentials: true, which exposes the write-scoped PAT to code executed
from that branch; change the checkout steps that use env.BRANCH to set
persist-credentials: false (and avoid supplying the PAT there) so branch code
runs without stored credentials, and then perform a separate authenticated
checkout or git push step only when you need to push/raise the PR using
GH_ACCESS_TOKEN (use a dedicated actions/checkout or git/auth step that supplies
the token at that final push point); apply this change to both the checkout
occurrence that references env.BRANCH and the secondary checkout block analogous
at lines 82-101.
In `@ci/generate_code.sh`:
- Around line 9-11: The generator commands are still run relative to the current
working directory; add an explicit directory change to the computed REPO_ROOT
before invoking the generators (e.g., insert cd "${REPO_ROOT}" || exit 1 at the
top of ci/generate_code.sh) so that commands like "${REPO_ROOT}/uv" run
scripts/dockerfile_fragments.py, "${REPO_ROOT}/uv" run
manifests/tools/generate_kustomization.py, and "${REPO_ROOT}/uv" run
scripts/pylocks_generator.py execute reliably regardless of where the script is
invoked.
In `@dependencies/cve-constraints.txt`:
- Line 14: The keras constraint line "keras>=3.13.1" is missing the required
CVE/issue reference comment; update that entry to follow the file's documented
format by appending a comment containing the CVE identifier(s) and a link or
issue reference (e.g., "CVE-YYYY-NNNN - <issue link/PR>") similar to how the
urllib3 entry is annotated so that the constraint documents the security reason
for the pin.
In `@docs/cves/python.md`:
- Around line 19-21: The fenced code blocks in docs/cves/python.md are missing
language tags (e.g., the block showing "dependencies/cve-constraints.txt" and
the other blocks at the ranges mentioned), which triggers markdownlint; update
each triple-backtick block by adding an appropriate language tag (for example
use "text" for plain filenames, "bash" for shell commands, or "toml" for config
snippets) so the examples at the noted locations (around lines 19-21, 28-32,
41-45, 116-119, 176-179) are annotated and lint-clean.
- Around line 34-35: Update all references to the removed Bash generator
`pylocks_generator.sh` in the document: replace invocation examples and
related-files mentions that call `pylocks_generator.sh` with the new command
`./uv run scripts/pylocks_generator.py ...` or document the alternative `make
refresh-lock-files` workflow; ensure examples, explanatory text, and any command
blocks that instruct users to run `pylocks_generator.sh` instead show the new
Python invocation (or the Make target) and adjust any surrounding wording so the
reader knows they can use either `./uv run scripts/pylocks_generator.py` or
`make refresh-lock-files`.
In `@scripts/pylocks_generator.py`:
- Around line 241-246: The current logic assigns output paths (output =
"pylock.toml" or output = f"uv.lock.d/pylock.{flavor}.toml") and later writes
directly to that destination, which unlinks the existing lockfile on failure;
change the write flow to write to a temporary file (e.g. same directory with a
unique suffix) and only atomically replace/move the temp file to the final
destination after the compile/write succeeds; update both the blocks that set
output (the branch using project_dir / "uv.lock.d".mkdir(...) and the earlier
public-index branch) and the later write/cleanup code (the code around lines
290-296) so the temp-to-final rename is used and the original file is preserved
on errors.
---
Nitpick comments:
In `@scripts/pylocks_generator.py`:
- Line 68: Update the UV_MIN_VERSION constant to match the actual minimum uv
requirement used by the script: change UV_MIN_VERSION from (0, 4, 0) to (0, 10,
5) (or implement logic to parse and use the pinned version from uv.toml) so the
lock-generation logic that relies on features introduced in 0.10.5 works
correctly; specifically modify the UV_MIN_VERSION symbol in pylocks_generator.py
or add code to read uv.toml and set UV_MIN_VERSION dynamically before the
lock-generation checks that reference it.
In `@uv`:
- Around line 34-40: Add an explicit precheck that the `uv` binary exists and is
executable before attempting the fast/slow path: test for `command -v uv
>/dev/null 2>&1` (or `if ! command -v uv >/dev/null 2>&1; then ...`) and when
missing call `printf`/`echo` to emit a clear error like "uv is not installed;
please install uv or ensure it's on PATH" and exit with non-zero status; keep
this check before the `current=$(uv --version ...)` line so the subsequent
`current` assignment and the `exec uv "$@"` / `exec uv tool run "uv@${version}"
"$@"` paths assume `uv` is available.
🪄 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: Pro
Run ID: bebb15d5-14cd-473a-8ba2-eaae2b99e36b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
.github/workflows/build-notebooks-TEMPLATE.yaml.github/workflows/code-quality.yaml.github/workflows/docs.yaml.github/workflows/piplock-renewal.yaml.github/workflows/security.yaml.pre-commit-config.yamlMakefileREADME.mdci/generate_code.shdependencies/cve-constraints.txtdocs/cves/python.mdjupyter/datascience/ubi9-python-3.12/pyproject.tomljupyter/pytorch+llmcompressor/ubi9-python-3.12/pyproject.tomljupyter/pytorch/ubi9-python-3.12/pyproject.tomljupyter/rocm/pytorch/ubi9-python-3.12/pyproject.tomljupyter/rocm/tensorflow/ubi9-python-3.12/pyproject.tomljupyter/tensorflow/ubi9-python-3.12/pyproject.tomljupyter/trustyai/ubi9-python-3.12/pyproject.tomlpyproject.tomlruntimes/rocm-tensorflow/ubi9-python-3.12/pyproject.tomlruntimes/tensorflow/ubi9-python-3.12/pyproject.tomlscripts/pylocks_generator.pyscripts/pylocks_generator.shuvuv.toml
💤 Files with no reviewable changes (1)
- scripts/pylocks_generator.sh
Comment on lines
60
to
+65
| - name: Checkout code | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| ref: ${{ env.BRANCH }} | ||
| token: ${{ secrets.GH_ACCESS_TOKEN }} | ||
| persist-credentials: true |
There was a problem hiding this comment.
Don't persist GH_ACCESS_TOKEN before executing branch code.
This checks out a user-selected branch with a write-scoped PAT and persist-credentials: true, then executes make from that branch. A malicious branch can read the stored remote credentials and exfiltrate the PAT. Checkout without persisted credentials and use the PAT only for the final push/PR call.
Suggested fix
- name: Checkout code
uses: actions/checkout@v6
with:
ref: ${{ env.BRANCH }}
- token: ${{ secrets.GH_ACCESS_TOKEN }}
- persist-credentials: true
+ persist-credentials: false
@@
BRANCH_NAME="lockfile-update-$(date +%Y%m%d-%H%M)"
git checkout -b "$BRANCH_NAME"
git commit -m "Update lock files"
- git push -u origin "$BRANCH_NAME"
+ git push -u "https://x-access-token:${GH_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" "$BRANCH_NAME"Also applies to: 82-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/piplock-renewal.yaml around lines 60 - 65, The workflow
currently checks out the user-selected branch (ref: ${{ env.BRANCH }}) with
token: ${{ secrets.GH_ACCESS_TOKEN }} and persist-credentials: true, which
exposes the write-scoped PAT to code executed from that branch; change the
checkout steps that use env.BRANCH to set persist-credentials: false (and avoid
supplying the PAT there) so branch code runs without stored credentials, and
then perform a separate authenticated checkout or git push step only when you
need to push/raise the PR using GH_ACCESS_TOKEN (use a dedicated
actions/checkout or git/auth step that supplies the token at that final push
point); apply this change to both the checkout occurrence that references
env.BRANCH and the secondary checkout block analogous at lines 82-101.
Member
Author
There was a problem hiding this comment.
There was a problem hiding this comment.
@jiridanek, thanks for pointing to the upstream PR opendatahub-io/notebooks#3202. Since this is a cherry-pick branch targeting rhoai-3.3, and the security fix is being handled upstream, the concern remains open in this PR until either the fix is cherry-picked here as well, or the team decides to accept the current behavior for this branch.
The risk of persist-credentials: true with GH_ACCESS_TOKEN on a user-controlled branch checkout is still present in the current state of this file — worth keeping in mind if rhoai-3.3 also runs this workflow against user-submitted branches.
🧠 Learnings used
Learnt from: CR
Repo: red-hat-data-services/notebooks PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T19:41:58.522Z
Learning: Applies to **/{pyproject,pylock}.toml : Use `pyproject.toml` and `pylock.toml` for Python dependencies; regenerate lock files after dependency changes by running `make refresh-pipfilelock-files`
Learnt from: CR
Repo: red-hat-data-services/notebooks PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-23T11:14:48.339Z
Learning: Branch prefix policy: Exempt PR titles starting with "Revert ", "Update lock files", "Update BuildConfigs", "Sync PipelineRuns", "Merge pull request", or authored by bot accounts from branch prefix validation.
| # RHAIENG-2458: CVE-2025-66418 urllib3 decompression vulnerability | ||
| # Upstream: https://github.com/elyra-ai/elyra/issues/3325 | ||
| urllib3>=2.6.0 | ||
| keras>=3.13.1 |
There was a problem hiding this comment.
Missing CVE/issue reference for keras constraint.
Per the file's own documented format (lines 6-7), each constraint should include "CVE ID and issue reference in a comment." The urllib3 entry follows this pattern, but keras>=3.13.1 lacks any CVE or issue documentation.
📝 Suggested fix
# RHAIENG-2458: CVE-2025-66418 urllib3 decompression vulnerability
# Upstream: https://github.com/elyra-ai/elyra/issues/3325
urllib3>=2.6.0
+
+# <ISSUE-ID>: <CVE-ID> keras <brief description>
+# Upstream: <link>
keras>=3.13.1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dependencies/cve-constraints.txt` at line 14, The keras constraint line
"keras>=3.13.1" is missing the required CVE/issue reference comment; update that
entry to follow the file's documented format by appending a comment containing
the CVE identifier(s) and a link or issue reference (e.g., "CVE-YYYY-NNNN -
<issue link/PR>") similar to how the urllib3 entry is annotated so that the
constraint documents the security reason for the pin.
Comment on lines
+241
to
+246
| output = "pylock.toml" | ||
| desc = "pylock.toml (public index)" | ||
| print("➡️ Generating pylock.toml from public PyPI index...") | ||
| else: | ||
| (project_dir / "uv.lock.d").mkdir(exist_ok=True) | ||
| output = f"uv.lock.d/pylock.{flavor}.toml" |
There was a problem hiding this comment.
Preserve the existing lockfile when compilation fails.
On failure this unlinks the real destination file, so a transient resolver error deletes the last known-good pylock.toml/uv.lock.d/... instead of just cleaning up partial output. Write to a temporary file and replace the destination only after a successful compile.
Suggested fix
if mode == IndexMode.public_index:
output = "pylock.toml"
desc = "pylock.toml (public index)"
print("➡️ Generating pylock.toml from public PyPI index...")
@@
output = f"uv.lock.d/pylock.{flavor}.toml"
desc = f"{flavor.upper()} lock file"
print(f"➡️ Generating {flavor.upper()} lock file...")
+ output_path = project_dir / output
+ temp_output = output_path.with_name(f"{output_path.name}.tmp")
+
# Tag filtering was added in uv 0.9.16 (https://github.com/astral-sh/uv/pull/16956)
@@
str(UV),
"pip",
"compile",
"pyproject.toml",
"--output-file",
- output,
+ os.path.relpath(temp_output, project_dir),
"--format",
"pylock.toml",
@@
result = subprocess.run(cmd, cwd=project_dir, check=False)
if result.returncode != 0:
warn(f"Failed to generate {desc} in {project_dir}")
- output_path = project_dir / output
- output_path.unlink(missing_ok=True)
+ temp_output.unlink(missing_ok=True)
return False
+ temp_output.replace(output_path)
ok(f"{desc} generated successfully.")
return TrueAlso applies to: 290-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/pylocks_generator.py` around lines 241 - 246, The current logic
assigns output paths (output = "pylock.toml" or output =
f"uv.lock.d/pylock.{flavor}.toml") and later writes directly to that
destination, which unlinks the existing lockfile on failure; change the write
flow to write to a temporary file (e.g. same directory with a unique suffix) and
only atomically replace/move the temp file to the final destination after the
compile/write succeeds; update both the blocks that set output (the branch using
project_dir / "uv.lock.d".mkdir(...) and the earlier public-index branch) and
the later write/cleanup code (the code around lines 290-296) so the
temp-to-final rename is used and the original file is preserved on errors.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/cves/python.md (1)
19-21:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks.
These blocks are still missing fence languages and trigger MD040 (
markdownlint).Suggested doc fix
-``` +```text dependencies/cve-constraints.txt
# CVE-ID: Description # Reference: https://... package>=fixed_version
# RHAIENG-XXXX: CVE-YYYY-ZZZZZ package_name vulnerability description # Upstream: https://github.com/... package_name>=fixed_version
# RHAIENG-2448: CVE-XXXX-YYYY tornado quadratic DoS tornado>=6.5.3
# RHAIENG-2458: CVE-2025-66418 urllib3 decompression vulnerability urllib3>=2.6.0</details> Also applies to: 28-32, 41-45, 116-119, 176-179 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/cves/python.mdaround lines 19 - 21, Several fenced code blocks (the
one containing "dependencies/cve-constraints.txt" and the subsequent blocks
showing CVE entries like "# CVE-ID: Description", "# RHAIENG-XXXX:
CVE-YYYY-ZZZZZ ..." and the tornado/urllib3 examples) are missing a language
identifier and trigger MD040; add a language tag (e.g.,text) to each opening fence for those blocks (and the other occurrences noted) so they read liketext followed by the existing content and closing ``` to satisfy the linter.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@docs/cves/python.md:
- Around line 90-96: Replace direct invocations of the uv CLI (e.g., the strings
"uv tree" and "uv tree --invert tornado") with the pinned wrapper invocation
"./uv tree" throughout the document so examples always use the pinned ./uv
binary; update every occurrence (including the other instances mentioned) to use
"./uv" before subcommands to ensure consistent, pinned execution.
Duplicate comments:
In@docs/cves/python.md:
- Around line 19-21: Several fenced code blocks (the one containing
"dependencies/cve-constraints.txt" and the subsequent blocks showing CVE entries
like "# CVE-ID: Description", "# RHAIENG-XXXX: CVE-YYYY-ZZZZZ ..." and the
tornado/urllib3 examples) are missing a language identifier and trigger MD040;
add a language tag (e.g.,text) to each opening fence for those blocks (and the other occurrences noted) so they read liketext followed by the existing
content and closing ``` to satisfy the linter.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `a622424f-5a41-4386-8e87-23bcd5f54db0` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 78e7f067ebfdcb760acb421e11b6cda3c917102e and 7b77e4ab215acb6ee6d822cc7739088820d666d8. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `docs/cves/python.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+90
to
+96
| ```bash | ||
| # Using uv (preferred) | ||
| uv tree | grep -A5 -B5 tornado | ||
|
|
||
| # Or check the package's dependents | ||
| uv tree --invert tornado | ||
| ``` |
There was a problem hiding this comment.
Use the pinned ./uv wrapper in all command examples.
The guide uses uv tree directly in some places; that bypasses the pinned version workflow documented in this PR and can produce inconsistent results across environments.
Suggested doc fix
-uv tree | grep -A5 -B5 tornado
+./uv tree | grep -A5 -B5 tornado
-uv tree --invert tornado
+./uv tree --invert tornado
...
-uv tree
+./uv tree
-uv tree --invert package-name
+./uv tree --invert package-nameAlso applies to: 219-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/cves/python.md` around lines 90 - 96, Replace direct invocations of the
uv CLI (e.g., the strings "uv tree" and "uv tree --invert tornado") with the
pinned wrapper invocation "./uv tree" throughout the document so examples always
use the pinned ./uv binary; update every occurrence (including the other
instances mentioned) to use "./uv" before subcommands to ensure consistent,
pinned execution.
…txt (opendatahub-io#2886) RHAIENG-2458: CVE-2025-66418 urllib3 decompression vulnerability - Create dependencies/cve-constraints.txt as single source of truth for CVE-induced minimum version constraints - Update pylocks_generator.sh to use --constraints flag with CVE file - Update comments on urllib3 overrides in jupyter images to explain that override is needed because odh-elyra's appengine-python-standard has an obnoxious urllib3<2 constraint - Add docs/cves/python.md documenting the CVE resolution workflow This approach: - Centralizes CVE fixes in one file - Applies constraints at resolution time via uv pip compile --constraints - Uses override-dependencies only where needed (odh-elyra conflict) - Prevents CVEs from returning through transitive dependencies Thanks to Adriana Theodorakopoulou for contributing the CVE resolution workflow documentation. (cherry picked from commit 2e8d387)
(cherry picked from commit 012d396)
…generated-code failure (opendatahub-io#3034) The check-generated-code CI job is failing on main and all PRs because astral-sh/setup-uv@v7 installs version: "latest" (currently uv 0.10.6), which produces different pylock.toml output than the committed files. The root cause is astral-sh/uv#18081 (released in uv 0.10.5, 2026-02-23) which added wheel filtering to pylock.toml even in --universal mode. See opendatahub-io#3032. Fix: pin uv version in uv.toml using required-version, remove version: "latest" from all workflows so setup-uv auto-detects the pin, and regenerate pylocks. 1. Create uv.toml at repo root required-version = "==0.10.6" setup-uv@v7 auto-detects this and installs the pinned version. Locally, uv errors if the running version doesn't match. 2. Update .github/workflows/code-quality.yaml Two setup-uv blocks (lines 17-24 and 46-53): - Remove version: "latest" line - Rename step from "Install the latest version of uv" to "Install uv" 3. Update .github/workflows/docs.yaml One setup-uv block (lines 21-28): - Remove version: "latest" line - Rename step 4. Update .github/workflows/security.yaml One setup-uv block (lines 19-26): - Remove version: "latest" line - Rename step 5. Update .github/workflows/build-notebooks-TEMPLATE.yaml One setup-uv block (lines 304-311): - Remove version: "latest" line - Rename step 6. Update ci/generate_code.sh Line 4 has a fallback pip install "uv==0.9.6" — update to "uv==0.10.6" to match the pinned version. 7. Regenerate pylocks bash ci/generate_code.sh This regenerates the 6 affected pylock.toml files with uv 0.10.6 filtering. - uv.toml (new) - .github/workflows/code-quality.yaml - .github/workflows/docs.yaml - .github/workflows/security.yaml - .github/workflows/build-notebooks-TEMPLATE.yaml - ci/generate_code.sh - 6 pylock.*.toml files (regenerated) # Check that uv reads the required-version and doesn't error uv version # Regenerate and verify no diff bash ci/generate_code.sh git diff --stat # should show no changes after regeneration * ISSUE opendatahub-io#3032: chore(uv): create a ./uv wrapper to run the correct version of uv 1. Pre-flight check (lines 93-100): Added explicit validation that $UV wrapper exists and is executable before the existing command -v uv check. This prevents a misleading version error if the wrapper is missing. 2. Constraints flag (lines 291-316): Changed constraints_flag from a plain string to a bash array (local -a constraints_flag=()), and expanded it as "${constraints_flag[@]}" in the pip compile invocation. This avoids word-splitting on paths containing spaces. (cherry picked from commit 137aa9b)
…generator.sh` to Python (opendatahub-io#3057) **Files created:** - `scripts/pylocks_generator.py` -- Python rewrite of the bash script using typer for CLI **Files modified:** - `pyproject.toml` -- added `typer` to dev dependencies - `uv.lock` -- updated by `uv lock` to include typer as explicit dep - `Makefile` -- `refresh-lock-files` target now calls `python3 scripts/pylocks_generator.py` - `ci/generate_code.sh` -- uses `"${REPO_ROOT}/uv" run scripts/pylocks_generator.py` (consistent with adjacent lines) - `scripts/lockfile-generators/create-requirements-lockfile.sh` -- all references updated from `.sh` to `.py` **Files deleted:** - `scripts/pylocks_generator.sh` -- the original bash script **Verification results:** - `--help` shows the correct typer CLI with `INDEX_MODE` and `TARGET_DIR` arguments - Running against `jupyter/minimal/ubi9-python-3.12` produced byte-identical lock files to the bash script (zero diff after fixing arg formatting) - Correctly detects Python version, flavors, effective mode, and generates all three flavor locks (CPU, CUDA, ROCM) Minimal, focused PR: rewrite the script, update call sites, delete old script. No tests, no ruff include changes (follow-up). - **1 deleted file** (`pylocks_generator.sh`, 376 lines) - **1 added file** (`pylocks_generator.py`, ~200-250 lines), structured to mirror the bash script's section layout so reviewers can compare side by side - **4 small edits** (pyproject.toml, Makefile, ci/generate_code.sh, create-requirements-lockfile.sh) Add `typer` to `[dependency-groups] dev` in [pyproject.toml](pyproject.toml) (it's already in `uv.lock` as a transitive dep, but needs to be explicit). Run `uv lock` to sync. Use typer for CLI with the same interface as the bash script: ```python """Generate Python dependency lock files (pylock.toml) using uv pip compile.""" from __future__ import annotations import os import re import subprocess import sys from pathlib import Path import typer ROOT_DIR = Path(__file__).resolve().parent.parent UV = ROOT_DIR / "uv" CVE_CONSTRAINTS_FILE = ROOT_DIR / "dependencies" / "cve-constraints.txt" PUBLIC_INDEX = "--default-index=https://pypi.org/simple" MAIN_DIRS = ["jupyter", "runtimes", "rstudio", "codeserver"] UV_MIN_VERSION = (0, 4, 0) app = typer.Typer() @app.command() def main( index_mode: str = typer.Argument("auto", help="..."), target_dir: Path | None = typer.Argument(None, help="..."), ) -> None: ... ``` Key functions (same logical sections as the bash script): - `read_conf_value(conf_file: Path, key: str) -> str | None` -- simple line parser replacing awk - `check_uv()` -- verify `./uv` exists, parse version, compare as `tuple(int, ...)` >= `(0, 4, 0)` - `find_target_dirs(target_dir: Path | None) -> list[Path]` -- `pathlib.glob("**/pyproject.toml")` - `detect_flavors(project_dir: Path) -> set[str]` -- check `Dockerfile.{cpu,cuda,rocm}` existence - `get_index_flags(project_dir: Path, flavor: str) -> list[str] | None` -- read `build-args/<flavor>.conf` - `run_lock(project_dir, flavor, index_flags, mode, python_version, upgrade) -> bool` -- `subprocess.run(cwd=project_dir)` with list args Implementation notes: - Build the `uv pip compile` command as a list, conditionally appending `--upgrade` - Use `cwd=target_dir` in `subprocess.run()` and `os.path.relpath()` for CVE constraints path - `FORCE_LOCKFILES_UPGRADE` env var read via `os.environ.get()` - [Makefile](Makefile) line 443: `bash scripts/pylocks_generator.sh $(INDEX_MODE) $(DIR)` -> `python3 scripts/pylocks_generator.py $(INDEX_MODE) $(DIR)` - [ci/generate_code.sh](ci/generate_code.sh) line 11: `bash scripts/pylocks_generator.sh` -> `"${REPO_ROOT}/uv" run scripts/pylocks_generator.py` (matches the two lines above it) - [scripts/lockfile-generators/create-requirements-lockfile.sh](scripts/lockfile-generators/create-requirements-lockfile.sh) -- all references: - Line 31: `PYLOCKS_GENERATOR="scripts/pylocks_generator.sh"` -> `.py` - Line 43: help text reference - Line 57: help text reference - Lines 73-74: guard check and error message - Line 115: `bash "$PYLOCKS_GENERATOR"` -> `python3 "$PYLOCKS_GENERATOR"` Clean deletion. It remains in git history. Run `python3 scripts/pylocks_generator.py --help` and a targeted invocation like `python3 scripts/pylocks_generator.py auto jupyter/minimal/ubi9-python-3.12`. * Here's what I addressed from the CodeRabbit review: **Fixed (3 items):** - **Makefile**: Changed `python3` to `./uv run` so typer is available via the dev environment - **`mode: str` -> `mode: IndexMode`**: `run_lock` now takes the enum directly, and the caller passes `IndexMode.rh_index` / `IndexMode.public_index` instead of raw strings - **`find_target_dirs` sort/dedup**: Uses a `set` for dedup and returns `sorted(dirs)` for stable CI output **Rejected (4 items):** - **`@app.callback()`**: Tested it -- produces a misleading `COMMAND [ARGS]...` in help. `@app.command()` works correctly with enum arguments; `auto` is parsed as the enum value, not a subcommand name - **`read_conf_value` hardening (export/quotes)**: The actual `.conf` files don't use these patterns, and the original bash awk parser didn't handle them either. Unnecessary scope creep - **Skipped dirs tracking**: The bash script didn't track these either. Behavior parity - **`source "$CONF_FILE"` in create-requirements-lockfile.sh**: Pre-existing, not introduced by this PR (cherry picked from commit 8da26d3)
…pt (opendatahub-io#3077) (cherry picked from commit 8239f40)
The shell script was replaced by the Python rewrite in this PR. Update docs/cves/python.md to reference pylocks_generator.py and the ./uv wrapper. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jiridanek
force-pushed
the
cherry-pick-qol-rhoai-3.3
branch
from
7b77e4a to
7124110
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
docs/cves/python.md (1)
218-223:⚠️ Potential issue | 🟡 MinorUse
./uvwrapper in "Useful Commands" section.These examples should use
./uv treeand./uv tree --invertfor consistency with the pinned version strategy.Suggested fix
# Check dependency tree -uv tree +./uv tree # Find what depends on a package -uv tree --invert package-name +./uv tree --invert package-name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cves/python.md` around lines 218 - 223, Update the "Useful Commands" examples to use the local wrapper by replacing the bare commands 'uv tree' and 'uv tree --invert package-name' with './uv tree' and './uv tree --invert package-name' so examples consistently use the pinned ./uv wrapper; locate the two occurrences of 'uv tree' and 'uv tree --invert' and change them accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cves/python.md`:
- Around line 19-21: The fenced code blocks in the markdown examples (e.g., the
block showing "dependencies/cve-constraints.txt" and the constraints format
example starting with "# CVE-ID: Description") are missing language tags; update
those triple-backtick fences to include appropriate language identifiers—use
```text for file paths and plain text snippets and ```toml for any TOML
configuration examples—so change the opening fences accordingly and apply the
same fix to the other similar blocks referenced in the file (the blocks around
the constraints examples and the other three occurrences).
In `@jupyter/rocm/tensorflow/ubi9-python-3.12/pyproject.toml`:
- Around line 18-19: The pyproject.toml references the package string
"odh-notebooks-meta-workbench-datascience-deps" via a local path that doesn't
exist; either create the missing consolidated package under
dependencies/odh-notebooks-meta-workbench-datascience-deps/ with a valid
pyproject.toml (matching the package name) or update the dependency entry in
pyproject.toml to the correct existing package/path or published package name so
dependency resolution succeeds when building.
In `@scripts/pylocks_generator.py`:
- Around line 143-147: The code currently builds version_tuple from version_str
using tuple(int(x) for x in version_str.split(".")) which will raise ValueError
for non-numeric parts like "0.10.6-dev"; update the logic around
version_str/version_tuple to sanitize or parse each segment before int
conversion (e.g., split on hyphen or use a regex to extract leading digits per
segment) and catch ValueError so non-numeric suffixes fall back to their numeric
prefix or to 0; ensure the change is applied where version_str is set and where
version_tuple is computed so functions/variables version_str and version_tuple
handle inputs like "0.10.6-dev" without raising.
---
Duplicate comments:
In `@docs/cves/python.md`:
- Around line 218-223: Update the "Useful Commands" examples to use the local
wrapper by replacing the bare commands 'uv tree' and 'uv tree --invert
package-name' with './uv tree' and './uv tree --invert package-name' so examples
consistently use the pinned ./uv wrapper; locate the two occurrences of 'uv
tree' and 'uv tree --invert' and change them accordingly.
🪄 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: Pro
Run ID: a954378e-442f-456f-94f3-73cd041b14b5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
.github/workflows/build-notebooks-TEMPLATE.yaml.github/workflows/code-quality.yaml.github/workflows/docs.yaml.github/workflows/piplock-renewal.yaml.github/workflows/security.yaml.pre-commit-config.yamlMakefileREADME.mdci/generate_code.shdependencies/cve-constraints.txtdocs/cves/python.mdjupyter/datascience/ubi9-python-3.12/pyproject.tomljupyter/pytorch+llmcompressor/ubi9-python-3.12/pyproject.tomljupyter/pytorch/ubi9-python-3.12/pyproject.tomljupyter/rocm/pytorch/ubi9-python-3.12/pyproject.tomljupyter/rocm/tensorflow/ubi9-python-3.12/pyproject.tomljupyter/tensorflow/ubi9-python-3.12/pyproject.tomljupyter/trustyai/ubi9-python-3.12/pyproject.tomlpyproject.tomlruntimes/rocm-tensorflow/ubi9-python-3.12/pyproject.tomlruntimes/tensorflow/ubi9-python-3.12/pyproject.tomlscripts/pylocks_generator.pyscripts/pylocks_generator.shuvuv.toml
💤 Files with no reviewable changes (1)
- scripts/pylocks_generator.sh
✅ Files skipped from review due to trivial changes (9)
- pyproject.toml
- uv.toml
- jupyter/rocm/pytorch/ubi9-python-3.12/pyproject.toml
- dependencies/cve-constraints.txt
- jupyter/trustyai/ubi9-python-3.12/pyproject.toml
- jupyter/datascience/ubi9-python-3.12/pyproject.toml
- .github/workflows/docs.yaml
- jupyter/pytorch/ubi9-python-3.12/pyproject.toml
- jupyter/pytorch+llmcompressor/ubi9-python-3.12/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/build-notebooks-TEMPLATE.yaml
- .github/workflows/security.yaml
- .github/workflows/code-quality.yaml
- uv
- ci/generate_code.sh
- runtimes/rocm-tensorflow/ubi9-python-3.12/pyproject.toml
- jupyter/tensorflow/ubi9-python-3.12/pyproject.toml
- .github/workflows/piplock-renewal.yaml
Comment on lines
+19
to
+21
| ``` | ||
| dependencies/cve-constraints.txt | ||
| ``` |
There was a problem hiding this comment.
Add language tags to fenced code blocks.
These code blocks are missing language identifiers, triggering markdownlint warnings. Use text for file paths, toml for configuration snippets.
Suggested fixes
-```
-dependencies/cve-constraints.txt
-```
+```text
+dependencies/cve-constraints.txt
+```For the constraints file format examples, use:
-```
+```text
# CVE-ID: DescriptionAlso applies to: 28-32, 41-45, 116-119, 176-179
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/cves/python.md` around lines 19 - 21, The fenced code blocks in the
markdown examples (e.g., the block showing "dependencies/cve-constraints.txt"
and the constraints format example starting with "# CVE-ID: Description") are
missing language tags; update those triple-backtick fences to include
appropriate language identifiers—use ```text for file paths and plain text
snippets and ```toml for any TOML configuration examples—so change the opening
fences accordingly and apply the same fix to the other similar blocks referenced
in the file (the blocks around the constraints examples and the other three
occurrences).
Comment on lines
+18
to
+19
| "skl2onnx~=1.19.1", | ||
| "odh-notebooks-meta-workbench-datascience-deps", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the meta-dependency package exists and has proper structure
if [ -d "dependencies/odh-notebooks-meta-workbench-datascience-deps" ]; then
echo "✅ Directory exists"
if [ -f "dependencies/odh-notebooks-meta-workbench-datascience-deps/pyproject.toml" ]; then
echo "✅ pyproject.toml exists"
cat "dependencies/odh-notebooks-meta-workbench-datascience-deps/pyproject.toml"
else
echo "❌ pyproject.toml missing"
fi
else
echo "❌ Meta-dependency directory not found"
echo "Available directories in dependencies/:"
ls -la dependencies/ 2>/dev/null || echo "dependencies/ directory not found"
fiRepository: red-hat-data-services/notebooks
Length of output: 506
🏁 Script executed:
cat jupyter/rocm/tensorflow/ubi9-python-3.12/pyproject.tomlRepository: red-hat-data-services/notebooks
Length of output: 2449
🏁 Script executed:
# Check if the existing meta-packages have pyproject.toml files
ls -la dependencies/odh-notebooks-meta-llmcompressor-deps/
ls -la dependencies/odh-notebooks-meta-runtime-elyra-deps/Repository: red-hat-data-services/notebooks
Length of output: 421
🏁 Script executed:
# Search for references to the missing package in the codebase
rg "odh-notebooks-meta-workbench-datascience-deps" --type tomlRepository: red-hat-data-services/notebooks
Length of output: 655
🏁 Script executed:
# Check if the package might be in a different location or with different naming
fd "workbench-datascience" .Repository: red-hat-data-services/notebooks
Length of output: 57
🏁 Script executed:
# Check git status to see if this file/directory is staged or being created
git status dependencies/Repository: red-hat-data-services/notebooks
Length of output: 140
🏁 Script executed:
# Look for any recent commits or file creations related to this package
git log --oneline --all -- "*workbench-datascience*" 2>/dev/null | head -20Repository: red-hat-data-services/notebooks
Length of output: 671
Create the missing odh-notebooks-meta-workbench-datascience-deps package or correct the reference.
The pyproject.toml at lines 18-19 and 48-49 references odh-notebooks-meta-workbench-datascience-deps via a local path that does not exist in the repository. Dependency resolution will fail at build time. Verify that the intended consolidated datascience dependencies package exists at dependencies/odh-notebooks-meta-workbench-datascience-deps/ with a valid pyproject.toml, or update the reference if the path is incorrect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jupyter/rocm/tensorflow/ubi9-python-3.12/pyproject.toml` around lines 18 -
19, The pyproject.toml references the package string
"odh-notebooks-meta-workbench-datascience-deps" via a local path that doesn't
exist; either create the missing consolidated package under
dependencies/odh-notebooks-meta-workbench-datascience-deps/ with a valid
pyproject.toml (matching the package name) or update the dependency entry in
pyproject.toml to the correct existing package/path or published package name so
dependency resolution succeeds when building.
Comment on lines
+143
to
+147
| version_str = result.stdout.strip().split()[1] if result.stdout.strip() else "0.0.0" | ||
| except (IndexError, FileNotFoundError): | ||
| version_str = "0.0.0" | ||
|
|
||
| version_tuple = tuple(int(x) for x in version_str.split(".")) |
There was a problem hiding this comment.
Handle potential ValueError from non-numeric version parts.
If version_str contains non-numeric parts (e.g., "0.10.6-dev"), the int(x) conversion will raise ValueError.
Suggested defensive fix
- version_tuple = tuple(int(x) for x in version_str.split("."))
+ try:
+ version_tuple = tuple(int(x) for x in version_str.split(".")[:3])
+ except ValueError:
+ version_tuple = (0, 0, 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/pylocks_generator.py` around lines 143 - 147, The code currently
builds version_tuple from version_str using tuple(int(x) for x in
version_str.split(".")) which will raise ValueError for non-numeric parts like
"0.10.6-dev"; update the logic around version_str/version_tuple to sanitize or
parse each segment before int conversion (e.g., split on hyphen or use a regex
to extract leading digits per segment) and catch ValueError so non-numeric
suffixes fall back to their numeric prefix or to 0; ensure the change is applied
where version_str is set and where version_tuple is computed so
functions/variables version_str and version_tuple handle inputs like
"0.10.6-dev" without raising.
- RUF067: move PROJECT_ROOT to tests/_common.py - pyright: remove return from finally block - ruff format: reformat 2 test files - Remove missing generate_kustomization.py from generate_code.sh - Use python-version-file in CI workflows instead of hardcoded version Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
PR needs rebase. DetailsInstructions 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cherry-picks developer tooling improvements from
maintorhoai-3.3:dependencies/cve-constraints.txt— centralized CVE version constraints applied during lock generation (fix(CVE): centralize CVE constraints and document resolution workflow opendatahub-io/notebooks#2886)override-dependenciestocve-constraints.txtuv.toml+./uvwrapper — pins uv version (==0.10.6) to avoid version mismatch issues (ISSUE #3032: chore(uv): pin uv version to fix CI check-generated-code failure opendatahub-io/notebooks#3034)scripts/pylocks_generator.py— Python rewrite of lock generator, replacing shell script (ISSUE #3036: chore(scripts/): rewritescripts/pylocks_generator.shto Python opendatahub-io/notebooks#3057)./uvfix — uses pinned uv in test target (NO-JIRA: chore: fix Makefiletesttarget to use correct./uvscript opendatahub-io/notebooks#3077)All commits cherry-picked with
-xfor traceability.Related: opendatahub-io#3197
Test plan
./uv --versionshows0.10.6cat dependencies/cve-constraints.txtshows urllib3 and keras constraintsmake refresh-lock-filestarget worksmake testpasses🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Improvements