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

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

Closed
3 tasks done
CodyCBakerPhD opened this issue Sep 16, 2022 · 1 comment · Fixed by #510
Closed
3 tasks done

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

CodyCBakerPhD opened this issue Sep 16, 2022 · 1 comment · Fixed by #510
Labels
category: bug errors in the code or code behavior

Comments

@CodyCBakerPhD
Copy link
Collaborator

What happened?

Pertaining to conda-forge/nwbinspector-feedstock#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

  • I agree to follow this project's Code of Conduct
  • Have you ensured this bug was not already reported?
  • To the best of your ability, have you ensured this is a bug within the code that checks the NWBFile, rather than a bug in the NWBFile reader (e.g., PyNWB or MatNWB)?
@CodyCBakerPhD CodyCBakerPhD added the category: bug errors in the code or code behavior label Sep 16, 2022
@CodyCBakerPhD CodyCBakerPhD changed the title [Bug]: [Bug]: Philosophy for interactions with PyNWB versions Sep 16, 2022
@CodyCBakerPhD
Copy link
Collaborator Author

From @bendichter in #458 (comment)

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.

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

Successfully merging a pull request may close this issue.

1 participant