Repository: NeurodataWithoutBorders/nwbinspector (language: python)

Issue:
[Add Check]: identifier field is unique across folder
### What would you like to see added to the NWBInspector?

An interesting test this would be; it is certainly a Best Practice that the `identifier` be unique to each individual NWBFile, but checks in the NWBInspector are designed to be run on single NWBFiles only. Will have to think about how to do this when calling the CLI on a folder or `inspect_all` on a folder path.

### Pseudocode or other logic of the check

```shell
assert len(set(identifiers_from_nwbfiles_in_folder)) == len(list_of_nwbfiles)
```


### Do you have any interest in helping implement the check function?

Yes.

### Code of Conduct

- [X] I agree to follow this project's [Code of Conduct](https://github.com/NeurodataWithoutBorders/nwbinspector/blob/dev/.github/CODE_OF_CONDUCT.rst)
- [X] Have you ensured this check was not already [requested](https://github.com/neurodatawithoutborders/nwbinspector/issues)?


Discussion / hints:
Maybe something like 

```python
IDENTIFIERS = []

def check_unique_identifier(NWBFile):
    if NWBFile.identifier in IDENTIFIERS:
        return InspectorMessage(...)
    else:
        IDENTIFIERS.append(NWBFile.identifier)

```

though that might not work if you are parallelizing
In principle it *should* work fine with parallelization (both processes and threading) since they share the global variable space and a list or strings is pickleable.

Two things about that approach we would need to be careful about...

(1) not an issue for CLI, but when running as as library, we would need a mechanism for resetting the `IDENTIFIERS` if running any `inspect_*` function more than once, such as `inspect_all` on two separate non-recursive folders. We could just define `global IDENTIFIERS` at the top, then set `IDENTIFIERS=[]` inside each `inspect_*` function, but that seems an excessive and uncontained solution - it also doesn't seem as if it would scale well if we ever wanted to add more folder-wide checks like this.

I guess solution would be to have private functions like `_reset_global_checks` and `_init_global_checks` that served as the centralized place for all such variables, and these get called before and after all inspections.

(2) this ID-specific approach will return one message per non-unique occurrence - while these might be aggregated by the generalized formatting, we would need to be careful there that the `message` field of the NWBInspector does not use file-specific f-strings/ I was originally imagining a single kind of 'pre-check' that only ran once (and only in `inspect_all`, and grabbed all the values from all the `nwbfiles` in one shot. But I guess this approach might fit better into our pre-existing unit-check structure.


Fix patch:
diff --git a/nwbinspector/nwbinspector.py b/nwbinspector/nwbinspector.py
index 88de61d7e..5909130fd 100644
--- a/nwbinspector/nwbinspector.py
+++ b/nwbinspector/nwbinspector.py
@@ -13,12 +13,13 @@
 from types import FunctionType
 from warnings import filterwarnings, warn
 from distutils.util import strtobool
-from time import sleep
+from collections import defaultdict
 
 import click
 import pynwb
 import yaml
 from tqdm import tqdm
+from natsort import natsorted
 
 from . import available_checks
 from .inspector_tools import (
@@ -386,6 +387,30 @@ def inspect_all(
     # Filtering of checks should apply after external modules are imported, in case those modules have their own checks
     checks = configure_checks(config=config, ignore=ignore, select=select, importance_threshold=importance_threshold)
 
+    # Manual identifier check over all files in the folder path
+    identifiers = defaultdict(list)
+    for nwbfile_path in nwbfiles:
+        with pynwb.NWBHDF5IO(path=nwbfile_path, mode="r", driver=driver) as io:
+            nwbfile = robust_s3_read(io.read)
+            identifiers[nwbfile.identifier].append(nwbfile_path)
+    if len(identifiers) != len(nwbfiles):
+        for identifier, nwbfiles_with_identifier in identifiers.items():
+            if len(nwbfiles_with_identifier) > 1:
+                yield InspectorMessage(
+                    message=(
+                        f"The identifier '{identifier}' is used across the .nwb files: "
+                        f"{natsorted([x.name for x in nwbfiles_with_identifier])}. "
+                        "The identifier of any NWBFile should be a completely unique value - "
+                        "we recommend using uuid4 to achieve this."
+                    ),
+                    importance=Importance.CRITICAL,
+                    check_function_name="check_unique_identifiers",
+                    object_type="NWBFile",
+                    object_name="root",
+                    location="/",
+                    file_path=str(path),
+                )
+
     nwbfiles_iterable = nwbfiles
     if progress_bar:
         nwbfiles_iterable = tqdm(nwbfiles_iterable, **progress_bar_options)


Test patch:
diff --git a/tests/test_inspector.py b/tests/test_inspector.py
index 34a344aea..45112e026 100644
--- a/tests/test_inspector.py
+++ b/tests/test_inspector.py
@@ -4,12 +4,14 @@
 from tempfile import mkdtemp
 from pathlib import Path
 from unittest import TestCase
+from datetime import datetime
 
 import numpy as np
 from pynwb import NWBFile, NWBHDF5IO, TimeSeries
 from pynwb.file import TimeIntervals
 from pynwb.behavior import SpatialSeries, Position
 from hdmf.common import DynamicTable
+from natsort import natsorted
 
 from nwbinspector import (
     Importance,
@@ -628,3 +630,74 @@ def test_dandiset_streaming_cli_parallel(self):
             f"> {console_output_file}"
         )
         self.assertFileExists(path=self.tempdir / "test_nwbinspector_streaming_report_7.txt")
+
+
+class TestCheckUniqueIdentifiersPass(TestCase):
+    maxDiff = None
+
+    @classmethod
+    def setUpClass(cls):
+        cls.tempdir = Path(mkdtemp())
+        num_nwbfiles = 3
+        unique_id_nwbfiles = list()
+        for j in range(num_nwbfiles):
+            unique_id_nwbfiles.append(make_minimal_nwbfile())
+
+        cls.unique_id_nwbfile_paths = [str(cls.tempdir / f"unique_id_testing{j}.nwb") for j in range(num_nwbfiles)]
+        for nwbfile_path, nwbfile in zip(cls.unique_id_nwbfile_paths, unique_id_nwbfiles):
+            with NWBHDF5IO(path=nwbfile_path, mode="w") as io:
+                io.write(nwbfile)
+
+    @classmethod
+    def tearDownClass(cls):
+        rmtree(cls.tempdir)
+
+    def test_check_unique_identifiers_pass(self):
+        assert list(inspect_all(path=self.tempdir, select=["check_data_orientation"])) == []
+
+
+class TestCheckUniqueIdentifiersFail(TestCase):
+    maxDiff = None
+
+    @classmethod
+    def setUpClass(cls):
+        cls.tempdir = Path(mkdtemp())
+        num_nwbfiles = 3
+        non_unique_id_nwbfiles = list()
+        for j in range(num_nwbfiles):
+            non_unique_id_nwbfiles.append(
+                NWBFile(
+                    session_description="",
+                    identifier="not a unique identifier!",
+                    session_start_time=datetime.now().astimezone(),
+                )
+            )
+
+        cls.non_unique_id_nwbfile_paths = [
+            str(cls.tempdir / f"non_unique_id_testing{j}.nwb") for j in range(num_nwbfiles)
+        ]
+        for nwbfile_path, nwbfile in zip(cls.non_unique_id_nwbfile_paths, non_unique_id_nwbfiles):
+            with NWBHDF5IO(path=nwbfile_path, mode="w") as io:
+                io.write(nwbfile)
+
+    @classmethod
+    def tearDownClass(cls):
+        rmtree(cls.tempdir)
+
+    def test_check_unique_identifiers_fail(self):
+        assert list(inspect_all(path=self.tempdir, select=["check_data_orientation"])) == [
+            InspectorMessage(
+                message=(
+                    "The identifier 'not a unique identifier!' is used across the .nwb files: "
+                    f"{natsorted([Path(x).name for x in self.non_unique_id_nwbfile_paths])}. "
+                    "The identifier of any NWBFile should be a completely unique value - "
+                    "we recommend using uuid4 to achieve this."
+                ),
+                importance=Importance.CRITICAL,
+                check_function_name="check_unique_identifiers",
+                object_type="NWBFile",
+                object_name="root",
+                location="/",
+                file_path=str(self.tempdir),
+            )
+        ]