refactor: expand a library's imports into manifest membership

Imports are followed when a library is added rather than resolved again on
every validation, so a manifest is an explicit and complete list: what
library_names shows is exactly what the model is compiled and validated
against, with no resolution step in between that could add or drop a
graph.

    model.manifest.add(shapes_lib)      # owl:imports Brick
    model.manifest.library_names
    # ['https://brickschema.org/schema/1.4/Brick', 'urn:my/shapes']

add() takes import_depth with OntoEnv's own meaning -- -1 (default) for
the full closure, 0 for the named library alone, 1 for it and its direct
imports. For a library OntoEnv knows the names come from list_closure, a
name lookup rather than a graph build; for one it does not know (a
directory-loaded library, named after its directory) the same walk runs
over that library's own shape collection, which is where its owl:imports
live. Removal does not cascade: dropping the shapes library leaves Brick
a member, because the manifest is a flat set that reads as what it is.

This replaces the manifest-rooted closure from the previous commit.
Manifest.register() and imports_closure() are gone, and with them the
write-on-a-read-path -- validating no longer registers the manifest graph
with OntoEnv. Manifest.shapes_graph() is now the union of the members'
shape collections, so validation sees each library's *own* stored graph,
including whatever SHACL inference added when it was loaded, rather than
OntoEnv's separately stored copy of the same ontology. That divergence
measured zero triples on every library at hand, but it is now structurally
impossible rather than merely unobserved.

shapes_graph() still asks OntoEnv the one question resolve_imports always
asked -- is anything imported here unaccounted for? -- once over the union
instead of once per collection. Expansion normally makes the answer no; it
catches a library reloaded with new imports since, or one added with
import_depth=0. It raises OntologyImportsNotFound unless
error_on_missing_imports=False.

Consequence worth knowing: compile() now infers against everything the
members import, because those are members. The previous commit deliberately
kept them out of the inference input; complete membership makes that
distinction unavailable, which is the point of the design.

Full unit suite: 572 passed, 1 skipped.

Author: gtfierro

.agents/skills/buildingmotif/references/validation.md

@@ -394,11 +394,11 @@ Two things in this script that trip people up:
   validate against the manifest plus extras, spread `model.manifest.shape_collections()`
   into the list. `model.validate()` with no list validates against the manifest alone —
   that is, against the shape collections of every library the manifest names.
-- **The no-argument form is also the faster one.** A manifest is an ontology whose
-  `owl:imports` name its members, so `model.validate()` resolves them as a single OntoEnv
-  closure rooted at the manifest instead of resolving each collection's imports separately
-  — flat in the number of members rather than linear. Passing an explicit list gives up
-  that path, since a bare list has no manifest to root a closure at.
+- **The no-argument form does no import resolution at all.** A manifest expands a
+  library's `owl:imports` into members when you add it, so `model.validate()` just unions
+  the members' shape collections. Passing an explicit list carries no such guarantee, so
+  that path still resolves each collection's imports. `model.manifest.library_names` is
+  therefore the complete list of what a no-argument `validate()` checks against.
 - **`error_on_missing_imports=False` is the notebooks' default for real models.** A real
   model's shapes usually `owl:imports` something you haven't loaded; with `True` (the
   default) validation raises and stops. `False` gets you a report now — but always check

.agents/skills/buildingmotif/references/writing_shapes.md

@@ -238,6 +238,13 @@ environment (cache first, then a fetch if it is a URL), so
 `model.manifest.add("https://brickschema.org/schema/1.4/Brick")` loads Brick if it has
 to; a name that resolves nowhere raises rather than failing later at validation time.
 
+Adding a library also adds what it **imports**, transitively, as members of their own, so
+`library_names` is the complete list of what the model is validated and compiled against —
+there is no resolution step at validation time that could add or drop a graph.
+`add(lib, import_depth=0)` records just the library named; `import_depth` otherwise takes
+OntoEnv's meaning (`-1` full closure, `1` direct imports). Removal does not cascade: drop
+the shapes library and Brick stays a member until you remove it too.
+
 Storage-wise the manifest is a graph of `owl:imports` and nothing else —
 `model.manifest.graph` hands you a copy to serialize or diff.
 

buildingmotif/dataclasses/compiled_model.py

@@ -51,10 +51,10 @@ def __init__(
             ``Model.validate`` both already take ``Optional[str]``.
         :type shacl_engine: Optional[str]
         :param manifest: the manifest ``shape_collections`` came from, when it
-            came from one. :py:meth:`validate` then resolves imports as a
-            single OntoEnv closure rooted at the manifest rather than once per
-            collection. It is held rather than resolved here because compiling
-            does not need the imports at all -- only validating does.
+            came from one. :py:meth:`validate` then takes the shapes graph
+            straight from its members, since a manifest already lists
+            everything they import. It is held rather than resolved here
+            because the graph is only needed to validate, not to compile.
         :type manifest: Optional[Manifest]
         """
         self.model = model
@@ -214,12 +214,11 @@ def validate(
             )
         backend = get_shacl_backend(shacl_engine)
 
-        # One closure rooted at the manifest, rather than one resolve_imports
-        # per collection: the manifest names every collection as an
-        # owl:imports, so OntoEnv can do the whole transitive resolution in a
-        # single call and deduplicate it on the way.
+        # A manifest's members already include everything they import --
+        # add() followed the imports when they were added -- so the shapes
+        # graph is their union, with no import resolution at validation time.
         resolved_shapes = (
-            self.manifest.imports_closure(error_on_missing=error_on_missing_imports)
+            self.manifest.shapes_graph(error_on_missing=error_on_missing_imports)
             if self.manifest is not None
             else None
         )

buildingmotif/dataclasses/manifest.py

@@ -254,10 +254,20 @@ def __repr__(self) -> str:
         return f"Manifest({self.uri}, {self.library_names})"
 
     def add(
-        self, *libraries: Union[LibraryRef, Iterable[LibraryRef]], resolve: bool = True
+        self,
+        *libraries: Union[LibraryRef, Iterable[LibraryRef]],
+        resolve: bool = True,
+        import_depth: int = -1,
     ) -> None:
         """Add libraries to this manifest. Adding a member twice is a no-op.
 
+        What a library ``owl:imports`` is pulled in **here**, as members of its
+        own, rather than resolved again on every validation. A manifest is
+        therefore an explicit and complete list: what
+        :py:attr:`library_names` shows is exactly what the model is compiled
+        and validated against, with no resolution step in between that could
+        quietly add or drop a graph.
+
         :param libraries: :py:class:`Library` objects, library names, or
             iterables of either
         :param resolve: if True (default), a name that is not already a loaded
@@ -266,8 +276,14 @@ def add(
             the active BuildingMOTIF permits fetching -- and loaded as a
             library. A name that resolves nowhere raises rather than being
             recorded as an import that will fail later. Pass False to record
-            the import without resolving it.
+            the import without resolving it; no expansion happens either, since
+            nothing was loaded to read imports from.
         :type resolve: bool
+        :param import_depth: how far to follow each library's ``owl:imports``,
+            with OntoEnv's own meaning: ``-1`` (default) for the full closure,
+            ``0`` for the named libraries alone, ``1`` for them and what they
+            import directly, and so on.
+        :type import_depth: int
         :raises ManifestLibraryNotFound: if ``resolve`` and a name resolves
             nowhere
         :raises TypeError: if handed something that is not a library or a name
@@ -276,9 +292,12 @@ def add(
         self._ensure_declared()
         for item in _flatten(libraries):
             name = _name_of(item)
-            if resolve and not isinstance(item, Library):
+            names = [name]
+            if resolve:
                 self._resolve_one(name)
-            graph.add((self.uri, OWL.imports, library_iri(name)))
+                names = self._expand(name, import_depth)
+            for member in names:
+                graph.add((self.uri, OWL.imports, library_iri(member)))
 
     def remove(self, *libraries: Union[LibraryRef, Iterable[LibraryRef]]) -> None:
         """Remove libraries from this manifest.
@@ -355,89 +374,110 @@ def shape_collections(self, error_on_missing: bool = True) -> List[ShapeCollecti
             for library in self.resolve(error_on_missing=error_on_missing)
         ]
 
-    def register(self) -> str:
-        """Register this manifest's graph with the ontology environment.
-
-        The manifest *is* an ontology -- a declaration and a list of
-        ``owl:imports`` -- so OntoEnv can resolve it like any other, which is
-        what makes :py:meth:`imports_closure` a single call. Registering is
-        idempotent and overwrites, so it is cheap to redo rather than track
-        whether the manifest changed since last time (the graph is one triple
-        per member).
-
-        :return: the name OntoEnv registered it under, i.e. :py:attr:`uri`
-        :rtype: str
-        """
-        # OntoEnv names a graph by its owl:Ontology declaration, so a manifest
-        # that has never been added to has nothing to register *as*.
-        self._ensure_declared()
-        return self._model._bm.ontology_environment.add(
-            self.graph, fetch_imports=False, overwrite=True
-        )
-
-    def imports_closure(self, error_on_missing: bool = True) -> Graph:
-        """The merged shapes graph this model is validated against.
-
-        One OntoEnv closure rooted at this manifest, rather than resolving each
-        member's imports separately: the manifest declares every member as an
-        ``owl:imports``, so the closure is exactly "every graph this model is
-        checked against", already transitive and already deduplicated.
-
-        Members OntoEnv cannot resolve -- a directory-loaded library, whose
-        name is not an ontology URI, or one built with :py:meth:`Library.create`
-        -- come from their own shape collections instead. Each member is taken
-        from **one** source, never both: OntoEnv's copy of an ontology and the
-        library's shape collection hold the same triples but relabel blank
-        nodes, so unioning them would duplicate every SHACL property shape and
-        report each violation twice.
-
-        :param error_on_missing: if True (default), an import that resolves
-            neither through OntoEnv nor as a library raises, as
-            :py:meth:`ShapeCollection.resolve_imports` has always done. If
-            False they are logged and skipped.
+    def shapes_graph(self, error_on_missing: bool = True) -> Graph:
+        """The merged shapes graph this model is validated and compiled against.
+
+        Simply the union of the member libraries' shape collections. There is
+        no import resolution here, and no round trip through OntoEnv: a
+        library's ``owl:imports`` were followed when it was added
+        (:py:meth:`add`), so every graph that matters is already a member.
+        That also means validation sees each library's **own** stored graph --
+        including whatever SHACL inference added when the library was loaded --
+        rather than a separately stored copy of the same ontology.
+
+        :param error_on_missing: if True (default), raise when a member's
+            ``owl:imports`` names something neither loaded nor resolvable,
+            which is the check :py:meth:`ShapeCollection.resolve_imports` has
+            always made. If False they are logged and skipped.
         :type error_on_missing: bool
         :raises OntologyImportsNotFound: per ``error_on_missing``
         :return: the shapes graph
         :rtype: Graph
         """
-        if not self.imports:
-            # An empty manifest asks nothing of the model. Short-circuit rather
-            # than round-tripping through OntoEnv, which would have no graph to
-            # root a closure at anyway.
-            return Graph()
+        graph = Graph()
+        for shape_collection in self.shape_collections(
+            error_on_missing=error_on_missing
+        ):
+            graph += shape_collection.graph
+        if not len(graph):
+            return graph
+
+        # Ask OntoEnv the same question resolve_imports asks, once over the
+        # union rather than once per collection: is anything imported here
+        # unaccounted for? Expansion at add() time means the answer is normally
+        # no -- this catches a library reloaded with new imports since, or one
+        # added with import_depth=0.
+        missing = self._model._bm.ontology_environment.missing_imports(graph)
+        if missing:
+            if error_on_missing:
+                raise OntologyImportsNotFound(missing)
+            logger.warning(
+                "Manifest of %s: %s is imported but not a member and could not "
+                "be resolved; shapes it defines will not be applied.",
+                self._model.name,
+                ", ".join(sorted(missing)),
+            )
+        return graph
 
-        env = self._model._bm.ontology_environment
-        self.register()
+    def _expand(self, name: str, import_depth: int) -> List[str]:
+        """``name`` plus the libraries it imports, to ``import_depth``.
 
-        closure, _ = env.closure_copy(str(self.uri))
+        OntoEnv answers this for free when it knows the ontology
+        (``list_closure`` is a name lookup, not a graph build). For a member it
+        does not know -- a directory-loaded library, named after its directory
+        -- the same walk is done over the library's own shape collection, which
+        is where its ``owl:imports`` live.
+        """
+        if import_depth == 0:
+            return [name]
 
-        # Whatever the closure could not reach: manifest members OntoEnv does
-        # not know, plus anything transitively imported and missing (which
-        # resolve_imports has always treated as an error).
-        unresolved = []
-        for iri in env.missing_imports(str(self.uri)):
-            name = library_name(URIRef(iri))
+        env = self._model._bm.ontology_environment
+        if env.knows(name):
+            candidates = env.closure_names(name, recursion_depth=import_depth)
+        else:
+            candidates = self._imported_names(name, import_depth)
+
+        names = []
+        for candidate in candidates:
             try:
-                library = self._resolve_one(name)
+                self._resolve_one(candidate)
             except ManifestLibraryNotFound:
-                unresolved.append(iri)
+                # An import that resolves nowhere is not a reason to reject the
+                # library that named it; shapes_graph() reports it if it still
+                # matters at validation time.
+                logger.warning(
+                    "Manifest of %s: %r imports %r, which could not be loaded "
+                    "as a library and is not a member.",
+                    self._model.name,
+                    name,
+                    candidate,
+                )
                 continue
-            closure += (
-                library.get_shape_collection()
-                .resolve_imports(error_on_missing_imports=error_on_missing)
-                .graph
-            )
-
-        if unresolved:
-            if error_on_missing:
-                raise OntologyImportsNotFound(unresolved)
-            logger.warning(
-                "Manifest of %s could not resolve: %s. Validation will not see "
-                "any shapes those graphs define.",
-                self._model.name,
-                ", ".join(sorted(unresolved)),
-            )
-        return closure
+            names.append(candidate)
+        return names
+
+    def _imported_names(self, name: str, import_depth: int) -> List[str]:
+        """:py:meth:`_expand`'s walk for libraries OntoEnv does not know."""
+        found: List[str] = []
+        seen = set()
+        queue = [(name, import_depth)]
+        while queue:
+            current, depth = queue.pop(0)
+            if current in seen:
+                continue
+            seen.add(current)
+            found.append(current)
+            if depth == 0:
+                continue
+            try:
+                library = self._resolve_one(current)
+            except ManifestLibraryNotFound:
+                continue
+            for imported in library.get_shape_collection().graph.objects(
+                None, OWL.imports
+            ):
+                queue.append((str(imported), -1 if depth < 0 else depth - 1))
+        return found
 
     def _ensure_declared(self) -> None:
         """Give the manifest graph an ``owl:Ontology`` declaration if it has none."""

buildingmotif/dataclasses/model.py

@@ -330,11 +330,10 @@ def compile(
             manifest = self.manifest
             shape_collections = manifest.shape_collections()
         backend = get_shacl_backend(shacl_engine or self._bm.shacl_engine)
-        # NB: inference compiles against the member shape collections
-        # themselves, *not* the manifest's imports closure -- what a model
-        # infers from should be the shapes it was compiled against, and pulling
-        # every transitively imported ontology into the inference input would
-        # change what lands in every compiled model.
+        # Compiling against the members is compiling against everything they
+        # import, because a manifest expands imports when a library is added
+        # rather than resolving them here. There is deliberately no separate
+        # resolution step: the explicit membership is the whole story.
         compiled_graph = backend.compile_model_graph(self.graph, shape_collections)
         return CompiledModel(
             self,

buildingmotif/shacl.py

@@ -114,11 +114,11 @@ def _resolved_shape_graph(
     """The shapes graph, with ``owl:imports`` resolved.
 
     ``resolved_shapes`` is the whole answer when it is given: validating
-    against a manifest computes one OntoEnv closure rooted at the manifest
-    (:py:meth:`Manifest.imports_closure`), which is transitive, deduplicated,
-    and one call rather than one per collection. The per-collection path
-    remains for an explicit list of shape collections, which has no manifest to
-    root a closure at.
+    against a manifest takes the shapes graph from
+    :py:meth:`Manifest.shapes_graph`, whose members already include everything
+    they import. The per-collection path remains for an explicit list of shape
+    collections, which carries no such guarantee and so still has to resolve
+    each collection's imports.
     """
     if resolved_shapes is not None:
         return resolved_shapes
@@ -173,9 +173,9 @@ def validation_graphs(
         resolved_shapes: Optional[Graph] = None,
     ) -> ValidationGraphs:
         """
-        :param resolved_shapes: the shapes graph with its ``owl:imports``
-            already resolved -- see :func:`_resolved_shape_graph`. Defaults to
-            resolving each shape collection's imports separately.
+        :param resolved_shapes: a shapes graph that needs no import resolution
+            -- see :func:`_resolved_shape_graph`. Defaults to resolving each
+            shape collection's imports separately.
         :type resolved_shapes: Optional[Graph]
         """
         from buildingmotif.utils import (