docs(beacon): properly spec the atomic node cutover (issue #16)
Full line-by-line inventory of every reference in src/comfydv/ollama.py and tests/test_ollama.py (1820 lines, read in full — not estimated) affected by the OllamaClient/OllamaChatCompletion/OllamaModelSelector/ OllamaLoadModel/OllamaUnloadModel rename, plus the design decisions it surfaced that "just rename it" was hiding: - Single source of truth for HTTP/cache infra (comfydv._llm.ollama_provider) - client == "<host string>" equality is removed, not preserved (OllamaProvider isn't a str subclass) - Bare-string client backward compatibility is removed (undocumented side effect of the old str-subclass trick, never an intended feature) - Test-layer split: node-contract/delegation tests stay in test_ollama.py using a _FakeProvider double; Ollama-wire-protocol tests move to a new test_ollama_provider.py — this is what actually resolves the 35 relocated _post_json monkeypatches, rather than patching them 1:1 at a seam that no longer exists - Structured-output retry tests are not ported 1:1 — already covered by tests/test_llm_chat_structured.py's existing 6 tests Produces a 12-step sequenced task list (T-CUT-01..12) in atomic-cutover-plan.md that supersedes tasks.md's original US1/US3 decomposition (struck through, kept for history). This is now the authoritative plan for the next BUILD session to execute. 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
736ffd1e77
commit
ef2464aebb
@@ -89,6 +89,18 @@ correction note for full detail. Open question for the next session: does
|
||||
this take priority over `ux-and-install` (active, 1/4 specs shipped), since
|
||||
llama.cpp (issue #15) has no deadline.
|
||||
|
||||
**2026-07-11 — properly specced:** the deferred cutover is now fully
|
||||
inventoried and planned in
|
||||
`specs/007-llm-provider-abstraction/atomic-cutover-plan.md` — a full
|
||||
line-by-line read of the ~125 affected references (not an estimate), the
|
||||
design decisions it surfaced (cache-singleton duplication, `client ==
|
||||
"<string>"` equality breaking, bare-string-client backward compat removal,
|
||||
and a test-layer split so the 35 `_post_json` monkeypatches land at the
|
||||
right architectural seam), and a 12-step sequenced task list (T-CUT-01 …
|
||||
T-CUT-12). This is now the authoritative implementation plan for the
|
||||
cutover — the next BUILD session executes it directly rather than
|
||||
re-deriving the approach.
|
||||
|
||||
`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
|
||||
|
||||
@@ -0,0 +1,178 @@
|
||||
# Atomic Cutover Plan: Ollama Node Rename → Generic LLM Nodes
|
||||
|
||||
Companion to `tasks.md`'s "⚠️ Correction (2026-07-11)" section and
|
||||
[issue #16](https://github.com/darth-veitcher/comfydv/issues/16). This is
|
||||
the properly-specced version of that deferred work — based on a full,
|
||||
line-by-line inventory of every one of the ~125 affected references in
|
||||
`src/comfydv/ollama.py` and `tests/test_ollama.py` (1820 lines, read in
|
||||
full), not an estimate.
|
||||
|
||||
The inventory surfaced that this isn't one mechanical find-and-replace —
|
||||
several genuine design decisions were implicit in "rename it" and needed
|
||||
resolving before any code changes. Those decisions are below, followed by
|
||||
the sequenced task list that implements them.
|
||||
|
||||
## Resolved decisions
|
||||
|
||||
### D1 — Single source of truth for HTTP/cache infra
|
||||
|
||||
`comfydv._llm.ollama_provider` owns `_post_json`, `_run_async`,
|
||||
`_fetch_models`, `_TTLLRUCache`, `_cache_key`, `_MODEL_LIST_CACHE` (already
|
||||
ported there — see `ollama_provider.py`). `ollama.py` stops defining its
|
||||
own copies of these. The one remaining non-node use case —
|
||||
`_load_default_models()` (combo-widget population at import time, before
|
||||
any `OllamaClient` node exists) and the `/dv/ollama/models` refresh route —
|
||||
imports `_fetch_models`/`_run_async` from `comfydv._llm.ollama_provider`
|
||||
instead of duplicating them. `_CHAT_RESPONSE_CACHE` and `_post_json`
|
||||
disappear from `ollama.py` entirely — nothing there needs them once
|
||||
load/unload/chat delegate to `client.*`.
|
||||
|
||||
**Why not keep two copies:** they were already flagged as byte-identical by
|
||||
the inventory (§2.13) — the only reason `ollama.py` still has them is that
|
||||
nothing has repointed the imports yet. Keeping a second copy "just in case"
|
||||
is exactly the kind of duplication ADR-007 exists to eliminate.
|
||||
|
||||
### D2 — `client == "<host string>"` equality is removed, not preserved
|
||||
|
||||
`OllamaProvider` is a plain object, not a `str` subclass — this is a
|
||||
deliberate consequence of the adapter boundary (ADR-007), not an oversight
|
||||
to work around. Two tests assert string equality on `client`
|
||||
(`test_client_outputs_ollama_client_type:208-209`,
|
||||
`test_client_carries_headers:1233-1234`) — both get rewritten to assert
|
||||
`client.host == "..."` and `isinstance(client, OllamaProvider)`.
|
||||
`OllamaClientType` (the `str`-subclass, `ollama.py:97-109`) is left in
|
||||
place but becomes unused by `OllamaClient.create_client()` — not deleted in
|
||||
this cutover (no test depends on deleting it, and removing a class nothing
|
||||
references is a separate, lower-risk cleanup, not part of this bullet's
|
||||
scope).
|
||||
|
||||
### D3 — Bare-string `client` backward compatibility is removed
|
||||
|
||||
`test_plain_string_client_has_no_headers` (`test_ollama.py:1328-1344`)
|
||||
documents and tests that wiring a plain `STRING` node directly into
|
||||
`client` (skipping `OllamaClient` entirely) silently works, because
|
||||
`f"{client}/..."` succeeds on any string. This was never a documented,
|
||||
intended feature — it's a side effect of `OllamaClientType` being a `str`
|
||||
subclass, not mentioned in ADR-005 or the original Ollama epic's spec. Once
|
||||
node methods call `client.chat(...)`/`client.load_model(...)`, a bare
|
||||
string raises `AttributeError`. **This test is deleted**, not rewritten —
|
||||
its premise (bare-string clients are supported) is being intentionally
|
||||
removed, and asserting the new failure mode would just be testing that
|
||||
Python raises `AttributeError` on missing methods, which isn't
|
||||
comfydv-specific behavior worth a test.
|
||||
|
||||
The same bare-string pattern appears incidentally in ~8 other tests
|
||||
(`ollama_host` fixture returns a plain string, used as `client=ollama_host`
|
||||
in several integration tests — `test_ollama.py:220, 386, 407, 588, ...`).
|
||||
These need `client=OllamaClient().create_client(ollama_host)[0]` instead of
|
||||
`client=ollama_host` — a required edit, not optional, since they'll raise
|
||||
`AttributeError` otherwise. See task T-CUT-08 below.
|
||||
|
||||
### D4 — Test layer split (resolves all 35 relocated `_post_json` monkeypatches)
|
||||
|
||||
This is the biggest structural decision. Today, `test_ollama.py` tests
|
||||
ComfyUI node behavior by mocking `aiohttp` at the `ollama_mod._post_json`
|
||||
seam and asserting on the exact Ollama wire payload (`keep_alive`,
|
||||
`/api/generate`, tool-calling JSON shape, retry counts) *through* the node.
|
||||
That seam moves — nodes no longer call `_post_json` directly, they call
|
||||
`client.chat(...)` etc. Two options: (a) keep patching at whatever the new
|
||||
seam is, 1:1 per test, or (b) recognize this is an architectural boundary
|
||||
and split coverage accordingly. Going with **(b)**:
|
||||
|
||||
- **`tests/test_ollama.py`** — ComfyUI node **contract + delegation** only.
|
||||
A new `_FakeProvider` test double (implements `list_models`/
|
||||
`load_model`/`unload_model`/`chat`/`chat_structured`, records calls made
|
||||
to it) stands in for `client`. Tests assert: right method called, right
|
||||
arguments passed, return value flows through to the node's output tuple
|
||||
correctly. **No `aiohttp`/`_post_json` mocking at this layer anymore.**
|
||||
This directly matches the protocol contract's own rule ("generic nodes
|
||||
MUST NOT branch on which concrete provider type they received") — if the
|
||||
node tests don't need to know Ollama's wire format, they shouldn't mock
|
||||
it either.
|
||||
- **`tests/test_ollama_provider.py`** (new file) — `OllamaProvider`'s
|
||||
actual Ollama-wire-protocol behavior: `/api/generate`+`keep_alive` int
|
||||
shape, `/api/tags` parsing into `ModelInfo`, header/timeout forwarding,
|
||||
response caching, cache-key composition. This is where the *substance* of
|
||||
today's 35 `_post_json` monkeypatches lands — not 1:1, since several
|
||||
collapse or move (see D5).
|
||||
- **`tests/test_llm_chat_structured.py`** (exists, unchanged) — already
|
||||
covers the shared retry/validation/error-contract mechanism.
|
||||
|
||||
### D5 — Structured-output retry tests are not ported 1:1
|
||||
|
||||
`TestStructuredOutput` has 15 tests monkeypatching `_post_json` to assert
|
||||
exact retry-count behavior (`test_retries_on_invalid_json_then_succeeds`,
|
||||
`test_exhausts_retries_raises_runtime_error`, `test_max_retries_clamped_*`,
|
||||
etc.). Once `OllamaProvider.chat_structured()` delegates to the already-
|
||||
tested shared `chat_structured()` helper (`src/comfydv/_llm/chat.py`,
|
||||
covered by `tests/test_llm_chat_structured.py`'s 6 tests), re-asserting
|
||||
retry counts at the Ollama-node layer duplicates that coverage without
|
||||
adding confidence. Replaced with:
|
||||
- A handful of `test_ollama.py` delegation tests: `ChatCompletion` with
|
||||
`structured_output=True` calls `client.chat_structured(model, messages,
|
||||
schema, ...)` with the right schema/model/messages.
|
||||
- One `test_ollama_provider.py` test: `OllamaProvider.chat_structured()`
|
||||
builds `base_url=f"{self.host}/v1"` and forwards to the shared helper
|
||||
with the right arguments.
|
||||
- `test_structured_output_true_sends_tool_call_payload` (asserts the exact
|
||||
`tools`/`tool_choice` JSON shape) is **deleted** — that's `pydantic-ai`'s
|
||||
internal tool-calling mechanism now, not comfydv's; asserting on a
|
||||
third-party library's internals isn't a test worth keeping.
|
||||
- Pure schema-parsing tests that don't touch HTTP at all (`_parse_output_schema`
|
||||
fail-fast checks, `_coerce_structured_value`, dynamic-socket
|
||||
`RETURN_TYPES` mutation) are unaffected — they test code that stays in
|
||||
`ollama.py` unchanged, only need the class-name rename.
|
||||
|
||||
### D6 — `_client_headers()` is deleted
|
||||
|
||||
Dead code once `OllamaLoadModel`/`OllamaUnloadModel`/`OllamaChatCompletion`
|
||||
delegate to `client.*` (headers become internal to `OllamaProvider`,
|
||||
captured once at construction). No test calls it directly.
|
||||
|
||||
### D7 — Contract doc gets a small fix
|
||||
|
||||
`contracts/llm_provider_protocol.md`'s illustrative code sample is missing
|
||||
`timeout_secs` on `chat()`/`chat_structured()` — the actual `provider.py`
|
||||
(built after the doc) has it. Fix the doc to match the real protocol; docs
|
||||
follow code here, not the reverse.
|
||||
|
||||
### D8 — `conftest.py`'s `_clear_ollama_caches` fixture repoints
|
||||
|
||||
Per D1, there's now one cache source (`comfydv._llm.ollama_provider`).
|
||||
Fixture imports `_CHAT_RESPONSE_CACHE`/`_MODEL_LIST_CACHE` from there
|
||||
instead of `comfydv.ollama`, and `ChatCompletion` instead of
|
||||
`OllamaChatCompletion`. This is the single highest-priority fixture change
|
||||
— every `TestResponseCache` test (12) and every `structured_output`-
|
||||
toggling test depends on it for isolation.
|
||||
|
||||
## Sequenced task list
|
||||
|
||||
Replaces `tasks.md`'s Phase 3 (US1) + Phase 5 (US3) + the deferred T014.
|
||||
One coordinated PR/session, ordered so the codebase stays important at each
|
||||
step even though it can't be split across separate merges (per the
|
||||
2026-07-11 correction — this is genuinely atomic).
|
||||
|
||||
1. **T-CUT-01** — `ollama.py`: add `from comfydv._llm.ollama_provider import OllamaProvider, _fetch_models, _run_async` (drop the local `_post_json`, `_TTLLRUCache`, `_cache_key`, `_MODEL_LIST_CACHE`, `_CHAT_RESPONSE_CACHE`, `_run_async`, `_fetch_models`, `_post_json` definitions — lines 42-194 collapse to the import). Repoint `_load_default_models()` and the `/dv/ollama/models` route to the imported `_fetch_models`. (D1)
|
||||
2. **T-CUT-02** — `ollama_provider.py`: implement `OllamaProvider.list_models()` (port `_fetch_models`'s `/api/tags` logic, map to `ModelInfo`/`ModelStatus.UNLOADED`/`LOADED` — Ollama never emits `SLEEPING`/`DOWNLOADING`, per ADR-007's documented approximation), `load_model()` (port `/api/generate` + `keep_alive: -1`), `unload_model()` (port `/api/generate` + `keep_alive: 0`), `chat()` (port native `/api/chat` non-structured path), `chat_structured()` (build `base_url=f"{self.host}/v1"`, delegate to `comfydv._llm.chat.chat_structured()`).
|
||||
3. **T-CUT-03** — `tests/test_ollama_provider.py` (new): tests for T-CUT-02's method bodies, mocking at `ollama_provider_mod._post_json`/`aiohttp.ClientSession` — ports the *substance* of the 35 relocated monkeypatches per D4/D5 (not 1:1 — collapses redundant retry-count tests per D5).
|
||||
4. **T-CUT-04** — `ollama.py`: `OllamaClient.RETURN_TYPES = ("LLM_CLIENT",)`, `create_client()` returns `OllamaProvider(host, headers)`. (D2)
|
||||
5. **T-CUT-05** — `ollama.py`: rename `OllamaModelSelector`→`LLMModelSelector`, `OllamaLoadModel`→`LLMLoadModel`, `OllamaUnloadModel`→`LLMUnloadModel`, `OllamaChatCompletion`→`ChatCompletion`; every `"OLLAMA_CLIENT"` input socket → `"LLM_CLIENT"`; rewrite the 3 method bodies (`load_model`, `unload_model`, `chat`) to delegate to `client.*` instead of `_post_json`/f-string URLs; delete `_client_headers` (D6); update the 3 `OllamaChatCompletion.*` references in the `/dv/ollama/update_structured_outputs` route body.
|
||||
6. **T-CUT-06** — `src/comfydv/__init__.py`: update imports and `NODE_CLASS_MAPPINGS`/`NODE_DISPLAY_NAME_MAPPINGS` for the 4 renamed classes.
|
||||
7. **T-CUT-07** — `tests/conftest.py`: repoint `_clear_ollama_caches` (D8) and `first_generative_model`'s `_fetch_models` import (D1).
|
||||
8. **T-CUT-08** — `tests/test_ollama.py`: update the import block (4 class renames); add `_FakeProvider` test double; convert every `_post_json`-monkeypatched test to use `_FakeProvider` as `client` instead (D4); replace bare-string `client=ollama_host`/`client="http://..."` usages with a constructed provider (D3); rewrite the 2 `client == "<string>"` assertions (D2); delete `test_plain_string_client_has_no_headers` (D3) and `test_structured_output_true_sends_tool_call_payload` (D5); collapse the 15 `TestStructuredOutput` retry-count tests per D5; update `TestNodeContracts`'s `NODE_CLASSES` list (4 renames).
|
||||
9. **T-CUT-09** — `contracts/llm_provider_protocol.md`: add missing `timeout_secs` params (D7).
|
||||
10. **T-CUT-10** — Full suite green (`uv run pytest -m "not integration and not system"`), `ruff check --fix && ruff format`, `ty check`, `beacon doctor --strict`.
|
||||
11. **T-CUT-11** — `tasks.md`: mark T007-T010/T015-T018/T014 done, referencing this plan; migration mapping (old→new names, FR-009) as a module-level constant/docstring in `ollama.py`.
|
||||
12. **T-CUT-12** — Manual smoke test against a live local Ollama server per `quickstart.md`.
|
||||
|
||||
## What stays exactly as originally scoped
|
||||
|
||||
`OllamaHeader*`, `OllamaOption*`, `OllamaDebugHistory`, `OllamaHistoryLength`
|
||||
classes and the `OLLAMA_HEADERS`/`OLLAMA_OPTIONS`/`OLLAMA_HISTORY` socket
|
||||
types are **out of scope** — confirmed zero test dependencies force a
|
||||
change, and ADR-007 never proposed touching them (only the
|
||||
model-management/chat surface generalizes). `_parse_output_schema`,
|
||||
`_comfy_types_for_schema`, `_build_structured_model`,
|
||||
`_coerce_structured_value` stay in `ollama.py` unchanged — pure/local
|
||||
schema logic with no network dependency, still needed by the live-preview
|
||||
route.
|
||||
@@ -28,10 +28,12 @@ class LLMProvider(Protocol):
|
||||
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
|
||||
self, model: str, messages: list[Message], options: dict | None = None,
|
||||
timeout_secs: float = 300.0,
|
||||
) -> str: ...
|
||||
async def chat_structured(
|
||||
self, model: str, messages: list[Message], schema: type[BaseModel], options: dict
|
||||
self, model: str, messages: list[Message], schema: type[BaseModel],
|
||||
options: dict | None = None, timeout_secs: float = 300.0, max_retries: int = 2,
|
||||
) -> BaseModel: ...
|
||||
```
|
||||
|
||||
|
||||
@@ -46,16 +46,25 @@ Single project: `src/comfydv/`, `tests/` at repository root (per plan.md's Proje
|
||||
|
||||
**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
|
||||
**Superseded 2026-07-11 — see `atomic-cutover-plan.md`.** T007–T010 as
|
||||
written below assumed US1 was independently deliverable; it isn't (see the
|
||||
correction at the bottom of this file). The actual work is now
|
||||
`atomic-cutover-plan.md`'s **T-CUT-04, T-CUT-05, T-CUT-06, T-CUT-08**
|
||||
(`OllamaClient` output-type change, `ChatCompletion` rename+delegation,
|
||||
`__init__.py` registration, and the corresponding `test_ollama.py`
|
||||
rewrite using a `_FakeProvider` double). Original text kept below for
|
||||
history, not as the active task list:
|
||||
|
||||
**Checkpoint**: US1 fully functional and independently testable (MVP).
|
||||
- [ ] ~~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**: superseded — see `atomic-cutover-plan.md`'s checkpoint (T-CUT-10, full suite green).
|
||||
|
||||
---
|
||||
|
||||
@@ -71,8 +80,8 @@ Single project: `src/comfydv/`, `tests/` at repository root (per plan.md's Proje
|
||||
- [x] 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`, with the Agent's own internal retries disabled (`retries=0`) so the error contract is comfydv's — makes T012-T pass (depends on T011-I)
|
||||
- [x] T013-T [US2] Write test: exhausted retries raise `RuntimeError` naming the model, attempt count, and a truncated last-response snippet, in `tests/test_llm_chat_structured.py` (witnesses `features/us2_structured_output.feature` scenario "Exhausted retries fail clearly instead of passing through bad data")
|
||||
- [x] T013-I [US2] Implement the exhausted-retry error path in `src/comfydv/_llm/chat.py`, matching ADR-006's existing error message contract (model, attempt count, truncated last response) — 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` _Deferred — folded into the atomic node cutover (issue #16); this touches `ollama.py`, which per the 2026-07-11 correction above cannot land independently of the US1/US3 rename._
|
||||
- [-] 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) _Deferred — same reason as T014-T._
|
||||
- [-] T014-T [US2] _Superseded — see `atomic-cutover-plan.md` D5 and T-CUT-08. Original: write FAILING test that `ChatCompletion`'s `structured_output=True` path wires a schema through `chat_structured()` to per-field dynamic ComfyUI output sockets. D5 replaces the originally-planned retry-count-style test with a delegation test against a `_FakeProvider`, since retry behavior is already covered by `tests/test_llm_chat_structured.py`._
|
||||
- [-] T014-I [US2] _Superseded — see `atomic-cutover-plan.md` T-CUT-05. Original: wire `ChatCompletion`'s `structured_output`/`output_schema` inputs to `LLMProvider.chat_structured()`, preserving the dynamic-socket UX from ADR-006 — still the right implementation shape, just executed as part of the coordinated T-CUT-05 rename, not standalone._
|
||||
|
||||
**Checkpoint**: US1 + US2 both independently functional — matches today's Ollama capability, now on the shared mechanism.
|
||||
|
||||
@@ -84,16 +93,22 @@ Single project: `src/comfydv/`, `tests/` at repository root (per plan.md's Proje
|
||||
|
||||
**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)
|
||||
**Superseded 2026-07-11 — see `atomic-cutover-plan.md`.** Maps to
|
||||
**T-CUT-02** (`OllamaProvider.list_models`/`load_model`/`unload_model`
|
||||
method bodies), **T-CUT-03** (`tests/test_ollama_provider.py`, new file),
|
||||
and **T-CUT-05/T-CUT-06/T-CUT-08** (the node renames + delegation +
|
||||
registration + test rewrite). Original text kept for history:
|
||||
|
||||
**Checkpoint**: US1 + US2 + US3 independently functional.
|
||||
- [ ] ~~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**: superseded — see `atomic-cutover-plan.md`.
|
||||
|
||||
---
|
||||
|
||||
@@ -103,10 +118,9 @@ Single project: `src/comfydv/`, `tests/` at repository root (per plan.md's Proje
|
||||
|
||||
**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
|
||||
- [ ] T019 [US4] _Maps to `atomic-cutover-plan.md` T-CUT-11_ — add a migration mapping (old node/socket name → new node/socket name) as a module-level constant in `src/comfydv/ollama.py`, surfaced in the replacement nodes' docstrings per FR-009
|
||||
- [ ] T020 [US4] _Maps to T-CUT-10_ — full `tests/test_ollama.py` + `tests/test_ollama_provider.py` suite green, confirming SC-003 equivalence (witnesses `features/us4_migration.feature` scenario "Reconnected workflow produces equivalent output")
|
||||
- [ ] T021 [P] [US4] _Maps to T-CUT-12_ — update `quickstart.md`'s "Migrating an existing pre-upgrade workflow" section to match the actual replacement mapping, then walk it manually against a live local server
|
||||
|
||||
**Checkpoint**: all four user stories independently functional; migration path documented.
|
||||
|
||||
@@ -114,7 +128,7 @@ Single project: `src/comfydv/`, `tests/` at repository root (per plan.md's Proje
|
||||
|
||||
## 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`
|
||||
- [ ] T022 [P] Run `uv run ruff check --fix && uv run ruff format` across `src/comfydv/_llm/`, `tests/test_ollama_provider.py`, 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
|
||||
@@ -203,3 +217,18 @@ ADR-007's own decision (breaking rename, no deprecated aliases) is
|
||||
**unaffected** — that call was about user-facing blast radius (small,
|
||||
Ollama integration shipped 2026-07-04), which this finding doesn't change.
|
||||
What's re-scoped is delivery sequencing, not the design decision.
|
||||
|
||||
---
|
||||
|
||||
## ✅ Properly specced (2026-07-11) — see `atomic-cutover-plan.md`
|
||||
|
||||
Full line-by-line inventory of every affected reference in `ollama.py` and
|
||||
`tests/test_ollama.py` (1820 lines, read in full), the design decisions it
|
||||
surfaced (cache-singleton duplication, `client == "<string>"` equality
|
||||
breaking, bare-string-client backward compat removal, and — the big one —
|
||||
a test-layer split so the 35 relocated `_post_json` monkeypatches land at
|
||||
the right architectural seam instead of being patched 1:1), and a
|
||||
12-step sequenced task list (T-CUT-01 … T-CUT-12) that supersedes the
|
||||
struck-through tasks above. That file is now the authoritative task list
|
||||
for this remaining work; this file's Phase 3/5/6 entries are kept only for
|
||||
history.
|
||||
|
||||
Reference in New Issue
Block a user