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

add logic to erase file if writing fails #912

Merged
merged 1 commit into from
Jun 13, 2024
Merged

add logic to erase file if writing fails #912

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 18 additions & 9 deletions src/neuroconv/tools/nwb_helpers/_metadata_and_file_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,9 @@
from typing import Literal, Optional
from warnings import warn

import h5py
from hdmf_zarr import NWBZarrIO
from pydantic import FilePath
from pynwb import NWBHDF5IO, NWBFile, get_nwbfile_version
from pynwb import NWBHDF5IO, NWBFile
from pynwb.file import Subject

from . import BackendConfiguration, configure_backend, get_default_backend_configuration
Expand Down Expand Up @@ -195,13 +194,15 @@ def make_or_load_nwbfile(
raise NotImplementedError("Appending a Zarr file is not yet supported!")

load_kwargs = dict()
success = True
file_initially_exists = nwbfile_path_in.exists() if nwbfile_path_in is not None else None
if nwbfile_path_in:
load_kwargs.update(path=str(nwbfile_path_in))
if file_initially_exists and not overwrite:

append_mode = file_initially_exists and not overwrite
if append_mode:
load_kwargs.update(mode="r+", load_namespaces=True)

# Check if the selected backend is the backend of the file in nwfile_path
backends_that_can_read = [
backend_name
for backend_name, backend_io_class in BACKEND_NWB_IO.items()
Expand All @@ -221,27 +222,35 @@ def make_or_load_nwbfile(

io = backend_io_class(**load_kwargs)

nwbfile_loaded_succesfully = True
try:
if load_kwargs.get("mode", "") == "r+":
nwbfile = io.read()
elif nwbfile is None:
nwbfile = make_nwbfile_from_metadata(metadata=metadata)
yield nwbfile
except Exception as e:
success = False
raise e
except Exception as load_error:
nwbfile_loaded_succesfully = False
CodyCBakerPhD marked this conversation as resolved.
Show resolved Hide resolved
raise load_error
finally:
if nwbfile_path_in:
nwbfile_written_succesfully = True
try:
if success:
if nwbfile_loaded_succesfully:
io.write(nwbfile)

if verbose:
print(f"NWB file saved at {nwbfile_path_in}!")
except Exception as write_error:
nwbfile_written_succesfully = False
raise write_error
Comment on lines +244 to +246
Copy link
Member

Choose a reason for hiding this comment

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

I see, you also added a capture block here to further scope the success of the operation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. I think this is the core. Separating errors from loading or creating from errors of writing.

But as you were mentioning maybe us handling unlinking is not a good idea ...

Copy link
Member

Choose a reason for hiding this comment

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

In this case we're attempting to leave the system as we left it, which is fine

The case I am against is what you don't have here

if any_error_condition and file_initially_exists:
    nwbfile_path_in.unlink()

finally:
io.close()

if not success and not file_initially_exists:
if not nwbfile_loaded_succesfully and not file_initially_exists: # Not sure we need this
CodyCBakerPhD marked this conversation as resolved.
Show resolved Hide resolved
nwbfile_path_in.unlink()

if not nwbfile_written_succesfully and not file_initially_exists:
Copy link
Member

Choose a reason for hiding this comment

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

Aside from better variable naming, the key difference in this PR is this line by limiting the global notion of 'success' to just 'success during write process' and if file didn't initially exist but write started and failed, try to clean it up

Emphasis on 'try' since not guaranteed to succeed without error

nwbfile_path_in.unlink()


Expand Down
Loading