Repository: NeurodataWithoutBorders/nwbinspector (language: python)

Issue:
inspect_nwb() accepts nwb file object
## Motivation

This should be useful for when a nwbfile object is already loaded. 
An use case example is auto inspection before uploading to dandi after a sorting+conversion workflow.



Discussion / hints:
If the NWBFIle is already loaded in memory, you can just call `run_checks` on it: https://github.com/catalystneuro/neuroconv/blob/main/src/neuroconv/datainterfaces/ophys/baseimagingextractorinterface.py#L49-L50

That was the intended design, too, to reduce the complexity of any one function in the workflow.

`inspect_all_cli` (CLI wrapper of API)

-> `inspect_all` (deploys `inspect_nwb` over a folder`)

-> `inspect_nwb` (deploys checks on a file on disk, handles I/O)

-> `run_checks` (actually runs the checks on any open I/O)

Can you try giving that approach a try and make any needed improvements to `run_checks` instead of making `inspect_nwb` do more than it's intended to do?
All I want is a function to easily run the inspection on an open nwb file object, like this:

```
easy_to_inspect_function(nwbfile)
```

this will be immediately useful to our automatic conversion and publishing workflows.

`run_checks` still requires a list of checks as input, and that’s why it sounds like `inspect_nwb` should be the one to handle it, because the logic for forming the list of check objects is already built there. 

I don’t think we should push the complexity of dealing with this to the applications either, when it is already dealt with by inspect_nwb.

Sounds more reasonable to me to add a very simple logic for dealing with NWBFile objects to inspect_nwb instead.


Fix patch:
diff --git a/src/nwbinspector/__init__.py b/src/nwbinspector/__init__.py
index 3228437c9..bad199d9b 100644
--- a/src/nwbinspector/__init__.py
+++ b/src/nwbinspector/__init__.py
@@ -1,7 +1,7 @@
 from .version import __version__
 from .register_checks import available_checks, Importance
-from .nwbinspector import inspect_nwb, inspect_all, run_checks, load_config
-from .checks.behavior import *
+from .nwbinspector import inspect_all, inspect_nwbfile, inspect_nwbfile_object, run_checks, load_config
+from .nwbinspector import inspect_nwb  # TODO: remove after 7/1/2023
 from .checks.ecephys import *
 from .checks.general import *
 from .checks.image_series import *
diff --git a/src/nwbinspector/nwbinspector.py b/src/nwbinspector/nwbinspector.py
index ae7924a47..2d6917a88 100644
--- a/src/nwbinspector/nwbinspector.py
+++ b/src/nwbinspector/nwbinspector.py
@@ -7,7 +7,7 @@
 import jsonschema
 from pathlib import Path
 from enum import Enum
-from typing import Union, Optional, List
+from typing import Union, Optional, List, Iterable
 from concurrent.futures import ProcessPoolExecutor, as_completed
 from types import FunctionType
 from warnings import filterwarnings, warn
@@ -448,7 +448,7 @@ def inspect_all(
                     yield message
     else:
         for nwbfile_path in nwbfiles_iterable:
-            for message in inspect_nwb(nwbfile_path=nwbfile_path, checks=checks, driver=driver):
+            for message in inspect_nwbfile(nwbfile_path=nwbfile_path, checks=checks, driver=driver):
                 if stream:
                     message.file_path = nwbfiles[message.file_path]
                 yield message
@@ -458,9 +458,10 @@ def _pickle_inspect_nwb(
     nwbfile_path: str, checks: list = available_checks, skip_validate: bool = False, driver: Optional[str] = None
 ):
     """Auxiliary function for inspect_all to run in parallel using the ProcessPoolExecutor."""
-    return list(inspect_nwb(nwbfile_path=nwbfile_path, checks=checks, skip_validate=skip_validate, driver=driver))
+    return list(inspect_nwbfile(nwbfile_path=nwbfile_path, checks=checks, skip_validate=skip_validate, driver=driver))
 
 
+# TODO: remove after 7/1/2023
 def inspect_nwb(
     nwbfile_path: FilePathType,
     checks: list = available_checks,
@@ -471,16 +472,59 @@ def inspect_nwb(
     driver: Optional[str] = None,
     skip_validate: bool = False,
     max_retries: int = 10,
-) -> List[InspectorMessage]:
+) -> Iterable[InspectorMessage]:
+    warn(
+        "The API function 'inspect_nwb' has been deprecated and will be removed in a future release! "
+        "To remove ambiguity, please call either "
+        "'inspect_nwbfile' giving a path to the unopened file on a system, or "
+        "'inspect_nwbfile_object' passing an already open pynwb.NWBFile object.",
+        category=DeprecationWarning,
+        stacklevel=2,
+    )
+    for inspector_message in inspect_nwbfile(
+        nwbfile_path=nwbfile_path,
+        checks=checks,
+        config=config,
+        ignore=ignore,
+        select=select,
+        importance_threshold=importance_threshold,
+        driver=driver,
+        skip_validate=skip_validate,
+        max_retries=max_retries,
+    ):
+        yield inspector_message
+
+
+def inspect_nwbfile(
+    nwbfile_path: FilePathType,
+    driver: Optional[str] = None,
+    skip_validate: bool = False,
+    max_retries: int = 10,
+    checks: list = available_checks,
+    config: dict = None,
+    ignore: OptionalListOfStrings = None,
+    select: OptionalListOfStrings = None,
+    importance_threshold: Union[str, Importance] = Importance.BEST_PRACTICE_SUGGESTION,
+) -> Iterable[InspectorMessage]:
     """
-    Inspect a NWBFile object and return suggestions for improvements according to best practices.
+    Open an NWB file, inspect the contents, and return suggestions for improvements according to best practices.
 
     Parameters
     ----------
     nwbfile_path : FilePathType
-        Path to the NWBFile.
+        Path to the NWB file on disk or on S3.
+    driver: str, optional
+        Forwarded to h5py.File(). Set to "ros3" for reading from s3 url.
+    skip_validate : bool
+        Skip the PyNWB validation step. This may be desired for older NWBFiles (< schema version v2.10).
+        The default is False, which is also recommended.
+    max_retries : int, optional
+        When using the ros3 driver to stream data from an s3 path, occasional curl issues can result.
+        AWS suggests using iterative retry with an exponential backoff of 0.1 * 2^retries.
+        This sets a hard bound on the number of times to attempt to retry the collection of messages.
+        Defaults to 10 (corresponds to 102.4s maximum delay on final attempt).
     checks : list, optional
-        list of checks to run
+        List of checks to run.
     config : dict
         Dictionary valid against our JSON configuration schema.
         Can specify a mapping of importance levels and list of check functions whose importance you wish to change.
@@ -501,24 +545,7 @@ def inspect_nwb(
                 - improvable data representation
 
         The default is the lowest level, BEST_PRACTICE_SUGGESTION.
-    driver: str, optional
-        Forwarded to h5py.File(). Set to "ros3" for reading from s3 url.
-    skip_validate : bool
-        Skip the PyNWB validation step. This may be desired for older NWBFiles (< schema version v2.10).
-        The default is False, which is also recommended.
-    max_retries : int, optional
-        When using the ros3 driver to stream data from an s3 path, occasional curl issues can result.
-        AWS suggests using iterative retry with an exponential backoff of 0.1 * 2^retries.
-        This sets a hard bound on the number of times to attempt to retry the collection of messages.
-        Defaults to 10 (corresponds to 102.4s maximum delay on final attempt).
     """
-    importance_threshold = (
-        Importance[importance_threshold] if isinstance(importance_threshold, str) else importance_threshold
-    )
-    if any(x is not None for x in [config, ignore, select, importance_threshold]):
-        checks = configure_checks(
-            checks=checks, config=config, ignore=ignore, select=select, importance_threshold=importance_threshold
-        )
     nwbfile_path = str(nwbfile_path)
     filterwarnings(action="ignore", message="No cached namespaces found in .*")
     filterwarnings(action="ignore", message="Ignoring cached namespace .*")
@@ -536,8 +563,15 @@ def inspect_nwb(
                 )
 
         try:
-            nwbfile = robust_s3_read(command=io.read, max_retries=max_retries)
-            for inspector_message in run_checks(nwbfile=nwbfile, checks=checks):
+            nwbfile_object = robust_s3_read(command=io.read, max_retries=max_retries)
+            for inspector_message in inspect_nwbfile_object(
+                nwbfile_object=nwbfile_object,
+                checks=checks,
+                config=config,
+                ignore=ignore,
+                select=select,
+                importance_threshold=importance_threshold,
+            ):
                 inspector_message.file_path = nwbfile_path
                 yield inspector_message
         except Exception as ex:
@@ -549,6 +583,57 @@ def inspect_nwb(
             )
 
 
+def inspect_nwbfile_object(
+    nwbfile_object: pynwb.NWBFile,
+    checks: Optional[list] = None,
+    config: Optional[dict] = None,
+    ignore: Optional[List[str]] = None,
+    select: Optional[List[str]] = None,
+    importance_threshold: Union[str, Importance] = Importance.BEST_PRACTICE_SUGGESTION,
+) -> List[InspectorMessage]:
+    """
+    Inspect an in-memory NWBFile object and return suggestions for improvements according to best practices.
+
+    Parameters
+    ----------
+    nwbfile_path : FilePathType
+        An in-memory NWBFile object.
+    checks : list, optional
+        list of checks to run
+    config : dict, optional
+        Dictionary valid against our JSON configuration schema.
+        Can specify a mapping of importance levels and list of check functions whose importance you wish to change.
+        Typically loaded via json.load from a valid .json file
+    ignore: list, optional
+        Names of functions to skip.
+    select: list, optional
+        Names of functions to pick out of available checks.
+    importance_threshold : string or Importance, optional
+        Ignores tests with an assigned importance below this threshold.
+        Importance has three levels:
+
+            CRITICAL
+                - potentially incorrect data
+            BEST_PRACTICE_VIOLATION
+                - very suboptimal data representation
+            BEST_PRACTICE_SUGGESTION
+                - improvable data representation
+
+        The default is the lowest level, BEST_PRACTICE_SUGGESTION.
+    """
+    checks = checks or available_checks
+    importance_threshold = (
+        Importance[importance_threshold] if isinstance(importance_threshold, str) else importance_threshold
+    )
+    if any(argument is not None for argument in [config, ignore, select, importance_threshold]):
+        checks = configure_checks(
+            checks=checks, config=config, ignore=ignore, select=select, importance_threshold=importance_threshold
+        )
+
+    for inspector_message in run_checks(nwbfile=nwbfile_object, checks=checks):
+        yield inspector_message
+
+
 def run_checks(nwbfile: pynwb.NWBFile, checks: list):
     """
     Run checks on an open NWBFile object.


Test patch:
diff --git a/tests/test_inspector.py b/tests/test_inspector.py
index d3aa7088b..5d540453a 100644
--- a/tests/test_inspector.py
+++ b/tests/test_inspector.py
@@ -22,7 +22,7 @@
     check_subject_exists,
     load_config,
 )
-from nwbinspector import inspect_all, inspect_nwb, available_checks
+from nwbinspector import inspect_all, inspect_nwbfile, available_checks
 from nwbinspector.register_checks import Severity, InspectorMessage, register_check
 from nwbinspector.tools import make_minimal_nwbfile
 from nwbinspector.utils import FilePathType
@@ -277,8 +277,8 @@ def test_inspect_all_parallel(self):
             ]
             self.assertCountEqual(first=test_results, second=true_results)
 
-    def test_inspect_nwb(self):
-        test_results = list(inspect_nwb(nwbfile_path=self.nwbfile_paths[0], checks=self.checks))
+    def test_inspect_nwbfile(self):
+        test_results = list(inspect_nwbfile(nwbfile_path=self.nwbfile_paths[0], checks=self.checks))
         true_results = [
             InspectorMessage(
                 message="data is not compressed. Consider enabling compression when writing a dataset.",
@@ -327,9 +327,9 @@ def test_inspect_nwb(self):
         ]
         self.assertCountEqual(first=test_results, second=true_results)
 
-    def test_inspect_nwb_importance_threshold_as_importance(self):
+    def test_inspect_nwbfile_importance_threshold_as_importance(self):
         test_results = list(
-            inspect_nwb(
+            inspect_nwbfile(
                 nwbfile_path=self.nwbfile_paths[0], checks=self.checks, importance_threshold=Importance.CRITICAL
             )
         )
@@ -358,9 +358,9 @@ def test_inspect_nwb_importance_threshold_as_importance(self):
         ]
         self.assertCountEqual(first=test_results, second=true_results)
 
-    def test_inspect_nwb_importance_threshold_as_string(self):
+    def test_inspect_nwbfile_importance_threshold_as_string(self):
         test_results = list(
-            inspect_nwb(nwbfile_path=self.nwbfile_paths[0], checks=self.checks, importance_threshold="CRITICAL")
+            inspect_nwbfile(nwbfile_path=self.nwbfile_paths[0], checks=self.checks, importance_threshold="CRITICAL")
         )
         true_results = [
             InspectorMessage(
@@ -457,7 +457,7 @@ def iterable_check_function(table: DynamicTable):
             for col in table.columns:
                 yield InspectorMessage(message=f"Column: {col.name}")
 
-        test_results = list(inspect_nwb(nwbfile_path=self.nwbfile_paths[0], select=["iterable_check_function"]))
+        test_results = list(inspect_nwbfile(nwbfile_path=self.nwbfile_paths[0], select=["iterable_check_function"]))
         true_results = [
             InspectorMessage(
                 message="Column: start_time",
@@ -478,8 +478,8 @@ def iterable_check_function(table: DynamicTable):
         ]
         self.assertCountEqual(first=test_results, second=true_results)
 
-    def test_inspect_nwb_manual_iteration(self):
-        generator = inspect_nwb(nwbfile_path=self.nwbfile_paths[0], checks=self.checks)
+    def test_inspect_nwbfile_manual_iteration(self):
+        generator = inspect_nwbfile(nwbfile_path=self.nwbfile_paths[0], checks=self.checks)
         message = next(generator)
         true_result = InspectorMessage(
             message="data is not compressed. Consider enabling compression when writing a dataset.",
@@ -493,15 +493,15 @@ def test_inspect_nwb_manual_iteration(self):
         )
         self.assertEqual(message, true_result)
 
-    def test_inspect_nwb_manual_iteration_stop(self):
-        generator = inspect_nwb(nwbfile_path=self.nwbfile_paths[2], checks=self.checks)
+    def test_inspect_nwbfile_manual_iteration_stop(self):
+        generator = inspect_nwbfile(nwbfile_path=self.nwbfile_paths[2], checks=self.checks)
         with self.assertRaises(expected_exception=StopIteration):
             next(generator)
 
-    def test_inspect_nwb_dandi_config(self):
+    def test_inspect_nwbfile_dandi_config(self):
         config_checks = [check_subject_exists] + self.checks
         test_results = list(
-            inspect_nwb(
+            inspect_nwbfile(
                 nwbfile_path=self.nwbfile_paths[0],
                 checks=config_checks,
                 config=load_config(filepath_or_keyword="dandi"),
@@ -591,9 +591,9 @@ def setUpClass(cls):
     def tearDownClass(cls):
         rmtree(cls.tempdir)
 
-    def test_inspect_nwb_dandi_config_critical_only_entire_registry(self):
+    def test_inspect_nwbfile_dandi_config_critical_only_entire_registry(self):
         test_results = list(
-            inspect_nwb(
+            inspect_nwbfile(
                 nwbfile_path=self.nwbfile_paths[0],
                 checks=available_checks,
                 config=load_config(filepath_or_keyword="dandi"),
@@ -622,9 +622,9 @@ def test_inspect_nwb_dandi_config_critical_only_entire_registry(self):
         ]
         self.assertCountEqual(first=test_results, second=true_results)
 
-    def test_inspect_nwb_dandi_config_violation_and_above_entire_registry(self):
+    def test_inspect_nwbfile_dandi_config_violation_and_above_entire_registry(self):
         test_results = list(
-            inspect_nwb(
+            inspect_nwbfile(
                 nwbfile_path=self.nwbfile_paths[1],
                 checks=available_checks,
                 config=load_config(filepath_or_keyword="dandi"),