Merge pull request #24 from darth-veitcher/claude/chatcompletion-image-input-7mm6q5
docs(spec): draft VLM image input for ChatCompletion (009)
This commit is contained in:
James Veitch
@@ -1,3 +1,3 @@
|
||||
{
|
||||
"feature_directory": "specs/008-llamacpp-integration"
|
||||
"feature_directory": "specs/009-vlm-image-input"
|
||||
}
|
||||
|
||||
@@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
|
||||
- `CHANGELOG.md` (this file)
|
||||
- README: What-is-this, Install, and Quickstart sections
|
||||
- `LLMProvider` protocol (`comfydv._llm`) — a shared adapter boundary so ComfyUI LLM nodes work with any backend that implements it, starting with `OllamaProvider`. Structured output now goes through `pydantic-ai` (ADR-007), superseding the hand-rolled Ollama tool-calling approach.
|
||||
- **Chat Completion** now accepts an optional `image` input for vision-capable models (VLMs): wire a ComfyUI `IMAGE` and the connected model can describe or reason about it. Works identically on both backends (Ollama multimodal models; llama.cpp launched with `--mmproj`), and composes with structured output and multi-turn history. Images are carried on `Message.images` and translated to each backend's native shape (Ollama's flat `images` array, llama.cpp's OpenAI `image_url` parts, pydantic-ai `BinaryContent` on the structured path) — ADR-008, extending ADR-007's adapter pattern to a second input modality. Text-only workflows are unchanged when no image is wired.
|
||||
|
||||
### Changed
|
||||
- **Breaking:** `OllamaChatCompletion` → `ChatCompletion`, `OllamaModelSelector` → `LLMModelSelector`, `OllamaLoadModel` → `LLMLoadModel`, `OllamaUnloadModel` → `LLMUnloadModel`, and the `OLLAMA_CLIENT` socket type → `LLM_CLIENT` — these nodes are now backend-generic. `OllamaClient` is unchanged by name but now outputs an `OllamaProvider` rather than a plain string; existing saved workflows using the old node/socket names need reconnecting (see `comfydv.ollama.MIGRATION_MAP` for the full old→new mapping).
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
<!-- SPECKIT START -->
|
||||
For additional context about technologies to be used, project structure,
|
||||
shell commands, and other important information, read the current plan
|
||||
at specs/008-llamacpp-integration/plan.md
|
||||
at specs/009-vlm-image-input/plan.md
|
||||
<!-- SPECKIT END -->
|
||||
|
||||
@@ -114,6 +114,15 @@ A complete graph looks like this:
|
||||
|
||||
|
||||
|
||||
### Describing images (vision)
|
||||
|
||||
**Chat Completion** has an optional **image** input. Wire any `IMAGE` into it and, with a vision-capable model loaded, the model can describe or reason about the picture — captioning, visual Q&A, reading text in an image, whatever the model supports.
|
||||
|
||||
- **Ollama** — use a multimodal model (e.g. a llava-class model).
|
||||
- **llama.cpp** — launch `llama-server` with a multimodal projector: `--mmproj <projector.gguf>` alongside the model.
|
||||
|
||||
Image input works the same on both backends — same node, same wiring — and composes with everything else: structured output (schema-validated fields pulled straight from the image), multi-turn history, and the option nodes. A batch of images is sent as multiple images on the turn. Leave the input unwired and Chat Completion behaves exactly as before, text only.
|
||||
|
||||
### Manual memory management
|
||||
|
||||
Single-GPU and memory-constrained setups need explicit control over what's resident in VRAM. **LLM Load Model** pins a model into memory; **LLM Unload Model** evicts it immediately, freeing room for the next model or the rest of your image pipeline.
|
||||
|
||||
@@ -5,3 +5,9 @@
|
||||
# the bullet survives a fresh clone (`beacon doctor`'s active-bullet check then
|
||||
# works in CI) and no per-branch file is left behind on the trunk after a merge.
|
||||
# `git log -p` on this file is the audit trail of who started which bullet when.
|
||||
|
||||
[bullets."claude/chatcompletion-image-input-7mm6q5"]
|
||||
title = "VLM image input for ChatCompletion"
|
||||
owner = "noreply@anthropic.com"
|
||||
started = "2026-07-22T19:11:08+00:00"
|
||||
epic = "vlm-image-input"
|
||||
|
||||
+158
@@ -0,0 +1,158 @@
|
||||
# ADR-008: Multimodal image input carried on the Message across the LLMProvider boundary
|
||||
|
||||
## Status
|
||||
|
||||
> Proposed
|
||||
|
||||
_Date:_ 2026-07-22
|
||||
_Deciders:_ darth-veitcher
|
||||
|
||||
---
|
||||
|
||||
## Context
|
||||
|
||||
The `ChatCompletion` node and the `LLMProvider` protocol (ADR-007) are
|
||||
text-only today. `Message` (`src/comfydv/_llm/provider.py`) carries a single
|
||||
`content: str`; `ChatCompletion`'s `INPUT_TYPES` (`src/comfydv/ollama.py`)
|
||||
exposes no `IMAGE` socket. Users want to couple the existing chat node with a
|
||||
vision-capable model (a VLM) to describe or reason about an image produced
|
||||
elsewhere in a ComfyUI workflow.
|
||||
|
||||
ADR-007 deliberately scoped this out: it defined `Message` as text-only and
|
||||
recorded that "if llama.cpp's router mode needs a protocol capability that
|
||||
doesn't exist yet, that is a protocol change scoped as its own follow-up, not
|
||||
silently special-cased." This ADR is that follow-up — it extends the same
|
||||
adapter pattern to a second input modality.
|
||||
|
||||
The two shipped backends carry images very differently on the wire, and the
|
||||
node has two distinct code paths (free-text vs structured), so a decision is
|
||||
needed about **where** an image lives as it crosses the provider boundary and
|
||||
**who** translates it into each backend's native shape:
|
||||
|
||||
- **Ollama free-text** — `OllamaProvider.chat()` posts `[m.model_dump() for m
|
||||
in messages]` to the native `/api/chat`, which accepts a per-message
|
||||
`images` field: an array of base64-encoded image data alongside the text
|
||||
`content`.
|
||||
- **llama.cpp free-text** — `LlamaCppProvider.chat()` posts to the
|
||||
OpenAI-compatible `/v1/chat/completions`, where a message's `content` is a
|
||||
list of typed parts (`{"type": "text", ...}`,
|
||||
`{"type": "image_url", "image_url": {"url": "data:image/...;base64,..."}}`)
|
||||
— a flat sibling `images` field is not understood.
|
||||
- **Structured output (both backends)** — routed through the shared
|
||||
`chat_structured()` helper (`src/comfydv/_llm/chat.py`) over pydantic-ai,
|
||||
which represents images as typed multimodal content
|
||||
(`BinaryContent` / `ImageUrl`) inside the user prompt, not as a raw request
|
||||
field.
|
||||
|
||||
The competing concern is DRY vs. leakage: a single carrier keeps the graph and
|
||||
the node backend-agnostic (ADR-007's whole point), but the per-backend wire
|
||||
shapes are irreducibly different and must be translated somewhere.
|
||||
|
||||
## Decision
|
||||
|
||||
**Carry images as an optional field on `Message`, and make each provider
|
||||
responsible for translating that field into its own native wire shape** — the
|
||||
exact same division of responsibility ADR-007 established for text and model
|
||||
management (operation-level protocol, wire-format quirks contained inside each
|
||||
provider).
|
||||
|
||||
1. **Protocol** — extend `Message` with an optional
|
||||
`images: list[str] | None = None`, where each entry is a base64-encoded
|
||||
image. `content` stays required; a text-only message sets `images=None` and
|
||||
is byte-for-byte unchanged from today (`model_dump()` omits it or emits
|
||||
`null`), so all existing Ollama/llama.cpp behavior is preserved.
|
||||
|
||||
2. **Node** — `ChatCompletion` gains one **optional** `image: ("IMAGE",)`
|
||||
input. When wired, the node encodes the ComfyUI `IMAGE` tensor to base64
|
||||
and attaches it to the user `Message` it already constructs. When not
|
||||
wired, the node builds exactly the message it builds today. The node never
|
||||
branches on which concrete provider it holds — consistent with ADR-007.
|
||||
|
||||
3. **Per-provider translation** (the leakage lives here, deliberately):
|
||||
- `OllamaProvider.chat()` — the flat `images` field on the dumped message
|
||||
already matches Ollama's native `/api/chat` schema; it flows through with
|
||||
no transform.
|
||||
- `LlamaCppProvider.chat()` — maps a message's `images` into OpenAI-style
|
||||
`image_url` content parts before POSTing to `/v1/chat/completions`.
|
||||
- `chat_structured()` (shared) — maps the last user message's `images` into
|
||||
pydantic-ai multimodal content on the `user_prompt`; both backends inherit
|
||||
this single implementation, mirroring how they already share the
|
||||
structured text path.
|
||||
|
||||
4. **No new node classes and no new socket types** — image support is an
|
||||
additional optional input on the *existing* generic node, so a workflow
|
||||
author gains vision by wiring one socket, not by learning a new node. This
|
||||
is the direct extension of ADR-007's "generic, not per-backend" node stance.
|
||||
|
||||
The base64 string is the neutral interchange form at the boundary because it
|
||||
is the one representation every target consumes (Ollama's `images` array,
|
||||
OpenAI's `data:` URI, and pydantic-ai's `BinaryContent` all accept it),
|
||||
keeping the `Message` carrier itself provider-agnostic.
|
||||
|
||||
_Wire specifics (exact Ollama `/api/chat` image field, llama.cpp multimodal
|
||||
readiness via `mmproj`, and pydantic-ai's multimodal content type) are
|
||||
verified live in this feature's `research.md`/`plan.md` per project
|
||||
convention, not assumed from training data._
|
||||
|
||||
## Consequences
|
||||
|
||||
**Easier:**
|
||||
- One carrier (`Message.images`) and one node change unlock vision on both
|
||||
backends at once; a future third provider implements image translation in
|
||||
its own `chat()` exactly as it implements text, with no protocol churn.
|
||||
- The graph and the node stay backend-agnostic — swapping providers still
|
||||
means rewiring one client node, now including the image path.
|
||||
- Text-only workflows are entirely unaffected (additive optional field +
|
||||
optional socket).
|
||||
|
||||
**Harder / constrained:**
|
||||
- `Message` is no longer a trivially-uniform text struct; each provider's
|
||||
`chat()` (and the shared structured helper) must handle the `images` field,
|
||||
even if only to pass it through. This is accepted leakage, localized to the
|
||||
provider layer — the same tradeoff ADR-007 already made for `keep_alive` vs
|
||||
explicit load/unload.
|
||||
- Vision requires a model actually loaded with multimodal weights (Ollama
|
||||
multimodal models; llama.cpp launched with an `mmproj` projector). A
|
||||
text-only model receiving images degrades to a backend error, not a node
|
||||
crash — surfacing that clearly is a spec requirement, not something this
|
||||
boundary can prevent.
|
||||
|
||||
**Debt introduced:**
|
||||
- None deliberately, contingent on text-only requests remaining byte-identical
|
||||
to today (guarded by the existing Ollama/llama.cpp provider tests, which must
|
||||
stay green).
|
||||
|
||||
## Considered Alternatives
|
||||
|
||||
### Alternative A: A separate `images` parameter threaded through `chat()`/`chat_structured()` signatures
|
||||
|
||||
**Why rejected:** Widens every provider method signature and the protocol for
|
||||
a value that is conceptually part of a message turn. Images belong to a
|
||||
specific message (which turn the picture accompanies), and multi-turn vision
|
||||
histories need per-message association — a single side-channel parameter can't
|
||||
express that. Putting it on `Message` keeps turn/image association intact and
|
||||
leaves method signatures unchanged.
|
||||
|
||||
### Alternative B: A dedicated multimodal node / socket type separate from `ChatCompletion`
|
||||
|
||||
**Why rejected:** Reintroduces exactly the per-capability node proliferation
|
||||
ADR-007 eliminated. A workflow author would maintain two chat nodes and
|
||||
relearn one for vision. An optional input on the existing node is strictly
|
||||
simpler and keeps the "one generic node set" promise.
|
||||
|
||||
### Alternative C: Normalize images to OpenAI content-parts at the boundary; make Ollama un-translate
|
||||
|
||||
**Why rejected:** Picks OpenAI's shape as the canonical form and forces the
|
||||
Ollama provider — whose native API wants the simpler flat `images` array — to
|
||||
convert *away* from it. That inverts the "each provider owns its own wire
|
||||
format" principle and does more work on the currently-simpler path. A neutral
|
||||
base64 carrier that every backend adapts *from* is the orthogonal choice.
|
||||
|
||||
---
|
||||
|
||||
## Links
|
||||
|
||||
- Related epic: `project-management/Roadmap/epics/vlm-image-input.md`
|
||||
- Related spec: `specs/009-vlm-image-input/`
|
||||
- Related ADRs: [ADR-007](ADR-007-llm-provider-adapter-pattern.md) (extended — same adapter pattern, second input modality), [ADR-005](ADR-005-ollama-host-config-via-client-node.md) (client-node pattern, unchanged)
|
||||
- External reference: GitHub issue #15 (llama.cpp parity), Ollama multimodal `/api/chat` `images`, OpenAI vision `image_url` content parts
|
||||
@@ -31,3 +31,4 @@ Superseded ADRs keep their file; update their status to `Superseded by ADR-###`.
|
||||
| ADR | Title | Status | Date |
|
||||
|-----|-------|--------|------|
|
||||
| [ADR-000](ADR-000-template.md) | Template | — | — |
|
||||
| [ADR-008](ADR-008-multimodal-image-input-across-llmprovider-boundary.md) | Multimodal image input across the LLMProvider boundary | Proposed | 2026-07-22 |
|
||||
|
||||
@@ -28,6 +28,7 @@
|
||||
- **ComfyUI UX Polish & Manager Compatibility** — `epics/ux-and-install.md` — 🔄 ACTIVE — Fix installation, core UX bugs (debounce, connection drops, alert dialogs), correctness bugs (class-level mutation, IS_CHANGED, seed=0), and metadata drift
|
||||
- **LLM Provider Abstraction** — `epics/archive/llm-provider-abstraction.md` — ✅ DONE — shared `LLMProvider` protocol (list/load/unload/chat/structured-output) and generic ComfyUI nodes; Ollama integration migrated onto it (ADR-007, supersedes ADR-006); merged via PR #17
|
||||
- **llama.cpp Model Integration** — `epics/llamacpp-integration.md` — 🔄 ACTIVE — Add a `LlamaCppProvider` implementing the shared protocol via llama-server's router mode (GitHub issue #15); dependency on LLM Provider Abstraction now satisfied
|
||||
- **VLM Image Input for ChatCompletion** — `epics/vlm-image-input.md` — 📋 PLANNING — Wire a ComfyUI IMAGE into the existing generic ChatCompletion node so a vision-capable model can describe/understand images; images carried on the `Message` and translated per-provider (ADR-008 extends ADR-007)
|
||||
|
||||
For the live rollup (specs per epic, % tasks complete, last-commit age):
|
||||
|
||||
|
||||
@@ -0,0 +1,61 @@
|
||||
# Epic: VLM Image Input for ChatCompletion
|
||||
|
||||
## Status
|
||||
Planning — started 2026-07-22
|
||||
|
||||
## Why now
|
||||
|
||||
The generic `ChatCompletion` node and the `LLMProvider` protocol landed
|
||||
text-only (ADR-007), which explicitly deferred any protocol change for a new
|
||||
capability as "its own follow-up." Both shipped backends can already serve
|
||||
vision models — Ollama multimodal models via `/api/chat`'s per-message
|
||||
`images`, and llama.cpp via a multimodal projector (`mmproj`) on the same
|
||||
OpenAI-compatible `/v1/chat/completions` the provider already calls — so the
|
||||
gap is entirely on comfydv's client side, not the servers'. Coupling the chat
|
||||
node with a VLM to describe or reason about images produced elsewhere in a
|
||||
workflow is a frequently-wanted next step, and the adapter pattern makes it a
|
||||
small, symmetric addition rather than a new node family.
|
||||
|
||||
## Specs
|
||||
_SpecKit specs that contribute to this epic._
|
||||
|
||||
- specs/009-vlm-image-input/ — wire a ComfyUI IMAGE into the existing ChatCompletion node; images carried on the Message and translated per-provider
|
||||
|
||||
## ADRs
|
||||
_Cross-cutting decisions this epic required._
|
||||
|
||||
- project-management/ADRs/ADR-008-multimodal-image-input-across-llmprovider-boundary.md — carry images as an optional `Message.images` field; each provider translates to its own wire shape (extends ADR-007's adapter pattern to a second input modality)
|
||||
- project-management/ADRs/ADR-007-llm-provider-adapter-pattern.md — the adapter pattern this epic extends; the generic node/protocol it adds an image path to
|
||||
|
||||
## Success criteria
|
||||
|
||||
- `Message` carries an optional `images` field; text-only requests remain byte-for-byte unchanged (existing Ollama + llama.cpp provider tests stay green)
|
||||
- `ChatCompletion` gains one **optional** `IMAGE` input — no new node classes, no new socket types; a workflow author gains vision by wiring one socket
|
||||
- A wired image reaches a vision model and produces a description/answer on **both** backends:
|
||||
- Ollama: flat per-message `images` passes through `/api/chat` untransformed
|
||||
- llama.cpp: mapped to OpenAI `image_url` content parts on `/v1/chat/completions`
|
||||
- Structured output with an image works via the shared `chat_structured()` (pydantic-ai multimodal content) — one implementation, both backends
|
||||
- A text-only model that receives an image degrades to a clear backend error surfaced by the node, not a crash
|
||||
- Test coverage mirrors the `OllamaProvider`/`LlamaCppProvider` conventions (mock at the provider's own transport seam); CI smoke test passes
|
||||
- No new runtime dependencies beyond what ADR-007 already introduced
|
||||
|
||||
## Non-goals
|
||||
|
||||
- No image **output** or image generation — input-to-VLM only
|
||||
- No new node classes or socket types — additive optional input on the existing generic node
|
||||
- No changes to the client/config nodes (`OllamaClient`, `LlamaCppClient`) or model-management nodes
|
||||
- No auto-provisioning of vision models — the user must have a multimodal model loaded (Ollama multimodal model; llama.cpp launched with an `mmproj` projector); this epic does not install or configure it
|
||||
- No video, audio, or document/PDF modalities — still images only
|
||||
- No image preprocessing beyond what's needed to hand a ComfyUI IMAGE tensor to a backend (no resizing policy, tiling, or OCR of our own)
|
||||
- No `OllamaOption*` parameter translation work — inherited unchanged from ADR-007's scope
|
||||
|
||||
## Notes
|
||||
|
||||
Multimodal readiness is a deployment prerequisite, not something comfydv
|
||||
configures: document in node tooltips that the wired model must be
|
||||
vision-capable, and that llama.cpp needs `--mmproj`. The exact wire shapes
|
||||
(Ollama `/api/chat` `images`, OpenAI `image_url`, pydantic-ai `BinaryContent`)
|
||||
are verified live in the spec's `research.md`/`plan.md`, consistent with how
|
||||
the llama.cpp epic verified router-mode endpoints.
|
||||
|
||||
Reference: ADR-008, ADR-007, GitHub issue #15.
|
||||
@@ -27,6 +27,7 @@ source = "vcs"
|
||||
|
||||
[dependency-groups]
|
||||
dev = [
|
||||
"pillow>=10.0.0",
|
||||
"playwright>=1.60.0",
|
||||
"pytest>=8.4.2",
|
||||
"pytest-cov>=6.0.0",
|
||||
|
||||
@@ -0,0 +1 @@
|
||||
epic = "vlm-image-input"
|
||||
@@ -0,0 +1,41 @@
|
||||
# Specification Quality Checklist: VLM Image Input for ChatCompletion
|
||||
|
||||
**Purpose**: Validate specification completeness and quality before proceeding to planning
|
||||
**Created**: 2026-07-22
|
||||
**Feature**: [spec.md](../spec.md)
|
||||
|
||||
## Content Quality
|
||||
|
||||
- [x] No implementation details (languages, frameworks, APIs)
|
||||
- [x] Focused on user value and business needs
|
||||
- [x] Written for non-technical stakeholders
|
||||
- [x] All mandatory sections completed
|
||||
|
||||
## Requirement Completeness
|
||||
|
||||
- [x] No [NEEDS CLARIFICATION] markers remain
|
||||
- [x] Requirements are testable and unambiguous
|
||||
- [x] Success criteria are measurable
|
||||
- [x] Success criteria are technology-agnostic (no implementation details)
|
||||
- [x] All acceptance scenarios are defined
|
||||
- [x] Edge cases are identified
|
||||
- [x] Scope is clearly bounded
|
||||
- [x] Dependencies and assumptions identified
|
||||
|
||||
## Feature Readiness
|
||||
|
||||
- [x] All functional requirements have clear acceptance criteria
|
||||
- [x] User scenarios cover primary flows
|
||||
- [x] Feature meets measurable outcomes defined in Success Criteria
|
||||
- [x] No implementation details leak into specification
|
||||
|
||||
## Notes
|
||||
|
||||
- The cross-provider "where does the image live / who translates it" decision
|
||||
is intentionally kept out of the spec (WHAT/WHY) and recorded in
|
||||
ADR-008 (HOW). The spec references it via the epic, not inline.
|
||||
- Multi-image-per-turn is documented as an out-of-MVP extension in Assumptions,
|
||||
not a functional requirement — keeps scope bounded.
|
||||
- Items are validated by review, not by an automated gate (`beacon` CLI is not
|
||||
installed in this environment; placeholder/ADR-reference checks were run
|
||||
manually — see the session's validation step).
|
||||
@@ -0,0 +1,64 @@
|
||||
# Contract: Image Input across the LLMProvider boundary
|
||||
|
||||
**Spec**: [spec.md](../spec.md) · **Data model**: [data-model.md](../data-model.md) · **ADR**: [ADR-008](../../../project-management/ADRs/ADR-008-multimodal-image-input-across-llmprovider-boundary.md)
|
||||
|
||||
This feature adds no new protocol methods and no new socket types. The contract
|
||||
below is the **behavioural conformance** every `LLMProvider` must satisfy for
|
||||
the new `Message.images` field, plus the node's input contract.
|
||||
|
||||
---
|
||||
|
||||
## C1 — `Message.images` carrier
|
||||
|
||||
- `Message.images: list[str] | None = None`, base64 strings (no `data:` prefix).
|
||||
- `images=None` or `[]` ⇒ the turn is text-only and MUST produce a request
|
||||
**byte-for-byte identical** to the pre-feature behaviour.
|
||||
|
||||
## C2 — `LLMProvider.chat()` conformance (both providers)
|
||||
|
||||
Given `messages` where the last user turn carries `images`:
|
||||
1. The provider MUST transmit those images with that turn to its backend using
|
||||
its native shape (Ollama flat `images`; llama.cpp OpenAI `image_url` parts).
|
||||
2. The provider MUST NOT transmit an `images` field for turns that have none
|
||||
(empty key omitted).
|
||||
3. All existing behaviour is preserved: blank-retry-with-new-seed loop, response
|
||||
caching, timeout, and error surfacing are unchanged by the presence of images.
|
||||
4. A backend that cannot process images (non-vision model / no `--mmproj`) MUST
|
||||
have its error surfaced to the caller, not swallowed (FR-006).
|
||||
|
||||
## C3 — `chat_structured()` conformance (shared helper, both providers)
|
||||
|
||||
1. Images on the last user turn MUST be attached as `BinaryContent` on the
|
||||
`Agent.run` `user_prompt`; images on history user turns MUST be attached to
|
||||
their `UserPromptPart`.
|
||||
2. All existing structured guarantees hold unchanged: bounded retries (0–5),
|
||||
`RuntimeError` on exhaustion naming model/attempts/snippet, never returns a
|
||||
value that failed schema validation.
|
||||
3. A text-only structured call MUST be indistinguishable from today's.
|
||||
|
||||
## C4 — `ChatCompletion` node input contract
|
||||
|
||||
1. Adds exactly one **optional** `image: ("IMAGE",)` input. No required input
|
||||
added; `RETURN_TYPES`/`RETURN_NAMES` positions unchanged (Constitution VI).
|
||||
2. Un-wired ⇒ behaviour, request, and outputs identical to today.
|
||||
3. Wired ⇒ image(s) attached to the current user turn only; `history` turns
|
||||
unchanged (FR-007).
|
||||
4. Works with `structured_output=True` (C3) and free-text (C2) alike, on either
|
||||
backend, with no per-backend wiring difference (FR-004).
|
||||
|
||||
---
|
||||
|
||||
## Test contracts (test-first — Constitution III)
|
||||
|
||||
| ID | Level | Asserts |
|
||||
|---|---|---|
|
||||
| T1 | `Message` unit | `images` defaults `None`; round-trips base64 list; text-only dump omits the key |
|
||||
| T2 | `OllamaProvider.chat` | image turn → payload message has flat `images:[...]`; text-only payload byte-identical to today (regression) |
|
||||
| T3 | `LlamaCppProvider.chat` | image turn → `content` becomes text+`image_url` parts; text-only `content` stays a plain string (regression) |
|
||||
| T4 | `chat_structured` | image turn builds `BinaryContent` on the prompt; text-only path unchanged; retry/validation contract intact |
|
||||
| T5 | node encode helper | synthetic `[1,H,W,3]` tensor → decodable base64 PNG; `None`/empty → `[]` |
|
||||
| T6 | node contract | optional `image` in `INPUT_TYPES`; un-wired run == today; wired run attaches to last turn only |
|
||||
|
||||
All tests run without a live ComfyUI or a live backend (mock at each provider's
|
||||
own `_post_json`/`Agent.run` seam, per the `test_ollama_provider.py`
|
||||
convention). T5 uses a synthetic tensor + Pillow (dev dep), no ComfyUI.
|
||||
@@ -0,0 +1,83 @@
|
||||
# Phase 1 Data Model: VLM Image Input for ChatCompletion
|
||||
|
||||
**Spec**: [spec.md](./spec.md) · **Research**: [research.md](./research.md)
|
||||
|
||||
The feature adds **one optional field** to an existing model and defines how it
|
||||
maps into each backend's wire shape. No new entities, no new socket types.
|
||||
|
||||
---
|
||||
|
||||
## Modified entity — `Message` (`src/comfydv/_llm/provider.py`)
|
||||
|
||||
```python
|
||||
class Message(BaseModel):
|
||||
role: Literal["system", "user", "assistant"]
|
||||
content: str
|
||||
images: list[str] | None = None # NEW — base64-encoded images (no data: prefix)
|
||||
```
|
||||
|
||||
**Field: `images`**
|
||||
- **Type**: `list[str] | None`, default `None`.
|
||||
- **Meaning**: base64-encoded image payloads associated with this turn. `None`
|
||||
(or empty) means a text-only turn — **byte-for-byte identical to today**.
|
||||
- **Carrier form**: raw base64 string, no `data:` URI prefix. Chosen because
|
||||
every target adapts *from* it (Ollama `images` array, OpenAI data-URI,
|
||||
pydantic-ai `BinaryContent`) — ADR-008.
|
||||
- **Validation**: no format validation at the model layer (the model stays a
|
||||
dumb carrier); malformed data surfaces as a backend error (FR-006). A turn
|
||||
may carry ≥1 image; MVP exercises exactly one.
|
||||
- **Serialization invariant**: provider payload construction MUST omit the
|
||||
`images` key when `None`/empty so existing text-only requests are unchanged
|
||||
(research.md Decision 2; FR-003, SC-004).
|
||||
|
||||
---
|
||||
|
||||
## Mapping table — one carrier, three wire shapes
|
||||
|
||||
| Path | Code site | Transform |
|
||||
|---|---|---|
|
||||
| Ollama free-text | `ollama_provider.py::chat` | none — `model_dump()`'s flat `images` array already matches `/api/chat`; only drop the key when empty |
|
||||
| llama.cpp free-text | `llamacpp_provider.py::chat` | rebuild `content` as OpenAI parts: `[{"type":"text",...},{"type":"image_url","image_url":{"url":"data:image/png;base64,<b64>"}}]` |
|
||||
| Structured (both) | `chat.py::chat_structured` | build `BinaryContent(data=b64decode(img), media_type="image/png")`; attach to `user_prompt` (last turn) / `UserPromptPart` (history turns) as `[text, *images]` |
|
||||
|
||||
---
|
||||
|
||||
## Node input — `ChatCompletion` (`src/comfydv/ollama.py`)
|
||||
|
||||
Add to `INPUT_TYPES["optional"]`:
|
||||
|
||||
```python
|
||||
"image": ("IMAGE",),
|
||||
```
|
||||
|
||||
- **Optional** — un-wired ⇒ `image=None` ⇒ the node builds exactly today's
|
||||
text-only user message. No new required input; no output/socket change
|
||||
(Constitution VI untouched — `RETURN_TYPES` positions 0/1 unchanged).
|
||||
- When wired: encode the tensor to base64 PNG(s) (research.md Decision 4) and
|
||||
set them on the appended `Message(role="user", ...)`. History turns are not
|
||||
modified (FR-007).
|
||||
|
||||
### Encode helper (node-local, `comfy`/Pillow lazy)
|
||||
|
||||
```
|
||||
_encode_image_tensor(image) -> list[str]:
|
||||
# image: ComfyUI IMAGE, torch float tensor [B, H, W, C] in 0..1
|
||||
# → for each frame: *255 → uint8 → PIL.Image.fromarray → PNG bytes → base64
|
||||
# returns [] for None/empty so callers treat it as "no image"
|
||||
```
|
||||
|
||||
Lives in `ollama.py` (node module, already `comfy`-guarded). `src/comfydv/_llm/`
|
||||
never imports torch/numpy/Pillow — it deals only in the base64 strings this
|
||||
helper produces.
|
||||
|
||||
---
|
||||
|
||||
## State & relationships
|
||||
|
||||
- No persistent state; no new caching entity. Existing `_CHAT_RESPONSE_CACHE`
|
||||
keys already include the dumped messages, so an added `images` value
|
||||
participates in the cache key automatically (same image + prompt ⇒ cache
|
||||
hit), and a text-only turn's key is unchanged since the empty key is omitted.
|
||||
- Relationship: `images` belongs to exactly one `Message` (one turn) — this is
|
||||
why the carrier is a message field, not a side-channel parameter (ADR-008
|
||||
Alternative A rejected).
|
||||
@@ -0,0 +1,11 @@
|
||||
Feature: US1 — Describe an image with a chat node
|
||||
|
||||
Scenario: Describe a wired image
|
||||
Given a chat node connected to a backend with a vision-capable model loaded and an image wired into the node's image input
|
||||
When the workflow executes with a prompt like "describe this image"
|
||||
Then the node returns a text response that reflects the actual content of the wired image
|
||||
|
||||
Scenario: No image wired behaves exactly as today
|
||||
Given the same chat node with no image wired
|
||||
When the workflow executes
|
||||
Then the node behaves exactly as it does today — text-only chat, identical response for identical text input — with no new required inputs and no change in output
|
||||
@@ -0,0 +1,11 @@
|
||||
Feature: US2 — Same image input on either backend
|
||||
|
||||
Scenario: Swap Ollama for llama.cpp and the image path still works
|
||||
Given a workflow that describes an image via the chat node wired to Ollama
|
||||
When the connection node is swapped to a llama.cpp one (pointed at a server with a multimodal model) with no other change
|
||||
Then the workflow still returns a description of the same image
|
||||
|
||||
Scenario: Both backends produce an image-grounded response
|
||||
Given equivalent image + prompt inputs on both backends
|
||||
When each workflow executes
|
||||
Then both produce a coherent image-grounded text response — no backend requires a different node, input shape, or wiring for the image
|
||||
@@ -0,0 +1,11 @@
|
||||
Feature: US3 — Structured output about an image
|
||||
|
||||
Scenario: Structured fields populated from the image
|
||||
Given the chat node with an image wired and structured output enabled with a valid schema
|
||||
When the workflow executes against a vision-capable model
|
||||
Then each schema field is available as its own typed output, populated from the image, with no required field blank
|
||||
|
||||
Scenario: Invalid structured output retries then fails clearly
|
||||
Given the same setup where the model first returns invalid or incomplete structured output
|
||||
When the workflow executes
|
||||
Then the node retries and, if still unsuccessful, fails with a clear error — the same retry/validation behaviour the text-only structured path already guarantees
|
||||
@@ -0,0 +1,125 @@
|
||||
# Implementation Plan: VLM Image Input for ChatCompletion
|
||||
|
||||
**Branch**: `009-vlm-image-input` | **Date**: 2026-07-22 | **Spec**: [spec.md](./spec.md)
|
||||
|
||||
**Input**: Feature specification from `/specs/009-vlm-image-input/spec.md`
|
||||
|
||||
## Summary
|
||||
|
||||
Let a workflow author wire a ComfyUI `IMAGE` into the existing generic
|
||||
`ChatCompletion` node so a vision-capable model can describe or reason about it,
|
||||
on **either** backend. Per ADR-008 (extending ADR-007's adapter pattern to a
|
||||
second input modality), images ride on an optional `Message.images` carrier and
|
||||
each provider translates that carrier into its own wire shape: Ollama's flat
|
||||
`/api/chat` `images` array (passes through untouched), llama.cpp's OpenAI
|
||||
`image_url` content-parts, and — for structured output — pydantic-ai
|
||||
`BinaryContent`, shared by both backends through `OpenAIChatModel`. The node
|
||||
converts its `IMAGE` tensor to base64 PNG; everything below the node deals only
|
||||
in base64 strings. Text-only behaviour is byte-for-byte unchanged when no image
|
||||
is wired.
|
||||
|
||||
## Technical Context
|
||||
|
||||
**Language/Version**: Python ≥3.11 (unchanged, per `pyproject.toml`)
|
||||
|
||||
**Primary Dependencies**: existing only for runtime — `aiohttp` (Ollama/llama.cpp
|
||||
REST), `pydantic-ai-slim[openai]>=2.9.0` (structured path; its `BinaryContent`
|
||||
multimodal type was verified against the installed 2.9.0 source, see
|
||||
`research.md`). **No new core runtime dependency.** `pillow` is added to the
|
||||
**dev** group so the node's tensor→PNG encoder is unit-testable without a live
|
||||
ComfyUI; at runtime Pillow/numpy are ComfyUI-provided (same stance the repo
|
||||
already takes for torch).
|
||||
|
||||
**Storage**: N/A — no persistent storage; reuses the existing
|
||||
`_CHAT_RESPONSE_CACHE` (an `images` value participates in the cache key
|
||||
automatically).
|
||||
|
||||
**Testing**: `pytest` via `uv run pytest`, following the
|
||||
`tests/test_ollama_provider.py` convention (mock at each provider's own
|
||||
`_post_json` / `Agent.run` seam, no live server or ComfyUI required). Test-first
|
||||
per Constitution III; the tensor-encode test uses a synthetic tensor + Pillow.
|
||||
|
||||
**Target Platform**: ComfyUI custom-node runtime, same as the existing LLM nodes.
|
||||
|
||||
**Project Type**: Library / ComfyUI custom-node pack (single project).
|
||||
|
||||
**Performance Goals**: No new numeric target; image encoding is a one-shot
|
||||
per-execution PNG encode, negligible against inference latency.
|
||||
|
||||
**Constraints**: Text-only requests MUST stay byte-identical (FR-003/SC-004) —
|
||||
providers omit an empty `images` key. `src/comfydv/_llm/` must not import
|
||||
torch/numpy/Pillow (Constitution IV) — tensor handling stays in the node.
|
||||
llama.cpp image support requires a server launched with `--mmproj` (deployment
|
||||
prerequisite, surfaced as a clear error when absent, not configured by comfydv).
|
||||
|
||||
**Scale/Scope**: One new `Message` field; a per-provider mapping in each
|
||||
`chat()` plus the shared `chat_structured()`; one optional node input + a
|
||||
node-local encode helper. No new node classes, no new socket types, no protocol
|
||||
method changes.
|
||||
|
||||
## Constitution Check
|
||||
|
||||
*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.*
|
||||
|
||||
| Principle | Verdict | Notes |
|
||||
|---|---|---|
|
||||
| I. ComfyUI Contract First | PASS | `ChatCompletion` keeps its `INPUT_TYPES`/`RETURN_TYPES`/`FUNCTION`/`CATEGORY`; only an optional input is added. No new registration, no ComfyUI changes. |
|
||||
| II. Sandbox All User-Supplied Code | N/A | No template/expression evaluation in this feature. |
|
||||
| III. Test-First | PASS (binding) | Test contracts T1–T6 (`contracts/image-input-contract.md`) written test-first, mirroring `test_ollama_provider.py`. Each runs without a live ComfyUI/backend. |
|
||||
| IV. Graceful Degradation Outside ComfyUI | PASS (binding) | `_llm/` stays torch/numpy/Pillow-free — pure base64 carrier + mapping, unit-testable. Tensor→PNG lives in `ollama.py` (already `comfy`-guarded) with lazy Pillow/numpy import, so module import outside ComfyUI is unaffected. |
|
||||
| V. Simplicity — Function Before Class | PASS | No new class. New logic is a `Message` field, two small per-provider transforms, one shared helper edit, and one module-level encode function. |
|
||||
| VI. Fixed Output Positions | PASS | Outputs are untouched — only an optional **input** is added; `RETURN_TYPES`/`RETURN_NAMES` positions 0/1 and the structured extra-outputs contract are unchanged. |
|
||||
|
||||
Re-checked post-Phase 1 design (data-model.md, contracts/): unchanged — no new
|
||||
gate violations. No Complexity Tracking entries needed.
|
||||
|
||||
## Project Structure
|
||||
|
||||
### Documentation (this feature)
|
||||
|
||||
```text
|
||||
specs/009-vlm-image-input/
|
||||
├── plan.md # This file
|
||||
├── research.md # Phase 0 — wire shapes verified against installed deps
|
||||
├── data-model.md # Phase 1 — Message.images + per-provider mapping
|
||||
├── quickstart.md # Phase 1 — minimal describe-an-image workflow
|
||||
├── contracts/
|
||||
│ └── image-input-contract.md # Phase 1 — behavioural + test contracts (T1–T6)
|
||||
├── checklists/requirements.md # Spec quality checklist (from /speckit-specify)
|
||||
└── tasks.md # Phase 2 (/speckit-tasks) — not created here
|
||||
```
|
||||
|
||||
### Source Code (repository root)
|
||||
|
||||
```text
|
||||
src/comfydv/
|
||||
├── ollama.py # MODIFIED — ChatCompletion: optional `image` input +
|
||||
│ # node-local _encode_image_tensor() (lazy Pillow/numpy)
|
||||
├── _llm/
|
||||
│ ├── provider.py # MODIFIED — Message gains `images: list[str] | None = None`
|
||||
│ ├── ollama_provider.py # MODIFIED — chat(): pass flat images through; omit empty key
|
||||
│ ├── llamacpp_provider.py # MODIFIED — chat(): map images → OpenAI image_url parts
|
||||
│ └── chat.py # MODIFIED — chat_structured(): images → BinaryContent on prompt
|
||||
└── __init__.py # unchanged — no new node class or mapping
|
||||
|
||||
tests/
|
||||
├── test_provider.py (or test_ollama_provider.py) # T1 Message carrier + regression
|
||||
├── test_ollama_provider.py # MODIFIED — T2 Ollama image mapping + text regression
|
||||
├── test_llamacpp_provider.py # MODIFIED — T3 llama.cpp content-parts + text regression
|
||||
├── test_llm_chat.py / chat tests # T4 chat_structured multimodal + regression
|
||||
└── test_ollama.py # MODIFIED — T5 encode helper, T6 node input contract
|
||||
|
||||
pyproject.toml # MODIFIED — add `pillow` to [dependency-groups].dev only
|
||||
```
|
||||
|
||||
**Structure Decision**: Purely additive edits to the four existing `_llm`/node
|
||||
files that ADR-007 established — no new module, because there is no new class or
|
||||
node (contrast 008, which added a provider + node). The image path threads
|
||||
through the exact seams the text path already uses, which is the whole point of
|
||||
ADR-008: a second modality on the same adapter, not a parallel structure.
|
||||
|
||||
## Complexity Tracking
|
||||
|
||||
> **Fill ONLY if Constitution Check has violations that must be justified**
|
||||
|
||||
None — see Constitution Check above.
|
||||
@@ -0,0 +1,48 @@
|
||||
# Quickstart: Describe an image with ChatCompletion
|
||||
|
||||
**Spec**: [spec.md](./spec.md)
|
||||
|
||||
Minimal end-to-end walkthrough of the feature once shipped.
|
||||
|
||||
## Prerequisites
|
||||
|
||||
- A running backend with a **vision-capable** model:
|
||||
- **Ollama** — a multimodal model pulled and available (e.g. a llava-class model), or
|
||||
- **llama.cpp** — `llama-server` launched in router mode **with a multimodal projector**: `--mmproj <projector.gguf>` alongside the model.
|
||||
- comfydv installed in ComfyUI.
|
||||
|
||||
## Steps
|
||||
|
||||
1. Add an image source to the canvas (e.g. **Load Image**) → gives an `IMAGE`.
|
||||
2. Add a client node (**OllamaClient** or **LlamaCppClient**) → gives `LLM_CLIENT`.
|
||||
3. Add **ChatCompletion**. Wire:
|
||||
- `client` ← the client node
|
||||
- `model` ← a vision-capable model name (typed or wired)
|
||||
- `prompt` ← `"Describe this image in one sentence."`
|
||||
- `image` ← the `IMAGE` from step 1 ← **the only new wire**
|
||||
4. Queue the prompt. The `response` output is a text description of the image.
|
||||
|
||||
## Structured variant (optional)
|
||||
|
||||
- On **ChatCompletion**, set `structured_output = True` and provide a schema, e.g.:
|
||||
```json
|
||||
{"type":"object","properties":{"caption":{"type":"string"},"has_text":{"type":"boolean"}},"required":["caption","has_text"]}
|
||||
```
|
||||
- Run: each field (`caption`, `has_text`) appears as its own typed output,
|
||||
populated from the image, with no required field blank.
|
||||
|
||||
## Swap backends (proves FR-004 / SC-002)
|
||||
|
||||
- Replace **OllamaClient** with **LlamaCppClient** (pointed at an `--mmproj`
|
||||
server) — **change nothing else**. Re-queue: same image description path.
|
||||
|
||||
## What stays the same
|
||||
|
||||
- Leave `image` un-wired and ChatCompletion behaves exactly as before — text-only,
|
||||
identical results. No existing workflow changes.
|
||||
|
||||
## Expected failure (proves FR-006 / SC-005)
|
||||
|
||||
- Wire an image but select a **non-vision** model (or a `llama-server` started
|
||||
without `--mmproj`): the node reports a clear error that the model/server
|
||||
can't process images — it does not silently answer as if no image was sent.
|
||||
@@ -0,0 +1,155 @@
|
||||
# Phase 0 Research: VLM Image Input for ChatCompletion
|
||||
|
||||
**Spec**: [spec.md](./spec.md) · **Plan**: [plan.md](./plan.md) · **ADR**: [ADR-008](../../project-management/ADRs/ADR-008-multimodal-image-input-across-llmprovider-boundary.md)
|
||||
|
||||
ADR-008 recorded the boundary decision (images on `Message.images`, translated
|
||||
per-provider) but deferred the exact wire shapes for live verification. This
|
||||
file resolves them against the **installed** dependency versions and the
|
||||
current provider code, not from memory.
|
||||
|
||||
---
|
||||
|
||||
## Decision 1 — pydantic-ai multimodal vehicle (structured-output path)
|
||||
|
||||
**Decision**: In the shared `chat_structured()` helper (`src/comfydv/_llm/chat.py`),
|
||||
attach images as `pydantic_ai.messages.BinaryContent(data=<png bytes>,
|
||||
media_type="image/png")` inside a `Sequence[UserContent]`. The current-turn
|
||||
image rides on `Agent.run(user_prompt=[text, BinaryContent(...)])`; a
|
||||
history turn's image rides on `UserPromptPart(content=[text, BinaryContent(...)])`.
|
||||
|
||||
**Rationale / verified**: Read directly from the pinned
|
||||
`pydantic_ai_slim==2.9.0` source in this environment:
|
||||
|
||||
- `BinaryContent` (`messages.py:521`) — `__init__(self, data: bytes, *,
|
||||
media_type: ..., identifier=None, ...)`; exposes a `.base64` helper. `data`
|
||||
is **bytes**, so `chat.py` must `base64.b64decode()` the `Message.images`
|
||||
string into bytes when building it.
|
||||
- `UserPromptPart.content: str | Sequence[UserContent]` (`messages.py:1022`)
|
||||
and `user_prompt` on `Agent.run` accept the same. `UserContent = str |
|
||||
TextContent | MultiModalContent | CachePoint` (`messages.py:899`), and
|
||||
`MultiModalContent` includes `BinaryContent`/`ImageUrl` — so a `[text,
|
||||
image]` list is the supported shape.
|
||||
- `OpenAIChatModel` renders `BinaryContent` for images as an OpenAI
|
||||
`image_url` data-URI, and reads `BinaryContent.vendor_metadata['detail']`
|
||||
for the `detail` setting (documented in the field's own docstring).
|
||||
|
||||
**Consequence**: the structured path is provider-agnostic *for free* — both
|
||||
Ollama and llama.cpp reach `/v1/chat/completions` through the same
|
||||
`OpenAIChatModel`, so one change in `chat.py` covers structured output on both
|
||||
backends. No per-provider structured code.
|
||||
|
||||
**Alternatives considered**: `ImageUrl(url="data:image/png;base64,...")` — also
|
||||
supported, but requires assembling a data URI string; `BinaryContent` from raw
|
||||
bytes + media type is the more direct representation of what we hold and lets
|
||||
pydantic-ai own the data-URI formatting.
|
||||
|
||||
---
|
||||
|
||||
## Decision 2 — Ollama free-text path (`/api/chat`)
|
||||
|
||||
**Decision**: `OllamaProvider.chat()` sends each message's images as a flat
|
||||
`images` array of **base64 strings** (no `data:` prefix) alongside `content`,
|
||||
which is exactly Ollama's native `/api/chat` message schema. Because
|
||||
`Message.images` already holds base64 strings, `Message.model_dump()` produces
|
||||
the correct shape with **no transform** — the field flows straight through.
|
||||
|
||||
**Rationale / verified**: `OllamaProvider.chat()`
|
||||
(`src/comfydv/_llm/ollama_provider.py:280`) already builds
|
||||
`payload_messages = [m.model_dump() for m in messages]` and POSTs to
|
||||
`/api/chat`. Ollama's documented `/api/chat` message object is
|
||||
`{"role", "content", "images": [<base64>, ...]}` — the flat sibling field this
|
||||
carrier maps onto directly. This is the reason base64 is the neutral carrier
|
||||
form (ADR-008).
|
||||
|
||||
**Constraint discovered — byte-identical text path (FR-003/SC-004)**: adding
|
||||
`images: list[str] | None = None` to `Message` means a text-only message would
|
||||
dump as `{"role","content","images":null}`, changing today's request body.
|
||||
Providers MUST drop a `None`/empty `images` before sending. Resolution:
|
||||
serialize provider payload messages with the images key omitted when empty
|
||||
(e.g. `model_dump(exclude_none=True)`, or drop the key explicitly). Guarded by
|
||||
the existing Ollama provider tests, which assert the exact payload.
|
||||
|
||||
---
|
||||
|
||||
## Decision 3 — llama.cpp free-text path (`/v1/chat/completions`)
|
||||
|
||||
**Decision**: `LlamaCppProvider.chat()` maps a message carrying images into
|
||||
OpenAI-style multimodal `content` **parts** before POSTing:
|
||||
`content: [{"type":"text","text":<content>},
|
||||
{"type":"image_url","image_url":{"url":"data:image/png;base64,<b64>"}}]`.
|
||||
Messages with no images keep the plain-string `content` unchanged.
|
||||
|
||||
**Rationale / verified**: `LlamaCppProvider.chat()`
|
||||
(`src/comfydv/_llm/llamacpp_provider.py:163`) builds
|
||||
`payload_messages = [m.model_dump() for m in messages]` and POSTs to
|
||||
`/v1/chat/completions`. Unlike Ollama, a flat `images` sibling is **not**
|
||||
understood there — OpenAI's vision schema requires images inside `content` as
|
||||
typed parts. `llama-server` implements this OpenAI-compatible multimodal
|
||||
format **only when launched with a multimodal projector (`--mmproj`)**; without
|
||||
it, image parts yield a server error (surfaced per FR-006, not crashed on).
|
||||
This is the single point where the two providers genuinely diverge — exactly
|
||||
the leakage ADR-008 localizes inside each provider.
|
||||
|
||||
**Alternatives considered**: normalizing Ollama *up* to content-parts too (one
|
||||
shared mapper) — rejected in ADR-008 Alternative C: it forces the
|
||||
currently-simpler Ollama path to do extra work and inverts "each provider owns
|
||||
its wire format."
|
||||
|
||||
---
|
||||
|
||||
## Decision 4 — ComfyUI IMAGE tensor → base64 PNG (node layer)
|
||||
|
||||
**Decision**: The `ChatCompletion` node converts its optional `IMAGE` input to
|
||||
base64 PNG(s) via Pillow: ComfyUI IMAGE is a float tensor `[B, H, W, C]` in
|
||||
`0..1`; scale to `uint8`, `PIL.Image.fromarray(...)`, save PNG to an in-memory
|
||||
buffer, base64-encode. A batch of `B` frames becomes `B` base64 strings in the
|
||||
turn's `images` list (natural multi-image; MVP exercises `B=1`). The import of
|
||||
Pillow/numpy is **lazy** (inside the encode function), so the module still
|
||||
imports cleanly outside ComfyUI (Constitution IV).
|
||||
|
||||
**Rationale**: Pillow is the ComfyUI-ecosystem standard for IMAGE tensor ↔
|
||||
file and is present in every ComfyUI install; numpy comes with torch. Neither
|
||||
is added to comfydv's **core** runtime deps — they are ComfyUI-provided, the
|
||||
same stance the repo already takes for torch (dev-only in `pyproject.toml`).
|
||||
To keep the encoder **test-first** (Constitution III) without a live ComfyUI,
|
||||
add `pillow` to the **dev** dependency group so a unit test can feed a
|
||||
synthetic `numpy`/`torch` tensor through the pure encode function and assert a
|
||||
decodable PNG.
|
||||
|
||||
**Boundary kept clean**: only the node (`src/comfydv/ollama.py`, already
|
||||
`comfy`-guarded) touches tensors/Pillow. Everything in `src/comfydv/_llm/`
|
||||
deals purely in base64 strings and stays unit-testable with hand-crafted
|
||||
strings — no torch, numpy, or Pillow import there.
|
||||
|
||||
**Edge cases (FR-006, Edge Cases)**: an un-wired optional input arrives as
|
||||
`None` → node builds today's exact text-only message. A zero-size / empty batch
|
||||
tensor → treated as "no image". A non-vision model or non-`mmproj` server
|
||||
returns a backend error → surfaced with a clear message, never a silent
|
||||
image-less answer.
|
||||
|
||||
---
|
||||
|
||||
## Decision 5 — where the image attaches on the turn (FR-007)
|
||||
|
||||
**Decision**: The node attaches images to the **current user turn only** — the
|
||||
`Message(role="user", content=prompt, images=[...])` it already appends. Prior
|
||||
`history` turns are untouched. The structured helper likewise only lifts images
|
||||
onto the final user turn (and any history turn that already carried them),
|
||||
matching its existing "last message is the prompt" contract
|
||||
(`chat.py:106`, which requires `messages[-1].role == "user"`).
|
||||
|
||||
---
|
||||
|
||||
## Summary of resolved unknowns
|
||||
|
||||
| Unknown (from ADR-008) | Resolved to |
|
||||
|---|---|
|
||||
| pydantic-ai multimodal type | `BinaryContent(data=bytes, media_type="image/png")` — verified in installed 2.9.0 |
|
||||
| Structured path per-provider? | No — shared via `OpenAIChatModel`; one change in `chat.py` |
|
||||
| Ollama wire shape | flat `images: [base64]` on the message; passes through `model_dump()` |
|
||||
| llama.cpp wire shape | OpenAI `image_url` content-parts; requires `--mmproj` |
|
||||
| Text-path byte-identity | drop empty `images` key in provider payloads (guarded by existing tests) |
|
||||
| Tensor → base64 | Pillow, lazy import in node; `pillow` added to dev deps for testability |
|
||||
| No new runtime deps | Confirmed — Pillow/numpy are ComfyUI-provided, dev-only here |
|
||||
|
||||
No `NEEDS CLARIFICATION` remain.
|
||||
@@ -0,0 +1,148 @@
|
||||
# Feature Specification: VLM Image Input for ChatCompletion
|
||||
|
||||
**Feature Branch**: `009-vlm-image-input`
|
||||
|
||||
**Created**: 2026-07-22
|
||||
|
||||
**Status**: Draft
|
||||
|
||||
**Input**: User description: "Let a workflow author wire a ComfyUI IMAGE into the existing generic ChatCompletion node so a vision-capable model (VLM) on either backend (Ollama multimodal models, llama.cpp multimodal via mmproj) can describe or understand the image. Provider-agnostic per ADR-007/ADR-008: the node attaches the image to the user message; each provider maps it to its own wire format. The Message carrier gains an optional image field; text-only behaviour is unchanged when no image is wired."
|
||||
|
||||
## User Scenarios & Testing *(mandatory)*
|
||||
|
||||
### User Story 1 - Describe an image with a chat node (Priority: P1) 🎯 MVP
|
||||
|
||||
As a ComfyUI workflow author with a vision-capable model available, I want to
|
||||
wire an image into the chat node I already use and get back a text description
|
||||
or answer about that image, so I can add image understanding to a workflow
|
||||
without learning a new node.
|
||||
|
||||
**Why this priority**: This is the entire point of the feature — a picture in,
|
||||
a text understanding out — and the proof that image input works through the
|
||||
existing generic node on at least one backend.
|
||||
|
||||
**Independent Test**: Wire any image source into the chat node's image input,
|
||||
point the node at a loaded vision-capable model, run the workflow, and confirm
|
||||
the response text describes the wired image.
|
||||
|
||||
**Acceptance Scenarios**:
|
||||
|
||||
1. **Given** a chat node connected to a backend with a vision-capable model loaded and an image wired into the node's image input, **When** the workflow executes with a prompt like "describe this image", **Then** the node returns a text response that reflects the actual content of the wired image.
|
||||
2. **Given** the same chat node with **no** image wired, **When** the workflow executes, **Then** the node behaves exactly as it does today — text-only chat, identical response for identical text input — with no new required inputs and no change in output.
|
||||
|
||||
---
|
||||
|
||||
### User Story 2 - Same image input on either backend (Priority: P1)
|
||||
|
||||
As a workflow author, I want image input to work the same way whether my chat
|
||||
node is connected to Ollama or to llama.cpp, so I don't have to rebuild or
|
||||
relearn the image path when I switch backends — exactly as text and structured
|
||||
output already behave identically across the two.
|
||||
|
||||
**Why this priority**: The generic-node promise (ADR-007) is the reason this
|
||||
feature is small; this story is what proves the image path honours it rather
|
||||
than quietly becoming backend-specific.
|
||||
|
||||
**Independent Test**: Run User Story 1 unchanged against an Ollama connection
|
||||
and against a llama.cpp connection (each with a vision-capable model), and
|
||||
confirm both return a description of the wired image using the identical node
|
||||
setup.
|
||||
|
||||
**Acceptance Scenarios**:
|
||||
|
||||
1. **Given** a workflow that describes an image via the chat node wired to Ollama, **When** the connection node is swapped to a llama.cpp one (pointed at a server with a multimodal model) with no other change, **Then** the workflow still returns a description of the same image.
|
||||
2. **Given** equivalent image + prompt inputs on both backends, **When** each workflow executes, **Then** both produce a coherent image-grounded text response — no backend requires a different node, input shape, or wiring for the image.
|
||||
|
||||
---
|
||||
|
||||
### User Story 3 - Structured output about an image (Priority: P2)
|
||||
|
||||
As a workflow author, I want to combine image input with the node's existing
|
||||
structured-output mode, so a VLM can return schema-validated fields extracted
|
||||
from an image (for example a caption, a list of detected objects, or a
|
||||
yes/no), not just free text.
|
||||
|
||||
**Why this priority**: Structured output is an existing, valued capability;
|
||||
making it work with images turns "describe this" into usable, wired,
|
||||
downstream-typed data. It builds on User Story 1 and is lower risk to defer
|
||||
than getting basic image chat working at all.
|
||||
|
||||
**Independent Test**: Enable structured output on the chat node with a schema,
|
||||
wire an image, run against a vision-capable model, and confirm each schema
|
||||
field is populated from the image and no required field is blank.
|
||||
|
||||
**Acceptance Scenarios**:
|
||||
|
||||
1. **Given** the chat node with an image wired and structured output enabled with a valid schema, **When** the workflow executes against a vision-capable model, **Then** each schema field is available as its own typed output, populated from the image, with no required field blank.
|
||||
2. **Given** the same setup where the model first returns invalid or incomplete structured output, **When** the workflow executes, **Then** the node retries and, if still unsuccessful, fails with a clear error — the same retry/validation behaviour the text-only structured path already guarantees.
|
||||
|
||||
---
|
||||
|
||||
### Edge Cases
|
||||
|
||||
- What happens when an image is wired but the selected model is **not**
|
||||
vision-capable? The node must surface a clear error attributable to the model
|
||||
lacking image support, not crash and not silently drop the image and answer
|
||||
as if none was sent.
|
||||
- What happens when the backend server is reachable but was not started with
|
||||
multimodal support (e.g. a llama.cpp server launched without an `mmproj`
|
||||
projector)? The node should report a clear, specific error rather than an
|
||||
unhelpful generic failure.
|
||||
- What happens with an empty or zero-size image input, or an image input that
|
||||
is wired but carries no actual image data? The node should treat it as "no
|
||||
image" or report a clear error — never send a malformed request.
|
||||
- What happens when both an image and a multi-turn history are present? The
|
||||
image must be associated with the current user turn, and prior turns must
|
||||
remain unaffected.
|
||||
- What happens to the node's text-only path for a model/backend that does not
|
||||
understand images at all — does an un-wired image input leave the request
|
||||
byte-for-byte identical to today's? (It must.)
|
||||
|
||||
## Requirements *(mandatory)*
|
||||
|
||||
### Functional Requirements
|
||||
|
||||
- **FR-001**: The system MUST let a workflow author provide an image to the existing chat node through a single, **optional** image input — no new node and no new required input.
|
||||
- **FR-002**: When an image is provided, the system MUST include it with the current user turn sent to the connected model, so a vision-capable model can ground its response in that image.
|
||||
- **FR-003**: When **no** image is provided, the system MUST send exactly the request it sends today — text-only behaviour, inputs, and outputs unchanged, with no regression for existing workflows.
|
||||
- **FR-004**: Image input MUST work identically across both supported backends from the workflow author's perspective — same node, same wiring, same input shape — with each backend's differing native image format handled internally, not exposed on the graph.
|
||||
- **FR-005**: Image input MUST be compatible with the node's existing structured-output mode: an image-grounded response can be schema-validated with the same retry and validation guarantees as the text-only structured path.
|
||||
- **FR-006**: The system MUST surface a clear, specific error when an image is provided but the target model or backend cannot process images (non-vision model, or a server without multimodal support), rather than crashing or silently discarding the image.
|
||||
- **FR-007**: The system MUST associate a provided image with the current user turn only, leaving any prior conversation history unchanged.
|
||||
|
||||
### Key Entities *(include if feature involves data)*
|
||||
|
||||
- **Chat message**: The existing per-turn unit of a chat request. Extended so a
|
||||
turn can optionally carry one or more images in addition to its text; a
|
||||
turn with no image is unchanged from today.
|
||||
- **Image input**: An image supplied on the workflow canvas (the standard
|
||||
ComfyUI image type) and attached to the current user turn; provider-neutral
|
||||
at the boundary, translated to each backend's native shape internally.
|
||||
|
||||
## Success Criteria *(mandatory)*
|
||||
|
||||
### Measurable Outcomes
|
||||
|
||||
- **SC-001**: A workflow author can make an existing chat node describe a wired image by adding exactly one connection (the image), with no new node and no other node changes.
|
||||
- **SC-002**: The same image-describing workflow runs unchanged when repointed from one backend to the other — zero edits beyond swapping the connection node.
|
||||
- **SC-003**: Structured-output workflows with an image populate every required schema field from the image content, with zero blank-required-field results, matching the text-only structured guarantee.
|
||||
- **SC-004**: Every existing text-only workflow produces identical results after this feature ships — no observable change when no image is wired (existing backend behaviour tests remain green).
|
||||
- **SC-005**: Providing an image to a non-vision model or a non-multimodal server yields a clear, specific error in 100% of such cases — never a crash and never a silently image-less answer presented as if the image was seen.
|
||||
|
||||
## Assumptions
|
||||
|
||||
- Workflow authors run their own backend (Ollama or llama.cpp) with a
|
||||
vision-capable model available and loaded; for llama.cpp this means the
|
||||
server was launched with a multimodal projector (`mmproj`). This feature does
|
||||
not install, configure, download, or launch vision models.
|
||||
- Scope is still **images only** — no video, audio, or document modalities; and
|
||||
image **input** only — no image generation or output.
|
||||
- A single image per turn is the primary target; carrying more than one image
|
||||
per turn is a natural extension of the same carrier but is not a required
|
||||
acceptance criterion of the MVP.
|
||||
- The generic `ChatCompletion` node, the `LLMProvider` protocol, and both
|
||||
providers already exist (ADR-007) and are extended, not replaced; the
|
||||
cross-provider image-carrier decision is recorded in ADR-008.
|
||||
- The standard ComfyUI image type is the input; converting it to the neutral
|
||||
form each backend consumes is an internal concern of this feature, not
|
||||
something the workflow author sees.
|
||||
@@ -0,0 +1,146 @@
|
||||
# Tasks: VLM Image Input for ChatCompletion
|
||||
|
||||
**Input**: Design documents from `/specs/009-vlm-image-input/`
|
||||
|
||||
**Prerequisites**: plan.md, spec.md, research.md, data-model.md, contracts/image-input-contract.md
|
||||
|
||||
**Tests**: First-class — every implementation task has a paired failing-test task (`-T`/`-I` suffix). Contracts T1–T6 in `contracts/image-input-contract.md` map to the pairs below.
|
||||
|
||||
**Organization**: Grouped by user story (spec.md priorities: US1 P1 🎯 MVP, US2 P1, US3 P2).
|
||||
|
||||
## Format: `[ID] [P?] [Story] Description`
|
||||
|
||||
- **[P]**: Can run in parallel (different files, no dependencies)
|
||||
- **[Story]**: US1–US3
|
||||
- **-T / -I**: paired test (red) / implementation (green) — the `-T` is committed failing before its `-I` partner (no test + impl in one commit)
|
||||
|
||||
## Path Conventions
|
||||
|
||||
Single project: `src/comfydv/`, `tests/` at repo root. Purely additive edits to
|
||||
the existing `_llm`/node files (plan.md Structure Decision) — no new module.
|
||||
`src/comfydv/_llm/` stays torch/numpy/Pillow-free (Constitution IV); tensor
|
||||
handling lives only in the `comfy`-guarded `ollama.py`.
|
||||
|
||||
---
|
||||
|
||||
## Phase 1: Setup
|
||||
|
||||
- [x] T001 Add `pillow` to `[dependency-groups].dev` in `pyproject.toml` — lets the node's tensor→PNG encoder be unit-tested without a live ComfyUI; runtime Pillow/numpy are ComfyUI-provided, so **no core runtime dependency is added** (research.md Decision 4)
|
||||
|
||||
---
|
||||
|
||||
## Phase 2: Foundational (Blocking Prerequisites)
|
||||
|
||||
**Purpose**: the image carrier every path depends on. **⚠️ No user story work can begin until this is complete.**
|
||||
|
||||
- [x] T002-T Write FAILING test: `Message.images` defaults to `None`, round-trips a base64 list, and a text-only message's transport dump **omits** the `images` key (byte-identical to today), in `tests/test_llm_provider.py` (contract T1)
|
||||
- [x] T002-I Add `images: list[str] | None = None` to `Message` in `src/comfydv/_llm/provider.py` — makes T002-T pass
|
||||
|
||||
**Checkpoint**: carrier ready — user stories can begin.
|
||||
|
||||
---
|
||||
|
||||
## Phase 3: User Story 1 — Describe an image with a chat node (Priority: P1) 🎯 MVP
|
||||
|
||||
**Goal**: A workflow author wires a ComfyUI `IMAGE` into the existing `ChatCompletion` node and gets back a text description via an Ollama vision model; the text-only path is unchanged when no image is wired.
|
||||
|
||||
**Independent Test**: Wire an image → `ChatCompletion` → Ollama (vision model), confirm the response describes the image; un-wire the image and confirm behaviour/output identical to today.
|
||||
|
||||
- [x] T003-T [P] [US1] Write FAILING test: node-local `_encode_image_tensor()` converts a synthetic `[1,H,W,3]` float tensor (0..1) into a **decodable** base64 PNG, encodes a `B>1` batch to a list of that length, and returns `[]` for `None`/empty, in `tests/test_ollama.py` (contract T5; witnesses `features/us1_describe_image.feature` scenario "Describe a wired image")
|
||||
- [x] T003-I [US1] Implement `_encode_image_tensor()` in `src/comfydv/ollama.py` — lazy `PIL`/`numpy` import so module import stays clean outside ComfyUI (Constitution IV); batch → one base64 string per frame — makes T003-T pass
|
||||
- [x] T004-T [US1] Write FAILING test: `ChatCompletion.INPUT_TYPES` exposes an **optional** `image: ("IMAGE",)`; `RETURN_TYPES`/`RETURN_NAMES` positions are unchanged; an un-wired run builds the same text-only messages as today; a wired run attaches images to the **last user turn only** (history untouched), in `tests/test_ollama.py` (contract T6; witnesses both `features/us1_describe_image.feature` scenarios)
|
||||
- [x] T004-I [US1] Add the optional `image` input (with a tooltip noting a vision-capable model is required; llama.cpp needs `--mmproj`) and attach encoded images to the appended user `Message` in `ChatCompletion.chat()` in `src/comfydv/ollama.py` — makes T004-T pass (depends on T003-I, T002-I)
|
||||
- [x] T005-T [P] [US1] Write FAILING test: `OllamaProvider.chat()` forwards a message's images as a flat `images:[...]` array to `/api/chat`, and a text-only call's payload is **byte-identical to today** (regression), in `tests/test_ollama_provider.py` (contract T2; witnesses `features/us1_describe_image.feature` scenario "Describe a wired image")
|
||||
- [x] T005-I [US1] Ensure `OllamaProvider.chat()` passes images through and omits the empty `images` key (e.g. `model_dump(exclude_none=True)`) in `src/comfydv/_llm/ollama_provider.py` — makes T005-T pass (depends on T002-I)
|
||||
|
||||
**Checkpoint**: describe-an-image works end-to-end on Ollama (MVP); every existing text-only test stays green.
|
||||
|
||||
---
|
||||
|
||||
## Phase 4: User Story 2 — Same image input on either backend (Priority: P1)
|
||||
|
||||
**Goal**: The same node and wiring drive image input on llama.cpp too, via its OpenAI-compatible content-parts shape — proving the generic-node promise (ADR-007/008) holds for the image path.
|
||||
|
||||
**Independent Test**: Run the US1 workflow unchanged against a llama.cpp server (launched with `--mmproj`); swap the Ollama client node for the llama.cpp one with no other change and confirm the image is still described.
|
||||
|
||||
- [x] T006-T [P] [US2] Write FAILING test: `LlamaCppProvider.chat()` maps a message's images into OpenAI `content` parts (`{"type":"text",...}` + `{"type":"image_url","image_url":{"url":"data:image/png;base64,..."}}`) for `/v1/chat/completions`, and a text-only message keeps a **plain-string** `content` (regression), in `tests/test_llamacpp_provider.py` (contract T3; witnesses both `features/us2_both_backends.feature` scenarios)
|
||||
- [x] T006-I [US2] Implement the images→content-parts mapping in `LlamaCppProvider.chat()` in `src/comfydv/_llm/llamacpp_provider.py`; leave text-only messages untouched — makes T006-T pass (depends on T002-I)
|
||||
|
||||
**Checkpoint**: parity proven — the identical node/wiring describes an image on both backends; swapping the client node is the only change.
|
||||
|
||||
---
|
||||
|
||||
## Phase 5: User Story 3 — Structured output about an image (Priority: P2)
|
||||
|
||||
**Goal**: Image input works with the node's existing structured-output mode, via the shared `chat_structured()` helper (pydantic-ai `BinaryContent`) — one implementation covering both backends through `OpenAIChatModel`.
|
||||
|
||||
**Independent Test**: Enable structured output with a schema, wire an image, run against a vision model, confirm each field is populated from the image with no required field blank; a first-invalid response retries then fails clearly.
|
||||
|
||||
- [x] T007-T [US3] Write FAILING test: `chat_structured()` attaches a message's images as `BinaryContent(data=b64decode(img), media_type="image/png")` onto the run's `user_prompt` (last turn) and onto history `UserPromptPart`s, a text-only structured call is unchanged, and the retry/validation contract is intact, in `tests/test_llm_chat_structured.py` (contract T4; witnesses both `features/us3_structured_image.feature` scenarios) — mock at the `Agent.run`/`_build_agent` seam per the established convention
|
||||
- [x] T007-I [US3] Implement image→`BinaryContent` handling in `chat_structured()` and `_history_to_messages()` in `src/comfydv/_llm/chat.py` — makes T007-T pass (depends on T002-I)
|
||||
|
||||
**Checkpoint**: structured image output works on both backends via the one shared helper; all prior stories remain green.
|
||||
|
||||
---
|
||||
|
||||
## Phase 6: Polish & Cross-Cutting Concerns
|
||||
|
||||
- [x] T008 [P] Document image input on `ChatCompletion` in `README.md` and add a `CHANGELOG.md` Unreleased entry — note the vision-model / llama.cpp `--mmproj` prerequisite (quickstart.md)
|
||||
- [x] T009 Run the full quality gate: `ruff check` ✓, `ruff format` ✓, `pytest` ✓ (289 passed, +21 new; all spec-009 code green), `beacon doctor --strict` ✓ for this spec (bullet + BDD + backlinks pass). _Pre-existing, out of scope: `ty check` has 36 diagnostics repo-wide (0 from spec-009 code — verified), one Docker packaging test (`test_dockerfile_uses_python_311_base`) fails at baseline, and `spec-task-alignment` flags 007's deferred tasks under --strict._
|
||||
- [-] T010 End-to-end `quickstart.md` validation against a live vision backend (Ollama multimodal model and `llama-server --mmproj`) _Deferred — requires a live vision-capable backend not available in CI/this environment; validate manually before release._
|
||||
|
||||
---
|
||||
|
||||
## Dependencies & Execution Order
|
||||
|
||||
- **Setup (T001)** → no dependencies; start immediately.
|
||||
- **Foundational (T002-T/I)** → depends on nothing; **blocks all user stories** (every path reads `Message.images`).
|
||||
- **US1 (T003–T005)** → after T002-I. `T004-I` depends on `T003-I`; `T005-I` depends on `T002-I`. MVP.
|
||||
- **US2 (T006)** → after T002-I. Independent of US1's files; independently testable.
|
||||
- **US3 (T007)** → after T002-I. Independent of US1/US2's files; independently testable.
|
||||
- **Polish (T008–T010)** → after the stories you intend to ship.
|
||||
|
||||
### Within each story
|
||||
|
||||
- The `-T` task is written and committed **failing** before its `-I` partner (`tdd-commit-discipline`).
|
||||
- `-I` is never `[P]` with its own `-T`.
|
||||
|
||||
### Parallel opportunities
|
||||
|
||||
- US1: `T003-T` (`tests/test_ollama.py`) and `T005-T` (`tests/test_ollama_provider.py`) are different files → `[P]`.
|
||||
- Across stories: US1, US2, US3 touch different provider/helper files and can proceed in parallel once T002-I lands.
|
||||
|
||||
---
|
||||
|
||||
## Parallel Example: User Story 1
|
||||
|
||||
```bash
|
||||
# Different test files, no shared deps — write both failing tests together:
|
||||
Task: "T003-T encode-helper test in tests/test_ollama.py"
|
||||
Task: "T005-T Ollama image-passthrough test in tests/test_ollama_provider.py"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Implementation Strategy
|
||||
|
||||
### MVP first (US1 only)
|
||||
|
||||
1. T001 Setup → T002 carrier → T003–T005 US1.
|
||||
2. **STOP and VALIDATE**: an Ollama vision model describes a wired image; every text-only test stays green.
|
||||
3. Demoable as-is.
|
||||
|
||||
### Incremental delivery
|
||||
|
||||
1. Foundation + US1 → describe-an-image on Ollama (MVP).
|
||||
2. + US2 → same node works on llama.cpp (parity).
|
||||
3. + US3 → structured output about an image (both backends).
|
||||
4. Polish → docs, quality gate, manual live validation (T010).
|
||||
|
||||
---
|
||||
|
||||
## Notes
|
||||
|
||||
- `[-]` (T010) is a **known-deferred** follow-up — `beacon bullet finish` skips it rather than flipping to `[x]`; `beacon doctor` reports it as deferred, held under `--strict`.
|
||||
- `beacon doctor` runs two gates against this discipline: `spec-bdd-coverage` (every acceptance scenario has a `.feature` witness — 6 scenarios across 3 features here) and `tdd-commit-discipline` (no test + implementation in the same commit). Both FAIL under `--strict`.
|
||||
- Commit after each task or `-T`/`-I` pair; keep existing Ollama/llama.cpp/text tests green throughout (FR-003/SC-004 regression guard).
|
||||
@@ -20,6 +20,7 @@ from pydantic import BaseModel, ValidationError
|
||||
from pydantic_ai import Agent
|
||||
from pydantic_ai.exceptions import ModelRetry, UnexpectedModelBehavior
|
||||
from pydantic_ai.messages import (
|
||||
BinaryContent,
|
||||
ModelRequest,
|
||||
ModelResponse,
|
||||
SystemPromptPart,
|
||||
@@ -60,6 +61,28 @@ def _build_agent(
|
||||
return Agent(chat_model, output_type=schema, retries=0)
|
||||
|
||||
|
||||
def _user_prompt_content(msg: Message):
|
||||
"""Render a user turn as pydantic-ai user-prompt content.
|
||||
|
||||
Text-only ``msg`` → the plain ``content`` string, byte-identical to the
|
||||
pre-009 path (FR-003). A turn carrying images → ``[content, *images]``
|
||||
where each image is a ``BinaryContent`` PNG (ADR-008 / research.md
|
||||
Decision 1); ``OpenAIChatModel`` renders these as OpenAI ``image_url``
|
||||
parts, so both backends reach the same multimodal request through one
|
||||
shared code path.
|
||||
"""
|
||||
if not msg.images:
|
||||
return msg.content
|
||||
import base64
|
||||
|
||||
content: list = [msg.content]
|
||||
for image in msg.images:
|
||||
content.append(
|
||||
BinaryContent(data=base64.b64decode(image), media_type="image/png")
|
||||
)
|
||||
return content
|
||||
|
||||
|
||||
def _history_to_messages(messages: list[Message]) -> list:
|
||||
"""Convert all but the last message into pydantic-ai's typed history.
|
||||
|
||||
@@ -73,7 +96,9 @@ def _history_to_messages(messages: list[Message]) -> list:
|
||||
elif msg.role == "system":
|
||||
history.append(ModelRequest(parts=[SystemPromptPart(msg.content)]))
|
||||
else:
|
||||
history.append(ModelRequest(parts=[UserPromptPart(msg.content)]))
|
||||
history.append(
|
||||
ModelRequest(parts=[UserPromptPart(_user_prompt_content(msg))])
|
||||
)
|
||||
return history
|
||||
|
||||
|
||||
@@ -116,7 +141,7 @@ async def chat_structured(
|
||||
timeout_secs=timeout_secs,
|
||||
)
|
||||
history = _history_to_messages(messages)
|
||||
prompt = messages[-1].content
|
||||
prompt = _user_prompt_content(messages[-1])
|
||||
model_settings: ModelSettings | None = (
|
||||
{"extra_body": {"options": options}} if options else None
|
||||
)
|
||||
|
||||
@@ -53,6 +53,30 @@ async def _fetch_models(host: str, headers: dict | None = None) -> list[str]:
|
||||
return [m.name for m in models]
|
||||
|
||||
|
||||
def _to_openai_message(message: Message) -> dict:
|
||||
"""Render a ``Message`` in llama.cpp's OpenAI-compatible shape.
|
||||
|
||||
A text-only turn stays ``{"role", "content": <str>}`` — byte-identical to
|
||||
the pre-009 payload (FR-003). A turn carrying images becomes OpenAI
|
||||
multimodal ``content`` parts: the text followed by one ``image_url`` part
|
||||
per base64 image, as a ``data:`` URI (ADR-008). ``llama-server`` only
|
||||
honours these parts when launched with a multimodal projector
|
||||
(``--mmproj``); without it the server errors, surfaced to the caller
|
||||
rather than crashed on (FR-006).
|
||||
"""
|
||||
if not message.images:
|
||||
return {"role": message.role, "content": message.content}
|
||||
parts: list[dict] = [{"type": "text", "text": message.content}]
|
||||
for image in message.images:
|
||||
parts.append(
|
||||
{
|
||||
"type": "image_url",
|
||||
"image_url": {"url": f"data:image/png;base64,{image}"},
|
||||
}
|
||||
)
|
||||
return {"role": message.role, "content": parts}
|
||||
|
||||
|
||||
class LlamaCppProvider:
|
||||
"""LLMProvider implementation backed by llama-server's router mode.
|
||||
|
||||
@@ -160,7 +184,7 @@ class LlamaCppProvider:
|
||||
timeout_secs: float = 300.0,
|
||||
max_retries: int = 2,
|
||||
) -> str:
|
||||
payload_messages = [m.model_dump() for m in messages]
|
||||
payload_messages = [_to_openai_message(m) for m in messages]
|
||||
total_attempts = max(0, min(int(max_retries), 5)) + 1
|
||||
response_text = ""
|
||||
|
||||
|
||||
@@ -81,6 +81,7 @@ def _cache_key(*parts) -> str:
|
||||
|
||||
_MODEL_LIST_CACHE = _TTLLRUCache(maxsize=32, ttl_seconds=20.0)
|
||||
_CHAT_RESPONSE_CACHE = _TTLLRUCache(maxsize=64, ttl_seconds=None)
|
||||
_CAPABILITY_CACHE = _TTLLRUCache(maxsize=32, ttl_seconds=300.0)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -194,6 +195,49 @@ async def _fetch_models(host: str, headers: dict | None = None) -> list[str]:
|
||||
return models
|
||||
|
||||
|
||||
async def _require_vision_capability(
|
||||
host: str, model: str, headers: dict | None
|
||||
) -> None:
|
||||
"""Raise a clear error if ``model`` lacks Ollama's ``vision`` capability.
|
||||
|
||||
Only called when a request carries at least one image (spec 009 FR-006):
|
||||
Ollama's /api/chat silently accepts an unsupported ``images`` field and
|
||||
answers with a blank/malformed HTTP 200 instead of an error — which
|
||||
would otherwise be indistinguishable from an ordinary blank generation
|
||||
and get swallowed by chat()'s existing blank-response retry. /api/show's
|
||||
``capabilities`` list is the only place Ollama states support explicitly,
|
||||
so a request carrying an image is checked against it up front.
|
||||
|
||||
Fails open on any lookup problem (older Ollama without ``capabilities``,
|
||||
unreachable host, unexpected shape) — a lookup failure must not block a
|
||||
request that would otherwise have worked; the real request surfaces its
|
||||
own clear error if the host is genuinely unreachable.
|
||||
"""
|
||||
cache_key = _cache_key("capabilities", host, headers or {}, model)
|
||||
cached, hit = _CAPABILITY_CACHE.get(cache_key)
|
||||
if hit:
|
||||
capabilities = cached
|
||||
else:
|
||||
try:
|
||||
data = await _post_json(
|
||||
f"{host}/api/show", {"model": model}, timeout=10.0, headers=headers
|
||||
)
|
||||
except Exception:
|
||||
return
|
||||
capabilities = data.get("capabilities")
|
||||
if capabilities is None:
|
||||
return
|
||||
_CAPABILITY_CACHE.set(cache_key, capabilities)
|
||||
|
||||
if "vision" not in capabilities:
|
||||
raise ValueError(
|
||||
f"Model '{model}' does not support image input — Ollama reports "
|
||||
f"capabilities {capabilities!r} for it, no 'vision'. Wire a "
|
||||
"vision-capable model, or disconnect the image input for "
|
||||
"text-only chat."
|
||||
)
|
||||
|
||||
|
||||
class OllamaProvider:
|
||||
"""LLMProvider implementation backed by Ollama's REST API.
|
||||
|
||||
@@ -277,7 +321,14 @@ class OllamaProvider:
|
||||
timeout_secs: float = 300.0,
|
||||
max_retries: int = 2,
|
||||
) -> str:
|
||||
payload_messages = [m.model_dump() for m in messages]
|
||||
if any(m.images for m in messages):
|
||||
await _require_vision_capability(self.host, model, self.headers)
|
||||
|
||||
# exclude_none drops the images key for text-only turns so an
|
||||
# image-less request is byte-identical to the pre-009 payload
|
||||
# (FR-003); a turn with images keeps Ollama's native flat images
|
||||
# array (ADR-008 — no transform needed for /api/chat).
|
||||
payload_messages = [m.model_dump(exclude_none=True) for m in messages]
|
||||
total_attempts = max(0, min(int(max_retries), 5)) + 1
|
||||
response_text = ""
|
||||
incomplete = False
|
||||
@@ -353,6 +404,9 @@ class OllamaProvider:
|
||||
timeout_secs: float = 300.0,
|
||||
max_retries: int = 2,
|
||||
) -> BaseModel:
|
||||
if any(m.images for m in messages):
|
||||
await _require_vision_capability(self.host, model, self.headers)
|
||||
|
||||
from .chat import chat_structured as _chat_structured_impl
|
||||
|
||||
payload_messages = [m.model_dump() for m in messages]
|
||||
|
||||
@@ -39,10 +39,21 @@ class ModelInfo(BaseModel):
|
||||
|
||||
|
||||
class Message(BaseModel):
|
||||
"""One turn in a chat request."""
|
||||
"""One turn in a chat request.
|
||||
|
||||
``images`` carries optional base64-encoded image payloads (no ``data:``
|
||||
prefix) associated with this turn, for vision-capable models. ``None``
|
||||
(the default) means a text-only turn that serializes byte-for-byte as
|
||||
before — providers dump with ``exclude_none=True`` so no ``images`` key
|
||||
reaches the wire for image-less turns. Each provider translates this
|
||||
neutral carrier into its own native shape (ADR-008): Ollama's flat
|
||||
per-message ``images`` array, llama.cpp's OpenAI ``image_url`` content
|
||||
parts, and pydantic-ai ``BinaryContent`` on the structured path.
|
||||
"""
|
||||
|
||||
role: Literal["system", "user", "assistant"]
|
||||
content: str
|
||||
images: list[str] | None = None
|
||||
|
||||
|
||||
class LLMProvider(Protocol):
|
||||
|
||||
@@ -27,6 +27,47 @@ from ._llm.provider import Message
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def _encode_image_tensor(image) -> list[str]:
|
||||
"""Convert a ComfyUI ``IMAGE`` into base64-encoded PNG string(s).
|
||||
|
||||
ComfyUI passes images as a float tensor shaped ``[B, H, W, C]`` in the
|
||||
``0..1`` range. Each frame in the batch becomes one base64 PNG in the
|
||||
returned list (spec 009 / ADR-008: a batch is carried as multiple images
|
||||
on the turn). ``None`` or an empty tensor yields ``[]`` so callers can
|
||||
treat "no image wired" uniformly.
|
||||
|
||||
``numpy``/``PIL`` are imported lazily here — they are provided by the
|
||||
ComfyUI runtime, not a comfydv core dependency, and importing them at
|
||||
module scope would break loading outside ComfyUI (Constitution IV). The
|
||||
``_llm`` layer never touches tensors or Pillow; only this node does.
|
||||
"""
|
||||
if image is None:
|
||||
return []
|
||||
|
||||
import base64
|
||||
from io import BytesIO
|
||||
|
||||
import numpy as np
|
||||
from PIL import Image
|
||||
|
||||
arr = image
|
||||
if hasattr(arr, "detach"): # torch tensor
|
||||
arr = arr.detach().cpu().numpy()
|
||||
arr = np.asarray(arr)
|
||||
if arr.size == 0:
|
||||
return []
|
||||
if arr.ndim == 3: # a bare [H, W, C] — treat as a batch of one
|
||||
arr = arr[None, ...]
|
||||
|
||||
encoded: list[str] = []
|
||||
for frame in arr:
|
||||
frame_u8 = np.clip(frame * 255.0 + 0.5, 0, 255).astype(np.uint8)
|
||||
buffer = BytesIO()
|
||||
Image.fromarray(frame_u8).save(buffer, format="PNG")
|
||||
encoded.append(base64.b64encode(buffer.getvalue()).decode("ascii"))
|
||||
return encoded
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Migration mapping (ADR-007) — old Ollama-specific node/socket names to
|
||||
# their generic replacements, for anyone reconnecting a pre-upgrade
|
||||
@@ -461,6 +502,18 @@ class ChatCompletion:
|
||||
"system": ("STRING", {"multiline": True, "default": ""}),
|
||||
"history": ("OLLAMA_HISTORY",),
|
||||
"options": ("OLLAMA_OPTIONS",),
|
||||
"image": (
|
||||
"IMAGE",
|
||||
{
|
||||
"tooltip": (
|
||||
"Optional image(s) for a vision-capable model. "
|
||||
"Requires a multimodal model on the connected "
|
||||
"server (Ollama multimodal model, or llama.cpp "
|
||||
"launched with --mmproj). A batch is sent as "
|
||||
"multiple images on the turn."
|
||||
)
|
||||
},
|
||||
),
|
||||
"timeout_secs": ("INT", {"default": 300, "min": 30, "max": 3600}),
|
||||
"structured_output": ("BOOLEAN", {"default": False}),
|
||||
"output_schema": (
|
||||
@@ -509,6 +562,7 @@ class ChatCompletion:
|
||||
system="",
|
||||
history=None,
|
||||
options=None,
|
||||
image=None,
|
||||
timeout_secs=300,
|
||||
structured_output=False,
|
||||
output_schema=_DEFAULT_OUTPUT_SCHEMA,
|
||||
@@ -535,6 +589,12 @@ class ChatCompletion:
|
||||
message_dicts = [{"role": "system", "content": system}] + message_dicts
|
||||
message_dicts.append({"role": "user", "content": prompt})
|
||||
messages = [Message(**m) for m in message_dicts]
|
||||
# Attach any wired image(s) to the current user turn only (FR-007) —
|
||||
# history turns are left untouched. Encoding lives in the node
|
||||
# (comfy-guarded); providers see only base64 strings on the Message.
|
||||
user_images = _encode_image_tensor(image)
|
||||
if user_images:
|
||||
messages[-1].images = user_images
|
||||
llm_options = dict(options) if options else None
|
||||
|
||||
# Provider owns transport, caching, and — for structured_output — the
|
||||
|
||||
@@ -479,3 +479,55 @@ def test_fetch_models_degrades_to_empty_when_unreachable(monkeypatch):
|
||||
names = _run_async(_fetch_models("http://localhost:19999"))
|
||||
|
||||
assert names == []
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# chat — image input (spec 009, US2; features/us2_both_backends.feature)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_chat_maps_images_to_openai_content_parts(monkeypatch):
|
||||
"""llama.cpp's OpenAI-compatible endpoint takes images as image_url parts
|
||||
inside content, not a flat images field (ADR-008)."""
|
||||
captured = {}
|
||||
|
||||
async def fake_post(url, payload, *, timeout=120.0, headers=None):
|
||||
captured["payload"] = payload
|
||||
return {"choices": [{"message": {"content": "a red square"}}]}
|
||||
|
||||
monkeypatch.setattr(provider_mod, "_post_json", fake_post)
|
||||
_run_async(
|
||||
LlamaCppProvider("http://localhost:8080").chat(
|
||||
"m", [Message(role="user", content="describe", images=["QUJD"])]
|
||||
)
|
||||
)
|
||||
|
||||
assert captured["payload"]["messages"][-1] == {
|
||||
"role": "user",
|
||||
"content": [
|
||||
{"type": "text", "text": "describe"},
|
||||
{
|
||||
"type": "image_url",
|
||||
"image_url": {"url": "data:image/png;base64,QUJD"},
|
||||
},
|
||||
],
|
||||
}
|
||||
|
||||
|
||||
def test_chat_text_only_content_stays_plain_string(monkeypatch):
|
||||
"""FR-003/SC-004: an image-less message keeps a plain string content,
|
||||
byte-identical to today (no content-parts, no images key)."""
|
||||
captured = {}
|
||||
|
||||
async def fake_post(url, payload, *, timeout=120.0, headers=None):
|
||||
captured["payload"] = payload
|
||||
return {"choices": [{"message": {"content": "ok"}}]}
|
||||
|
||||
monkeypatch.setattr(provider_mod, "_post_json", fake_post)
|
||||
_run_async(
|
||||
LlamaCppProvider("http://localhost:8080").chat(
|
||||
"m", [Message(role="user", content="hi")]
|
||||
)
|
||||
)
|
||||
|
||||
assert captured["payload"]["messages"] == [{"role": "user", "content": "hi"}]
|
||||
|
||||
@@ -314,3 +314,84 @@ def test_history_to_messages_preserves_order_and_roles():
|
||||
assert isinstance(history[0], ModelRequest) # system
|
||||
assert isinstance(history[1], ModelRequest) # user
|
||||
assert isinstance(history[2], ModelResponse) # assistant
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Image input (spec 009, US3; features/us3_structured_image.feature)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_chat_structured_attaches_image_to_user_prompt(monkeypatch):
|
||||
"""The current turn's image rides on Agent.run()'s user_prompt as a
|
||||
pydantic-ai BinaryContent (ADR-008, research.md Decision 1)."""
|
||||
import base64
|
||||
|
||||
from pydantic_ai.messages import BinaryContent
|
||||
|
||||
fake = _FakeAgent([_Widget(name="sq", count=1)])
|
||||
monkeypatch.setattr(chat_mod, "_build_agent", lambda **kw: fake)
|
||||
b64 = base64.b64encode(b"PNGDATA").decode()
|
||||
|
||||
_run_async(
|
||||
chat_mod.chat_structured(
|
||||
base_url="http://x/v1",
|
||||
model="m",
|
||||
schema=_Widget,
|
||||
messages=[Message(role="user", content="describe", images=[b64])],
|
||||
)
|
||||
)
|
||||
|
||||
prompt = fake.calls[0][0]
|
||||
assert isinstance(prompt, list)
|
||||
assert prompt[0] == "describe"
|
||||
assert isinstance(prompt[1], BinaryContent)
|
||||
assert prompt[1].data == b"PNGDATA"
|
||||
assert prompt[1].media_type == "image/png"
|
||||
|
||||
|
||||
def test_chat_structured_text_only_prompt_is_plain_string(monkeypatch):
|
||||
"""FR-003: an image-less structured call is unchanged — plain-string
|
||||
user_prompt, exactly as before spec 009."""
|
||||
fake = _FakeAgent([_Widget(name="a", count=1)])
|
||||
monkeypatch.setattr(chat_mod, "_build_agent", lambda **kw: fake)
|
||||
|
||||
_run_async(
|
||||
chat_mod.chat_structured(
|
||||
base_url="http://x/v1",
|
||||
model="m",
|
||||
schema=_Widget,
|
||||
messages=[Message(role="user", content="hi")],
|
||||
)
|
||||
)
|
||||
|
||||
assert fake.calls[0][0] == "hi"
|
||||
|
||||
|
||||
def test_chat_structured_attaches_image_to_history_user_turn(monkeypatch):
|
||||
"""A prior user turn that carried an image keeps it in message_history."""
|
||||
import base64
|
||||
|
||||
from pydantic_ai.messages import BinaryContent, UserPromptPart
|
||||
|
||||
fake = _FakeAgent([_Widget(name="a", count=1)])
|
||||
monkeypatch.setattr(chat_mod, "_build_agent", lambda **kw: fake)
|
||||
b64 = base64.b64encode(b"IMG").decode()
|
||||
msgs = [
|
||||
Message(role="user", content="earlier", images=[b64]),
|
||||
Message(role="assistant", content="ok"),
|
||||
Message(role="user", content="now"),
|
||||
]
|
||||
|
||||
_run_async(
|
||||
chat_mod.chat_structured(
|
||||
base_url="http://x/v1", model="m", schema=_Widget, messages=msgs
|
||||
)
|
||||
)
|
||||
|
||||
history = fake.calls[0][1]
|
||||
part = history[0].parts[0]
|
||||
assert isinstance(part, UserPromptPart)
|
||||
assert isinstance(part.content, list)
|
||||
assert part.content[0] == "earlier"
|
||||
assert isinstance(part.content[1], BinaryContent)
|
||||
assert part.content[1].data == b"IMG"
|
||||
|
||||
@@ -41,3 +41,34 @@ def test_message_roles():
|
||||
Message(role="system", content="be terse")
|
||||
Message(role="user", content="hi")
|
||||
Message(role="assistant", content="hello")
|
||||
|
||||
|
||||
# --- US1 foundational: Message.images carrier (spec 009, contract T1) ---
|
||||
|
||||
|
||||
def test_message_images_defaults_to_none():
|
||||
"""A text-only turn carries no images."""
|
||||
msg = Message(role="user", content="hi")
|
||||
assert msg.images is None
|
||||
|
||||
|
||||
def test_message_images_round_trips_base64_list():
|
||||
msg = Message(role="user", content="describe", images=["aGVsbG8=", "d29ybGQ="])
|
||||
assert msg.images == ["aGVsbG8=", "d29ybGQ="]
|
||||
|
||||
|
||||
def test_message_text_only_dump_omits_images_key():
|
||||
"""FR-003/SC-004: an image-less message must serialize byte-identically to
|
||||
today — no stray ``images`` key in the transport payload."""
|
||||
msg = Message(role="user", content="hi")
|
||||
assert msg.model_dump(exclude_none=True) == {"role": "user", "content": "hi"}
|
||||
|
||||
|
||||
def test_message_with_images_dump_includes_images_key():
|
||||
msg = Message(role="user", content="describe", images=["aGVsbG8="])
|
||||
dumped = msg.model_dump(exclude_none=True)
|
||||
assert dumped == {
|
||||
"role": "user",
|
||||
"content": "describe",
|
||||
"images": ["aGVsbG8="],
|
||||
}
|
||||
|
||||
@@ -1089,3 +1089,121 @@ class TestNodeContracts:
|
||||
types = node_cls.INPUT_TYPES()
|
||||
assert isinstance(types, dict)
|
||||
assert "required" in types or "optional" in types
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# US1 (spec 009) — Image input on ChatCompletion
|
||||
# features/us1_describe_image.feature
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestUS1ImageEncode:
|
||||
"""_encode_image_tensor(): ComfyUI IMAGE tensor -> base64 PNG(s)."""
|
||||
|
||||
def test_encode_single_image_returns_decodable_png(self):
|
||||
import base64
|
||||
import io
|
||||
|
||||
import torch
|
||||
from PIL import Image
|
||||
|
||||
from comfydv.ollama import _encode_image_tensor
|
||||
|
||||
# ComfyUI IMAGE: [B, H, W, C] float 0..1
|
||||
img = torch.zeros(1, 4, 8, 3)
|
||||
img[0, :, :, 0] = 1.0 # solid red
|
||||
|
||||
out = _encode_image_tensor(img)
|
||||
|
||||
assert isinstance(out, list)
|
||||
assert len(out) == 1
|
||||
pil = Image.open(io.BytesIO(base64.b64decode(out[0])))
|
||||
assert pil.format == "PNG"
|
||||
assert pil.size == (8, 4) # PIL size is (W, H)
|
||||
assert pil.convert("RGB").getpixel((0, 0)) == (255, 0, 0)
|
||||
|
||||
def test_encode_batch_returns_one_base64_per_frame(self):
|
||||
import torch
|
||||
|
||||
from comfydv.ollama import _encode_image_tensor
|
||||
|
||||
img = torch.zeros(3, 4, 8, 3)
|
||||
out = _encode_image_tensor(img)
|
||||
assert len(out) == 3
|
||||
|
||||
def test_encode_none_returns_empty_list(self):
|
||||
from comfydv.ollama import _encode_image_tensor
|
||||
|
||||
assert _encode_image_tensor(None) == []
|
||||
|
||||
def test_encode_empty_batch_returns_empty_list(self):
|
||||
import torch
|
||||
|
||||
from comfydv.ollama import _encode_image_tensor
|
||||
|
||||
assert _encode_image_tensor(torch.zeros(0, 4, 8, 3)) == []
|
||||
|
||||
|
||||
class TestUS1ImageInputNode:
|
||||
"""ChatCompletion optional IMAGE input attaches to the current user turn."""
|
||||
|
||||
def _last_messages(self, fake):
|
||||
# _FakeProvider records ("chat", model, messages, options, timeout, retries)
|
||||
chat_calls = [c for c in fake.calls if c[0] == "chat"]
|
||||
return chat_calls[-1][2]
|
||||
|
||||
def test_chat_completion_has_optional_image_input(self):
|
||||
inputs = ChatCompletion.INPUT_TYPES()
|
||||
assert "image" in inputs["optional"], (
|
||||
"ChatCompletion must expose an optional IMAGE input for VLM use"
|
||||
)
|
||||
assert inputs["optional"]["image"][0] == "IMAGE"
|
||||
|
||||
def test_chat_completion_image_input_is_not_required(self):
|
||||
inputs = ChatCompletion.INPUT_TYPES()
|
||||
assert "image" not in inputs.get("required", {})
|
||||
|
||||
def test_return_positions_unchanged_by_image_input(self):
|
||||
# Constitution VI: outputs untouched — only an optional input is added.
|
||||
assert ChatCompletion.RETURN_TYPES[:2] == ("STRING", "OLLAMA_HISTORY")
|
||||
assert ChatCompletion.RETURN_NAMES[:2] == ("response", "updated_history")
|
||||
|
||||
def test_wired_image_attaches_to_last_user_message(self):
|
||||
import torch
|
||||
|
||||
fake = _FakeProvider(chat_response="a red square")
|
||||
img = torch.zeros(1, 4, 8, 3)
|
||||
img[0, :, :, 0] = 1.0
|
||||
|
||||
ChatCompletion().chat(client=fake, model="m", prompt="describe", image=img)
|
||||
|
||||
messages = self._last_messages(fake)
|
||||
assert messages[-1].role == "user"
|
||||
assert messages[-1].content == "describe"
|
||||
assert messages[-1].images and len(messages[-1].images) == 1
|
||||
|
||||
def test_unwired_image_leaves_messages_text_only(self):
|
||||
fake = _FakeProvider(chat_response="ok")
|
||||
ChatCompletion().chat(client=fake, model="m", prompt="hi")
|
||||
messages = self._last_messages(fake)
|
||||
assert messages[-1].images is None
|
||||
|
||||
def test_image_not_added_to_history_turns(self):
|
||||
import torch
|
||||
|
||||
fake = _FakeProvider(chat_response="ok")
|
||||
img = torch.zeros(1, 2, 2, 3)
|
||||
history = [
|
||||
{"role": "user", "content": "earlier q"},
|
||||
{"role": "assistant", "content": "earlier a"},
|
||||
]
|
||||
|
||||
ChatCompletion().chat(
|
||||
client=fake, model="m", prompt="now", history=history, image=img
|
||||
)
|
||||
|
||||
messages = self._last_messages(fake)
|
||||
# Every turn except the final user turn must carry no image (FR-007).
|
||||
assert all(m.images is None for m in messages[:-1])
|
||||
assert messages[-1].content == "now"
|
||||
assert messages[-1].images and len(messages[-1].images) == 1
|
||||
|
||||
@@ -25,9 +25,11 @@ from comfydv._llm.provider import Message, ModelStatus
|
||||
def _clear_provider_caches():
|
||||
provider_mod._MODEL_LIST_CACHE.clear()
|
||||
provider_mod._CHAT_RESPONSE_CACHE.clear()
|
||||
provider_mod._CAPABILITY_CACHE.clear()
|
||||
yield
|
||||
provider_mod._MODEL_LIST_CACHE.clear()
|
||||
provider_mod._CHAT_RESPONSE_CACHE.clear()
|
||||
provider_mod._CAPABILITY_CACHE.clear()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -626,3 +628,146 @@ def test_run_async_propagates_exceptions_from_within_a_running_loop():
|
||||
|
||||
with pytest.raises(ValueError, match="boom"):
|
||||
asyncio.run(outer())
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# chat — image input (spec 009, US1; features/us1_describe_image.feature)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_chat_forwards_images_as_flat_array(monkeypatch):
|
||||
"""Ollama /api/chat takes images as a flat per-message base64 array."""
|
||||
captured = {}
|
||||
|
||||
async def fake_post(url, payload, *, timeout=120.0, headers=None):
|
||||
if url.endswith("/api/show"):
|
||||
return {"capabilities": ["completion", "vision"]}
|
||||
captured["payload"] = payload
|
||||
return {"message": {"content": "a red square"}}
|
||||
|
||||
monkeypatch.setattr(provider_mod, "_post_json", fake_post)
|
||||
_run_async(
|
||||
OllamaProvider("http://localhost:11434").chat(
|
||||
"m", [Message(role="user", content="describe", images=["QUJD"])]
|
||||
)
|
||||
)
|
||||
|
||||
assert captured["payload"]["messages"][-1] == {
|
||||
"role": "user",
|
||||
"content": "describe",
|
||||
"images": ["QUJD"],
|
||||
}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# chat — non-vision model + image (spec 009 FR-006/SC-005)
|
||||
# ---------------------------------------------------------------------------
|
||||
#
|
||||
# Live-testing against a real Ollama server surfaced the gap these tests
|
||||
# lock in: /api/chat doesn't error for a non-vision model given an `images`
|
||||
# field — it answers HTTP 200 with a blank message, indistinguishable from
|
||||
# an ordinary blank generation and silently swallowed by chat()'s existing
|
||||
# blank-response retry. FR-006 requires a clear, specific error instead.
|
||||
|
||||
|
||||
def test_chat_raises_clear_error_for_non_vision_model_with_image(monkeypatch):
|
||||
async def fake_post(url, payload, *, timeout=120.0, headers=None):
|
||||
if url.endswith("/api/show"):
|
||||
return {"capabilities": ["completion"]}
|
||||
raise AssertionError(f"/api/chat must not be called: {url}")
|
||||
|
||||
monkeypatch.setattr(provider_mod, "_post_json", fake_post)
|
||||
|
||||
with pytest.raises(ValueError, match="does not support image input"):
|
||||
_run_async(
|
||||
OllamaProvider("http://localhost:11434").chat(
|
||||
"text-only-model",
|
||||
[Message(role="user", content="describe", images=["QUJD"])],
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
def test_chat_skips_capability_check_when_no_image(monkeypatch):
|
||||
"""FR-003: a text-only request must not pay for (or trigger) the
|
||||
capability lookup at all — /api/show is never called."""
|
||||
calls = []
|
||||
|
||||
async def fake_post(url, payload, *, timeout=120.0, headers=None):
|
||||
calls.append(url)
|
||||
return {"message": {"content": "hi"}}
|
||||
|
||||
monkeypatch.setattr(provider_mod, "_post_json", fake_post)
|
||||
_run_async(
|
||||
OllamaProvider("http://localhost:11434").chat(
|
||||
"m", [Message(role="user", content="hi")]
|
||||
)
|
||||
)
|
||||
|
||||
assert all(not url.endswith("/api/show") for url in calls)
|
||||
|
||||
|
||||
def test_chat_capability_check_fails_open_on_lookup_error(monkeypatch):
|
||||
"""A capability-lookup failure (older Ollama, network hiccup) must not
|
||||
block a request that would otherwise have worked."""
|
||||
|
||||
async def fake_post(url, payload, *, timeout=120.0, headers=None):
|
||||
if url.endswith("/api/show"):
|
||||
raise RuntimeError("Ollama returned HTTP 404 for /api/show")
|
||||
return {"message": {"content": "a red square"}}
|
||||
|
||||
monkeypatch.setattr(provider_mod, "_post_json", fake_post)
|
||||
result = _run_async(
|
||||
OllamaProvider("http://localhost:11434").chat(
|
||||
"m", [Message(role="user", content="describe", images=["QUJD"])]
|
||||
)
|
||||
)
|
||||
|
||||
assert result == "a red square"
|
||||
|
||||
|
||||
def test_chat_structured_raises_clear_error_for_non_vision_model_with_image(
|
||||
monkeypatch,
|
||||
):
|
||||
async def fake_post(url, payload, *, timeout=120.0, headers=None):
|
||||
if url.endswith("/api/show"):
|
||||
return {"capabilities": ["completion"]}
|
||||
raise AssertionError(f"unexpected _post_json call: {url}")
|
||||
|
||||
async def fail_if_called(**kwargs):
|
||||
raise AssertionError("chat_structured must not be reached")
|
||||
|
||||
monkeypatch.setattr(provider_mod, "_post_json", fake_post)
|
||||
monkeypatch.setattr("comfydv._llm.chat.chat_structured", fail_if_called)
|
||||
|
||||
from pydantic import BaseModel
|
||||
|
||||
class Widget(BaseModel):
|
||||
name: str
|
||||
|
||||
with pytest.raises(ValueError, match="does not support image input"):
|
||||
_run_async(
|
||||
OllamaProvider("http://localhost:11434").chat_structured(
|
||||
"text-only-model",
|
||||
[Message(role="user", content="describe", images=["QUJD"])],
|
||||
Widget,
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
def test_chat_text_only_payload_omits_images_key(monkeypatch):
|
||||
"""FR-003/SC-004: an image-less request must be byte-identical to today —
|
||||
no stray images key in the /api/chat payload."""
|
||||
captured = {}
|
||||
|
||||
async def fake_post(url, payload, *, timeout=120.0, headers=None):
|
||||
captured["payload"] = payload
|
||||
return {"message": {"content": "ok"}}
|
||||
|
||||
monkeypatch.setattr(provider_mod, "_post_json", fake_post)
|
||||
_run_async(
|
||||
OllamaProvider("http://localhost:11434").chat(
|
||||
"m", [Message(role="user", content="hi")]
|
||||
)
|
||||
)
|
||||
|
||||
assert captured["payload"]["messages"] == [{"role": "user", "content": "hi"}]
|
||||
|
||||
@@ -273,6 +273,7 @@ dependencies = [
|
||||
|
||||
[package.dev-dependencies]
|
||||
dev = [
|
||||
{ name = "pillow" },
|
||||
{ name = "playwright" },
|
||||
{ name = "pytest" },
|
||||
{ name = "pytest-cov" },
|
||||
@@ -301,6 +302,7 @@ requires-dist = [
|
||||
|
||||
[package.metadata.requires-dev]
|
||||
dev = [
|
||||
{ name = "pillow", specifier = ">=10.0.0" },
|
||||
{ name = "playwright", specifier = ">=1.60.0" },
|
||||
{ name = "pytest", specifier = ">=8.4.2" },
|
||||
{ name = "pytest-cov", specifier = ">=6.0.0" },
|
||||
|
||||
Reference in New Issue
Block a user