fix: improve missing package error messages by ktdreyer · Pull Request #149 · opendatahub-io/llama-stack-distribution

ktdreyer · 2025-12-05T22:09:24Z

Summary

Improve error messages when required commands are not found by providing specific installation instructions.

Changes

Refactor check_package_installed() to accept optional package name parameter
Display uv pip install command when llama is missing

Test plan

Verify error messages display correct installation instructions when commands are missing
Confirm existing functionality works when commands are present

Summary by CodeRabbit

Bug Fixes
- Clearer, more actionable error messages when required build commands are missing, with optional package hints.
- Reduced redundant or confusing installation prompts during pre-build checks.
Refactor
- Unified command/installation validation into a single, consistent flow for simpler, more reliable diagnostics and output.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-05T22:09:35Z

Walkthrough

Replaced check_package_installed with check_command_installed(command, package_name=None) in distribution/build.py. Updated call sites to check commands ("uv", "llama") and changed error messaging to optionally include a package hint; removed the old function and its direct prints.

Changes

Cohort / File(s)	Summary
Command checker refactor `distribution/build.py`	Removed `check_package_installed(package_name)` and added `check_command_installed(command, package_name=None)`. Call sites updated to `check_command_installed("uv")` and `check_command_installed("llama", "llama-stack-client")`. Consolidated error messaging and unified exit-on-failure behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Verify PATH-based command detection and that stderr/stdout behavior is appropriate in check_command_installed.
Confirm the package_name hint is used only for message text and does not affect detection logic.
Search repository for any remaining check_package_installed references.

Poem

🐰 I hopped through code with a nimble mind,
Swapped checks for commands, left prints behind.
A hint in the message, a tidy small tweak,
Quietly cleaner — a rabbit's soft peek.
🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'fix: improve missing package error messages' directly aligns with the main objective: improving error messages when required commands are not found by providing specific installation instructions.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8c954cb and 6638a7d.

📒 Files selected for processing (1)

distribution/build.py (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

distribution/build.py

⏰ 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). (2)

GitHub Check: build-test-push (linux/amd64)
GitHub Check: Summary

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

distribution/build.py (1)

51-59: Docstring lacks parameter documentation.

The function parameters command and package_name should be documented in the docstring to clarify their purpose and usage:

def check_command_installed(command, package_name=None):
-    """Check if a command is installed and accessible.
-    """
+    """Check if a command is installed and accessible.
+    
+    Args:
+        command: The command name to check for availability.
+        package_name: Optional package name to display in error message for installation guidance.
+    """

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f82fc15 and 9b0b979.

📒 Files selected for processing (1)

distribution/build.py (2 hunks)

🧰 Additional context used

🪛 GitHub Actions: Pre-commit

distribution/build.py

[error] 1-1: pre-commit hook 'ruff-format' failed. 1 file reformatted; exit code 1 during 'pre-commit run --show-diff-on-failure --color=always --all-files'.

⏰ 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). (2)

GitHub Check: build-test-push (linux/amd64)
GitHub Check: Summary

🔇 Additional comments (2)

distribution/build.py (2)
272-272: LGTM!

The error message for the missing llama command now provides specific installation instructions pointing to the correct package (llama-stack-client). This aligns well with the PR's objective.

269-269: Provide specific installation instructions for uv.

The call to check_command_installed("uv") doesn't provide a package name, resulting in a generic error message. This doesn't align with the PR's goal of providing specific installation instructions.

Consider providing the package name:
-    check_command_installed("uv")
+    check_command_installed("uv", "uv")
However, note that installing uv via uv pip may be circular. You may want to either:

Keep the generic message for uv since it's a bootstrapping tool

Provide a different installation method in the error message (e.g., "Please install uv from https://...")

nathan-weinberg

pre-commit needs fixing but LGTM!

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

distribution/build.py (1)

51-59: New check_command_installed helper matches requirements; consider minor ergonomics.

The helper cleanly generalizes the old check and supports the package_name hint; this will give a nice uv pip install llama-stack-client message for missing llama, which aligns with the PR goal. The control flow and use of shutil.which/sys.exit(1) are fine for this script-style module.

If you want to polish further (optional):

Send error messages to stderr (print(..., file=sys.stderr)) so tooling can separate errors from normal output.

Slightly expand the generic message to hint how to install uv (e.g., link to docs or pipx), but that’s more of a product decision than a code issue.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9b0b979 and 0fc9be8.

📒 Files selected for processing (1)

distribution/build.py (2 hunks)

🧰 Additional context used

🪛 GitHub Actions: Pre-commit

distribution/build.py

[error] 1-1: ruff-format failed during pre-commit hook: 1 file reformatted; changes committed by hook. This caused the CI step to fail (pre-commit run --show-diff-on-failure --color=always --all-files).

⏰ 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). (2)

GitHub Check: build-test-push (linux/amd64)
GitHub Check: Summary

🔇 Additional comments (2)

distribution/build.py (2)
268-273: Call-site updates correctly gate later operations on required commands.

The new main() checks:

check_command_installed("uv") before any uv pip install usage.

check_command_installed("llama", "llama-stack-client") before invoking llama stack ....

This ensures clearer, actionable failures when prerequisites are missing and satisfies the PR objective without changing behavior when the commands are present.

1-291: Run pre-commit hooks locally to fix ruff-format issues.

The CI pipeline indicates ruff-format has identified formatting issues in this file. Run the following locally and commit any changes:
pre-commit run --all-files
git status
Push the updated commit to resolve the pipeline failure.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

distribution/build.py (1)

51-60: Command-check helper correctly encapsulates PATH check and error messaging; only minor polish possible

The function cleanly wraps shutil.which and exits with a clear message, and the optional package_name hook satisfies the PR goal of giving a uv pip install ... hint for specific tools. Behavior matches the old helper’s semantics while improving clarity.

If you expect future callers that might not use uv, you could later generalize package_name into a more generic install_hint or similar, but that’s purely optional and not required for this PR.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6fcdf0b and 35f74f7.

📒 Files selected for processing (1)

distribution/build.py (2 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). (2)

GitHub Check: build-test-push (linux/amd64)
GitHub Check: Summary

🔇 Additional comments (1)

distribution/build.py (1)

269-274: Updated call sites align with new helper and PR intent

Using check_command_installed("uv") before invoking any uv commands and check_command_installed("llama", "llama-stack-client") before running llama stack ... ensures failures surface early with actionable guidance (uv pip install llama-stack-client). This directly achieves the stated objective of clearer missing-command errors without changing control flow.

nathan-weinberg · 2025-12-17T02:28:49Z

@Artemon-line PTAL at the CI failure - it is related to the Vertex AI auth

Artemon-line · 2025-12-17T10:09:11Z

@Artemon-line PTAL at the CI failure - it is related to the Vertex AI auth

I know pls see the error message: "By default, secrets are not passed to workflows triggered from forks, including Dependabot." -- shortly you cannot access those secrets from your fork, for obvious reasons.

skamenan7

LGTM, thanks

Provide clear installation instructions when commands are missing rather than just stating they are not found.

ktdreyer changed the title ~~Improve missing package error messages~~ fix: improve missing package error messages Dec 5, 2025

coderabbitai bot reviewed Dec 5, 2025

View reviewed changes

nathan-weinberg approved these changes Dec 8, 2025

View reviewed changes

ktdreyer force-pushed the command-not-found-error branch 2 times, most recently from f18c18c to 0fc9be8 Compare December 8, 2025 17:48

coderabbitai bot reviewed Dec 8, 2025

View reviewed changes

ktdreyer force-pushed the command-not-found-error branch 2 times, most recently from 6fcdf0b to 35f74f7 Compare December 8, 2025 20:34

coderabbitai bot reviewed Dec 8, 2025

View reviewed changes

nathan-weinberg requested a review from a team December 9, 2025 14:38

ktdreyer force-pushed the command-not-found-error branch from 35f74f7 to dfff56d Compare December 9, 2025 19:27

rhdedgar approved these changes Dec 16, 2025

View reviewed changes

nathan-weinberg force-pushed the command-not-found-error branch from dfff56d to 8c954cb Compare December 17, 2025 02:25

skamenan7 approved these changes Dec 17, 2025

View reviewed changes

fix: improve missing package error messages

6638a7d

Provide clear installation instructions when commands are missing rather than just stating they are not found.

nathan-weinberg force-pushed the command-not-found-error branch from 8c954cb to 6638a7d Compare December 17, 2025 19:25

mergify bot merged commit b803a35 into opendatahub-io:main Dec 17, 2025
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improve missing package error messages#149

fix: improve missing package error messages#149
mergify[bot] merged 1 commit intoopendatahub-io:mainfrom
ktdreyer:command-not-found-error

ktdreyer commented Dec 5, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 5, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

nathan-weinberg left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

nathan-weinberg commented Dec 17, 2025

Uh oh!

Artemon-line commented Dec 17, 2025

Uh oh!

skamenan7 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

ktdreyer commented Dec 5, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

nathan-weinberg left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

nathan-weinberg commented Dec 17, 2025

Uh oh!

Artemon-line commented Dec 17, 2025

Uh oh!

skamenan7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ktdreyer commented Dec 5, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 5, 2025 •

edited

Loading