Moved file-open operation in BinaryRecordingSegment out of constructor

[cheydrick]

I recently report issue #3295 (BinaryRecordingExtractor opens the binary file, but doesn't close it, preventing later deleting or modifying binary file).

This PR fixes the problem that I described in that issue. I don't think it has any unwanted side-effects, though hopefully someone more familiar with the bigger picture will point out if it does!

[h-mayorquin]

Sup, @cheydrick, hope you are doing well in Texas.

The behavior in your PR is what I wanted to implement but @samuelgarcia pointed out some possible complication that we have not had time to test/experiment.

The problem is that if the file is remote it might take a while to open it (this is what we want to do experiments with). If I remember well @samuelgarcia did some small experiments with its clusters and find some small effect in that direction. I still believe that this effect is a small price to pay for IO security but leaving the file handles open was the compromise we reached.

Can you test the solution that @alejoe91 proposed on the linked issue? (to del the recording).
If that does not work, we can provide a method that iterates over all the open files and closes them.

We should do more conclusive experiments to assess the cost of re-opening the file on every call when the files are remote but now that I think about it maybe we could solve this with a flag. We will have to wait till Sam returns from his vacations to discuss it.

[cheydrick]

The suggestion that @alejoe91 made worked.

I had first tried setting recording = None thinking that would force the cleanup, but using del recording worked to release the files.

I closed the issue, but will wait for someone else to close this PR in case further discussion as you described should happen.

Thanks for the replying @alejoe91 and @h-mayorquin!

[cheydrick]

I updated #3295 to note that running Kilosort 4 prevents the .bin file from being deleted, despite first forcing deletion of the recording object. This PR remedies that, too, though obviously it doesn't address the big picture as @h-mayorquin noted.

[paolahydra]

If that does not work, we can provide a method that iterates over all the open files and closes them.

Hi @h-mayorquin, I would be interested in this patch since I am having the same problem!
Thanks,
Paola

[samuelgarcia]

Hi.
@h-mayorquin exactly. I am pretty sure that open/close file have a cost on network drive but I do not have number.
So I would not be in favor of this.

For me, when we do rec = read_something() then it is natural that the file is opened (ressource locked) and cannot be delete until del rec. Otherwise what would be the behavior of rec.get_trace() after deletion ?

[JoeZiminski]

It is the case that every time a data chunk is accessed the file would need to be reopened and memory mapped? e.g. preprocessing a 1h recording in 2 second chunks, this would result in 1800 reopen-memory map-close cycles? I am not so familiar with the performance costs of this operation but it seems suboptimal if it is the case.

However I've also got confused trying to figure out how to close the file handle to the recording object in a similar situation. Can this be solved by documentation and API e.g. can we add a recording.close_file() method? in this way the file handle can be closed without deleting the entire recording object. Also, we can mention in the docs that extractors will keep the file handle open unless explicitly closed with close_file().

[zm711]

I think there is some work on the Neo side for changing how memmapping will work for recording objects, but it has been on the backburner for a while.