diff --git a/app/ui/features/settings/models.js b/app/ui/features/settings/models.js index f37e171e..4af2e087 100644 --- a/app/ui/features/settings/models.js +++ b/app/ui/features/settings/models.js @@ -153,7 +153,9 @@ export const models = (app) => function mount() { function status(only) { const all = ps.incomplete(), refused = ps.refused(), notes = ps.saveNotes(); - const incomplete = only ? all.filter((t) => t.startsWith(`${only}:`)) : all; + const mine = (t, id) => t.startsWith(`${id}:`) || t.startsWith(`${id}.`) + || ((slotsNow().find((s) => s.id === id) || {}).roles || []).some((r) => r.label && t.startsWith(`${r.label} is `)); // refused by the name the person sees + const incomplete = only ? all.filter((t) => mine(t, only)) : all; return [refused.length ? c.banner.warn({ text: `Could not save: ${refused.join(" ")}` }) : null, notes.length ? c.banner.info({ text: notes.join(" ") }) : null, incomplete.length ? c.banner.warn({ text: only ? incomplete.join(" ") : `Not ready to generate yet: ${incomplete.length} node${incomplete.length === 1 ? "" : "s"} still need something (the orange dots). Open one to see what.` }) : ps.queueable() && !only ? c.hint.default({ text: "Every slot is filled — this pipeline is ready to generate." }) : null]; } diff --git a/app/ui/shell/pipeline_link.js b/app/ui/shell/pipeline_link.js index b9013b78..488b4a4f 100644 --- a/app/ui/shell/pipeline_link.js +++ b/app/ui/shell/pipeline_link.js @@ -5,7 +5,7 @@ export function linkPipeline({ project, pipeline, onOpen, say = () => {} }) { // it is written. A project opened while an open or a save is still in flight must never receive the other's pipeline. // An id, not the object: Undo puts a copy of the same project in place. let owner = null; - let retries = 0, opens = 0, retry = null, waiting = false; // waiting: gave up because ComfyUI did not answer + let retries = 0, opens = 0, retry = null, waiting = false, replaceOnEdit = false; // waiting: gave up because ComfyUI did not answer // Only a real change is written: opening a project must not rewrite its file. const live = (base, slots) => ({ ...base, slots: JSON.parse(JSON.stringify(slots || [])), removed: pipeline.removedIds(), unwired: pipeline.unwiredMap(), whole: pipeline.whole() }); const store = (base, slots) => { @@ -15,7 +15,7 @@ export function linkPipeline({ project, pipeline, onOpen, say = () => {} }) { async function adopt() { clearTimeout(retry); // a retry still waiting is for a project that may no longer be open - waiting = false; + waiting = false; replaceOnEdit = false; if (pipeline.settled) await pipeline.settled(); // an edit on its way lands in the project it was made in first const here = project.project && project.project.models; // Undo / Redo of something else: the same project, its pipeline already live, nothing to put in (or to lose). @@ -42,21 +42,28 @@ export function linkPipeline({ project, pipeline, onOpen, say = () => {} }) { retry = setTimeout(adopt, 15000); if (retries === 4) say("ComfyUI is not answering, so this project's pipeline is not loaded yet: it goes in as soon as ComfyUI answers."); } else { - say("This project's saved pipeline could not be loaded; the default is in use."); + replaceOnEdit = true; + say(refused()); } } pipeline.subscribe((slots) => { if (waiting === "load" && owner === null) { retries = 0; adopt(); return; } + // Refused for good: the person's next edit of the stand-in pipeline becomes this project's pipeline. + if (replaceOnEdit && owner === null && project.project) { replaceOnEdit = false; owner = project.project.id; } if (!project.project || project.project.id !== owner) return; store(project.project.models || {}, slots); }); onOpen(() => { retries = 0; return adopt(); }); // each project opened gets its own retries // Whether the live pipeline is the open project's (false while it is going in, or ComfyUI has not answered). + const refused = () => { + const reason = pipeline.refusal && pipeline.refusal(); + return `ComfyUI refused this project's saved pipeline${reason ? ` (${reason})` : ""}, so the default is in use. Your next pipeline edit replaces the saved one; or open the project again to retry.`; + }; const owns = () => !!project.project && owner === project.project.id; /** Why the open project's pipeline is not live, in words for the person; null when it is. */ const why = () => owns() ? null : waiting ? "ComfyUI is not answering, so this project's pipeline is not loaded yet. It goes in as soon as ComfyUI answers." - : retries > 3 ? "This project's saved pipeline could not be loaded. Open the project again to retry, or rebuild it in Settings ▸ Models & Pipeline." + : retries > 3 ? refused() : "This project's pipeline is still loading. Try again in a moment."; return { adopt, owns, why }; } diff --git a/app/ui/shell/pipeline_state.js b/app/ui/shell/pipeline_state.js index 8b65e808..08497111 100644 --- a/app/ui/shell/pipeline_state.js +++ b/app/ui/shell/pipeline_state.js @@ -74,6 +74,7 @@ export function createPipelineState(rawApi, { answerMs = ANSWER_MS } = {}) { modulesById = {}; (manifest.modules || []).forEach((m) => { modulesById[m.id] = m; }); controlState = manifest.control || controlState; + if (Array.isArray(manifest.sinks)) sinks = manifest.sinks; if (traits !== undefined) lastProbedFile = file; // only once it worked: a failed probe is tried again loadError = null; // the pipeline is in and its modules are known: nothing is failing now return true; @@ -87,6 +88,7 @@ export function createPipelineState(rawApi, { answerMs = ANSWER_MS } = {}) { let removed = new Set(); // default slots the person took out of this project's pipeline let adoptGate = null; // a promise while a project's pipeline is being put in let whole = false; // the pipeline replaced the default (a preset, an import, the wizard) rather than editing it + let refusalLast = null; // the server's own reason for refusing the last project's pipeline let unreachableLast = false; // the last project's pipeline failed because a request did not get through, not because it was refused let adopting = 0; // adopts under way (waiting for the load, or putting a pipeline in) let deferred = null; // a project's saved pipeline ([slots, removed, unwired, whole]) waiting for the first successful load @@ -104,17 +106,21 @@ export function createPipelineState(rawApi, { answerMs = ANSWER_MS } = {}) { // plain object -- and only accepts a decoded key that names a module this // session already knows is installed, so an unrelated node's incidentally // JSON-shaped string can't inject a bogus entry that round-trips forever. + // Once the server has named its sinks, only those are read, and EVERY module's entry in them is kept: a module the + // current model hides still has its values there, and writing the tree back must not drop them. + let sinks = null; // [{node, input}] from the manifest; null until known (then the scan below is the fallback) + const isSink = (slot, key) => sinks.some((k) => k.node === slot.node && k.input === key); function valuesAlreadyPlaced(from = slots) { const known = new Set([...Object.keys(modulesById), "_off"]); // _off: the modules this project turned off const merged = {}; (from || []).forEach((slot) => { - Object.values(slot.inputs || {}).forEach((v) => { - if (typeof v !== "string") return; + Object.entries(slot.inputs || {}).forEach(([key, v]) => { + if (typeof v !== "string" || (sinks && !isSink(slot, key))) return; let parsed; try { parsed = JSON.parse(v); } catch (_) { return; } if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) return; Object.entries(parsed).forEach(([moduleId, own]) => { - if (!known.has(moduleId)) return; + if (!sinks && !known.has(moduleId)) return; if (own && typeof own === "object") merged[moduleId] = { ...(merged[moduleId] || {}), ...own }; }); }); @@ -313,6 +319,7 @@ export function createPipelineState(rawApi, { answerMs = ANSWER_MS } = {}) { const known = new Set([...Object.keys(modulesById), "_off"]); (slots || []).forEach((slot) => { Object.entries(slot.inputs || {}).forEach(([key, v]) => { + if (sinks) { if (isSink(slot, key) && !Array.isArray(v)) slot.inputs[key] = JSON.stringify(values); return; } // a fresh "{}" too if (typeof v !== "string") return; let parsed; try { parsed = JSON.parse(v); } catch (_) { return; } @@ -578,7 +585,7 @@ export function createPipelineState(rawApi, { answerMs = ANSWER_MS } = {}) { groupEdits = {}; pendingBody = null; pending = false; for (let waited = 0; saving && waited < 10000; waited += 20) await new Promise((r) => setTimeout(r, 20)); - let unreachable = false; + let unreachable = false, reasons = []; for (const extras of [true, false]) { try { const res = await API.editPipeline({ slots: lay(extras) }); @@ -592,12 +599,15 @@ export function createPipelineState(rawApi, { answerMs = ANSWER_MS } = {}) { try { await refreshManifest(); } catch (_) { /* the next save retries it */ } return true; } + reasons = (res && res.refused) || reasons; } catch (e) { // No answer, or a gateway's (a tunnel, a restarting ComfyUI): try again later. An answer refusing it is a refusal. if (!e || !e.status || e.status >= 502) unreachable = true; + else reasons = [e.message]; } } unreachableLast = unreachable; + refusalLast = reasons.length ? String(reasons[0]) : null; // Nothing saved was accepted (a failed request, or a file the server refuses): the default // runs, said, and the caller must NOT save it over the project's own copy. slots = JSON.parse(JSON.stringify(offered)); @@ -622,7 +632,7 @@ export function createPipelineState(rawApi, { answerMs = ANSWER_MS } = {}) { frozenInputs, ensureLoaded, save, edit, restore, snapshot, setGroup, adopt, settled, subscribe: (fn) => { listeners.add(fn); return () => listeners.delete(fn); }, valuesAlreadyPlaced, currentValues, setModuleValue, modulesById: () => modulesById, - unreachable: () => slots === null || unreachableLast, activeModules, useful, usefulness, isOff, setOff, allOff, setAllOff, refreshControl, control: () => controlState, + unreachable: () => slots === null || unreachableLast, refusal: () => refusalLast, activeModules, useful, usefulness, isOff, setOff, allOff, setAllOff, refreshControl, control: () => controlState, removedIds: () => [...removed], whole: () => whole, unwiredMap: () => JSON.parse(JSON.stringify(unwired)), diff --git a/app/ui/shell/tests/pipeline_link.test.js b/app/ui/shell/tests/pipeline_link.test.js index 937ecd78..143463ff 100644 --- a/app/ui/shell/tests/pipeline_link.test.js +++ b/app/ui/shell/tests/pipeline_link.test.js @@ -188,3 +188,20 @@ test("a project whose pipeline could not get through mid-session keeps being ret heard([{ ...slots[0], inputs: { ckpt_name: "edited" } }]); assert.deepStrictEqual(writes, ["edited"]); }); + +test("a pipeline ComfyUI refuses says the server's reason, and the person's next edit becomes the project's pipeline", async (t) => { + t.mock.timers.enable({ apis: ["setTimeout"] }); + const writes = [], said = []; + let opened, heard; + const project = { project: { id: "a", models: { slots, removed: [], unwired: {}, whole: false } }, setField: (k, v) => { writes.push(v.slots[0].inputs.ckpt_name); project.project[k] = v; } }; + const pipeline = { adopt: async () => false, slots: () => slots, unreachable: () => false, refusal: () => "slot 0 role 0 has no input", + removedIds: () => [], unwiredMap: () => ({}), whole: () => false, subscribe: (fn) => { heard = fn; } }; + const link = linkPipeline({ project, pipeline, onOpen: (fn) => { opened = fn; }, say: (x) => said.push(x) }); + const flush = async () => { for (let i = 0; i < 5; i++) await new Promise((r) => setImmediate(r)); }; + await opened(); + for (let i = 0; i < 4; i++) { t.mock.timers.tick(6000); await flush(); } + assert.match(said.join(" "), /refused .*slot 0 role 0 has no input/); + assert.match(link.why(), /refused/); + heard([{ ...slots[0], inputs: { ckpt_name: "fixed" } }]); + assert.deepStrictEqual(writes, ["fixed"]); +}); diff --git a/app/ui/shell/tests/pipeline_state.test.js b/app/ui/shell/tests/pipeline_state.test.js index 0cc6ed6a..127d38ce 100644 --- a/app/ui/shell/tests/pipeline_state.test.js +++ b/app/ui/shell/tests/pipeline_state.test.js @@ -507,3 +507,28 @@ test("settings can be read from a frozen pipeline, apart from the live one and e assert.deepStrictEqual([PS.currentValues(frozen).continuity.carry, PS.allOff(frozen)], [true, false]); assert.deepStrictEqual([PS.currentValues().continuity.carry, PS.allOff()], [false, true]); }); + +const withSink = (blob, mods) => { + const PS = load([]); + lastApi.pipeline = async () => ({ slots: [{ id: "settings", node: "Sink", inputs: { settings: blob } }, { id: "other", node: "X", inputs: { note: '{"sharpen": {"amount": 9}}' } }], incomplete: [], refused: [], queueable: true }); + lastApi.modules = async () => ({ modules: mods, sinks: [{ node: "Sink", input: "settings" }], control: {} }); + return PS; +}; +const sharpen = { id: "sharpen", settings: { enabled: { type: "bool", default: false }, amount: { type: "float", default: 0.5 } } }; + +test("a first setting on a fresh pipeline is in its settings slot at once, so a failed save cannot leave the run on old values", async () => { + const PS = withSink("{}", [sharpen]); + await PS.ensureLoaded(); + lastApi.editPipeline = async () => { throw new Error("boom"); }; + await PS.setModuleValue("sharpen", "enabled", true); + assert.strictEqual(JSON.parse(PS.slots().find((s) => s.id === "settings").inputs.settings).sharpen.enabled, true); + assert.strictEqual(PS.slots().find((s) => s.id === "other").inputs.note, '{"sharpen": {"amount": 9}}'); // not a sink: never read or written +}); + +test("settings of a module the current model hides are kept when another setting is changed", async () => { + const PS = withSink(JSON.stringify({ reins: { enabled: true } }), [sharpen]); + await PS.ensureLoaded(); + await PS.setModuleValue("sharpen", "enabled", true); + const blob = JSON.parse(PS.slots().find((s) => s.id === "settings").inputs.settings); + assert.deepStrictEqual([blob.reins, blob.sharpen.enabled, PS.currentValues().sharpen.amount], [{ enabled: true }, true, 0.5]); +}); diff --git a/core/graph.py b/core/graph.py index 4f30881c..c75c8934 100644 --- a/core/graph.py +++ b/core/graph.py @@ -266,7 +266,7 @@ def build(slots: Sequence[dict], schemas: Optional[Schemas] = None) -> Tuple[dic declared = schemas.inputs(class_type) limits = schemas.limits(class_type) - inputs = {} + inputs, refused_here = {}, set() for name, value in (slot.get("inputs") or {}).items(): if name not in declared: problems.append(f"{slot_id}: {class_type} has no input {name!r}") @@ -297,10 +297,11 @@ def build(slots: Sequence[dict], schemas: Optional[Schemas] = None) -> Tuple[dic label = next((r.get("label") for r in slot.get("roles") or [] if isinstance(r, dict) and r.get("input") == name and r.get("label")), None) problems.append(unacceptable(label, value, limits.get(name)) if label else f"{slot_id}.{problem}") + refused_here.add(name) continue inputs[name] = value for name in schemas.required(class_type): - if name not in inputs: + if name not in inputs and name not in refused_here: # a refused value was just said; "nothing fills it" would be false problems.append(f"{slot_id}: {class_type} needs {name!r} and nothing " f"fills it") diff --git a/core/routes.py b/core/routes.py index c1ee927d..11440ae7 100644 --- a/core/routes.py +++ b/core/routes.py @@ -115,6 +115,20 @@ def modules(rescan=False): return registry_mod.current(rescan) +def settings_sinks(): + """Every place a module says the app's settings can be put: [{node, input}].""" + found = [] + for _spec, make in modules().providers("settings_sink"): + try: + sink = make() + except Exception as exc: # noqa: BLE001 + log.failed("settings_sink", exc) + continue + if isinstance(sink, dict) and sink.get("node") and sink.get("input"): + found.append({"node": sink["node"], "input": sink["input"]}) + return found + + def manifest(traits=None): """What the browser is told: only modules that loaded and validated. @@ -146,6 +160,8 @@ def manifest(traits=None): ], # Which modules the person can switch, and which are quarantined after a fault. "control": control_mod.state(ordered), + # Where the settings live in a pipeline: the app reads and writes them there, and nowhere else. + "sinks": settings_sinks(), } @@ -200,18 +216,7 @@ def register(routes, prefix=None): return make() return [] - def _sinks(): - """Every place a module says the app's settings can be put.""" - found = [] - for _spec, make in modules().providers("settings_sink"): - try: - sink = make() - except Exception as exc: # noqa: BLE001 - log.failed("settings_sink", exc) - continue - if isinstance(sink, dict): - found.append(sink) - return found + _sinks = settings_sinks # A module's own routes live under its id, so two modules can never collide # and turning one off takes its routes with it on the next start. diff --git a/core/tests/test_graph.py b/core/tests/test_graph.py index 96af6b84..5f9c9169 100644 --- a/core/tests/test_graph.py +++ b/core/tests/test_graph.py @@ -272,6 +272,7 @@ def test_a_combo_value_that_is_no_longer_on_disk_is_refused_before_queueing(): _schemas_with_limits()) assert any("gone.safetensors" in p and "not one of" in p for p in problems), problems assert any("model.ckpt_name" in p for p in problems), problems + assert not any("nothing fills it" in p for p in problems), problems # refused, not missing: said once, truly def test_a_combo_value_that_is_on_disk_is_accepted(): @@ -302,6 +303,7 @@ def test_a_value_the_app_writes_is_refused_by_the_name_the_person_sees(): "inputs": {"ckpt_name": "a.safetensors", "steps": 1000}}], _schemas_with_limits()) assert any(p.startswith("Length is 1000, above the largest") for p in problems), problems + assert not any("nothing fills it" in p for p in problems), problems # the refused value was said; it is not "missing" def test_a_bool_is_not_measured_against_a_number_bound(): diff --git a/core/tests/test_schema.py b/core/tests/test_schema.py index e7143920..eb264f22 100644 --- a/core/tests/test_schema.py +++ b/core/tests/test_schema.py @@ -312,3 +312,8 @@ def test_uses_when_must_name_a_declared_setting(): with pytest.raises(SchemaError, match="uses_when"): validate(announce(uses=["taste_store"], uses_when={"nope": "x"})) assert validate(announce(uses=["taste_store"], uses_when={"enabled": True})).to_manifest()["uses_when"] == {"enabled": True} + + +def test_the_manifest_names_where_settings_live(comfyui): + from core import routes + assert {"node": "FunPackModifierSettings", "input": "settings"} in routes.manifest()["sinks"]