Integrate NWBInspector with DANDI validation - #941
Conversation
| [ | ||
| error.message | ||
| for error in inspect_nwb( | ||
| nwbfile_path=self.filepath, | ||
| checks=_required_nwb_checks, | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Note two items here:
(a) I notice in your validation API you expect the output to be a list of strings; our native output from the Inspector is a sophisticated data class that also tracks a bunch of other contextual information, the most immediately important of which here is likely the message, which is why I'm only pulling out that item.
If desired, we could figure out some sort of 'nice' f-string that incorporates other relevant attributes. But for now, the form of the output can be seen in the modified and added tests for this PR
(b) All the highest-level inspect functionalities such as inspect_nwb used here are generators that yield data streams of messages; this iteration is being collapsed in this stage similar to the existing functionality, but if desired we could look into yielding here as well.
There was a problem hiding this comment.
We definitely should refactor validation output to support more informative structured records. For the purpose of this PR let's KISS but it relates to #896 for adding BIDS validation.
But I thought that inspector just provides "hints" or "warnings" not "error" class of notifications.
In refactoring we might provide generalization over types of records known to bids-validator and nwb inspector. Filed #943 to start dialog on that topic. Haven't looked yet into nwbinspector records
There was a problem hiding this comment.
But I thought that inspector just provides "hints" or "warnings" not "error" class of notifications.
Precisely; the Inspector only returns messages, we don't/can't distinguish errors vs. warnings most of the time. The behavior occurring here is to basically consider ANY message returned as an error to prevent validation+upload.
There was a problem hiding this comment.
Well, technically, there are two types of explicit errors that we can and do catch, (i) actual Python errors that occur during the process, these get returned as traceback messages but inspection proceeds, and (ii) pynwb validation, but we're planning to disable that here according the other discussion.
You could technically consider anything else from the inspector a 'warning', but the intended use here is for things like critical missing metadata to be considered as errors in the context of validation/upload.
There was a problem hiding this comment.
(ii) pynwb validation, but we're planning to disable that here according the other discussion.
any reference to the discussion/notes? ATM we do treat pynwb validation warning/errors as errors thus preventing upload into archive. We do skip few known to be benign, but in general I think we should ensure that nwb files validate fine with pynwb -- people better address them before uploading those files to the archive.
Following the (i) and (ii) separation I adjusted #943 to separate out loading from validating with pynwb (directly within name for now but might redo differently) since indeed different aspects.
You could technically consider anything else from the inspector a 'warning', but the intended use here is for things like critical missing metadata to be considered as errors in the context of validation/upload.
correct! So, within the scope of dandi validate command we should (by default) output all -- warnings and errors, while in the scope of dandi upload we should trigger validate for the level of error, i.e. ignoring warnings. Also added to #943
There was a problem hiding this comment.
the question here -- should list comprehension be adjusted to only consider errors and skipping any hints etc, or those wouldn't be included ATM any ways? Or should we specify that config should be "dandi"? (I guess it relates to discussion on "tag"ing which checks to run for dandi below)
There was a problem hiding this comment.
@yarikoptic we are using the nwb inspector in two different modes.
- provide a report, tailored to the needs of dandi, recommending changes to the file
- validating the file to ensure all essential requirements are met.
this PR is for mode 2. Mode 1 is taken care of by this PR: dandi/dandi-docs#40 and requires no changes to the dandi cli
There was a problem hiding this comment.
the question here -- should list comprehension be adjusted to only consider errors and skipping any hints etc, or those wouldn't be included ATM any ways?
The best way would, as per the discussion of release cycles (#941 (comment)), be to harness our dandi config, and to not bother including any checks that aren't considered important for dandi.
I have a few ideas for that which utilize existing functionality with minimal changes, so I'll try those out and explain them thoroughly later today.
any reference to the discussion/notes? ATM we do treat pynwb validation warning/errors as errors thus preventing upload into archive. We do skip few known to be benign, but in general I think we should ensure that nwb files validate fine with pynwb -- people better address them before uploading those files to the archive.
Sorry for the confusion. You guys run pynwb.validate but I didn't initially understand the rest of the code in your pynwb_utils.validate helper, which essentially just skips any schema validation below version 2.10 due to the validation + namespace caching issues linked in your code comments for the helper.
We also run pynwb.validate as a core part of the NWBInspector, but we do NOT skip those versions on our side, so that would be a behavioral change to this PR if you wanted the Inspector to become responsible for that (non-metadata) type of validation.
I was simply mentioning that a PR in progress (NeurodataWithoutBorders/pynwb#1432) is trying to solve the underlying issue so that we won't have to skip any schema versions at all, but that's for a future discussion beyond our scope here.
|
This pull request introduces 1 alert when merging 4e0252d into a941ab4 - view on LGTM.com new alerts:
|
Codecov Report
@@ Coverage Diff @@
## master #941 +/- ##
===========================================
+ Coverage 64.55% 87.27% +22.71%
===========================================
Files 65 65
Lines 8085 8107 +22
===========================================
+ Hits 5219 7075 +1856
+ Misses 2866 1032 -1834
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Aha, I wondered if that error would trigger on the CI. We already have a fix for that on our end, just need to pin to it here. |
@yarikoptic No no, I'll restore your validation method and we'll include the changes to the inspector only with a new release on that. The PyNWB changes are just for the long-term plan, once they support validation on cached namespaces themselves then we can run the validation through NWBInspector appropriately on pre-2.10 files, instead of skipping them like we do here. |
| pycryptodomex # for EncryptedKeyring backend in keyrings.alt | ||
| pydantic >= 1.9.0 | ||
| pynwb >= 1.0.3,!=1.1.0 | ||
| nwbinspector @ git+https://github.com/NeurodataWithoutBorders/nwbinspector@validate_optional |
There was a problem hiding this comment.
I'll update this once we have a new release with all the desired fixes in place for this PR
| error.message | ||
| for error in inspect_nwb( | ||
| nwbfile_path=self.filepath, | ||
| skip_validate=True, |
There was a problem hiding this comment.
This allows your local v2.10-skipping method to run instead of ours.
| config=load_config(filepath_or_keyword="dandi"), | ||
| importance_threshold=Importance.CRITICAL, |
There was a problem hiding this comment.
@yarikoptic OK, let me explain what's going on here. We've had a config file for dandi-related purposes for a while now, and this change makes explicit use of it.
You can view the structure of the config here: https://github.com/NeurodataWithoutBorders/nwbinspector/blob/validate_optional/nwbinspector/internal_configs/dandi.inspector_config.yaml
The config (and any config, in general) has the main role of taking certain check functions that we normally consider a low priority for generic NWB purposes and sets them to higher levels of importance for DANDI purposes.
By coupling this with the importance_threshold, this effectively means that our entire registry of checks is loaded, but only those that have a level of CRITICAL will be run on the file.
Note two items with this:
- With this setup, we have a couple other check functions that are CRITICAL right now that would produce a minor change in the behavior of dandi validate via this PR, but they are all basically extensions of the PyNWB validation, and they are 100% accurate at predicting wrong data in the files.
- The one CRITICAL method that is not 100% accurate has been deelevated in importance so that it is skipped by this function call, but would still be run by users running our CLI with -- config dandi prior to attempting dandi validate as per Bens point #1 here. This last point is rather important, because outside of the particular usage enforced by these lines, we don't want that check (or any others) to be explicitly skipped by a general DANDI config because they are actually very important things for a user to be aware of, even if it turns out their data is indeed correct.
There was a problem hiding this comment.
Looking towards the future you envision for easily extending the behavior of dandi validate to include the list of extra metadata checks that you want (Icephys cell things, ontologies, specific forms of subject fields, etc): these would be very easy to include simply by following the comments I left in the config file. That is, when you are all ready for that change, all you have to do is move an item from the BEST_PRACTICE_VIOLATION level to the CRITICAL level and your dandi validate call will now assert that the practice is followed. Likewise for any new checks, you would simply add them under that heading.
Now, we do strongly recommend however that, for further customization and control for any dandi validate-exclusive purposes, you should move that config file (or make your own) over to live on the DANDI side of things. I believe this is the best option for both of us, since it is more general than you having to hard-code a list of explicit checks or metadata fields (you automatically inherit any additional checks/debugs we ever add to the NWBInspector) but it would also soften your own dependency on our release cycle if you want to make any on-the-fly adjustments to metadata considerations.
Either way, this integration is perhaps beginning to err on the side of meriting a joint meeting - please schedule with Ben if you'd like to discuss all this in person.
|
🚀 PR was released in |
4 participants
Hello everyone!
This is a first attempt at integrating the NWBInspector in place of existing metadata validation applied to single NWBAsset instances. This approach will make including other types of metadata checking on NWBAssets much easier to extend; all you would have to do is add the specific check to the
listof_required_nwb_checksat the top of dandi/files.No significant overall change to your CLI behavior is expected here, up to some changes we may have to make on our end to support PyNWB validation on older NWB schemas.
For 'next steps' after this PR that would change behavior by adding extra metadata conditions, this would be very easy to do and could immediately include checking that Subject fields follow certain patterns, such as age/duration in ISO 8601, species being in Latin binomial form, etc...
I just wanted to check first off that this is the desired injection point for utilizing the generalized Inspector during explicit validation (see PR #40 for documentation changes suggesting users to run the full NWBInspector prior to DANDI validation, but this is purely optional).
CC: @bendichter