Fix attribute retrieval from version-dependent icephys #264
Conversation
CodyCBakerPhD
changed the title
Codecov Report
@@ Coverage Diff @@
## dev #264 +/- ##
=======================================
Coverage 94.03% 94.03%
=======================================
Files 20 20
Lines 1022 1022
=======================================
Hits 961 961
Misses 61 61
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@bendichter @oruebel @rly If someone could review when any of you get a chance |
nwbinspector/checks/icephys.py
Outdated
| @@ -6,5 +6,5 @@ | |||
| @register_check(importance=Importance.BEST_PRACTICE_VIOLATION, neurodata_type=IntracellularElectrode) | |||
| def check_intracellular_electrode_cell_id_exists(intracellular_electrode: IntracellularElectrode): | |||
| """Check if the IntracellularElectrode contains a cell_id.""" | |||
| if intracellular_electrode.cell_id is None: | |||
| if getattr(intracellular_electrode, "cell_id", "") is None: # Will only be None with PyNWB>=2.1.0 | |||
There was a problem hiding this comment.
I understand that this would work, but maybe a better option is to be a bit more explicit, using get_package_version(name="pynwb") < Version("2.1.0") within the check function itself.
There was a problem hiding this comment.
I thought about that but that is precisely the precedence that leads us down a rabbit hole in the future to having to figure out which parts of which tests depend on which version of PyNWB is being used at runtime.
So it's a lot easier on us to set the precedence of just safely checking if an attribute is accessible and do an early return if not, independent of the reason that field might not be accessible.
Also, apologies - I had thought the last commit included an adjustment I made swapping it from getattr to hasattr but it didn't get pushed, my bad.
| @@ -0,0 +1,32 @@ | |||
| name: Past PyNWB Version | |||
There was a problem hiding this comment.
is there a reason you are setting this up as a cron job instead of just adding it to the PR triggered workflow?
There was a problem hiding this comment.
Multiplicity - If I include it in the main workflow we'd have (number of platforms) x (number of Python versions) x (number of PyNWB versions), which gets a bit out of hand.
This instead constrains to ubuntu-latest + Python 3.9 and then goes back as far in the version history as desired.
|
Right now you are detecting the version of PyNWB. Wouldn't we instead want to check the version of the nwb schema used in the NWB file? |
The testing suite detects PyNWB version to determine which type of in-memory object to create and determine the expected testing behavior. That's only important for the devs and the CI. The check itself, what users will actually call, merely checks for existence of the attribute, which is the simplest approach we can take for this kind of thing. This is especially robust to any future changes to the NWB schema that might change the data model for that object or even minor adjustments to the name of an attribute. |
|
Leaving here for provenance: this PR is a simple hotfix to allow various downstream releases to go forward. After some discussion, we need a better long-term solution for handling and communicating NWB schema version-dependent checks. The open issue #220 and PR #225 should be a good alternative for implementing this since it's analogous. |
After discussing we're just going to follow KISS principles and add safe attribute access to things like this instead of getting bogged down by version specifics.
Should fix the core issue of #263 and the corresponding conda-forge issue.