6fcb9c9892
There were none. `workflow.DEFAULTS` was the only source, so 512x512, euler and twenty steps were what every instance got whatever card it was running on -- and 512 square on an SDXL checkpoint is precisely what the tool's own description warns produces duplicated limbs. The two ways round it were both bad: bake literals into a template where the placeholders should be, or write prose in the instructions box and hope. Three rungs now, most specific winning, with DEFAULTS staying underneath as the floor so an instance that sets nothing behaves exactly as it did and a floor improved in code still reaches everybody. An empty box is "no opinion" rather than zero, which matters: read as a number it would set every instance to zero steps, and ComfyUI refuses that in a way that looks like a broken model. The right control for each, because a text box is wrong for most of them. The samplers and schedulers were already being discovered by the Test button, stored, and read by nothing at all -- they are the pickers now. A stored value missing from the list is kept as an option anyway, or opening this page and pressing Save would silently clear a working setting. Checkpoints are chosen rather than typed, and the instance default is a rung of its own instead of "whatever happens to be first in a textarea somebody filled in some order". And batch, at last: `batch_size` was a literal 1 in the base template, so an administrator whose card can comfortably make four had no way of saying so. Deliberately not something a model may set -- one asking for six because it is unsure is the exact cost this must not invite. The tool's schema restates the defaults it quotes. Every "Default 20." in there was written when there was one set of defaults in the world; left alone, an instance drawing at 1024 would go on telling the model 512, and the model reasons from that sentence rather than ignoring it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
176 lines
11 KiB
Markdown
176 lines
11 KiB
Markdown
# Image generation
|
||
|
||
Split out of `CLAUDE.md` -- same document, same rules, kept here because that
|
||
file is loaded in full on every session and this part is only wanted when you
|
||
are working on drawing on a ComfyUI. Read it before you do.
|
||
|
||
Covers `services/images/` -- `comfy.py`, `workflow.py`, `tool.py` -- and
|
||
`api/admin_images.py`.
|
||
|
||
**Image generation is a ComfyUI workflow with holes in it, and the holes are the
|
||
administrator's statement.** `services/images/` is three modules: `comfy.py`
|
||
speaks HTTP, `workflow.py` fills a template, `tool.py` ties them to a chat.
|
||
Which node holds the prompt is *declared* with `{{prompt}}` rather than sniffed
|
||
by node type — looking for the first `CLIPTextEncode` works on the shipped
|
||
workflow and on nothing else, and swaps positive for negative the first time
|
||
somebody reorders them.
|
||
|
||
**Substitution walks the parsed JSON, not the text of it.** A value that is
|
||
*exactly* `"{{steps}}"` becomes the number 20; ComfyUI validates types and
|
||
refuses the string. A placeholder inside a longer string is still text, which is
|
||
what makes `"{{prompt}}, masterpiece"` work. Doing it textually would also mean
|
||
a prompt containing a quotation mark produced a document that no longer parses,
|
||
on the one input guaranteed to hold arbitrary text. `seed` has no fixed default
|
||
— one would make every unspecified generation identical and make the retry loop
|
||
redraw the same rejected picture four times. **A negative seed means random**,
|
||
because `-1` is what ComfyUI's own interface, A1111 and everything else that has
|
||
ever asked for a seed use for it, so a model that has read any of them writes
|
||
it: without that it went through the uint64 wrap and arrived as
|
||
18446744073709551615, a perfectly valid *fixed* seed, so "give me something new"
|
||
returned the same picture every time.
|
||
|
||
**One call is one finished image, and the retrying is inside the tool.**
|
||
Returning every attempt to the conversation would cost a round each, make the
|
||
ceiling advisory rather than enforced, and walk the reader past every reject. So
|
||
the reviewer — the admin's chosen vision model, else the chat's own if it has
|
||
vision, else nobody — is asked about *bytes* rather than about a row: an attempt
|
||
about to be discarded should not leave an `Attachment` behind, so it sees a
|
||
downscaled preview built in memory and only the kept image is written. Anything
|
||
that goes wrong in review is a **keep**; losing a picture because a judging
|
||
request timed out would be the check destroying the thing it was checking. The
|
||
last attempt is kept whatever the verdict, so a request always produces
|
||
something. Rejected images are not stored — their verdicts are, in `event.text`.
|
||
|
||
**`task.image_review` is a `GROUP_TASKS` fragment**, so it is editable and
|
||
excluded from the harness, exactly like `task.title` and `task.compact` — and
|
||
clearing it switches reviewing off, the same way clearing `task.compact` switches
|
||
compaction off. It is biased hard towards KEEP on purpose: a reviewer that
|
||
retries on taste spends the GPU four times and usually ends up back at the first
|
||
image.
|
||
|
||
**A failed generation is `completed: false` for ever, so waiting on that flag
|
||
hangs the reply.** ComfyUI writes its history entry in `task_done` and nowhere
|
||
else, so the entry appearing *is* "finished" — but it sets `completed=e.success`,
|
||
which means an out-of-memory, a cancelled job and a broken node all stay
|
||
incomplete permanently. The first version waited on the flag, so every failure
|
||
sat for the full 600s timeout and then reported a timeout, when ComfyUI had known
|
||
within one second and written down exactly what happened. The terminal condition
|
||
is now *a record with a status*, and `status.messages` is read for the last
|
||
`execution_error` or `execution_interrupted` in it, which carries the node and
|
||
the exception.
|
||
|
||
Two failures get their own class because they have an obvious next move.
|
||
`OutOfMemory` — matched on `exception_type`, not on the message, which is a
|
||
paragraph of allocator advice addressed to whoever runs the box — makes the tool
|
||
tell the model to retry at a named smaller size (worked out from what it actually
|
||
asked for, because "use a lower resolution" against a request that was already
|
||
512x512 is advice nobody can follow) or with a lighter checkpoint. `Interrupted`
|
||
is not a fault at all: somebody pressed stop, and the model is told not to simply
|
||
start it again. **Everything else gets the reason and no advice** — a model told
|
||
to "try again" after a broken workflow tries the identical thing, and a
|
||
suggestion invented for a failure nobody understands is a guess wearing the
|
||
application's authority.
|
||
|
||
**A tool's parameter descriptions are instructions, and terse ones are why a
|
||
model sends only the prompt.** "cfg: prompt adherence, default 8" tells a model
|
||
nothing it can act on. Measured against a 4B model on the same request: with the
|
||
terse descriptions it sent `prompt` and `template` and nothing else — meaning
|
||
512x512 defaults on an SDXL checkpoint, which is precisely the duplicated-limbs
|
||
failure the width description now warns about. With descriptions that say what
|
||
each value *does to the picture* and when to move it, the same model sent a
|
||
portrait 1024x1536 and a deliberate sampler. It costs ~3KB of schema per request
|
||
in a chat that can draw, and it is the difference between having ten parameters
|
||
and having one. `docs/image-generation-instructions.md` is the long version, to
|
||
paste into the admin instructions box for models that need more than the harness
|
||
can afford to carry.
|
||
|
||
**Preserve VRAM unloads the chat's own connection and nothing else.**
|
||
`Connection.unload_url` is a column because the memory being freed belongs to one
|
||
machine: a local llama-swap answers `GET /unload`, and a box on the network has
|
||
no reason to be unloaded when ComfyUI wants memory *here*. Empty means "cannot be
|
||
unloaded", which is the honest default — there is no call that works everywhere.
|
||
The swap goes round the *review*, not round the tool: unload, generate, free
|
||
ComfyUI, ask the reviewer (which loads the LLM again), round again if it said no.
|
||
Two model loads per retry, which is why the two settings are independent and the
|
||
page says so when both are on. **Nothing loads the LLM back at the end** — the
|
||
reply's next request does, and llama-swap loads on demand; that step exists in
|
||
the description and not in the code, which is why the code says so.
|
||
|
||
**A generated image rides on the assistant message, so `message_payload` sends
|
||
images only on `user` turns.** No assistant message had ever carried one before,
|
||
so the distinction had never been drawn — and the moment one does, the
|
||
multimodal list form on an `assistant` turn is rejected by OpenAI and most local
|
||
runners, breaking not that turn but every later one in the chat. What follows and
|
||
is worth knowing: on a *later* turn the model cannot see the picture it made
|
||
(tool results are not replayed either), so "make it bluer" regenerates rather
|
||
than edits. Honest for a text-to-image workflow with no img2img path.
|
||
|
||
**The runner writes the file; only the loop says which turn owns it.**
|
||
`event["attachment_id"]` is carried by `generation._run` exactly as
|
||
`event["canvas"]` and `event["plan"]` are, because `_persist` is the single
|
||
writer. `_bind_attachments` narrows on this chat and on rows still unbound, for
|
||
the reason `files.claim` does: the ids arrive on a dict a runner built.
|
||
|
||
**`files.store(keep_original=True)` skips the resize and the transcode, and
|
||
nothing else.** `_process_image` turns anything without alpha into JPEG q85 at
|
||
1400px, which is right for a phone photo and a visible loss on generated art.
|
||
Pillow still opens it, so a malformed file is still refused and the dimensions
|
||
are still measured rather than claimed.
|
||
|
||
**`/image` forces one tool for one round.** It sends the ordinary message with
|
||
`force_tool`, which becomes `tool_choice` — reusing the whole loop rather than
|
||
inventing a second generation path. `FORCEABLE_TOOLS` is an allow list because
|
||
this is read off a form, and `resolve_tools` still decides whether the tool
|
||
exists, so forcing one that was never offered does nothing. `payload.pop(
|
||
"tool_choice")` after the first round is load-bearing: left in place the reply
|
||
would draw a picture, be asked again, and draw another.
|
||
|
||
## The defaults an administrator can set
|
||
|
||
**There were none, for the whole life of the feature.** `workflow.DEFAULTS` was
|
||
the only source, so 512×512, `euler` and twenty steps were what every instance
|
||
got whatever card it was running on — and 512² on an SDXL checkpoint is exactly
|
||
what the tool's own `width` description warns produces duplicated limbs. The two
|
||
ways round it were both bad: bake literals into a template where the
|
||
placeholders should be, or write prose in the instructions box and hope the
|
||
model obeys it.
|
||
|
||
`resolve(given, settings=…)` is three rungs now, most specific winning:
|
||
**`DEFAULTS` → the instance's `default_*` settings → what the model asked for.**
|
||
`DEFAULTS` stays underneath as the floor, so an instance that sets nothing
|
||
behaves exactly as it did, and improving a floor in code still reaches everyone.
|
||
|
||
**An empty setting is "no opinion", not zero.** `_number` in `admin_images`
|
||
returns `""` for an empty box and `instance_defaults` skips it. Reading it as a
|
||
number instead would set every instance to zero steps, which ComfyUI refuses in
|
||
a way that looks like a broken model.
|
||
|
||
**The samplers and schedulers were already being discovered and read by
|
||
nothing.** `comfy.discover()` has fetched all three lists since the Test button
|
||
existed, and only `checkpoints` was ever used. The pickers are built from the
|
||
other two. A stored value that is not in the list is kept as an option anyway,
|
||
or opening the page and pressing Save would silently clear a working setting.
|
||
|
||
**`batch` is a placeholder a model cannot set.** `batch_size` was a literal `1`
|
||
in the base template, so an administrator whose card can make four at a time had
|
||
no way of saying so. It is absent from `MODEL_SETTABLE`, deliberately: a model
|
||
asking for six because it is unsure is the exact cost this must not invite.
|
||
|
||
**The schema restates the defaults it quotes.** Every "Default 20." in
|
||
`SCHEMA` was written when there was one set of defaults in the world.
|
||
`_restate_defaults` rewrites each one from what this instance actually resolves
|
||
to — a schema saying "Default 512" beside an instance that draws at 1024 is
|
||
worse than saying nothing, because the model reasons from it and omits the
|
||
parameter, arriving at the right behaviour for the wrong reason or the wrong one
|
||
silently. The regex keeps the punctuation it found, since `denoise` says
|
||
"Default 1, which is…" and the rest use a full stop.
|
||
|
||
**The workflow editor's legend shows the resolved value beside each
|
||
placeholder.** A list of names answers "what may I write"; the question somebody
|
||
has in front of a workflow that came out wrong is "what happens if I leave this
|
||
out", and that answer moved the day instance defaults arrived. It is resolved
|
||
through the same call a generation makes, so the two cannot disagree. The legend
|
||
also states the two names that are not ComfyUI's own — `{{model}}` fills
|
||
`ckpt_name` and `{{sampler}}` fills `sampler_name` — which is the mistake that
|
||
costs an afternoon.
|