diff --git a/.specify/feature.json b/.specify/feature.json index 28d7a7e..1452fe3 100644 --- a/.specify/feature.json +++ b/.specify/feature.json @@ -1,3 +1,3 @@ { - "feature_directory": "specs/008-llamacpp-integration" + "feature_directory": "specs/009-vlm-image-input" } diff --git a/CHANGELOG.md b/CHANGELOG.md index d337098..b854dac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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). diff --git a/CLAUDE.md b/CLAUDE.md index ef142f2..3c8447f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,5 +1,5 @@ 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 diff --git a/README.md b/README.md index ca4a1f8..e1712dc 100644 --- a/README.md +++ b/README.md @@ -114,6 +114,15 @@ A complete graph looks like this: ![Full LLM workflow](docs/assets/ollama_workflow.png) +### 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 ` 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. diff --git a/project-management/.beacon/bullets.toml b/project-management/.beacon/bullets.toml index e60d5c1..1418c7c 100644 --- a/project-management/.beacon/bullets.toml +++ b/project-management/.beacon/bullets.toml @@ -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" diff --git a/project-management/ADRs/ADR-008-multimodal-image-input-across-llmprovider-boundary.md b/project-management/ADRs/ADR-008-multimodal-image-input-across-llmprovider-boundary.md new file mode 100644 index 0000000..38f113c --- /dev/null +++ b/project-management/ADRs/ADR-008-multimodal-image-input-across-llmprovider-boundary.md @@ -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 diff --git a/project-management/ADRs/README.md b/project-management/ADRs/README.md index 4c36f18..8c016cc 100644 --- a/project-management/ADRs/README.md +++ b/project-management/ADRs/README.md @@ -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 | diff --git a/project-management/Roadmap/README.md b/project-management/Roadmap/README.md index 5b73915..2701ec2 100644 --- a/project-management/Roadmap/README.md +++ b/project-management/Roadmap/README.md @@ -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): diff --git a/project-management/Roadmap/epics/vlm-image-input.md b/project-management/Roadmap/epics/vlm-image-input.md new file mode 100644 index 0000000..aa9b1c4 --- /dev/null +++ b/project-management/Roadmap/epics/vlm-image-input.md @@ -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. diff --git a/pyproject.toml b/pyproject.toml index bfaf05a..7332c81 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -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", diff --git a/specs/009-vlm-image-input/.beacon.toml b/specs/009-vlm-image-input/.beacon.toml new file mode 100644 index 0000000..23bc8df --- /dev/null +++ b/specs/009-vlm-image-input/.beacon.toml @@ -0,0 +1 @@ +epic = "vlm-image-input" diff --git a/specs/009-vlm-image-input/checklists/requirements.md b/specs/009-vlm-image-input/checklists/requirements.md new file mode 100644 index 0000000..7682c96 --- /dev/null +++ b/specs/009-vlm-image-input/checklists/requirements.md @@ -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). diff --git a/specs/009-vlm-image-input/contracts/image-input-contract.md b/specs/009-vlm-image-input/contracts/image-input-contract.md new file mode 100644 index 0000000..32fa648 --- /dev/null +++ b/specs/009-vlm-image-input/contracts/image-input-contract.md @@ -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. diff --git a/specs/009-vlm-image-input/data-model.md b/specs/009-vlm-image-input/data-model.md new file mode 100644 index 0000000..af9e48c --- /dev/null +++ b/specs/009-vlm-image-input/data-model.md @@ -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,"}}]` | +| 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). diff --git a/specs/009-vlm-image-input/features/us1_describe_image.feature b/specs/009-vlm-image-input/features/us1_describe_image.feature new file mode 100644 index 0000000..0448102 --- /dev/null +++ b/specs/009-vlm-image-input/features/us1_describe_image.feature @@ -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 diff --git a/specs/009-vlm-image-input/features/us2_both_backends.feature b/specs/009-vlm-image-input/features/us2_both_backends.feature new file mode 100644 index 0000000..1d6de31 --- /dev/null +++ b/specs/009-vlm-image-input/features/us2_both_backends.feature @@ -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 diff --git a/specs/009-vlm-image-input/features/us3_structured_image.feature b/specs/009-vlm-image-input/features/us3_structured_image.feature new file mode 100644 index 0000000..cca7d52 --- /dev/null +++ b/specs/009-vlm-image-input/features/us3_structured_image.feature @@ -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 diff --git a/specs/009-vlm-image-input/plan.md b/specs/009-vlm-image-input/plan.md new file mode 100644 index 0000000..835ac87 --- /dev/null +++ b/specs/009-vlm-image-input/plan.md @@ -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. diff --git a/specs/009-vlm-image-input/quickstart.md b/specs/009-vlm-image-input/quickstart.md new file mode 100644 index 0000000..d296dda --- /dev/null +++ b/specs/009-vlm-image-input/quickstart.md @@ -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 ` 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. diff --git a/specs/009-vlm-image-input/research.md b/specs/009-vlm-image-input/research.md new file mode 100644 index 0000000..87f5607 --- /dev/null +++ b/specs/009-vlm-image-input/research.md @@ -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=, +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": [, ...]}` — 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":}, +{"type":"image_url","image_url":{"url":"data:image/png;base64,"}}]`. +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. diff --git a/specs/009-vlm-image-input/spec.md b/specs/009-vlm-image-input/spec.md new file mode 100644 index 0000000..6cdafc4 --- /dev/null +++ b/specs/009-vlm-image-input/spec.md @@ -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. diff --git a/specs/009-vlm-image-input/tasks.md b/specs/009-vlm-image-input/tasks.md new file mode 100644 index 0000000..78a440a --- /dev/null +++ b/specs/009-vlm-image-input/tasks.md @@ -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). diff --git a/src/comfydv/_llm/chat.py b/src/comfydv/_llm/chat.py index dca8bef..1c78793 100644 --- a/src/comfydv/_llm/chat.py +++ b/src/comfydv/_llm/chat.py @@ -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 ) diff --git a/src/comfydv/_llm/llamacpp_provider.py b/src/comfydv/_llm/llamacpp_provider.py index dc55e4a..8048ca3 100644 --- a/src/comfydv/_llm/llamacpp_provider.py +++ b/src/comfydv/_llm/llamacpp_provider.py @@ -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": }`` — 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 = "" diff --git a/src/comfydv/_llm/ollama_provider.py b/src/comfydv/_llm/ollama_provider.py index b9ce828..94f32e5 100644 --- a/src/comfydv/_llm/ollama_provider.py +++ b/src/comfydv/_llm/ollama_provider.py @@ -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] diff --git a/src/comfydv/_llm/provider.py b/src/comfydv/_llm/provider.py index 353e594..25f65e5 100644 --- a/src/comfydv/_llm/provider.py +++ b/src/comfydv/_llm/provider.py @@ -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): diff --git a/src/comfydv/ollama.py b/src/comfydv/ollama.py index f010553..9d133d3 100644 --- a/src/comfydv/ollama.py +++ b/src/comfydv/ollama.py @@ -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 diff --git a/tests/test_llamacpp_provider.py b/tests/test_llamacpp_provider.py index 89e57ae..4aa6ecb 100644 --- a/tests/test_llamacpp_provider.py +++ b/tests/test_llamacpp_provider.py @@ -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"}] diff --git a/tests/test_llm_chat_structured.py b/tests/test_llm_chat_structured.py index ea57ed6..24b7422 100644 --- a/tests/test_llm_chat_structured.py +++ b/tests/test_llm_chat_structured.py @@ -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" diff --git a/tests/test_llm_provider.py b/tests/test_llm_provider.py index 78af9b9..d10acea 100644 --- a/tests/test_llm_provider.py +++ b/tests/test_llm_provider.py @@ -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="], + } diff --git a/tests/test_ollama.py b/tests/test_ollama.py index 8577f4f..d8b962d 100644 --- a/tests/test_ollama.py +++ b/tests/test_ollama.py @@ -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 diff --git a/tests/test_ollama_provider.py b/tests/test_ollama_provider.py index 6b3d73c..314d8f2 100644 --- a/tests/test_ollama_provider.py +++ b/tests/test_ollama_provider.py @@ -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"}] diff --git a/uv.lock b/uv.lock index 167e86e..5121ab8 100644 --- a/uv.lock +++ b/uv.lock @@ -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" },