-
Notifications
You must be signed in to change notification settings - Fork 4
feat(predictions): add warnings for callback_url and batch usage in image predictions #193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| from __future__ import annotations | ||
| import json | ||
| import warnings | ||
| from pathlib import Path | ||
| from typing import List, Optional, Union | ||
| from PIL import Image | ||
|
|
@@ -42,6 +43,37 @@ def get_response_model( | |
| return schema_response.response_model | ||
|
|
||
|
|
||
| def _warn_callback_requires_batch( | ||
| callback_url: Optional[str], batch: bool, is_image: bool = False | ||
| ) -> None: | ||
| if is_image: | ||
| if batch: | ||
| warnings.warn( | ||
| "`batch=True` is not supported for image predictions. " | ||
| "Image predictions are processed synchronously; the `batch` flag will be ignored by the server.", | ||
| UserWarning, | ||
| stacklevel=3, | ||
| ) | ||
| if callback_url: | ||
| warnings.warn( | ||
| "`callback_url` is not supported for image predictions. " | ||
| "Image predictions are synchronous and return the result directly; " | ||
| "the callback URL will be ignored.", | ||
| UserWarning, | ||
| stacklevel=3, | ||
| ) | ||
| return | ||
|
|
||
| if callback_url and not batch: | ||
| warnings.warn( | ||
| "`callback_url` will be ignored because `batch=False`. " | ||
| "Webhook callbacks are only delivered for batched (async) predictions. " | ||
| "Set `batch=True` to receive callbacks.", | ||
| UserWarning, | ||
| stacklevel=3, | ||
| ) | ||
|
|
||
|
|
||
| class SchemaCastMixin: | ||
| """Mixin class to handle schema casting for predictions.""" | ||
|
|
||
|
|
@@ -230,6 +262,7 @@ def execute( | |
| Raises: | ||
| ValueError: If neither images nor urls are provided, or if both are provided | ||
| """ | ||
| _warn_callback_requires_batch(callback_url, batch, is_image=True) | ||
| images_data = self._handle_images_or_urls(images, urls) | ||
| additional_kwargs = {} | ||
| if config: | ||
|
|
@@ -287,6 +320,7 @@ def generate( | |
| Raises: | ||
| ValueError: If neither images nor urls are provided, or if both are provided | ||
| """ | ||
| _warn_callback_requires_batch(callback_url, batch, is_image=True) | ||
| # Input validation | ||
| if not images and not urls: | ||
| raise ValueError("Either `images` or `urls` must be provided") | ||
|
Comment on lines
324
to
326
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 Duplicate image processing in ImagePredictions.generate persists The (Refers to lines 324-354) Was this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
@@ -448,6 +482,7 @@ def generate( | |
| Returns: | ||
| PredictionResponse: Prediction response | ||
| """ | ||
| _warn_callback_requires_batch(callback_url, batch) | ||
| is_url, file_or_url = self._handle_file_or_url(file, url) | ||
|
|
||
| additional_kwargs = {} | ||
|
|
@@ -517,6 +552,7 @@ def execute( | |
| Returns: | ||
| PredictionResponse: Prediction response | ||
| """ | ||
| _warn_callback_requires_batch(callback_url, batch) | ||
| is_url, file_or_url = self._handle_file_or_url(file, url) | ||
|
|
||
| additional_kwargs = {} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 New code uses
Optional[str]instead ofstr | Noneviolating AGENTS.md modern Python style ruleThe AGENTS.md explicitly mandates: "Use
X | Noneinstead ofOptional[X]for type hints (PEP 604)" and "Use|for union types in all new code." The newly added_warn_callback_requires_batchfunction usesOptional[str]for itscallback_urlparameter type hint, which violates this rule. Since the file already hasfrom __future__ import annotations(line 3), usingstr | Noneis fully supported.Was this helpful? React with 👍 or 👎 to provide feedback.