From 3e5ffe2c2da066c2eb25666ef894b0ecb42ba944 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 8 Nov 2026 15:56:30 +0000 Subject: [PATCH] Review fixes round 2: only Upscale and Interpolate jobs wait for each other, a queued one stops at once; tour numbers the parts on screen and Help sees it running Tour and docs wording match the UI names. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01Tw9fjVsBg3ixjnT1Fgdy12 --- app/ui/features/menus/index.js | 2 +- app/ui/features/tour/index.js | 16 +++---- core/jobs.py | 24 ++++++++--- docs/how-to-start.md | 4 +- modules/output/interpolate/__init__.py | 2 +- modules/output/render/__init__.py | 2 +- .../output/render/tests/test_jobs_routes.py | 42 +++++++++++++++++-- 7 files changed, 72 insertions(+), 20 deletions(-) diff --git a/app/ui/features/menus/index.js b/app/ui/features/menus/index.js index 5b03134d..3a55742f 100644 --- a/app/ui/features/menus/index.js +++ b/app/ui/features/menus/index.js @@ -70,7 +70,7 @@ export default { { id: "exclude", label: "Toggle Exclude", disabled: !sel.focus }], View: () => [{ id: "refresh", label: "Refresh Preview" }, { id: "reset", label: "Reset Layout" }], Help: () => { - const running = Boolean(app.tourOn?.()); + const running = app.has.has("tour.running"); return [...(app.has.has("tour") ? [{ id: "tour", label: running ? "Restart tour" : "Start tour" }, { id: "faq", label: "Skip to FAQ" }, ...(running ? [{ id: "exit", label: "Exit tour" }] : []), { separator: true }] : []), { id: "welcome", label: "Welcome window…" }]; }, diff --git a/app/ui/features/tour/index.js b/app/ui/features/tour/index.js index b44846d4..600d5948 100644 --- a/app/ui/features/tour/index.js +++ b/app/ui/features/tour/index.js @@ -13,7 +13,7 @@ const STEPS = [ { at: "button:⏱ Sampler", side: "top", title: "Sampler", text: "Steps, sampler, scheduler, seed and the common switches, one click from the timeline. While a run goes, the step count shows to its left." }, { at: "button:◆ Composer", side: "top", title: "Composer", text: "Write the whole story in one box, cut at the cut word; keep templates, $variables and shortcuts; chat with the prompt enhancer." }, { at: "button:＋ Add", side: "top", title: "Add and edit", text: "＋ Add opens one menu: a scene, a video clip or audio track from the bin, an effect or transition, a text or picture overlay. Next to it: Split, Remove, Export, Save to media bin and Separate audio." }, - { at: "button:⧉ Render", side: "top", title: "Render", text: "Joins the whole timeline into one file, with the separated sound mixed in. Export downloads just the picked clip." }, + { at: "button:⧉ Render", side: "top", title: "Render", text: "Joins the whole timeline into one file, with the separated sound mixed in. Export downloads the picked clips." }, { at: "main", side: "bottom", title: "FAQ", text: "Nothing happens? Check the ComfyUI chip at the top right and the Log window. Wrong look? Settings ▸ Appearance. Lost a layout? View ▸ Reset Layout. Lost a project? File ▸ Open recent. That is the tour: Help ▸ Start tour runs it again." }, ]; @@ -23,26 +23,28 @@ export default { needs: ["project", "dom"], setup({ app }) { let at = -1, pop = null; - const stop = () => { if (pop) { const p = pop; pop = null; p.close(); } at = -1; }; + const stop = () => { if (pop) { const p = pop; pop = null; p.close(); } at = -1; app.has.delete("tour.running"); }; // a flag the Help menu reads: features see copies of app, the Set is shared + const shown = (i) => { const el = app.dom.landmark(STEPS[i].at); return el && el.getBoundingClientRect().width ? el : null; }; function show(i) { if (pop) { const p = pop; pop = null; p.close(); } let target; - while (i >= 0 && i < STEPS.length && !((target = app.dom.landmark(STEPS[i].at)) && target.getBoundingClientRect().width)) i += i >= at ? 1 : -1; // a part that is hidden right now is skipped + while (i >= 0 && i < STEPS.length && !(target = shown(i))) i += i >= at ? 1 : -1; // a part that is hidden right now is skipped if (i < 0 || i >= STEPS.length) return stop(); at = i; + app.has.add("tour.running"); + const seen = STEPS.map((_, k) => k).filter((k) => k === i || shown(k)), place = seen.indexOf(i) + 1; // numbered among the parts on screen, so the count has no gaps const s = STEPS[i], last = i === STEPS.length - 1; - const body = c.region.stack({ gap: "sm", children: [c.label.section({ text: `${i + 1} / ${STEPS.length} · ${s.title}` }), c.text.sm({ text: s.text }), + const body = c.region.stack({ gap: "sm", children: [c.label.section({ text: `${place} / ${seen.length} · ${s.title}` }), c.text.sm({ text: s.text }), c.toolbar.default({ items: [c.button.sm({ label: "Back", tone: "ghost", disabled: i === 0, onClick: () => show(i - 1) }), c.button.sm({ label: last ? "Done" : "Next", tone: "primary", onClick: () => (last ? stop() : show(i + 1)) })], trailing: [c.button.sm({ label: "Exit tour", tone: "ghost", onClick: stop })] })] }); - pop = c.popover.anchored({ anchor: target, body, side: s.side, align: "center", gap: 10, closeOnOutside: false, onClose: () => { if (pop) { pop = null; at = -1; } } }); + pop = c.popover.anchored({ anchor: target, body, side: s.side, align: "center", gap: 10, closeOnOutside: false, onClose: () => { if (pop) { pop = null; at = -1; app.has.delete("tour.running"); } } }); } app.has.add("tour"); - app.tourOn = () => pop !== null; // Help shows "Exit tour" only while one is running const off = app.on((what) => { if (what === "tour.start") show(0); else if (what === "tour.faq") show(STEPS.length - 1); else if (what === "tour.stop") stop(); }); - return () => { off(); app.has.delete("tour"); delete app.tourOn; }; + return () => { off(); app.has.delete("tour"); app.has.delete("tour.running"); }; }, }; diff --git a/core/jobs.py b/core/jobs.py index 947eeaf1..86cdced5 100644 --- a/core/jobs.py +++ b/core/jobs.py @@ -8,6 +8,7 @@ from collections import OrderedDict MAX_JOBS = 50 # finished jobs are kept this long for the poll that reads them +_waiting: set = set() # ids queued behind another job _jobs: "OrderedDict[str, dict]" = OrderedDict() _stop: set = set() # ids asked to stop; the job notices between chunks _can_stop: set = set() # ids whose work checks for it @@ -23,6 +24,8 @@ def stop(job: str) -> bool: if job not in _can_stop or not entry or entry.get("state") not in ("queued", "running"): return False _stop.add(job) + if entry.get("state") == "queued" and job in _waiting: + _jobs[job] = {"state": "stopped"} # still waiting for its turn: it ends now, not when the one ahead does return True @@ -49,15 +52,23 @@ def get(job: str) -> dict | None: return _jobs.get(job) -_turns: "weakref.WeakKeyDictionary" = weakref.WeakKeyDictionary() # one lock per event loop: one job at a time (they share the GPU, the loaded effects and ffmpeg) +_turns: "weakref.WeakKeyDictionary" = weakref.WeakKeyDictionary() # one lock per event loop -async def run(job: str, fn, *args, error, stoppable: bool = False) -> None: +async def run(job: str, fn, *args, error, stoppable: bool = False, exclusive: bool = False) -> None: """Run the blocking `fn(*args)` off the event loop and record how it ended. `error` is the exception class whose message is meant for the person; anything else is reported plainly. - Jobs run one after the other (a second one waits as "queued"): two effects of one kind share a buffer.""" - turn = _turns.setdefault(asyncio.get_running_loop(), asyncio.Lock()) - async with turn: + `exclusive`: the work uses a GPU effect that keeps one buffer per kind (Upscale, Interpolate), so such jobs + run one after the other and a second waits as "queued". Other jobs (the render) never wait for them.""" + turn = _turns.setdefault(asyncio.get_running_loop(), asyncio.Lock()) if exclusive else None + if turn is not None and turn.locked(): + _waiting.add(job) + try: + if turn is not None: + await turn.acquire() + finally: + _waiting.discard(job) + try: if job in _stop: # stopped while it was still queued _stop.discard(job) _can_stop.discard(job) @@ -76,3 +87,6 @@ async def run(job: str, fn, *args, error, stoppable: bool = False) -> None: finally: _stop.discard(job) _can_stop.discard(job) + finally: + if turn is not None: + turn.release() diff --git a/docs/how-to-start.md b/docs/how-to-start.md index 3209a487..9094b7c4 100644 --- a/docs/how-to-start.md +++ b/docs/how-to-start.md @@ -23,8 +23,8 @@ Start ComfyUI as usual and open `/fun/` on the same address, for example `http:/ | Start from | State | |---|---| | MiniMax H3 · Reference to Video, MiniMax H3 · Fast & Simple R2V | The model FunUI is built and tested around. | -| Stable Diffusion 1.5 · Text to Image | Experimental. | -| LTX 2.3 · Image to Video (from References), LTX 2.3 · Video to Video (direct) | Tested on a real model, with LoRAs and Noise merging. Other enhancements are not tested on LTX yet; please report problems in [Issues](https://github.com/digital-garbage/FunUI/issues). | +| Stable Diffusion 1.5 · Text to Image (held as a clip) | Experimental. | +| LTX 2.3 · Image to Video, from References, LTX 2.3 · Video to Video, direct | Tested on a real model, with LoRAs and Noise merging. Other enhancements are not tested on LTX yet; please report problems in [Issues](https://github.com/digital-garbage/FunUI/issues). | ## 4. Pick your files diff --git a/modules/output/interpolate/__init__.py b/modules/output/interpolate/__init__.py index ac6ba9c1..3d7b9b9d 100644 --- a/modules/output/interpolate/__init__.py +++ b/modules/output/interpolate/__init__.py @@ -43,7 +43,7 @@ def routes(table, base, web): except ValueError as exc: return bad(str(exc)) job = jobs.new(stoppable=True) - asyncio.create_task(jobs.run(job, nodes.interpolate, media, body.get("options") or {}, error=nodes.InterpolateError, stoppable=True)) + asyncio.create_task(jobs.run(job, nodes.interpolate, media, body.get("options") or {}, error=nodes.InterpolateError, stoppable=True, exclusive=True)) return web.json_response({"job_id": job}) @table.post(base + "/jobs/{job}/stop") diff --git a/modules/output/render/__init__.py b/modules/output/render/__init__.py index 77eb2166..aac932a9 100644 --- a/modules/output/render/__init__.py +++ b/modules/output/render/__init__.py @@ -102,7 +102,7 @@ def routes(table, base, web): if not isinstance(media, dict) or not isinstance(media.get("filename"), str) or not media["filename"] or not isinstance(model, str) or not model: return bad("Send {media: {filename, subfolder, type}, model, rtx_options}.") job = jobs.new(stoppable=True) - asyncio.create_task(jobs.run(job, upscale.run, media, model, body.get("rtx_options") or "{}", error=(RuntimeError, ValueError, files.ClipError), stoppable=True)) + asyncio.create_task(jobs.run(job, upscale.run, media, model, body.get("rtx_options") or "{}", error=(RuntimeError, ValueError, files.ClipError), stoppable=True, exclusive=True)) return web.json_response({"job_id": job}) @table.post(base + "/jobs/{job}/stop") diff --git a/modules/output/render/tests/test_jobs_routes.py b/modules/output/render/tests/test_jobs_routes.py index ae6a83bd..ea2987ff 100644 --- a/modules/output/render/tests/test_jobs_routes.py +++ b/modules/output/render/tests/test_jobs_routes.py @@ -235,7 +235,7 @@ def test_a_stop_that_lands_after_the_last_chunk_still_removes_the_file(comfy): assert not dst.exists() -def test_jobs_run_one_after_the_other_and_the_second_waits_as_queued(): +def test_exclusive_jobs_run_one_after_the_other_and_the_second_waits_as_queued(): import asyncio import threading gate, order = threading.Event(), [] @@ -252,10 +252,10 @@ def test_jobs_run_one_after_the_other_and_the_second_waits_as_queued(): async def go(): a, b = jobs.new(), jobs.new() - ta = asyncio.create_task(jobs.run(a, first, error=RuntimeError)) + ta = asyncio.create_task(jobs.run(a, first, error=RuntimeError, exclusive=True)) while "first start" not in order: await asyncio.sleep(0.01) - tb = asyncio.create_task(jobs.run(b, second, error=RuntimeError)) + tb = asyncio.create_task(jobs.run(b, second, error=RuntimeError, exclusive=True)) await asyncio.sleep(0.1) waiting = (jobs.get(a)["state"], jobs.get(b)["state"], list(order)) gate.set() @@ -265,3 +265,39 @@ def test_jobs_run_one_after_the_other_and_the_second_waits_as_queued(): waiting, order, last = asyncio.run(go()) assert waiting == ("running", "queued", ["first start"]) assert order == ["first start", "first end", "second start"] and last == "done" + + +def test_a_render_never_waits_for_an_exclusive_job_and_a_queued_one_can_be_stopped(): + import asyncio + import threading + gate, ran = threading.Event(), [] + + def long_effect(interrupted): + gate.wait(5) + return {} + + def render(): + ran.append("render") + return {} + + def effect(interrupted): + ran.append("effect") + return {} + + async def go(): + a, b, r = jobs.new(stoppable=True), jobs.new(stoppable=True), jobs.new() + ta = asyncio.create_task(jobs.run(a, long_effect, error=RuntimeError, stoppable=True, exclusive=True)) + await asyncio.sleep(0.05) + tb = asyncio.create_task(jobs.run(b, effect, error=RuntimeError, stoppable=True, exclusive=True)) + await asyncio.sleep(0.05) + await jobs.run(r, render, error=RuntimeError) # not held up by the effect running + seen = (jobs.get(r)["state"], jobs.get(b)["state"]) + assert jobs.stop(b) is True + stopped_now = jobs.get(b)["state"] # ends at once, not when the one ahead ends + gate.set() + await asyncio.gather(ta, tb) + return seen, stopped_now, jobs.get(b)["state"] + + seen, stopped_now, last = asyncio.run(go()) + assert seen == ("done", "queued") and stopped_now == "stopped" and last == "stopped" + assert ran == ["render"] # the stopped one never started