fix: Use list-deps --format uv to simplify dependency generation#215
fix: Use list-deps --format uv to simplify dependency generation#215gyliu513 wants to merge 1 commit intoopendatahub-io:mainfrom
Conversation
📝 WalkthroughWalkthroughRefactors dependency extraction in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In `@distribution/build.py`:
- Around line 145-150: The replacement currently munges existing extras (e.g.,
turning pymilvus[grpc] into pymilvus[milvus-lite][grpc]); update the logic
around deps_line to detect pymilvus with optional extras and then either add
"milvus-lite" into the existing bracketed extras (unless already present) or add
a new [milvus-lite] when no extras exist; reference the same variable deps_line
and the current regex usage, replacing it with a function-based re.sub callback
that inspects the match, parses existing extras, de-duplicates "milvus-lite",
and returns the correct specifier (pymilvus[...milvus-lite...] or
pymilvus[milvus-lite]).
In `@tests/test_build.py`:
- Around line 1-7: Remove the shebang line ("#!/usr/bin/env python3") at the top
of tests/test_build.py so the file no longer triggers the executable-shebang
pre-commit check, then re-run ruff-format (and pre-commit) to apply formatting
and ensure the file passes checks before committing; alternatively, if you
prefer keeping the shebang, mark the file executable and re-run
ruff-format/pre-commit instead.
🧹 Nitpick comments (1)
tests/test_build.py (1)
36-39: Assert the exactlist-depscommand.Right now the test only checks call count. Tighten it to assert the exact command and flags to catch regressions in the CLI invocation.
✅ Suggested assertion
- run.assert_called_once() + run.assert_called_once_with( + "llama stack list-deps --format uv distribution/config.yaml", + shell=True, + capture_output=True, + text=True, + check=True, + )
| #!/usr/bin/env python3 | ||
| import sys | ||
| from pathlib import Path | ||
| import subprocess | ||
| import unittest | ||
| from unittest import mock | ||
|
|
There was a problem hiding this comment.
Fix pre-commit failures (shebang + formatting).
Pre-commit failed because the file has a shebang but isn’t executable, and ruff-format reformatted it. Remove the shebang (or mark executable) and re-run ruff-format before re-pushing.
🧹 Minimal fix (remove shebang)
-#!/usr/bin/env python3📝 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.
| #!/usr/bin/env python3 | |
| import sys | |
| from pathlib import Path | |
| import subprocess | |
| import unittest | |
| from unittest import mock | |
| import sys | |
| from pathlib import Path | |
| import subprocess | |
| import unittest | |
| from unittest import mock | |
🧰 Tools
🪛 GitHub Actions: Pre-commit
[error] 1-1: Pre-commit check-shebang-scripts-are-executable failed: has a shebang but is not marked executable. If it is supposed to be executable, run 'chmod +x tests/test_build.py'.
[error] 1-1: ruff-format reformatted 1 file. 1 file reformatted, 2 files left unchanged.
🤖 Prompt for AI Agents
In `@tests/test_build.py` around lines 1 - 7, Remove the shebang line
("#!/usr/bin/env python3") at the top of tests/test_build.py so the file no
longer triggers the executable-shebang pre-commit check, then re-run ruff-format
(and pre-commit) to apply formatting and ensure the file passes checks before
committing; alternatively, if you prefer keeping the shebang, mark the file
executable and re-run ruff-format/pre-commit instead.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@distribution/build.py`:
- Around line 137-144: The loop handling result.stdout in distribution/build.py
assumes every non-empty raw_line starts with "uv pip install " before slicing
into deps_line; guard this by checking raw_line.startswith("uv pip install ")
before doing deps_line = raw_line[len("uv pip install "):], and skip or
log/raise for unexpected lines (e.g., warnings) so malformed install commands
are not produced; adjust the loop around the for line in
result.stdout.splitlines() / raw_line variables accordingly.
♻️ Duplicate comments (2)
distribution/build.py (1)
145-150: Handle existingpymilvusextras to avoid invalid specifiers.
Current regex injects[milvus-lite]even when extras already exist, producing invalidpymilvus[milvus-lite][grpc](Line 145‑150). Update the rewrite to merge extras instead.🛠️ Suggested fix
- deps_line = re.sub( - r"\bpymilvus(?!\[milvus-lite\])\b", - "pymilvus[milvus-lite]", - deps_line, - ) + # If extras already exist, append milvus-lite; otherwise add it. + deps_line = re.sub( + r"\bpymilvus\[(?![^\]]*milvus-lite)([^\]]+)\]", + r"pymilvus[\1,milvus-lite]", + deps_line, + ) + deps_line = re.sub( + r"\bpymilvus\b(?!\[)", + "pymilvus[milvus-lite]", + deps_line, + )tests/test_build.py (1)
1-1: Remove the shebang (or mark the file executable).
The pre‑commit hook fails because the file has a shebang but isn’t executable (Line 1). Remove it or chmod +x.🧹 Minimal fix
-#!/usr/bin/env python3
| for line in result.stdout.splitlines(): | ||
| line = line.strip() | ||
| if not line: # Skip empty lines | ||
| raw_line = line.strip() | ||
| if not raw_line: | ||
| continue | ||
|
|
||
| # New format: just packages, possibly with flags | ||
| # Fixes: https://issues.redhat.com/browse/RHAIENG-2710 | ||
| # TODO: remove this once we have a stable version of kubernetes. | ||
| # Use --prerelease=allow to permit pre-release dependencies (e.g. kubernetes==35.0.0a1) | ||
| cmd_parts = ["RUN", "uv", "pip", "install", "--prerelease=allow"] | ||
| packages_str = line | ||
|
|
||
| # Parse packages and flags from the line | ||
| # Use shlex.split to properly handle quoted package names | ||
| parts_list = shlex.split(packages_str) | ||
| packages = [] | ||
| flags = [] | ||
| extra_index_url = None | ||
|
|
||
| i = 0 | ||
| while i < len(parts_list): | ||
| if parts_list[i] == "--extra-index-url" and i + 1 < len(parts_list): | ||
| extra_index_url = parts_list[i + 1] | ||
| flags.extend([parts_list[i], parts_list[i + 1]]) | ||
| i += 2 | ||
| elif parts_list[i] == "--index-url" and i + 1 < len(parts_list): | ||
| flags.extend([parts_list[i], parts_list[i + 1]]) | ||
| i += 2 | ||
| elif parts_list[i] in ["--no-deps", "--no-cache"]: | ||
| flags.append(parts_list[i]) | ||
| i += 1 | ||
| else: | ||
| packages.append(parts_list[i]) | ||
| i += 1 | ||
|
|
||
| # Sort and deduplicate packages | ||
| packages = sorted(set(packages)) | ||
|
|
||
| # Add quotes to packages with > or < to prevent bash redirection | ||
| packages = [ | ||
| f"'{package}'" if (">" in package or "<" in package) else package | ||
| for package in packages | ||
| ] | ||
| # Format: uv pip install <packages/flags> | ||
| deps_line = raw_line[len("uv pip install ") :] | ||
|
|
There was a problem hiding this comment.
Guard against non‑install stdout lines before slicing.
If llama stack list-deps emits warnings or other stdout lines, deps_line = raw_line[len("uv pip install "):] will generate malformed install commands. Add a prefix check to skip or raise on unexpected lines (Line 137‑144).
🛠️ Proposed fix
- # Format: uv pip install <packages/flags>
- deps_line = raw_line[len("uv pip install ") :]
+ # Format: uv pip install <packages/flags>
+ if not raw_line.startswith("uv pip install "):
+ continue
+ deps_line = raw_line[len("uv pip install ") :]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for line in result.stdout.splitlines(): | |
| line = line.strip() | |
| if not line: # Skip empty lines | |
| raw_line = line.strip() | |
| if not raw_line: | |
| continue | |
| # New format: just packages, possibly with flags | |
| # Fixes: https://issues.redhat.com/browse/RHAIENG-2710 | |
| # TODO: remove this once we have a stable version of kubernetes. | |
| # Use --prerelease=allow to permit pre-release dependencies (e.g. kubernetes==35.0.0a1) | |
| cmd_parts = ["RUN", "uv", "pip", "install", "--prerelease=allow"] | |
| packages_str = line | |
| # Parse packages and flags from the line | |
| # Use shlex.split to properly handle quoted package names | |
| parts_list = shlex.split(packages_str) | |
| packages = [] | |
| flags = [] | |
| extra_index_url = None | |
| i = 0 | |
| while i < len(parts_list): | |
| if parts_list[i] == "--extra-index-url" and i + 1 < len(parts_list): | |
| extra_index_url = parts_list[i + 1] | |
| flags.extend([parts_list[i], parts_list[i + 1]]) | |
| i += 2 | |
| elif parts_list[i] == "--index-url" and i + 1 < len(parts_list): | |
| flags.extend([parts_list[i], parts_list[i + 1]]) | |
| i += 2 | |
| elif parts_list[i] in ["--no-deps", "--no-cache"]: | |
| flags.append(parts_list[i]) | |
| i += 1 | |
| else: | |
| packages.append(parts_list[i]) | |
| i += 1 | |
| # Sort and deduplicate packages | |
| packages = sorted(set(packages)) | |
| # Add quotes to packages with > or < to prevent bash redirection | |
| packages = [ | |
| f"'{package}'" if (">" in package or "<" in package) else package | |
| for package in packages | |
| ] | |
| # Format: uv pip install <packages/flags> | |
| deps_line = raw_line[len("uv pip install ") :] | |
| for line in result.stdout.splitlines(): | |
| raw_line = line.strip() | |
| if not raw_line: | |
| continue | |
| # Format: uv pip install <packages/flags> | |
| if not raw_line.startswith("uv pip install "): | |
| continue | |
| deps_line = raw_line[len("uv pip install ") :] |
🤖 Prompt for AI Agents
In `@distribution/build.py` around lines 137 - 144, The loop handling
result.stdout in distribution/build.py assumes every non-empty raw_line starts
with "uv pip install " before slicing into deps_line; guard this by checking
raw_line.startswith("uv pip install ") before doing deps_line = raw_line[len("uv
pip install "):], and skip or log/raise for unexpected lines (e.g., warnings) so
malformed install commands are not produced; adjust the loop around the for line
in result.stdout.splitlines() / raw_line variables accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@distribution/build.py`:
- Around line 159-163: The regex rewriting in deps_line is too broad and
rewrites any dotted package name; restrict it to only match package names
starting with the intended prefix by updating the re.sub call so it looks for a
leading "llama_stack_provider_" package token, capture the provider base and the
extra segment separately, and replace with base[extra] plus the original
comparator; in other words, modify the pattern used in the re.sub on deps_line
to match something like "llama_stack_provider_<name>.<extra><comparator>" and
change the replacement to use the captured groups (base, extra, comparator) so
only llama_stack_provider_* packages get converted to the .extra -> [extra]
form.
♻️ Duplicate comments (3)
distribution/build.py (2)
137-144: Guard against unexpected stdout lines before slicing.If
llama stack list-depsemits warnings, Line 143 will slice non‑install lines into malformed commands. Consider skipping or raising when the expected prefix isn’t present.🔧 Proposed fix
- # Format: uv pip install <packages/flags> - deps_line = raw_line[len("uv pip install ") :] + # Format: uv pip install <packages/flags> + if not raw_line.startswith("uv pip install "): + continue + deps_line = raw_line[len("uv pip install ") :]
145-150: Handle existingpymilvusextras to avoid invalid specifiers.Line 147 will turn
pymilvus[grpc]intopymilvus[milvus-lite][grpc], which is invalid. Please merge extras when already present.💡 Proposed fix
- deps_line = re.sub( - r"\bpymilvus(?!\[milvus-lite\])\b", - "pymilvus[milvus-lite]", - deps_line, - ) + # If extras already exist, append milvus-lite; otherwise add it. + deps_line = re.sub( + r"\bpymilvus\[(?![^\]]*milvus-lite)([^\]]+)\]", + r"pymilvus[\1,milvus-lite]", + deps_line, + ) + deps_line = re.sub( + r"\bpymilvus\b(?!\[)", + "pymilvus[milvus-lite]", + deps_line, + )tests/test_build.py (1)
1-7: Remove shebang or mark executable to satisfy pre-commit.The pre-commit check fails because the file has a shebang but isn’t executable. Remove the shebang or
chmod +xthe file.🧹 Minimal fix (remove shebang)
-#!/usr/bin/env python3
🧹 Nitpick comments (1)
distribution/Containerfile (1)
20-22: Consider de‑duplicating repeated packages in the consolidated install line.Line 20 repeats entries like
pymilvus[milvus-lite]>=2.4.10andaiosqlite, which adds noise and extra work for the resolver. If these are unintended, consider de‑duping at the generator/list‑deps stage so the Containerfile stays minimal.Based on learnings, this file is auto-generated—please adjust the generator rather than editing this file directly. Based on learnings, ...
| deps_line = re.sub( | ||
| r"\.([a-zA-Z_][a-zA-Z0-9_]*)(==|>=|<=|>|<|~=|!=)", | ||
| r"[\1]\2", | ||
| deps_line, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the code in distribution/build.py around lines 159-163
fd -t f "build.py" | xargs -I {} sh -c 'echo "File: {}"; wc -l {}'Repository: opendatahub-io/llama-stack-distribution
Length of output: 185
🏁 Script executed:
# Find and read the specific file
cat -n distribution/build.py | sed -n '150,170p'Repository: opendatahub-io/llama-stack-distribution
Length of output: 1326
🏁 Script executed:
# Get more context around the regex to understand its purpose
cat -n distribution/build.py | sed -n '140,180p'Repository: opendatahub-io/llama-stack-distribution
Length of output: 2242
🏁 Script executed:
# Check if the config file exists
fd -t f "config.yaml" distribution/Repository: opendatahub-io/llama-stack-distribution
Length of output: 107
🏁 Script executed:
# Try to understand what the script does and what distributions are being processed
rg -B5 -A5 "\.extra" distribution/build.py | head -50Repository: opendatahub-io/llama-stack-distribution
Length of output: 787
🏁 Script executed:
# Read the config.yaml to see what packages are processed
cat distribution/config.yaml | head -100Repository: opendatahub-io/llama-stack-distribution
Length of output: 3465
🏁 Script executed:
# Search for any references to dotted packages or legitimate third-party packages
rg "zope\.|\.interface|\.extra" distribution/config.yamlRepository: opendatahub-io/llama-stack-distribution
Length of output: 65
🏁 Script executed:
# Look for all package references to understand the scope
rg -i "==|>=|<=|>|<|~=|!=" distribution/config.yaml | head -30Repository: opendatahub-io/llama-stack-distribution
Length of output: 345
🏁 Script executed:
# Check test files to see if there are test cases that cover this regex behavior
cat -n tests/test_build.pyRepository: opendatahub-io/llama-stack-distribution
Length of output: 3328
Scope the .extra rewrite to avoid mutating dotted package names.
The current regex rewrites any dotted name before a version comparator (e.g., zope.interface==6.4 → zope[interface]==6.4), despite the intent stated in the comments being to handle only llama_stack_provider_* packages. While the current codebase only contains llama_stack_provider_ragas.inline and llama_stack_provider_ragas.remote, scoping the regex to the intended package prefix prevents accidental corruption if other packages are added.
🛠️ Proposed refinement
- deps_line = re.sub(
- r"\.([a-zA-Z_][a-zA-Z0-9_]*)(==|>=|<=|>|<|~=|!=)",
- r"[\1]\2",
- deps_line,
- )
+ deps_line = re.sub(
+ r"\b(llama_stack_provider_[a-zA-Z0-9_]+)\.([a-zA-Z_][a-zA-Z0-9_]*)(==|>=|<=|>|<|~=|!=)",
+ r"\1[\2]\3",
+ deps_line,
+ )🤖 Prompt for AI Agents
In `@distribution/build.py` around lines 159 - 163, The regex rewriting in
deps_line is too broad and rewrites any dotted package name; restrict it to only
match package names starting with the intended prefix by updating the re.sub
call so it looks for a leading "llama_stack_provider_" package token, capture
the provider base and the extra segment separately, and replace with base[extra]
plus the original comparator; in other words, modify the pattern used in the
re.sub on deps_line to match something like
"llama_stack_provider_<name>.<extra><comparator>" and change the replacement to
use the captured groups (base, extra, comparator) so only llama_stack_provider_*
packages get converted to the .extra -> [extra] form.
|
Thanks, I did not know about |
| def test_get_dependencies_transforms_and_groups(self) -> None: | ||
| fake_stdout = "\n".join( | ||
| [ | ||
| "uv pip install pillow 'fastapi==1.0' aiosqlite 'pymilvus>=2.4.10'", |
There was a problem hiding this comment.
I'm not a fan of managing dependencies inside .py files.
@gyliu513 It might be cleaner to handle this through standard configuration files like pyproject.toml or requirements.txt.
|
This pull request has been automatically marked as stale because it has not had activity within 60 days. It will be automatically closed if no further activity occurs within 30 days. |
|
This pull request has merge conflicts that must be resolved before it can be merged. @gyliu513 please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
3 participants
What does this PR do?
@leseb can you help review if this align with your thinking? Thanks!
Test Plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.