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

get_class fails with recursive definition #1299

Open
t-b opened this issue Sep 21, 2020 · 5 comments
Open

get_class fails with recursive definition #1299

t-b opened this issue Sep 21, 2020 · 5 comments
Assignees
Labels
category: bug errors in the code or code behavior

Comments

@t-b
Copy link
Collaborator

t-b commented Sep 21, 2020

I'm trying to add some get_class defaults for ndx-MIES as requested by @rly in nwb-extensions/staged-extensions#15.

But it fails on the recursive type definition:

https://github.com/t-b/ndx-MIES/blob/5e91258312b835aab87942d336081cec37192f86/src/spec/create_extension_spec.py#L55-L67

Versions:
pynwb 1.4.0
hdmf 2.2.0

Reproducable example:

git clone https://github.com/t-b/ndx-MIES
git checkout pynwb-issue-recursive-def
pip install -e .
cd src/pynwb/tests
pytest .

Output:

================================================= test session starts =================================================
platform win32 -- Python 3.7.4, pytest-5.2.1, py-1.8.0, pluggy-0.13.0
rootdir: E:\projekte\ndx-mies
plugins: arraydiff-0.3, doctestplus-0.4.0, openfiles-0.4.0, remotedata-0.3.2
collected 0 items / 1 errors

======================================================= ERRORS ========================================================
___________________________________ ERROR collecting src/pynwb/tests/test_basics.py ___________________________________
test_basics.py:2: in <module>
    import ndx_mies
..\ndx_mies\__init__.py:30: in <module>
    StimulusSetReferencedFolder = get_class('StimulusSetReferencedFolder', 'ndx-mies')
C:\users\thomas\Anaconda3\lib\site-packages\hdmf\utils.py:565: in func_call
    return func(**pargs)
C:\users\thomas\Anaconda3\lib\site-packages\pynwb\__init__.py:187: in get_class
    return __TYPE_MAP.get_container_cls(namespace, neurodata_type)
C:\users\thomas\Anaconda3\lib\site-packages\hdmf\utils.py:561: in func_call
    return func(args[0], **pargs)
C:\users\thomas\Anaconda3\lib\site-packages\hdmf\build\manager.py:592: in get_container_cls
    self.__resolve_child_container_classes(spec, namespace)
C:\users\thomas\Anaconda3\lib\site-packages\hdmf\build\manager.py:635: in __resolve_child_container_classes
    self.get_container_cls(namespace, child_spec.data_type_inc)
C:\users\thomas\Anaconda3\lib\site-packages\hdmf\utils.py:561: in func_call
    return func(args[0], **pargs)
E   RecursionError: maximum recursion depth exceeded in comparison
!!! Recursion detected (same locals & position)
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
================================================== 1 error in 5.18s ===================================================

Just to be clear, there is data released with this extension already, so I can not change the extension schema in an incompatible way.

@t-b t-b added the category: bug errors in the code or code behavior label Sep 21, 2020
@bendichter
Copy link
Contributor

thanks @t-b. Unfortunately, I think this is a fundamental limitation of our current implementation of get_class. Would be interesting to discuss a solution to this for get_class, but for now you'll have to specify the API manually in these cases.

@oruebel
Copy link
Contributor

oruebel commented Oct 1, 2020

BTW, eliminating nested definitions in the schema should not change it in an incompatible way as it typically just means placing the type definitions at the top level and then using neurodata_type_inc to include them in the proper places.

@t-b
Copy link
Collaborator Author

t-b commented Oct 14, 2020

@bendichter I'm not sure I'll find time to write custom code for that.

@oruebel This is not the usual nested spec problem.

From the posted link above

    stimset_referenced_folder = NWBGroupSpec(doc='Folder',
                                             neurodata_type_def='StimulusSetReferencedFolder',
                                             datasets=[...],
                                             groups=[NWBGroupSpec(doc='Nested Folder',
                                                                  neurodata_type_inc='StimulusSetReferencedFolder',
                                                                  quantity='*')
                                                          ],
                                             )

which means that the group spec contains itself recursively. And get_class chokes on that. I don't see a way to rewrite that.

@rly
Copy link
Contributor

rly commented Oct 15, 2020

If this were a simple recursion where type A can contain one instance of type A, then I have a solution that involves some changes in HDMF. However, this is more complicated in that type A can contain multiple instances of type A, which results in the automatic generation of a MultiContainerInterface subclass. I have tried a couple different approaches and it looks like changing get_class to handle this use case will be pretty involved. I will have to get back to this in a few days.

This also reveals another bug where it is not possible to generate classes that are MultiContainerInterface where the base class is a class that has fields beyond just a name.

@t-b
Copy link
Collaborator Author

t-b commented Apr 2, 2021

@rly Is it possible to write a custom class for StimulusSetReferencedFolder?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: bug errors in the code or code behavior
Projects
None yet
Development

No branches or pull requests

4 participants