Load dependent/included types in namespace/extension (#613)

CHANGELOG.md

@@ -1,5 +1,10 @@
 # HDMF Changelog
 
+## HDMF 2.5.4 (May 17, 2021)
+
+### Bug fix
+- Fix issue where dependencies of included types were not being loaded in namespaces / extensions. @rly (#613)
+
 ## HDMF 2.5.3 (May 12, 2021)
 
 ### Bug fix

src/hdmf/build/manager.py

@@ -5,7 +5,7 @@
 from .builders import DatasetBuilder, GroupBuilder, LinkBuilder, Builder, BaseBuilder
 from .classgenerator import ClassGenerator, CustomClassGenerator, MCIClassGenerator
 from ..container import AbstractContainer, Container, Data
-from ..spec import DatasetSpec, GroupSpec, LinkSpec, NamespaceCatalog, SpecReader
+from ..spec import DatasetSpec, GroupSpec, NamespaceCatalog, SpecReader
 from ..spec.spec import BaseStorageSpec
 from ..utils import docval, getargs, call_docval_func, ExtenderMeta
 
@@ -534,11 +534,10 @@ def __check_dependent_types_helper(spec, namespace):
             if isinstance(spec, (GroupSpec, DatasetSpec)):
                 if spec.data_type_inc is not None:
                     self.get_dt_container_cls(spec.data_type_inc, namespace)  # TODO handle recursive definitions
-                if spec.data_type_def is not None:
+                if spec.data_type_def is not None:  # nested type definition
                     self.get_dt_container_cls(spec.data_type_def, namespace)
-            elif isinstance(spec, LinkSpec):
-                if spec.target_type is not None:
-                    self.get_dt_container_cls(spec.target_type, namespace)
+            else:  # spec is a LinkSpec
+                self.get_dt_container_cls(spec.target_type, namespace)
             if isinstance(spec, GroupSpec):
                 for child_spec in (spec.groups + spec.datasets + spec.links):
                     __check_dependent_types_helper(child_spec, namespace)

src/hdmf/spec/namespace.py

@@ -455,22 +455,51 @@ def __load_namespace(self, namespace, reader, resolve=True):
                     raise ValueError("Could not load namespace '%s'" % s['namespace']) from e
                 if types_to_load is None:
                     types_to_load = inc_ns.get_registered_types()  # load all types in namespace
+                registered_types = set()
                 for ndt in types_to_load:
-                    spec = inc_ns.get_spec(ndt)
-                    spec_file = inc_ns.catalog.get_spec_source_file(ndt)
-                    if isinstance(spec, DatasetSpec):
-                        spec = self.dataset_spec_cls.build_spec(spec)
-                    else:
-                        spec = self.group_spec_cls.build_spec(spec)
-                    catalog.register_spec(spec, spec_file)
-                included_types[s['namespace']] = tuple(types_to_load)
+                    self.__register_type(ndt, inc_ns, catalog, registered_types)
+                included_types[s['namespace']] = tuple(sorted(registered_types))
             else:
                 raise ValueError("Spec '%s' schema must have either 'source' or 'namespace' key" % ns_name)
         # construct namespace
         ns = self.__spec_namespace_cls.build_namespace(catalog=catalog, **namespace)
         self.__namespaces[ns_name] = ns
         return included_types
 
+    def __register_type(self, ndt, inc_ns, catalog, registered_types):
+        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)
+        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):
+        """Ensure that classes for all types used by this type are registered
+        """
+        # TODO test cross-namespace registration...
+        def __register_dependent_types_helper(spec, inc_ns, catalog, registered_types):
+            if isinstance(spec, (GroupSpec, DatasetSpec)):
+                if spec.data_type_inc is not None:
+                    # TODO handle recursive definitions
+                    self.__register_type(spec.data_type_inc, inc_ns, catalog, registered_types)
+                if spec.data_type_def is not None:  # nested type definition
+                    self.__register_type(spec.data_type_def, inc_ns, catalog, registered_types)
+            else:  # spec is a LinkSpec
+                self.__register_type(spec.target_type, inc_ns, catalog, registered_types)
+            if isinstance(spec, GroupSpec):
+                for child_spec in (spec.groups + spec.datasets + spec.links):
+                    __register_dependent_types_helper(child_spec, inc_ns, catalog, registered_types)
+
+        if spec.data_type_inc is not None:
+            self.__register_type(spec.data_type_inc, inc_ns, catalog, registered_types)
+        if isinstance(spec, GroupSpec):
+            for child_spec in (spec.groups + spec.datasets + spec.links):
+                __register_dependent_types_helper(child_spec, inc_ns, catalog, registered_types)
+
     @docval({'name': 'namespace_path', 'type': str, 'doc': 'the path to the file containing the namespaces(s) to load'},
             {'name': 'resolve',
              'type': bool,

tests/unit/spec_tests/test_load_namespace.py

@@ -301,8 +301,8 @@ def test_load_namespaces(self):
         namespace_path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'test.namespace.yaml')
         namespace_deps = self.ns_catalog.load_namespaces(namespace_path)
 
-        # test that the dependencies are correct
-        expected = set(['Data', 'Container', 'DynamicTable'])
+        # test that the dependencies are correct, including dependencies of the dependencies
+        expected = set(['Data', 'Container', 'DynamicTable', 'ElementIdentifiers', 'VectorData'])
         self.assertSetEqual(set(namespace_deps['test']['hdmf-common']), expected)
 
         # test that the types are loaded
@@ -341,8 +341,9 @@ def test_load_namespaces_ext(self):
         ext_namespace_path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'test-ext.namespace.yaml')
         ext_namespace_deps = self.ns_catalog.load_namespaces(ext_namespace_path)
 
-        # test that the dependencies are correct
-        expected_deps = set(['TestData', 'TestContainer', 'TestTable', 'Container', 'Data', 'DynamicTable'])
+        # test that the dependencies are correct, including dependencies of the dependencies
+        expected_deps = set(['TestData', 'TestContainer', 'TestTable', 'Container', 'Data', 'DynamicTable',
+                             'ElementIdentifiers', 'VectorData'])
         self.assertSetEqual(set(ext_namespace_deps['test-ext']['test']), expected_deps)
 
     def test_load_namespaces_bad_path(self):

tests/unit/test_io_hdf5_h5tools.py

@@ -1980,23 +1980,27 @@ def test_load_namespaces_with_pathlib_path(self):
 
     def test_load_namespaces_with_dependencies(self):
         """Test loading namespaces where one includes another."""
-        file_spec = GroupSpec(doc="A FooFile", data_type_def='FooFile')
+        class MyFoo(Container):
+            pass
+
+        myfoo_spec = GroupSpec(doc="A MyFoo", data_type_def='MyFoo', data_type_inc='Foo')
         spec_catalog = SpecCatalog()
         name = 'test_core2'
         namespace = SpecNamespace(
             doc='a test namespace',
             name=name,
-            schema=[{'source': 'test.yaml', 'namespace': 'test_core'}],  # depends on test_core
+            schema=[{'source': 'test2.yaml', 'namespace': 'test_core'}],  # depends on test_core
             version='0.1.0',
             catalog=spec_catalog
         )
-        spec_catalog.register_spec(file_spec, 'test.yaml')
+        spec_catalog.register_spec(myfoo_spec, 'test2.yaml')
         namespace_catalog = NamespaceCatalog()
         namespace_catalog.add_namespace(name, namespace)
         type_map = TypeMap(namespace_catalog)
-        type_map.register_container_type(name, 'FooFile', FooFile)
+        type_map.register_container_type(name, 'MyFoo', MyFoo)
+        type_map.merge(self.manager.type_map, ns_catalog=True)
         manager = BuildManager(type_map)
-        container = FooFile()
+        container = MyFoo(name='myfoo')
         with HDF5IO(self.path, manager=manager, mode='a') as io:  # append to file
             io.write(container)