Skip to content

feat(architectures): add the architecture facet registry - #27

Draft
Pfannkuchensack wants to merge 2 commits into
mainfrom
refactor/arch-facet-registry
Draft

feat(architectures): add the architecture facet registry#27
Pfannkuchensack wants to merge 2 commits into
mainfrom
refactor/arch-facet-registry

Conversation

@Pfannkuchensack

Copy link
Copy Markdown
Collaborator

First of a short series that moves per-architecture facts out of core dispatch chains and into one
registry. Spec: .ideas/Backend Modularization Plan.md.

This PR adds no behaviour. It is the scaffolding the following PRs land on, plus the tests that
make an incompletely registered architecture a CI failure rather than a runtime one.

Why

Adding a BaseModelType costs ~16 edited core files. The count is not the problem — the failure
mode is. Most of those edits, when forgotten, fail at generation time, not at boot: a missing
step_callback branch raises Unsupported base model on the first preview, an unregistered
*ConditioningInfo breaks deserialization mid-graph.

What

invokeai/backend/architectures/register / get / require / generative_bases /
validate, and one defs/<base>.py per architecture. All 15 register an empty facet set; the
facets themselves arrive in the follow-ups.

facet.py ← registry.py ← facets/*.py ← defs/*.py ← __init__.py ← core
  • Facet lives in its own module, not in registry.py. Facet modules need to import the
    registry for their accessors; if Facet were defined in registry.py, the edge
    facets → registry and the edge registry → facets would be one line apart.
  • Which facets are mandatory is declared by the facet classes (Facet.REQUIRED), collected via
    __init_subclass__ — the Config_Base.CONFIG_CLASSES pattern. A registry that knew the required
    set would have to import concrete facets. FACET_TYPES is a dict, not a set:
    CONFIG_CLASSES is a set and its non-deterministic iteration order has bitten this repo before
    (tests/backend/model_manager/configs/test_wan_lora_probe_independence.py).
  • The defs import list is explicit, not a glob, so a missing definition is distinguishable
    from a base that does not exist. This one line is the intended residual per-architecture edit.
  • File names are derived from the enum value (z-imagedefs/z_image.py), so require() and
    validate() can compute the file to edit instead of maintaining a second table.

require() names the file to edit:

Architecture 'z-image' is not registered, so it cannot declare LatentSpaceFacet. Create
invokeai/backend/architectures/defs/z_image.py with a `register(BaseModelType.ZImage,
LatentSpaceFacet(...))` call, then add the module to the import list in
invokeai/backend/architectures/__init__.py.

ArchitectureError subclasses ValueError deliberately: it replaces
raise ValueError(f"Unsupported base model: ...") call sites, so existing except ValueError
handlers are unaffected.

Boot validation

validate() raises, unlike the invocation-output check it sits beside in run_app.py — that
one warns because it checks third-party node packs, which may legitimately be broken. Architectures
are first-party and the enum is closed.

It is also called from ApiDependencies.initialize(), covering embedders that never go through
run_app.py. The architectures import in dependencies.py is at module level on purpose:
importing that module is what populates the registry, and add_safe_globals mutates torch
process-globally, so the registry must be complete before initialize() runs — not merely before
it is first read. test_import_isolation pins that in a fresh interpreter.

Honest scope

With no facet declaring itself required yet, validate() is structurally sharp and semantically
empty
. The load-bearing gate in this PR is test_registry_completeness, which holds the only copy
of set(BaseModelType) - {Any, External, Unknown} in the tree — production code asks the registry,
because registration is the definition — and checks the defs directory against the registered
set in both directions. That catches a definition module that exists but is never imported and
therefore silently registers nothing.

test_layering

tests/test_imports.py cannot catch import-direction problems: it imports every module into one
shared process, where an order-dependent cycle passes. This parses the sources instead. Two rules
earn their keep — a defs module importing the aggregate is a circular import onto a partially
initialised module (and is the natural thing for a contributor to write), and core reaching past the
facade quietly makes the public surface whatever anyone happened to import.

The allowlist is deliberately narrower than the eventual target, so every widening is a visible line
in the PR that needs it. Imports under if TYPE_CHECKING: count — a type-only edge is still an
architectural edge.

The self-tests are the point: a walker with a bug reports zero violations and stays green forever,
which is this test's only realistic failure mode. Modelled on
invokeai/frontend/webv2/src/architecture/dependencyPolicy.test.ts, including its named rules.

Verification

  • pytest tests/backend/architectures — 56 passed
  • mypy invokeai/backend/architectures (strict, no override entry in pyproject.toml) — clean
  • ruff@0.11.2 check . + format --check — clean
  • openapi.json regenerated and compared normalized — identical. No Pydantic class, invocation
    field or enum is touched, so schema.ts and docs/src/generated/invocation-context.json are
    unchanged too.
  • Negative probes, both reverted afterwards: an injected defs → core import fails
    test_no_layering_violations with exactly
    defs-allowlist: invokeai.backend.architectures.defs.wan -> invokeai.app.util.step_callback; a
    REQUIRED facet no architecture declares makes validate() report all 15, each naming its file.
  • Full suite: 4180 passed. The 9 failures are HF_ENDPOINT-related — a local HuggingFace mirror
    rewrites the URLs these mock-based download tests register adapters for. They pass with the
    variable unset, on this branch.

🤖 Generated with Claude Code

Adding a BaseModelType costs ~16 edited core files today. The count is not the
problem -- the failure mode is. Most of those edits, when forgotten, fail at
generation time rather than at boot: a missing step_callback branch raises
"Unsupported base model" on the first preview, an unregistered *ConditioningInfo
breaks deserialization mid-graph.

This is the foundation those facts will move onto, one concern per follow-up.
Each architecture registers itself once from its own defs/<base>.py, declaring
the facets it supports; core code will read them through narrow accessors
instead of branching on BaseModelType. No facet types exist yet and no core
behaviour changes: all 15 architectures register an empty facet set.

The 15 defs modules are here rather than in the follow-up because without them
the completeness gate is vacuously green, and because the naming convention --
the enum value with dashes replaced by underscores, derived so require() and
validate() can compute the file to edit rather than maintain a second table --
is better settled before any facet data hangs off it.

Structure. Facet lives in its own module rather than in registry, so facet
modules can import the registry without closing the loop; were Facet defined in
registry.py, the edge facets->registry and the edge registry->facets would be
one line apart. Which facets are mandatory is declared by the facet classes
themselves via Facet.REQUIRED, because a registry that knew the required set
would have to import concrete facets. FACET_TYPES is a dict rather than a set:
Config_Base.CONFIG_CLASSES is a set and its non-deterministic iteration order
has already bitten this codebase once. The defs list in __init__.py is explicit
rather than globbed, so a missing definition is distinguishable from a base that
does not exist.

Boot validation. validate() raises rather than warns, unlike the invocation
check it sits beside: architectures are first-party and the enum is closed, so
an incomplete one is a bug in this repository, not in someone's node pack. The
error names the missing facet and the file to add it to. It is called from
ApiDependencies.initialize() as well, covering every embedder that never goes
through run_app.py. With no required facets yet this is structurally sharp and
semantically empty; it becomes load-bearing with the first REQUIRED facet.

The architectures import in dependencies.py is at module level on purpose and
must stay there. Importing that module is what populates the registry, and
add_safe_globals mutates torch process-globally, so the registry has to be
complete before initialize() runs -- not merely before it is first read.

Tests. test_registry_completeness holds the only copy of
`set(BaseModelType) - {Any, External, Unknown}` in the tree; production code
asks the registry, because registration is the definition. It checks the defs
directory against the registered set in both directions, which catches a
definition module that exists but is never imported and therefore silently
registers nothing.

test_layering enforces the import direction by parsing sources.
tests/test_imports.py cannot: it imports every module into one shared process,
where an order-dependent cycle passes. Two rules earn their keep -- a defs
module importing the aggregate is a circular import onto a partially initialised
module, and is the natural thing for a contributor to write; core reaching past
the facade quietly makes the public surface whatever anyone happened to import.
The allowlist is deliberately narrower than the eventual target so that every
widening is a visible line in the PR that needs it. Imports under
`if TYPE_CHECKING:` count -- a type-only edge is still an architectural edge.
The self-tests are the point: a walker with a bug reports zero violations and
stays green forever, which is this test's only realistic failure mode. Modelled
on webv2/src/architecture/dependencyPolicy.test.ts, including its named rules.

test_import_isolation runs in a fresh interpreter, because in the shared pytest
process every module has already been imported by some other test and the
property holds vacuously.

openapi.json and schema.ts are unchanged: no Pydantic class, invocation field or
enum is touched.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant