added endpoint for testing contributed nwb files #402

dkeller9 · 2025-10-09T13:25:04Z

This endpoint runs pynwb validation on nwb files and returns an error if they do not pass.

james-isbister

Two comments:

I'm not familiar with this await calling another async function. Perhaps @GianlucaFicarelli could comment on whether this part looks ok?
If I understand correctly, the validation is passed if the file can be opened? _process_nwb and _validate_and_read_nwb_file look like they're doing quite similar things in this regard. Could you clarrify the difference? And do they need to be in two separate async functions.

Thanks!

app/endpoints/declared_endpoints.py

dkeller9 · 2025-10-16T21:18:50Z

@james-isbister _process_nwb and _validate_and_read_nwb_file have now been consolidated.

app/endpoints/declared_endpoints.py

GianlucaFicarelli · 2025-10-24T14:14:23Z

app/endpoints/declared_endpoints.py

+
+            test_all_nwb_readers(temp_file_path_local)
+
+            return temp_file_path_local  # Return the path for cleanup


Why the temp file isn't automatically deleted in the context manager, instead of returning the path?

The file path (temp_file_path_local) is returned from blocking_file_operations and stored in the main function's temp_file_path variable. It is then explicitly and reliably deleted in the finally block of validate_nwb_file via the helper function _cleanup_file(temp_file_path).This manual cleanup is essential because the file needs to persist on the disk while the test_all_nwb_readers function runs on it.

It's enough to move validate_all_nwb_readers inside the context manager to avoid all this complexity, since the file is needed only by that function.

Okay, it has now been moved to the context manager to avoid having to explicitly delete temp files.

app/endpoints/declared_endpoints.py

GianlucaFicarelli · 2025-10-27T08:44:33Z

app/endpoints/declared_endpoints.py

+        summary="Validate new format.",
+        description="Tests a new file (.nwb) with basic validation.",
+    )
+    async def validate_nwb_file(


Defining this endpoint as async doesn't provide any advantage because it still needs to use threads, and it makes the logic more complicated.
It should be defined as a regular sync endpoint, similarly to the other endpoints in obi-one handled by fastapi.

Okay, I have made it sync again.

GianlucaFicarelli · 2025-10-27T16:00:46Z

app/endpoints/declared_endpoints.py

+            # 2. Run the synchronous validation function
+            try:
+                # This function now runs synchronously and cleanup is handled
+                # automatically when exiting the 'with' block.


This and other comments that seem to have been added automatically should be removed because useless. I didn't mark all of them.

They have been removed.

GianlucaFicarelli · 2025-10-27T16:01:15Z

app/endpoints/declared_endpoints.py

+
+NWB_READERS = [BBPNWBReader, ScalaNWBReader, AIBSNWBReader, TRTNWBReader]  # , VUNWBReader]
+
+test_protocols = ["APThreshold", "SAPThres1", "SAPThres2", "SAPThres3", "SAPTres1",


This variable is a constants and should be upper case.

Note also that this file doesn't look to be formatted with make format and make lint. After formatting, it would be even clearer to move this big list into a separate file (constants.py or something similar, without any code).

Changed to upper case and reformatted.

GianlucaFicarelli · 2025-10-27T16:01:53Z

app/endpoints/declared_endpoints.py


+# List of all available NWB Reader classes to iterate over
+
+NWB_READERS = [BBPNWBReader, ScalaNWBReader, AIBSNWBReader, TRTNWBReader]  # , VUNWBReader]


VUNWBReader is commented out, but it should be uncommented or removed, or explained.

It has been removed.

GianlucaFicarelli · 2025-10-27T16:19:43Z

app/endpoints/declared_endpoints.py

 def _handle_empty_file(file: UploadFile) -> None:
    """Handle empty file upload by raising an appropriate HTTPException."""
    L.error(f"Empty file uploaded: {file.filename}")
    raise HTTPException(


For consistency with most of the other code, it should raise ApiError instead of HTTPException.
Moreover, ApiErrorCode.BAD_REQUEST doesn't exist, but a more specific error code can be defined.

ApiError is now used.

GianlucaFicarelli · 2025-10-27T16:22:27Z

app/endpoints/declared_endpoints.py

+    )
+    def validate_nwb_file(
+        file: Annotated[UploadFile, File(description="Nwb file to upload (.swc, .h5, or .asc)")],
+    ) -> dict:


Instead of a generic dict, the endpoint should return a pydantic schema. This has also the advantage to appear in the openapi docs.

A pydantic schema is now returned.

app/endpoints/declared_endpoints.py

GianlucaFicarelli · 2025-10-27T16:29:49Z

app/endpoints/declared_endpoints.py

+        return {
+            "status": "success",
+            "message": "NWB file validation successful.",
+        }


Not related to the code above, but there should be unit tests for testing the new endpoint.

Unit tests added.

GianlucaFicarelli · 2025-10-30T12:22:48Z

app/endpoints/declared_endpoints.py

+        # File extension is needed for the NWB readers to recognize the file type
+        file_extension = Path(file.filename).suffix.lower() if file.filename else ""
+        if file_extension != ".nwb":
+            raise ApiError(
+                message="Invalid file extension. Must be .nwb",
+                error_code=ApiErrorCode.INVALID_REQUEST,
+                http_status_code=HTTPStatus.BAD_REQUEST,
+            )


This check can be moved at the beginning, so the file isn't even read if the extension is incorrect.

GianlucaFicarelli · 2025-10-30T12:24:42Z

app/endpoints/declared_endpoints.py

+            L.error(f"Error reading uploaded file content: {e!s}")
+            raise ApiError(
+                message=f"Error reading uploaded file content: {e!s}",
+                error_code=ApiErrorCode.INTERNAL_SERVER_ERROR,


ApiErrorCode.INTERNAL_SERVER_ERROR doesn't exist, it can be GENERIC_ERROR

GianlucaFicarelli · 2025-10-30T12:24:54Z

app/endpoints/declared_endpoints.py

+                L.error(f"Error writing file to disk: {e!s}")
+                raise ApiError(
+                    message=f"Error writing file to disk: {e!s}",
+                    error_code=ApiErrorCode.INTERNAL_SERVER_ERROR,


ApiErrorCode.INTERNAL_SERVER_ERROR doesn't exist, it can be GENERIC_ERROR

GianlucaFicarelli · 2025-10-30T12:29:28Z

app/endpoints/declared_endpoints.py

+        files created during processing are automatically cleaned up.
+        """
+        try:
+            content = file.file.read()


Looking at the logic below, we don't need to load the content in memory here, since the content is just saved to a temporary file below.
It should be possible to unify the two blocks of code reading and writing, so that the temp file is written directly from the stream.

GianlucaFicarelli · 2025-10-30T12:38:41Z

app/endpoints/declared_endpoints.py

+    """
+    for readerclass in NWB_READERS:
+        try:
+            reader = readerclass(nwb_file_path, TEST_PROTOCOLS)


Looking at the classes defined in NWB_READERS, the __init__ wants a hdf5 object (h5py.File) and not a path.
I didn't try the code, does it work for you?
The function validate_all_nwb_readers isn't tested in test_validate_nwb.py because the function is patched, so it would be safer to add an explicit test with a small nwb file.

GianlucaFicarelli · 2025-10-30T12:56:15Z

app/endpoints/declared_endpoints.py

+                http_status_code=HTTPStatus.BAD_REQUEST,
+            )
+
+        with tempfile.TemporaryDirectory() as temp_dir:


What is the maximum expected file size for nwb files?
We should ensure that there is enough space, and it's also better to put a limit on the maximum file size.

GianlucaFicarelli · 2025-10-30T13:06:51Z

tests/routers/declared/test_validate_nwb.py

+        response = client.post(
+            ROUTE,
+            # Note: Filenames are case-sensitive on the extension check unless lowercase() is used.
+            # Assuming the check is case-insensitive if .NWB is tested.


This comment seems wrong, since the check for the extension is case insensitive, but .NWB is expected to fail in this test.

GianlucaFicarelli · 2025-10-30T13:07:35Z

tests/routers/declared/test_validate_nwb.py

@@ -0,0 +1,137 @@
+from http import HTTPStatus
+from unittest.mock import MagicMock, patch


Tests are not run in the CI of this PR. Can you try to rebase or merge from main, to see if it's fixed?

added endpoint for testing contributed nwb files

d1c7616

dkeller9 requested a review from james-isbister October 9, 2025 13:25

dkeller9 and others added 9 commits October 9, 2025 15:26

Merge branch 'main' into nwb_validation_endpoint

8667ceb

linting

d783179

linting

c71f5f0

linting

13e7db9

linting

5cd96dc

linting

5f0bd32

linting

806326b

linting

23dcda7

linting

a43bfd2

james-isbister reviewed Oct 13, 2025

View reviewed changes

GianlucaFicarelli reviewed Oct 13, 2025

View reviewed changes

app/endpoints/declared_endpoints.py Outdated Show resolved Hide resolved

Daniel Keller added 17 commits October 16, 2025 20:16

multiple nwb reader tests

1017a74

multiple nwb reader tests

2f53849

multiple nwb reader tests

d4d7666

multiple nwb reader tests

f2f96e4

multiple nwb reader tests

0e14134

multiple nwb reader tests

9e1b833

multiple nwb reader tests

8c97442

multiple nwb reader tests

cff79f9

multiple nwb reader tests

fc41d0e

multiple nwb reader tests

d4d1866

multiple nwb reader tests

3840936

multiple nwb reader tests

2801d3f

multiple nwb reader tests

03c17ca

multiple nwb reader tests

5cf02bc

multiple nwb reader tests

5a630fc

multiple nwb reader tests

294d826

multiple nwb reader tests

ae571e7

multiple nwb reader tests

4a161a4

GianlucaFicarelli reviewed Oct 17, 2025

View reviewed changes

app/endpoints/declared_endpoints.py Outdated Show resolved Hide resolved

app/endpoints/declared_endpoints.py Outdated Show resolved Hide resolved

Daniel Keller added 6 commits October 24, 2025 12:31

additional nwb reader tests

7e89a48

additional nwb reader tests

da89dc9

made writing non-blocking

749904b

made writing non-blocking

100e8cc

made writing non-blocking

290dafb

change from test to validate

46b87e8

GianlucaFicarelli reviewed Oct 24, 2025

View reviewed changes

Daniel Keller added 12 commits October 24, 2025 17:02

properly created success file

e251685

properly created success file

26811f5

properly created success file

2036a29

properly created success file

4eb2874

made response json

973bc89

made response json

d819e0e

made response json

87ddc7b

made response json

1a7f3e1

made response json

d431ad0

made response json

d6ba7ba

made response json

19e496b

added comment for extension

03ad5ae

GianlucaFicarelli requested changes Oct 27, 2025

View reviewed changes

Daniel Keller added 3 commits October 27, 2025 11:50

made nwb endpoint function sync instead of async

dd35e4f

made nwb endpoint function sync instead of async

430f664

made nwb endpoint function sync instead of async

9488024

GianlucaFicarelli requested changes Oct 27, 2025

View reviewed changes

Daniel Keller added 2 commits October 29, 2025 07:12

assorted fixes in format

5d0ef2b

bad request changed

fb2d1ce

GianlucaFicarelli requested changes Oct 30, 2025

View reviewed changes


		test_all_nwb_readers(temp_file_path_local)

		return temp_file_path_local # Return the path for cleanup


		NWB_READERS = [BBPNWBReader, ScalaNWBReader, AIBSNWBReader, TRTNWBReader] # , VUNWBReader]

		test_protocols = ["APThreshold", "SAPThres1", "SAPThres2", "SAPThres3", "SAPTres1",


		# List of all available NWB Reader classes to iterate over

		NWB_READERS = [BBPNWBReader, ScalaNWBReader, AIBSNWBReader, TRTNWBReader] # , VUNWBReader]

		@@ -0,0 +1,137 @@
		from http import HTTPStatus
		from unittest.mock import MagicMock, patch

added endpoint for testing contributed nwb files #402

Are you sure you want to change the base?

added endpoint for testing contributed nwb files #402

Uh oh!

Conversation

dkeller9 commented Oct 9, 2025

Uh oh!

james-isbister left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dkeller9 commented Oct 16, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GianlucaFicarelli Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GianlucaFicarelli Oct 30, 2025 •

edited

Loading