hdmf-dev / hdmf

The Hierarchical Data Modeling Framework
http://hdmf.readthedocs.io
Other
46 stars 24 forks source link

Regression: UserWarning: Unexpected keys ['value'] in spec ... #1142

Open yarikoptic opened 3 days ago

yarikoptic commented 3 days ago

our testing of dandi-cli against dev versions of hdmf and pynwb started to fail 2 days ago

https://github.com/dandi/dandi-cli/actions/runs/9772685521/job/26977490874

Run python -m pytest -s -v --cov=dandi --cov-report=xml dandi
ImportError while loading conftest '/home/runner/work/dandi-cli/dandi-cli/dandi/conftest.py'.
dandi/conftest.py:5: in <module>
    from .tests.fixtures import *  # noqa: F401, F403  # lgtm [py/polluting-import]
dandi/tests/fixtures.py:20: in <module>
    import pynwb
/opt/hostedtoolcache/Python/3.8.[18](https://github.com/dandi/dandi-cli/actions/runs/9772685521/job/26977490874#step:8:19)/x64/lib/python3.8/site-packages/pynwb/__init__.py:145: in <module>
    load_namespaces(__resources['namespace_path'])
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/hdmf/utils.py:672: in func_call
    return func(**pargs)
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pynwb/__init__.py:139: in load_namespaces
    return __TYPE_MAP.load_namespaces(namespace_path)
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/hdmf/utils.py:668: in func_call
    return func(args[0], **pargs)
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/hdmf/build/manager.py:482: in load_namespaces
    deps = self.__ns_catalog.load_namespaces(**kwargs)
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/hdmf/utils.py:668: in func_call
    return func(args[0], **pargs)
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/hdmf/spec/namespace.py:537: in load_namespaces
    ret[ns['name']] = self.__load_namespace(ns, reader, resolve=resolve)
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/hdmf/spec/namespace.py:447: in __load_namespace
    self.__load_spec_file(reader, s['source'], catalog, types_to_load=types_to_load, resolve=resolve)
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/hdmf/spec/namespace.py:401: in __load_spec_file
    temp_dict = {k: None for k in __reg_spec(self.__group_spec_cls, spec_dict)}
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/hdmf/spec/namespace.py:387: in __reg_spec
    spec_obj = spec_cls.build_spec(spec_dict)
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/hdmf/spec/spec.py:93: in build_spec
    vargs = cls.build_const_args(spec_dict)
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pynwb/spec.py:105: in build_const_args
    ret = proxy.build_const_args(spec_dict)
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/hdmf/spec/spec.py:1385: in build_const_args
    ret['datasets'] = list(map(cls.dataset_spec_cls().build_spec, ret['datasets']))
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/hdmf/spec/spec.py:102: in build_spec
    warn(f'Unexpected keys {unused_vargs} in spec {spec_dict}')
E   UserWarning: Unexpected keys ['value'] in spec {'name': 'bias_current', 'dtype': 'float[32](https://github.com/dandi/dandi-cli/actions/runs/9772685521/job/26977490874#step:8:33)', 'value': 0.0, 'doc': 'Bias current, in amps, fixed to 0.0.'}
rly commented 5 hours ago

The spec for IZeroClampSeries has three scalar datasets with a fixed value of 0.0. https://github.com/NeurodataWithoutBorders/nwb-schema/blob/dev/core/nwb.icephys.yaml#L77-L89

IZeroClampSeries extends CurrentClampSeries where these datasets are optional and can be set to any value.

The hdmf-schema-language version 2 (currently in use) says that "value" is an allowed property of a Dataset. However, HDMF has never supported this property. The spec was read and value: 0.0 was simply ignored. Instead, the PyNWB API set a fixed value of 0.0 for these datasets: https://github.com/NeurodataWithoutBorders/pynwb/blob/dev/src/pynwb/icephys.py#L211

In the latest hdmf-schema-language, beta version 3 (not yet adopted), we removed the "value" property of a Dataset because it was not being read by HDMF and there was no clear use case. These IZeroClampSeries datasets demonstrate a valid use case (though, really, these should be attributes, but changing that would be a breaking change). So, I think we should re-allow the "value" property on Datasets and add proper support to HDMF.

We recently implemented a check in HDMF that warns when spec properties are provided that HDMF does not expect https://github.com/hdmf-dev/hdmf/pull/1134 . This check flagged the above three datasets.

rly commented 5 hours ago

Thanks for the bug report @yarikoptic !