diff --git a/app/boot.js b/app/boot.js index 908c5af8..dc4b10bb 100644 --- a/app/boot.js +++ b/app/boot.js @@ -29,12 +29,21 @@ async function start() { } const { mounted, hidden } = await mountAll(manifest); - console.info(`[FunPack] ${mounted.length} module(s) mounted`, - hidden.length ? `· ${hidden.length} hidden` : ""); - for (const { id, why } of hidden) console.warn(`[FunPack] ${id} is hidden: ${why}`); + const failed = manifest.failed || []; - // Handy while there is no generate button to send them anywhere. - window.FunPack = { manifest, values: allValues, mounted: mounted.map((m) => m.id), hidden }; + console.info(`[FunPack] ${mounted.length} module(s) mounted`, + hidden.length ? `· ${hidden.length} hidden` : "", + failed.length ? `· ${failed.length} failed to load` : ""); + for (const { id, why } of hidden) console.warn(`[FunPack] ${id} is hidden: ${why}`); + // These never reached the manifest, so no panel could be missing "in a way + // the user notices" -- which is exactly why they have to be said. A module + // that failed to import looks identical to one nobody installed. + for (const { where, why } of failed) console.warn(`[FunPack] ${where} did not load: ${why}`); + + window.FunPack = { + manifest, values: allValues, failed, hidden, + mounted: mounted.map((m) => m.id), + }; } start(); diff --git a/app/catalogue/modules.html b/app/catalogue/modules.html new file mode 100644 index 00000000..4b255767 --- /dev/null +++ b/app/catalogue/modules.html @@ -0,0 +1,6 @@ + + +FunPack modules +
+ + diff --git a/app/catalogue/modules.js b/app/catalogue/modules.js new file mode 100644 index 00000000..3548ab74 --- /dev/null +++ b/app/catalogue/modules.js @@ -0,0 +1,85 @@ +// What is installed, what announced itself, and what did not make it. +// +// The plan called this a required tool rather than a nicety, and it was right: +// a module that fails to import is ABSENT, which is the correct behaviour and +// also indistinguishable from one nobody installed. Somewhere has to say the +// difference out loud, and it must not be the main app, where an absent feature +// is supposed to be simply absent. + +import { composer } from "../composer/composer.js"; +import { fetchManifest } from "../shell/manifest.js"; +import { build } from "../shell/layout.js"; +import { offered, _reset } from "../shell/mounts.js"; + +const root = document.querySelector("#app"); + +function rows(items) { + return composer.list.rows({ items, reorder: false }); +} + +async function start() { + let manifest; + try { + manifest = await fetchManifest(); + } catch (err) { + root.replaceChildren(composer.emptyState.default({ + icon: "▲", title: "Could not reach FunPack", hint: err.message, + }).node); + return; + } + + // Build the real shell off-screen purely to learn which mount points exist. + // Asking the layout is the only honest answer: nothing else knows. + _reset(); + build(document.createElement("div")); + const available = new Set(offered()); + + const modules = manifest.modules || []; + const failed = manifest.failed || []; + const unclaimed = modules.filter( + (m) => Object.keys(m.settings || {}).length && m.mount && !available.has(m.mount)); + + const sections = []; + + sections.push(composal("Loaded", `${modules.length} module(s) announced themselves.`, + modules.map((m) => ({ + id: m.id, + label: m.id, + hint: `${m.stage} · ${m.status}` + + (m.mount ? ` · ${m.mount}` : " · no panel") + + (Object.keys(m.settings || {}).length ? ` · ${Object.keys(m.settings).length} setting(s)` : ""), + })))); + + if (unclaimed.length) { + sections.push(composal( + "Asking for a mount point nothing offers", + "These loaded correctly and their panels will not appear anywhere. " + + `Offered: ${[...available].join(", ")}.`, + unclaimed.map((m) => ({ id: m.id, label: m.id, hint: `wants "${m.mount}"` })))); + } + + if (failed.length) { + sections.push(composal("Did not load", "Absent from the app, and from the graph.", + failed.map((f, i) => ({ id: `f${i}`, label: f.where, hint: f.why })))); + } + + if (manifest.incompatible?.length) { + sections.push(composal("Not for this model", "Filtered out by what the model is.", + manifest.incompatible.map((m) => ({ + id: m.id, label: m.id, hint: `needs ${m.requires.join(", ")}` })))); + } + + root.replaceChildren(...sections.map((s) => s.node)); +} + +function composal(title, hint, items) { + return composer.panel.default({ + title, + body: items.length + ? composer.group.default({ children: [ + composer.hint.default({ text: hint }), rows(items)] }) + : composer.hint.default({ text: "None." }), + }); +} + +start(); diff --git a/app/shell/layout.js b/app/shell/layout.js index 34dcf746..b864f39f 100644 --- a/app/shell/layout.js +++ b/app/shell/layout.js @@ -28,8 +28,11 @@ export function build(root) { // The mount point IS the contract with modules; the region behind it can be // rearranged freely as long as the name survives. offer("settings.general", general.body); - offer("generation.timing", generation.body); offer("generation.model", generation.body); + offer("generation.latent", generation.body); + offer("generation.sampling", generation.body); + offer("generation.timing", generation.body); + offer("generation.post", generation.body); return { general, generation, page }; } diff --git a/core/tests/test_module_imports.py b/core/tests/test_module_imports.py new file mode 100644 index 00000000..bb811273 --- /dev/null +++ b/core/tests/test_module_imports.py @@ -0,0 +1,59 @@ +"""No module may reach for `core` by absolute name. + +ComfyUI loads a pack by file location and never puts its directory on sys.path, +so `from core import ...` inside a module resolves under pytest and finds nothing +under ComfyUI. This has now cost the same bug twice: once at module level, where +the node quietly failed to register, and once INSIDE A FUNCTION BODY, where it +imported fine, registered fine, passed every test, and raised at execute time -- +during a real generation. + +That second one is why this is a static check and not a runtime one. A test that +imports every module cannot see an import that only happens when a node runs. +""" + +import ast +from pathlib import Path + +import pytest + +MODULES = Path(__file__).resolve().parents[2] / "modules" +SHIM = "_core.py" + + +def _module_files(): + return [p for p in MODULES.rglob("*.py") + if p.name != SHIM and "/tests/" not in str(p) and p.parent.name != "tests"] + + +def _absolute_core_imports(path: Path): + """Every `import core...` / `from core... import` anywhere in the file. + + Walks the AST rather than the text, so an import nested in a function, a + method or a try block is found exactly like a top-level one. + """ + tree = ast.parse(path.read_text(), filename=str(path)) + found = [] + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom): + # level > 0 is relative, which is what the shim exists to provide. + if node.level == 0 and (node.module or "").split(".")[0] == "core": + found.append((node.lineno, f"from {node.module} import ...")) + elif isinstance(node, ast.Import): + for alias in node.names: + if alias.name.split(".")[0] == "core": + found.append((node.lineno, f"import {alias.name}")) + return found + + +def test_every_module_file_exists(): + assert _module_files(), "found no module files to check" + + +@pytest.mark.parametrize("path", _module_files(), ids=lambda p: str(p.relative_to(MODULES))) +def test_a_module_reaches_core_only_through_the_shim(path): + offences = _absolute_core_imports(path) + assert not offences, ( + f"{path.relative_to(MODULES)} imports core by absolute name at " + + ", ".join(f"line {line} ({what})" for line, what in offences) + + ". Under ComfyUI there is no top-level `core`. Use `from ..._core import ...`." + ) diff --git a/modules/_core.py b/modules/_core.py index 76b26847..cc910ccf 100644 --- a/modules/_core.py +++ b/modules/_core.py @@ -11,8 +11,8 @@ leading underscore keeps it out of the module scan. """ try: # root on sys.path: tests, dev server - from core import log, registry, schema, traits + from core import contract, log, registry, relations, schema, traits except ImportError: # inside ComfyUI: a subpackage of the pack - from ..core import log, registry, schema, traits + from ..core import contract, log, registry, relations, schema, traits -__all__ = ["log", "registry", "schema", "traits"] +__all__ = ["contract", "log", "registry", "relations", "schema", "traits"] diff --git a/modules/sampling/modifiers/nodes.py b/modules/sampling/modifiers/nodes.py index e25f924c..51aa8f78 100644 --- a/modules/sampling/modifiers/nodes.py +++ b/modules/sampling/modifiers/nodes.py @@ -28,7 +28,8 @@ import json from comfy_api.latest import io -from ..._core import log, registry as registry_mod, schema as schema_mod +from ..._core import (log, registry as registry_mod, relations as relations_mod, + schema as schema_mod) CAPABILITY = "modifier" @@ -139,7 +140,7 @@ class FunPackLoadModifiers(io.ComfyNode): @classmethod def execute(cls, model, settings=None) -> io.NodeOutput: from ..._core import traits as traits_mod - from core.relations import order + order = relations_mod.order if settings is not None and not isinstance(settings, dict): raise RuntimeError( diff --git a/tools/devserver.py b/tools/devserver.py index 37e2b444..2d3b022c 100644 --- a/tools/devserver.py +++ b/tools/devserver.py @@ -14,6 +14,58 @@ from urllib.parse import parse_qs, unquote, urlparse sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +def _add_comfyui() -> str: + """Put ComfyUI on the path, because most modules import it. + + Without this the dev server loads only the modules that need nothing from + ComfyUI -- one out of nine -- and the app looks almost empty while reporting + nothing wrong. Looking at the app is supposed to be how the app gets tested, + so the preview has to see what ComfyUI would see. + """ + import os + candidates = [os.environ.get("COMFYUI_ROOT"), + str(Path.home() / "Documents" / "ComfyUI")] + for candidate in candidates: + if candidate and (Path(candidate) / "comfy" / "supported_models.py").is_file(): + sys.path.append(candidate) + return candidate + return "" + + +def _reexec_with_comfyui_python(comfyui: str) -> None: + """Restart under ComfyUI's own interpreter when this one cannot import torch. + + Finding ComfyUI's SOURCE is not enough: the modules import torch, and the + interpreter that launches this may be a bare system python. Getting that + wrong is not a crash, it is nine modules quietly absent and an app that + looks almost empty -- so the server corrects it rather than serving a + misleading page. + """ + import os + if os.environ.get("FUNPACK_DEVSERVER_REEXEC"): + return # already tried; do not loop + try: + import torch # noqa: F401 + return + except ImportError: + pass + + candidate = Path(comfyui) / "venv" / "bin" / "python" + if not candidate.is_file(): + print(f" WARNING: {sys.executable} cannot import torch and there is no " + f"venv at {candidate}. Modules needing it will be absent.") + return + + print(f" re-running under {candidate} (this one has no torch)") + os.environ["FUNPACK_DEVSERVER_REEXEC"] = "1" + os.execv(str(candidate), [str(candidate), *sys.argv]) + + +COMFYUI = _add_comfyui() +if COMFYUI: + _reexec_with_comfyui_python(COMFYUI) + from core import config, routes, serve as static # noqa: E402 P = config.UI_PREFIX @@ -60,4 +112,10 @@ class Handler(BaseHTTPRequestHandler): if __name__ == "__main__": port = int(sys.argv[1]) if len(sys.argv) > 1 else 8188 print(f"FunPack v5 dev server: http://127.0.0.1:{port}{P}/") + if COMFYUI: + print(f" with ComfyUI from {COMFYUI}") + else: + # Said out loud rather than left to be discovered as a half-empty page. + print(" WITHOUT ComfyUI: every module that imports it will be absent. " + "Set COMFYUI_ROOT to fix.") HTTPServer(("127.0.0.1", port), Handler).serve_forever()