docs(beacon): DESIGN artifacts for LLM provider abstraction (ADR-007)
Adopt a shared LLMProvider protocol (list/load/unload/chat/structured-chat) as the adapter pattern answer to GitHub issue #15 — Ollama and llama.cpp share the chat/structured-output mechanism via pydantic-ai while model lifecycle stays backend-specific per provider. Supersedes ADR-006, narrows ADR-004's scope to non-chat REST calls. Adds epic 007 (llm-provider-abstraction, prerequisite) and the llamacpp-integration epic it unblocks, plus the full spec/plan/tasks/BDD scaffold for the prerequisite epic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
This commit is contained in:
James Veitch
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
b3374c88c4
commit
fb0556fff3
@@ -1,3 +1,3 @@
|
||||
{
|
||||
"feature_directory": "specs/006-ollama-model-integration"
|
||||
"feature_directory": "specs/007-llm-provider-abstraction"
|
||||
}
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
<!-- SPECKIT START -->
|
||||
For additional context about technologies to be used, project structure,
|
||||
shell commands, and other important information, read the current plan
|
||||
at specs/006-ollama-model-integration/plan.md
|
||||
at specs/007-llm-provider-abstraction/plan.md
|
||||
<!-- SPECKIT END -->
|
||||
|
||||
+1
-1
@@ -2,7 +2,7 @@
|
||||
|
||||
## Status
|
||||
|
||||
> Accepted
|
||||
> Superseded by [ADR-007](ADR-007-llm-provider-adapter-pattern.md)
|
||||
|
||||
_Date:_ 2026-07-09
|
||||
_Deciders:_ darth-veitcher
|
||||
|
||||
@@ -0,0 +1,198 @@
|
||||
# ADR-007: LLMProvider adapter pattern shared across Ollama and llama.cpp
|
||||
|
||||
## Status
|
||||
|
||||
> Accepted
|
||||
|
||||
_Date:_ 2026-07-11
|
||||
_Deciders:_ darth-veitcher
|
||||
|
||||
---
|
||||
|
||||
## Context
|
||||
|
||||
GitHub issue #15 asks comfydv to add ComfyUI nodes for llama.cpp, mirroring
|
||||
the existing Ollama integration
|
||||
(`project-management/Roadmap/epics/archive/ollama-integration.md`), and
|
||||
explicitly poses the design question: separate nodes per backend, or an
|
||||
adapter pattern that shares code? It's motivated by llama.cpp's new "router
|
||||
mode" (`llama-server --models-dir <dir>`, via
|
||||
[llama.cpp PR #18228](https://github.com/ggml-org/llama.cpp/pull/18228),
|
||||
merged 2025-12-21), which exposes `GET /models`, `POST /models/load`,
|
||||
`POST /models/unload`, and `--sleep-idle-seconds` auto-unload — giving
|
||||
llama.cpp the same manual load/unload memory-management primitives comfydv
|
||||
already relies on for Ollama.
|
||||
|
||||
`src/comfydv/ollama.py` (1056 lines, 17 node classes) has no abstraction
|
||||
layer today: two module-level free functions (`_post_json`, `_fetch_models`)
|
||||
are called directly by every node, with Ollama endpoint paths hardcoded
|
||||
inline; socket types (`OLLAMA_CLIENT`, `OLLAMA_OPTIONS`, `OLLAMA_HISTORY`)
|
||||
and `PromptServer` routes (`/dv/ollama/...`) are Ollama-named throughout.
|
||||
|
||||
**Two things needed resolving to answer issue #15's question honestly:**
|
||||
|
||||
1. **Does model lifecycle management (list/load/unload) actually converge
|
||||
between backends, or not?** At the wire-protocol level, no: Ollama uses
|
||||
`/api/tags` + a per-request `keep_alive` TTL on `/api/generate`;
|
||||
llama.cpp router mode uses `/models` + explicit `/models/load` /
|
||||
`/models/unload` + named status states
|
||||
(`unloaded`/`loading`/`loaded`/`sleeping`/`downloading`). Judged at that
|
||||
level, a shared interface looks forced. But at the *conceptual* level,
|
||||
both APIs support exactly the same four operations — list models with
|
||||
status, load a model, unload a model, and generate/chat against a loaded
|
||||
model — just with different mechanics. Ollama's `keep_alive`-based
|
||||
load/unload already *is* `load_model()`/`unload_model()`, mechanically
|
||||
implemented as a side effect of a `/api/generate` call rather than a
|
||||
dedicated endpoint. A `Protocol` boundary at the operation level, not the
|
||||
wire-format level, fits both backends without forcing anything.
|
||||
|
||||
2. **Does `pydantic-ai` make sense now that a second backend exists?**
|
||||
[ADR-006](ADR-006-structured-ollama-output-tool-calling-not-pydantic-ai.md)
|
||||
(2026-07-09) rejected `pydantic-ai` for a single backend because its
|
||||
Ollama support pulls in `httpx` + the `openai` SDK, reversing
|
||||
[ADR-004](ADR-004-aiohttp-over-httpx-for-ollama.md)'s aiohttp-only
|
||||
stance. A research pass against current `pydantic-ai` docs/source (this
|
||||
moves fast and postdates training data, so verified live rather than
|
||||
assumed) found: `httpx` is a **base dependency of `pydantic-ai-slim`
|
||||
itself**, not merely pulled in by an OpenAI-specific extra; `openai` SDK
|
||||
+ `tiktoken` are additionally required to reach any OpenAI-compatible
|
||||
backend; there is no aiohttp transport option anywhere in pydantic-ai.
|
||||
This is a **fixed, one-time dependency tax**, not one that grows per
|
||||
backend. Separately, `pydantic.create_model()`-built `BaseModel`
|
||||
subclasses (comfydv's existing pattern for validating against a
|
||||
user-supplied JSON-Schema string at workflow-execution time) work as
|
||||
pydantic-ai's `output_type` with no special-casing — the dynamic-schema
|
||||
requirement is not a blocker. And `OpenAIProvider(base_url=...)` is the
|
||||
exact generic mechanism pydantic-ai's own `OllamaProvider` is built on
|
||||
internally, so llama.cpp's `/v1/chat/completions` reaches an identical
|
||||
code path with a different `base_url` — genuinely shared implementation,
|
||||
not just a shared shape.
|
||||
|
||||
Both findings point the same direction: a real `Protocol`-based adapter,
|
||||
with `pydantic-ai` as the mechanism behind its structured-output method.
|
||||
|
||||
## Decision
|
||||
|
||||
Define a `LLMProvider` `Protocol` (new internal module, e.g.
|
||||
`src/comfydv/_llm/provider.py`) with the common surface:
|
||||
|
||||
```python
|
||||
class LLMProvider(Protocol):
|
||||
async def list_models(self) -> list[ModelInfo]: ...
|
||||
async def load_model(self, model: str) -> None: ...
|
||||
async def unload_model(self, model: str) -> None: ...
|
||||
async def chat(self, model: str, messages: ..., options: ...) -> str: ...
|
||||
async def chat_structured(self, model: str, messages: ..., schema: type[BaseModel], options: ...) -> BaseModel: ...
|
||||
```
|
||||
|
||||
`OllamaProvider` and `LlamaCppProvider` each implement it, absorbing their
|
||||
own REST mechanics internally (Ollama: `/api/tags`, `/api/generate` with
|
||||
`keep_alive`; llama.cpp: `/models`, `/models/load`, `/models/unload`) via
|
||||
`aiohttp`, unchanged from ADR-004's stance for non-chat calls. Both
|
||||
implement `chat_structured()` via the same `pydantic-ai` `Agent`/
|
||||
`output_type` call through `OpenAIProvider(base_url=...)` — one shared
|
||||
implementation, differing only in `base_url` and model name.
|
||||
|
||||
ComfyUI nodes become **generic, not per-backend**: `OllamaClient` and
|
||||
`LlamaCppClient` both output the same `LLM_CLIENT` socket type (each
|
||||
internally constructs the matching provider); a single `LLMModelSelector`,
|
||||
`LLMLoadModel`, `LLMUnloadModel`, and `ChatCompletion` node operate against
|
||||
`LLM_CLIENT` generically. Swapping providers on the canvas means rewiring
|
||||
which client node feeds the chat/management nodes, not swapping node
|
||||
classes — this is the direct answer to issue #15's question: **adapter
|
||||
pattern**, implemented as a protocol boundary at the operation level.
|
||||
|
||||
This **supersedes ADR-006**: `OllamaChatCompletion`'s `structured_output=True`
|
||||
path moves from hand-rolled tool-calling to `pydantic-ai` via the protocol.
|
||||
|
||||
This **narrows ADR-004's scope**: aiohttp remains the transport for every
|
||||
non-chat REST call inside each provider; `httpx`/`openai` enter the
|
||||
dependency tree scoped specifically to `chat_structured()`, via
|
||||
`pydantic-ai`.
|
||||
|
||||
**Documented approximation:** `ModelStatus` includes `sleeping` and
|
||||
`downloading`, states that exist in llama.cpp router mode but not in
|
||||
Ollama's API. `OllamaProvider.list_models()` normalizes into the same enum
|
||||
rather than inventing Ollama-specific states — a model that's resident and
|
||||
idle maps to `loaded` (Ollama has no distinct "kept warm but not serving"
|
||||
signal via this API), and `downloading` is simply never emitted by
|
||||
`OllamaProvider` (Ollama's pull/download flow is out of scope per the
|
||||
original Ollama epic's non-goals). This is an accepted, explicit
|
||||
approximation, not a silent gap.
|
||||
|
||||
**Confirmed:** adopting generic node/socket names (`LLM_CLIENT`,
|
||||
`ChatCompletion`, etc.) means renaming away from `OLLAMA_CLIENT`,
|
||||
`OllamaChatCompletion`, and similar — a breaking change for any saved
|
||||
workflow using the current names. The Ollama integration shipped
|
||||
2026-07-04, so the blast radius is small. Confirmed 2026-07-11: rename in
|
||||
place now rather than carry Ollama-prefixed names forward or maintain
|
||||
deprecated aliases indefinitely.
|
||||
|
||||
## Consequences
|
||||
|
||||
**Easier:**
|
||||
- Issue #15's question gets a real answer: one generic node set works with
|
||||
any backend that implements `LLMProvider`, including future ones (a third
|
||||
local server, or a hosted OpenAI/Anthropic provider) without new node
|
||||
classes.
|
||||
- One implementation of tool-calling/structured-output logic instead of
|
||||
duplicating it per backend; `pydantic-ai` brings built-in retry/validation
|
||||
machinery, replacing ADR-006's hand-rolled retry loop.
|
||||
- The protocol boundary keeps each backend's REST quirks contained inside
|
||||
its provider — the graph never has to know Ollama uses `keep_alive` while
|
||||
llama.cpp uses explicit load/unload endpoints.
|
||||
|
||||
**Harder / constrained:**
|
||||
- New dependencies (`pydantic-ai`, `openai`, `tiktoken`, `httpx`) land in a
|
||||
project that was previously aiohttp-only.
|
||||
- This is a nontrivial migration of tested, shipped Ollama code (the
|
||||
`ollama-integration` epic is Done) — not purely additive work. Must be
|
||||
proven regression-safe before it's trusted as the foundation for
|
||||
llama.cpp.
|
||||
- `ModelStatus` is not perfectly symmetric across backends — the
|
||||
`sleeping`/`downloading` states are llama.cpp-only in practice; documented
|
||||
above, but still a leak of llama.cpp's richer vocabulary into a
|
||||
nominally-generic type.
|
||||
- The node/socket rename is a breaking change for existing saved workflows —
|
||||
confirmed acceptable given the small blast radius (see above).
|
||||
|
||||
**Debt introduced:**
|
||||
- None deliberately, contingent on the migration preserving existing
|
||||
Ollama behavior exactly (verified against `tests/test_ollama.py`).
|
||||
|
||||
## Considered Alternatives
|
||||
|
||||
### Alternative A: Shared `pydantic-ai` chat layer only; separate per-backend management nodes
|
||||
|
||||
**Why rejected:** This was the first-pass design — judged convergence at
|
||||
the REST wire-protocol level (Ollama's `/api/tags`+`keep_alive` vs.
|
||||
llama.cpp's `/models`+`/models/load`+`/models/unload` don't look alike) and
|
||||
concluded a shared interface would be forced. That framing was wrong: the
|
||||
right level to judge convergence is the *operation* (list/load/unload/chat),
|
||||
not the wire format. Both backends genuinely support the same four
|
||||
operations; only their REST mechanics differ, and those differences belong
|
||||
inside each provider implementation, not on the graph.
|
||||
|
||||
### Alternative B: Hand-roll llama.cpp's structured output too (duplicate ADR-006's approach)
|
||||
|
||||
**Why rejected:** Two independent implementations of the same
|
||||
OpenAI-compatible tool-calling mechanism is the DRY violation issue #15
|
||||
raises in the first place, with no offsetting benefit now that a second
|
||||
backend exists to justify a shared layer.
|
||||
|
||||
### Alternative C: Keep `pydantic-ai` rejected; extract a shared aiohttp-based internal helper instead
|
||||
|
||||
**Why rejected:** Avoids new dependencies entirely, but forces re-deriving
|
||||
`pydantic-ai`'s retry/validation machinery by hand for no benefit beyond
|
||||
dependency-avoidance — and doesn't change the model-management convergence
|
||||
question at all (that's orthogonal to which HTTP client the chat path
|
||||
uses). The one-time dependency tax is judged worth paying for the fuller
|
||||
abstraction, now that two backends exist to amortize it against.
|
||||
|
||||
---
|
||||
|
||||
## Links
|
||||
|
||||
- Related epics: `project-management/Roadmap/epics/llm-provider-abstraction.md`, `project-management/Roadmap/epics/llamacpp-integration.md`
|
||||
- Related ADRs: [ADR-004](ADR-004-aiohttp-over-httpx-for-ollama.md) (narrowed), [ADR-005](ADR-005-ollama-host-config-via-client-node.md) (client-node pattern generalized to `LLM_CLIENT`), [ADR-006](ADR-006-structured-ollama-output-tool-calling-not-pydantic-ai.md) (superseded)
|
||||
- External reference: [llama.cpp PR #18228](https://github.com/ggml-org/llama.cpp/pull/18228) (router mode, merged 2025-12-21), GitHub issue #15
|
||||
@@ -26,6 +26,8 @@
|
||||
- **BEACON bootstrap** — `epics/archive/beacon-bootstrap.md` — ✅ DONE — BEACON framework wired up; problem statement, constitution, roadmap, and ADR template populated; quality gates clean
|
||||
- **Logging modernisation** — `epics/logging-modernisation.md` — ✅ DONE — stdlib logging, NullHandler, silent-by-default; colorama/rich/termcolor removed; 11 tests
|
||||
- **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/llm-provider-abstraction.md` — 🔄 ACTIVE — Introduce a shared `LLMProvider` protocol (list/load/unload/chat/structured-output) and generic ComfyUI nodes; migrate the existing Ollama integration onto it (ADR-007, supersedes ADR-006)
|
||||
- **llama.cpp Model Integration** — `epics/llamacpp-integration.md` — 📋 PROPOSED — Add a `LlamaCppProvider` implementing the shared protocol via llama-server's router mode; depends on LLM Provider Abstraction landing first (GitHub issue #15)
|
||||
|
||||
For the live rollup (specs per epic, % tasks complete, last-commit age):
|
||||
|
||||
@@ -40,6 +42,7 @@ beacon epic list --detailed
|
||||
- **BEACON bootstrap** is a prerequisite for all other epics (quality gates need to pass before new work merges)
|
||||
- **Test hardening** is independent of documentation and can run in parallel
|
||||
- **Documentation** depends on the final node API (output order, input names) being stable — start after test hardening locks the contracts
|
||||
- **llama.cpp Model Integration** depends on **LLM Provider Abstraction** landing first — its `LlamaCppProvider` implements the protocol that epic defines, and reuses its generic nodes as-is
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -0,0 +1,66 @@
|
||||
# Epic: llama.cpp Model Integration
|
||||
|
||||
## Status
|
||||
Planning — started 2026-07-11
|
||||
|
||||
## Why now
|
||||
|
||||
GitHub issue #15 requests llama.cpp support "similar to Ollama." This is now
|
||||
practical because llama.cpp's `llama-server` gained a "router mode" via
|
||||
[llama.cpp PR #18228](https://github.com/ggml-org/llama.cpp/pull/18228)
|
||||
(merged 2025-12-21): launched with `--models-dir <dir>` (or
|
||||
`--models-preset <file>.ini`) instead of `-m`, it exposes `GET /models`
|
||||
(with live status: `unloaded`/`loading`/`loaded`/`sleeping`/`downloading`),
|
||||
`POST /models/load`, `POST /models/unload`, and `--sleep-idle-seconds`
|
||||
auto-unload — giving llama.cpp the same manual load/unload
|
||||
memory-management primitives comfydv already relies on for Ollama. With the
|
||||
`llm-provider-abstraction` epic in place, adding llama.cpp is now a matter
|
||||
of implementing one more `LLMProvider`, not building a parallel set of
|
||||
ComfyUI nodes.
|
||||
|
||||
## Dependencies
|
||||
|
||||
Depends on the `llm-provider-abstraction` epic landing first. This epic's
|
||||
`LlamaCppProvider` implements the `LLMProvider` protocol that epic defines,
|
||||
and its `LlamaCppClient` node emits the same `LLM_CLIENT` socket type the
|
||||
generic `ChatCompletion`/`LLMModelSelector`/`LLMLoadModel`/`LLMUnloadModel`
|
||||
nodes already consume — none of those node classes are touched by this
|
||||
epic.
|
||||
|
||||
## Specs
|
||||
|
||||
_Filled by `beacon specify --epic llamacpp-integration` / `/speckit-specify`
|
||||
once this epic is accepted._
|
||||
|
||||
## ADRs
|
||||
|
||||
- project-management/ADRs/ADR-007-llm-provider-adapter-pattern.md — decided during the prerequisite epic; this epic implements the second `LLMProvider` the ADR anticipated
|
||||
|
||||
## Success criteria
|
||||
|
||||
- `LlamaCppProvider` implements the `LLMProvider` protocol from the prerequisite epic:
|
||||
- `list_models()` via `GET /models`, surfacing native status (`unloaded`/`loading`/`loaded`/`sleeping`/`downloading`) directly — no normalization needed, since llama.cpp's vocabulary is the `ModelStatus` enum's superset
|
||||
- `load_model()` / `unload_model()` via `POST /models/load` / `POST /models/unload`
|
||||
- `chat_structured()` via the same shared `pydantic-ai` mechanism as `OllamaProvider`, `OpenAIProvider(base_url=<llama-server host>/v1)` — no new structured-output code, just a different `base_url`
|
||||
- `LlamaCppClient` config node (reuses the [ADR-005](../../ADRs/ADR-005-ollama-host-config-via-client-node.md) config-node pattern), outputs the same `LLM_CLIENT` socket type `OllamaClient` does
|
||||
- No new node classes for model selection, load/unload, or chat — the generic `LLMModelSelector`, `LLMLoadModel`, `LLMUnloadModel`, and `ChatCompletion` nodes from the prerequisite epic work unchanged once a `LlamaCppClient` is wired in
|
||||
- `LlamaCppClient` registered in `NODE_CLASS_MAPPINGS` / `NODE_DISPLAY_NAME_MAPPINGS`
|
||||
- Test coverage for `LlamaCppProvider` mirrors the `OllamaProvider` test conventions established in the prerequisite epic
|
||||
- No new runtime dependencies beyond what the prerequisite epic already introduced (`aiohttp` for model management, `pydantic-ai`/`openai` for chat)
|
||||
- CI smoke test passes
|
||||
|
||||
## Non-goals
|
||||
|
||||
- No support for llama-server's non-router single-model launch mode (`-m`) — router mode only, since that's what gives load/unload parity with Ollama
|
||||
- No GPU inference optimisation or quantisation tuning — CPU-first dev harness, consistent with the Ollama epic's own non-goal
|
||||
- No auth/TLS/remote-serving hardening — localhost/configurable host via client node only, consistent with the Ollama epic
|
||||
- No ComfyUI Manager registry listing in this epic
|
||||
- No changes to the generic nodes or `LLMProvider` protocol themselves — if llama.cpp's router mode needs something the protocol doesn't support, that's a protocol change scoped back into the prerequisite epic's follow-up, not silently special-cased here
|
||||
|
||||
## Notes
|
||||
|
||||
Router mode is a deployment prerequisite, not something comfydv configures:
|
||||
the user must launch `llama-server` with `--models-dir`/`--models-preset`
|
||||
themselves. Document this clearly in the eventual spec/node tooltips.
|
||||
|
||||
Reference: [llama.cpp PR #18228](https://github.com/ggml-org/llama.cpp/pull/18228), GitHub issue #15.
|
||||
@@ -0,0 +1,84 @@
|
||||
# Epic: LLM Provider Abstraction
|
||||
|
||||
## Status
|
||||
Active — started 2026-07-11
|
||||
|
||||
## Why now
|
||||
|
||||
GitHub issue #15 asks for llama.cpp support "similar to Ollama," and
|
||||
explicitly raises the question of separate nodes vs. an adapter pattern.
|
||||
[ADR-007](../../ADRs/ADR-007-llm-provider-adapter-pattern.md) answers that
|
||||
with a real adapter: a `LLMProvider` protocol (`list_models`/`load_model`/
|
||||
`unload_model`/`chat`/`chat_structured`) that any backend implements, backing
|
||||
a set of generic ComfyUI nodes (`ChatCompletion`, `LLMModelSelector`,
|
||||
`LLMLoadModel`, `LLMUnloadModel`) that work with whichever provider is wired
|
||||
in. For that to be real — not just aspirational — the existing Ollama
|
||||
integration has to migrate onto the protocol first, including moving its
|
||||
structured-output mechanism from ADR-006's hand-rolled tool-calling onto
|
||||
`pydantic-ai`. This epic is that migration; it's a prerequisite for
|
||||
`llamacpp-integration`, not additive scope on top of it — building
|
||||
llama.cpp against generic nodes that don't exist yet isn't possible.
|
||||
|
||||
## Dependencies
|
||||
|
||||
_None to start — this epic can begin immediately._ The
|
||||
`llamacpp-integration` epic depends on this one landing first: its
|
||||
`LlamaCppProvider` implements the protocol this epic defines, and its
|
||||
`LlamaCppClient` node emits the same `LLM_CLIENT` socket type this epic
|
||||
introduces.
|
||||
|
||||
## Specs
|
||||
|
||||
_Filled by `beacon specify --epic llm-provider-abstraction` /
|
||||
`/speckit-specify` once this epic is accepted._
|
||||
|
||||
- specs/007-llm-provider-abstraction/
|
||||
## ADRs
|
||||
|
||||
- project-management/ADRs/ADR-007-llm-provider-adapter-pattern.md — defines the `LLMProvider` protocol as the adapter boundary; supersedes ADR-006; narrows ADR-004's scope to non-chat REST calls per-provider
|
||||
|
||||
## Success criteria
|
||||
|
||||
- `LLMProvider` protocol defined (new internal module, e.g. `src/comfydv/_llm/provider.py`): `list_models()`, `load_model(name)`, `unload_model(name)`, `chat(...)`, `chat_structured(..., schema)`
|
||||
- `ModelStatus` enum defined (`unloaded`/`loading`/`loaded`/`sleeping`/`downloading`) per ADR-007's documented approximation
|
||||
- `OllamaProvider` implements the protocol, wrapping all existing Ollama REST logic (`/api/tags`, `/api/generate` with `keep_alive`) over `aiohttp` — behavior-preserving port of the current `_post_json`/`_fetch_models` logic, not a rewrite of the underlying calls
|
||||
- `chat_structured()` implemented via `pydantic-ai`'s `Agent`/`output_type`, called through `OpenAIProvider(base_url=<host>/v1)`, using the same dynamic `pydantic.create_model()`-from-JSON-Schema pattern as ADR-006 — same dynamic-socket UX, same retry/validation contract (bounded `max_retries`, required-string-non-empty check, clear `RuntimeError` on exhaustion)
|
||||
- `pydantic-ai` and `openai` added to `pyproject.toml`, curated into `requirements.txt` per [ADR-003](../../ADRs/ADR-003-requirements-txt-authoring-policy.md)
|
||||
- Generic ComfyUI nodes replace the current Ollama-specific ones: `OllamaClient` now outputs `LLM_CLIENT` (constructing an `OllamaProvider` internally); `LLMModelSelector`, `LLMLoadModel`, `LLMUnloadModel`, `ChatCompletion` operate against `LLM_CLIENT` generically
|
||||
- The non-structured-output chat path (native `/api/chat`) is behavior-unchanged
|
||||
- All existing `tests/test_ollama.py` coverage passes against the migrated implementation (adjusted for renamed node/socket types, unchanged in behavior otherwise)
|
||||
- Model-management calls remain entirely on `aiohttp`, inside `OllamaProvider` — untouched transport-wise
|
||||
- CI smoke test passes
|
||||
|
||||
## Non-goals
|
||||
|
||||
- No `LlamaCppProvider` or llama.cpp nodes in this epic — that's `llamacpp-integration`
|
||||
- No behavior change to non-structured-output chat
|
||||
- No tracing/observability integration (e.g. Logfire), even though `pydantic-ai` supports it
|
||||
- No multi-turn agentic tool use beyond the existing single structured-output call
|
||||
- No backward-compat aliases for the old `Ollama`-prefixed node/socket names — confirmed 2026-07-11 to rename in place (see ADR-007)
|
||||
|
||||
## Notes
|
||||
|
||||
This is the riskiest part of the whole llama.cpp proposal: it changes the
|
||||
implementation of tested, shipped code from a Done epic
|
||||
(`archive/ollama-integration.md`), not just adding new code, and it's a
|
||||
breaking rename (`OLLAMA_CLIENT`→`LLM_CLIENT`,
|
||||
`OllamaChatCompletion`→`ChatCompletion`, etc.) for anyone with saved
|
||||
workflows using the current node/socket names. Confirmed 2026-07-11 (per
|
||||
ADR-007): rename in place now — the Ollama integration only shipped
|
||||
2026-07-04, so the blast radius is small — rather than carrying
|
||||
`Ollama`-prefixed generic nodes forward or maintaining deprecated aliases
|
||||
indefinitely.
|
||||
|
||||
Recommend an adversarial pass (`/beacon:review` or `/beacon:engineering`)
|
||||
before merging, specifically checking that the retry/validation contract
|
||||
from ADR-006's `## Decision` section is preserved exactly by the
|
||||
`pydantic-ai` reimplementation, and that `OllamaProvider`'s REST calls are a
|
||||
faithful port of the current `_post_json`/`_fetch_models` logic.
|
||||
|
||||
`pydantic-ai`'s `StructuredDict` (raw-JSON-Schema output, no Python class)
|
||||
was considered as a lighter-weight alternative to `create_model()` during
|
||||
research and rejected: it performs no pydantic validation at all, which
|
||||
would silently drop the "reject blank required strings" safeguard ADR-006
|
||||
introduced. Stick with `create_model()`-built `BaseModel` subclasses.
|
||||
@@ -0,0 +1 @@
|
||||
epic = "llm-provider-abstraction"
|
||||
@@ -0,0 +1,38 @@
|
||||
# Specification Quality Checklist: LLM Provider Abstraction
|
||||
|
||||
**Purpose**: Validate specification completeness and quality before proceeding to planning
|
||||
**Created**: 2026-07-11
|
||||
**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
|
||||
|
||||
All items pass on first pass — no [NEEDS CLARIFICATION] markers were needed;
|
||||
scope boundaries (llama.cpp out of scope, no automatic workflow migration,
|
||||
no new tracing capability) came directly from the parent epic's Non-goals
|
||||
(`project-management/Roadmap/epics/llm-provider-abstraction.md`) and
|
||||
ADR-007, so no ambiguity required flagging back to the user.
|
||||
@@ -0,0 +1,79 @@
|
||||
# Contract: `LLMProvider` protocol
|
||||
|
||||
This is the interface the follow-on `llamacpp-integration` epic implements
|
||||
against (`LlamaCppProvider`) — it's the actual deliverable that makes ADR-007's
|
||||
adapter pattern real, not internal implementation detail. Treat changes to
|
||||
this contract as requiring epic-level sign-off (per ADR-007's own scope),
|
||||
not a routine refactor.
|
||||
|
||||
```python
|
||||
class ModelStatus(str, Enum):
|
||||
UNLOADED = "unloaded"
|
||||
LOADING = "loading"
|
||||
LOADED = "loaded"
|
||||
SLEEPING = "sleeping" # not all providers emit this
|
||||
DOWNLOADING = "downloading" # not all providers emit this
|
||||
|
||||
class ModelInfo(BaseModel):
|
||||
name: str
|
||||
status: ModelStatus
|
||||
size: int | None = None
|
||||
|
||||
class Message(BaseModel):
|
||||
role: Literal["system", "user", "assistant"]
|
||||
content: str
|
||||
|
||||
class LLMProvider(Protocol):
|
||||
async def list_models(self) -> list[ModelInfo]: ...
|
||||
async def load_model(self, model: str) -> None: ...
|
||||
async def unload_model(self, model: str) -> None: ...
|
||||
async def chat(
|
||||
self, model: str, messages: list[Message], options: dict
|
||||
) -> str: ...
|
||||
async def chat_structured(
|
||||
self, model: str, messages: list[Message], schema: type[BaseModel], options: dict
|
||||
) -> BaseModel: ...
|
||||
```
|
||||
|
||||
## Behavioral requirements (every implementation MUST satisfy)
|
||||
|
||||
- `load_model`/`unload_model` are **idempotent** — calling either on a model
|
||||
already in that state is not an error.
|
||||
- `chat_structured` **MUST NOT** return a `BaseModel` instance with a blank
|
||||
required `str` field — validate and retry (bounded, provider-internal)
|
||||
rather than pass through invalid data. On exhausted retries, raise
|
||||
`RuntimeError` naming the model, attempt count, and a truncated snippet of
|
||||
the last invalid response (FR-004 in `../spec.md`).
|
||||
- `list_models` MUST return every model the server currently knows about,
|
||||
including ones not currently loaded — this is a status listing, not a
|
||||
"loaded models only" filter.
|
||||
- A provider that cannot represent a given `ModelStatus` value (e.g. Ollama
|
||||
has no `sleeping`/`downloading` concept) MUST normalize to the closest
|
||||
applicable status rather than omit the model or invent a new status value
|
||||
outside this enum.
|
||||
- Connection state (host, auth headers, or equivalent) is captured once at
|
||||
provider-construction time; no method takes connection details as a
|
||||
parameter.
|
||||
|
||||
## Non-requirements (explicitly not part of this contract)
|
||||
|
||||
- No requirement that every provider support every `ModelStatus` value —
|
||||
see `data-model.md`'s per-provider emission notes.
|
||||
- No streaming contract — `chat`/`chat_structured` return a complete result,
|
||||
not a stream. (Not requested by the parent spec; a future contract change
|
||||
if ever needed.)
|
||||
- No multi-turn agent/tool-use contract beyond a single structured-output
|
||||
call — out of scope per the parent epic's Non-goals.
|
||||
|
||||
## ComfyUI-facing contract: `LLM_CLIENT` socket
|
||||
|
||||
An `LLMProvider`-implementing instance is the value carried by ComfyUI's
|
||||
`LLM_CLIENT` custom socket type. Any node that outputs `LLM_CLIENT` (e.g.
|
||||
`OllamaClient`, and later `LlamaCppClient`) is committing to have constructed
|
||||
a fully-configured provider instance — no partial/lazy construction that
|
||||
defers connection details to the consuming node.
|
||||
|
||||
Generic nodes (`LLMModelSelector`, `LLMLoadModel`, `LLMUnloadModel`,
|
||||
`ChatCompletion`) accept `LLM_CLIENT` as their only connection-related input
|
||||
and MUST NOT branch on which concrete provider type they received — doing so
|
||||
would defeat the point of the protocol boundary (ADR-007).
|
||||
@@ -0,0 +1,69 @@
|
||||
# Data Model: LLM Provider Abstraction
|
||||
|
||||
## `ModelStatus` (enum)
|
||||
|
||||
Residency status of a model on a provider's server.
|
||||
|
||||
| Value | Meaning | Emitted by |
|
||||
|---|---|---|
|
||||
| `unloaded` | Known to the server, not resident in memory | all providers |
|
||||
| `loading` | Transitioning into memory | all providers |
|
||||
| `loaded` | Resident and ready to serve requests | all providers |
|
||||
| `sleeping` | Resident but idle-parked | llama.cpp only; Ollama has no distinct signal for this via its API and normalizes resident-and-idle to `loaded` (documented approximation, ADR-007) |
|
||||
| `downloading` | Server is fetching model weights | llama.cpp only; `OllamaProvider` never emits this (Ollama's pull/download flow is out of scope, per the original Ollama epic's non-goals) |
|
||||
|
||||
## `ModelInfo`
|
||||
|
||||
One entry returned by `list_models()`.
|
||||
|
||||
| Field | Type | Notes |
|
||||
|---|---|---|
|
||||
| `name` | `str` | Model identifier as the provider's server knows it |
|
||||
| `status` | `ModelStatus` | See above |
|
||||
| `size` | `int \| None` | Bytes, if the provider reports it; `None` otherwise |
|
||||
|
||||
## `LLMProvider` (Protocol)
|
||||
|
||||
The adapter boundary. Every backend (`OllamaProvider` now, `LlamaCppProvider`
|
||||
in the follow-on epic) implements this shape; ComfyUI nodes depend only on
|
||||
the protocol, never on a concrete provider class.
|
||||
|
||||
| Method | Signature | Notes |
|
||||
|---|---|---|
|
||||
| `list_models` | `async def list_models(self) -> list[ModelInfo]` | |
|
||||
| `load_model` | `async def load_model(self, model: str) -> None` | Idempotent: loading an already-loaded model is not an error |
|
||||
| `unload_model` | `async def unload_model(self, model: str) -> None` | Idempotent: unloading an already-unloaded model is not an error |
|
||||
| `chat` | `async def chat(self, model: str, messages: list[Message], options: dict) -> str` | Free-text response |
|
||||
| `chat_structured` | `async def chat_structured(self, model: str, messages: list[Message], schema: type[BaseModel], options: dict) -> BaseModel` | Validated response; raises on exhausted retries (see FR-004) |
|
||||
|
||||
A concrete provider instance is constructed once per ComfyUI client node with
|
||||
its connection's host/headers as instance state (Constitution Principle V
|
||||
justification — see `research.md`), and that instance is the value carried
|
||||
by the `LLM_CLIENT` ComfyUI socket type.
|
||||
|
||||
## `Message`
|
||||
|
||||
One turn in a chat request, matching the existing shape already sent to
|
||||
Ollama's `/api/chat`/`/v1/chat/completions` (`role` + `content`); unchanged
|
||||
by this feature, carried forward as-is.
|
||||
|
||||
| Field | Type | Notes |
|
||||
|---|---|---|
|
||||
| `role` | `Literal["system", "user", "assistant"]` | |
|
||||
| `content` | `str` | |
|
||||
|
||||
## Relationships
|
||||
|
||||
```
|
||||
ProviderConnection (ComfyUI client node)
|
||||
└─ produces → LLM_CLIENT socket value (an LLMProvider instance)
|
||||
└─ consumed by → LLMModelSelector, LLMLoadModel, LLMUnloadModel, ChatCompletion (ComfyUI nodes)
|
||||
├─ list_models() → ModelInfo[]
|
||||
├─ load_model()/unload_model() → mutates server-side residency, no return value
|
||||
└─ chat()/chat_structured() → str | BaseModel
|
||||
```
|
||||
|
||||
No new persistent storage is introduced — every entity above is
|
||||
constructed per-request or per-node-execution from the connected server's
|
||||
live state; the only caching is the existing in-memory TTL cache for model
|
||||
listing (`_TTLLRUCache`, unchanged, reused inside `OllamaProvider`).
|
||||
@@ -0,0 +1,16 @@
|
||||
Feature: US1 — Connect to a local inference server and get chat responses
|
||||
|
||||
Scenario: Client node feeds a chat node
|
||||
Given a running local inference server and a workflow with a client node wired into a chat node
|
||||
When the workflow executes
|
||||
Then the chat node returns the model's text response
|
||||
|
||||
Scenario: Unreachable server surfaces a clear error
|
||||
Given a client node configured with an unreachable server address
|
||||
When the workflow executes
|
||||
Then the chat node reports a clear connection error rather than hanging indefinitely or crashing the workflow
|
||||
|
||||
Scenario: One client node configures multiple chat nodes
|
||||
Given two chat nodes in the same workflow wired to the same client node
|
||||
When the host address is changed on the client node
|
||||
Then both chat nodes use the new address without being edited individually
|
||||
@@ -0,0 +1,16 @@
|
||||
Feature: US2 — Get structured, validated output instead of parsing raw text
|
||||
|
||||
Scenario: Valid structured response exposes typed fields
|
||||
Given a chat node with structured output enabled and a valid schema
|
||||
When the workflow executes and the model responds correctly
|
||||
Then each schema field is available as its own typed output, and no required field is blank
|
||||
|
||||
Scenario: Invalid response triggers automatic retry
|
||||
Given a model that returns invalid, incomplete, or empty-required-field output
|
||||
When the workflow executes
|
||||
Then the node automatically retries the request up to a configured limit
|
||||
|
||||
Scenario: Exhausted retries fail clearly instead of passing through bad data
|
||||
Given a model that continues to return invalid output after all retries are exhausted
|
||||
When the workflow executes
|
||||
Then the node fails with a clear, specific error rather than silently passing through invalid or partial data
|
||||
@@ -0,0 +1,16 @@
|
||||
Feature: US3 — Manage which models are resident in memory
|
||||
|
||||
Scenario: List models with current status
|
||||
Given a running local server with at least one available model
|
||||
When a workflow author uses the model-listing node
|
||||
Then they see each available model along with its current status
|
||||
|
||||
Scenario: Load a model into memory
|
||||
Given a model that is not currently loaded
|
||||
When a workflow author runs the load-model node against it
|
||||
Then the model becomes loaded and is then usable by the chat node
|
||||
|
||||
Scenario: Unload a model from memory
|
||||
Given a model that is loaded and idle
|
||||
When a workflow author runs the unload-model node against it
|
||||
Then the model is freed from memory and its reported status updates accordingly
|
||||
@@ -0,0 +1,12 @@
|
||||
Feature: US4 — Reconnect an existing workflow after upgrading
|
||||
|
||||
Scenario: Renamed nodes are reported with a documented replacement
|
||||
Given a saved workflow using the current Ollama-specific node and connection-socket names
|
||||
When it is opened after upgrading
|
||||
Then ComfyUI reports the now-missing node types
|
||||
And documentation identifies the replacement node for each one
|
||||
|
||||
Scenario: Reconnected workflow produces equivalent output
|
||||
Given a workflow that has been reconnected to the new generic nodes
|
||||
When it executes with the same inputs and model as before the upgrade
|
||||
Then it produces equivalent output
|
||||
@@ -0,0 +1,121 @@
|
||||
# Implementation Plan: LLM Provider Abstraction
|
||||
|
||||
**Branch**: `007-llm-provider-abstraction` | **Date**: 2026-07-11 | **Spec**: [spec.md](./spec.md)
|
||||
|
||||
**Input**: Feature specification from `/specs/007-llm-provider-abstraction/spec.md`
|
||||
|
||||
**Note**: This template is filled in by the `/speckit-plan` command. See `.specify/templates/plan-template.md` for the execution workflow.
|
||||
|
||||
## Summary
|
||||
|
||||
Define a shared `LLMProvider` protocol (list/load/unload/chat/structured-chat)
|
||||
and generic ComfyUI nodes so workflow authors can connect any supported local
|
||||
inference backend the same way. Migrate the existing Ollama integration onto
|
||||
it — `OllamaProvider` becomes the first (and, in this feature, only)
|
||||
implementation — including moving structured-output from ADR-006's
|
||||
hand-rolled tool-calling onto `pydantic-ai`, per ADR-007. This is a
|
||||
behavior-preserving mechanism swap for existing capability, plus the new
|
||||
protocol boundary that the follow-on `llamacpp-integration` epic builds a
|
||||
second provider against.
|
||||
|
||||
## Technical Context
|
||||
|
||||
**Language/Version**: Python ≥3.11 (per `pyproject.toml`)
|
||||
|
||||
**Primary Dependencies**: `aiohttp` (existing, unchanged — model-management
|
||||
REST calls), `pydantic` (existing, unchanged — validation), `pydantic-ai` +
|
||||
`openai` (new, per ADR-007 — powers `chat_structured()` only)
|
||||
|
||||
**Storage**: N/A — no persistent storage; the existing in-memory
|
||||
`_TTLLRUCache` for model listing is reused unchanged inside `OllamaProvider`
|
||||
|
||||
**Testing**: `pytest` via `uv run pytest`, following `tests/test_ollama.py`'s
|
||||
existing conventions (mocked `aiohttp`/`pydantic-ai` calls, no live server
|
||||
required for unit tests; the existing `integration` pytest marker — "requiring
|
||||
live Ollama at localhost:11434" — is reused for tests that exercise a real
|
||||
server)
|
||||
|
||||
**Target Platform**: ComfyUI custom-node runtime, cross-platform wherever
|
||||
ComfyUI runs; CPU-only dev harness per the project's stated vision
|
||||
|
||||
**Project Type**: Library / ComfyUI custom-node pack (single project,
|
||||
existing `src/comfydv/` layout — no new top-level project)
|
||||
|
||||
**Performance Goals**: No new numeric target; must not add latency beyond the
|
||||
existing bounded retry loop already in ADR-006 (`max_retries`, 0–5)
|
||||
|
||||
**Constraints**: Behavior-preserving for existing Ollama structured/
|
||||
non-structured chat and all model-management calls (FR-007, FR-008); no new
|
||||
dependency beyond what ADR-007 already accepted (`pydantic-ai`, `openai`,
|
||||
their transitive `httpx`/`tiktoken`); model-management stays on `aiohttp`
|
||||
|
||||
**Scale/Scope**: One new internal package (`src/comfydv/_llm/`), migration of
|
||||
the existing 1056-line `ollama.py` node/HTTP logic to consume it, five
|
||||
ComfyUI node classes renamed to generic names — no change to the project's
|
||||
single-repo, single-package scope
|
||||
|
||||
## Constitution Check
|
||||
|
||||
*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.*
|
||||
|
||||
| Principle | Verdict | Notes |
|
||||
|---|---|---|
|
||||
| I. ComfyUI Contract First | PASS | Generic nodes (`ChatCompletion`, `LLMModelSelector`, `LLMLoadModel`, `LLMUnloadModel`, `OllamaClient`) still expose `INPUT_TYPES`/`RETURN_TYPES`/`RETURN_NAMES`/`FUNCTION`/`CATEGORY`; `NODE_CLASS_MAPPINGS` in `__init__.py` remains the only install-time interface. `LLMProvider` is internal, not a ComfyUI-facing contract change beyond the node/socket rename. |
|
||||
| II. Sandbox All User-Supplied Code | N/A | No template/expression evaluation in this feature — structured-output schemas are parsed as JSON Schema by `pydantic`, never `eval`/`exec`. |
|
||||
| III. Test-First | PASS (binding on tasks/implement phases) | `tests/test_ollama.py`'s existing assertions are the regression oracle (see `research.md`); new `_llm` package gets tests written before implementation, red→green→refactor. |
|
||||
| IV. Graceful Degradation Outside ComfyUI | PASS (binding on implementation) | `src/comfydv/_llm/` must not import `comfy`/`server` at module scope, matching `ollama.py`'s existing guarded-import pattern. |
|
||||
| V. Simplicity — Function Before Class | **Justified exception — see Complexity Tracking** | `LLMProvider` is a `Protocol` implemented by stateful provider classes, not module-level functions. |
|
||||
| VI. Fixed Output Positions | PASS (binding on implementation) | `ChatCompletion`'s (renamed from `OllamaChatCompletion`) `RETURN_TYPES`/`RETURN_NAMES` positions 0/1 carry forward unchanged — only the class/node name and internal mechanism change. |
|
||||
|
||||
Re-checked post-Phase 1 design (data-model.md, contracts/): unchanged — the
|
||||
`Protocol`-based design in `contracts/llm_provider_protocol.md` is exactly
|
||||
what was justified below, no new gate violations introduced by the detailed
|
||||
design.
|
||||
|
||||
## Project Structure
|
||||
|
||||
### Documentation (this feature)
|
||||
|
||||
```text
|
||||
specs/[###-feature]/
|
||||
├── plan.md # This file (/speckit-plan command output)
|
||||
├── research.md # Phase 0 output (/speckit-plan command)
|
||||
├── data-model.md # Phase 1 output (/speckit-plan command)
|
||||
├── quickstart.md # Phase 1 output (/speckit-plan command)
|
||||
├── contracts/ # Phase 1 output (/speckit-plan command)
|
||||
└── tasks.md # Phase 2 output (/speckit-tasks command - NOT created by /speckit-plan)
|
||||
```
|
||||
|
||||
### Source Code (repository root)
|
||||
|
||||
```text
|
||||
src/comfydv/
|
||||
├── ollama.py # existing — node classes renamed to generic names,
|
||||
│ # delegates HTTP/chat logic to _llm/ internally
|
||||
├── _llm/ # new internal package (not a ComfyUI node module)
|
||||
│ ├── __init__.py
|
||||
│ ├── provider.py # LLMProvider Protocol, ModelStatus, ModelInfo, Message
|
||||
│ ├── ollama_provider.py # OllamaProvider — wraps existing aiohttp REST logic
|
||||
│ └── chat.py # shared chat_structured() pydantic-ai helper
|
||||
└── __init__.py # NODE_CLASS_MAPPINGS updated for renamed nodes
|
||||
|
||||
tests/
|
||||
├── test_ollama.py # existing — updated for renamed nodes; behavior-
|
||||
│ # preserving assertions carried forward unchanged
|
||||
└── test_llm_provider.py # new — protocol conformance + OllamaProvider unit tests
|
||||
```
|
||||
|
||||
**Structure Decision**: Single project (existing `src/comfydv/` layout, no new
|
||||
top-level project). New internal package `src/comfydv/_llm/` (underscore
|
||||
prefix marks it as internal, consistent with existing internal helpers like
|
||||
`_TTLLRUCache` that already live inside `ollama.py`) hosts the protocol and
|
||||
shared chat logic; `ollama.py` keeps the actual ComfyUI-registered node
|
||||
classes and becomes a thin caller into `_llm`.
|
||||
|
||||
## Complexity Tracking
|
||||
|
||||
> **Fill ONLY if Constitution Check has violations that must be justified**
|
||||
|
||||
| Violation | Why Needed | Simpler Alternative Rejected Because |
|
||||
|-----------|------------|-------------------------------------|
|
||||
| `LLMProvider` as a `Protocol` implemented by stateful classes (Principle V: Function Before Class) | Every one of the five protocol methods (`list_models`/`load_model`/`unload_model`/`chat`/`chat_structured`) needs the same connection state (host, auth headers) — genuine shared state, the exact condition under which the constitution allows a class. A `Protocol` also lets ComfyUI's `LLM_CLIENT` socket carry one opaque object satisfying the shape, which is what makes the adapter pattern (ADR-007) work on the canvas. | Module-level functions taking host/headers as explicit parameters on every call were considered and rejected: they'd reintroduce the exact per-call-site repetition [ADR-005](../../project-management/ADRs/ADR-005-ollama-host-config-via-client-node.md)'s config-node pattern was built to eliminate, and a bare function can't be the typed payload of a ComfyUI socket the way an object implementing a `Protocol` can. |
|
||||
@@ -0,0 +1,49 @@
|
||||
# Quickstart: LLM Provider Abstraction
|
||||
|
||||
A minimal ComfyUI workflow using the generic nodes this feature introduces.
|
||||
|
||||
## 1. Connect to a local server
|
||||
|
||||
Add an **Ollama Client** node. Set its host widget (default
|
||||
`http://localhost:11434`). This is the only node that knows it's talking to
|
||||
Ollama specifically — everything downstream just sees `LLM_CLIENT`.
|
||||
|
||||
## 2. Chat
|
||||
|
||||
Add a **Chat Completion** node. Wire the client node's `LLM_CLIENT` output
|
||||
into it. Set a model name (or feed one from a model-selector node — see
|
||||
below) and a prompt. Run the workflow: the node returns the model's text
|
||||
response.
|
||||
|
||||
## 3. Get structured output instead of free text
|
||||
|
||||
On the same **Chat Completion** node, enable `structured_output` and supply
|
||||
a JSON Schema (e.g. `{"type": "object", "properties": {"summary": {"type": "string"}, "score": {"type": "number"}}, "required": ["summary", "score"]}`).
|
||||
Re-run: the node now exposes one typed output socket per schema property
|
||||
(`summary`, `score`) instead of a single text blob, and guarantees neither
|
||||
is blank.
|
||||
|
||||
## 4. Manage what's loaded in memory
|
||||
|
||||
Add an **LLM Model Selector** node wired to the same client, to see every
|
||||
model the server knows about and its current status (`unloaded` /
|
||||
`loading` / `loaded` / …). Add **LLM Load Model** / **LLM Unload Model**
|
||||
nodes, wired to the same client, to explicitly control residency before a
|
||||
chat node needs a model.
|
||||
|
||||
## 5. (Follow-on epic) Swap backends without touching downstream nodes
|
||||
|
||||
Once `llamacpp-integration` ships a **Llama.cpp Client** node, replacing the
|
||||
**Ollama Client** node in step 1 with it — and nothing else — is the whole
|
||||
migration: it emits the same `LLM_CLIENT` socket type, so every node from
|
||||
steps 2–4 keeps working unmodified. That's the point of this feature.
|
||||
|
||||
## Migrating an existing pre-upgrade workflow
|
||||
|
||||
If you have a saved workflow using the old node names (`OllamaClient`,
|
||||
`OllamaChatCompletion`, `OllamaModelSelector`, `OllamaLoadModel`,
|
||||
`OllamaUnloadModel`), ComfyUI will report those node types as missing on
|
||||
load. Replace each with its generic equivalent from the list above and
|
||||
reconnect — behavior is unchanged, only the node names and the
|
||||
`LLM_CLIENT` socket type (replacing `OLLAMA_CLIENT`) are different. See
|
||||
`spec.md`'s User Story 4 and Edge Cases for the full detail.
|
||||
@@ -0,0 +1,94 @@
|
||||
# Research: LLM Provider Abstraction
|
||||
|
||||
All unknowns below were already resolved during DESIGN-phase work on
|
||||
[ADR-007](../../project-management/ADRs/ADR-007-llm-provider-adapter-pattern.md);
|
||||
this file consolidates that research for the plan gate rather than re-deriving it.
|
||||
|
||||
## Decision: `pydantic-ai` for `chat_structured()`, not hand-rolled tool-calling
|
||||
|
||||
**Decision**: Both `OllamaProvider` and the future `LlamaCppProvider` implement
|
||||
`chat_structured()` via `pydantic-ai`'s `Agent`/`output_type`, called through
|
||||
`OpenAIProvider(base_url=<host>/v1)`.
|
||||
|
||||
**Rationale**: A live research pass against current `pydantic-ai` docs/source
|
||||
found `httpx` is a base dependency of `pydantic-ai-slim` itself (not merely
|
||||
pulled in by an OpenAI extra), and `openai`+`tiktoken` are required for any
|
||||
OpenAI-compatible provider — a fixed, one-time dependency tax rather than a
|
||||
per-backend one. `pydantic.create_model()`-built `BaseModel` subclasses
|
||||
(comfydv's existing dynamic-schema pattern) work as `output_type` with no
|
||||
special-casing. `OpenAIProvider(base_url=...)` is one generic code path both
|
||||
Ollama's and llama.cpp's OpenAI-compatible `/v1/chat/completions` reach
|
||||
identically.
|
||||
|
||||
**Alternatives considered**: hand-roll llama.cpp's structured output too
|
||||
(duplicates [ADR-006](../../project-management/ADRs/ADR-006-structured-ollama-output-tool-calling-not-pydantic-ai.md)'s
|
||||
mechanism — rejected, defeats the DRY goal); extract a shared aiohttp-based
|
||||
helper with no new dependencies (rejected — forces re-deriving pydantic-ai's
|
||||
retry/validation machinery by hand for no benefit now that two backends exist
|
||||
to amortize the dependency cost against).
|
||||
|
||||
## Decision: aiohttp stays authoritative for model-management REST calls
|
||||
|
||||
**Decision**: `list_models()` / `load_model()` / `unload_model()` on every
|
||||
provider use `aiohttp` — no dependency change from the existing Ollama
|
||||
integration for this surface.
|
||||
|
||||
**Rationale**: [ADR-004](../../project-management/ADRs/ADR-004-aiohttp-over-httpx-for-ollama.md)'s
|
||||
reasoning (ComfyUI's own server is aiohttp-based; httpx was an unjustified
|
||||
addition) still applies fully to REST calls that don't need pydantic-ai's
|
||||
machinery. ADR-007 narrows ADR-004's scope to exactly this surface, rather
|
||||
than superseding it.
|
||||
|
||||
**Alternatives considered**: route everything (including model management)
|
||||
through `pydantic-ai`/httpx for consistency — rejected, pydantic-ai has no
|
||||
model-lifecycle-management concept (it's a chat/agent framework, not a
|
||||
generic REST client) and would add no value over plain aiohttp calls that
|
||||
already exist and work.
|
||||
|
||||
## Decision: `LLMProvider` as a `Protocol` implemented by stateful provider classes
|
||||
|
||||
**Decision**: `list_models`/`load_model`/`unload_model`/`chat`/`chat_structured`
|
||||
are defined as a `typing.Protocol`, implemented by `OllamaProvider` (and later
|
||||
`LlamaCppProvider`) classes, each constructed once per ComfyUI client node
|
||||
with the connection's host/headers as instance state.
|
||||
|
||||
**Rationale**: This is a Constitution Principle V ("Function Before Class")
|
||||
gate — classes are only justified when there's shared state a group of
|
||||
functions would otherwise have to thread through every call. Here there is:
|
||||
every one of the five protocol methods needs the same host/headers, exactly
|
||||
the connection config [ADR-005](../../project-management/ADRs/ADR-005-ollama-host-config-via-client-node.md)'s
|
||||
config-node pattern centralizes. A `Protocol` (structural typing, no
|
||||
inheritance required) keeps this lightweight — `LlamaCppProvider` doesn't
|
||||
need to import or subclass `OllamaProvider`, it only needs to match the
|
||||
method shapes.
|
||||
|
||||
**Alternatives considered**: module-level functions taking host/headers as
|
||||
explicit parameters on every call — rejected, this reintroduces the exact
|
||||
per-call-site repetition ADR-005 eliminated, and loses the ability for a
|
||||
ComfyUI `LLM_CLIENT` socket to carry one opaque object implementing the
|
||||
protocol (functions can't be typed as a socket payload the way an object
|
||||
implementing a `Protocol` can).
|
||||
|
||||
## Decision: behavior-preserving migration, verified against existing tests
|
||||
|
||||
**Decision**: `tests/test_ollama.py`'s existing assertions (retry bounds,
|
||||
required-string validation, error messages) are the acceptance bar for the
|
||||
migrated `OllamaProvider.chat_structured()` — this is a mechanism swap, not a
|
||||
new capability, per the parent epic's Non-goals and spec FR-008.
|
||||
|
||||
**Rationale**: Constitution Principle III (Test-First) and the epic's
|
||||
explicit framing of this as the riskiest change in the whole llama.cpp
|
||||
proposal (touches a Done, shipped epic's code) both point the same way: the
|
||||
existing test suite is the regression oracle, not a new one written from
|
||||
scratch.
|
||||
|
||||
## Testing approach
|
||||
|
||||
Per Constitution Principle IV (Graceful Degradation Outside ComfyUI), the new
|
||||
`src/comfydv/_llm/` package must not import `comfy`/`server` at module scope,
|
||||
matching `ollama.py`'s existing runtime-guarded pattern. Unit tests mock
|
||||
`aiohttp`/`pydantic-ai` calls (no live server required, matching
|
||||
`tests/test_ollama.py`'s existing convention); the `integration` pytest
|
||||
marker (already defined in `pyproject.toml`, "requiring live Ollama at
|
||||
localhost:11434") is reused, not redefined, for tests that exercise a real
|
||||
local server.
|
||||
@@ -0,0 +1,119 @@
|
||||
# Feature Specification: LLM Provider Abstraction
|
||||
|
||||
**Feature Branch**: `007-llm-provider-abstraction`
|
||||
|
||||
**Created**: 2026-07-11
|
||||
|
||||
**Status**: Draft
|
||||
|
||||
**Input**: User description: "Introduce a shared LLMProvider protocol for comfydv's LLM backend nodes so ComfyUI workflow authors can swap between local inference servers (starting with Ollama, with llama.cpp planned next) without changing their chat/model-management nodes. Migrate the existing Ollama integration onto generic nodes (client config, model list, load, unload, chat completion with optional structured/validated output) backed by this shared interface, per ADR-007."
|
||||
|
||||
## User Scenarios & Testing *(mandatory)*
|
||||
|
||||
### User Story 1 - Connect to a local inference server and get chat responses (Priority: P1)
|
||||
|
||||
As a ComfyUI workflow author, I want to point a single configuration node at my local LLM server and get chat responses through a generic chat node, so I can generate text without hardcoding a server address into every node that needs one.
|
||||
|
||||
**Why this priority**: This is the minimum viable path — without a working connection and a basic chat response, nothing else in this feature has value. It also directly replaces the most-used capability of the existing Ollama integration, so it carries the highest regression risk.
|
||||
|
||||
**Independent Test**: Wire a client configuration node into a chat node, run the workflow against a running local server, and confirm the chat node returns the model's text response.
|
||||
|
||||
**Acceptance Scenarios**:
|
||||
|
||||
1. **Given** a running local inference server and a workflow with a client node wired into a chat node, **When** the workflow executes, **Then** the chat node returns the model's text response.
|
||||
2. **Given** a client node configured with an unreachable server address, **When** the workflow executes, **Then** the chat node reports a clear connection error rather than hanging indefinitely or crashing the workflow.
|
||||
3. **Given** two chat nodes in the same workflow wired to the same client node, **When** the host address is changed on the client node, **Then** both chat nodes use the new address without being edited individually.
|
||||
|
||||
---
|
||||
|
||||
### User Story 2 - Get structured, validated output instead of parsing raw text (Priority: P1)
|
||||
|
||||
As a workflow author, I want to describe the shape of data I need and turn on structured output for a chat node, so downstream nodes receive individually typed fields I can trust are present and non-empty, instead of me parsing free text myself.
|
||||
|
||||
**Why this priority**: This is an existing, relied-upon capability of the current Ollama integration (structured output with retry-on-invalid-response). Preserving it exactly is required for this migration to be considered safe, so it's equal priority to basic chat.
|
||||
|
||||
**Independent Test**: Enable structured output on a chat node with a schema describing two or three fields, run the workflow against a model, and confirm each schema field is exposed as its own typed output socket with a valid value.
|
||||
|
||||
**Acceptance Scenarios**:
|
||||
|
||||
1. **Given** a chat node with structured output enabled and a valid schema, **When** the workflow executes and the model responds correctly, **Then** each schema field is available as its own typed output, and no required field is blank.
|
||||
2. **Given** a model that returns invalid, incomplete, or empty-required-field output, **When** the workflow executes, **Then** the node automatically retries the request up to a configured limit.
|
||||
3. **Given** a model that continues to return invalid output after all retries are exhausted, **When** the workflow executes, **Then** the node fails with a clear, specific error rather than silently passing through invalid or partial data.
|
||||
|
||||
---
|
||||
|
||||
### User Story 3 - Manage which models are resident in memory (Priority: P2)
|
||||
|
||||
As a workflow author running models locally, I want to see which models are currently loaded, loading, or unloaded, and explicitly load or unload a model, so I can control memory usage on my machine without leaving ComfyUI or using a separate terminal.
|
||||
|
||||
**Why this priority**: Valuable and already present in the current Ollama integration, but a workflow can still generate output without ever calling load/unload explicitly (servers can auto-load on first use) — so this is lower risk to defer than basic chat.
|
||||
|
||||
**Independent Test**: Use a model-listing node against a running server, confirm it shows each available model with a current status; use load/unload nodes against one model and confirm its reported status changes accordingly.
|
||||
|
||||
**Acceptance Scenarios**:
|
||||
|
||||
1. **Given** a running local server with at least one available model, **When** a workflow author uses the model-listing node, **Then** they see each available model along with its current status.
|
||||
2. **Given** a model that is not currently loaded, **When** a workflow author runs the load-model node against it, **Then** the model becomes loaded and is then usable by the chat node.
|
||||
3. **Given** a model that is loaded and idle, **When** a workflow author runs the unload-model node against it, **Then** the model is freed from memory and its reported status updates accordingly.
|
||||
|
||||
---
|
||||
|
||||
### User Story 4 - Reconnect an existing workflow after upgrading (Priority: P3)
|
||||
|
||||
As an existing user of the current Ollama nodes, when I open a workflow I saved before this change, I want it to be clear which new node replaces each renamed one, so I can reconnect my workflow with minimal effort and get the same results as before.
|
||||
|
||||
**Why this priority**: This is migration friction, not new capability — it matters for a good upgrade experience but doesn't block anyone building a new workflow from scratch, so it's the lowest priority of the four.
|
||||
|
||||
**Independent Test**: Open a workflow saved against the current Ollama-specific node names, follow the provided migration guidance to reconnect it to the new generic nodes, and confirm it produces the same output as before, given the same inputs and model.
|
||||
|
||||
**Acceptance Scenarios**:
|
||||
|
||||
1. **Given** a saved workflow using the current Ollama-specific node and connection-socket names, **When** it is opened after upgrading, **Then** ComfyUI reports the now-missing node types (standard ComfyUI behavior for renamed nodes), and documentation identifies the replacement node for each one.
|
||||
2. **Given** a workflow that has been reconnected to the new generic nodes, **When** it executes with the same inputs and model as before the upgrade, **Then** it produces equivalent output.
|
||||
|
||||
---
|
||||
|
||||
### Edge Cases
|
||||
|
||||
- What happens when the configured server address is unreachable at the moment a model-listing, load, or unload node runs (not just the chat node)?
|
||||
- What happens when a workflow author supplies an invalid or malformed schema to structured output, rather than an invalid model response?
|
||||
- What happens when the connected server does not support structured/validated output at all?
|
||||
- What happens to an in-flight chat request if the model it depends on is unloaded by another node in the same workflow run?
|
||||
- What happens when a workflow author tries to wire a pre-upgrade Ollama-specific node's output into a new generic node, or vice versa? (Expected: ComfyUI's own type-checking refuses the connection, since the socket types differ — this is the intended, safe failure mode, not a bug to work around.)
|
||||
|
||||
## Requirements *(mandatory)*
|
||||
|
||||
### Functional Requirements
|
||||
|
||||
- **FR-001**: The system MUST allow a workflow author to configure a connection to a local inference server once and reuse that single configuration across multiple nodes in the same workflow.
|
||||
- **FR-002**: The system MUST allow a workflow author to request either free-text or schema-validated structured output from the same chat node, choosing per request.
|
||||
- **FR-003**: When structured output is requested, the system MUST validate the response against the supplied schema and MUST NOT deliver output to downstream nodes where a required field is missing or empty.
|
||||
- **FR-004**: When validation fails, the system MUST retry the request automatically up to a configurable limit before reporting a clear, actionable error that identifies the model, the number of attempts made, and a snippet of the last invalid response.
|
||||
- **FR-005**: The system MUST allow a workflow author to list available models on a connected server along with each model's current residency status.
|
||||
- **FR-006**: The system MUST allow a workflow author to explicitly load a model into memory and explicitly unload a model from memory.
|
||||
- **FR-007**: The system's chat and model-management nodes MUST behave identically regardless of which supported local inference server is connected, given equivalent inputs.
|
||||
- **FR-008**: The existing chat and structured-output behavior for the currently-supported local inference server (Ollama) MUST be unchanged in outcome after this migration — same retry limits, same validation rules, same error conditions — since this feature changes the underlying mechanism, not the capability.
|
||||
- **FR-009**: The system MUST document, for each node type renamed or removed by this change, which new node replaces it.
|
||||
|
||||
### Key Entities *(include if feature involves data)*
|
||||
|
||||
- **Provider connection**: A configured connection to one local inference server (address and any authentication), created once and reused by every model-management and chat node that needs it.
|
||||
- **Model**: An inference model known to a provider connection, identified by name, with a current residency status (e.g., unloaded, loading, loaded, and — on servers that support it — sleeping or downloading).
|
||||
- **Chat request/response**: A request for a model's output, optionally carrying a schema describing the required shape of a structured response, and the corresponding validated or free-text result.
|
||||
|
||||
## Success Criteria *(mandatory)*
|
||||
|
||||
### Measurable Outcomes
|
||||
|
||||
- **SC-001**: A workflow author can go from no nodes to a working chat response using no more than two nodes (one connection node, one chat node).
|
||||
- **SC-002**: Structured-output workflows never deliver a blank or missing required field to a downstream node — every request either produces fully valid data or a clear error, with zero silent partial results.
|
||||
- **SC-003**: Existing example/reference workflows built against the current Ollama nodes remain reproducible on the new nodes with equivalent output, after a workflow author reconnects the renamed nodes.
|
||||
- **SC-004**: Adding support for a second local inference server (planned as a follow-on feature) requires no visible change to chat or model-management node behavior — only a new connection node is needed.
|
||||
|
||||
## Assumptions
|
||||
|
||||
- Workflow authors run their own local inference server (e.g., Ollama) reachable over HTTP from the machine running ComfyUI; this feature does not host, install, or manage that server.
|
||||
- Users with workflows saved against the current Ollama-specific node and socket names will need to manually reconnect them after upgrading. This is an accepted, intentional breaking change (confirmed 2026-07-11), not a defect — see FR-009 for the mitigation (documented replacement mapping), not automatic migration.
|
||||
- Support for a second local inference server (llama.cpp) is planned as a separate, follow-on feature and is out of scope here — this feature only needs to prove the shared design works end-to-end for one real backend (Ollama).
|
||||
- Structured-output schemas remain limited to flat object shapes with typed properties, consistent with what the current Ollama integration already supports — deeper nested schemas are unaffected by (neither improved nor degraded by) this change.
|
||||
- No new observability/tracing capability is introduced for workflow authors as part of this feature, even though the underlying mechanism change makes it feasible to add later.
|
||||
@@ -0,0 +1,159 @@
|
||||
# Tasks: LLM Provider Abstraction
|
||||
|
||||
**Input**: Design documents from `/specs/007-llm-provider-abstraction/`
|
||||
|
||||
**Prerequisites**: plan.md, spec.md, research.md, data-model.md, contracts/llm_provider_protocol.md
|
||||
|
||||
**Tests**: First-class (spec carries Acceptance Scenarios) — every implementation task has a paired failing-test task (`-T`/`-I` suffix) per BEACON's test-first discipline.
|
||||
|
||||
**Organization**: Tasks are grouped by user story (spec.md priorities P1/P1/P2/P3) to enable independent implementation and testing of each.
|
||||
|
||||
## Format: `[ID] [P?] [Story] Description`
|
||||
|
||||
- **[P]**: Can run in parallel (different files, no dependencies)
|
||||
- **[Story]**: Which user story this task belongs to (US1–US4)
|
||||
- **-T / -I**: paired test (red) / implementation (green) — a `-I` task is never parallel with its own `-T`
|
||||
|
||||
## Path Conventions
|
||||
|
||||
Single project: `src/comfydv/`, `tests/` at repository root (per plan.md's Project Structure).
|
||||
|
||||
---
|
||||
|
||||
## Phase 1: Setup
|
||||
|
||||
- [ ] T001 Add `pydantic-ai` and `openai` to `pyproject.toml` dependencies; curate the addition into `requirements.txt` per [ADR-003](../../project-management/ADRs/ADR-003-requirements-txt-authoring-policy.md)
|
||||
- [ ] T002 [P] Create `src/comfydv/_llm/__init__.py` (empty package init)
|
||||
|
||||
---
|
||||
|
||||
## Phase 2: Foundational (Blocking Prerequisites)
|
||||
|
||||
**⚠️ CRITICAL**: No user story work can begin until this phase is complete.
|
||||
|
||||
- [ ] T003 [P] Define `Message`, `ModelStatus`, `ModelInfo` in `src/comfydv/_llm/provider.py` per `data-model.md`
|
||||
- [ ] T004 Define the `LLMProvider` `Protocol` in `src/comfydv/_llm/provider.py` per `contracts/llm_provider_protocol.md` (depends on T003)
|
||||
- [ ] T005 [P] Add the `LLM_CLIENT` custom ComfyUI socket type constant in `src/comfydv/ollama.py`, alongside the existing `OLLAMA_CLIENT` declaration
|
||||
- [ ] T006 Scaffold `OllamaProvider.__init__(self, host, headers)` in `src/comfydv/_llm/ollama_provider.py`, porting the existing module-level `_post_json`/`_fetch_models` helpers from `ollama.py` into it as private methods — behavior-preserving port, not a rewrite (depends on T004)
|
||||
|
||||
**Checkpoint**: protocol + provider skeleton exist; user story work can begin.
|
||||
|
||||
---
|
||||
|
||||
## Phase 3: User Story 1 — Connect to a local server and get chat responses (Priority: P1) 🎯 MVP
|
||||
|
||||
**Goal**: A workflow author wires a client node into a generic chat node and gets a text response.
|
||||
|
||||
**Independent Test**: Wire `OllamaClient` → `ChatCompletion`, run against a live server, confirm text output.
|
||||
|
||||
- [ ] T007-T [US1] Write FAILING test: `OllamaClient` node constructs and outputs an `OllamaProvider` via the `LLM_CLIENT` socket, in `tests/test_ollama.py` (witnesses `features/us1_connect_and_chat.feature` scenario "Client node feeds a chat node")
|
||||
- [ ] T007-I [US1] Update `OllamaClient` in `src/comfydv/ollama.py` to construct and output an `OllamaProvider` via `LLM_CLIENT` — makes T007-T pass
|
||||
- [ ] T008-T [P] [US1] Write FAILING test: `OllamaProvider.chat()` returns model text via the existing `/api/chat` aiohttp call, in `tests/test_llm_provider.py`
|
||||
- [ ] T008-I [US1] Implement `OllamaProvider.chat()` in `src/comfydv/_llm/ollama_provider.py` (port of the existing non-structured path from `OllamaChatCompletion.chat`) — makes T008-T pass
|
||||
- [ ] T009-T [US1] Write FAILING test: generic `ChatCompletion` node (non-structured) calls `provider.chat()` and surfaces a clear error on an unreachable host, in `tests/test_ollama.py` (witnesses `features/us1_connect_and_chat.feature` scenarios "Client node feeds a chat node" and "Unreachable server surfaces a clear error")
|
||||
- [ ] T009-I [US1] Rename `OllamaChatCompletion` → `ChatCompletion` in `src/comfydv/ollama.py`, delegate the non-structured path to `LLMProvider.chat()`, update `NODE_CLASS_MAPPINGS`/`NODE_DISPLAY_NAME_MAPPINGS` in `src/comfydv/__init__.py` — makes T009-T pass (depends on T007-I, T008-I)
|
||||
- [ ] T010-T [US1] Write FAILING test: two `ChatCompletion` nodes sharing one `OllamaClient` both pick up a host change, in `tests/test_ollama.py` (witnesses `features/us1_connect_and_chat.feature` scenario "One client node configures multiple chat nodes")
|
||||
- [ ] T010-I [US1] Verify/adjust that `OllamaClient` → `OllamaProvider` construction happens per node execution (not stale-cached), so a shared client's host change propagates — makes T010-T pass
|
||||
|
||||
**Checkpoint**: US1 fully functional and independently testable (MVP).
|
||||
|
||||
---
|
||||
|
||||
## Phase 4: User Story 2 — Structured, validated output (Priority: P1)
|
||||
|
||||
**Goal**: The chat node's `structured_output` toggle returns validated typed fields via the shared `pydantic-ai` mechanism.
|
||||
|
||||
**Independent Test**: Enable `structured_output` with a schema, run against a model, confirm typed sockets are populated and never blank.
|
||||
|
||||
- [ ] T011-T [P] [US2] Write FAILING test: `chat_structured()` builds a dynamic `pydantic` model via `create_model()` from a JSON-Schema input and returns validated fields, in `tests/test_llm_provider.py` (witnesses `features/us2_structured_output.feature` scenario "Valid structured response exposes typed fields")
|
||||
- [ ] T011-I [US2] Implement the shared `chat_structured()` helper in `src/comfydv/_llm/chat.py` using `pydantic-ai`'s `Agent`/`output_type` through `OpenAIProvider(base_url=<host>/v1)`, reusing the existing JSON-Schema→pydantic `create_model()` logic ported from `ollama.py` — makes T011-T pass (depends on T004)
|
||||
- [ ] T012-T [US2] Write FAILING test: invalid, incomplete, or blank-required-field responses trigger automatic retry up to `max_retries`, in `tests/test_llm_provider.py` (witnesses `features/us2_structured_output.feature` scenario "Invalid response triggers automatic retry")
|
||||
- [ ] T012-I [US2] Implement the bounded retry loop (0–5, matching ADR-006's existing contract) around the `pydantic-ai` call in `src/comfydv/_llm/chat.py` — makes T012-T pass (depends on T011-I)
|
||||
- [ ] T013-T [US2] Write FAILING test: exhausted retries raise `RuntimeError` naming the model, attempt count, and a truncated last-response snippet, in `tests/test_llm_provider.py` (witnesses `features/us2_structured_output.feature` scenario "Exhausted retries fail clearly instead of passing through bad data")
|
||||
- [ ] T013-I [US2] Implement the exhausted-retry error path in `src/comfydv/_llm/chat.py`, matching ADR-006's existing error message contract exactly — makes T013-T pass (depends on T012-I)
|
||||
- [ ] T014-T [US2] Write FAILING test: `ChatCompletion`'s `structured_output=True` path wires a schema through `chat_structured()` to per-field dynamic ComfyUI output sockets, in `tests/test_ollama.py`
|
||||
- [ ] T014-I [US2] Wire `ChatCompletion`'s existing `structured_output`/`output_schema` inputs to `LLMProvider.chat_structured()`, preserving the existing dynamic-socket (`RETURN_TYPES` mutation via `unique_id`) UX from ADR-006 — makes T014-T pass (depends on T009-I, T013-I)
|
||||
|
||||
**Checkpoint**: US1 + US2 both independently functional — matches today's Ollama capability, now on the shared mechanism.
|
||||
|
||||
---
|
||||
|
||||
## Phase 5: User Story 3 — Manage model residency (Priority: P2)
|
||||
|
||||
**Goal**: List/load/unload models through generic nodes against any connected provider.
|
||||
|
||||
**Independent Test**: List models via `LLMModelSelector`; load/unload one via `LLMLoadModel`/`LLMUnloadModel`; confirm status changes.
|
||||
|
||||
- [ ] T015-T [P] [US3] Write FAILING test: `OllamaProvider.list_models()` returns `ModelInfo` entries with status normalized into `ModelStatus` (never emitting `sleeping`/`downloading`), in `tests/test_llm_provider.py` (witnesses `features/us3_model_lifecycle.feature` scenario "List models with current status")
|
||||
- [ ] T015-I [US3] Implement `OllamaProvider.list_models()` in `src/comfydv/_llm/ollama_provider.py` (port of the existing `_fetch_models`/`/api/tags` logic, reusing the existing `_TTLLRUCache`) — makes T015-T pass (depends on T006)
|
||||
- [ ] T016-T [P] [US3] Write FAILING test: `OllamaProvider.load_model()`/`unload_model()` are idempotent and use `keep_alive` semantics equivalent to today's `OllamaLoadModel`/`OllamaUnloadModel`, in `tests/test_llm_provider.py` (witnesses `features/us3_model_lifecycle.feature` scenarios "Load a model into memory" and "Unload a model from memory")
|
||||
- [ ] T016-I [US3] Implement `OllamaProvider.load_model()`/`unload_model()` in `src/comfydv/_llm/ollama_provider.py` (port of the existing `keep_alive` logic) — makes T016-T pass (depends on T006)
|
||||
- [ ] T017-T [US3] Write FAILING test: generic `LLMModelSelector` node returns model+status pairs from any connected `LLM_CLIENT`, in `tests/test_ollama.py`
|
||||
- [ ] T017-I [US3] Rename `OllamaModelSelector` → `LLMModelSelector` in `src/comfydv/ollama.py`, delegate to `LLMProvider.list_models()`, update `NODE_CLASS_MAPPINGS` — makes T017-T pass (depends on T015-I)
|
||||
- [ ] T018-T [US3] Write FAILING test: generic `LLMLoadModel`/`LLMUnloadModel` nodes call `LLMProvider.load_model()`/`unload_model()` and reflect updated status, in `tests/test_ollama.py`
|
||||
- [ ] T018-I [US3] Rename `OllamaLoadModel`/`OllamaUnloadModel` → `LLMLoadModel`/`LLMUnloadModel` in `src/comfydv/ollama.py`, delegate to the protocol, update `NODE_CLASS_MAPPINGS` — makes T018-T pass (depends on T016-I)
|
||||
|
||||
**Checkpoint**: US1 + US2 + US3 independently functional.
|
||||
|
||||
---
|
||||
|
||||
## Phase 6: User Story 4 — Reconnect an existing workflow after upgrading (Priority: P3)
|
||||
|
||||
**Goal**: A clear old→new node mapping exists, and migrated workflows are output-equivalent.
|
||||
|
||||
**Independent Test**: Follow the mapping to reconnect a pre-upgrade workflow; confirm equivalent output.
|
||||
|
||||
- [ ] T019-T [US4] Write FAILING test: every renamed/removed node and socket name (`OllamaChatCompletion`, `OllamaModelSelector`, `OllamaLoadModel`, `OllamaUnloadModel`, `OLLAMA_CLIENT`) resolves to a documented replacement via a migration-mapping constant, in `tests/test_ollama.py` (witnesses `features/us4_migration.feature` scenario "Renamed nodes are reported with a documented replacement")
|
||||
- [ ] T019-I [US4] Add a migration mapping (old node/socket name → new node/socket name) as a module-level constant in `src/comfydv/ollama.py`, and surface it in the replacement nodes' `DESCRIPTION`/docstrings per FR-009 — makes T019-T pass (depends on T009-I, T017-I, T018-I)
|
||||
- [ ] T020 [US4] Run the full `tests/test_ollama.py` suite against the migrated implementation and confirm every existing assertion still passes unmodified in behavior (SC-003 equivalence check; witnesses `features/us4_migration.feature` scenario "Reconnected workflow produces equivalent output") — regression pass, not a new test
|
||||
- [ ] T021 [P] [US4] Update `quickstart.md`'s "Migrating an existing pre-upgrade workflow" section if the actual replacement mapping (T019-I) differs from what was drafted at plan time
|
||||
|
||||
**Checkpoint**: all four user stories independently functional; migration path documented.
|
||||
|
||||
---
|
||||
|
||||
## Phase 7: Polish & Cross-Cutting Concerns
|
||||
|
||||
- [ ] T022 [P] Run `uv run ruff check --fix && uv run ruff format` across `src/comfydv/_llm/` and the modified `ollama.py`/`__init__.py`
|
||||
- [ ] T023 [P] Run `uv run ty check` and resolve any typing errors introduced by the `LLMProvider` `Protocol`
|
||||
- [ ] T024 Confirm Constitution Principle IV: `src/comfydv/_llm/` imports no `comfy`/`server` at module scope (guarded, matching `ollama.py`'s existing pattern)
|
||||
- [ ] T025 Run `beacon doctor --strict` and resolve any findings, including `spec-bdd-coverage` and `tdd-commit-discipline` for this spec
|
||||
- [ ] T026 Walk `quickstart.md` end-to-end manually against a live local server to confirm the documented flow works as written
|
||||
|
||||
---
|
||||
|
||||
## Dependencies & Execution Order
|
||||
|
||||
### Phase Dependencies
|
||||
|
||||
- **Setup (Phase 1)**: no dependencies
|
||||
- **Foundational (Phase 2)**: depends on Setup — BLOCKS all user stories
|
||||
- **User Stories (Phase 3–6)**: all depend on Foundational; US1 has no dependency on US2/US3/US4; US2's `ChatCompletion` wiring (T014) depends on US1's node rename (T009-I); US3 is independent of US1/US2 except for sharing `OllamaProvider`'s constructor (T006); US4 depends on the node renames done in US1/US3 (T009-I, T017-I, T018-I) since it documents them
|
||||
- **Polish (Phase 7)**: depends on all four user stories
|
||||
|
||||
### Parallel Opportunities
|
||||
|
||||
- T002 (package init) can run alongside T001 (dependency addition)
|
||||
- T003 and T005 can run in parallel (different files/concerns) within Foundational
|
||||
- T008-T (provider-level test) can run in parallel with T007-T (node-level test) — different files
|
||||
- T011-T, T015-T, T016-T can each start as soon as Foundational is done, in parallel with US1 — different files, no shared dependency beyond T004/T006
|
||||
- T022/T023 (lint/type-check) can run in parallel in Polish
|
||||
|
||||
---
|
||||
|
||||
## Implementation Strategy
|
||||
|
||||
### MVP First
|
||||
|
||||
1. Phase 1 (Setup) → Phase 2 (Foundational) → Phase 3 (US1) → **STOP and validate US1 independently** against a live local server.
|
||||
|
||||
### Incremental Delivery
|
||||
|
||||
1. Setup + Foundational → foundation ready.
|
||||
2. US1 → validate → this alone restores basic chat parity with today's Ollama integration, on the new mechanism.
|
||||
3. US2 → validate → restores structured-output parity (the ADR-006→ADR-007 migration is now complete in behavior).
|
||||
4. US3 → validate → restores model-management parity.
|
||||
5. US4 → validate → migration guidance ships; full regression pass (T020) confirms SC-003.
|
||||
6. Polish.
|
||||
|
||||
Each story adds value without breaking the previous one — this mirrors the epic's own framing: US1+US2 together are the risky "prove the migration is behavior-preserving" core; US3 and US4 round out parity and upgrade experience.
|
||||
Reference in New Issue
Block a user