Say what actually failed, and tell the model how to use the thing
Two problems, both found by looking rather than by guessing. 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 completed:false for ever. await_images waited on that 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 the node, the exception type and the message. Proved by causing both against the real instance: an OOM now raises in 1.0s and an interrupt in 4.0s, each naming the node. The terminal condition is a record with a status, and status.messages is read for the last execution_error or execution_interrupted. OutOfMemory and Interrupted are their own classes because they are the two failures with an obvious next move: the first tells the model to retry at a named smaller size -- worked out from what it actually asked for, since "use a lower resolution" against a request that was already 512x512 is advice nobody can follow -- or with a lighter checkpoint; the second says somebody pressed stop, so do not simply start again. Everything else gets the reason and no advice, because a model told to try again after a broken workflow tries the identical thing. The OOM message is cut to its first sentence. The rest is allocator advice -- PYTORCH_CUDA_ALLOC_CONF, fragmentation notes -- addressed to whoever runs the box and meaningless to a model, in a tool result that is already a failure. Second: the parameters were described in the register of a reference table, and "cfg: prompt adherence, default 8" tells a model nothing it can act on. Measured on a 4B model, same request, same everything else: with the old wording it sent prompt and template and nothing more -- so 512x512 on an SDXL checkpoint, which is exactly 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. ~3KB of schema per request in a chat that can draw, and the difference between having ten parameters and having one. docs/image-generation-instructions.md is the long version for the admin instructions box, for models that need more than the harness can afford to carry on every request in every chat. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jaroslav Beneš
@@ -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
|
||||
|
||||
@@ -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"]
|
||||
|
||||
Reference in New Issue
Block a user