Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not autogenerate classes when loading namespace #555

Merged
merged 8 commits into from
Apr 9, 2021
Merged

Conversation

ajtritt
Copy link
Contributor

@ajtritt ajtritt commented Apr 7, 2021

Motivation

Fix #553

How to test the behavior?

Set breakpoints or print statements deep in TypeMap.get_container_cls.

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This can be checked running flake8 from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

@ajtritt ajtritt requested review from rly and oruebel April 7, 2021 23:28
@rly
Copy link
Contributor

rly commented Apr 8, 2021

Looks good, though one test is failing. IIRC that test was the following:
'test-core' namespace contains type Bar
'ndx-qux' namespace contains types Qux, Spam
'ndx-test' namespace contains type Baz which extends Bar and includes types Qux and Bar

I think I figured out the core issue.

  1. some dependencies (data_type_inc and links) were not being resolved. That can be resolved with the following new get_container_cls and renamed __resolve_child_container_classes:
    def get_container_cls(self, **kwargs):
        """Get the container class from data type specification.
        If no class has been associated with the ``data_type`` from ``namespace``, a class will be dynamically
        created and returned.
        """
        namespace, data_type, autogen = getargs('namespace', 'data_type', 'autogen', kwargs)
        cls = self.__get_container_cls(namespace, data_type)
        if cls is None and autogen:  # dynamically generate a class
            spec = self.__ns_catalog.get_spec(namespace, data_type)
            self.__resolve_dependent_container_classes(spec, namespace)
            parent_cls = self.__get_parent_cls(namespace, data_type, spec)
            attr_names = self.__default_mapper_cls.get_attr_names(spec)
            cls = self.__class_generator.generate_class(data_type, spec, parent_cls, attr_names, self)
            self.register_container_type(namespace, data_type, cls)
        return cls

    def __resolve_dependent_container_classes(self, spec, namespace):
        """Trigger class generation and registration for dependent data types."""
        if spec.data_type_inc is not None:
            self.get_container_cls(namespace, spec.data_type_inc)
        if isinstance(spec, GroupSpec):
            for child_spec in (spec.groups + spec.datasets):
                if child_spec.data_type_inc is not None:
                    self.get_container_cls(namespace, child_spec.data_type_inc)
                elif child_spec.data_type_def is not None:
                    self.get_container_cls(namespace, child_spec.data_type_def)
            for child_spec in spec.links:
                if child_spec.target_type is not None:
                    self.get_container_cls(namespace, child_spec.target_type)
  1. The test generates classes from different namespaces and expects the TypeMap to resolve between them, BUT when a data type from one namespace references types from another namespace, those type classes should be included/added in the first namespace and it is those generated classes with the first namespace that will be referred to in the docval and other properties of the original generated class.

So the test can be modified to generate classes all from the ndx-test namespace

baz_cls = self.type_map.get_container_cls('ndx-test', 'Baz')
bar_cls = self.type_map.get_container_cls('ndx-test', 'Bar')
qux_cls = self.type_map.get_container_cls('ndx-test', 'Qux')

This cross-namespace strategy might not work fully though.
Let's say:
Namespace N1 contains group type A
Namespace N2 contains group type B extends A. And so N2 will also include its own version of type A.
Namespace N3 contains group type C which includes subgroup type A. And so N3 will also include its own version of type A.

Type B inherits from type A built with namespace N2
Type C allows inclusion of classes of type A built with namespace N3. So the constructor for type C will not allow Type B to be passed in.

I'm not sure what the solution to this is yet, but this situation is pretty rare. It has happened already with the Allen Institute so we should test that before releasing this (but we can merge this).

@rly
Copy link
Contributor

rly commented Apr 8, 2021

This change will fix #554 as well.

src/hdmf/build/manager.py Outdated Show resolved Hide resolved
@rly
Copy link
Contributor

rly commented Apr 8, 2021

Never mind my comments above. I just realized the purpose of TypeSource and made a proposed change in #557 to fix the failing test.

oruebel
oruebel previously approved these changes Apr 8, 2021
Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I'm wondering whether this will also help speed-up importing HDMF.

@ajtritt ajtritt marked this pull request as ready for review April 8, 2021 17:40
* Check dependent types, gen classes for them, and link them

* Add comment

* Update src/hdmf/build/manager.py

Co-authored-by: Andrew Tritt <[email protected]>
@ajtritt ajtritt merged commit fb37665 into dev Apr 9, 2021
@ajtritt
Copy link
Contributor Author

ajtritt commented Apr 9, 2021

@rly @oruebel This change will require a change in pynwb to use the new form of TypeMap.get_container_cls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extra auto classgeneration on import of hdmf.common types
3 participants