feat: allow images as urls for supported backends#1260
Conversation
planetf1
left a comment
There was a problem hiding this comment.
One test regression and a docstring gap to fix before merge, plus a couple of things worth flagging.
Test assertion needs updating (test/backends/test_vision_openai.py:163–164): The ollama version (test_vision_ollama.py:148–150) was updated to pull out first_image and compare .value, but the OpenAI test still compares last_action.images[0] directly against image_block.value. Since images now returns block objects, this is comparing an ImageBlock to a str and will fail when the e2e test runs:
# first image in image list should be the same as the image block
first_image = last_action.images[0] # type: ignore
assert isinstance(first_image, ImageBlock)
assert first_image.value == ImageBlock.from_pil_image(pil_image).valueRaises: entry missing (mellea/backends/ollama.py:366–367): generate_from_chat_context now raises ValueError for ImageUrlBlock, but the docstring only lists RuntimeError. Per AGENTS.md (Section 10, item 6) every raise in a public function wants a matching entry — the quality gate won’t catch this one since RuntimeError is already listed:
Raises:
RuntimeError: If not called from a thread with a running event loop.
ValueError: If a message contains an ``ImageUrlBlock``; Ollama requires
base64-encoded images — convert to an ``ImageBlock`` first.Breaking change: Message.images now returns list[ImageBlock | ImageUrlBlock] rather than list[str]. Right call, but worth a line in the PR description and a changelog entry so people know to expect it on upgrade.
Missing tests: Three new paths that’d be worth covering before this merges, since they’re the heart of the feature:
get_image_blocks()with anhttps://URL returning anImageUrlBlock— the http/https branch atcli/serve/models.py:89–90isn’t exercised yet (the existing pydantic-parse test only checks the model deserialises)- Ollama rejecting an
ImageUrlBlockwithValueError— the guard atollama.py:401–402has no test message_to_openai_messagepassing a URL straight through as{"image_url": {"url": ...}}instead of wrapping it in a data URI (openai_compatible_helpers.py:200–206) — nothing hits that branch currently
|
Addressed feedback, will rebase / fix conflicts as prior to re-review. |
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
jakelorocco
left a comment
There was a problem hiding this comment.
lgtm; a few nits and some open questions I have about the code. I think it's good to merge even absent the changes, but I disabled auto-merge just so you could at least see these comments before it gets merged. Feel free to re-enable / merge. Thank you!
| associated with the message. Use `ImageBlock` for base64-encoded | ||
| images (supported by all vision backends) or `ImageUrlBlock` for | ||
| URL-referenced images (supported by OpenAI-compatible backends only; | ||
| backends that require base64 will raise a ``ValueError``). |
There was a problem hiding this comment.
I believe we are using single- for things. I see a few places in this PR that have the double- syntax.
There was a problem hiding this comment.
Indeed we are - I'll add this to the follow up as well.
| if isinstance(img, ImageUrlBlock): | ||
| raise ValueError( | ||
| "OllamaModelBackend does not support URL images (ImageUrlBlock). " | ||
| "Convert the image to a base64-encoded ImageBlock before passing it to Ollama." | ||
| ) |
There was a problem hiding this comment.
Is there a reason not to autoconvert? I assume we could attempt to download the reference image and then base64 encode it.
There was a problem hiding this comment.
See below, will open a follow up 👍
| class ImageUrlBlock(CBlock): | ||
| """An `ImageUrlBlock` represents an image as a URL. | ||
|
|
||
| Use this when the image is hosted remotely and you want to pass the URL | ||
| directly to backends that support it (e.g. OpenAI). Backends that only | ||
| accept base64-encoded images (e.g. Ollama) will raise a ``ValueError`` | ||
| rather than silently drop the image. |
There was a problem hiding this comment.
I think this is a valid way to encode the difference between ImageBlocks and ImageUrlBlocks. Is there a reason to prefer this multi-class approach to having a flag / field within the ImageBlock class?
I'm wondering if it's more confusing for users to have to know when to use either class. It might be helpful to add a helper function that can take all types of images and spit out the proper class (maybe even with a convert_to_base64 flag for url versions, which would address my other comment about auto-conversion). This could also be a separate issue / task.
Would like to know what you think since you've put more thought / time into this aspect of the code base!
There was a problem hiding this comment.
The original decision behind the multi class approach was network fetch vs no network fetch at construction time.
Having auto conversion with something like this is maybe the best approach?
image_block_from(source: str | PIL.Image.Image, *, fetch_url: bool = False) -> ImageBlock | ImageUrlBlock:
I'll open a follow up issue - this would take care of the autoconvert comment as well.
|
Follow up here #1312 |
f024797
|
Thanks for this — the design is clean, the Ollama guard failing loud rather than silently is exactly right, and the test coverage is solid. A few minor observations while reading through: Compatibility note — Small things, take or leave:
|
Pull Request
Issue
Fixes #127
Description
allows images as urls for supported backends
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.