-
Notifications
You must be signed in to change notification settings - Fork 122
feat: add automated dependency lock generation script #2644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughA new Bash script is added to automate Python lock file generation. The script scans designated project directories for pyproject.toml files, identifies Python flavors via Dockerfile variants (CPU, CUDA, ROCm), and invokes uv pip compile with appropriate index URLs to generate corresponding lock files, including error handling and result aggregation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/pylocks_generator.sh (3)
7-9: Quote the$indexvariable for consistency and defensive coding.While the unquoted expansion on line 97 works correctly (the URL has no spaces and expands as a single argument), it's inconsistent with other quoted variables in the command and less defensive. Quote it for consistency.
$HAS_CPU && run_lock "cpu" "$CPU_INDEX" $HAS_CUDA && run_lock "cuda" "$CUDA_INDEX" $HAS_ROCM && run_lock "rocm" "$ROCM_INDEX"Then in the
run_lockfunction:uv pip compile pyproject.toml \ --output-file "$output" \ --format pylock.toml \ --generate-hashes \ --emit-index-url \ --python-version="$PYTHON_VERSION" \ --python-platform linux \ --no-annotate \ - $index + "$index"Also applies to: 97-97
101-107: Consider capturing and logginguv pip compilestderr for better diagnostics.When
uv pip compilefails on line 98, the error output is not captured or logged. This makes it difficult for users to diagnose failures when they occur (e.g., missing dependencies, invalid Python version, malformed pyproject.toml).Consider capturing stderr to help users debug:
if [ $status -ne 0 ]; then warn "${flavor^^} lock failed in $TARGET_DIR" warn "Stderr output: $(uv pip compile ... 2>&1 || true)" rm -f "$output" DIR_SUCCESS=false elseAlternatively, document that users can re-run the command manually with full output for debugging.
24-25: Add validation for targeted directory input.When a user provides a targeted directory via command-line argument (line 25), the script doesn't validate that the directory exists or contains
pyproject.toml. This leads to opaque failures later (e.g., duringcdor whenuv pip compilecan't findpyproject.toml).Add validation after line 25:
if [ $# -gt 0 ]; then TARGET_DIRS=("$1") if [ ! -d "$1" ] || [ ! -f "$1/pyproject.toml" ]; then error "Target directory '$1' does not exist or does not contain pyproject.toml" exit 1 fiThis provides immediate, actionable feedback to users.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/pylocks_generator.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: code-static-analysis
🔇 Additional comments (1)
scripts/pylocks_generator.sh (1)
1-2: Solid error handling setup.The shebang and
set -euo pipefailestablish safe defaults that will catch and exit on errors, undefined variables, and pipeline failures.
| for TARGET_DIR in "${TARGET_DIRS[@]}"; do | ||
| echo | ||
| echo "===================================================================" | ||
| info "Processing directory: $TARGET_DIR" | ||
| echo "===================================================================" | ||
|
|
||
| cd "$TARGET_DIR" || continue | ||
| mkdir -p uv.lock | ||
| PYTHON_VERSION="${PWD##*-}" | ||
|
|
||
| # Detect available Dockerfiles (flavors) | ||
| HAS_CPU=false | ||
| HAS_CUDA=false | ||
| HAS_ROCM=false | ||
| [ -f "Dockerfile.cpu" ] && HAS_CPU=true | ||
| [ -f "Dockerfile.cuda" ] && HAS_CUDA=true | ||
| [ -f "Dockerfile.rocm" ] && HAS_ROCM=true | ||
|
|
||
| if ! $HAS_CPU && ! $HAS_CUDA && ! $HAS_ROCM; then | ||
| warn "No Dockerfiles found in $TARGET_DIR (cpu/cuda/rocm). Skipping." | ||
| cd - >/dev/null | ||
| continue | ||
| fi | ||
|
|
||
| echo "📦 Python version: $PYTHON_VERSION" | ||
| echo "🧩 Detected flavors:" | ||
| $HAS_CPU && echo " • CPU" | ||
| $HAS_CUDA && echo " • CUDA" | ||
| $HAS_ROCM && echo " • ROCm" | ||
| echo | ||
|
|
||
| DIR_SUCCESS=true | ||
|
|
||
| run_lock() { | ||
| local flavor="$1" | ||
| local index="$2" | ||
| local output="uv.lock/pylock.${flavor}.toml" | ||
|
|
||
| echo "➡️ Generating ${flavor^^} lock file..." | ||
| set +e | ||
| uv pip compile pyproject.toml \ | ||
| --output-file "$output" \ | ||
| --format pylock.toml \ | ||
| --generate-hashes \ | ||
| --emit-index-url \ | ||
| --python-version="$PYTHON_VERSION" \ | ||
| --python-platform linux \ | ||
| --no-annotate \ | ||
| $index | ||
| local status=$? | ||
| set -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Add pre-flight check for uv command availability.
The script invokes uv pip compile (line 89) without verifying that the uv command is installed or in PATH. If uv is missing, the error will be cryptic and unhelpful to users.
Add a check near the beginning of the script (after line 41):
# Pre-flight check
if ! command -v uv &>/dev/null; then
error "uv command not found. Please install uv: https://github.com/astral-sh/uv"
exit 1
fiAdditionally, verify that the uv pip compile flags used in the script are supported:
--emit-index-url(line 93)--format pylock.toml(line 92)
These flags are critical to correct behavior and should be tested or documented.
I need to gather information to verify the review comment. Let me examine the script and check the uv tool documentation.
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if scripts/pylocks_generator.sh exists and show its content from the beginning
fd "pylocks_generator.sh" --type f --exec cat {} \;
Length of output: 3895
🌐 Web query:
uv pip compile --emit-index-url --format pylock.toml flags documentation
💡 Result:
Short answer — what those flags do and how to use them with uv:
-
--emit-index-url — include index URLs in the compiled output (uv by default omits index URLs). When passed, uv will emit all index URLs it knows about (including the default PyPI URL). [1][2]
-
--format pylock.toml — tell uv to write the lock in the PEP‑751 pylock.toml format instead of requirements.txt. uv supports exporting/compiling to pylock.toml and can later install/sync from it. [1]
-
Notes / usage:
- uv pip compile prints output to stdout by default; use -o/--output-file to write to a file. Example: uv pip compile requirements.in -o pylock.toml --format pylock.toml --emit-index-url. [1]
- uv’s --emit-index-url behavior differs from pip-tools: uv will include all index URLs when asked, including the default. [2]
Sources:
[1] uv — Locking / pip compile documentation.
[2] uv — Compatibility with pip (describes --emit-index-url differences).
Add pre-flight check for uv command availability.
The script invokes uv pip compile (line 89) without verifying that the uv command is installed or in PATH. If uv is missing, the error will be cryptic and unhelpful to users.
Add a check near the beginning of the script (after line 41):
# Pre-flight check
if ! command -v uv &>/dev/null; then
error "uv command not found. Please install uv: https://github.com/astral-sh/uv"
exit 1
fiThe flags --emit-index-url and --format pylock.toml are both valid uv commands supported for pip compile operations.
🤖 Prompt for AI Agents
In scripts/pylocks_generator.sh around lines 49 to 99 (add check after line 41),
the script invokes `uv` without verifying it exists; add a pre-flight check
immediately after line 41 that uses `command -v uv` to detect availability, and
if missing call the existing error/log helper with a clear message (e.g. "uv
command not found. Please install uv: https://github.com/astral-sh/uv") and exit
1; ensure the check returns non-zero on missing `uv` so the script fails fast
before reaching the `uv pip compile` invocation.
|
|
||
| cd "$TARGET_DIR" || continue | ||
| mkdir -p uv.lock | ||
| PYTHON_VERSION="${PWD##*-}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Add validation for Python version extraction from directory name.
The extraction PYTHON_VERSION="${PWD##*-}" assumes the directory follows a naming convention (e.g., ubi9-python-3.12). If a directory doesn't end with a version suffix or lacks a -, the extraction produces an invalid or empty result, leading to silent failures or cryptic errors from uv pip compile --python-version="".
Add a validation check after line 57:
PYTHON_VERSION="${PWD##*-}"
# Validate Python version extraction
if [[ ! "$PYTHON_VERSION" =~ ^[0-9]+\.[0-9]+$ ]]; then
warn "Could not extract valid Python version from directory name: $PWD"
warn "Expected directory format: .../ubi9-python-X.Y"
cd - >/dev/null
continue
fiThis ensures only directories with valid version suffixes are processed.
🤖 Prompt for AI Agents
In scripts/pylocks_generator.sh around line 57, the code extracts PYTHON_VERSION
using PYTHON_VERSION="${PWD##*-}" but does not validate the result; add a
validation step immediately after that extraction to check the extracted value
matches a semantic version pattern like X.Y (e.g., using a regex
^[0-9]+\.[0-9]+$), and if it fails log/warn that the directory name could not
yield a valid Python version (include the PWD and expected format), then change
back to the previous directory and continue the loop to skip processing this
directory.
|
@atheo89: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Related to: https://issues.redhat.com/browse/RHAIENG-1489
and partially to: https://issues.redhat.com/browse/RHAIENG-1649
Description
This PR introduces
pylock_generator.sh, a Bash automation tool that generatesuvlock files (pylock.toml) for all supported notebook image flavors and runtimes.Key features:
How can be used
1. Run for in automatic mode (repo-wide)
bash scripts/pylock_generator.sh2. Run for a specific image directory
bash scripts/pylock_generator.sh jupyter/minimal/ubi9-python-3.12NOTE: If there is a missing package on
pyproject.tomlit will produces the following(for example):so in this case you can comment out problematic package in the above case the kubeflow-training, and run it again, and probably it will print another missing package, and so on. So these missing packages should be reported to AIPCC team.
(PS: I didn't find a good solution to skip the missing packages and report a log error with the missing onces)
Merge criteria:
Summary by CodeRabbit