Fix confusing error UX when model serve has missing deps#965
Fix confusing error UX when model serve has missing deps#965alfredoclarifai wants to merge 1 commit intocli-improvementfrom
Conversation
- Fix NoneType crash in check_requirements_installed when model_path is None - Show package names in missing deps list (was only showing version specifiers) - Show relative path in pip install hint instead of absolute - Replace UserError traceback with clean click.Abort() since error details are already logged - Add tip about --mode env/container alternatives - Print playground/API URLs via click.echo for cleaner terminal output Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Improves the clarifai model serve missing-dependency user experience by making dependency checks more informative (showing package names, better install hints) and by reducing traceback-heavy failures during local serving.
Changes:
- Enhance missing-deps reporting in
check_requirements_installed()(include package names; show relativerequirements.txtpath when possible; add mode tips). - Pass
model_pathintocheck_requirements_installed()from model serving flows and switch one failure path toclick.Abort(). - Add additional CLI output logic intended to print Playground/API URLs via
click.echo().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
clarifai/utils/cli.py |
Improves missing dependency list formatting and install guidance output. |
clarifai/cli/model.py |
Updates local serve flows to use improved requirements checking and adjusts abort/error handling and output. |
| if not dependencies: | ||
| dependencies = parse_requirements(model_path) | ||
| missing = [ | ||
| full_req | ||
| f"{package_name}{full_req}" if full_req else package_name | ||
| for package_name, full_req in dependencies.items() | ||
| if not _is_package_installed(package_name) | ||
| ] |
There was a problem hiding this comment.
check_requirements_installed() assumes dependencies is a dict and unconditionally iterates dependencies.items(), but parse_requirements() returns [] when requirements.txt is missing. In that case this code raises (list has no items) and falls into the generic exception path, turning a missing requirements.txt into a failed requirements check. Consider returning {} from parse_requirements() (or handling non-dict/empty dependencies here) so “no requirements.txt” is treated predictably (e.g., as no deps) and produces a clean message.
| if mode not in ("container", "env"): | ||
| dependencies = parse_requirements(model_path) | ||
| if not check_requirements_installed(dependencies=dependencies): | ||
| if not check_requirements_installed(model_path=model_path, dependencies=dependencies): |
There was a problem hiding this comment.
This call passes both model_path and dependencies. In check_requirements_installed() that combination triggers a warning and re-parses requirements from disk, making the pre-parsed dependencies redundant and adding noisy output. Pass only model_path (letting the helper parse) or adjust check_requirements_installed() to accept both without warning/extra parsing.
| if not check_requirements_installed(model_path=model_path, dependencies=dependencies): | |
| if not check_requirements_installed(model_path=model_path): |
| if mode not in ("container", "env"): | ||
| if not check_requirements_installed(dependencies=dependencies): | ||
| raise UserError(f"Requirements not installed for model at {model_path}.") | ||
| if not check_requirements_installed(model_path=model_path, dependencies=dependencies): |
There was a problem hiding this comment.
Same issue here: passing both model_path and dependencies will cause check_requirements_installed() to warn and re-parse requirements, which undermines the “clean error UX” goal. Prefer calling it with only model_path (or update the helper’s argument-handling logic).
| if not check_requirements_installed(model_path=model_path, dependencies=dependencies): | |
| if not check_requirements_installed(model_path=model_path): |
| def print_code_snippet(): | ||
| if ctx.obj.current is None: | ||
| logger.debug("Context is None. Skipping code snippet generation.") | ||
| else: | ||
| from clarifai.runners.utils import code_script | ||
|
|
||
| snippet = code_script.generate_client_script( |
There was a problem hiding this comment.
print_code_snippet() is defined but never called, so the new click.echo() URL output won’t actually take effect and the extra code becomes dead/maintenance burden. Either call this function (and ensure it uses the local user_id/app_id/model_id/version_id values rather than ctx.obj.current.model_id/model_version_id, which aren’t set here) or remove it and keep the existing snippet/Next Steps output below.
|
@alfredoclarifai I have included these improvements in my branch, so closing this PR |
Summary
NoneTypecrash incheck_requirements_installedwhenmodel_pathisNone— was causing a traceback instead of a clean error message>=2.6.0)pip install -rhint instead of absoluteUserErrortraceback with cleanclick.Abort()since error details are already logged--mode env/--mode containeralternativesclick.echo()for cleaner terminal outputTest plan
clarifai model serve .in a directory with missing deps and verify clean error output (no traceback)pip install -rshows relative pathclarifai model serve .with all deps installed and verify playground URL prints cleanly🤖 Generated with Claude Code