run conversion: Use context manager only in append mode #1180

h-mayorquin · 2025-01-17T23:51:43Z

This change addresses issues like bug #1114.

This also fixes #1159 by adding proper error messages.

Moreover, this fixes #1183 as the context manager interprets a faulty file on nwbfile_path as intent to use append mode.

To add more description to #1114, the core problem lies in the risk of creating a new interface without adhering to the implicit contract for importing extensions during initialization. This PR eliminates the possibility of unintentionally introducing subtle, hard-to-detect bugs. Furthermore, as highlighted in the discussion of #1143, the logic without a context manager is simpler to understand and covers approximately 95% of use cases.

h-mayorquin · 2025-01-21T17:59:05Z

src/neuroconv/tools/testing/data_interface_mixins.py

@@ -131,23 +131,13 @@ def test_run_conversion_with_backend(self, setup_interface, tmp_path, backend):
                io.read()

    @pytest.mark.parametrize("backend", ["hdf5", "zarr"])
-    def test_run_conversion_with_backend_configuration(self, setup_interface, tmp_path, backend):
+    def test_create_backend_configuration(self, setup_interface, tmp_path, backend):


Those tests were not doing anything. See the bug described here:

#1159

And if we were to make them work they would be duplicating a test that is already run in two other places.

So I changed the name to reflect what the test was actually doing but I would be fine with removing them entirely.

Shouldn't this test be checking that run_conversion runs properly when you pass a custom backend_configuration? How is this redundant with the other tests? What am I missing here?

Yes, you are right, it is not redundant especially if I change both converter and interface versions.

Thinking about this I do want to get rid of these tests completely but I will open another PR describing my reasoning as I don't want this to get in the way.

I will revert this to just eliminate the passing of the nwbfile which does not make sense.

pauladkisson

Overall looks good!

I just suggested a small fix for the ValueError and some clarification about test_run_conversion_with_backend_configuration

pauladkisson · 2025-01-22T19:15:58Z

src/neuroconv/basedatainterface.py

+            if appending_to_disk_nwbfile:
+                raise ValueError(
+                    "Cannot append to an existing file while also providing an in-memory NWBFile. "
+                    "Either set overwrite=True to replace the existing file, or remove the nwbfile parameter to append to the existing file on disk."
+                )


I would prefer to raise this value error right at the beginning for clarity. Also, I think it should be

if appending_to_disk_nwbfile and appending_to_in_memory_nwbfile: raise ValueError(...)

right?

The condition was wrong, I fixed it now. Thanks.

I would like the error to be nested in the apend mode branch. I am also coming at this from the clarity angle but with a different reasoning.

People who read the code should not be bothered with the complexities of the apend case unless they get to that branch. In most cases, run_conversion is working as a write_to_nwbfile (which I think should be a method!) and I don't want the first reading of the function to introduce unrelated complexities.

I really disagree. This is input validation for the function, so it should go right at the top with the inputs. By having in the append branch it's more nested, and to understand the condition you have to combine an else with a not: else(not appending_to_in_disk_nwbfile) + appending_to_in_memory_file. In contrast, I don't think it is hard to understand at all that appending_to_in_memory_nwbfile and appending_to_in_disk_nwbfile can't both be true at the same time --> ValueError, even without understanding anything about the details of those branches.

With all that being said, this is just style, so if you are still adamant about keeping it where it is, I won't insist.

Those are good points. I will move it.

I still have the lingering feeling of wanting to push the complexities of appending mode outside of the happy path but maybe I should push for a write_to_nwbfile functionality outside of this PR. I think should be easier to read, use, and document for both developers and users.

run_conversion, like the context manager here, has too many responsibilities which leads to too many arguments an anti-pattern and a code smell.

I still have the lingering feeling of wanting to push the complexities of appending mode outside of the happy path but maybe I should push for a write_to_nwbfile functionality outside of this PR. I think should be easier to read, use, and document for both developers and users.

run_conversion, like the context manager here, has too many responsibilities which leads to too many arguments an anti-pattern and a code smell.

Yep, isolating the complexity of append mode makes sense to me. I look forward to seeing those changes, and lmk if you need any help.

pauladkisson · 2025-01-22T19:17:04Z

src/neuroconv/nwbconverter.py

+            if appending_to_disk_nwbfile:
+                raise ValueError(
+                    "Cannot append to an existing file while also providing an in-memory NWBFile. "
+                    "Either set overwrite=True to replace the existing file, or remove the nwbfile parameter to append to the existing file on disk."
+                )


Same as with basedatainterface

pauladkisson · 2025-01-22T19:26:30Z

src/neuroconv/tools/testing/data_interface_mixins.py

@@ -131,23 +131,13 @@ def test_run_conversion_with_backend(self, setup_interface, tmp_path, backend):
                io.read()

    @pytest.mark.parametrize("backend", ["hdf5", "zarr"])
-    def test_run_conversion_with_backend_configuration(self, setup_interface, tmp_path, backend):
+    def test_create_backend_configuration(self, setup_interface, tmp_path, backend):


Shouldn't this test be checking that run_conversion runs properly when you pass a custom backend_configuration? How is this redundant with the other tests? What am I missing here?

h-mayorquin · 2025-01-23T16:58:09Z

This will also fix #1183

codecov · 2025-01-24T02:50:40Z

Codecov Report

Attention: Patch coverage is 62.50000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 89.58%. Comparing base (96dfdff) to head (37740c8).
Report is 26 commits behind head on main.

Files with missing lines	Patch %	Lines
src/neuroconv/basedatainterface.py	64.70%	6 Missing ⚠️
src/neuroconv/nwbconverter.py	60.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1180      +/-   ##
==========================================
- Coverage   90.69%   89.58%   -1.11%     
==========================================
  Files         129      129              
  Lines        8189     8357     +168     
==========================================
+ Hits         7427     7487      +60     
- Misses        762      870     +108

Flag	Coverage Δ
unittests	`89.58% <62.50%> (-1.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...c/neuroconv/tools/testing/data_interface_mixins.py	`95.65% <ø> (+0.09%)`	⬆️
src/neuroconv/basedatainterface.py	`80.21% <64.70%> (-15.13%)`	⬇️
src/neuroconv/nwbconverter.py	`81.94% <60.00%> (-13.02%)`	⬇️

... and 19 files with indirect coverage changes

h-mayorquin added 4 commits December 16, 2024 14:46

basic work

f207b42

Merge branch 'main' into avoid_context_for_plain_writing

6f49746

Merge branch 'main' into avoid_context_for_plain_writing

926eac1

modify test

44d8a62

h-mayorquin self-assigned this Jan 17, 2025

h-mayorquin added 6 commits January 17, 2025 19:37

fix fiber photometry I

7465000

fix med pc

e3267f0

fix fiber photometry tests

e2692b8

Merge branch 'main' into avoid_context_for_plain_writing

bad29af

changelog

a786946

doctest

980ca72

h-mayorquin marked this pull request as ready for review January 21, 2025 14:34

h-mayorquin added 4 commits January 21, 2025 09:52

improve variable naming

52080dc

docstring revert

1ffdb8e

remove empty test

4a7848e

add errors

1a0d767

h-mayorquin commented Jan 21, 2025

View reviewed changes

h-mayorquin requested a review from pauladkisson January 21, 2025 18:56

pauladkisson requested changes Jan 22, 2025

View reviewed changes

h-mayorquin added 3 commits January 22, 2025 16:29

use correct fixes

97a7ff1

revert test change

4cfd603

Merge branch 'main' into avoid_context_for_plain_writing

4484090

h-mayorquin mentioned this pull request Jan 23, 2025

[Bug]: IndexError: list index out of range, .abf conversion #1183

Closed

2 tasks

h-mayorquin changed the title ~~Fix #1143~~ Use context manager only in append mode Jan 23, 2025

variable naming

c0f02f8

h-mayorquin changed the title ~~Use context manager only in append mode~~ run conversion: Use context manager only in append mode Jan 23, 2025

h-mayorquin added 2 commits January 23, 2025 12:10

revert another test

4c40aa1

move error to start

9397ab6

pauladkisson approved these changes Jan 23, 2025

View reviewed changes

h-mayorquin enabled auto-merge (squash) January 23, 2025 18:43

Merge branch 'main' into avoid_context_for_plain_writing

37740c8

h-mayorquin merged commit ce63b7c into main Jan 24, 2025
40 checks passed

h-mayorquin deleted the avoid_context_for_plain_writing branch January 24, 2025 02:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

run conversion: Use context manager only in append mode #1180

run conversion: Use context manager only in append mode #1180

h-mayorquin commented Jan 17, 2025 •

edited

Loading

h-mayorquin Jan 21, 2025 •

edited

Loading

pauladkisson Jan 22, 2025

h-mayorquin Jan 23, 2025

pauladkisson left a comment

pauladkisson Jan 22, 2025

h-mayorquin Jan 23, 2025

pauladkisson Jan 23, 2025 •

edited

Loading

h-mayorquin Jan 23, 2025

pauladkisson Jan 23, 2025 •

edited

Loading

pauladkisson Jan 22, 2025

pauladkisson Jan 22, 2025

h-mayorquin commented Jan 23, 2025

codecov bot commented Jan 24, 2025

run conversion: Use context manager only in append mode #1180

run conversion: Use context manager only in append mode #1180

Conversation

h-mayorquin commented Jan 17, 2025 • edited Loading

h-mayorquin Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

pauladkisson Jan 22, 2025

Choose a reason for hiding this comment

h-mayorquin Jan 23, 2025

Choose a reason for hiding this comment

pauladkisson left a comment

Choose a reason for hiding this comment

pauladkisson Jan 22, 2025

Choose a reason for hiding this comment

h-mayorquin Jan 23, 2025

Choose a reason for hiding this comment

pauladkisson Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

h-mayorquin Jan 23, 2025

Choose a reason for hiding this comment

pauladkisson Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

pauladkisson Jan 22, 2025

Choose a reason for hiding this comment

pauladkisson Jan 22, 2025

Choose a reason for hiding this comment

h-mayorquin commented Jan 23, 2025

codecov bot commented Jan 24, 2025

Codecov Report

h-mayorquin commented Jan 17, 2025 •

edited

Loading

h-mayorquin Jan 21, 2025 •

edited

Loading

pauladkisson Jan 23, 2025 •

edited

Loading

pauladkisson Jan 23, 2025 •

edited

Loading