ISSUE #3036: chore(scripts/): rewrite scripts/pylocks_generator.sh to Python#3057
Merged
jiridanek merged 4 commits intoopendatahub-io:mainfrom Mar 5, 2026
Merged
Conversation
…generator.sh` to Python
**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)
## Scope
Minimal, focused PR: rewrite the script, update call sites, delete old script. No tests, no ruff include changes (follow-up).
## Diff readability strategy
- **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)
## Implementation
### 1. Add typer dependency
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.
### 2. Create `scripts/pylocks_generator.py`
Use typer for CLI with the same interface as the bash script:
```python
#!/usr/bin/env python3
"""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()`
### 3. Update call sites
- [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"`
### 4. Delete `scripts/pylocks_generator.sh`
Clean deletion. It remains in git history.
### 5. Verify
Run `python3 scripts/pylocks_generator.py --help` and a targeted invocation like `python3 scripts/pylocks_generator.py auto jupyter/minimal/ubi9-python-3.12` (will fail at uv compile without network, but validates arg parsing and directory discovery).
…upported features, and index modes details
Contributor
|
Skipping CI for Draft Pull Request. |
github-actions
Bot
added
the
review-requested
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📝 WalkthroughWalkthroughThis PR migrates the Python lock file generator from a Bash script to a Python script using Typer. The new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
I liked the rewrite from Bash to Python /lgtm |
daniellutz
approved these changes
Mar 5, 2026
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: daniellutz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
437-444:⚠️ Potential issue | 🟠 MajorUse
./uv runto ensure Typer dev dependency is available.The script imports Typer (a dev dependency), so plain
python3invocation will fail unless the environment is explicitly configured. Using./uv runensures the development environment is activated with all dependencies and aligns with the existinguv run pytestpattern already in this Makefile.Proposed change
refresh-lock-files: `@echo` "===================================================================" `@echo` "🔁 Refreshing lock files using INDEX_MODE=$(INDEX_MODE)" `@echo` "===================================================================" - `@cd` $(ROOT_DIR) && python3 scripts/pylocks_generator.py $(INDEX_MODE) $(DIR) + `@cd` $(ROOT_DIR) && ./uv run scripts/pylocks_generator.py $(INDEX_MODE) $(DIR)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 437 - 444, The Makefile target refresh-lock-files invokes the script scripts/pylocks_generator.py with plain python3 but Typer is a dev dependency and may not be available; update the command in the refresh-lock-files recipe to run the script via the development runner (use ./uv run) instead of plain python3 so the Typer dependency is available at runtime (i.e., change the cd $(ROOT_DIR) && python3 scripts/pylocks_generator.py ... line to use ./uv run to execute the script with the same $(INDEX_MODE) and $(DIR) arguments).
🧹 Nitpick comments (5)
scripts/pylocks_generator.py (4)
231-299: Prefermode: IndexModeovermode: strto avoid accidental string drift.
run_lock(..., mode: str, ...)is passed values derived from the enum; typing it asIndexMode(or at leastLiteral["public-index","rh-index"]) reduces accidental mismatch and cleans up the"public-index"string compare.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pylocks_generator.py` around lines 231 - 299, The function run_lock currently types the mode parameter as str which allows accidental string drift; change its signature to accept the IndexMode enum (or Literal["public-index","rh-index"]) instead of str, update any callers that pass enum members to pass IndexMode values (or the specified Literals), and replace the string compare if mode == "public-index" with a comparison against the enum member (e.g., if mode is IndexMode.PUBLIC_INDEX) or the literal value accordingly; ensure imports include IndexMode and update type hints in run_lock and any related function signatures to maintain type consistency.
113-122: Harden.confparsing slightly (strip simple quotes +exportprefix).
read_conf_value()is intentionally simple, but a tiny bit more tolerance (e.g.,INDEX_URL="..."orexport INDEX_URL=...) will prevent confusing “INDEX_URL not found” failures. This should stay non-shell-evaluating for safety.Proposed hardening
def read_conf_value(conf_file: Path, key: str) -> str | None: """Read a key=value from a .conf file, skipping comments and blank lines.""" for line in conf_file.read_text().splitlines(): stripped = line.strip() if stripped.startswith("#") or "=" not in stripped: continue k, _, v = stripped.partition("=") - if k.strip() == key: - return v.strip() + k = k.strip() + if k.startswith("export "): + k = k.removeprefix("export ").strip() + if k == key: + value = v.strip() + if len(value) >= 2 and value[0] == value[-1] and value[0] in ("'", '"'): + value = value[1:-1] + return value return NoneAlso applies to: 199-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pylocks_generator.py` around lines 113 - 122, Update read_conf_value to tolerate an optional "export " prefix on the left-hand side and to strip matching surrounding single or double quotes from the parsed value without invoking a shell; specifically, in the function read_conf_value() detect and ignore a leading "export " (case-sensitive) before comparing the key (so "export INDEX_URL=..." matches INDEX_URL), and after extracting v.strip() remove one pair of matching surrounding quotes if present (i.e., both start and end are both "'" or both '"') before returning; apply the same logic to the other similar parser function(s) referenced in the file (the block around the later similar parsing code) so both places accept quoted values and optional export prefixes while still avoiding any shell evaluation.
160-171: Deduplicate + sort discovered target dirs for stable output.
find_target_dirs()can theoretically return duplicates and has filesystem-order iteration. Sorting + deduping makes the run log and summary stable (helpful for CI diffs).Proposed tweak
def find_target_dirs(target_dir: Path | None) -> list[Path]: """Find directories containing pyproject.toml.""" if target_dir is not None: return [target_dir] @@ - return dirs + # stable / unique + return sorted(set(dirs))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pylocks_generator.py` around lines 160 - 171, find_target_dirs may return duplicate Path entries and rely on filesystem order, causing non-deterministic output; update the function (referencing find_target_dirs, MAIN_DIRS, ROOT_DIR) to deduplicate the discovered dirs and return them in a stable sorted order: collect discovered Path objects, remove duplicates (e.g., via a set or dict.fromkeys to preserve uniqueness), then sort the resulting paths deterministically (e.g., by their string or as_posix()) and return the sorted list of Path objects so logs and summaries are stable for CI.
345-355: Skipped dirs aren’t reported as failed/success; consider tracking them explicitly.If a directory is discovered but skipped (bad naming / no Dockerfiles), it won’t appear in either list and the script can exit 0. If CI expects full coverage, consider a
skipped_dirslist in the summary (even if you still exit 0).Also applies to: 401-408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pylocks_generator.py` around lines 345 - 355, The loop that checks python_version (via extract_python_version) and flavors (via detect_flavors) currently calls warn and continues but doesn't record skipped directories; add a skipped_dirs list and push entries like (tdir, "invalid_name") when python_version is None and (tdir, "no_dockerfiles") when not flavors, then include skipped_dirs alongside the existing success/failure summaries at the end of the script (and repeat the same change for the second similar block around the detect_flavors check mentioned at lines 401-408) so CI can see which dirs were discovered but skipped while retaining the current exit behavior.scripts/lockfile-generators/create-requirements-lockfile.sh (1)
96-103: FYI:source "$CONF_FILE"executes shell; keep conf files non-executable.Not introduced by this PR, but worth reiterating: sourcing a conf file is code execution. If these files are ever user-controlled, this becomes a sharp edge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lockfile-generators/create-requirements-lockfile.sh` around lines 96 - 103, The script currently uses source "$CONF_FILE" which executes arbitrary shell from CONF_FILE (CONF_FILE, PROJECT_DIR, FLAVOR); replace this with a safe parser that reads only simple KEY=VALUE pairs (e.g., read line-by-line, ignore lines starting with #, validate keys like INDEX_URL and other expected vars, strip quotes) instead of sourcing, or if sourcing must remain, add strict checks beforehand: ensure CONF_FILE is owned by a trusted user, not writable by others, and not executable, and explicitly document that conf files must be trusted; update the code around CONF_FILE and the place where INDEX_URL and other variables are consumed to use the parsed values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/pylocks_generator.py`:
- Around line 130-152: The Ruff S603 lint error is raised for the subprocess.run
calls used to query the local uv wrapper; fix by either adding an inline
exemption comment "# noqa: S603" with a brief justification comment explaining
the call is not using shell=True and arguments are controlled (trusted local
./uv wrapper), or refactor both call sites (the one inside check_uv and the
other subprocess.run usage nearby) into a single helper function (e.g.,
run_trusted_subprocess or invoke_uv) that wraps subprocess.run, includes a
docstring explaining the trust boundary, and places the "# noqa: S603" on the
single subprocess.run line — update call sites to use that helper.
- Around line 306-317: The CLI is wired as a subcommand via `@app.command`(),
causing invocations like `python ... auto` to be treated as command names;
change the decorator on the main function from `@app.command`() to `@app.callback`()
(keeping the existing typer.Typer(add_completion=False) instance named app and
the same main(...) signature) so main becomes the root command and positional
arguments (IndexMode) are parsed as intended when calling app().
---
Outside diff comments:
In `@Makefile`:
- Around line 437-444: The Makefile target refresh-lock-files invokes the script
scripts/pylocks_generator.py with plain python3 but Typer is a dev dependency
and may not be available; update the command in the refresh-lock-files recipe to
run the script via the development runner (use ./uv run) instead of plain
python3 so the Typer dependency is available at runtime (i.e., change the cd
$(ROOT_DIR) && python3 scripts/pylocks_generator.py ... line to use ./uv run to
execute the script with the same $(INDEX_MODE) and $(DIR) arguments).
---
Nitpick comments:
In `@scripts/lockfile-generators/create-requirements-lockfile.sh`:
- Around line 96-103: The script currently uses source "$CONF_FILE" which
executes arbitrary shell from CONF_FILE (CONF_FILE, PROJECT_DIR, FLAVOR);
replace this with a safe parser that reads only simple KEY=VALUE pairs (e.g.,
read line-by-line, ignore lines starting with #, validate keys like INDEX_URL
and other expected vars, strip quotes) instead of sourcing, or if sourcing must
remain, add strict checks beforehand: ensure CONF_FILE is owned by a trusted
user, not writable by others, and not executable, and explicitly document that
conf files must be trusted; update the code around CONF_FILE and the place where
INDEX_URL and other variables are consumed to use the parsed values.
In `@scripts/pylocks_generator.py`:
- Around line 231-299: The function run_lock currently types the mode parameter
as str which allows accidental string drift; change its signature to accept the
IndexMode enum (or Literal["public-index","rh-index"]) instead of str, update
any callers that pass enum members to pass IndexMode values (or the specified
Literals), and replace the string compare if mode == "public-index" with a
comparison against the enum member (e.g., if mode is IndexMode.PUBLIC_INDEX) or
the literal value accordingly; ensure imports include IndexMode and update type
hints in run_lock and any related function signatures to maintain type
consistency.
- Around line 113-122: Update read_conf_value to tolerate an optional "export "
prefix on the left-hand side and to strip matching surrounding single or double
quotes from the parsed value without invoking a shell; specifically, in the
function read_conf_value() detect and ignore a leading "export "
(case-sensitive) before comparing the key (so "export INDEX_URL=..." matches
INDEX_URL), and after extracting v.strip() remove one pair of matching
surrounding quotes if present (i.e., both start and end are both "'" or both
'"') before returning; apply the same logic to the other similar parser
function(s) referenced in the file (the block around the later similar parsing
code) so both places accept quoted values and optional export prefixes while
still avoiding any shell evaluation.
- Around line 160-171: find_target_dirs may return duplicate Path entries and
rely on filesystem order, causing non-deterministic output; update the function
(referencing find_target_dirs, MAIN_DIRS, ROOT_DIR) to deduplicate the
discovered dirs and return them in a stable sorted order: collect discovered
Path objects, remove duplicates (e.g., via a set or dict.fromkeys to preserve
uniqueness), then sort the resulting paths deterministically (e.g., by their
string or as_posix()) and return the sorted list of Path objects so logs and
summaries are stable for CI.
- Around line 345-355: The loop that checks python_version (via
extract_python_version) and flavors (via detect_flavors) currently calls warn
and continues but doesn't record skipped directories; add a skipped_dirs list
and push entries like (tdir, "invalid_name") when python_version is None and
(tdir, "no_dockerfiles") when not flavors, then include skipped_dirs alongside
the existing success/failure summaries at the end of the script (and repeat the
same change for the second similar block around the detect_flavors check
mentioned at lines 401-408) so CI can see which dirs were discovered but skipped
while retaining the current exit behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db67afe4-a0b7-4bea-9c65-8ebe9227ad89
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Makefileci/generate_code.shpyproject.tomlscripts/lockfile-generators/create-requirements-lockfile.shscripts/pylocks_generator.pyscripts/pylocks_generator.sh
💤 Files with no reviewable changes (1)
- scripts/pylocks_generator.sh
Comment on lines
+130
to
+152
| def check_uv() -> None: | ||
| """Verify the uv wrapper exists and meets the minimum version requirement.""" | ||
| if not UV.is_file() or not os.access(UV, os.X_OK): | ||
| error(f"Expected uv wrapper at '{UV}' but it is missing or not executable.") | ||
| raise SystemExit(1) | ||
|
|
||
| try: | ||
| result = subprocess.run( | ||
| [str(UV), "--version"], | ||
| capture_output=True, | ||
| text=True, | ||
| check=False, | ||
| ) | ||
| 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(".")) | ||
| if version_tuple < UV_MIN_VERSION: | ||
| min_ver = ".".join(str(x) for x in UV_MIN_VERSION) | ||
| error(f"uv version {version_str} found, but >= {min_ver} is required.") | ||
| error("Please upgrade uv: https://github.com/astral-sh/uv") | ||
| raise SystemExit(1) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file exists and get its total line count
wc -l scripts/pylocks_generator.pyRepository: opendatahub-io/notebooks
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Read the specific lines mentioned in the review (130-152 and 260-299)
echo "=== Lines 130-152 ==="
sed -n '130,152p' scripts/pylocks_generator.py
echo ""
echo "=== Lines 260-299 ==="
sed -n '260,299p' scripts/pylocks_generator.pyRepository: opendatahub-io/notebooks
Length of output: 2159
🏁 Script executed:
#!/bin/bash
# Search for all subprocess.run calls in the file
echo "=== All subprocess.run calls ==="
rg -n 'subprocess\.run' scripts/pylocks_generator.py -A 5Repository: opendatahub-io/notebooks
Length of output: 553
🏁 Script executed:
#!/bin/bash
# Check if ruff is available and run it on the file to see actual S603 warnings
if command -v ruff &> /dev/null; then
echo "=== Ruff S603 check on the file ==="
ruff check scripts/pylocks_generator.py --select S603 2>&1
else
echo "ruff not available, searching for S603 in context"
fiRepository: opendatahub-io/notebooks
Length of output: 1005
🏁 Script executed:
#!/bin/bash
# Check what index_flags is and where it comes from
rg -n 'index_flags' scripts/pylocks_generator.py -B 3 -A 1Repository: opendatahub-io/notebooks
Length of output: 849
🏁 Script executed:
#!/bin/bash
# Get more context around where cmd is built and index_flags is set
sed -n '250,290p' scripts/pylocks_generator.pyRepository: opendatahub-io/notebooks
Length of output: 1905
🏁 Script executed:
#!/bin/bash
# Check what NO_EMIT_PACKAGES and CVE_CONSTRAINTS_FILE are
rg -n 'NO_EMIT_PACKAGES|CVE_CONSTRAINTS_FILE' scripts/pylocks_generator.py | head -20Repository: opendatahub-io/notebooks
Length of output: 333
🏁 Script executed:
#!/bin/bash
# Check for any shell=True parameter in the subprocess calls
grep -n 'shell=' scripts/pylocks_generator.pyRepository: opendatahub-io/notebooks
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Get the function signature and parameter types for run_lock
sed -n '231,245p' scripts/pylocks_generator.pyRepository: opendatahub-io/notebooks
Length of output: 540
🏁 Script executed:
#!/bin/bash
# Check UV definition and imports
sed -n '1,100p' scripts/pylocks_generator.py | grep -E 'import|UV\s*='Repository: opendatahub-io/notebooks
Length of output: 261
🏁 Script executed:
#!/bin/bash
# Get full context of UV definition
rg -n '^UV' scripts/pylocks_generator.py -A 2Repository: opendatahub-io/notebooks
Length of output: 288
Address Ruff S603 on subprocess.run calls (CI blocker).
Ruff is flagging both subprocess.run(...) calls (lines 137 and 290) with S603. While the security concern doesn't apply here (no shell=True, all arguments are controlled), the linting error blocks CI. Add # noqa: S603 comments with brief justification, or centralize subprocess invocation in a helper that documents the trust boundary (local repo ./uv wrapper, arguments are not shell-interpreted).
Proposed minimal lint fix
result = subprocess.run(
[str(UV), "--version"],
capture_output=True,
text=True,
check=False,
- )
+ ) # noqa: S603 # local uv wrapper; args are not shell-interpreted
@@
- result = subprocess.run(cmd, cwd=project_dir, check=False)
+ result = subprocess.run(cmd, cwd=project_dir, check=False) # noqa: S603 # controlled command from internal config; no shell=True🧰 Tools
🪛 Ruff (0.15.2)
[error] 137-137: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/pylocks_generator.py` around lines 130 - 152, The Ruff S603 lint
error is raised for the subprocess.run calls used to query the local uv wrapper;
fix by either adding an inline exemption comment "# noqa: S603" with a brief
justification comment explaining the call is not using shell=True and arguments
are controlled (trusted local ./uv wrapper), or refactor both call sites (the
one inside check_uv and the other subprocess.run usage nearby) into a single
helper function (e.g., run_trusted_subprocess or invoke_uv) that wraps
subprocess.run, includes a docstring explaining the trust boundary, and places
the "# noqa: S603" on the single subprocess.run line — update call sites to use
that helper.
Comment on lines
+306
to
+317
| app = typer.Typer(add_completion=False) | ||
|
|
||
|
|
||
| @app.command() | ||
| def main( | ||
| index_mode: Annotated[ | ||
| IndexMode, typer.Argument(help="Index mode: auto, rh-index, or public-index") | ||
| ] = IndexMode.auto, | ||
| target_dir: Annotated[ | ||
| Path | None, typer.Argument(help="Specific project directory to process") | ||
| ] = None, | ||
| ) -> None: |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the actual code at the mentioned lines
cat -n scripts/pylocks_generator.py | sed -n '300,320p'Repository: opendatahub-io/notebooks
Length of output: 828
🏁 Script executed:
# Check the __main__ block and app() invocation
cat -n scripts/pylocks_generator.py | sed -n '400,420p'Repository: opendatahub-io/notebooks
Length of output: 490
🏁 Script executed:
# Find all call sites that invoke this script
find . -type f \( -name "Makefile" -o -name "*.sh" -o -name "*.py" \) -exec grep -l "pylocks_generator" {} \;Repository: opendatahub-io/notebooks
Length of output: 214
🏁 Script executed:
# Search for how this script is invoked
rg "pylocks_generator" --type makefile --type sh --type py -B 2 -A 2Repository: opendatahub-io/notebooks
Length of output: 100
🏁 Script executed:
# Search for how pylocks_generator is called in Makefile
grep -n "pylocks_generator" Makefile -B 2 -A 2Repository: opendatahub-io/notebooks
Length of output: 347
🏁 Script executed:
# Check ci/generate_code.sh
cat -n ci/generate_code.shRepository: opendatahub-io/notebooks
Length of output: 500
🏁 Script executed:
# Check tests/test_main.py
cat -n tests/test_main.py | head -100Repository: opendatahub-io/notebooks
Length of output: 5170
🏁 Script executed:
# Check if tests actually invoke the script with arguments
rg "pylocks_generator" tests/ -B 3 -A 3Repository: opendatahub-io/notebooks
Length of output: 1175
🏁 Script executed:
# Check if there are subprocess calls in test files
rg "subprocess" tests/test_main.py -B 2 -A 2Repository: opendatahub-io/notebooks
Length of output: 597
🏁 Script executed:
# Look for actual test invocations of the script
grep -r "INDEX_MODE" . --include="*.sh" --include="*.py" -B 2 -A 2Repository: opendatahub-io/notebooks
Length of output: 50
🏁 Script executed:
# Search for other Typer usage in the codebase to understand the pattern
rg "typer\." scripts/ manifests/ -B 3 -A 3 | head -80Repository: opendatahub-io/notebooks
Length of output: 1031
🏁 Script executed:
# Check Typer documentation/examples in imports or comments
rg "@app\." scripts/pylocks_generator.py -B 2 -A 5Repository: opendatahub-io/notebooks
Length of output: 258
🏁 Script executed:
# Check the function signature to understand parameter passing
cat -n scripts/pylocks_generator.py | sed -n '309,330p'Repository: opendatahub-io/notebooks
Length of output: 949
🏁 Script executed:
# Verify the Makefile target that uses this
cat -n Makefile | sed -n '435,450p'Repository: opendatahub-io/notebooks
Length of output: 927
🏁 Script executed:
# Check if ci/generate_code.sh is part of critical CI/test workflow
cat -n .github/workflows/*.yml 2>/dev/null | grep -A 5 -B 5 "generate_code" | head -40Repository: opendatahub-io/notebooks
Length of output: 50
🏁 Script executed:
# Search for workflow files and check for refresh-lock-files or pylocks references
find .github -name "*.yml" -o -name "*.yaml" | xargs grep -l "refresh-lock-files\|pylocks" 2>/dev/nullRepository: opendatahub-io/notebooks
Length of output: 106
🏁 Script executed:
# Check if there are any recent runs or examples showing the actual invocation
git log --oneline scripts/pylocks_generator.py | head -5Repository: opendatahub-io/notebooks
Length of output: 188
🏁 Script executed:
# Verify Typer behavior with a simple test: what happens with `@app.command`() when no subcommand given?
python3 << 'EOF'
import typer
from typing import Annotated
app = typer.Typer(add_completion=False)
`@app.command`()
def main(value: Annotated[str, typer.Argument()] = "default") -> None:
print(f"Got: {value}")
# Simulate what happens when we call app() with positional args
import sys
sys.argv = ["script.py", "rh-index"]
try:
app()
except SystemExit as e:
print(f"SystemExit: {e.code}")
except Exception as e:
print(f"Error: {type(e).__name__}: {e}")
EOFRepository: opendatahub-io/notebooks
Length of output: 185
🏁 Script executed:
# Check the piplock-renewal workflow to see how pylocks is invoked
cat -n .github/workflows/piplock-renewal.yamlRepository: opendatahub-io/notebooks
Length of output: 11282
Typer wiring makes main a subcommand; current invocations will fail.
With @app.command() + app(), Typer expects python scripts/pylocks_generator.py main .... But the Make target (line 443) and GitHub Actions workflow (.github/workflows/piplock-renewal.yaml:84) pass python ... rh-index or python ... auto, which Typer interprets as command names instead of arguments. This breaks the weekly lock file renewal workflow and any manual make refresh-lock-files INDEX_MODE=... invocations.
Proposed fix (make it the root command via callback)
app = typer.Typer(add_completion=False)
-@app.command()
+@app.callback()
def main(
index_mode: Annotated[
IndexMode, typer.Argument(help="Index mode: auto, rh-index, or public-index")
] = IndexMode.auto,
@@
if __name__ == "__main__":
app()📝 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.
Suggested change
| app = typer.Typer(add_completion=False) | |
| @app.command() | |
| def main( | |
| index_mode: Annotated[ | |
| IndexMode, typer.Argument(help="Index mode: auto, rh-index, or public-index") | |
| ] = IndexMode.auto, | |
| target_dir: Annotated[ | |
| Path | None, typer.Argument(help="Specific project directory to process") | |
| ] = None, | |
| ) -> None: | |
| app = typer.Typer(add_completion=False) | |
| `@app.callback`() | |
| def main( | |
| index_mode: Annotated[ | |
| IndexMode, typer.Argument(help="Index mode: auto, rh-index, or public-index") | |
| ] = IndexMode.auto, | |
| target_dir: Annotated[ | |
| Path | None, typer.Argument(help="Specific project directory to process") | |
| ] = None, | |
| ) -> None: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/pylocks_generator.py` around lines 306 - 317, The CLI is wired as a
subcommand via `@app.command`(), causing invocations like `python ... auto` to be
treated as command names; change the decorator on the main function from
`@app.command`() to `@app.callback`() (keeping the existing
typer.Typer(add_completion=False) instance named app and the same main(...)
signature) so main becomes the root command and positional arguments (IndexMode)
are parsed as intended when calling app().
Member
Author
There was a problem hiding this comment.
❯ ./uv run python3 /Users/jdanek/IdeaProjects/notebooks/scripts/pylocks_generator.py --help
Usage: pylocks_generator.py [OPTIONS] [INDEX_MODE]:[auto|rh-index|public-
index] [TARGET_DIR]
Generate pylock.toml lock files for Python project directories.
╭─ Arguments ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ index_mode [INDEX_MODE]:[auto|rh-index|public-index] Index mode: auto, rh-index, or public-index [default: auto] │
│ target_dir [TARGET_DIR] Specific project directory to process │
╰─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
╭─ Options ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ --help Show this message and exit. │
╰─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
already works fine, no main subcommand in sight
Contributor
There was a problem hiding this comment.
@jiridanek, you're right — my analysis was incorrect. Typer only creates a subcommand structure when multiple @app.command() decorators are registered on the same Typer instance. With a single @app.command(), Typer wires that function directly as the root command, so invocations like python pylocks_generator.py rh-index <dir> work exactly as intended. The --help output you shared confirms this. I apologize for the noise.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
Contributor
|
@jiridanek: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Contributor
|
New changes are detected. LGTM label has been removed. |
jiridanek
commented
Mar 5, 2026
Member
Author
There was a problem hiding this comment.
- Makefile: python3 -> ./uv run -- Valid, typer won't be available without the venv.
- @app.command() -> @app.callback() -- Valid, this is how typer handles root-level positional args.
- mode: str -> mode: IndexMode -- Good type safety improvement.
- find_target_dirs sort/dedup -- Good for CI stability.
- read_conf_value hardening (export, quotes) -- Not needed, the actual .conf files don't use these, and the bash awk parser didn't handle them either. Skip.
- Skipped dirs tracking -- The bash script didn't do this either. Skip to preserve behavior parity.
- source "$CONF_FILE" in create-requirements-lockfile.sh -- Pre-existing, not introduced by this PR. Skip.
- S603 lint -- scripts/ isn't in ruff's include, so this won't fire. Skip.
Contributor
There was a problem hiding this comment.
@jiridanek, thanks for the thorough triage — understood on all points!
To summarize the accepted suggestions for tracking: ./uv run in Makefile, @app.callback(), mode: IndexMode type safety, and sort/dedup in find_target_dirs are all valid and worth addressing.
And the skipped items all make sense:
read_conf_valuehardening — the.conffiles don't useexportor quoted values, so it's unnecessary.- Skipped dirs tracking — behavior parity with the original Bash script is the right call.
source "$CONF_FILE"— pre-existing, out of scope.- S603 —
scripts/is outside ruff's include, so it's a non-issue here.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
**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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/pylocks_generator.py`:
- Around line 160-163: The function find_target_dirs currently returns a
provided Path without validation; update it to verify the Path exists, is a
directory, and contains a pyproject.toml (use Path.exists()/is_dir() and (path /
"pyproject.toml").exists()); if validation fails, raise a clear ValueError (or
SystemExit) with a message indicating the bad target_dir and the missing
pyproject.toml, otherwise return [target_dir] as before; keep the function
signature find_target_dirs(target_dir: Path | None) -> list[Path] and ensure
callers will receive the exception instead of silently doing nothing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d4e5994f-adb9-4f3e-8cae-996f382a5c18
📒 Files selected for processing (3)
Makefilescripts/lockfile-generators/create-requirements-lockfile.shscripts/pylocks_generator.py
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- scripts/lockfile-generators/create-requirements-lockfile.sh
Comment on lines
+160
to
+163
| def find_target_dirs(target_dir: Path | None) -> list[Path]: | ||
| """Find directories containing pyproject.toml.""" | ||
| if target_dir is not None: | ||
| return [target_dir] |
Contributor
There was a problem hiding this comment.
Validate target_dir before proceeding to avoid silent no-op runs.
When a specific directory is passed, the function returns it without checking whether it exists or contains pyproject.toml. A typo can make the command exit successfully while generating nothing.
Suggested fix
def find_target_dirs(target_dir: Path | None) -> list[Path]:
"""Find directories containing pyproject.toml."""
if target_dir is not None:
- return [target_dir]
+ candidate = target_dir if target_dir.is_absolute() else ROOT_DIR / target_dir
+ if not candidate.is_dir() or not (candidate / "pyproject.toml").is_file():
+ error(f"Target directory must exist and contain pyproject.toml: {candidate}")
+ raise SystemExit(1)
+ return [candidate]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/pylocks_generator.py` around lines 160 - 163, The function
find_target_dirs currently returns a provided Path without validation; update it
to verify the Path exists, is a directory, and contains a pyproject.toml (use
Path.exists()/is_dir() and (path / "pyproject.toml").exists()); if validation
fails, raise a clear ValueError (or SystemExit) with a message indicating the
bad target_dir and the missing pyproject.toml, otherwise return [target_dir] as
before; keep the function signature find_target_dirs(target_dir: Path | None) ->
list[Path] and ensure callers will receive the exception instead of silently
doing nothing.
Member
Author
There was a problem hiding this comment.
the bash version did not validate this either, but it's a good suggestion
create a github issue for later
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
gh issue create \
--repo opendatahub-io/notebooks \
--title "chore(scripts/pylocks_generator.py): validate target_dir exists and contains pyproject.toml" \
--body "## Summary
The \`find_target_dirs\` function in \`scripts/pylocks_generator.py\` returns a user-supplied \`target_dir\` without validating that it exists or contains a \`pyproject.toml\`. A typo can cause the command to exit successfully while generating nothing.
## Suggested Fix
\`\`\`diff
def find_target_dirs(target_dir: Path | None) -> list[Path]:
\"\"\"Find directories containing pyproject.toml.\"\"\"
if target_dir is not None:
- return [target_dir]
+ candidate = target_dir if target_dir.is_absolute() else ROOT_DIR / target_dir
+ if not candidate.is_dir() or not (candidate / \"pyproject.toml\").is_file():
+ error(f\"Target directory must exist and contain pyproject.toml: {candidate}\")
+ raise SystemExit(1)
+ return [candidate]
\`\`\`
## Context
- Raised during review of PR `#3057` (rewrite of \`scripts/pylocks_generator.sh\` to Python)
- The original Bash script did not perform this validation either, so this is a new improvement opportunity
- Review comment: https://github.com/opendatahub-io/notebooks/pull/3057#discussion_r2892622428
- Requested by `@jiridanek`
" \
--assignee jiridanek
Length of output: 55
@jiridanek, done! I've created a follow-up issue for this: #3058
It includes the suggested fix and backlinks to this PR and review comment.
jiridanek
force-pushed
the
jd/26/02/envs_in_bases
branch
from
fae8ad0 to
577baf6
Compare
jiridanek
added a commit
to red-hat-data-services/notebooks
that referenced
this pull request
Mar 26, 2026
…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)
Open
4 tasks
jiridanek
added a commit
to red-hat-data-services/notebooks
that referenced
this pull request
Mar 26, 2026
…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)
jiridanek
added a commit
to red-hat-data-services/notebooks
that referenced
this pull request
Mar 31, 2026
…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)
jiridanek
added a commit
to red-hat-data-services/notebooks
that referenced
this pull request
Mar 31, 2026
…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)
jiridanek
added a commit
to red-hat-data-services/notebooks
that referenced
this pull request
Apr 17, 2026
…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)
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.
https://redhat-internal.slack.com/archives/C096ZR053RQ/p1772745335297379
Scope
Minimal, focused PR: rewrite the script, update call sites, delete old script. No tests, no ruff include changes (follow-up).
Diff readability strategy
pylocks_generator.sh, 376 lines)pylocks_generator.py, ~200-250 lines), structured to mirror the bash script's section layout so reviewers can compare side by sideImplementation
1. Add typer dependency
Add
typerto[dependency-groups] devin pyproject.toml (it's already inuv.lockas a transitive dep, but needs to be explicit). Runuv lockto sync.2. Create
scripts/pylocks_generator.pyUse typer for CLI with the same interface as the bash script:
Key functions (same logical sections as the bash script):
read_conf_value(conf_file: Path, key: str) -> str | None-- simple line parser replacing awkcheck_uv()-- verify./uvexists, parse version, compare astuple(int, ...)>=(0, 4, 0)find_target_dirs(target_dir: Path | None) -> list[Path]--pathlib.glob("**/pyproject.toml")detect_flavors(project_dir: Path) -> set[str]-- checkDockerfile.{cpu,cuda,rocm}existenceget_index_flags(project_dir: Path, flavor: str) -> list[str] | None-- readbuild-args/<flavor>.confrun_lock(project_dir, flavor, index_flags, mode, python_version, upgrade) -> bool--subprocess.run(cwd=project_dir)with list argsImplementation notes:
uv pip compilecommand as a list, conditionally appending--upgradecwd=target_dirinsubprocess.run()andos.path.relpath()for CVE constraints pathFORCE_LOCKFILES_UPGRADEenv var read viaos.environ.get()3. Update call sites
bash scripts/pylocks_generator.sh $(INDEX_MODE) $(DIR)->python3 scripts/pylocks_generator.py $(INDEX_MODE) $(DIR)bash scripts/pylocks_generator.sh->"${REPO_ROOT}/uv" run scripts/pylocks_generator.py(matches the two lines above it)PYLOCKS_GENERATOR="scripts/pylocks_generator.sh"->.pybash "$PYLOCKS_GENERATOR"->python3 "$PYLOCKS_GENERATOR"4. Delete
scripts/pylocks_generator.shClean deletion. It remains in git history.
How Has This Been Tested?
Run
python3 scripts/pylocks_generator.py --helpand a targeted invocation likepython3 scripts/pylocks_generator.py auto jupyter/minimal/ubi9-python-3.12.Self checklist (all need to be checked):
make test(gmakeon macOS) before asking for reviewDockerfile.konfluxfiles should be done inodh/notebooksand automatically synced torhds/notebooks. For Konflux-specific changes, modifyDockerfile.konfluxfiles directly inrhds/notebooksas these require special attention in the downstream repository and flow to the upcoming RHOAI release.Merge criteria:
Summary by CodeRabbit