Improve organization (#485)

* wip * wip * include additional files to commit * remove unintended global * some debugs * debugs * mirror API docs * mirror API docs * correct test imports * correct test imports * add soft deprecation cycle to public methods * fix some autocorrections from src * fix some autocorrections from src * add stacklevel to soft deprecation warnings * fix file names * Apply suggestions from code review Co-authored-by: Cody Baker <[email protected]> * Apply suggestions from code review * pr suggestion * pr suggestion --------- Co-authored-by: CodyCBakerPhD <[email protected]> Co-authored-by: Steph Prince <[email protected]>
NeurodataWithoutBorders · Aug 13, 2024 · d6a13ca · d6a13ca
1 parent 6fc57e5
commit d6a13ca
Show file tree

Hide file tree

Showing 55 changed files with 747 additions and 530 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,8 +1,18 @@
 # Upcoming
 
+### Deprecation (API)
+* Certain low-level functions have been marked as private (such as the former `nwbinspector.register_checks.auto_parse`) indicating they should not have been imported by downstream users. [#485](https://github.com/NeurodataWithoutBorders/nwbinspector/issues/485)
+* Various inappropriate imports from certain submodules have been hard deprecated (e.g., `from nwbinspector.inspector_tools import natsorted`). [#485](https://github.com/NeurodataWithoutBorders/nwbinspector/issues/485)
+
+### Pending Deprecation (API)
+* The `inspector_tools`, `register_check`, and `` submodules have been soft deprecated and will be removed in the next major release. [#485](https://github.com/NeurodataWithoutBorders/nwbinspector/issues/485)
+
 ### Improvements
 
 * Update util function `is_ascending_series` to discard nan values and add `check_timestamps_without_nans` fun to check if timestamps contain NaN values [#476](https://github.com/NeurodataWithoutBorders/nwbinspector/issues/476)
+* Updated the import structure to match modern Python packaging standards. [#485](https://github.com/NeurodataWithoutBorders/nwbinspector/issues/485)
+
+
 
 # v0.4.37
 

diff --git a/docs/api/nwbinspector.rst b/docs/api/nwbinspector.rst
@@ -1,4 +1,4 @@
 Core Library
 ============
 
-.. automodule:: nwbinspector.nwbinspector
+.. automodule:: nwbinspector._inspection
diff --git a/docs/api/register_check.rst b/docs/api/register_check.rst
@@ -3,4 +3,6 @@ Data Classes and Check Registration
 
 All common data class used across the package, as well as the decorator for adding a check function to the registry.
 
-.. automodule:: nwbinspector.register_checks
+.. automodule:: nwbinspector._types
+
+.. automodule:: nwbinspector._registration
diff --git a/docs/api/tools.rst b/docs/api/tools.rst
@@ -1,5 +1,5 @@
-Tools for Organizating and Displaying Reports
-=============================================
+Tools for Organizing and Displaying Reports
+===========================================
 
 .. automodule:: nwbinspector
     :members:
@@ -9,5 +9,8 @@ Tools for Organizating and Displaying Reports
 .. toctree::
     :maxdepth: 4
 
-.. automodule:: nwbinspector.inspector_tools
+.. automodule:: nwbinspector._organization
+    :noindex:
+
+.. automodule:: nwbinspector._formatting
     :noindex:
diff --git a/docs/conf.py b/docs/conf.py
@@ -1,9 +1,12 @@
+import os
 import sys
 from pathlib import Path
 
+from nwbinspector import available_checks
+from collections import defaultdict
+
 sys.path.append(str(Path(__file__).parent))
 from conf_extlinks import extlinks, intersphinx_mapping
-from gen_checks_by_importance import gen_checks_by_importance
 
 sys.path.insert(0, Path(__file__).resolve().parents[1])
 
@@ -73,4 +76,35 @@ def setup(app):
     app.connect("autodoc-process-docstring", add_refs_to_docstrings)
 
 
-gen_checks_by_importance()
+def _gen_checks_by_importance():
+    dd = defaultdict(list)
+
+    for x in available_checks:
+        dd[x.importance.name.replace("_", " ")].append(x)
+
+    generate_checks_rst_file = os.path.join(os.path.dirname(os.path.abspath(__file__)), "checks_by_importance.rst")
+    with open(generate_checks_rst_file, "w") as f:
+        f.write(
+            """Checks by Importance
+======================
+
+This section lists the available checks organized by their importance level.
+
+"""
+        )
+
+        for importance_level, checks in dd.items():
+            f.write(
+                f"""{importance_level}
+{'-' * (len(f'{importance_level}') + 1)}
+
+"""
+            )
+
+            for check in checks:
+                f.write(f"*  :py:func:`~{check.__module__}.{check.__name__}`\n")
+
+            f.write("\n")
+
+
+_gen_checks_by_importance()
diff --git a/docs/gen_checks_by_importance.py b/docs/gen_checks_by_importance.py
diff --git a/setup.py b/setup.py
@@ -7,7 +7,7 @@
     long_description = f.read()
 with open(root / "requirements.txt") as f:
     install_requires = f.readlines()
-with open(root / "src" / "nwbinspector" / "version.py") as f:
+with open(root / "src" / "nwbinspector" / "_version.py") as f:
     version = f.read()
 
 # Instantiate the testing configuration file from the base file `base_test_config.json`
@@ -34,7 +34,7 @@
     install_requires=install_requires,
     # zarr<2.18.0 because of https://github.com/NeurodataWithoutBorders/nwbinspector/pull/460
     extras_require=dict(dandi=["dandi>=0.39.2", "zarr<2.18.0"], zarr=["hdmf_zarr>=0.3.0", "zarr<2.18.0"]),
-    entry_points={"console_scripts": ["nwbinspector=nwbinspector.nwbinspector:inspect_all_cli"]},
+    entry_points={"console_scripts": ["nwbinspector=nwbinspector._inspection_cli:_inspect_all_cli"]},
     license="BSD-3-Clause",
     classifiers=[
         "Development Status :: 4 - Beta",

diff --git a/src/nwbinspector/__init__.py b/src/nwbinspector/__init__.py
@@ -1,24 +1,47 @@
-from .version import __version__
-from .register_checks import available_checks, Importance
-from .nwbinspector import (
-    InspectorOutputJSONEncoder,
+from ._version import __version__
+from ._registration import available_checks, register_check
+from ._types import Importance, Severity, InspectorMessage
+from ._configuration import load_config, validate_config, configure_checks
+from ._inspection 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 *
-from .checks.images import *
-from .checks.nwb_containers import *
-from .checks.nwbfile_metadata import *
-from .checks.ogen import *
-from .checks.ophys import *
-from .checks.tables import *
-from .checks.time_series import *
-from .checks.icephys import *
+from ._inspection import inspect_nwb  # TODO: remove after 7/1/2023
+from ._formatting import (
+    format_messages,
+    print_to_console,
+    save_report,
+    MessageFormatter,
+    FormatterOptions,
+    InspectorOutputJSONEncoder,
+)
+from ._organization import organize_messages
+from .checks import *
 
 default_check_registry = {check.__name__: check for check in available_checks}
+
+__all__ = [
+    "available_checks",
+    "default_check_registry",
+    "register_check",
+    "Importance",
+    "Severity",
+    "InspectorMessage",
+    "load_config",
+    "configure_checks",
+    "InspectorOutputJSONEncoder",
+    "inspect_all",
+    "inspect_nwbfile",
+    "inspect_nwbfile_object",
+    "inspect_nwb",  # TODO: remove after 7/1/2023
+    "run_checks",
+    "format_messages",
+    "print_to_console",
+    "save_report",
+    "MessageFormatter",
+    "FormatterOptions",
+    "organize_messages",
+    "__version__",
+]
diff --git a/src/nwbinspector/_configuration.py b/src/nwbinspector/_configuration.py
@@ -0,0 +1,137 @@
+"""Primary functions for inspecting NWBFiles."""
+
+import json
+import jsonschema
+from pathlib import Path
+from enum import Enum
+from typing import Optional, List
+from types import FunctionType
+from packaging.version import Version
+
+import yaml
+
+from . import available_checks
+from ._registration import InspectorMessage, Importance
+from nwbinspector.utils._utils import (
+    PathType,
+)
+
+INTERNAL_CONFIGS = dict(dandi=Path(__file__).parent / "internal_configs" / "dandi.inspector_config.yaml")
+
+
+def validate_config(config: dict):
+    """Validate an instance of configuration against the official schema."""
+    with open(file=Path(__file__).parent / "config.schema.json", mode="r") as fp:
+        schema = json.load(fp=fp)
+    jsonschema.validate(instance=config, schema=schema)
+
+
+def _copy_function(function):
+    """Copy the core parts of a given function, excluding wrappers, then return a new function."""
+    copied_function = FunctionType(
+        function.__code__, function.__globals__, function.__name__, function.__defaults__, function.__closure__
+    )
+
+    # in case f was given attrs (note this dict is a shallow copy)
+    copied_function.__dict__.update(function.__dict__)
+    return copied_function
+
+
+def copy_check(check):
+    """
+    Copy a check function so that internal attributes can be adjusted without changing the original function.
+
+    Required to ensure our configuration of functions in the registry does not effect the registry itself.
+
+    Also copies the wrapper for auto-parsing results,
+    see https://github.com/NeurodataWithoutBorders/nwbinspector/pull/218 for explanation.
+    """
+    copied_check = _copy_function(function=check)
+    copied_check.__wrapped__ = _copy_function(function=check.__wrapped__)
+    return copied_check
+
+
+def load_config(filepath_or_keyword: PathType) -> dict:
+    """
+    Load a config dictionary either via keyword search of the internal configs, or an explicit filepath.
+
+    Currently supported keywords are:
+        - 'dandi'
+            For all DANDI archive related practices, including validation and upload.
+    """
+    file = INTERNAL_CONFIGS.get(filepath_or_keyword, filepath_or_keyword)
+    with open(file=file, mode="r") as stream:
+        config = yaml.load(stream=stream, Loader=yaml.Loader)
+    return config
+
+
+def configure_checks(
+    checks: list = available_checks,
+    config: Optional[dict] = None,
+    ignore: Optional[List[str]] = None,
+    select: Optional[List[str]] = None,
+    importance_threshold: Importance = Importance.BEST_PRACTICE_SUGGESTION,
+) -> list:
+    """
+    Filter a list of check functions (the entire base registry by default) according to the configuration.
+
+    Parameters
+    ----------
+    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.
+        Typically loaded via json.load from a valid .json file
+    checks : list of check functions
+        Defaults to all registered checks.
+    ignore: list, optional
+        Names of functions to skip.
+    select: list, optional
+        If loading all registered checks, this can be shorthand for selecting only a handful of them.
+    importance_threshold : string, optional
+        Ignores all tests with an post-configuration 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.
+    """
+    if ignore is not None and select is not None:
+        raise ValueError("Options 'ignore' and 'select' cannot both be used.")
+    if importance_threshold not in Importance:
+        raise ValueError(
+            f"Indicated importance_threshold ({importance_threshold}) is not a valid importance level! Please choose "
+            "from [CRITICAL_IMPORTANCE, BEST_PRACTICE_VIOLATION, BEST_PRACTICE_SUGGESTION]."
+        )
+    if config is not None:
+        validate_config(config=config)
+        checks_out = []
+        ignore = ignore or []
+        for check in checks:
+            mapped_check = copy_check(check=check)
+            for importance_name, func_names in config.items():
+                if check.__name__ in func_names:
+                    if importance_name == "SKIP":
+                        ignore.append(check.__name__)
+                        continue
+                    mapped_check.importance = Importance[importance_name]
+                    # Output wrappers are apparently parsed at time of wrapping not of time of output return...
+                    # Attempting to re-wrap the copied function if the importance level is being adjusted...
+                    # From https://github.com/NeurodataWithoutBorders/nwbinspector/issues/302
+                    # new_check_wrapper = _copy_function(function=mapped_check.__wrapped__)
+                    # new_check_wrapper.importance = Importance[importance_name]
+                    # mapped_check.__wrapped__ = new_check_wrapper
+            checks_out.append(mapped_check)
+    else:
+        checks_out = checks
+    if select:
+        checks_out = [x for x in checks_out if x.__name__ in select]
+    elif ignore:
+        checks_out = [x for x in checks_out if x.__name__ not in ignore]
+    if importance_threshold:
+        checks_out = [x for x in checks_out if x.importance.value >= importance_threshold.value]
+    return checks_out