Fix circular dependency handling in spec resolution (#1388)

Add tests

Debug errors

Fix and refactor stochastic resolution errors by not using graphlib static_order

Refactor to ensure correct ordering

Test

Author: rly

src/hdmf/spec/namespace.py

@@ -430,49 +430,61 @@ def __collect_nested_subspecs(self, spec: GroupSpec) -> list[BaseStorageSpec]:
             nested_subspecs.extend(self.__collect_nested_subspecs(subgroup_spec))
         return nested_subspecs
 
-    def __get_spec_dependencies(self, spec: BaseStorageSpec) -> set[tuple[str, str]]:
-        """Get the set of edges representing the dependencies of the given spec."""
+    def __get_spec_dependencies(
+        self, spec: BaseStorageSpec, catalog: SpecCatalog
+    ) -> tuple[set[tuple[str, str]], set[tuple[str, str]]]:
+        """Get edges representing the dependencies of the given spec.
+
+        Returns (edges, skipped_edges) where skipped_edges are cycle-creating edges
+        that were excluded from the dependency graph.
+        """
         edges = set()
+        skipped_edges = set()
         if spec.data_type_inc is not None:
-            # The included spec should be resolved before this spec
             edges.add((spec.data_type_def, spec.data_type_inc))
         if isinstance(spec, GroupSpec):
-            # For each nested subspec, the included specs of that nested subspec should be resolved before
-            # this spec
             nested_subspecs = self.__collect_nested_subspecs(spec)
             for subspec in nested_subspecs:
                 if subspec.data_type_inc is not None:
-                    # TODO: cycles are not yet supported
-                    # if spec.data_type_def == subspec.data_type_inc:
-                    #     # Allow the simple case of a "cycle" where A contains B, and B includes A
-                    #     # but do not add this edge to the graph because it makes a cycle.
-                    #     continue
+                    if spec.data_type_def == subspec.data_type_inc:
+                        skipped_edges.add((spec.data_type_def, subspec.data_type_inc))
+                        continue
+                    hierarchy = catalog.get_hierarchy(subspec.data_type_inc)
+                    if spec.data_type_def in hierarchy:
+                        skipped_edges.add((spec.data_type_def, subspec.data_type_inc))
+                        continue
                     edges.add((spec.data_type_def, subspec.data_type_inc))
-        return edges
+        return edges, skipped_edges
+
+    def __resolve_local(
+        self, namespace: SpecNamespace, spec: BaseStorageSpec, resolving_in_progress: set = None
+    ) -> None:
+        """Resolve the data_type_inc of this spec and its children recursively.
+
+        Parameters
+        ----------
+        namespace : SpecNamespace
+            The namespace containing the specs.
+        spec : BaseStorageSpec
+            The spec to resolve.
+        resolving_in_progress : set, optional
+            Set of type names currently being resolved, used to prevent infinite recursion
+            in circular dependencies.
+        """
+        if resolving_in_progress is None:
+            resolving_in_progress = set()
 
-    def __resolve_local(self, namespace: SpecNamespace, spec: BaseStorageSpec) -> None:
         if spec.data_type_inc is not None and not spec.inc_spec_resolved:
-            # NOTE: The included spec may have already been resolved into the current spec if the current spec
-            # was copied (included) from another spec. For example, if A has a subspec B that includes C, and
-            # D includes A, then when resolving D, first, already resolved subspec B is copied from A to D, and
-            # then resolve_local may be called on B again
+            if spec.data_type_inc in resolving_in_progress:
+                return
             included_spec = self.get_spec(namespace.name, spec.data_type_inc)
-
-            # NOTE: In most cases, because we are resolving specs in topological order, the included spec
-            # should have already been resolved. However, in the case of the "cycle" described above where
-            # A contains B, and B includes A, then the included spec will not have been resolved yet.
-
-            # Resolve the included spec into this spec
             spec.resolve_inc_spec(included_spec, namespace)
+            resolving_in_progress = resolving_in_progress | {spec.data_type_inc}
 
         if isinstance(spec, GroupSpec):
-            # Recursively resolve all subspecs
-            nested_subspecs = self.__collect_nested_subspecs(spec)
-            for subspec in nested_subspecs:
-                self.__resolve_local(namespace, subspec)
+            for subspec in list(spec.groups) + list(spec.datasets):
+                self.__resolve_local(namespace, subspec, resolving_in_progress)
 
-        # Mark this spec as resolved if the included spec has been resolved and all subspecs have been resolved.
-        # This is not necessary / not used anywhere, but may be useful for debugging.
         spec.resolved = True
 
     def resolve_all_specs(self) -> None:
@@ -481,38 +493,65 @@ def resolve_all_specs(self) -> None:
             self.__resolve_namespace_specs(namespace)
 
     def __resolve_namespace_specs(self, namespace: SpecNamespace) -> None:
-        """Resolve all specs in the catalog."""
-        # Build a graph of all type dependencies
-        # For example, if A includes B, A has subspec that includes C, and B includes D, then A -> B, A -> C, B -> D
+        """Resolve all specs in the catalog using topological sort order.
+
+        A dependency graph is built from each type's data_type_inc and nested subspec data_type_inc values.
+        Cycle-creating edges are skipped. The graph is then sorted topologically, and types at the same
+        level are sorted alphabetically to ensure deterministic ordering across environments.
+        """
+        # Build dependency graph, skipping edges that would create cycles
         ts = graphlib.TopologicalSorter()
-        specs_without_deps = set()  # track specs that have no dependencies
+        all_type_names = set()
+        all_skipped_edges = set()
         for type_name in namespace.catalog.get_registered_types():
+            all_type_names.add(type_name)
             spec = namespace.catalog.get_spec(type_name)
-            edges = self.__get_spec_dependencies(spec)
-            if not edges:
-                specs_without_deps.add(type_name)
-            else:
-                for e in edges:
-                    ts.add(*e)
-
-        # Check for cycles and get static topological order
-        # For example, in the ABCD example above, the static order is D, B, C, A
-        try:
-            static_order = list(ts.static_order())
-        except graphlib.CycleError:  # pragma: no cover
-            # This should not happen because cycles will cause an error during spec object creation
-            raise ValueError("Cycle detected in specification dependencies. Cannot resolve specifications.")
-
-        # In rare cases, a namespace may have specs that have no dependencies and are not included by any other
-        # spec, so they will not be in the topological sort. Add them to the front of the order.
-        for s in specs_without_deps:
+            edges, skipped_edges = self.__get_spec_dependencies(spec, namespace.catalog)
+            all_skipped_edges.update(skipped_edges)
+            for edge in edges:
+                ts.add(*edge)
+
+        # For types that inherit from a type with skipped (cycle) edges, add dependencies on the
+        # cycle partner. E.g., if DynamicTable -> MeaningsTable was skipped (cycle), and
+        # AlignedDynamicTable extends DynamicTable, add AlignedDynamicTable -> MeaningsTable.
+        # This ensures MeaningsTable is resolved before AlignedDynamicTable, so that when
+        # AlignedDynamicTable inherits DynamicTable's MeaningsTable subspec and re-resolves it,
+        # the catalog MeaningsTable is already fully resolved.
+        # Walk the full inheritance hierarchy to handle longer chains (D extends C extends A).
+        if all_skipped_edges:
+            skipped_deps = dict()
+            for parent, dep in all_skipped_edges:
+                skipped_deps.setdefault(parent, set()).add(dep)
+            for type_name in namespace.catalog.get_registered_types():
+                for ancestor in namespace.catalog.get_hierarchy(type_name):
+                    if ancestor not in skipped_deps:
+                        continue
+                    for dep in skipped_deps[ancestor]:
+                        if dep == type_name:
+                            continue
+                        # Only add if it won't create a new cycle
+                        if type_name not in namespace.catalog.get_hierarchy(dep):
+                            ts.add(type_name, dep)
+
+        # Get topological order using level-by-level processing with alphabetical sorting
+        # within each level to ensure deterministic ordering regardless of insertion order.
+        ts.prepare()
+        static_order = []
+        while ts.is_active():
+            ready = sorted(ts.get_ready())  # alphabetical sort within each level
+            static_order.extend(ready)
+            for node in ready:
+                ts.done(node)
+
+        # Add types that have no dependencies and are not depended on by any other type
+        for s in all_type_names:
             if s not in static_order:
                 static_order.insert(0, s)
 
         # Resolve specs in topological order
         for type_name in static_order:
             spec = self.get_spec(namespace.name, type_name)
-            self.__resolve_local(namespace, spec)
+            self.__resolve_local(namespace, spec, resolving_in_progress={type_name})
 
 
     def __load_namespace(self, namespace, reader):
@@ -558,42 +597,42 @@ def __load_namespace(self, namespace, reader):
 
         return included_types
 
-    def __register_type(self, ndt, inc_ns, catalog, registered_types, in_progress_registrations=None):
+    def __register_type(self, ndt, inc_ns, catalog, registered_types, registering_in_progress=None):
         """Register a type and its dependencies from a namespace into a catalog.
 
         :param ndt: The name of the data type to register
         :param inc_ns: The namespace containing the type
         :param catalog: The catalog to register the type into
         :param registered_types: Set of already registered types (to avoid re-registering)
-        :param in_progress_registrations: Set of types currently being registered (for circular dependency detection).
+        :param registering_in_progress: Set of types currently being registered (for circular dependency detection).
             None if the registration process is starting.
         """
-        if in_progress_registrations is None:
-            in_progress_registrations = set()
-        if ndt in registered_types or ndt in in_progress_registrations:
+        if registering_in_progress is None:
+            registering_in_progress = set()
+        if ndt in registered_types or ndt in registering_in_progress:
             # Already registered or currently being registered (circular dependency)
             return
         # Track that we're currently registering this type
-        in_progress_registrations.add(ndt)
+        registering_in_progress.add(ndt)
         spec = inc_ns.get_spec(ndt)
         spec_file = inc_ns.catalog.get_spec_source_file(ndt)
-        self.__register_dependent_types(spec, inc_ns, catalog, registered_types, in_progress_registrations)
+        self.__register_dependent_types(spec, inc_ns, catalog, registered_types, registering_in_progress)
         if isinstance(spec, DatasetSpec):
             built_spec = self.dataset_spec_cls.build_spec(spec)
         else:
             built_spec = self.group_spec_cls.build_spec(spec)
         registered_types.add(ndt)
         catalog.register_spec(built_spec, spec_file)
 
-    def __register_dependent_types(self, spec, inc_ns, catalog, registered_types, in_progress_registrations):
+    def __register_dependent_types(self, spec, inc_ns, catalog, registered_types, registering_in_progress):
         """Ensure that all types used by this type are registered.
 
         This handles:
         - Parent types (data_type_inc)
         - Nested type definitions (data_type_def in child specs)
         - Link target types
 
-        Circular dependencies are handled by checking in_progress_registrations before
+        Circular dependencies are handled by checking registering_in_progress before
         recursing. This allows patterns like:
         - Type A contains Type B, and Type B extends Type A
         - Type A contains a reference to Type A (self-reference)
@@ -602,23 +641,23 @@ def __register_dependent_types_helper(child_spec):
             if isinstance(child_spec, (GroupSpec, DatasetSpec)):
                 if child_spec.data_type_inc is not None:
                     self.__register_type(
-                        child_spec.data_type_inc, inc_ns, catalog, registered_types, in_progress_registrations
+                        child_spec.data_type_inc, inc_ns, catalog, registered_types, registering_in_progress
                     )
                 if child_spec.data_type_def is not None:  # nested type definition
                     self.__register_type(
-                        child_spec.data_type_def, inc_ns, catalog, registered_types, in_progress_registrations
+                        child_spec.data_type_def, inc_ns, catalog, registered_types, registering_in_progress
                     )
             else:  # spec is a LinkSpec
                 self.__register_type(
-                    child_spec.target_type, inc_ns, catalog, registered_types, in_progress_registrations
+                    child_spec.target_type, inc_ns, catalog, registered_types, registering_in_progress
                 )
             if isinstance(child_spec, GroupSpec):
                 for nested_spec in (child_spec.groups + child_spec.datasets + child_spec.links):
                     __register_dependent_types_helper(nested_spec)
 
         # Register parent type first
         if spec.data_type_inc is not None:
-            self.__register_type(spec.data_type_inc, inc_ns, catalog, registered_types, in_progress_registrations)
+            self.__register_type(spec.data_type_inc, inc_ns, catalog, registered_types, registering_in_progress)
 
         # Register types from child specs (groups, datasets, links)
         if isinstance(spec, GroupSpec):

tests/unit/spec_tests/test_load_namespace.py

@@ -675,6 +675,109 @@ def test_self_referential_type(self):
         registered_types = core_ns.get_registered_types()
         self.assertIn('RecursiveContainer', registered_types)
 
+    def test_circular_dependency_multi_level_inheritance(self):
+        """Test loading types where A contains C, C extends B, and B extends A.
+
+        This tests that the cycle detection walks the full inheritance hierarchy,
+        not just the direct parent.
+        """
+        a_spec = GroupSpec(
+            'Type A',
+            data_type_def='TypeA',
+            groups=[
+                GroupSpec(
+                    'A collection of TypeC',
+                    data_type_inc='TypeC',
+                    quantity='*',
+                )
+            ]
+        )
+        b_spec = GroupSpec(
+            'Type B extends A',
+            data_type_def='TypeB',
+            data_type_inc='TypeA',
+        )
+        c_spec = GroupSpec(
+            'Type C extends B (and thus transitively A)',
+            data_type_def='TypeC',
+            data_type_inc='TypeB',
+        )
+
+        core_ns_builder = NamespaceBuilder('Core namespace', 'core', version='1.0.0')
+        core_ns_builder.add_spec(self.core_source, a_spec)
+        core_ns_builder.add_spec(self.core_source, b_spec)
+        core_ns_builder.add_spec(self.core_source, c_spec)
+        core_ns_builder.export(self.core_ns_path, outdir=self.tempdir)
+
+        ns_catalog = NamespaceCatalog()
+        ns_catalog.load_namespaces(os.path.join(self.tempdir, self.core_ns_path))
+
+        core_ns = ns_catalog.get_namespace('core')
+        registered_types = core_ns.get_registered_types()
+        self.assertIn('TypeA', registered_types)
+        self.assertIn('TypeB', registered_types)
+        self.assertIn('TypeC', registered_types)
+
+        hierarchy = ns_catalog.get_hierarchy('core', 'TypeC')
+        self.assertEqual(hierarchy, ('TypeC', 'TypeB', 'TypeA'))
+
+    def test_circular_dependency_subtype_inherits_from_circular_parent(self):
+        """Test loading a type that extends a type involved in a circular dependency.
+
+        This is the pattern that caused infinite recursion:
+        - ParentTable contains ChildTable (circular dep)
+        - ChildTable extends ParentTable (circular dep)
+        - ExtendedTable extends ParentTable (inherits the circular dep)
+
+        Similar to AlignedDynamicTable extending DynamicTable which contains MeaningsTable.
+        """
+        parent_table_spec = GroupSpec(
+            'A parent table type',
+            data_type_def='ParentTable',
+            groups=[
+                GroupSpec(
+                    'A collection of child tables',
+                    data_type_inc='ChildTable',
+                    quantity='*',
+                )
+            ]
+        )
+        child_table_spec = GroupSpec(
+            'A child table that extends ParentTable',
+            data_type_def='ChildTable',
+            data_type_inc='ParentTable',
+        )
+        extended_table_spec = GroupSpec(
+            'An extended table that also extends ParentTable',
+            data_type_def='ExtendedTable',
+            data_type_inc='ParentTable',
+            groups=[
+                GroupSpec(
+                    'An additional group specific to ExtendedTable',
+                    data_type_inc='ParentTable',
+                    quantity='*',
+                )
+            ]
+        )
+
+        core_ns_builder = NamespaceBuilder('Core namespace', 'core', version='1.0.0')
+        core_ns_builder.add_spec(self.core_source, parent_table_spec)
+        core_ns_builder.add_spec(self.core_source, child_table_spec)
+        core_ns_builder.add_spec(self.core_source, extended_table_spec)
+        core_ns_builder.export(self.core_ns_path, outdir=self.tempdir)
+
+        ns_catalog = NamespaceCatalog()
+        ns_catalog.load_namespaces(os.path.join(self.tempdir, self.core_ns_path))
+
+        core_ns = ns_catalog.get_namespace('core')
+        registered_types = core_ns.get_registered_types()
+        self.assertIn('ParentTable', registered_types)
+        self.assertIn('ChildTable', registered_types)
+        self.assertIn('ExtendedTable', registered_types)
+
+        hierarchy = ns_catalog.get_hierarchy('core', 'ExtendedTable')
+        self.assertEqual(hierarchy, ('ExtendedTable', 'ParentTable'))
+
     def test_link_circular_dependency(self):
         """Test circular dependency through links."""
         # Create types where A links to B, and B extends A

tox.ini

@@ -28,7 +28,7 @@ extras =
     gallery:   docs
     optional:  tqdm
     optional:  sparse
-    optional:  zarr
+    ; optional:  zarr
     optional:  termset
     minimum:   min-reqs
 commands =