NeurodataWithoutBorders / nwbinspector

Tool to help inspect NWB files for compliance with NWB Best Practices
https://nwbinspector.readthedocs.io/
Other
14 stars 9 forks source link

[Bug]: Philosophy for interactions with PyNWB versions #263

Open CodyCBakerPhD opened 1 year ago

CodyCBakerPhD commented 1 year ago

What happened?

Pertaining to https://github.com/conda-forge/nwbinspector-feedstock/pull/8, if someone runs the NWB Inspector tests (or the NWB Inspector itself on a file that has the relevant neurodata types) in an environment that has an older PyNWB version (for example, 2.0.0 when 2.1.0 is the latest) certain errors can occur, such as IntracellularElectrodes not having a cell_id field.

There are two solutions:

(a) require latest PyNWB when running the Inspector. Note that this does not imply that people are required to use the latest PyNWB to create those files - they could be and probably are encouraged to install and run the Inspector in an independent virtual/conda environment and I'd even be happy to compile some Docker images if that's an even more convenient way to go about it. Point being that it is the act of opening that file with the latest PyNWB version that we implicitly base the Inspector behavior off of.

(b) skip NWB Inspector checks that in some way or another depend on certain version ranges of PyNWB. Probably easiest short-term solution, but this could get complicated to track relative to schema changes and whatnot going into the far future.

As an aside I should probably have a gallery test suite that spans multiple past PyNWB versions.

Steps to Reproduce

No response

Traceback

No response

Operating System

Linux

Python Executable

Python

Python Version

3.9

Usage

Library (Python code)

Were you streaming with ROS3?

No

Package Versions

No response

Code of Conduct

CodyCBakerPhD commented 2 months ago

From @bendichter in https://github.com/NeurodataWithoutBorders/nwbinspector/pull/458#issuecomment-2093591598

It looks like this is (edit: were) failing because there is a workflow that explicitly tests older versions of pynwb and this fails in that case since the feature is only supported in 2.7.0+. What should we do here? Here are some options:

  1. Add a skipif for the offending tests. I have implemented this here. This works for getting past the error, but the check will malfunction. That is not so bad in this particular case, since the error is erroneously silent, which is fine here since the user would not be able to fix that issue with their version of PyNWB anyway.
  2. Since NWB Inspector is about best practices, it may make sense to have strict requirements about only using the latest PyNWB. This makes sense from a best practices perspective, but may not be practical for everyone. For example, the Allen Institute has in the past found it difficult to easily migrate to new environments.
  3. We could introduce a skipif arg to the @register_check decorator, which would allow us to explicitly label checks as pertaining to certain conditions or configurations. This would solve the issue, but would fail to notify users that they could improve best practices if they upgrade pynwb. I suppose we could get around this downside by adding a check specifically for pynwb version, ensuring it is the latest release.