diff --git a/src/lembas/__init__.py b/src/lembas/__init__.py index d130a77..36a6efc 100644 --- a/src/lembas/__init__.py +++ b/src/lembas/__init__.py @@ -1,3 +1,3 @@ """LLeMbas - a Middle-earth themed web UI for OpenAI-compatible LLM endpoints.""" -__version__ = "0.7.1" +__version__ = "0.7.2" diff --git a/src/lembas/services/images/comfy.py b/src/lembas/services/images/comfy.py index 959aab8..672998d 100644 --- a/src/lembas/services/images/comfy.py +++ b/src/lembas/services/images/comfy.py @@ -93,6 +93,29 @@ class ComfyError(LLMError): """Anything that stopped a generation, in words worth showing somebody.""" +class OutOfMemory(ComfyError): + """The far side ran out of VRAM. + + Its own class because it is the one failure with an obvious next move -- + a smaller picture, or a smaller checkpoint -- and the model is told to make + it. Everything else is reported and stopped at. + """ + + +class Interrupted(ComfyError): + """Somebody cancelled it from ComfyUI's own interface, or it was stopped. + + Distinct because it is not a fault: retrying is reasonable, and "the + workflow failed" would be describing a decision as a breakage. + """ + + +# What `exception_type` looks like when a GPU has run out. Matched on the type +# rather than on the message, which is a paragraph of allocator advice written +# for whoever is running the box and not for a model. +_OOM_TYPES = ("outofmemory", "out_of_memory", "cuda error: out of memory") + + def _transport_error(exc: httpx.RequestError, config: Config) -> ComfyError: """The `wrap_transport_error` shape, said about ComfyUI rather than an LLM. @@ -133,9 +156,7 @@ async def submit(config: Config, workflow: dict[str, Any]) -> str: body = {"prompt": workflow, "client_id": uuid.uuid4().hex} try: async with httpx.AsyncClient(timeout=60.0) as client: - response = await client.post( - config.url("prompt"), headers=config.headers(), json=body - ) + response = await client.post(config.url("prompt"), headers=config.headers(), json=body) if response.status_code >= 400: raise ComfyError(_refusal(response)) data = response.json() @@ -185,19 +206,24 @@ def _describe_nodes(errors: dict[str, Any]) -> str: async def await_images(config: Config, prompt_id: str) -> list[Ref]: """Wait for one queued workflow and answer with what it saved. - `/history/{id}` is empty while the job is queued or running and gains the - whole record when it ends, so an empty answer is "not yet" rather than - "nothing" -- which is why the deadline is the only thing that ends this. + **The record existing is what "finished" means, not `status.completed`.** + ComfyUI writes the history entry in `task_done` and nowhere else, so it + appears exactly once the job is over -- but it sets `completed=e.success`, + so a run that failed is `completed: false` for ever. Waiting on that flag + means every out-of-memory, every cancelled job and every broken node hangs + the reply for the whole timeout and then reports a timeout, when ComfyUI + knew what was wrong within seconds and said so. + + So: no record means not yet, a record means done, and `status_str` says + which kind of done. """ deadline = time.monotonic() + config.timeout while True: record = (await _get_json(config, f"history/{prompt_id}")).get(prompt_id) - if isinstance(record, dict) and (record.get("status") or {}).get("completed"): + if isinstance(record, dict) and record.get("status") is not None: status = record.get("status") or {} - if status.get("status_str") not in (None, "success"): - raise ComfyError( - f"ComfyUI could not finish the workflow ({status.get('status_str')})." - ) + if status.get("status_str") != "success": + raise _failure(status) return _refs_in(record.get("outputs") or {}) if time.monotonic() > deadline: raise ComfyError( @@ -207,6 +233,51 @@ async def await_images(config: Config, prompt_id: str) -> list[Ref]: await asyncio.sleep(POLL_INTERVAL) +def _failure(status: dict[str, Any]) -> ComfyError: + """Why a workflow stopped, out of the messages ComfyUI recorded against it. + + `status.messages` is a list of `[name, payload]` pairs -- the lifecycle of + the run. The last `execution_error` or `execution_interrupted` in it is the + thing that ended it, and carries the node and the exception. Without reading + these the only thing that could be said is "error", which is what ComfyUI's + own status string amounts to. + """ + event, payload = "", {} + for entry in status.get("messages") or []: + if isinstance(entry, list | tuple) and len(entry) == 2: + name, body = entry + if name in ("execution_error", "execution_interrupted"): + event, payload = str(name), body if isinstance(body, dict) else {} + + node = str(payload.get("node_type") or "").strip() + where = f" in {node}" if node else "" + + if event == "execution_interrupted": + return Interrupted(f"The image was cancelled on the ComfyUI side{where}.") + + kind = str(payload.get("exception_type") or "") + detail = _first_sentence(str(payload.get("exception_message") or "")) + if any(marker in kind.lower() for marker in _OOM_TYPES) or "out of memory" in detail.lower(): + return OutOfMemory(f"ComfyUI ran out of video memory{where}. {detail}".strip()) + if not detail and not kind: + return ComfyError(f"ComfyUI could not finish the workflow{where}.") + return ComfyError(f"ComfyUI could not finish the workflow{where}: {detail or kind}") + + +def _first_sentence(message: str) -> str: + """Enough of an exception to act on, and no more. + + A torch OOM runs to several lines of allocator advice -- environment + variables to set, fragmentation notes -- addressed to whoever runs the box. + None of it means anything to a model, and all of it costs tokens in a tool + result that is already a failure. + """ + first = message.strip().split("\n", 1)[0].strip() + if len(first) > 200: + first = first[:200].rsplit(" ", 1)[0] + "…" + return first + + def _refs_in(outputs: dict[str, Any]) -> list[Ref]: """Every image any node saved, in node order. @@ -233,9 +304,7 @@ async def fetch_image(config: Config, ref: Ref) -> bytes: params = {"filename": ref.filename, "subfolder": ref.subfolder, "type": ref.kind} try: async with httpx.AsyncClient(timeout=120.0) as client: - response = await client.get( - config.url("view"), headers=config.headers(), params=params - ) + response = await client.get(config.url("view"), headers=config.headers(), params=params) response.raise_for_status() payload = response.content except httpx.HTTPStatusError as exc: diff --git a/src/lembas/services/images/tool.py b/src/lembas/services/images/tool.py index 89a961f..2449815 100644 --- a/src/lembas/services/images/tool.py +++ b/src/lembas/services/images/tool.py @@ -61,9 +61,19 @@ SCHEMA: dict[str, Any] = { "type": "string", "description": "What to draw. Describe the subject, the setting and the style.", }, + # Every description below says what the value *does to the picture* and + # when to move it, not what it is called. A model that is told "cfg: + # prompt adherence, default 8" has been told nothing it can act on, and + # the observable result is a model that sends the prompt alone and + # leaves ten parameters at their defaults for ever. "negative": { "type": "string", - "description": "What to keep out of the picture. Defaults to 'text, watermark'.", + "description": ( + "Comma-separated things to keep OUT of the picture, as plain nouns and " + "adjectives: 'blurry, extra fingers, text, watermark'. Not a sentence, " + "and never phrased as an instruction — 'do not add text' puts *text* in " + "the picture. Defaults to 'text, watermark'." + ), }, "template": { "type": "string", @@ -71,22 +81,80 @@ SCHEMA: dict[str, Any] = { }, "model": { "type": "string", - "description": "Which checkpoint to draw with. Omit to use this chat's usual one.", + "description": ( + "Which checkpoint to draw with. Pick by what it is good at; omit to use " + "this chat's usual one." + ), }, "seed": { "type": "integer", "description": ( "Omit it, or pass -1, for a new random image. Repeat a seed you were " - "told about to get the same image again." + "told about to get that same image again — which is how you change one " + "thing about a picture and keep the rest." + ), + }, + "steps": { + "type": "integer", + "description": ( + "How long to refine, 1-150. Default 20. Around 20-30 for most things; " + "8-12 for a quick draft or when several are wanted; 40+ only for fine " + "detail, and past about 50 it stops improving and only costs time." + ), + }, + "cfg": { + "type": "number", + "description": ( + "How literally to follow the prompt, 0-30. Default 8. 3-6 gives the " + "model room and looks more natural; 7-9 is the usual range; 12+ forces " + "the words through and starts to look burnt and over-saturated. Lower " + "it if the picture looks harsh, raise it if the subject is being " + "ignored." + ), + }, + "width": { + "type": "integer", + "description": ( + "Pixels, 64-2048, a multiple of 8. Default 512. Use the size the " + "checkpoint was trained for — about 512 for SD1.5, about 1024 for SDXL " + "— and change the ratio rather than the total: 512x768 for a portrait, " + "768x512 for a landscape. Going far above what the checkpoint expects " + "produces duplicated limbs and repeated horizons, not more detail." + ), + }, + "height": { + "type": "integer", + "description": ( + "Pixels, 64-2048, a multiple of 8. Default 512. See width: the aspect " + "ratio is the thing to choose, and taller than wide suits a person, " + "wider than tall suits a place." + ), + }, + "sampler": { + "type": "string", + "description": ( + "How the image is solved. Default euler. 'euler' is safe and fast; " + "'dpmpp_2m' is a good general improvement; 'dpmpp_2m_sde' for more " + "texture; 'ddim' for a clean flat look. Leave it out unless you have a " + "reason." + ), + }, + "scheduler": { + "type": "string", + "description": ( + "How the steps are spaced. Default normal. 'karras' pairs well with the " + "dpmpp samplers and usually helps at low step counts; 'normal' " + "otherwise. Leave it out unless you are also setting the sampler." + ), + }, + "denoise": { + "type": "number", + "description": ( + "How much of the starting noise to replace, 0-1. Default 1, which is " + "what you want for a picture drawn from nothing. Lower values only mean " + "something for a workflow that starts from an existing image." ), }, - "steps": {"type": "integer", "description": "Sampling steps. Default 20."}, - "cfg": {"type": "number", "description": "Prompt adherence. Default 8."}, - "width": {"type": "integer", "description": "Pixels. Default 512."}, - "height": {"type": "integer", "description": "Pixels. Default 512."}, - "sampler": {"type": "string", "description": "Sampler name. Default euler."}, - "scheduler": {"type": "string", "description": "Scheduler name. Default normal."}, - "denoise": {"type": "number", "description": "0 to 1. Default 1."}, }, "required": ["prompt"], } @@ -371,6 +439,9 @@ async def run(context: ToolContext, args: dict[str, Any]) -> ToolOutcome: attempts: list[Attempt] = [] kept: tuple[bytes, dict[str, Any]] | None = None + # What the last attempt actually asked for, so a failure can name concrete + # numbers back at the model rather than saying "try something smaller". + params_used: dict[str, Any] = workflow.resolve(given) try: for number in range(1, tries + 1): @@ -378,6 +449,7 @@ async def run(context: ToolContext, args: dict[str, Any]) -> ToolOutcome: await _unload_llm(context) params = workflow.resolve({**given, "seed": args.get("seed") if number == 1 else None}) + params_used = params refs = await comfy.await_images( config, await comfy.submit(config, workflow.fill(template, params)) ) @@ -402,9 +474,12 @@ async def run(context: ToolContext, args: dict[str, Any]) -> ToolOutcome: break except comfy.ComfyError as exc: if preserve: + # It failed *inside* the far side, so its models are still resident + # and the language model is still unloaded. Freeing here is what + # lets the reply carry on and say what happened. await comfy.free(config) return ToolOutcome( - f"The image could not be generated: {exc.message}", + f"The image could not be generated: {exc.message}{_advice(exc, params_used)}", {**event, "status": "error", "error": exc.message}, ) @@ -454,6 +529,34 @@ async def run(context: ToolContext, args: dict[str, Any]) -> ToolOutcome: ) +def _advice(exc: comfy.ComfyError, params: dict[str, Any]) -> str: + """What to do about a failure, when there is something to do about it. + + Only for the two that have an obvious next move. Everything else gets the + reason and nothing else -- a model told to "try again" after a broken + workflow will try the identical thing, and a suggestion invented for a + failure nobody understands is a guess wearing the application's authority. + + The numbers are concrete on purpose. "Use a lower resolution" against a + request that was already 512x512 is advice that cannot be followed, so the + halved size is worked out here where the request is known. + """ + if isinstance(exc, comfy.Interrupted): + return ( + " Somebody stopped it deliberately, so do not simply start it again — say so and ask." + ) + if not isinstance(exc, comfy.OutOfMemory): + return "" + + width, height = int(params.get("width") or 512), int(params.get("height") or 512) + smaller = f"{max(256, width // 2)}x{max(256, height // 2)}" + return ( + f" Try once more at a smaller size — {smaller} instead of {width}x{height} — " + "or with a lighter checkpoint if one is offered. Do not repeat the same " + "request unchanged; it will run out of memory again." + ) + + def _pick(rows: list[Any], wanted: str, chat_default: str, values: dict[str, Any]) -> Any: """The workflow to use: asked for, then the chat's, then the instance's.""" by_slug = {row.slug: row for row in rows} diff --git a/src/lembas/services/prompts.py b/src/lembas/services/prompts.py index 976d7e0..fe6f955 100644 --- a/src/lembas/services/prompts.py +++ b/src/lembas/services/prompts.py @@ -1025,21 +1025,38 @@ BUILTIN: tuple[Fragment, ...] = ( group=GROUP_TOOLS, order=243, families=("image",), - hint="Appears when image generation is offered. The sentence about the " - "picture already being on screen is the one that earns its place: " - "without it the commonest thing a model does next is offer to show you " - "the image, which it has no way of doing and which has already " - "happened.", + hint="Appears when image generation is offered. Two sentences here earn " + "their place against the tool's own descriptions. The picture already " + "being on screen, because without it the commonest thing a model does " + "next is offer to show you the image — which it cannot do and which has " + "already happened. And the shape of a prompt: a small model left to " + "itself passes the request through verbatim, which is why so many " + "generations look like nobody thought about them.", default=( - "- You can draw a picture with image_generate. Describe what you want in " - "the prompt as fully as you can — subject, setting, lighting, style — " - "because the prompt is the whole of what the picture is made from.\n" - "- The picture appears in the conversation as soon as the tool returns. " - "It is already on screen: do not offer to show it, link to it or " - "describe how to open it.\n" - "- Only the prompt is required. Everything else has a sensible default, " - "so set a parameter when you have a reason to and leave it out " - "otherwise. Repeat a seed to get the same picture again." + "- You can draw a picture with image_generate. Only `prompt` is required.\n" + "- Write the prompt as a description, not as the request you were given. " + "Comma-separated phrases work better than a sentence, and the order matters " + "— subject first, then what it is doing, then the setting, then the light, " + "then the style and medium. \"a red bicycle\" is a worse prompt than \"a red " + "bicycle leaning on a whitewashed wall, morning light, long shadows, 35mm " + "photograph, shallow depth of field\". Expand what you were asked for into " + "one of these; do not ask the person to write it for you.\n" + "- Use `negative` for what must not appear, as plain nouns: \"blurry, extra " + "fingers, text, watermark\". Never phrase it as an instruction — \"no text\" " + "puts text in the picture.\n" + "- Set `width` and `height` to suit the subject rather than leaving both at " + "the default: taller than wide for a person, wider than tall for a place. " + "Match the size the checkpoint expects; far above it produces duplicated " + "limbs rather than more detail.\n" + "- The other parameters have sensible defaults. Change one when you have a " + "reason — fewer steps for a quick draft, lower cfg when a picture looks " + "harsh — and leave it out otherwise.\n" + "- The picture appears in the conversation as soon as the tool returns. It " + "is already on screen: do not offer to show it, link to it, or describe how " + "to open it. Say what you made and what you would change.\n" + "- If it fails because the machine ran out of video memory, try once more at " + "a smaller size or with a lighter checkpoint. Do not repeat the same request " + "unchanged." ), ), Fragment( diff --git a/tests/test_images_comfy.py b/tests/test_images_comfy.py index 4810a0c..c71297b 100644 --- a/tests/test_images_comfy.py +++ b/tests/test_images_comfy.py @@ -279,3 +279,127 @@ async def test_a_custom_node_pack_that_changes_the_shape_is_survived(mock_http): ) ) assert await comfy.discover(CONFIG) == ([], [], []) + + +# --- Failing --------------------------------------------------------------- +# ComfyUI sets `completed=e.success`, so a run that *failed* is `completed: +# false` for ever. Waiting on that flag means every out-of-memory, every +# cancelled job and every broken node hangs the reply for the whole timeout and +# then reports a timeout -- when ComfyUI knew what was wrong within a second and +# had written it down. Every shape below was read off a real ComfyUI 0.27.0 by +# causing the failure rather than by imagining it. +def _failed(event, payload): + return { + "p1": { + "status": { + "status_str": "error", + "completed": False, + "messages": [ + ["execution_start", {"prompt_id": "p1"}], + [event, payload], + ], + }, + "outputs": {}, + } + } + + +def _history(record): + return _handler({"/history": lambda r: httpx.Response(200, json=record)}) + + +async def test_a_failure_is_noticed_at_once_rather_than_at_the_timeout(mock_http, monkeypatch): + monkeypatch.setattr(comfy, "POLL_INTERVAL", 0.01) + mock_http(_history(_failed("execution_error", {"exception_message": "boom"}))) + + # A timeout long enough that waiting for it would hang the test. + slow = comfy.Config(base_url=CONFIG.base_url, timeout=3600.0) + with pytest.raises(comfy.ComfyError) as caught: + await comfy.await_images(slow, "p1") + assert "did not finish within" not in caught.value.message + + +async def test_running_out_of_memory_is_its_own_kind(mock_http, monkeypatch): + """It is the one failure with an obvious next move, and the tool tells the + model to make it.""" + monkeypatch.setattr(comfy, "POLL_INTERVAL", 0.01) + mock_http( + _history( + _failed( + "execution_error", + { + "node_type": "KSampler", + "exception_type": "torch.OutOfMemoryError", + "exception_message": ( + "CUDA out of memory. Tried to allocate 5.62 GiB. GPU 0 has a total " + "capacity of 15.92 GiB of which 108.00 MiB is free.\n" + "If reserved but unallocated memory is large try setting " + "PYTORCH_CUDA_ALLOC_CONF=expandable_segments:True" + ), + }, + ) + ) + ) + + with pytest.raises(comfy.OutOfMemory) as caught: + await comfy.await_images(CONFIG, "p1") + assert "video memory" in caught.value.message + assert "KSampler" in caught.value.message, "which node ran out" + assert "PYTORCH_CUDA_ALLOC_CONF" not in caught.value.message, ( + "allocator advice is addressed to whoever runs the box, not to a model" + ) + + +async def test_being_cancelled_is_not_a_fault(mock_http, monkeypatch): + """Retrying a cancelled job is reasonable; "the workflow failed" would be + describing somebody's decision as a breakage.""" + monkeypatch.setattr(comfy, "POLL_INTERVAL", 0.01) + mock_http(_history(_failed("execution_interrupted", {"node_type": "KSampler"}))) + + with pytest.raises(comfy.Interrupted) as caught: + await comfy.await_images(CONFIG, "p1") + assert "cancelled" in caught.value.message + + +async def test_a_node_that_raised_says_which_and_why(mock_http, monkeypatch): + monkeypatch.setattr(comfy, "POLL_INTERVAL", 0.01) + mock_http( + _history( + _failed( + "execution_error", + { + "node_type": "VAEDecode", + "exception_type": "ValueError", + "exception_message": "given tensor has the wrong shape", + }, + ) + ) + ) + + with pytest.raises(comfy.ComfyError) as caught: + await comfy.await_images(CONFIG, "p1") + assert "VAEDecode" in caught.value.message + assert "wrong shape" in caught.value.message + assert not isinstance(caught.value, comfy.OutOfMemory) + + +async def test_a_failure_with_nothing_recorded_still_says_something(mock_http, monkeypatch): + monkeypatch.setattr(comfy, "POLL_INTERVAL", 0.01) + mock_http( + _history({"p1": {"status": {"status_str": "error", "completed": False}, "outputs": {}}}) + ) + + with pytest.raises(comfy.ComfyError) as caught: + await comfy.await_images(CONFIG, "p1") + assert "could not finish" in caught.value.message + + +async def test_a_record_without_a_status_is_still_not_yet(mock_http, monkeypatch): + """The terminal condition is the *status*, not the key. A record ComfyUI is + still assembling must not be read as a silent failure.""" + monkeypatch.setattr(comfy, "POLL_INTERVAL", 0.01) + mock_http(_history({"p1": {"outputs": {}}})) + + with pytest.raises(comfy.ComfyError) as caught: + await comfy.await_images(comfy.Config(base_url=CONFIG.base_url, timeout=0.05), "p1") + assert "did not finish within" in caught.value.message diff --git a/tests/test_images_tool.py b/tests/test_images_tool.py index b80bb5b..d7407c2 100644 --- a/tests/test_images_tool.py +++ b/tests/test_images_tool.py @@ -393,3 +393,97 @@ async def test_an_unload_that_fails_does_not_stop_the_generation( outcome = await image_tool.run(_context(db, user_id, configured), {"prompt": "x"}) assert outcome.event["status"] == "ok" + + +# --- What the model is told when it fails -------------------------------------- +async def test_running_out_of_memory_tells_the_model_what_to_do( + db, user_id, configured, fake, monkeypatch +): + """A bare "out of memory" gets the same request sent again, which fails the + same way. The numbers are concrete because "use a lower resolution" against + a request that was already 512x512 is advice nobody can follow.""" + + async def oom(config, wf): + raise comfy.OutOfMemory("ComfyUI ran out of video memory in KSampler.") + + monkeypatch.setattr(comfy, "submit", oom) + outcome = await image_tool.run( + _context(db, user_id, configured), {"prompt": "x", "width": 1024, "height": 1024} + ) + + assert outcome.event["status"] == "error" + assert "512x512" in outcome.content, "a size it can actually try" + assert "1024x1024" in outcome.content, "and what it just asked for" + assert "lighter checkpoint" in outcome.content + assert "unchanged" in outcome.content + + +async def test_a_cancelled_generation_is_not_retried_blindly( + db, user_id, configured, fake, monkeypatch +): + """Somebody pressed stop. Starting it again is arguing with them.""" + + async def stopped(config, wf): + raise comfy.Interrupted("The image was cancelled on the ComfyUI side.") + + monkeypatch.setattr(comfy, "submit", stopped) + outcome = await image_tool.run(_context(db, user_id, configured), {"prompt": "x"}) + + assert "do not simply start it again" in outcome.content + + +async def test_an_ordinary_failure_gets_no_invented_advice( + db, user_id, configured, fake, monkeypatch +): + """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.""" + + async def broken(config, wf): + raise comfy.ComfyError("ComfyUI could not finish the workflow in VAEDecode.") + + monkeypatch.setattr(comfy, "submit", broken) + outcome = await image_tool.run(_context(db, user_id, configured), {"prompt": "x"}) + + assert "VAEDecode" in outcome.content + assert "smaller size" not in outcome.content + assert "Try once more" not in outcome.content + + +async def test_preserve_vram_frees_comfyui_even_when_it_failed( + db, user_id, configured, fake, monkeypatch +): + """It failed *inside* the far side, so its models are still resident and the + language model is still unloaded. Without this the reply cannot even get far + enough to say what happened.""" + settings_store.update(db, {"preserve_vram": True}, key=settings_store.IMAGES) + db.commit() + + async def oom(config, wf): + raise comfy.OutOfMemory("out of video memory") + + monkeypatch.setattr(comfy, "submit", oom) + monkeypatch.setattr(image_tool, "_unload_llm", _noop) + + await image_tool.run(_context(db, user_id, configured), {"prompt": "x"}) + assert fake.frees >= 1 + + +async def _noop(context): + return True + + +# --- Telling a model how to use the thing -------------------------------------- +def test_every_parameter_says_when_to_move_it(db, user_id, configured): + """ "cfg: prompt adherence, default 8" tells a model nothing it can act on, + and the observable result is a model that sends the prompt alone and leaves + ten parameters at their defaults for ever.""" + schema = image_tool.schema_for(db, settings_store.images(db)) + for name in ("steps", "cfg", "width", "height", "sampler", "scheduler", "denoise", "negative"): + description = schema["properties"][name]["description"] + assert len(description) > 80, f"{name} is described too thinly to act on" + + assert "never" in schema["properties"]["negative"]["description"].lower(), ( + "the negative prompt's one real trap: phrasing it as an instruction" + ) + assert "portrait" in schema["properties"]["width"]["description"]