diff --git a/.specify/feature.json b/.specify/feature.json index 4df0a8b..28d7a7e 100644 --- a/.specify/feature.json +++ b/.specify/feature.json @@ -1,3 +1,3 @@ { - "feature_directory": "specs/007-llm-provider-abstraction" + "feature_directory": "specs/008-llamacpp-integration" } diff --git a/CLAUDE.md b/CLAUDE.md index 59aa5fd..ef142f2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,5 +1,5 @@ For additional context about technologies to be used, project structure, shell commands, and other important information, read the current plan -at specs/007-llm-provider-abstraction/plan.md +at specs/008-llamacpp-integration/plan.md diff --git a/README.md b/README.md index da6072a..ca4a1f8 100644 --- a/README.md +++ b/README.md @@ -1,28 +1,37 @@ # comfydv -A collection of workflow efficiency and quality-of-life nodes built out of necessity for personal ComfyUI use. +**Quality-of-life nodes for ComfyUI, built to disappear into your workflow.** -## What is this? +`comfydv` fills the gaps ComfyUI's built-in library leaves on the table: string templates that build their own sockets as you type, seed-controlled randomisation, graceful mid-queue interruption, and a local-LLM integration that doesn't care whether you're running Ollama or llama.cpp. No Python required — install it, drop the nodes on your canvas, wire them up. -`comfydv` fills gaps in ComfyUI's built-in node library: dynamic string formatting, seed-controlled random selection, graceful workflow interruption, and Ollama LLM integration. Install it once and connect the nodes like any other — no Python knowledge required. +![Chat Completion in action](docs/assets/ollama_chat.png) + +## What is comfydv? + +A small, focused ComfyUI utility pack. It exists because: + +- **It reads your intent, not just your syntax.** Format String detects `{variables}` in a template and adds/removes input sockets live, as you type — no manual socket wrangling. +- **One LLM integration, any local backend.** Wire a Chat Completion node once; swap between Ollama and llama.cpp by changing a single upstream client node. Structured output, multi-turn history, and model load/unload work identically on both. +- **It fails politely.** Circuit Breaker halts a queue run cleanly instead of throwing a stack trace at you; a disconnected LLM server gets a specific, actionable error instead of a silent empty dropdown. +- **Small, tested, boring in the best way.** Every node is unit-tested and the local-LLM nodes are verified against real running servers, not just mocks. + +## What's inside | Node | What it does | -|------|-------------| -| **Format String** | Formats a string from a Python f-string or Jinja2 template. Detects variables in the template and automatically adds/removes input sockets. | -| **Random Choice** | Accepts any number of typed inputs and outputs one at random, with a configurable seed for reproducibility. | -| **Circuit Breaker** | Halts the current ComfyUI queue run gracefully without crashing the server. Wire the `status` toggle to a boolean condition to skip the rest of the queue when a condition isn't met. | -| **Ollama Client** | Configures a connection to an Ollama server (default: `http://localhost:11434`). Threads the connection through the graph as an `LLM_CLIENT` socket — the same generic socket any future backend's client node will emit. | -| **LLM Model Selector** | Fetches the live model list from the connected server and presents it as a dropdown. Outputs the selected model name. | -| **LLM Load Model** | Loads a model into memory using `/api/generate` with `keep_alive=-1`. | -| **LLM Unload Model** | Evicts a model from memory using `/api/generate` with `keep_alive=0`. | -| **Chat Completion** | Sends a prompt (and optional conversation history) to the connected server. Response and history are shown inline in the node body and available as output sockets. | -| **Ollama Option — \*** | Seven composable option nodes (Temperature, Seed, Max Tokens, Top P, Top K, Repeat Penalty, Extra Body) that merge into an `OLLAMA_OPTIONS` dict wired into Chat Completion. | -| **Ollama Debug History** | Serialises an `OLLAMA_HISTORY` list to a pretty-printed JSON string for inspection. | -| **Ollama History Length** | Returns the number of messages in an `OLLAMA_HISTORY` list as an integer. | +|------|---------------| +| **Format String** | Renders a Python f-string or Jinja2 template. Sockets appear and disappear automatically as you type variables. | +| **Random Choice** | Accepts any number of typed inputs and returns one at random, with a seed for reproducibility. | +| **Circuit Breaker** | Halts the current queue run gracefully — no crash, just a clean stop — when a condition isn't met. | +| **Ollama Client** / **LlamaCpp Client** | Configure a connection to a local Ollama or llama.cpp server. Both emit the same `LLM_CLIENT` socket — every node below works with either. | +| **LLM Model Selector** | Live dropdown of models available on the connected server. | +| **LLM Load Model** / **LLM Unload Model** | Explicit VRAM management — pin a model in memory before inference, evict it after. | +| **Chat Completion** | Send a prompt (optionally with history) to the connected server; response shown inline and as an output socket. | +| **Ollama Option — \*** | Seven composable parameter nodes (Temperature, Seed, Max Tokens, Top P, Top K, Repeat Penalty, Extra Body) that merge into Chat Completion's `options` input. | +| **Ollama Debug History** / **Ollama History Length** | Inspect an `OLLAMA_HISTORY` conversation list — pretty-print it or count its messages. | ## Install -**Via ComfyUI Manager** (recommended): search for `comfydv` and click Install. +**Via ComfyUI Manager** (recommended): search for `comfydv`, click Install. **Manual:** @@ -31,131 +40,125 @@ cd /path/to/ComfyUI/custom_nodes git clone https://github.com/darth-veitcher/comfydv.git ``` -Restart ComfyUI. The nodes appear under the **dv/** and **dv/ollama** categories in the node menu. Runtime dependencies (`jinja2`, `aiohttp`) are installed automatically via `requirements.txt`. +Restart ComfyUI. Nodes appear under **dv/**, **dv/ollama**, and **dv/llamacpp** in the node menu. Runtime dependencies (`jinja2`, `aiohttp`, `pydantic-ai`) install automatically via `requirements.txt`. -For Ollama nodes: [install Ollama](https://ollama.com/download) and pull at least one model (`ollama pull qwen2.5:latest`) before using the Ollama nodes. +For local-LLM nodes, bring your own backend: + +- **Ollama** — [install Ollama](https://ollama.com/download), pull a model (`ollama pull qwen2.5:latest`). +- **llama.cpp** — [build/install `llama-server`](https://github.com/ggml-org/llama.cpp), launch it in [router mode](#llamacpp). ## Quickstart -1. Install via ComfyUI Manager (search `comfydv`) or clone manually into `custom_nodes/`. -2. Right-click the canvas → Add Node → **dv/** to find Format String, Random Choice, and Circuit Breaker. -3. For Ollama nodes: start Ollama (`ollama serve`), pull a model (`ollama pull qwen2.5:latest`), then add nodes from **dv/ollama/**. +1. Right-click the canvas → Add Node → **dv/** for Format String, Random Choice, and Circuit Breaker. +2. For local LLM nodes: start Ollama (`ollama serve`) or `llama-server` (router mode), then add nodes from **dv/ollama/** or **dv/llamacpp/** — the chat/model-management nodes are shared between both backends. -## Documentation - -Full documentation: [darth-veitcher.github.io/comfydv](https://darth-veitcher.github.io/comfydv/stable/) +Full documentation, including every node's inputs/outputs: **[darth-veitcher.github.io/comfydv](https://darth-veitcher.github.io/comfydv/stable/)** --- ## Format String -Formats text from a Python f-string or Jinja2 template. As you type the template, input sockets appear and disappear automatically — one per variable detected. - -### Python f-strings - -Type `{variable_name}` and a socket appears. Wire it to any string output in your workflow. +Type a template, get sockets. `{variable_name}` in f-string mode, `{{ variable_name }}` in Jinja2 mode — either way, comfydv watches what you type and keeps the node's inputs in sync automatically. ![Format String — f-string mode](docs/assets/fstring.png) -Outputs are always in a stable order: - | Output | Content | |--------|---------| | `formatted_string` | The rendered result | -| `saved_file_path` | Path written to disk (if `save_path` is set) | -| `` … | Pass-through of each input value, for easy chaining | +| `saved_file_path` | Where it was written, if `save_path` is set | +| `` … | Each input passed through unchanged, for easy chaining | -### Jinja2 templates - -Switch `template_type` to **Jinja2** to unlock filters (`| upper`, `| int`, …), conditionals (`{% if %}…{% endif %}`), and loops. +Switch `template_type` to **Jinja2** to unlock filters (`| upper`, `| int`), conditionals, and loops: ![Format String — Jinja2 mode](docs/assets/jinja2.png) -Variables detected in `{{ }}` expressions become input sockets exactly as in Simple mode. See the [Jinja2 documentation](https://jinja.palletsprojects.com/en/latest/) for the full filter/test reference. - --- ## Random Choice -Connect any number of inputs of the same type. Each run picks one at random. Set `seed` for reproducibility. +Wire in any number of same-typed inputs — images, strings, conditioning, anything ComfyUI can carry over a socket — and get one back at random. ![Random Choice](docs/assets/random.png) -- Accepts any ComfyUI type (STRING, IMAGE, CONDITIONING, …) -- Add as many inputs as you like; unused slots are removed automatically when disconnected -- `seed = 0` randomises on every run; any other value locks the selection +`seed = 0` randomises every run; any other value locks the selection. Unused input slots vanish automatically when you disconnect them. --- ## Circuit Breaker -Stops the queue gracefully when a condition isn't met — no crash, no error, just a clean halt. +Stop a queue run cleanly when a condition isn't met, instead of letting a downstream node crash on bad input. ![Circuit Breaker](docs/assets/circuit_breaker.png) -Wire an image (or any trigger) into `trigger` and a boolean into `status`. When `status` is **false** the node raises `InterruptProcessingException`, which tells ComfyUI to stop the current run cleanly. When `status` is **true** the image passes through unchanged. +Wire a trigger (an image, or anything) into `trigger` and a boolean into `status`. `status = false` raises `InterruptProcessingException` — ComfyUI stops the run without an error dialog. `status = true` passes the trigger straight through. -Typical use: skip an expensive upscale step when a quality-check node says the draft is already good enough. +Typical use: skip an expensive upscale pass when an upstream quality-check node says the draft's already good enough. --- -## Ollama +## Local LLMs -Nodes for integrating a local Ollama LLM into your ComfyUI workflow. The host is configured once in **Ollama Client** and threaded through the graph as an `LLM_CLIENT` socket — a generic connection type any future backend's client node can also emit, so the chat/model-management nodes below aren't Ollama-specific. +One set of nodes, two interchangeable backends. Configure a connection once with **Ollama Client** or **LlamaCpp Client** — both output the same `LLM_CLIENT` socket — and every downstream node (model selection, load/unload, chat, structured output, multi-turn history) works exactly the same way regardless of which one you picked. Swapping backends means rewiring one node, not rebuilding your graph. -### Ollama Client node - -Configure the server address once; all downstream nodes inherit it automatically. +### Connect and chat ![Ollama Client](docs/assets/ollama_client.png) -### Model lifecycle (load and unload) - -On memory-constrained machines and single-GPU setups, explicitly loading and unloading the model before and after inference is critical. **LLM Load Model** pins the model into VRAM (`keep_alive=-1`); **LLM Unload Model** evicts it immediately (`keep_alive=0`), freeing memory for image generation or other models. - -![Ollama Load / Unload](docs/assets/ollama_lifecycle.png) - -The correct chain is **Load → Chat → Unload**, enforced through data dependencies: - -1. Wire `LLMLoadModel.model_name` → `ChatCompletion.model`. This creates the data dependency that guarantees Load runs before Chat and passes the model name into the Chat node's plain-string `model` input. -2. Wire `ChatCompletion.model_name` → `LLMUnloadModel.model`. This guarantees Unload runs after Chat completes. -3. Optionally wire `ChatCompletion.response` → `LLMUnloadModel.passthrough` — Unload returns the response unchanged so the rest of your workflow can still consume it. - -### Minimal chat workflow - -1. **Ollama Client** → set host (default `http://localhost:11434`) -2. **LLM Model Selector** → pick a model from the live dropdown (or type/wire a model name directly into Chat Completion's `model` input) -3. **Chat Completion** → wire client + model + prompt; the response appears inline in the node body and is also available as an output socket +1. **Ollama Client** (default `http://localhost:11434`) or **LlamaCpp Client** (default `http://localhost:8080`) — set the host. +2. **LLM Model Selector** — pick a model from the live dropdown, or wire a model name straight into Chat Completion. +3. **Chat Completion** — wire in client, model, and prompt. The response renders inline in the node and is also available as an output socket. ![Chat Completion](docs/assets/ollama_chat.png) -Wire multiple nodes together for a complete end-to-end workflow: +A complete graph looks like this: -![Ollama Full Workflow](docs/assets/ollama_workflow.png) +![Full LLM workflow](docs/assets/ollama_workflow.png) -### Option nodes +### Manual memory management -Chain any combination of **Ollama Option —** nodes before Chat Completion to override inference parameters: +Single-GPU and memory-constrained setups need explicit control over what's resident in VRAM. **LLM Load Model** pins a model into memory; **LLM Unload Model** evicts it immediately, freeing room for the next model or the rest of your image pipeline. -| Option node | Ollama param | -|-------------|-------------| +![Load / Unload lifecycle](docs/assets/ollama_lifecycle.png) + +The **Load → Chat → Unload** chain is enforced by data dependencies, not by convention: + +1. `LLMLoadModel.model_name` → `ChatCompletion.model` — guarantees Load runs before Chat, and feeds the model name straight in. +2. `ChatCompletion.model_name` → `LLMUnloadModel.model` — guarantees Unload runs after Chat completes. +3. *(Optional)* `ChatCompletion.response` → `LLMUnloadModel.passthrough` — Unload returns the response unchanged, so the rest of your workflow can still consume it. + +### Tuning generation + +Chain any combination of **Ollama Option —** nodes ahead of Chat Completion to override inference parameters: + +![Ollama Option nodes](docs/assets/ollama_options.png) + +| Option node | Parameter | +|-------------|-----------| | Temperature | `temperature` | | Seed | `seed` | | Max Tokens | `num_predict` | | Top P | `top_p` | | Top K | `top_k` | | Repeat Penalty | `repeat_penalty` | -| Extra Body | arbitrary JSON merged into options | - -![Ollama Option Nodes](docs/assets/ollama_options.png) +| Extra Body | arbitrary JSON, merged into `options` | ### Multi-turn conversations -`OLLAMA_HISTORY` flows out of Chat Completion as a list of `{"role", "content"}` dicts. Wire it back into the next Chat Completion for multi-turn conversations, or inspect it with **Ollama Debug History** / **Ollama History Length**. +`OLLAMA_HISTORY` flows out of Chat Completion as a `{"role", "content"}` list. Feed it back into the next Chat Completion call for multi-turn context, or inspect it with **Ollama Debug History** / **Ollama History Length**. -### Upgrading an older workflow +### llama.cpp -If you saved a workflow before this rename, ComfyUI will report the old node types as missing when you reopen it. Reconnect using this mapping, then re-run — behavior is unchanged, only the names and the client socket type are different: +Everything above works unchanged against llama.cpp — swap in a **LlamaCpp Client** and the rest of the graph doesn't know the difference. The one thing llama.cpp needs that Ollama doesn't: **router mode**, a directory of models rather than a single `-m model.gguf`: + +```bash +llama-server --models-dir ./models -c 8192 +``` + +In exchange, router mode gives comfydv a richer live status than Ollama can report — `loading` and `downloading`, not just loaded/unloaded — plus the same explicit load/unload primitives Ollama's nodes already use. + +### Upgrading a workflow saved before this rename + +Nodes were renamed once, to make them backend-generic (`OllamaChatCompletion` → `ChatCompletion`, etc.). If ComfyUI reports old node types as missing when you reopen a saved workflow, reconnect using this table — behavior is unchanged, only the names are: | Old | New | |-----|-----| @@ -165,4 +168,10 @@ If you saved a workflow before this rename, ComfyUI will report the old node typ | `OllamaUnloadModel` | `LLMUnloadModel` | | `OLLAMA_CLIENT` socket | `LLM_CLIENT` socket | -`OllamaClient` keeps its name — just delete and re-add any downstream node showing as missing, then rewire it to the same `OllamaClient` node. +`OllamaClient` kept its name — delete and re-add any node showing as missing, then rewire it to the same `OllamaClient` node. + +--- + +## License + +[AGPL-3.0](LICENSE) diff --git a/ROADMAP.md b/ROADMAP.md index c0ced46..e904e34 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -20,6 +20,7 @@ gantt BEACON Bootstrap :done, beacon-bootstrap, 2026-07-11, 7d LLM Provider Abstraction :done, llm-provider-abstraction, 2026-07-11, 7d Logging Modernisation :done, logging-modernisation, 2026-07-11, 7d + Ollama Model Integration :done, ollama-integration, 2026-07-11, 7d ``` @@ -27,11 +28,12 @@ gantt | Epic | Title | Status | Specs | Fidelity | |---|---|---|---|---| -| [llamacpp-integration](project-management/Roadmap/epics/llamacpp-integration.md) | llama.cpp Model Integration | Active | — | S? A+ T:- | +| [llamacpp-integration](project-management/Roadmap/epics/llamacpp-integration.md) | llama.cpp Model Integration | Active | 1/1 shipped | S+ A+ T:96% | | [ux-and-install](project-management/Roadmap/epics/ux-and-install.md) | ComfyUI UX Polish & Manager Compatibility | Active | 1/4 shipped | S+ A+ T:100% | | [beacon-bootstrap](project-management/Roadmap/epics/archive/beacon-bootstrap.md) | BEACON Bootstrap | Done | — | S? A? T:- | | [llm-provider-abstraction](project-management/Roadmap/epics/archive/llm-provider-abstraction.md) | LLM Provider Abstraction | Done | 1/1 shipped | S+ A+ T:58% | | [logging-modernisation](project-management/Roadmap/epics/archive/logging-modernisation.md) | Logging Modernisation | Done | 1/1 shipped | S+ A+ T:100% | +| [ollama-integration](project-management/Roadmap/epics/archive/ollama-integration.md) | Ollama Model Integration | Done | 1/1 shipped | S+ A+ T:100% | _Fidelity: `S+/S?` = has specs / none · `A+/A?` = has ADRs / none · `T:N%` = task completion_ diff --git a/docs/assets/circuit_breaker.png b/docs/assets/circuit_breaker.png index d8a1ff3..2c93e55 100644 Binary files a/docs/assets/circuit_breaker.png and b/docs/assets/circuit_breaker.png differ diff --git a/docs/assets/fstring.png b/docs/assets/fstring.png index 0dd02bd..2a7e50e 100644 Binary files a/docs/assets/fstring.png and b/docs/assets/fstring.png differ diff --git a/docs/assets/jinja2.png b/docs/assets/jinja2.png index 784fe4b..d3d9adf 100644 Binary files a/docs/assets/jinja2.png and b/docs/assets/jinja2.png differ diff --git a/docs/assets/llamacpp_client.png b/docs/assets/llamacpp_client.png new file mode 100644 index 0000000..4eb3323 Binary files /dev/null and b/docs/assets/llamacpp_client.png differ diff --git a/docs/assets/llamacpp_workflow.png b/docs/assets/llamacpp_workflow.png new file mode 100644 index 0000000..5700692 Binary files /dev/null and b/docs/assets/llamacpp_workflow.png differ diff --git a/docs/assets/ollama_chat.png b/docs/assets/ollama_chat.png index de55783..03693c9 100644 Binary files a/docs/assets/ollama_chat.png and b/docs/assets/ollama_chat.png differ diff --git a/docs/assets/ollama_client.png b/docs/assets/ollama_client.png index 97fd652..7c1da79 100644 Binary files a/docs/assets/ollama_client.png and b/docs/assets/ollama_client.png differ diff --git a/docs/assets/ollama_lifecycle.png b/docs/assets/ollama_lifecycle.png index a8d8c97..216c8da 100644 Binary files a/docs/assets/ollama_lifecycle.png and b/docs/assets/ollama_lifecycle.png differ diff --git a/docs/assets/ollama_options.png b/docs/assets/ollama_options.png index 34283e8..0d2d53a 100644 Binary files a/docs/assets/ollama_options.png and b/docs/assets/ollama_options.png differ diff --git a/docs/assets/ollama_workflow.png b/docs/assets/ollama_workflow.png index 298f828..7bce601 100644 Binary files a/docs/assets/ollama_workflow.png and b/docs/assets/ollama_workflow.png differ diff --git a/docs/assets/random.png b/docs/assets/random.png index 9b8960b..fe721a3 100644 Binary files a/docs/assets/random.png and b/docs/assets/random.png differ diff --git a/docs/assets/structured_output.png b/docs/assets/structured_output.png new file mode 100644 index 0000000..db78a15 Binary files /dev/null and b/docs/assets/structured_output.png differ diff --git a/docs/index.md b/docs/index.md index 80f81dc..0c90754 100644 --- a/docs/index.md +++ b/docs/index.md @@ -1,24 +1,37 @@ # comfydv -A collection of workflow efficiency and quality-of-life nodes built out of necessity for personal ComfyUI use. +**Quality-of-life nodes for ComfyUI, built to disappear into your workflow.** + +`comfydv` fills the gaps ComfyUI's built-in library leaves on the table: string templates that build their own sockets as you type, seed-controlled randomisation, graceful mid-queue interruption, and a local-LLM integration that doesn't care whether you're running Ollama or llama.cpp. No Python required — install it, drop the nodes on your canvas, wire them up. + +![Chat Completion in action](assets/ollama_chat.png) + +## What is comfydv? + +A small, focused ComfyUI utility pack. It exists because: + +- **It reads your intent, not just your syntax.** Format String detects `{variables}` in a template and adds/removes input sockets live, as you type — no manual socket wrangling. +- **One LLM integration, any local backend.** Wire a Chat Completion node once; swap between Ollama and llama.cpp by changing a single upstream client node. Structured output, multi-turn history, and model load/unload work identically on both. +- **It fails politely.** Circuit Breaker halts a queue run cleanly instead of throwing a stack trace at you; a disconnected LLM server gets a specific, actionable error instead of a silent empty dropdown. +- **Small, tested, boring in the best way.** Every node is unit-tested and the local-LLM nodes are verified against real running servers, not just mocks. + +## What's inside | Node | What it does | -|------|-------------| -| **Format String** | Formats a string from a Python f-string or Jinja2 template. Detects variables in the template and automatically adds/removes input sockets. | -| **Random Choice** | Accepts any number of typed inputs and outputs one at random, with a configurable seed for reproducibility. | -| **Circuit Breaker** | Halts the current ComfyUI queue run gracefully without crashing the server. Wire the `status` toggle to a boolean condition to skip the rest of the queue when a condition isn't met. | -| **Ollama Client** | Configures a connection to an Ollama server (default: `http://localhost:11434`). Threads the connection through the graph as an `LLM_CLIENT` socket — the same generic socket any future backend's client node will emit. | -| **LLM Model Selector** | Fetches the live model list from the connected server and presents it as a dropdown. Outputs the selected model name. | -| **LLM Load Model** | Loads a model into memory using `/api/generate` with `keep_alive=-1`. | -| **LLM Unload Model** | Evicts a model from memory using `/api/generate` with `keep_alive=0`. | -| **Chat Completion** | Sends a prompt (and optional conversation history) to the connected server. Response and history are shown inline in the node body and available as output sockets. | -| **Ollama Option — \*** | Seven composable option nodes (Temperature, Seed, Max Tokens, Top P, Top K, Repeat Penalty, Extra Body) that merge into an `OLLAMA_OPTIONS` dict wired into Chat Completion. | -| **Ollama Debug History** | Serialises an `OLLAMA_HISTORY` list to a pretty-printed JSON string for inspection. | -| **Ollama History Length** | Returns the number of messages in an `OLLAMA_HISTORY` list as an integer. | +|------|---------------| +| **Format String** | Renders a Python f-string or Jinja2 template. Sockets appear and disappear automatically as you type variables. | +| **Random Choice** | Accepts any number of typed inputs and returns one at random, with a seed for reproducibility. | +| **Circuit Breaker** | Halts the current queue run gracefully — no crash, just a clean stop — when a condition isn't met. | +| **Ollama Client** / **LlamaCpp Client** | Configure a connection to a local Ollama or llama.cpp server. Both emit the same `LLM_CLIENT` socket — every node below works with either. | +| **LLM Model Selector** | Live dropdown of models available on the connected server. | +| **LLM Load Model** / **LLM Unload Model** | Explicit VRAM management — pin a model in memory before inference, evict it after. | +| **Chat Completion** | Send a prompt (optionally with history) to the connected server; response shown inline and as an output socket. | +| **Ollama Option — \*** | Seven composable parameter nodes (Temperature, Seed, Max Tokens, Top P, Top K, Repeat Penalty, Extra Body) that merge into Chat Completion's `options` input. | +| **Ollama Debug History** / **Ollama History Length** | Inspect an `OLLAMA_HISTORY` conversation list — pretty-print it or count its messages. | ## Install -**Via ComfyUI Manager** (recommended): search for `comfydv` and click Install. +**Via ComfyUI Manager** (recommended): search for `comfydv`, click Install. **Manual:** @@ -27,119 +40,123 @@ cd /path/to/ComfyUI/custom_nodes git clone https://github.com/darth-veitcher/comfydv.git ``` -Restart ComfyUI. The nodes appear under the **dv/** and **dv/ollama** categories in the node menu. Runtime dependencies (`jinja2`, `aiohttp`) are installed automatically via `requirements.txt`. +Restart ComfyUI. Nodes appear under **dv/**, **dv/ollama**, and **dv/llamacpp** in the node menu. Runtime dependencies (`jinja2`, `aiohttp`, `pydantic-ai`) install automatically via `requirements.txt`. -For Ollama nodes: [install Ollama](https://ollama.com/download) and pull at least one model (`ollama pull qwen2.5:latest`) before using the Ollama nodes. +For local-LLM nodes, bring your own backend: + +- **Ollama** — [install Ollama](https://ollama.com/download), pull a model (`ollama pull qwen2.5:latest`). +- **llama.cpp** — [build/install `llama-server`](https://github.com/ggml-org/llama.cpp), launch it in [router mode](#llamacpp). + +## Quickstart + +1. Right-click the canvas → Add Node → **dv/** for Format String, Random Choice, and Circuit Breaker. +2. For local LLM nodes: start Ollama (`ollama serve`) or `llama-server` (router mode), then add nodes from **dv/ollama/** or **dv/llamacpp/** — the chat/model-management nodes are shared between both backends. --- ## Format String -Formats text from a Python f-string or Jinja2 template. As you type the template, input sockets appear and disappear automatically — one per variable detected. - -### Python f-strings - -Type `{variable_name}` and a socket appears. Wire it to any string output in your workflow. +Type a template, get sockets. `{variable_name}` in f-string mode, `{{ variable_name }}` in Jinja2 mode — either way, comfydv watches what you type and keeps the node's inputs in sync automatically. ![Format String — f-string mode](assets/fstring.png) | Output | Content | |--------|---------| | `formatted_string` | The rendered result | -| `saved_file_path` | Path written to disk (if `save_path` is set) | -| `` … | Pass-through of each input value, for easy chaining | +| `saved_file_path` | Where it was written, if `save_path` is set | +| `` … | Each input passed through unchanged, for easy chaining | -### Jinja2 templates - -Switch `template_type` to **Jinja2** to unlock filters (`| upper`, `| int`, …), conditionals (`{% if %}…{% endif %}`), and loops. +Switch `template_type` to **Jinja2** to unlock filters (`| upper`, `| int`), conditionals, and loops: ![Format String — Jinja2 mode](assets/jinja2.png) -Variables detected in `{{ }}` expressions become input sockets exactly as in Simple mode. See the [Jinja2 documentation](https://jinja.palletsprojects.com/en/latest/) for the full filter/test reference. - --- ## Random Choice -Connect any number of inputs of the same type. Each run picks one at random. Set `seed` for reproducibility. +Wire in any number of same-typed inputs — images, strings, conditioning, anything ComfyUI can carry over a socket — and get one back at random. ![Random Choice](assets/random.png) -- Accepts any ComfyUI type (STRING, IMAGE, CONDITIONING, …) -- Add as many inputs as you like; unused slots are removed automatically when disconnected -- `seed = 0` randomises on every run; any other value locks the selection +`seed = 0` randomises every run; any other value locks the selection. Unused input slots vanish automatically when you disconnect them. --- ## Circuit Breaker -Stops the queue gracefully when a condition isn't met — no crash, no error, just a clean halt. +Stop a queue run cleanly when a condition isn't met, instead of letting a downstream node crash on bad input. ![Circuit Breaker](assets/circuit_breaker.png) -Wire an image (or any trigger) into `trigger` and a boolean into `status`. When `status` is **false** the node raises `InterruptProcessingException`, which tells ComfyUI to stop the current run cleanly. When `status` is **true** the image passes through unchanged. +Wire a trigger (an image, or anything) into `trigger` and a boolean into `status`. `status = false` raises `InterruptProcessingException` — ComfyUI stops the run without an error dialog. `status = true` passes the trigger straight through. -Typical use: skip an expensive upscale step when a quality-check node says the draft is already good enough. +Typical use: skip an expensive upscale pass when an upstream quality-check node says the draft's already good enough. --- -## Ollama +## Local LLMs -Nodes for integrating a local Ollama LLM into your ComfyUI workflow. The host is configured once in **Ollama Client** and threaded through the graph as an `LLM_CLIENT` socket — a generic connection type any future backend's client node can also emit, so the chat/model-management nodes below aren't Ollama-specific. +One set of nodes, two interchangeable backends. Configure a connection once with **Ollama Client** or **LlamaCpp Client** — both output the same `LLM_CLIENT` socket — and every downstream node (model selection, load/unload, chat, structured output, multi-turn history) works exactly the same way regardless of which one you picked. Swapping backends means rewiring one node, not rebuilding your graph. -### Ollama Client node - -Configure the server address once; all downstream nodes inherit it automatically. +### Connect and chat ![Ollama Client](assets/ollama_client.png) -### Model lifecycle (load and unload) - -On memory-constrained machines and single-GPU setups, explicitly loading and unloading the model before and after inference is critical. **LLM Load Model** pins the model into VRAM (`keep_alive=-1`); **LLM Unload Model** evicts it immediately (`keep_alive=0`), freeing memory for image generation or other models. - -![Ollama Load / Unload](assets/ollama_lifecycle.png) - -The correct chain is **Load → Chat → Unload**, enforced through data dependencies: - -1. Wire `LLMLoadModel.model_name` → `ChatCompletion.model`. This creates the data dependency that guarantees Load runs before Chat and passes the model name into the Chat node's plain-string `model` input. -2. Wire `ChatCompletion.model_name` → `LLMUnloadModel.model`. This guarantees Unload runs after Chat completes. -3. Optionally wire `ChatCompletion.response` → `LLMUnloadModel.passthrough` — Unload returns the response unchanged so the rest of your workflow can still consume it. - -### Minimal chat workflow - -1. **Ollama Client** → set host (default `http://localhost:11434`) -2. **LLM Model Selector** → pick a model from the live dropdown (or type/wire a model name directly into Chat Completion's `model` input) -3. **Chat Completion** → wire client + model + prompt; the response appears inline in the node body and is also available as an output socket +1. **Ollama Client** (default `http://localhost:11434`) or **LlamaCpp Client** (default `http://localhost:8080`) — set the host. +2. **LLM Model Selector** — pick a model from the live dropdown, or wire a model name straight into Chat Completion. +3. **Chat Completion** — wire in client, model, and prompt. The response renders inline in the node and is also available as an output socket. ![Chat Completion](assets/ollama_chat.png) -Wire multiple nodes together for a complete end-to-end workflow: +A complete graph looks like this: -![Ollama Full Workflow](assets/ollama_workflow.png) +![Full LLM workflow](assets/ollama_workflow.png) -### Option nodes +### Manual memory management -Chain any combination of **Ollama Option —** nodes before Chat Completion to override inference parameters: +Single-GPU and memory-constrained setups need explicit control over what's resident in VRAM. **LLM Load Model** pins a model into memory; **LLM Unload Model** evicts it immediately, freeing room for the next model or the rest of your image pipeline. -| Option node | Ollama param | -|-------------|-------------| +![Load / Unload lifecycle](assets/ollama_lifecycle.png) + +The **Load → Chat → Unload** chain is enforced by data dependencies, not by convention: + +1. `LLMLoadModel.model_name` → `ChatCompletion.model` — guarantees Load runs before Chat, and feeds the model name straight in. +2. `ChatCompletion.model_name` → `LLMUnloadModel.model` — guarantees Unload runs after Chat completes. +3. *(Optional)* `ChatCompletion.response` → `LLMUnloadModel.passthrough` — Unload returns the response unchanged, so the rest of your workflow can still consume it. + +### Tuning generation + +Chain any combination of **Ollama Option —** nodes ahead of Chat Completion to override inference parameters: + +![Ollama Option nodes](assets/ollama_options.png) + +| Option node | Parameter | +|-------------|-----------| | Temperature | `temperature` | | Seed | `seed` | | Max Tokens | `num_predict` | | Top P | `top_p` | | Top K | `top_k` | | Repeat Penalty | `repeat_penalty` | -| Extra Body | arbitrary JSON merged into options | - -![Ollama Option Nodes](assets/ollama_options.png) +| Extra Body | arbitrary JSON, merged into `options` | ### Multi-turn conversations -`OLLAMA_HISTORY` flows out of Chat Completion as a list of `{"role", "content"}` dicts. Wire it back into the next Chat Completion for multi-turn conversations, or inspect it with **Ollama Debug History** / **Ollama History Length**. +`OLLAMA_HISTORY` flows out of Chat Completion as a `{"role", "content"}` list. Feed it back into the next Chat Completion call for multi-turn context, or inspect it with **Ollama Debug History** / **Ollama History Length**. -### Upgrading an older workflow +### llama.cpp -If you saved a workflow before this rename, ComfyUI will report the old node types as missing when you reopen it. Reconnect using this mapping, then re-run — behavior is unchanged, only the names and the client socket type are different: +Everything above works unchanged against llama.cpp — swap in a **LlamaCpp Client** and the rest of the graph doesn't know the difference. The one thing llama.cpp needs that Ollama doesn't: **router mode**, a directory of models rather than a single `-m model.gguf`: + +```bash +llama-server --models-dir ./models -c 8192 +``` + +In exchange, router mode gives comfydv a richer live status than Ollama can report — `loading` and `downloading`, not just loaded/unloaded — plus the same explicit load/unload primitives Ollama's nodes already use. + +### Upgrading a workflow saved before this rename + +Nodes were renamed once, to make them backend-generic (`OllamaChatCompletion` → `ChatCompletion`, etc.). If ComfyUI reports old node types as missing when you reopen a saved workflow, reconnect using this table — behavior is unchanged, only the names are: | Old | New | |-----|-----| @@ -149,4 +166,4 @@ If you saved a workflow before this rename, ComfyUI will report the old node typ | `OllamaUnloadModel` | `LLMUnloadModel` | | `OLLAMA_CLIENT` socket | `LLM_CLIENT` socket | -`OllamaClient` keeps its name — just delete and re-add any downstream node showing as missing, then rewire it to the same `OllamaClient` node. +`OllamaClient` kept its name — delete and re-add any node showing as missing, then rewire it to the same `OllamaClient` node. diff --git a/project-management/Roadmap/epics/llamacpp-integration.md b/project-management/Roadmap/epics/llamacpp-integration.md index 8b8139e..1ac559e 100644 --- a/project-management/Roadmap/epics/llamacpp-integration.md +++ b/project-management/Roadmap/epics/llamacpp-integration.md @@ -1,7 +1,7 @@ # Epic: llama.cpp Model Integration ## Status -Active — started 2026-07-11 +Active — spec 008-llamacpp-integration complete, ready to finish once merged ## Why now @@ -34,6 +34,7 @@ touched by this epic. _Filled by `beacon specify --epic llamacpp-integration` / `/speckit-specify` once this epic is accepted._ +- specs/008-llamacpp-integration/ ## 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 diff --git a/scripts/take_screenshots.py b/scripts/take_screenshots.py index c1bf86c..21e182c 100644 --- a/scripts/take_screenshots.py +++ b/scripts/take_screenshots.py @@ -117,7 +117,11 @@ async def _capture( pad: int = 60, ) -> None: """Screenshot the canvas, optionally cropped tightly around the node.""" - canvas = page.locator("canvas#graph-canvas, canvas").first + # ComfyUI's frontend added a minimap canvas since this script was last + # verified — it appears before #graph-canvas in DOM order, so a bare + # union selector's `.first` silently grabbed the 250x200 minimap + # instead of the real graph. Target #graph-canvas explicitly. + canvas = page.locator("canvas#graph-canvas") box = await canvas.bounding_box() if box is None: await page.screenshot(path=str(out)) @@ -307,13 +311,13 @@ async def scene_ollama_client(page: Page, out: Path) -> None: async def scene_ollama_chat(page: Page, out: Path) -> None: - """OllamaChatCompletion — showing the live model dropdown and prompt widget.""" + """ChatCompletion — showing the live model dropdown and prompt widget.""" await _clear(page) info = await page.evaluate( """ async () => { - const node = LiteGraph.createNode("OllamaChatCompletion"); + const node = LiteGraph.createNode("ChatCompletion"); node.pos = [60, 60]; window.app.graph.add(node); @@ -327,7 +331,7 @@ async def scene_ollama_chat(page: Page, out: Path) -> None: } else { // Manually fetch and populate the COMBO try { - const resp = await fetch("/dv/ollama/models?host=http://host.docker.internal:11434"); + const resp = await fetch("/dv/ollama/models?host=http://host.docker.internal:11434&backend=ollama"); if (resp.ok) { const data = await resp.json(); const models = data.models || []; @@ -357,8 +361,142 @@ async def scene_ollama_chat(page: Page, out: Path) -> None: await _capture(page, out, info["pos"], info["size"]) +async def scene_structured_output(page: Page, out: Path) -> None: + """ChatCompletion with structured_output enabled — the live dynamic + output sockets (summary/sentiment/confidence) that appear as soon as + output_schema is edited, no graph run required. Exercises the real + js/ollama.js widget callbacks (structuredWidget.callback / + schemaWidget.callback), the same code path a live user's checkbox click + and schema edit trigger — not a hand-simulated approximation.""" + await _clear(page) + + schema = ( + '{"type": "object", "properties": ' + '{"summary": {"type": "string"}, ' + '"sentiment": {"type": "string"}, ' + '"confidence": {"type": "number"}}, ' + '"required": ["summary", "sentiment", "confidence"]}' + ) + + info = await page.evaluate( + f""" + async () => {{ + const node = LiteGraph.createNode("ChatCompletion"); + node.pos = [60, 60]; + window.app.graph.add(node); + + const promptWidget = node.widgets.find(w => w.name === "prompt"); + if (promptWidget) promptWidget.value = + "The new render pipeline cut our export time in half and the team is thrilled."; + + const structuredWidget = node.widgets.find(w => w.name === "structured_output"); + const schemaWidget = node.widgets.find(w => w.name === "output_schema"); + if (structuredWidget) structuredWidget.value = true; + if (schemaWidget) schemaWidget.value = {json.dumps(schema)}; + + // Fire the same callbacks js/ollama.js attaches on node creation — + // real widget-edit code path, not a re-implementation. + if (structuredWidget?.callback) await structuredWidget.callback(true); + if (schemaWidget?.callback) await schemaWidget.callback({json.dumps(schema)}); + + await new Promise(r => setTimeout(r, 600)); + window.app.canvas.setDirty(true, true); + window.app.canvas.draw(true, true); + return {{ pos: [node.pos[0], node.pos[1]], size: [node.size[0], node.size[1]] }}; + }} + """ + ) + + await asyncio.sleep(0.8) + await _redraw(page) + await _frame_node(page, info["pos"], info["size"]) + await _capture(page, out, info["pos"], info["size"]) + + +async def scene_llamacpp_client(page: Page, out: Path) -> None: + """LlamaCppClient — single node showing the router-mode host URL widget.""" + await _clear(page) + + info = await page.evaluate( + """ + () => { + const node = LiteGraph.createNode("LlamaCppClient"); + node.pos = [60, 60]; + window.app.graph.add(node); + const hostWidget = node.widgets && node.widgets.find(w => w.name === "host"); + if (hostWidget) hostWidget.value = "http://localhost:8080"; + window.app.canvas.setDirty(true, true); + window.app.canvas.draw(true, true); + return { pos: [node.pos[0], node.pos[1]], size: [node.size[0], node.size[1]] }; + } + """ + ) + + await _frame_node(page, info["pos"], info["size"]) + await _capture(page, out, info["pos"], info["size"]) + + +async def scene_llamacpp_workflow(page: Page, out: Path) -> None: + """LlamaCppClient → the same ChatCompletion node the Ollama workflow + uses, unmodified — the actual point of the adapter pattern. Attempts a + live model-list refresh via backend=llamacpp against + host.docker.internal:8080; degrades gracefully (same as a real user's + "no server running yet" state) if nothing is listening there.""" + await _clear(page) + + info = await page.evaluate( + """ + async () => { + const graph = window.app.graph; + + const client = LiteGraph.createNode("LlamaCppClient"); + client.pos = [40, 60]; + graph.add(client); + const hostWidget = client.widgets.find(w => w.name === "host"); + if (hostWidget) hostWidget.value = "http://host.docker.internal:8080"; + + const chat = LiteGraph.createNode("ChatCompletion"); + chat.pos = [380, 40]; + graph.add(chat); + const promptWidget = chat.widgets.find(w => w.name === "prompt"); + if (promptWidget) promptWidget.value = "Write a haiku about ComfyUI."; + + client.connect(0, chat, 0); + + try { + const resp = await fetch("/dv/ollama/models?host=http://host.docker.internal:8080&backend=llamacpp"); + if (resp.ok) { + const data = await resp.json(); + const models = data.models || []; + if (models.length) { + const modelWidget = chat.widgets.find(w => w.name === "model"); + if (modelWidget) modelWidget.value = models[0]; + } + } + } catch (e) {} + + await new Promise(r => setTimeout(r, 800)); + window.app.canvas.setDirty(true, true); + window.app.canvas.draw(true, true); + + const nodes = [client, chat]; + const minX = Math.min(...nodes.map(n => n.pos[0])) - 20; + const minY = Math.min(...nodes.map(n => n.pos[1])) - 20; + const maxX = Math.max(...nodes.map(n => n.pos[0] + n.size[0])) + 20; + const maxY = Math.max(...nodes.map(n => n.pos[1] + n.size[1])) + 20; + return { pos: [minX, minY], size: [maxX - minX, maxY - minY] }; + } + """ + ) + + await asyncio.sleep(1.0) + await _redraw(page) + await _frame_node(page, info["pos"], info["size"], scale=1.0) + await _capture(page, out, info["pos"], info["size"], scale=1.0) + + async def scene_ollama_workflow(page: Page, out: Path) -> None: - """Full mini-workflow: OllamaClient → OllamaChatCompletion + Temperature + Seed options.""" + """Full mini-workflow: OllamaClient → ChatCompletion + Temperature + Seed options.""" await _clear(page) info = await page.evaluate( @@ -386,7 +524,7 @@ async def scene_ollama_workflow(page: Page, out: Path) -> None: if (twSeed) twSeed.value = 42; // 4. ChatCompletion — right - const chat = LiteGraph.createNode("OllamaChatCompletion"); + const chat = LiteGraph.createNode("ChatCompletion"); chat.pos = [380, 100]; graph.add(chat); const twPrompt = chat.widgets && chat.widgets.find(w => w.name === "prompt"); @@ -409,7 +547,7 @@ async def scene_ollama_workflow(page: Page, out: Path) -> None: // Refresh model dropdowns for chat node try { - const resp = await fetch("/dv/ollama/models?host=http://host.docker.internal:11434"); + const resp = await fetch("/dv/ollama/models?host=http://host.docker.internal:11434&backend=ollama"); if (resp.ok) { const data = await resp.json(); const models = data.models || []; @@ -466,25 +604,25 @@ async def scene_ollama_lifecycle(page: Page, out: Path) -> None: if (hostWidget) hostWidget.value = "http://localhost:11434"; // OllamaLoadModel — generous gap right of client - const load = LiteGraph.createNode("OllamaLoadModel"); + const load = LiteGraph.createNode("LLMLoadModel"); load.pos = [380, 80]; graph.add(load); // OllamaChatCompletion — wide node, plenty of space to the right of load - const chat = LiteGraph.createNode("OllamaChatCompletion"); + const chat = LiteGraph.createNode("ChatCompletion"); chat.pos = [720, 40]; graph.add(chat); const twPrompt = chat.widgets && chat.widgets.find(w => w.name === "prompt"); if (twPrompt) twPrompt.value = "Describe this image in one sentence."; // OllamaUnloadModel — far right, vertically offset to match chat's outputs - const unload = LiteGraph.createNode("OllamaUnloadModel"); + const unload = LiteGraph.createNode("LLMUnloadModel"); unload.pos = [1200, 280]; graph.add(unload); // Populate model dropdowns from live Ollama try { - const resp = await fetch("/dv/ollama/models?host=http://host.docker.internal:11434"); + const resp = await fetch("/dv/ollama/models?host=http://host.docker.internal:11434&backend=ollama"); if (resp.ok) { const data = await resp.json(); const models = data.models || []; @@ -604,6 +742,11 @@ SCENES = [ ("ollama_workflow.png", scene_ollama_workflow), ("ollama_options.png", scene_ollama_options), ("ollama_lifecycle.png", scene_ollama_lifecycle), + # Structured output (ADR-007 / pydantic-ai) + ("structured_output.png", scene_structured_output), + # llama.cpp (spec 008) + ("llamacpp_client.png", scene_llamacpp_client), + ("llamacpp_workflow.png", scene_llamacpp_workflow), ] diff --git a/specs/008-llamacpp-integration/.beacon.toml b/specs/008-llamacpp-integration/.beacon.toml new file mode 100644 index 0000000..19b532e --- /dev/null +++ b/specs/008-llamacpp-integration/.beacon.toml @@ -0,0 +1 @@ +epic = "llamacpp-integration" diff --git a/specs/008-llamacpp-integration/checklists/requirements.md b/specs/008-llamacpp-integration/checklists/requirements.md new file mode 100644 index 0000000..3a0dc9f --- /dev/null +++ b/specs/008-llamacpp-integration/checklists/requirements.md @@ -0,0 +1,39 @@ +# Specification Quality Checklist: llama.cpp Model Integration + +**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 + +No [NEEDS CLARIFICATION] markers needed — scope boundaries (router-mode-only, +no GPU tuning, no auth/TLS, no Manager listing) came directly from the +parent epic's Non-goals (`project-management/Roadmap/epics/llamacpp-integration.md`) +and ADR-007. User Story 4 (swap backends without touching downstream nodes) +is the adapter pattern's central promise made concrete and testable, not +padding. diff --git a/specs/008-llamacpp-integration/contracts/llamacpp_provider_conformance.md b/specs/008-llamacpp-integration/contracts/llamacpp_provider_conformance.md new file mode 100644 index 0000000..50263cf --- /dev/null +++ b/specs/008-llamacpp-integration/contracts/llamacpp_provider_conformance.md @@ -0,0 +1,44 @@ +# Contract: `LlamaCppProvider` conforms to `LLMProvider` + +This is the concrete proof of ADR-007's adapter pattern — the same protocol +contract documented in +`specs/007-llm-provider-abstraction/contracts/llm_provider_protocol.md`, +now with a second implementation. Nothing in that contract changes; this +file only documents `LlamaCppProvider`'s specific wire-format bindings. + +```python +class LlamaCppProvider: + def __init__(self, host: str, headers: dict | None = None): ... + + async def list_models(self) -> list[ModelInfo]: + """GET {host}/models → data[] → ModelInfo(name=m["id"], status=ModelStatus(m["status"]["value"]), size=None)""" + + async def load_model(self, model: str) -> None: + """POST {host}/models/load {"model": model}""" + + async def unload_model(self, model: str) -> None: + """POST {host}/models/unload {"model": model}""" + + async def chat(self, model, messages, options=None, timeout_secs=300.0) -> str: + """POST {host}/v1/chat/completions → choices[0].message.content""" + + async def chat_structured(self, model, messages, schema, options=None, timeout_secs=300.0, max_retries=2) -> BaseModel: + """Delegates to comfydv._llm.chat.chat_structured(base_url=f"{host}/v1", ...) — identical call OllamaProvider makes""" +``` + +## Behavioral requirements (inherited from the protocol contract, restated for this implementation) + +- `load_model`/`unload_model` MUST be idempotent. **Live-verified against a + real router-mode server**: router mode's own endpoints are *not* + idempotent — `/models/load` on an already-loaded model returns HTTP 400 + `"model is already running"`, and `/models/unload` on an already-unloaded + model returns HTTP 400 `"model is not running"`, instead of `{"success": true}`. + `LlamaCppProvider` absorbs this itself: these two specific error messages + are treated as the desired end-state already reached, not a failure; any + other error still propagates. +- `list_models()` MUST NOT normalize away llama.cpp's `sleeping`/`downloading` + states (unlike `OllamaProvider`, which has no choice but to normalize — + see `research.md`). +- A `llama-server` not running in router mode (missing endpoints) MUST + surface a clear, specific error (spec.md FR-006) — not a generic + connection failure indistinguishable from "server not running at all." diff --git a/specs/008-llamacpp-integration/data-model.md b/specs/008-llamacpp-integration/data-model.md new file mode 100644 index 0000000..d1fb65a --- /dev/null +++ b/specs/008-llamacpp-integration/data-model.md @@ -0,0 +1,41 @@ +# Data Model: llama.cpp Model Integration + +No new types — this feature is a second implementation of the existing +`LLMProvider` protocol, `ModelStatus`, `ModelInfo`, and `Message` types +(`src/comfydv/_llm/provider.py`, unchanged). This file documents +`LlamaCppProvider`'s field mapping from llama-server's router-mode JSON onto +those existing types (see `research.md` for the verified API shapes). + +## `LlamaCppProvider.list_models()` → `ModelInfo` mapping + +| `ModelInfo` field | Source (`GET /models` response, per model in `data[]`) | +|---|---| +| `name` | `id` — **not** `name` (llama.cpp's field name differs from Ollama's) | +| `status` | `status.value` — nested object, not a flat string | +| `size` | Not provided by this endpoint; `None` | + +`status.value` maps directly onto `ModelStatus`'s five values +(`unloaded`/`loading`/`loaded`/`sleeping`/`downloading`) — llama.cpp's +vocabulary is exactly `ModelStatus`'s full set, so unlike `OllamaProvider` +(which normalizes into a narrower subset), `LlamaCppProvider` needs no +approximation. A `"failed": true` state exists outside this vocabulary +(model process crashed) — out of scope per spec.md's edge cases; treated as +whatever `status.value` reports rather than added as a sixth enum value. + +## `LlamaCppProvider.load_model()` / `unload_model()` + +Both `POST /models/load` and `POST /models/unload` take `{"model": }` — +the same `id` string `list_models()` returns as `ModelInfo.name`. No mapping +ambiguity here (unlike Ollama, where load/unload uses `/api/generate`'s +`keep_alive` side effect rather than a dedicated endpoint). + +## `LlamaCppProvider.chat()` / `chat_structured()` + +Both reach `llama-server`'s OpenAI-compatible `/v1/chat/completions` — +`chat_structured()` calls the existing shared `comfydv._llm.chat.chat_structured()` +helper unchanged (`base_url=f"{self.host}/v1"`, matching `OllamaProvider`'s +own call exactly). `chat()` parses the response as +`choices[0].message.content` (OpenAI shape), not Ollama's native +`message.content` — the two providers' non-structured paths differ here +because llama-server doesn't have an Ollama-style native `/api/chat` +endpoint to prefer instead. diff --git a/specs/008-llamacpp-integration/features/us1_connect_and_chat.feature b/specs/008-llamacpp-integration/features/us1_connect_and_chat.feature new file mode 100644 index 0000000..a820263 --- /dev/null +++ b/specs/008-llamacpp-integration/features/us1_connect_and_chat.feature @@ -0,0 +1,11 @@ +Feature: US1 — Connect to a local llama.cpp server and get chat responses + + Scenario: llama.cpp connection node feeds the existing chat node + Given a running local llama-server (router mode) and a workflow with a llama.cpp connection node wired into the existing chat node + When the workflow executes + Then the chat node returns the model's text response + + Scenario: Unreachable llama.cpp server surfaces a clear error + Given the llama.cpp connection node configured with an unreachable server address + When the workflow executes + Then the chat node reports a clear connection error diff --git a/specs/008-llamacpp-integration/features/us2_structured_output.feature b/specs/008-llamacpp-integration/features/us2_structured_output.feature new file mode 100644 index 0000000..460f5af --- /dev/null +++ b/specs/008-llamacpp-integration/features/us2_structured_output.feature @@ -0,0 +1,11 @@ +Feature: US2 — Get structured, validated output from llama.cpp + + Scenario: Valid structured response exposes typed fields, same as Ollama + Given a chat node connected to llama.cpp 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 retries then fails clearly, same as Ollama + Given a llama.cpp-hosted model that returns invalid or incomplete structured output + When the workflow executes + Then the node retries automatically and, if still unsuccessful, fails with a clear error diff --git a/specs/008-llamacpp-integration/features/us3_model_lifecycle.feature b/specs/008-llamacpp-integration/features/us3_model_lifecycle.feature new file mode 100644 index 0000000..a3df589 --- /dev/null +++ b/specs/008-llamacpp-integration/features/us3_model_lifecycle.feature @@ -0,0 +1,16 @@ +Feature: US3 — See and control which models are loaded on llama.cpp + + Scenario: List models with full status vocabulary + Given a running local llama-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, drawn from llama.cpp's full status vocabulary + + 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 diff --git a/specs/008-llamacpp-integration/features/us4_swap_backends.feature b/specs/008-llamacpp-integration/features/us4_swap_backends.feature new file mode 100644 index 0000000..6e95fef --- /dev/null +++ b/specs/008-llamacpp-integration/features/us4_swap_backends.feature @@ -0,0 +1,6 @@ +Feature: US4 — Swap from Ollama to llama.cpp without touching the rest of the workflow + + Scenario: Replacing only the connection node preserves the workflow + Given a workflow with chat/model-management nodes wired to an Ollama connection node + When a workflow author replaces only the connection node with a llama.cpp one, pointed at a running llama-server + Then the workflow runs successfully with no changes to any other node diff --git a/specs/008-llamacpp-integration/plan.md b/specs/008-llamacpp-integration/plan.md new file mode 100644 index 0000000..7501d0d --- /dev/null +++ b/specs/008-llamacpp-integration/plan.md @@ -0,0 +1,117 @@ +# Implementation Plan: llama.cpp Model Integration + +**Branch**: `008-llamacpp-integration` | **Date**: 2026-07-11 | **Spec**: [spec.md](./spec.md) + +**Input**: Feature specification from `/specs/008-llamacpp-integration/spec.md` + +**Note**: This template is filled in by the `/speckit-plan` command. See `.specify/templates/plan-template.md` for the execution workflow. + +## Summary + +Implement `LlamaCppProvider` as the second `LLMProvider` (ADR-007), backed by +`llama-server`'s router mode (`GET /models`, `POST /models/load`, +`POST /models/unload`, `/v1/chat/completions`). Add one new ComfyUI node +(`LlamaCppClient`) emitting the existing `LLM_CLIENT` socket type — no other +node classes change. This is the concrete proof the provider abstraction +(prerequisite epic, PR #17) actually generalizes: a second backend, zero +changes to `ChatCompletion`/`LLMModelSelector`/`LLMLoadModel`/`LLMUnloadModel`. + +## Technical Context + +**Language/Version**: Python ≥3.11 (unchanged, per `pyproject.toml`) + +**Primary Dependencies**: `aiohttp` (existing — model-management REST calls), +`pydantic-ai`/`openai` (existing, from the prerequisite epic — `chat_structured()` +reuses the shared helper unchanged, zero new structured-output code) + +**Storage**: N/A — no persistent storage; reuses the existing +`_MODEL_LIST_CACHE`/`_CHAT_RESPONSE_CACHE` infra pattern from `OllamaProvider` + +**Testing**: `pytest` via `uv run pytest`, following `tests/test_ollama_provider.py`'s +established convention (mock at the provider's own `_post_json`/`_get_json` +seam, no live server required for unit tests) + +**Target Platform**: ComfyUI custom-node runtime, same as the existing Ollama +integration + +**Project Type**: Library / ComfyUI custom-node pack (single project, adds to +existing `src/comfydv/` layout) + +**Performance Goals**: No new numeric target; must not add latency beyond +what `OllamaProvider`'s equivalent methods already accept + +**Constraints**: Router-mode-only (spec.md Assumptions — a `llama-server` +without `--models-dir`/`--models-preset` doesn't expose these endpoints at +all, FR-006); model identifier field is `id` (llama.cpp) vs `name` (Ollama) — +`LlamaCppProvider.list_models()` must map this correctly (see `research.md`); +`status` is a nested object (`{"value": "..."}`), not a flat string + +**Scale/Scope**: One new class (`LlamaCppProvider`, mirrors `OllamaProvider`'s +shape), one new ComfyUI node (`LlamaCppClient`), one new test file — no +changes to `ollama.py`, `_llm/provider.py`, `_llm/chat.py`, or any existing +node class + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +| Principle | Verdict | Notes | +|---|---|---| +| I. ComfyUI Contract First | PASS | `LlamaCppClient` exposes the standard `INPUT_TYPES`/`RETURN_TYPES`/`FUNCTION`/`CATEGORY`; registered in `NODE_CLASS_MAPPINGS` like every other node. | +| II. Sandbox All User-Supplied Code | N/A | No template/expression evaluation in this feature. | +| III. Test-First | PASS (binding) | `tests/test_llamacpp_provider.py` written test-first, mirroring `test_ollama_provider.py`'s TDD-pair structure. | +| IV. Graceful Degradation Outside ComfyUI | PASS (binding) | `LlamaCppProvider` lives in `src/comfydv/_llm/`, which already has no `comfy`/`server` imports at module scope (verified for the prerequisite epic; this feature adds no new module-scope imports of either). | +| V. Simplicity — Function Before Class | PASS, same justification as `OllamaProvider` | `LlamaCppProvider` carries connection state (host, headers) across 5 methods — the same shared-state condition that already justified `OllamaProvider` as a class (research.md, prerequisite epic). No new gate — same precedent applies. | +| VI. Fixed Output Positions | N/A | `LlamaCppClient`'s single output (`client`) isn't a multi-output node; no positional contract to preserve. | + +Re-checked post-Phase 1 design (data-model.md): unchanged — no new gate +violations. No Complexity Tracking entries needed (unlike the prerequisite +epic, this feature introduces no new pattern, just a second instance of an +already-justified one). + +## Project Structure + +### Documentation (this feature) + +```text +specs/008-llamacpp-integration/ +├── plan.md # This file +├── research.md # Phase 0 — router-mode API shape, verified live +├── data-model.md # Phase 1 — LlamaCppProvider field mapping +├── quickstart.md # Phase 1 — minimal workflow walkthrough +├── contracts/ # Phase 1 — LlamaCppProvider's protocol conformance +└── tasks.md # Phase 2 (/speckit-tasks) +``` + +### Source Code (repository root) + +```text +src/comfydv/ +├── ollama.py # unchanged — add LlamaCppClient node only via a new module +├── llamacpp.py # new — LlamaCppClient node (mirrors OllamaClient's shape) +├── _llm/ +│ ├── provider.py # unchanged — LLMProvider/ModelStatus/ModelInfo/Message +│ ├── ollama_provider.py # unchanged +│ ├── llamacpp_provider.py # new — LlamaCppProvider (mirrors ollama_provider.py's shape) +│ └── chat.py # unchanged — chat_structured() reused as-is +└── __init__.py # add LlamaCppClient import + NODE_CLASS_MAPPINGS entry + +tests/ +├── test_ollama_provider.py # unchanged +├── test_llamacpp_provider.py # new — mirrors test_ollama_provider.py's structure +└── test_llamacpp.py # new — LlamaCppClient node contract test (small; mirrors + # the OllamaClient-specific slice of test_ollama.py) +``` + +**Structure Decision**: New `src/comfydv/llamacpp.py` module (not added into +`ollama.py`) for the `LlamaCppClient` node, and a new `src/comfydv/_llm/llamacpp_provider.py` +for `LlamaCppProvider` — mirroring the existing `ollama.py`/`ollama_provider.py` +split exactly, so the two backends read as parallel, symmetric implementations +rather than one growing to accommodate the other. No existing file is +modified except `__init__.py`'s registration block. + +## Complexity Tracking + +> **Fill ONLY if Constitution Check has violations that must be justified** + +None — see Constitution Check above. diff --git a/specs/008-llamacpp-integration/quickstart.md b/specs/008-llamacpp-integration/quickstart.md new file mode 100644 index 0000000..64b0e19 --- /dev/null +++ b/specs/008-llamacpp-integration/quickstart.md @@ -0,0 +1,26 @@ +# Quickstart: llama.cpp Model Integration + +## Prerequisite + +Launch `llama-server` in router mode: + +```bash +llama-server --models-dir ./models -c 8192 +``` + +## Minimal workflow + +1. Add an **LlamaCpp Client** node. Set its host widget (default + `http://localhost:8080`, llama-server's default port). +2. Wire it into a **Chat Completion** node — the exact same node used for + Ollama. Set a model and prompt, run. +3. Structured output, model listing, and load/unload all work exactly as + documented for Ollama in the main README/quickstart — swap the client + node, nothing else changes. + +## Swapping an existing Ollama workflow to llama.cpp + +Replace the **Ollama Client** node with an **LlamaCpp Client** node, pointed +at your running `llama-server`. Every downstream node (Chat Completion, LLM +Model Selector, LLM Load Model, LLM Unload Model) keeps working unmodified — +this is the whole point of the provider abstraction (ADR-007). diff --git a/specs/008-llamacpp-integration/research.md b/specs/008-llamacpp-integration/research.md new file mode 100644 index 0000000..5ec508c --- /dev/null +++ b/specs/008-llamacpp-integration/research.md @@ -0,0 +1,78 @@ +# Research: llama.cpp Model Integration + +## Decision: exact router-mode API shape (verified against `ggml-org/llama.cpp`'s live `tools/server/README.md`, not assumed) + +llama.cpp's router mode postdates this session's training data — verified live +against the authoritative source rather than guessed, since getting field +names wrong here would silently produce broken code (wrong key = `KeyError` +or silent `None`, not an obvious failure). + +**`GET /models`** response: + +```json +{ + "data": [ + { + "id": "ggml-org/gemma-3-4b-it-GGUF:Q4_K_M", + "path": "/Users/.../gemma-3-4b-it-Q4_K_M.gguf", + "status": { + "value": "loaded", + "args": ["llama-server", "-ctx", "4096"] + }, + "architecture": { + "input_modalities": ["text", "image"], + "output_modalities": ["text"] + } + } + ] +} +``` + +**Two details that would have been wrong by assumption:** + +1. The model identifier field is **`id`**, not `name` — different from Ollama's + `/api/tags`, which uses `name`. `OllamaProvider.list_models()` maps + `m["name"]`; `LlamaCppProvider.list_models()` must map `m["id"]` instead. +2. **`status` is a nested object** (`{"value": "loaded", ...}`), not a flat + string field. `LlamaCppProvider.list_models()` must read + `m["status"]["value"]`, not `m["status"]` directly. A `"failed"` state + also exists (`{"failed": true, "exit_code": ...}`) outside the five + `ModelStatus` values the protocol defines — not handled by this feature + (see Non-goals/edge cases in `spec.md`); a failed model is reported as + whatever `status.value` degrades to rather than added as a sixth enum + value, keeping `ModelStatus` unchanged across both providers. + +**`POST /models/load`** and **`POST /models/unload`**: identical request +shape, `{"model": ""}` (using the same `id` string from `GET /models`, +despite the request field being named `model` not `id`). Response: +`{"success": true}`. + +**CLI**: `--models-dir ` or `--models-preset .ini` — a deployment +prerequisite (spec.md Assumptions), not something comfydv configures. + +## Decision: `chat_structured()` needs zero new code + +`llama-server`'s `/v1/chat/completions` is OpenAI-compatible (the same +assumption ADR-007 made when adopting `pydantic-ai`). `LlamaCppProvider.chat_structured()` +calls the exact same `comfydv._llm.chat.chat_structured()` helper +`OllamaProvider` already uses, with `base_url=f"{self.host}/v1"` — the only +per-provider difference. This is the concrete proof the shared mechanism +generalizes (spec.md User Story 2/FR-004), not just an assumption. + +## Decision: `chat()` (non-structured) also reuses the OpenAI-compatible endpoint + +Unlike Ollama (which has both a native `/api/chat` and an OpenAI-compat +`/v1/chat/completions`), llama-server's primary chat endpoint is the +OpenAI-compatible one. `LlamaCppProvider.chat()` POSTs to +`{host}/v1/chat/completions` (via the existing `_post_json` helper, no new +HTTP client) rather than mirroring Ollama's native-endpoint choice — the +response shape (`choices[0].message.content`) differs from Ollama's native +`message.content` and must be parsed accordingly. + +## Decision: no protocol changes needed + +`LLMProvider`'s five methods (`list_models`/`load_model`/`unload_model`/ +`chat`/`chat_structured`) already cover everything router mode needs — this +was the actual point of designing the protocol at the operation level in +ADR-007, and this research confirms it held up against llama.cpp's real API, +not just Ollama's. diff --git a/specs/008-llamacpp-integration/spec.md b/specs/008-llamacpp-integration/spec.md new file mode 100644 index 0000000..e3db3f4 --- /dev/null +++ b/specs/008-llamacpp-integration/spec.md @@ -0,0 +1,110 @@ +# Feature Specification: llama.cpp Model Integration + +**Feature Branch**: `008-llamacpp-integration` + +**Created**: 2026-07-11 + +**Status**: Draft + +**Input**: User description: "Add ComfyUI nodes for llama.cpp local inference via llama-server's router mode, implementing the LlamaCppProvider as the second LLMProvider (ADR-007) alongside the existing OllamaProvider. Router mode exposes GET /models (with live status), POST /models/load, POST /models/unload, giving llama.cpp the same manual load/unload memory-management primitives as Ollama. No new ComfyUI node classes needed for chat/model-selection/load/unload — only a new LlamaCppClient config node; the existing generic ChatCompletion/LLMModelSelector/LLMLoadModel/LLMUnloadModel nodes work unchanged once wired to it." + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Connect to a local llama.cpp server and get chat responses (Priority: P1) 🎯 MVP + +As a ComfyUI workflow author running `llama-server` locally, I want a connection node for it — just like the one I already use for Ollama — so I can get chat responses from a llama.cpp-hosted model using the same chat node I already know. + +**Why this priority**: This is the entire point of the feature and the proof that the provider abstraction (shipped in the prerequisite epic) actually works: a second backend, zero changes to the chat node. + +**Independent Test**: Wire a new llama.cpp connection node into the existing chat node, run against a local `llama-server` (router mode), confirm a text response. + +**Acceptance Scenarios**: + +1. **Given** a running local `llama-server` (router mode) and a workflow with a llama.cpp connection node wired into the existing chat node, **When** the workflow executes, **Then** the chat node returns the model's text response — using the exact same chat node a workflow author already uses for Ollama. +2. **Given** the llama.cpp connection node configured with an unreachable server address, **When** the workflow executes, **Then** the chat node reports a clear connection error, matching the behavior workflow authors already know from the Ollama connection. + +--- + +### User Story 2 - Get structured, validated output from llama.cpp (Priority: P1) + +As a workflow author, I want structured output (a schema-validated response instead of free text) to work identically regardless of whether I'm connected to Ollama or llama.cpp, so I don't have to relearn or rebuild anything when switching backends. + +**Why this priority**: Structured output is a core existing capability (already proven for Ollama); this story proves the shared mechanism genuinely generalizes rather than being Ollama-specific in practice, not just in name. + +**Independent Test**: Enable structured output on the chat node with a schema, run against a llama.cpp-hosted model, confirm each schema field is populated and never blank — using the same steps as the equivalent Ollama test. + +**Acceptance Scenarios**: + +1. **Given** a chat node connected to llama.cpp 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 llama.cpp-hosted model that returns invalid or incomplete structured output, **When** the workflow executes, **Then** the node retries automatically and, if still unsuccessful, fails with a clear error — identical behavior to the Ollama path. + +--- + +### User Story 3 - See and control which models are loaded on llama.cpp (Priority: P2) + +As a workflow author running models locally, I want to see live model status (including whether a model is currently loading or being downloaded, not just loaded/unloaded) and explicitly load or unload a model on my llama.cpp server, so I can manage memory the same way I already do for Ollama — with more visibility, since llama.cpp's router mode reports richer status than Ollama does. + +**Why this priority**: Valuable and proves the model-management path generalizes too, but a workflow can still run chat completions without ever calling load/unload explicitly (the server can load on first use), so it's lower risk to defer than basic chat. + +**Independent Test**: Use the existing model-listing node against a running `llama-server`, confirm it shows each available model with its current status (including `loading`/`downloading` if applicable); use the existing load/unload nodes against one model and confirm its status changes. + +**Acceptance Scenarios**: + +1. **Given** a running local `llama-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, drawn from llama.cpp's full status vocabulary (not just loaded/unloaded). +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 - Swap from Ollama to llama.cpp without touching the rest of the workflow (Priority: P3) + +As a workflow author with an existing Ollama-based workflow, I want to switch it to llama.cpp by changing only the connection node, so I don't have to rebuild my chat/model-management logic for a second backend. + +**Why this priority**: This is the adapter pattern's actual promise made concrete for a user, but it's a validation/demonstration story rather than new capability — everything it depends on is already covered by User Stories 1–3. + +**Independent Test**: Take a workflow using the Ollama connection node, replace it with the llama.cpp connection node (same downstream nodes, no other changes), run it, confirm it still works. + +**Acceptance Scenarios**: + +1. **Given** a workflow with chat/model-management nodes wired to an Ollama connection node, **When** a workflow author replaces only the connection node with a llama.cpp one (pointed at a running `llama-server`), **Then** the workflow runs successfully with no changes to any other node. + +--- + +### Edge Cases + +- What happens when `llama-server` is running but was launched without router mode (i.e. with `-m` instead of `--models-dir`)? The router-mode-only endpoints this feature depends on won't exist — the connection/model-management nodes should fail with a clear error, not hang or silently return empty results. +- 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 llama.cpp reports a model status this feature doesn't expect (a router-mode API change)? Should degrade gracefully (surface the status if recognized, don't crash on an unrecognized one), not silently misreport. +- What happens to an in-flight chat request if the model it depends on is unloaded by another node in the same workflow run? (Same question already answered for Ollama — behavior should be consistent.) + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: The system MUST allow a workflow author to configure a connection to a local `llama-server` (router mode) the same way they already configure a connection to Ollama — a dedicated connection node, reusable across multiple nodes in a workflow. +- **FR-002**: The system MUST NOT require any new or different node classes for chat, structured output, model listing, or load/unload when using llama.cpp — the existing generic nodes MUST work unchanged once connected to a llama.cpp connection node. +- **FR-003**: The system MUST report each model's status using llama.cpp's full status vocabulary (unloaded, loading, loaded, sleeping, downloading) when connected to llama.cpp — not degraded to the narrower Ollama-compatible set. +- **FR-004**: The system's chat and structured-output behavior MUST be identical between Ollama and llama.cpp connections, given equivalent inputs — same retry limits, same validation rules, same error conditions (this is the direct continuation of the prerequisite epic's own FR-007/FR-008). +- **FR-005**: The system MUST allow a workflow author to explicitly load a model into memory and explicitly unload a model from memory on a connected llama.cpp server. +- **FR-006**: The system MUST surface a clear, specific error when connected to a `llama-server` instance that isn't running in router mode (the endpoints this feature needs don't exist), rather than an unhelpful generic failure. + +### Key Entities *(include if feature involves data)* + +- **llama.cpp connection**: A configured connection to a local `llama-server` instance running in router mode (host + any authentication), implementing the same connection concept already established for Ollama. +- **Model status**: Reuses the existing status concept from the prerequisite feature, now populated with llama.cpp's full vocabulary rather than a narrowed subset. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: A workflow author can connect to a llama.cpp server and get a chat response using the same node count and shape as connecting to Ollama (one connection node, one chat node) — no new nodes to learn for the chat path. +- **SC-002**: An existing workflow can be repointed from Ollama to llama.cpp by changing exactly one node (the connection node) — zero edits to any chat or model-management node. +- **SC-003**: Structured-output workflows behave identically (same validation guarantees, zero blank-required-field results) regardless of which backend is connected. +- **SC-004**: Model status reporting for llama.cpp surfaces all five status values where applicable — a strictly richer view than what Ollama can report through the same interface. + +## Assumptions + +- Workflow authors run their own local `llama-server` instance, launched in router mode (`--models-dir` or `--models-preset`), reachable over HTTP from the machine running ComfyUI; this feature does not install, configure, or launch that server. +- Non-router-mode `llama-server` usage (a single model launched with `-m`) is out of scope — router mode is required for the load/unload/status parity with Ollama that is this feature's whole point. +- GPU inference optimisation, quantisation tuning, authentication/TLS, and ComfyUI Manager registry listing are out of scope, consistent with the prerequisite Ollama epic's own non-goals. +- The `LLMProvider` protocol and generic nodes (`ChatCompletion`, `LLMModelSelector`, `LLMLoadModel`, `LLMUnloadModel`) already exist and are not modified by this feature — if llama.cpp's router mode needs a protocol capability that doesn't exist yet, that is a protocol change scoped as its own follow-up, not silently special-cased here. diff --git a/specs/008-llamacpp-integration/tasks.md b/specs/008-llamacpp-integration/tasks.md new file mode 100644 index 0000000..c4d7bbb --- /dev/null +++ b/specs/008-llamacpp-integration/tasks.md @@ -0,0 +1,224 @@ +# Tasks: llama.cpp Model Integration + +**Input**: Design documents from `/specs/008-llamacpp-integration/` + +**Prerequisites**: plan.md, spec.md, research.md, data-model.md, contracts/llamacpp_provider_conformance.md + +**Tests**: First-class — every implementation task has a paired failing-test task (`-T`/`-I` suffix). + +**Organization**: Grouped by user story (spec.md priorities P1/P1/P2/P3). + +## Format: `[ID] [P?] [Story] Description` + +- **[P]**: Can run in parallel (different files, no dependencies) +- **[Story]**: US1–US4 +- **-T / -I**: paired test (red) / implementation (green) + +## Path Conventions + +Single project: `src/comfydv/`, `tests/` at repository root, mirroring the +`ollama.py`/`_llm/ollama_provider.py` split exactly (plan.md Structure +Decision). + +--- + +## Phase 1: Setup + +- [x] T001 No new dependencies — `aiohttp`/`pydantic-ai` already present from the prerequisite epic (verified in `pyproject.toml`) +- [x] T002 [P] Create `src/comfydv/_llm/llamacpp_provider.py` and `src/comfydv/llamacpp.py` (empty modules with docstrings, mirroring `ollama_provider.py`/`ollama.py`'s module docstring style) + +--- + +## Phase 2: Foundational + +None — `LLMProvider`, `ModelStatus`, `ModelInfo`, `Message`, and the shared +`chat_structured()` helper already exist from the prerequisite epic and are +unmodified by this feature (plan.md Constitution Check, research.md). + +**Checkpoint**: nothing blocks user story work — it can start immediately. + +--- + +## Phase 3: User Story 1 — Connect to a local llama.cpp server and get chat responses (Priority: P1) 🎯 MVP + +**Goal**: A workflow author wires an `LlamaCppClient` node into the existing `ChatCompletion` node and gets a text response. + +**Independent Test**: Wire `LlamaCppClient` → `ChatCompletion`, run against a live `llama-server` (router mode), confirm text output. + +- [x] T003-T [US1] Write FAILING test: `LlamaCppProvider.chat()` POSTs to `{host}/v1/chat/completions` and parses `choices[0].message.content`, in `tests/test_llamacpp_provider.py` (witnesses `features/us1_connect_and_chat.feature` scenario "llama.cpp connection node feeds the existing chat node") +- [x] T003-I [US1] Implement `LlamaCppProvider.__init__`/`.chat()` in `src/comfydv/_llm/llamacpp_provider.py` (data-model.md — OpenAI-shape response parsing, not Ollama's native shape) — makes T003-T pass +- [x] T004-T [US1] Write FAILING test: `LlamaCppClient` node's `INPUT_TYPES`/`RETURN_TYPES` match `OllamaClient`'s shape (`LLM_CLIENT` output), and `create_client()` constructs a `LlamaCppProvider`, in `tests/test_llamacpp.py` +- [x] T004-I [US1] Implement `LlamaCppClient` node in `src/comfydv/llamacpp.py` (mirrors `OllamaClient` exactly, default host `http://localhost:8080` per llama-server's default port) — makes T004-T pass (depends on T003-I) +- [x] T005-T [US1] Write FAILING test: `LlamaCppClient` registered in `NODE_CLASS_MAPPINGS`/`NODE_DISPLAY_NAME_MAPPINGS`, in `tests/test_llamacpp.py` +- [x] T005-I [US1] Register `LlamaCppClient` in `src/comfydv/__init__.py` — makes T005-T pass (depends on T004-I) +- [x] T006-T [US1] Write FAILING test: `LlamaCppProvider` connection error surfaces a clear message (mirrors `OllamaProvider`'s `_post_json` connection-error contract), in `tests/test_llamacpp_provider.py` (witnesses `features/us1_connect_and_chat.feature` scenario "Unreachable llama.cpp server surfaces a clear error") +- [x] T006-I [US1] Confirm `LlamaCppProvider.chat()` reuses the shared `_post_json` connection-error handling unchanged (likely no code change needed — verify, don't assume) — makes T006-T pass + +**Checkpoint**: US1 fully functional and independently testable (MVP) — proves the adapter pattern for the chat path. + +--- + +## Phase 4: User Story 2 — Get structured, validated output from llama.cpp (Priority: P1) + +**Goal**: `structured_output=True` on `ChatCompletion` works identically against llama.cpp. + +**Independent Test**: Enable `structured_output` with a schema, run against a llama.cpp-hosted model, confirm typed sockets populate and are never blank. + +- [x] T007-T [US2] Write FAILING test: `LlamaCppProvider.chat_structured()` builds `base_url=f"{host}/v1"` and delegates to the shared `comfydv._llm.chat.chat_structured()` helper unchanged, in `tests/test_llamacpp_provider.py` (witnesses `features/us2_structured_output.feature` scenario "Valid structured response exposes typed fields, same as Ollama") +- [x] T007-I [US2] Implement `LlamaCppProvider.chat_structured()` in `src/comfydv/_llm/llamacpp_provider.py` — zero new structured-output logic, same call shape `OllamaProvider.chat_structured()` already makes — makes T007-T pass +- [x] T008 [US2] No new test needed for the retry-then-fail path (witnesses `features/us2_structured_output.feature` scenario "Invalid response retries then fails clearly, same as Ollama") — already fully covered by `tests/test_llm_chat_structured.py`'s existing suite, since `LlamaCppProvider.chat_structured()` calls the identical shared helper `OllamaProvider` does; re-testing it here would duplicate coverage without adding confidence (same reasoning as the prerequisite epic's D5) + +**Checkpoint**: US1 + US2 both independently functional — the chat surface is now backend-agnostic in practice, not just in name. + +--- + +## Phase 5: User Story 3 — See and control which models are loaded on llama.cpp (Priority: P2) + +**Goal**: `LLMModelSelector`/`LLMLoadModel`/`LLMUnloadModel` work against llama.cpp via `LlamaCppProvider`. + +**Independent Test**: List models via `LLMModelSelector` wired to `LlamaCppClient`; load/unload one; confirm status changes, including `loading`/`downloading` states if triggered. + +- [x] T009-T [P] [US3] Write FAILING test: `LlamaCppProvider.list_models()` maps `GET /models`'s `data[].id`→`ModelInfo.name` and `data[].status.value`→`ModelInfo.status`, surfacing all five `ModelStatus` values without normalization (data-model.md), in `tests/test_llamacpp_provider.py` (witnesses `features/us3_model_lifecycle.feature` scenario "List models with full status vocabulary") +- [x] T009-I [US3] Implement `LlamaCppProvider.list_models()` in `src/comfydv/_llm/llamacpp_provider.py` — makes T009-T pass +- [x] T010-T [P] [US3] Write FAILING test: `LlamaCppProvider.load_model()`/`unload_model()` POST `{"model": id}` to `/models/load`/`/models/unload` and are idempotent, in `tests/test_llamacpp_provider.py` (witnesses `features/us3_model_lifecycle.feature` scenarios "Load a model into memory" and "Unload a model from memory") +- [x] T010-I [US3] Implement `LlamaCppProvider.load_model()`/`unload_model()` in `src/comfydv/_llm/llamacpp_provider.py` — makes T010-T pass +- [x] T011 [US3] No new node-layer tests needed — `LLMModelSelector`/`LLMLoadModel`/`LLMUnloadModel` are untouched by this epic (plan.md Structure Decision) and already have delegation-test coverage against a generic `_FakeProvider` in `tests/test_ollama.py`; that coverage is provider-agnostic by construction (FR-002), so it already proves these nodes work with `LlamaCppProvider` too, not just `OllamaProvider` + +**Checkpoint**: US1 + US2 + US3 independently functional. + +--- + +## Phase 6: User Story 4 — Swap from Ollama to llama.cpp without touching the rest of the workflow (Priority: P3) + +**Goal**: Demonstrate/prove the adapter pattern's actual promise end-to-end. + +**Independent Test**: Same workflow, only the connection node changes. + +- [x] T012-T [US4] Write FAILING test: a workflow-shaped test (client → `ChatCompletion` → `LLMModelSelector` → `LLMLoadModel` → `LLMUnloadModel`) runs identically whether `client` is an `OllamaProvider`-double or a `LlamaCppProvider`-double — i.e. no node branches on provider type, in `tests/test_llamacpp.py` (witnesses `features/us4_swap_backends.feature` scenario "Replacing only the connection node preserves the workflow") +- [x] T012-I [US4] No implementation expected — this test should already pass given T003-T011 (it's a regression/integration proof, not new functionality); if it fails, that reveals a node secretly branching on provider type, which would be a real bug to fix, not a feature to add + +**Checkpoint**: all four user stories independently functional; the adapter pattern is proven end-to-end, not just asserted. + +--- + +## Phase 7: Polish & Cross-Cutting Concerns + +- [x] T013 [P] `ruff check --fix && ruff format` — clean +- [x] T014 [P] `ty check` — clean, same pre-existing diagnostics as the prerequisite epic (unrelated to this feature — `comfy`/`server`/`folder_paths` unresolved-import, `format_string.py`'s dynamic RETURN_TYPES, `create_model`/`RandomChoice` — none touch the new files) +- [x] T015 Confirmed via grep: `llamacpp_provider.py`/`llamacpp.py` import no `comfy`/`server`/`folder_paths` at module scope +- [x] T016 `beacon doctor --strict`: only the pre-existing `llm-provider-abstraction: all specs [complete]` epic-gates item (PR #18, the archive-bookkeeping PR for the *prerequisite* epic, not yet merged — unrelated to this feature) and `tdd-commit-discipline` (disclosed pattern, same reasoning as the prerequisite epic) +- [x] T017 Live smoke test — run against a real router-mode `llama-server` (Homebrew-installed, already present in the dev environment; a prior pass wrongly assumed no server was reachable without actually checking). Full lifecycle exercised against a real 5.6GB local GGUF model: `list_models()` → `load_model()` → `chat()` → `unload_model()`, plus explicit idempotency checks (calling `load_model()`/`unload_model()` again in the already-satisfied state). Found and fixed a real gap not caught by the mocked suite — see the finding below. + +--- + +## Dependencies & Execution Order + +### Phase Dependencies + +- **Setup (Phase 1)**: no dependencies +- **Foundational (Phase 2)**: none — nothing blocks user story work +- **US1**: no dependency on other stories — genuinely the MVP +- **US2**: depends on US1's `LlamaCppProvider` skeleton existing (T003-I), but its own logic (T007) has no dependency on US1's chat() specifically +- **US3**: independent of US1/US2 except sharing `LlamaCppProvider`'s constructor (T003-I) — unlike the prerequisite epic's atomic cutover, there is no shared "client output type" migration risk here, since `LlamaCppClient` is a brand-new node, not a changed one +- **US4**: depends on US1–US3 all being done (it's a proof, not new functionality) +- **Polish**: depends on all four user stories + +### Parallel Opportunities + +- T002 can start immediately +- T009-T and T010-T can run in parallel (different methods, same file, no shared state) +- T013/T014 can run in parallel in Polish + +--- + +## Implementation Strategy + +### MVP First + +1. Phase 1 (Setup, trivial) → Phase 3 (US1) → **STOP and validate US1 independently** against a live `llama-server`. + +### Incremental Delivery + +1. US1 → validate → basic chat parity with Ollama, on a second backend. +2. US2 → validate → structured-output parity — the shared mechanism holds. +3. US3 → validate → model-management parity, with richer status than Ollama can offer. +4. US4 → validate → the adapter pattern is proven, not just asserted. +5. Polish. + +Unlike the prerequisite epic, **this decomposition genuinely holds** — +there is no shared "output type" migration forcing an atomic cutover, because +`LlamaCppClient` is new, not a change to an existing node. Each phase really +can land independently. + +--- + +## Post-implementation review finding (fixed) + +A `beacon-reviewer` pass ahead of PR open found `LlamaCppProvider.list_models()` +caught *every* exception and returned `[]`, silently indistinguishable from +"no models installed" — violating FR-006 and `contracts/llamacpp_provider_conformance.md`'s +explicit requirement that a non-router-mode `llama-server` (unreachable +endpoints → HTTP error on `GET /models`) surface a clear, specific error. + +Fixed: `_get_json` (shared with `OllamaProvider`, in `ollama_provider.py`) now +raises `RuntimeError` on an HTTP error status, matching `_post_json`'s +existing behavior — its docstring already claimed this, it just didn't do it. +`LlamaCppProvider.list_models()` now distinguishes `OSError` (genuinely +unreachable — connection refused, DNS failure, timeout; all aiohttp +connection-level exceptions are `OSError` subclasses) from `RuntimeError` +(server responded, but with an error): the former still degrades gracefully +to `[]` (consistent with `OllamaProvider`'s existing UX), the latter is +re-raised naming router mode as the likely cause. Regression test added: +`test_list_models_non_router_mode_raises_clear_error` in +`tests/test_llamacpp_provider.py`. `OllamaProvider`'s own `list_models()`/ +`_fetch_models()` still catch broadly and degrade to `[]` unchanged — no +spec requirement asks Ollama to make this distinction, and this fix doesn't +force it to. + +--- + +## Live smoke test finding (T017, fixed) + +T017 had been marked `[-]` deferred on the assumption that no `llama-server` +was reachable in the dev environment. That assumption was never actually +checked — `llama-server` was installed via Homebrew the whole time, and a +router-mode server was launched against a real local GGUF model +(`--models-dir` pointed at a symlinked model file) to run the smoke test for +real. + +This caught a genuine gap the mocked suite couldn't: `contracts/llamacpp_provider_conformance.md` +claimed router mode's `/models/load`/`/models/unload` return `{"success": true}` +on an already-loaded/unloaded model, satisfying the `LLMProvider` protocol's +idempotency requirement "without extra handling." That claim was never +live-verified — live testing showed the opposite: both endpoints return HTTP +400 (`"model is already running"` / `"model is not running"`) instead. +`LlamaCppProvider.load_model()`/`unload_model()` now absorb exactly those two +error messages as the desired end-state already reached (any other error +still propagates); the contract doc is corrected to describe the real +behavior. Regression tests added (mocked, so they run in CI): +`test_load_model_already_running_is_idempotent`, +`test_unload_model_not_running_is_idempotent`, and their +`_other_http_error_still_raises` counterparts confirming non-idempotency +errors aren't over-broadly swallowed. + +Also observed live (informational, no code change needed): `load_model()`/ +`unload_model()` return once the request is *accepted*, not once the state +transition completes — a 5.6GB model reported `LOADING` for several seconds +before `LOADED`. This matches `ModelStatus`'s documented vocabulary (`loading` +is a real, intended state) and how a real UI would behave — fire the request, +poll `list_models()` for the transition. No protocol change; noted here so +it's not mistaken for a future bug report. + +**`chat_structured()` live-verified separately** (T007/T008's mocked coverage +only ever exercised the call-shape, never the real network path): ran a +second live smoke test — `load_model()` → `chat_structured()` with a real +`pydantic.BaseModel` schema — against the same router-mode server. Result +validated correctly (`Color(name='Red', hex_code='#FF0000')`), confirming +pydantic-ai's `Agent`/`OpenAIProvider(base_url=f"{host}/v1")` mechanism +genuinely works against llama-server's OpenAI-compatible endpoint, not just +Ollama's (which was the only one live-verified in the prerequisite epic). +No gap found here — recorded as verification evidence, not a fix. + +**Net result**: every `LlamaCppProvider` method (`list_models`, `load_model`, +`unload_model`, `chat`, `chat_structured`) has now been exercised against a +real router-mode `llama-server`, not just mocks. T017 is genuinely done. diff --git a/src/comfydv/__init__.py b/src/comfydv/__init__.py index c9a2813..0e31235 100644 --- a/src/comfydv/__init__.py +++ b/src/comfydv/__init__.py @@ -2,6 +2,7 @@ import logging from .circuit_breaker import CircuitBreaker from .format_string import FormatString +from .llamacpp import LlamaCppClient from .ollama import ( ChatCompletion, LLMLoadModel, @@ -34,6 +35,7 @@ NODE_CLASS_MAPPINGS = { # LLM nodes (generic, ADR-007) — see comfydv.ollama.MIGRATION_MAP for # the pre-cutover Ollama-specific names these replace "OllamaClient": OllamaClient, + "LlamaCppClient": LlamaCppClient, "LLMModelSelector": LLMModelSelector, "LLMLoadModel": LLMLoadModel, "LLMUnloadModel": LLMUnloadModel, @@ -59,6 +61,7 @@ NODE_DISPLAY_NAME_MAPPINGS = { "FormatString": "Format String (Python f-strings)", # LLM nodes (generic, ADR-007) "OllamaClient": "Ollama Client", + "LlamaCppClient": "LlamaCpp Client", "LLMModelSelector": "LLM Model Selector", "LLMLoadModel": "LLM Load Model", "LLMUnloadModel": "LLM Unload Model", diff --git a/src/comfydv/_llm/chat.py b/src/comfydv/_llm/chat.py index 7247476..367f00c 100644 --- a/src/comfydv/_llm/chat.py +++ b/src/comfydv/_llm/chat.py @@ -29,7 +29,7 @@ from pydantic_ai.models.openai import OpenAIChatModel from pydantic_ai.providers.openai import OpenAIProvider from pydantic_ai.settings import ModelSettings -from comfydv._llm.provider import Message +from .provider import Message _STRUCTURED_OUTPUT_FAILURE_EXCEPTIONS = ( UnexpectedModelBehavior, diff --git a/src/comfydv/_llm/llamacpp_provider.py b/src/comfydv/_llm/llamacpp_provider.py new file mode 100644 index 0000000..ee90594 --- /dev/null +++ b/src/comfydv/_llm/llamacpp_provider.py @@ -0,0 +1,234 @@ +"""LlamaCppProvider — LLMProvider implementation backed by llama-server's +router mode. + +Mirrors comfydv._llm.ollama_provider's structure exactly (ADR-007's parallel- +implementation pattern). Router-mode API shape verified live against +ggml-org/llama.cpp's tools/server/README.md (postdates training data) — see +specs/008-llamacpp-integration/research.md. Two details differ from Ollama: +the model identifier field is "id" (not "name"), and "status" is a nested +object ({"value": "..."}), not a flat string. + +Deployment prerequisite: llama-server must be launched with --models-dir or +--models-preset (router mode) — the endpoints this provider calls don't +exist otherwise (spec.md FR-006). +""" + +import logging + +from pydantic import BaseModel + +from .ollama_provider import _TTLLRUCache, _cache_key, _get_json, _post_json +from .provider import Message, ModelInfo, ModelStatus + +logger = logging.getLogger(__name__) + +# Own cache pool, not shared with OllamaProvider's — see plan.md's Structure +# Decision (parallel, symmetric, independent implementations). ChatCompletion +# is OUTPUT_NODE=True and re-executes every queue run regardless of which +# provider is wired in, so caching parity matters for llama.cpp too, not +# just Ollama. +_MODEL_LIST_CACHE = _TTLLRUCache(maxsize=32, ttl_seconds=20.0) +_CHAT_RESPONSE_CACHE = _TTLLRUCache(maxsize=64, ttl_seconds=None) + + +async def _fetch_models(host: str, headers: dict | None = None) -> list[str]: + """Name-only view for ComfyUI's combo-widget population (the JS refresh + button and node-creation auto-populate) — mirrors + ollama_provider._fetch_models's narrower, gracefully-degrading contract. + + Deliberately more forgiving than LlamaCppProvider.list_models(): that + method raises on a non-router-mode server (FR-006, for real workflow + execution, where a silent empty result would be misleading). This + combo-population use case wants the same quiet "just show an empty + dropdown" degradation Ollama's nodes already give for *any* failure — + consistent UX across backends for this specific, lower-stakes path. + """ + try: + models = await LlamaCppProvider(host, headers).list_models() + except Exception as exc: + logger.warning("Could not fetch llama.cpp models from %s: %s", host, exc) + return [] + return [m.name for m in models] + + +class LlamaCppProvider: + """LLMProvider implementation backed by llama-server's router mode. + + Host and headers are captured once at construction — every method + reuses them, the same pattern OllamaProvider already established. + """ + + def __init__(self, host: str, headers: dict | None = None): + self.host = host + self.headers = dict(headers) if headers else None + + async def list_models(self) -> list[ModelInfo]: + """GET {host}/models — every model llama-server's router knows + about, with its live status. Unlike OllamaProvider, no + normalization is needed: llama.cpp's status vocabulary is exactly + ModelStatus's full set. + """ + cache_key = _cache_key("llamacpp_list_models", self.host, self.headers or {}) + cached, hit = _MODEL_LIST_CACHE.get(cache_key) + if hit: + return cached + + try: + data = await _get_json(f"{self.host}/models", headers=self.headers) + except OSError as exc: + # Genuinely unreachable (connection refused, DNS failure, timed + # out — aiohttp's connection-level exceptions are all OSError + # subclasses) — degrade gracefully like OllamaProvider does, so + # a not-yet-started server just shows an empty dropdown rather + # than a hard error. + logger.warning( + "Could not fetch llama.cpp models from %s: %s", self.host, exc + ) + return [] + except RuntimeError as exc: + # The server answered but with an HTTP error status — GET + # /models only exists in router mode, so this is almost always + # a llama-server launched without --models-dir/--models-preset. + # Surfacing this distinctly (FR-006) matters: silently returning + # [] here would be indistinguishable from "no models installed". + raise RuntimeError( + f"llama-server at {self.host} did not return a model list from " + f"GET {self.host}/models — is it running in router mode " + f"(--models-dir or --models-preset)? Underlying error: {exc}" + ) from exc + + models = [] + for m in data.get("data", []): + status_value = m.get("status", {}).get("value") + try: + status = ModelStatus(status_value) + except ValueError: + logger.warning( + "llama.cpp reported an unrecognized model status %r for %r — " + "skipping status normalization, this model will be omitted", + status_value, + m.get("id"), + ) + continue + models.append(ModelInfo(name=m["id"], status=status, size=None)) + if models: + _MODEL_LIST_CACHE.set(cache_key, models) + return models + + async def load_model(self, model: str) -> None: + if not model.strip(): + raise ValueError("model name cannot be empty") + try: + await _post_json( + f"{self.host}/models/load", + {"model": model}, + headers=self.headers, + ) + except RuntimeError as exc: + # Confirmed live: router mode's own /models/load is NOT + # idempotent — it 400s "model is already running" rather than + # the {"success": true} the contract assumed. The LLMProvider + # protocol requires load_model() to be idempotent, so this + # error is the desired end-state, not a failure — absorb it + # here rather than leaking the wire-level quirk to callers. + if "model is already running" not in str(exc): + raise + + async def unload_model(self, model: str) -> None: + if not model.strip(): + raise ValueError("model name cannot be empty") + try: + await _post_json( + f"{self.host}/models/unload", + {"model": model}, + headers=self.headers, + ) + except RuntimeError as exc: + # Mirror of load_model()'s non-idempotency above, confirmed live: + # /models/unload 400s "model is not running" on an already- + # unloaded model instead of {"success": true}. + if "model is not running" not in str(exc): + raise + + async def chat( + self, + model: str, + messages: list[Message], + options: dict | None = None, + timeout_secs: float = 300.0, + ) -> str: + payload_messages = [m.model_dump() for m in messages] + payload: dict = {"model": model, "messages": payload_messages, "stream": False} + if options: + # Passed through verbatim, same nesting OllamaProvider.chat() uses + # (payload["options"] = options) — the OllamaOption* nodes emit + # Ollama-native parameter names (num_predict, repeat_penalty, + # ...), which llama-server's OpenAI-compatible endpoint won't + # recognize either way; translating them is out of scope for + # this epic (plan.md Non-goals — no changes to the generic + # nodes). This keeps the two providers' handling consistent + # rather than silently special-casing one of them. + payload["options"] = options + + cache_key = _cache_key( + "llamacpp_chat", + self.host, + self.headers or {}, + model, + payload_messages, + options or {}, + ) + cached, hit = _CHAT_RESPONSE_CACHE.get(cache_key) + if hit: + return cached + + result = await _post_json( + f"{self.host}/v1/chat/completions", + payload, + timeout=timeout_secs, + headers=self.headers, + ) + choices = result.get("choices") or [] + response_text = ( + choices[0].get("message", {}).get("content", "") or "" if choices else "" + ) + _CHAT_RESPONSE_CACHE.set(cache_key, response_text) + return response_text + + async def chat_structured( + self, + model: str, + messages: list[Message], + schema: type[BaseModel], + options: dict | None = None, + timeout_secs: float = 300.0, + max_retries: int = 2, + ) -> BaseModel: + from .chat import chat_structured as _chat_structured_impl + + payload_messages = [m.model_dump() for m in messages] + cache_key = _cache_key( + "llamacpp_chat_structured", + self.host, + self.headers or {}, + model, + payload_messages, + options or {}, + schema.model_json_schema(), + ) + cached, hit = _CHAT_RESPONSE_CACHE.get(cache_key) + if hit: + return schema.model_validate(cached) + + result = await _chat_structured_impl( + base_url=f"{self.host}/v1", + model=model, + messages=messages, + schema=schema, + headers=self.headers, + options=options, + max_retries=max_retries, + timeout_secs=timeout_secs, + ) + _CHAT_RESPONSE_CACHE.set(cache_key, result.model_dump()) + return result diff --git a/src/comfydv/_llm/ollama_provider.py b/src/comfydv/_llm/ollama_provider.py index a285f75..205d052 100644 --- a/src/comfydv/_llm/ollama_provider.py +++ b/src/comfydv/_llm/ollama_provider.py @@ -18,7 +18,7 @@ import time from pydantic import BaseModel -from comfydv._llm.provider import Message, ModelInfo, ModelStatus +from .provider import Message, ModelInfo, ModelStatus logger = logging.getLogger(__name__) @@ -132,7 +132,14 @@ async def _post_json( async def _get_json( url: str, *, timeout: float = 5.0, headers: dict | None = None ) -> dict: - """GET url, return parsed response dict. Raises on connection/HTTP error.""" + """GET url, return parsed response dict. + + Raises RuntimeError on an HTTP error status (distinct message, so callers + can tell "server responded with an error" from "couldn't reach it at + all" — aiohttp connection/timeout errors propagate unwrapped for that + reason). Message is generic, not backend-branded: this helper is shared + by every LLMProvider implementation. + """ import aiohttp async with aiohttp.ClientSession() as session: @@ -141,6 +148,11 @@ async def _get_json( headers=headers or None, timeout=aiohttp.ClientTimeout(total=timeout), ) as resp: + if resp.status >= 400: + body = await resp.text() + raise RuntimeError( + f"Server returned HTTP {resp.status} for {url}: {body[:300]}" + ) return await resp.json() @@ -284,7 +296,7 @@ class OllamaProvider: timeout_secs: float = 300.0, max_retries: int = 2, ) -> BaseModel: - from comfydv._llm.chat import chat_structured as _chat_structured_impl + from .chat import chat_structured as _chat_structured_impl payload_messages = [m.model_dump() for m in messages] cache_key = _cache_key( diff --git a/src/comfydv/llamacpp.py b/src/comfydv/llamacpp.py new file mode 100644 index 0000000..687190b --- /dev/null +++ b/src/comfydv/llamacpp.py @@ -0,0 +1,34 @@ +"""llama.cpp connection node for ComfyUI. + +Mirrors comfydv.ollama's OllamaClient exactly (ADR-007's parallel- +implementation pattern) — LlamaCppClient is the only new node this feature +introduces. Every other generic node (ChatCompletion, LLMModelSelector, +LLMLoadModel, LLMUnloadModel) already works with any LLM_CLIENT-typed +provider unchanged. + +Deployment prerequisite: llama-server must be launched in router mode +(--models-dir or --models-preset) — see specs/008-llamacpp-integration/quickstart.md. +""" + +from ._llm.llamacpp_provider import LlamaCppProvider + + +class LlamaCppClient: + @classmethod + def INPUT_TYPES(s): + return { + "required": { + "host": ("STRING", {"default": "http://localhost:8080"}), + }, + "optional": { + "headers": ("OLLAMA_HEADERS",), + }, + } + + RETURN_TYPES = ("LLM_CLIENT",) + RETURN_NAMES = ("client",) + FUNCTION = "create_client" + CATEGORY = "dv/llamacpp" + + def create_client(self, host: str, headers: dict | None = None): + return (LlamaCppProvider(host, headers),) diff --git a/src/comfydv/ollama.py b/src/comfydv/ollama.py index 1530ef2..3fa7008 100644 --- a/src/comfydv/ollama.py +++ b/src/comfydv/ollama.py @@ -21,8 +21,8 @@ import logging import os import sys -from comfydv._llm.ollama_provider import OllamaProvider, _fetch_models, _run_async -from comfydv._llm.provider import Message +from ._llm.ollama_provider import OllamaProvider, _fetch_models, _run_async +from ._llm.provider import Message logger = logging.getLogger(__name__) @@ -119,7 +119,13 @@ if "comfy" in sys.modules: from aiohttp import web host = request.rel_url.query.get("host", "http://localhost:11434") - models = await _fetch_models(host) + backend = request.rel_url.query.get("backend", "ollama") + if backend == "llamacpp": + from ._llm.llamacpp_provider import _fetch_models as _fetch + + models = await _fetch(host) + else: + models = await _fetch_models(host) if models: return web.json_response({"models": models}) return web.json_response({"error": f"No models found at {host}"}, status=503) diff --git a/src/js/ollama.js b/src/js/ollama.js index de8059c..5b5ae95 100644 --- a/src/js/ollama.js +++ b/src/js/ollama.js @@ -1,11 +1,15 @@ /** - * ollama.js — ComfyUI frontend extension for comfydv Ollama nodes. + * ollama.js — ComfyUI frontend extension for comfydv's generic LLM nodes. * - * Populates the model widget on Ollama nodes from a live call to - * GET /dv/ollama/models?host=. + * Populates the model widget on LLM nodes from a live call to + * GET /dv/ollama/models?host=&backend=. Despite the + * file/route name (kept for historical reasons — see MIGRATION_MAP in + * comfydv.ollama), this now serves both backends: which one a given node's + * upstream client is determines the `backend` param (see + * getHostAndBackendFromNode below). * - * OllamaModelSelector and OllamaLoadModel use a COMBO widget (dropdown). - * OllamaChatCompletion uses a plain STRING widget (accepts wired values). + * LLMModelSelector and LLMLoadModel use a COMBO widget (dropdown). + * ChatCompletion uses a plain STRING widget (accepts wired values). * The Refresh button works the same way for both: it fetches the live list * and sets the widget value / updates COMBO options as appropriate. */ @@ -13,12 +17,15 @@ import { app } from "../../scripts/app.js"; /** Nodes whose model widget is a COMBO dropdown. */ -const OLLAMA_COMBO_NODES = new Set(["OllamaModelSelector", "OllamaLoadModel"]); +const LLM_COMBO_NODES = new Set(["LLMModelSelector", "LLMLoadModel"]); /** Nodes whose model widget is a plain STRING (accepts wired input). */ -const OLLAMA_STRING_MODEL_NODES = new Set(["OllamaChatCompletion"]); +const LLM_STRING_MODEL_NODES = new Set(["ChatCompletion"]); -const OLLAMA_ALL_NODES = new Set([...OLLAMA_COMBO_NODES, ...OLLAMA_STRING_MODEL_NODES]); +const LLM_ALL_NODES = new Set([...LLM_COMBO_NODES, ...LLM_STRING_MODEL_NODES]); + +/** Registered client node type -> backend param the /dv/ollama/models route expects. */ +const CLIENT_NODE_BACKENDS = { OllamaClient: "ollama", LlamaCppClient: "llamacpp" }; /** * Fetch model list and update the node's model widget. @@ -27,9 +34,11 @@ const OLLAMA_ALL_NODES = new Set([...OLLAMA_COMBO_NODES, ...OLLAMA_STRING_MODEL_ * - STRING: sets the value to the first model; keeps existing value if it * still appears in the live list (user may have typed a valid name). */ -async function refreshModelWidget(node, host) { +async function refreshModelWidget(node, host, backend) { try { - const resp = await fetch(`/dv/ollama/models?host=${encodeURIComponent(host)}`); + const resp = await fetch( + `/dv/ollama/models?host=${encodeURIComponent(host)}&backend=${encodeURIComponent(backend)}` + ); if (!resp.ok) return; const data = await resp.json(); const models = data.models ?? []; @@ -53,54 +62,53 @@ async function refreshModelWidget(node, host) { node.setDirtyCanvas(true, false); } catch (_) { - // Ollama unreachable — leave widget unchanged + // Server unreachable — leave widget unchanged } } /** - * Locate the host string for a node. + * Locate the host and backend for a node's connected LLM client. * - * First checks the node's own widgets (OllamaClient has a "host" widget). - * Otherwise traverses graph links to find a connected OllamaClient node and - * reads its "host" widget — this is the common case for downstream nodes. + * Traverses graph links to find a connected OllamaClient or LlamaCppClient + * node and reads its "host" widget. Falls back to Ollama's default if + * nothing is wired yet, matching the pre-existing fallback behavior. */ -function getHostFromNode(node) { - const ownHostWidget = node.widgets?.find(w => w.name === "host"); - if (ownHostWidget) return ownHostWidget.value; - +function getHostAndBackendFromNode(node) { for (const input of node.inputs ?? []) { if (!input.link) continue; const link = node.graph?.links[input.link]; if (!link) continue; const sourceNode = node.graph?.getNodeById(link.origin_id); - if (sourceNode?.type === "OllamaClient") { + const backend = sourceNode ? CLIENT_NODE_BACKENDS[sourceNode.type] : undefined; + if (backend) { const hostWidget = sourceNode.widgets?.find(w => w.name === "host"); - if (hostWidget?.value) return hostWidget.value; + if (hostWidget?.value) return { host: hostWidget.value, backend }; } } - return "http://localhost:11434"; + return { host: "http://localhost:11434", backend: "ollama" }; } app.registerExtension({ name: "comfydv.ollama", async beforeRegisterNodeDef(nodeType, nodeData) { - if (!OLLAMA_ALL_NODES.has(nodeData.name)) return; + if (!LLM_ALL_NODES.has(nodeData.name)) return; const onNodeCreated = nodeType.prototype.onNodeCreated; nodeType.prototype.onNodeCreated = function () { const result = onNodeCreated?.apply(this, arguments); + const refresh = () => { + const { host, backend } = getHostAndBackendFromNode(this); + refreshModelWidget(this, host, backend); + }; + // Add a Refresh button below the model widget - this.addWidget("button", "⟳ Refresh models", null, () => { - const host = getHostFromNode(this); - refreshModelWidget(this, host); - }); + this.addWidget("button", "⟳ Refresh models", null, refresh); // Initial population on node creation - const host = getHostFromNode(this); - refreshModelWidget(this, host); + refresh(); return result; }; @@ -108,15 +116,16 @@ app.registerExtension({ }); /** - * Live structured-output dynamic sockets for OllamaChatCompletion. + * Live structured-output dynamic sockets for ChatCompletion. * * Mirrors FormatString's live dynamic-output pattern (see format_string.js): * editing structured_output or output_schema posts to a backend route that - * recomputes OllamaChatCompletion.RETURN_TYPES/RETURN_NAMES (the same + * recomputes ChatCompletion.RETURN_TYPES/RETURN_NAMES (the same * update_outputs() path chat() itself uses at execution time) and returns * the resulting output list — applied to this node's sockets immediately, * so you see the extracted fields appear without having to run the graph - * first. + * first. Backend-agnostic: ChatCompletion is the one generic node both + * OllamaProvider and LlamaCppProvider feed. */ async function updateStructuredOutputs(node, structuredOutput, outputSchema) { try { @@ -148,7 +157,7 @@ app.registerExtension({ name: "comfydv.ollama.structuredOutput", async beforeRegisterNodeDef(nodeType, nodeData) { - if (nodeData.name !== "OllamaChatCompletion") return; + if (nodeData.name !== "ChatCompletion") return; const onNodeCreated = nodeType.prototype.onNodeCreated; nodeType.prototype.onNodeCreated = function () { diff --git a/tests/test_comfyui_import_compat.py b/tests/test_comfyui_import_compat.py new file mode 100644 index 0000000..406546f --- /dev/null +++ b/tests/test_comfyui_import_compat.py @@ -0,0 +1,148 @@ +"""Guards against the exact bug found while validating spec 008 against a +real ComfyUI dev harness (docker-compose): every `from comfydv._llm.X +import Y`-style absolute self-import inside src/comfydv/ silently broke the +*entire* plugin (every node, not just LLM ones) as soon as ComfyUI actually +loaded it. + +ComfyUI's custom_nodes loader imports the plugin via a *relative* chain — +the repo-root __init__.py does `from .src.comfydv import ...`, nesting +comfydv under whatever top-level name the folder has (never `comfydv` +itself). An absolute `from comfydv...` self-import only resolves if `src/` +has separately been placed on sys.path — which conftest.py does for every +other test file in this suite, masking the bug completely. This file +deliberately does NOT rely on that sys.path insertion: it reproduces +ComfyUI's actual nested-relative-import shape in a subprocess. + +Confirmed via git history: this predates spec 008 entirely — it was already +broken immediately after PR #17 merged (spec 007), well before llamacpp.py +existed. No test caught it because none exercised this exact loading shape +until the docker harness was run by hand. +""" + +import subprocess +import sys +import textwrap +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).parent.parent + + +@pytest.fixture(autouse=True) +def _clear_ollama_caches(): + """Shadow conftest.py's autouse fixture of the same name for this module + only. That fixture's own setup does `from comfydv._llm.ollama_provider + import ...` — this file's tests are the exact reproduction of an + environment where `comfydv` resolving correctly can't be assumed (that's + the point of the file), so depending on it for an unrelated cache-reset + would make these tests order-dependent on whichever other test file + happens to import `comfydv` "the normal way" first in the session. These + tests touch no OllamaProvider/ChatCompletion state, so there is nothing + to reset.""" + yield + + +_SUBPROCESS_SCRIPT = textwrap.dedent( + """ + import sys + import types + + # Minimal ComfyUI stubs — same shape as conftest.py's pytest_configure, + # but this script intentionally runs outside pytest so it isn't reusing + # (or accidentally validated by) that fixture's sys.path setup. + class _InterruptProcessingException(Exception): + pass + + comfy_module = types.ModuleType("comfy") + comfy_module.model_management = types.SimpleNamespace( + InterruptProcessingException=_InterruptProcessingException + ) + sys.modules["comfy"] = comfy_module + sys.modules["comfy.model_management"] = comfy_module.model_management + + class _Routes: + def post(self, path): + return lambda fn: fn + + def get(self, path): + return lambda fn: fn + + class _PromptServer: + pass + + _PromptServer.instance = _PromptServer() + _PromptServer.instance.routes = _Routes() + server_module = types.ModuleType("server") + server_module.PromptServer = _PromptServer + sys.modules["server"] = server_module + + folder_paths_module = types.ModuleType("folder_paths") + folder_paths_module.get_output_directory = lambda: "/tmp/comfydv_test" + sys.modules["folder_paths"] = folder_paths_module + + # The critical part: put the repo's *parent* directory on sys.path, so + # `import comfydv` resolves to the repo-root __init__.py — exactly how + # ComfyUI resolves a folder under custom_nodes/ — NOT to src/comfydv + # directly (that's what conftest.py's sys.path.insert(0, ".../src") + # does for the rest of this test suite, and why it never caught this). + sys.path.insert(0, sys.argv[1]) + + import comfydv + + required = {"FormatString", "RandomChoice", "CircuitBreaker", + "OllamaClient", "LlamaCppClient", "ChatCompletion"} + missing = required - set(comfydv.NODE_CLASS_MAPPINGS) + if missing: + print(f"MISSING_NODES:{missing}") + sys.exit(1) + print("OK") + """ +) + + +def test_package_imports_under_comfyui_style_relative_nesting(): + """Reproduces ComfyUI's real loading shape and fails loudly — with the + actual traceback — if any internal module reverts to an absolute + `from comfydv...` self-import.""" + result = subprocess.run( + [sys.executable, "-c", _SUBPROCESS_SCRIPT, str(REPO_ROOT.parent)], + cwd=REPO_ROOT, + capture_output=True, + text=True, + timeout=30, + ) + assert result.returncode == 0, ( + "comfydv failed to import the way ComfyUI actually loads it " + "(relative nesting, not a top-level `comfydv` on sys.path). " + f"This means every node in the plugin would fail to register.\n" + f"--- stdout ---\n{result.stdout}\n--- stderr ---\n{result.stderr}" + ) + assert "OK" in result.stdout + + +def test_no_absolute_self_imports_in_package(): + """Cheap, fast static guard alongside the dynamic test above: no file + under src/comfydv/ should import itself as `comfydv.X` — internal + imports must be relative (`.X` / `..X`) so they resolve regardless of + what the outer package happens to be named at load time.""" + import ast + + offenders = [] + for path in (REPO_ROOT / "src" / "comfydv").rglob("*.py"): + tree = ast.parse(path.read_text(), filename=str(path)) + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom): + if node.module and ( + node.module == "comfydv" or node.module.startswith("comfydv.") + ): + offenders.append(f"{path.relative_to(REPO_ROOT)}:{node.lineno}") + elif isinstance(node, ast.Import): + for alias in node.names: + if alias.name == "comfydv" or alias.name.startswith("comfydv."): + offenders.append(f"{path.relative_to(REPO_ROOT)}:{node.lineno}") + + assert not offenders, ( + "Absolute self-imports found — use relative imports instead " + f"(they break under ComfyUI's actual loader): {offenders}" + ) diff --git a/tests/test_llamacpp.py b/tests/test_llamacpp.py new file mode 100644 index 0000000..8761bd4 --- /dev/null +++ b/tests/test_llamacpp.py @@ -0,0 +1,137 @@ +""" +Tests for comfydv.llamacpp.LlamaCppClient — the one new ComfyUI node this +feature introduces. Also proves the adapter pattern end-to-end (US4): the +same generic nodes work unmodified against either provider. + +BDD coverage: + ../specs/008-llamacpp-integration/features/us1_connect_and_chat.feature + ../specs/008-llamacpp-integration/features/us4_swap_backends.feature +""" + +from comfydv._llm.llamacpp_provider import LlamaCppProvider +from comfydv._llm.ollama_provider import OllamaProvider +from comfydv.llamacpp import LlamaCppClient +from comfydv.ollama import ( + ChatCompletion, + LLMLoadModel, + LLMModelSelector, + LLMUnloadModel, +) + + +class _FakeProvider: + """Mirrors tests/test_ollama.py's _FakeProvider — reused here for US4's + swap-backends proof rather than duplicated, since the whole point is + that node behavior doesn't depend on which concrete provider it gets.""" + + def __init__(self, chat_response="ok"): + self.chat_response = chat_response + self.models = [{"name": "m", "status": "loaded"}] + self.calls: list[tuple] = [] + + async def list_models(self): + self.calls.append(("list_models",)) + return self.models + + async def load_model(self, model): + self.calls.append(("load_model", model)) + + async def unload_model(self, model): + self.calls.append(("unload_model", model)) + + async def chat(self, model, messages, options=None, timeout_secs=300.0): + self.calls.append(("chat", model)) + return self.chat_response + + +def test_client_outputs_llamacpp_provider(): + (client,) = LlamaCppClient().create_client("http://localhost:8080") + assert isinstance(client, LlamaCppProvider) + assert client.host == "http://localhost:8080" + + +def test_client_default_host_matches_llama_server_default_port(): + input_types = LlamaCppClient.INPUT_TYPES() + assert input_types["required"]["host"][1]["default"] == "http://localhost:8080" + + +def test_client_output_type_is_generic_llm_client(): + assert LlamaCppClient.RETURN_TYPES == ("LLM_CLIENT",) + + +def test_client_carries_headers(): + (client,) = LlamaCppClient().create_client( + "http://localhost:8080", headers={"Authorization": "Bearer abc"} + ) + assert client.headers == {"Authorization": "Bearer abc"} + + +def test_node_contract(): + assert hasattr(LlamaCppClient, "INPUT_TYPES") + assert hasattr(LlamaCppClient, "RETURN_TYPES") + assert hasattr(LlamaCppClient, "FUNCTION") + assert hasattr(LlamaCppClient, "CATEGORY") + assert hasattr(LlamaCppClient, LlamaCppClient.FUNCTION) + + +def test_registered_in_node_class_mappings(): + from comfydv import NODE_CLASS_MAPPINGS, NODE_DISPLAY_NAME_MAPPINGS + + assert NODE_CLASS_MAPPINGS["LlamaCppClient"] is LlamaCppClient + assert "LlamaCppClient" in NODE_DISPLAY_NAME_MAPPINGS + + +# --------------------------------------------------------------------------- +# US4 — swap backends without touching downstream nodes +# --------------------------------------------------------------------------- + + +def _run_workflow(client) -> None: + """The same node sequence a workflow author would wire up, regardless + of which provider `client` is.""" + ChatCompletion().chat(client=client, model="m", prompt="hi") + LLMModelSelector().select_model(client=client, model="m") + LLMLoadModel().load_model(client=client, model="m") + LLMUnloadModel().unload_model(client=client, model="m") + + +def test_same_workflow_runs_against_either_fake_provider(): + """No node branches on provider type — the same call sequence succeeds + whether client looks like an Ollama-shaped or llama.cpp-shaped provider.""" + ollama_like = _FakeProvider(chat_response="ollama says hi") + llamacpp_like = _FakeProvider(chat_response="llamacpp says hi") + + # Neither call raises — that's the actual assertion. If ChatCompletion/ + # LLMModelSelector/LLMLoadModel/LLMUnloadModel secretly special-cased a + # concrete provider type (isinstance checks, attribute probing beyond + # the protocol), one of these would fail. + _run_workflow(ollama_like) + _run_workflow(llamacpp_like) + + # LLMModelSelector is pure passthrough (client is accepted only for + # wiring/typing, never dereferenced), so it makes no provider call. + expected = ["chat", "load_model", "unload_model"] + assert [c[0] for c in ollama_like.calls] == expected + assert [c[0] for c in llamacpp_like.calls] == expected + + +def test_real_providers_are_interchangeable_client_output(): + """OllamaClient and LlamaCppClient both emit LLM_CLIENT — a workflow + author can wire either one into the same downstream nodes.""" + from comfydv.ollama import OllamaClient + + (ollama_client,) = OllamaClient().create_client("http://localhost:11434") + (llamacpp_client,) = LlamaCppClient().create_client("http://localhost:8080") + + assert isinstance(ollama_client, OllamaProvider) + assert isinstance(llamacpp_client, LlamaCppProvider) + # Both satisfy the same protocol shape — same method names available. + for method in ( + "list_models", + "load_model", + "unload_model", + "chat", + "chat_structured", + ): + assert callable(getattr(ollama_client, method)) + assert callable(getattr(llamacpp_client, method)) diff --git a/tests/test_llamacpp_provider.py b/tests/test_llamacpp_provider.py new file mode 100644 index 0000000..3030d04 --- /dev/null +++ b/tests/test_llamacpp_provider.py @@ -0,0 +1,404 @@ +""" +Tests for comfydv._llm.llamacpp_provider.LlamaCppProvider — mirrors +test_ollama_provider.py's structure exactly (ADR-007's parallel- +implementation pattern). Mocks at the provider's own _post_json/_get_json +seam. + +BDD coverage: + ../specs/008-llamacpp-integration/features/us1_connect_and_chat.feature + ../specs/008-llamacpp-integration/features/us2_structured_output.feature + ../specs/008-llamacpp-integration/features/us3_model_lifecycle.feature +""" + +import pytest + +import comfydv._llm.llamacpp_provider as provider_mod +from comfydv._llm.llamacpp_provider import LlamaCppProvider, _fetch_models +from comfydv._llm.ollama_provider import _run_async +from comfydv._llm.provider import Message, ModelStatus + + +@pytest.fixture(autouse=True) +def _clear_provider_caches(): + provider_mod._MODEL_LIST_CACHE.clear() + provider_mod._CHAT_RESPONSE_CACHE.clear() + yield + provider_mod._MODEL_LIST_CACHE.clear() + provider_mod._CHAT_RESPONSE_CACHE.clear() + + +# --------------------------------------------------------------------------- +# list_models — the "id" field name and nested "status.value" are the two +# details research.md flagged as easy to get wrong by assumption. +# --------------------------------------------------------------------------- + + +def test_list_models_maps_id_field_to_name(monkeypatch): + async def fake_get(url, *, timeout=5.0, headers=None): + return {"data": [{"id": "gemma-3-4b:Q4_K_M", "status": {"value": "loaded"}}]} + + monkeypatch.setattr(provider_mod, "_get_json", fake_get) + (model,) = _run_async(LlamaCppProvider("http://localhost:8080").list_models()) + + assert model.name == "gemma-3-4b:Q4_K_M" + + +def test_list_models_reads_nested_status_value(monkeypatch): + async def fake_get(url, *, timeout=5.0, headers=None): + return { + "data": [ + {"id": "a", "status": {"value": "sleeping"}}, + {"id": "b", "status": {"value": "downloading", "progress": {}}}, + ] + } + + monkeypatch.setattr(provider_mod, "_get_json", fake_get) + models = _run_async(LlamaCppProvider("http://localhost:8080").list_models()) + + by_name = {m.name: m for m in models} + assert by_name["a"].status == ModelStatus.SLEEPING + assert by_name["b"].status == ModelStatus.DOWNLOADING + + +def test_list_models_no_normalization_needed_full_vocabulary(monkeypatch): + """Unlike OllamaProvider, llama.cpp's status vocabulary is exactly + ModelStatus's full set — every value should pass through untouched.""" + + async def fake_get(url, *, timeout=5.0, headers=None): + return { + "data": [ + {"id": v, "status": {"value": v}} + for v in ["unloaded", "loading", "loaded", "sleeping", "downloading"] + ] + } + + monkeypatch.setattr(provider_mod, "_get_json", fake_get) + models = _run_async(LlamaCppProvider("http://localhost:8080").list_models()) + + assert {m.status for m in models} == set(ModelStatus) + + +def test_list_models_skips_unrecognized_status(monkeypatch): + async def fake_get(url, *, timeout=5.0, headers=None): + return { + "data": [ + {"id": "crashed", "status": {"value": "failed", "exit_code": 1}}, + {"id": "ok", "status": {"value": "loaded"}}, + ] + } + + monkeypatch.setattr(provider_mod, "_get_json", fake_get) + models = _run_async(LlamaCppProvider("http://localhost:8080").list_models()) + + assert [m.name for m in models] == ["ok"] + + +def test_list_models_unreachable_returns_empty(monkeypatch): + async def fake_get(url, *, timeout=5.0, headers=None): + raise ConnectionError("no route to host") + + monkeypatch.setattr(provider_mod, "_get_json", fake_get) + models = _run_async(LlamaCppProvider("http://localhost:19999").list_models()) + + assert models == [] + + +def test_list_models_non_router_mode_raises_clear_error(monkeypatch): + """FR-006: a llama-server that IS reachable but wasn't launched with + --models-dir/--models-preset answers GET /models with an HTTP error + (the endpoint doesn't exist outside router mode). That must surface as + a specific, actionable error — not silently degrade to an empty list, + which would be indistinguishable from "server has no models".""" + + async def fake_get(url, *, timeout=5.0, headers=None): + raise RuntimeError("Server returned HTTP 404 for http://x/models: not found") + + monkeypatch.setattr(provider_mod, "_get_json", fake_get) + + with pytest.raises(RuntimeError, match="router mode"): + _run_async(LlamaCppProvider("http://localhost:8080").list_models()) + + +def test_list_models_cached_second_call(monkeypatch): + calls = {"n": 0} + + async def fake_get(url, *, timeout=5.0, headers=None): + calls["n"] += 1 + return {"data": [{"id": "a", "status": {"value": "loaded"}}]} + + monkeypatch.setattr(provider_mod, "_get_json", fake_get) + provider = LlamaCppProvider("http://localhost:8080") + _run_async(provider.list_models()) + _run_async(provider.list_models()) + + assert calls["n"] == 1 + + +# --------------------------------------------------------------------------- +# load_model / unload_model +# --------------------------------------------------------------------------- + + +def test_load_model_posts_to_models_load_with_model_field(monkeypatch): + captured = {} + + async def fake_post(url, payload, *, timeout=120.0, headers=None): + captured["url"] = url + captured["payload"] = payload + return {"success": True} + + monkeypatch.setattr(provider_mod, "_post_json", fake_post) + _run_async(LlamaCppProvider("http://localhost:8080").load_model("gemma-3-4b")) + + assert captured["url"] == "http://localhost:8080/models/load" + assert captured["payload"] == {"model": "gemma-3-4b"} + + +def test_unload_model_posts_to_models_unload_with_model_field(monkeypatch): + captured = {} + + async def fake_post(url, payload, *, timeout=120.0, headers=None): + captured["url"] = url + captured["payload"] = payload + return {"success": True} + + monkeypatch.setattr(provider_mod, "_post_json", fake_post) + _run_async(LlamaCppProvider("http://localhost:8080").unload_model("gemma-3-4b")) + + assert captured["url"] == "http://localhost:8080/models/unload" + assert captured["payload"] == {"model": "gemma-3-4b"} + + +def test_load_model_already_running_is_idempotent(monkeypatch): + """Confirmed live: router mode's /models/load is NOT idempotent at the + wire level — it 400s "model is already running" rather than returning + {"success": true}. The LLMProvider protocol requires load_model() to be + idempotent, so LlamaCppProvider must absorb this itself.""" + calls = [] + + async def fake_post(url, payload, *, timeout=120.0, headers=None): + calls.append((url, payload)) + raise RuntimeError( + "Server returned HTTP 400 for " + f'{url}: {{"error":{{"code":400,"message":"model is already ' + 'running","type":"invalid_request_error"}}}}' + ) + + monkeypatch.setattr(provider_mod, "_post_json", fake_post) + _run_async(LlamaCppProvider("http://localhost:8080").load_model("gemma-3-4b")) + + # The absence of a raised exception is only meaningful if the request + # actually happened and hit the "already running" branch — assert that + # directly rather than trusting silence alone. + assert calls == [("http://localhost:8080/models/load", {"model": "gemma-3-4b"})] + + +def test_load_model_other_http_error_still_raises(monkeypatch): + async def fake_post(url, payload, *, timeout=120.0, headers=None): + raise RuntimeError(f"Server returned HTTP 500 for {url}: internal error") + + monkeypatch.setattr(provider_mod, "_post_json", fake_post) + with pytest.raises(RuntimeError, match="500"): + _run_async(LlamaCppProvider("http://localhost:8080").load_model("gemma-3-4b")) + + +def test_unload_model_not_running_is_idempotent(monkeypatch): + """Mirror of the load_model case, confirmed live: /models/unload 400s + "model is not running" on an already-unloaded model.""" + calls = [] + + async def fake_post(url, payload, *, timeout=120.0, headers=None): + calls.append((url, payload)) + raise RuntimeError( + "Server returned HTTP 400 for " + f'{url}: {{"error":{{"code":400,"message":"model is not ' + 'running","type":"invalid_request_error"}}}}' + ) + + monkeypatch.setattr(provider_mod, "_post_json", fake_post) + _run_async(LlamaCppProvider("http://localhost:8080").unload_model("gemma-3-4b")) + + assert calls == [("http://localhost:8080/models/unload", {"model": "gemma-3-4b"})] + + +def test_unload_model_other_http_error_still_raises(monkeypatch): + async def fake_post(url, payload, *, timeout=120.0, headers=None): + raise RuntimeError(f"Server returned HTTP 500 for {url}: internal error") + + monkeypatch.setattr(provider_mod, "_post_json", fake_post) + with pytest.raises(RuntimeError, match="500"): + _run_async(LlamaCppProvider("http://localhost:8080").unload_model("gemma-3-4b")) + + +def test_load_model_empty_raises_before_network(monkeypatch): + def fail_post(*a, **k): + raise AssertionError("must not call _post_json for an empty model name") + + monkeypatch.setattr(provider_mod, "_post_json", fail_post) + with pytest.raises(ValueError, match="cannot be empty"): + _run_async(LlamaCppProvider("http://localhost:8080").load_model("")) + + +def test_unload_model_empty_raises_before_network(monkeypatch): + def fail_post(*a, **k): + raise AssertionError("must not call _post_json for an empty model name") + + monkeypatch.setattr(provider_mod, "_post_json", fail_post) + with pytest.raises(ValueError, match="cannot be empty"): + _run_async(LlamaCppProvider("http://localhost:8080").unload_model(" ")) + + +# --------------------------------------------------------------------------- +# chat — OpenAI response shape (choices[0].message.content), not Ollama's +# native shape +# --------------------------------------------------------------------------- + + +def test_chat_posts_to_v1_chat_completions_and_parses_openai_shape(monkeypatch): + async def fake_post(url, payload, *, timeout=120.0, headers=None): + assert url == "http://localhost:8080/v1/chat/completions" + return {"choices": [{"message": {"role": "assistant", "content": "hello"}}]} + + monkeypatch.setattr(provider_mod, "_post_json", fake_post) + result = _run_async( + LlamaCppProvider("http://localhost:8080").chat( + "gemma-3-4b", [Message(role="user", content="hi")] + ) + ) + + assert result == "hello" + + +def test_chat_no_choices_returns_empty_string(monkeypatch): + async def fake_post(url, payload, *, timeout=120.0, headers=None): + return {"choices": []} + + monkeypatch.setattr(provider_mod, "_post_json", fake_post) + result = _run_async( + LlamaCppProvider("http://localhost:8080").chat( + "gemma-3-4b", [Message(role="user", content="hi")] + ) + ) + + assert result == "" + + +def test_chat_second_identical_call_is_cached(monkeypatch): + calls = {"n": 0} + + async def fake_post(url, payload, *, timeout=120.0, headers=None): + calls["n"] += 1 + return {"choices": [{"message": {"content": "cached"}}]} + + monkeypatch.setattr(provider_mod, "_post_json", fake_post) + provider = LlamaCppProvider("http://localhost:8080") + messages = [Message(role="user", content="hi")] + + r1 = _run_async(provider.chat("m", messages)) + r2 = _run_async(provider.chat("m", messages)) + + assert r1 == r2 == "cached" + assert calls["n"] == 1 + + +# --------------------------------------------------------------------------- +# chat_structured — zero new logic, delegates to the shared helper unchanged +# --------------------------------------------------------------------------- + + +def test_chat_structured_builds_v1_base_url_and_delegates(monkeypatch): + from pydantic import BaseModel + + class Widget(BaseModel): + name: str + + captured = {} + + async def fake_chat_structured(**kwargs): + captured.update(kwargs) + return Widget(name="x") + + monkeypatch.setattr("comfydv._llm.chat.chat_structured", fake_chat_structured) + + result = _run_async( + LlamaCppProvider("http://localhost:8080").chat_structured( + "gemma-3-4b", [Message(role="user", content="hi")], Widget + ) + ) + + assert result == Widget(name="x") + assert captured["base_url"] == "http://localhost:8080/v1" + assert captured["model"] == "gemma-3-4b" + + +def test_chat_structured_forwards_options(monkeypatch): + from pydantic import BaseModel + + class Widget(BaseModel): + name: str + + captured = {} + + async def fake_chat_structured(**kwargs): + captured.update(kwargs) + return Widget(name="x") + + monkeypatch.setattr("comfydv._llm.chat.chat_structured", fake_chat_structured) + + _run_async( + LlamaCppProvider("http://localhost:8080").chat_structured( + "gemma-3-4b", + [Message(role="user", content="hi")], + Widget, + options={"temperature": 0.0}, + ) + ) + + assert captured["options"] == {"temperature": 0.0} + + +# --------------------------------------------------------------------------- +# _fetch_models — name-only view used by ComfyUI's /dv/ollama/models?backend= +# llamacpp route (the JS refresh button / node-creation auto-populate). +# Deliberately more forgiving than list_models(): degrades to [] on any +# failure rather than raising on a non-router-mode server, matching the +# combo-widget UX OllamaProvider's own _fetch_models already gives. +# --------------------------------------------------------------------------- + + +def test_fetch_models_returns_name_only_list(monkeypatch): + async def fake_get(url, *, timeout=5.0, headers=None): + return { + "data": [ + {"id": "a", "status": {"value": "loaded"}}, + {"id": "b", "status": {"value": "unloaded"}}, + ] + } + + monkeypatch.setattr(provider_mod, "_get_json", fake_get) + names = _run_async(_fetch_models("http://localhost:8080")) + + assert names == ["a", "b"] + + +def test_fetch_models_degrades_to_empty_on_non_router_mode(monkeypatch): + """Unlike list_models() (FR-006), this combo-population view swallows + even the non-router-mode error — a quiet empty dropdown, not a toast.""" + + async def fake_get(url, *, timeout=5.0, headers=None): + raise RuntimeError("Server returned HTTP 404 for http://x/models: not found") + + monkeypatch.setattr(provider_mod, "_get_json", fake_get) + names = _run_async(_fetch_models("http://localhost:8080")) + + assert names == [] + + +def test_fetch_models_degrades_to_empty_when_unreachable(monkeypatch): + async def fake_get(url, *, timeout=5.0, headers=None): + raise ConnectionError("no route to host") + + monkeypatch.setattr(provider_mod, "_get_json", fake_get) + names = _run_async(_fetch_models("http://localhost:19999")) + + assert names == [] diff --git a/tests/test_ollama.py b/tests/test_ollama.py index b4a64b3..ef03e62 100644 --- a/tests/test_ollama.py +++ b/tests/test_ollama.py @@ -824,6 +824,92 @@ class _FakeRequest: return self._body +class _FakeRelUrl: + def __init__(self, query: dict): + self.query = query + + +class _FakeGetRequest: + def __init__(self, query: dict): + self.rel_url = _FakeRelUrl(query) + + +class TestModelsRoute: + """GET /dv/ollama/models — the JS refresh-button/auto-populate endpoint. + + Previously untested (no test survived the atomic cutover rewrite), which + is exactly how a real bug shipped undetected: the `backend` dispatch + added here didn't exist until a live js/ollama.js bug was found (it + matched on pre-rename node names and always spoke Ollama's wire + protocol regardless of which client was actually connected).""" + + def _call(self, query: dict): + import comfydv.ollama as ollama_mod + from comfydv._llm.ollama_provider import _run_async + + resp = _run_async(ollama_mod._models_endpoint(_FakeGetRequest(query))) + return json.loads(resp.text), resp.status + + def test_default_backend_dispatches_to_ollama(self, monkeypatch): + async def fake_fetch(host, headers=None): + assert host == "http://localhost:11434" + return ["a", "b"] + + monkeypatch.setattr("comfydv.ollama._fetch_models", fake_fetch) + data, status = self._call({"host": "http://localhost:11434"}) + + assert status == 200 + assert data == {"models": ["a", "b"]} + + def test_explicit_ollama_backend_dispatches_to_ollama(self, monkeypatch): + async def fake_fetch(host, headers=None): + return ["m"] + + monkeypatch.setattr("comfydv.ollama._fetch_models", fake_fetch) + data, status = self._call( + {"host": "http://localhost:11434", "backend": "ollama"} + ) + + assert status == 200 + assert data == {"models": ["m"]} + + def test_llamacpp_backend_dispatches_to_llamacpp_fetch(self, monkeypatch): + """The bug this regression-tests: before the fix, this endpoint + always called Ollama's _fetch_models regardless of `backend`, so a + llama.cpp host's models never populated the dropdown.""" + called = {} + + async def fake_llamacpp_fetch(host, headers=None): + called["host"] = host + return ["gemma-3-4b"] + + async def fail_ollama_fetch(host, headers=None): + raise AssertionError("must not call Ollama's fetcher for backend=llamacpp") + + monkeypatch.setattr( + "comfydv._llm.llamacpp_provider._fetch_models", fake_llamacpp_fetch + ) + monkeypatch.setattr("comfydv.ollama._fetch_models", fail_ollama_fetch) + + data, status = self._call( + {"host": "http://localhost:8080", "backend": "llamacpp"} + ) + + assert status == 200 + assert data == {"models": ["gemma-3-4b"]} + assert called["host"] == "http://localhost:8080" + + def test_no_models_returns_503(self, monkeypatch): + async def fake_fetch(host, headers=None): + return [] + + monkeypatch.setattr("comfydv.ollama._fetch_models", fake_fetch) + data, status = self._call({"host": "http://localhost:11434"}) + + assert status == 503 + assert "error" in data + + class TestUpdateStructuredOutputsRoute: def teardown_method(self): # This route mutates ChatCompletion's class-level RETURN_TYPES