Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 21 commits into from
Jan 24, 2025
Merged

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

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f207b42
basic work
h-mayorquin Dec 16, 2024
6f49746
Merge branch 'main' into avoid_context_for_plain_writing
h-mayorquin Dec 19, 2024
926eac1
Merge branch 'main' into avoid_context_for_plain_writing
h-mayorquin Jan 17, 2025
44d8a62
modify test
h-mayorquin Jan 17, 2025
7465000
fix fiber photometry I
h-mayorquin Jan 18, 2025
e3267f0
fix med pc
h-mayorquin Jan 18, 2025
e2692b8
fix fiber photometry tests
h-mayorquin Jan 20, 2025
bad29af
Merge branch 'main' into avoid_context_for_plain_writing
h-mayorquin Jan 20, 2025
a786946
changelog
h-mayorquin Jan 20, 2025
980ca72
doctest
h-mayorquin Jan 21, 2025
52080dc
improve variable naming
h-mayorquin Jan 21, 2025
1ffdb8e
docstring revert
h-mayorquin Jan 21, 2025
4a7848e
remove empty test
h-mayorquin Jan 21, 2025
1a0d767
add errors
h-mayorquin Jan 21, 2025
97a7ff1
use correct fixes
h-mayorquin Jan 22, 2025
4cfd603
revert test change
h-mayorquin Jan 23, 2025
4484090
Merge branch 'main' into avoid_context_for_plain_writing
h-mayorquin Jan 23, 2025
c0f02f8
variable naming
h-mayorquin Jan 23, 2025
4c40aa1
revert another test
h-mayorquin Jan 23, 2025
9397ab6
move error to start
h-mayorquin Jan 23, 2025
37740c8
Merge branch 'main' into avoid_context_for_plain_writing
h-mayorquin Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@
* Added `metadata` and `conversion_options` as arguments to `NWBConverter.temporally_align_data_interfaces` [PR #1162](https://github.com/catalystneuro/neuroconv/pull/1162)

## Improvements
* Simple writing no longer uses a context manager [PR #1180](https://github.com/catalystneuro/neuroconv/pull/1180)

# v0.6.7 (January 20, 2024)
# v0.6.7 (January 20, 2025)

## Deprecations

Expand Down
1 change: 0 additions & 1 deletion docs/conversion_examples_gallery/recording/openephys.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,3 @@ Convert OpenEphys data to NWB using :py:class:`~neuroconv.datainterfaces.ecephys
>>> # Choose a path for saving the nwb file and run the conversion
>>> nwbfile_path = f"{path_to_save_nwbfile}" # This should be something like: "./saved_file.nwb"
>>> interface.run_conversion(nwbfile_path=nwbfile_path, metadata=metadata)
NWB file saved at ...
64 changes: 43 additions & 21 deletions src/neuroconv/basedatainterface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@
make_nwbfile_from_metadata,
make_or_load_nwbfile,
)
from .tools.nwb_helpers._metadata_and_file_helpers import _resolve_backend
from .tools.nwb_helpers._metadata_and_file_helpers import (
_resolve_backend,
configure_and_write_nwbfile,
)
from .utils import (
get_json_schema_from_method_signature,
load_dict_from_file,
Expand Down Expand Up @@ -163,7 +166,7 @@ def run_conversion(
Parameters
----------
nwbfile_path : FilePathType
Path for where the data will be written or appended.
Path for where to write or load (if overwrite=False) the NWBFile.
nwbfile : NWBFile, optional
An in-memory NWBFile object to write to the location.
metadata : dict, optional
Expand All @@ -182,32 +185,51 @@ def run_conversion(
Otherwise, all datasets will use default configuration settings.
"""

backend = _resolve_backend(backend, backend_configuration)
no_nwbfile_provided = nwbfile is None # Otherwise, variable reference may mutate later on inside the context
appending_to_in_memory_nwbfile = nwbfile is not None
file_initially_exists = Path(nwbfile_path).exists() if nwbfile_path is not None else False
appending_to_disk_nwbfile = file_initially_exists and not overwrite

if metadata is None:
metadata = self.get_metadata()
self.validate_metadata(metadata=metadata, append_mode=appending_to_disk_nwbfile)

if not appending_to_disk_nwbfile:
if appending_to_in_memory_nwbfile:
self.add_to_nwbfile(nwbfile=nwbfile, metadata=metadata, **conversion_options)
else:
nwbfile = self.create_nwbfile(metadata=metadata, **conversion_options)

configure_and_write_nwbfile(
nwbfile=nwbfile,
output_filepath=nwbfile_path,
backend=backend,
backend_configuration=backend_configuration,
)

else: # We are only using the context in append mode, see issue #1143

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."
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@pauladkisson pauladkisson Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@pauladkisson pauladkisson Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


backend = _resolve_backend(backend, backend_configuration)
with make_or_load_nwbfile(
nwbfile_path=nwbfile_path,
nwbfile=nwbfile,
metadata=metadata,
overwrite=overwrite,
backend=backend,
verbose=getattr(self, "verbose", False),
) as nwbfile_out:

file_initially_exists = Path(nwbfile_path).exists() if nwbfile_path is not None else False
append_mode = file_initially_exists and not overwrite

self.validate_metadata(metadata=metadata, append_mode=append_mode)

with make_or_load_nwbfile(
nwbfile_path=nwbfile_path,
nwbfile=nwbfile,
metadata=metadata,
overwrite=overwrite,
backend=backend,
verbose=getattr(self, "verbose", False),
) as nwbfile_out:
if no_nwbfile_provided:
self.add_to_nwbfile(nwbfile=nwbfile_out, metadata=metadata, **conversion_options)

if backend_configuration is None:
backend_configuration = self.get_default_backend_configuration(nwbfile=nwbfile_out, backend=backend)
if backend_configuration is None:
backend_configuration = self.get_default_backend_configuration(nwbfile=nwbfile_out, backend=backend)

configure_backend(nwbfile=nwbfile_out, backend_configuration=backend_configuration)
configure_backend(nwbfile=nwbfile_out, backend_configuration=backend_configuration)

@staticmethod
def get_default_backend_configuration(
Expand Down
56 changes: 38 additions & 18 deletions src/neuroconv/nwbconverter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from .tools.nwb_helpers import (
HDF5BackendConfiguration,
ZarrBackendConfiguration,
configure_and_write_nwbfile,
configure_backend,
get_default_backend_configuration,
get_default_nwbfile_metadata,
Expand Down Expand Up @@ -243,35 +244,54 @@ def run_conversion(
" use Converter.add_to_nwbfile."
)

backend = _resolve_backend(backend, backend_configuration)
no_nwbfile_provided = nwbfile is None # Otherwise, variable reference may mutate later on inside the context

appending_to_in_memory_nwbfile = nwbfile is not None
file_initially_exists = Path(nwbfile_path).exists() if nwbfile_path is not None else False
append_mode = file_initially_exists and not overwrite
appending_to_disk_nwbfile = file_initially_exists and not overwrite

if metadata is None:
metadata = self.get_metadata()

self.validate_metadata(metadata=metadata, append_mode=append_mode)
self.validate_metadata(metadata=metadata, append_mode=appending_to_disk_nwbfile)
self.validate_conversion_options(conversion_options=conversion_options)

self.temporally_align_data_interfaces(metadata=metadata, conversion_options=conversion_options)

with make_or_load_nwbfile(
nwbfile_path=nwbfile_path,
nwbfile=nwbfile,
metadata=metadata,
overwrite=overwrite,
backend=backend,
verbose=getattr(self, "verbose", False),
) as nwbfile_out:
if no_nwbfile_provided:
if not appending_to_disk_nwbfile:

if appending_to_in_memory_nwbfile:
self.add_to_nwbfile(nwbfile=nwbfile, metadata=metadata, conversion_options=conversion_options)
else:
nwbfile = self.create_nwbfile(metadata=metadata, conversion_options=conversion_options)

configure_and_write_nwbfile(
nwbfile=nwbfile,
output_filepath=nwbfile_path,
backend=backend,
backend_configuration=backend_configuration,
)

else: # We are only using the context in append mode, see issue #1143

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."
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as with basedatainterface


backend = _resolve_backend(backend, backend_configuration)
with make_or_load_nwbfile(
nwbfile_path=nwbfile_path,
nwbfile=nwbfile,
metadata=metadata,
overwrite=overwrite,
backend=backend,
verbose=getattr(self, "verbose", False),
) as nwbfile_out:
self.add_to_nwbfile(nwbfile=nwbfile_out, metadata=metadata, conversion_options=conversion_options)

if backend_configuration is None:
backend_configuration = self.get_default_backend_configuration(nwbfile=nwbfile_out, backend=backend)
if backend_configuration is None:
backend_configuration = self.get_default_backend_configuration(nwbfile=nwbfile_out, backend=backend)

configure_backend(nwbfile=nwbfile_out, backend_configuration=backend_configuration)
configure_backend(nwbfile=nwbfile_out, backend_configuration=backend_configuration)

def temporally_align_data_interfaces(
self, metadata: Optional[dict] = None, conversion_options: Optional[dict] = None
Expand Down
55 changes: 10 additions & 45 deletions src/neuroconv/tools/testing/data_interface_mixins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Collaborator Author

@h-mayorquin h-mayorquin Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

metadata = self.interface.get_metadata()
if "session_start_time" not in metadata["NWBFile"]:
metadata["NWBFile"].update(session_start_time=datetime.now().astimezone())

nwbfile_path = str(tmp_path / f"conversion_with_backend_configuration{backend}-{self.test_name}.nwb")

nwbfile = self.interface.create_nwbfile(metadata=metadata, **self.conversion_options)
backend_configuration = self.interface.get_default_backend_configuration(nwbfile=nwbfile, backend=backend)
self.interface.run_conversion(
nwbfile_path=nwbfile_path,
nwbfile=nwbfile,
overwrite=True,
metadata=metadata,
backend_configuration=backend_configuration,
**self.conversion_options,
)

@pytest.mark.parametrize("backend", ["hdf5", "zarr"])
def test_configure_backend_for_equivalent_nwbfiles(self, setup_interface, tmp_path, backend):
Expand All @@ -169,7 +159,7 @@ def test_all_conversion_checks(self, setup_interface, tmp_path):
self.nwbfile_path = nwbfile_path

self.check_run_conversion_in_nwbconverter_with_backend(nwbfile_path=nwbfile_path, backend="hdf5")
self.check_run_conversion_in_nwbconverter_with_backend_configuration(nwbfile_path=nwbfile_path, backend="hdf5")
self.check_create_backend_configuration_with_converter(nwbfile_path=nwbfile_path, backend="hdf5")

self.check_read_nwb(nwbfile_path=nwbfile_path)

Expand Down Expand Up @@ -208,7 +198,7 @@ class TestNWBConverter(NWBConverter):
conversion_options=conversion_options,
)

def check_run_conversion_in_nwbconverter_with_backend_configuration(
def check_create_backend_configuration_with_converter(
self, nwbfile_path: str, backend: Union["hdf5", "zarr"] = "hdf5"
):
class TestNWBConverter(NWBConverter):
Expand All @@ -225,14 +215,6 @@ class TestNWBConverter(NWBConverter):

nwbfile = converter.create_nwbfile(metadata=metadata, conversion_options=conversion_options)
backend_configuration = converter.get_default_backend_configuration(nwbfile=nwbfile, backend=backend)
converter.run_conversion(
nwbfile_path=nwbfile_path,
nwbfile=nwbfile,
overwrite=True,
metadata=metadata,
backend_configuration=backend_configuration,
conversion_options=conversion_options,
)


class TemporalAlignmentMixin:
Expand Down Expand Up @@ -825,7 +807,7 @@ def test_metadata_schema_valid(self):
def test_run_conversion_with_backend(self):
pass

def test_run_conversion_with_backend_configuration(self):
def test_create_backend_configuration(self):
pass

def test_no_metadata_mutation(self):
Expand Down Expand Up @@ -889,7 +871,6 @@ def check_run_conversion_with_backend_configuration(
backend_configuration = self.interface.get_default_backend_configuration(nwbfile=nwbfile, backend=backend)
self.interface.run_conversion(
nwbfile_path=nwbfile_path,
nwbfile=nwbfile,
overwrite=True,
metadata=metadata,
backend_configuration=backend_configuration,
Expand All @@ -915,7 +896,7 @@ class TestNWBConverter(NWBConverter):
conversion_options=conversion_options,
)

def check_run_conversion_in_nwbconverter_with_backend_configuration(
def check_create_backend_configuration_with_converter(
self, nwbfile_path: str, metadata: dict, backend: Union["hdf5", "zarr"] = "hdf5"
):
class TestNWBConverter(NWBConverter):
Expand All @@ -929,14 +910,6 @@ class TestNWBConverter(NWBConverter):

nwbfile = converter.create_nwbfile(metadata=metadata, conversion_options=conversion_options)
backend_configuration = converter.get_default_backend_configuration(nwbfile=nwbfile, backend=backend)
converter.run_conversion(
nwbfile_path=nwbfile_path,
nwbfile=nwbfile,
overwrite=True,
metadata=metadata,
backend_configuration=backend_configuration,
conversion_options=conversion_options,
)

def test_all_conversion_checks(self, metadata: dict):
interface_kwargs = self.interface_kwargs
Expand All @@ -960,7 +933,7 @@ def test_all_conversion_checks(self, metadata: dict):
self.check_run_conversion_in_nwbconverter_with_backend(
nwbfile_path=self.nwbfile_path, metadata=metadata, backend="hdf5"
)
self.check_run_conversion_in_nwbconverter_with_backend_configuration(
self.check_create_backend_configuration_with_converter(
nwbfile_path=self.nwbfile_path, metadata=metadata, backend="hdf5"
)

Expand Down Expand Up @@ -1203,7 +1176,7 @@ def test_conversion_options_schema_valid(self):
def test_run_conversion_with_backend(self):
pass

def test_run_conversion_with_backend_configuration(self):
def test_create_backend_configuration(self):
pass

def test_no_metadata_mutation(self):
Expand Down Expand Up @@ -1262,7 +1235,7 @@ def check_run_conversion_with_backend_configuration(
backend_configuration = self.interface.get_default_backend_configuration(nwbfile=nwbfile, backend=backend)
self.interface.run_conversion(
nwbfile_path=nwbfile_path,
nwbfile=nwbfile,
metadata=metadata,
overwrite=True,
backend_configuration=backend_configuration,
**self.conversion_options,
Expand All @@ -1287,7 +1260,7 @@ class TestNWBConverter(NWBConverter):
conversion_options=conversion_options,
)

def check_run_conversion_in_nwbconverter_with_backend_configuration(
def check_create_backend_configuration_with_converter(
self, nwbfile_path: str, metadata: dict, backend: Union["hdf5", "zarr"] = "hdf5"
):
class TestNWBConverter(NWBConverter):
Expand All @@ -1301,14 +1274,6 @@ class TestNWBConverter(NWBConverter):

nwbfile = converter.create_nwbfile(metadata=metadata, conversion_options=conversion_options)
backend_configuration = converter.get_default_backend_configuration(nwbfile=nwbfile, backend=backend)
converter.run_conversion(
nwbfile_path=nwbfile_path,
nwbfile=nwbfile,
overwrite=True,
metadata=metadata,
backend_configuration=backend_configuration,
conversion_options=conversion_options,
)

def test_all_conversion_checks(self, metadata: dict):
interface_kwargs = self.interface_kwargs
Expand All @@ -1332,7 +1297,7 @@ def test_all_conversion_checks(self, metadata: dict):
self.check_run_conversion_in_nwbconverter_with_backend(
nwbfile_path=self.nwbfile_path, metadata=metadata, backend="hdf5"
)
self.check_run_conversion_in_nwbconverter_with_backend_configuration(
self.check_create_backend_configuration_with_converter(
nwbfile_path=self.nwbfile_path, metadata=metadata, backend="hdf5"
)

Expand Down
19 changes: 19 additions & 0 deletions tests/test_ecephys/test_ecephys_interfaces.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from hdmf.testing import TestCase
from packaging.version import Version

from neuroconv import ConverterPipe
from neuroconv.datainterfaces import Spike2RecordingInterface
from neuroconv.tools.nwb_helpers import get_module
from neuroconv.tools.testing.mock_interfaces import (
Expand Down Expand Up @@ -158,6 +159,24 @@ def test_group_naming_not_adding_extra_devices(self, setup_interface):
assert len(nwbfile.devices) == 1
assert len(nwbfile.electrode_groups) == 4

def test_error_for_append_with_in_memory_file(self, setup_interface, tmp_path):

nwbfile_path = tmp_path / "test.nwb"
self.interface.run_conversion(nwbfile_path=nwbfile_path)

nwbfile = self.interface.create_nwbfile()

expected_error_message = (
"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."
)
with pytest.raises(ValueError, match=expected_error_message):
self.interface.run_conversion(nwbfile=nwbfile, nwbfile_path=nwbfile_path, overwrite=False)

converter = ConverterPipe(data_interfaces=[self.interface])
with pytest.raises(ValueError, match=expected_error_message):
converter.run_conversion(nwbfile=nwbfile, nwbfile_path=nwbfile_path, overwrite=False)


class TestAssertions(TestCase):
@pytest.mark.skipif(python_version.minor != 10, reason="Only testing with Python 3.10!")
Expand Down
2 changes: 1 addition & 1 deletion tests/test_on_data/ecephys/test_recording_interfaces.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ def test_no_metadata_mutation(self):
def test_run_conversion_with_backend(self):
pass

def test_run_conversion_with_backend_configuration(self):
def test_create_backend_configuration(self):
pass

def test_interface_alignment(self):
Expand Down
Loading