Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions vlmrun/client/predictions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Contributor

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 of str | None violating AGENTS.md modern Python style rule

The AGENTS.md explicitly mandates: "Use X | None instead of Optional[X] for type hints (PEP 604)" and "Use | for union types in all new code." The newly added _warn_callback_requires_batch function uses Optional[str] for its callback_url parameter type hint, which violates this rule. Since the file already has from __future__ import annotations (line 3), using str | None is fully supported.

Suggested change
callback_url: Optional[str], batch: bool, is_image: bool = False
callback_url: str | None, batch: bool, is_image: bool = False
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

) -> 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."""

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Duplicate image processing in ImagePredictions.generate persists

The ImagePredictions.generate method (lines 325-354) has a pre-existing issue where images are processed inline (opened from disk at line 336, encoded at line 341) and then self._handle_images_or_urls(images, urls) is called again at line 354, which re-encodes the already-opened PIL images. The result from the inline processing (line 341's images_data) is overwritten. This means images get encoded to JPEG twice — wasteful but functionally correct since the second encoding produces the same result. This PR doesn't introduce or worsen this issue, but it's worth noting as technical debt.

(Refers to lines 324-354)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Expand Down Expand Up @@ -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 = {}
Expand Down Expand Up @@ -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 = {}
Expand Down
Loading