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.

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

BinaryRecordingExtractor opens the binary file, but doesn't close it, preventing later deleting or modifying binary file #3295

Open
cheydrick opened this issue Aug 9, 2024 · 14 comments
Labels
core Changes to core module

Comments

@cheydrick
Copy link

I attempted to programmatically delete a .bin file I had used after performing operations using a BinaryRecordingExtractor object, but was getting an exception saying that the file was in use by another process.

Brief pseudo-code example:

# create BinaryRecordingExtractor instance
recording = si.read_binary("file.bin", num_channels=256, sampling_frequency=40000, dtype='int16')
# Perform an operation
recording.save_to_folder(folder='saved', overwrite=True)
# attempting to delete the .bin file fails
os.remove(binfile) 

I found that the BinaryRecordingSegment class (used within BinaryRecordingExtractor) opens the binary file when it's instanced, but never closes it.

I moved the file open operation to the get_traces() method, wrapping the relevant code in a 'with open()' statement, which fixed the problem. I will submit a PR shortly.

@alejoe91
Copy link
Member

alejoe91 commented Aug 9, 2024

@cheydrick have you tried del recording before removing the file?

@cheydrick
Copy link
Author

Ah, that did release it and allowed the file to be deleted.

I had first tried setting recording = None figuring that would cause the garbage collector to destruct the instance, but it's not likely it would happen immediately.

I'll close this issue and pick up the discussion in the PR.

@cheydrick
Copy link
Author

I'm going to re-open this issue to note that deleting the recording object does not release the file if Kilosort 4 is first run.

For example:

# create BinaryRecordingExtractor instance
recording = si.read_binary("file.bin", num_channels=256, sampling_frequency=40000, dtype='int16')
# Perform an operation
sorting = ss.run_sorter(sorter_name='kilosort4', recording=recording, output_folder="ks4_output")
# delete the objects, which should release the file
del recording
del sorting
# deleting the .bin file fails
os.remove(binfile) 

I can't tell if it's something within the run_sorter() code or within Kilosort itself that's holding on to the file. I haven't yet tried it with other sorters.

@cheydrick cheydrick reopened this Aug 9, 2024
@zm711
Copy link
Collaborator

zm711 commented Aug 9, 2024

KS4 makes its own file handle which is likely a different reference than the recording reference, so that could be the issue. del would delete the reference from recording, but would not necessarily delete the KS4 reference to the same file.

@cheydrick
Copy link
Author

Perhaps, though when I modified the BinaryRecordingSegment class to open the .bin file only when it needs it, and close right afterwards, the problem went away (#3296).

If KS4 was also opening the file, this modification wouldn't have changed anything, right? Maybe there's something in BaseSorter or Kilosort4Sorter that's making a copy of the BinaryRecordingExtractor class instance, which means deleting the original may do nothing.

@h-mayorquin
Copy link
Collaborator

Hi @cheydrick , I am looking for where the BInaryRecording is used within kilosort in our code. Can you point it out to me? I can only find references to write_binary_recorder.

@zm711
Copy link
Collaborator

zm711 commented Aug 14, 2024

from kilosort.io import load_probe, RecordingExtractorAsArray, BinaryFiltered

This is the Kilosort version of our extractor. They read straight from our extractors just without any mutliprocessing. So I think we would actually need to read their code to see how they really interact with any of our extractors.

@zm711
Copy link
Collaborator

zm711 commented Aug 14, 2024

Maybe this part: though I haven't read it too closely recently:

https://github.com/MouseLand/Kilosort/blob/a6ba59a93305efca3c89c3588d3d515412b67f95/kilosort/io.py#L1055-L1296

@h-mayorquin
Copy link
Collaborator

Mmm but I don't think that's the case. Otherwise, the changes to BinaryRecordingSegment that @cheydrick mentions would have not changed a thing, right?

@zm711
Copy link
Collaborator

zm711 commented Aug 15, 2024

You could be right. I'm just spit-balling. But for me it would be nice to have a little clarity. Is this only happening with KS4 or is this happening running any sorter that relies on binary recordings (so KS2,2.5,3). Or even MS5 would stay in python and uses a binary recording in its most recent iteration.

@h-mayorquin
Copy link
Collaborator

Do our sorters rely on BinaryRecording? I only saw references to write_binary_recorder (which writes binary data but does not use BinaryRecording or BinaryRecordingSegment. In other words, I am trying to understand how our sorting dispatch mechanism uses BinaryRecording is int load_recording_from_folder? Do you know how this is accessed.

@zm711
Copy link
Collaborator

zm711 commented Aug 15, 2024

Yeah, so the load_recording_from_folder will load the class of the file. So since we write the binary then during loading it reloads the extractor as a binary recording. As far as I know.

@cheydrick
Copy link
Author

I just ran my test program on my Linux machine (I had been using Windows 11) and can't replicate the problem. I can run Kilosort4, and can then delete the .bin file without needing to manually delete the recording object.

FYI.

@zm711
Copy link
Collaborator

zm711 commented Aug 15, 2024

This is the classic file release issue between Linux and Windows. Windows tries to eagerly do the deletion and Linux just puts it in the queue to be deleted later. So this is something that we constantly struggle with on all versions of Windows. I'm on a windows work station for lab and I've just come up with the workarounds I need for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Changes to core module
Projects
None yet
Development

No branches or pull requests

4 participants