Skip to content

Add overwrite option to save to folder#2156

Merged
alejoe91 merged 2 commits intoSpikeInterface:mainfrom
h-mayorquin:add_overwrite_to_save_to_folder
Nov 3, 2023
Merged

Add overwrite option to save to folder#2156
alejoe91 merged 2 commits intoSpikeInterface:mainfrom
h-mayorquin:add_overwrite_to_save_to_folder

Conversation

@h-mayorquin
Copy link
Copy Markdown
Collaborator

As in the title.

@alejoe91 alejoe91 changed the title Add ovewrite option to save to folder Add overwrite option to save to folder Nov 2, 2023
@alejoe91 alejoe91 added the core Changes to core module label Nov 2, 2023
@zm711
Copy link
Copy Markdown
Member

zm711 commented Nov 2, 2023

I'm wondering if this should explicitly be tested to make sure it will work on Windows. I think it is always tricky to predict when a shutil.rmtree will fail on windows. And since core is tested on Windows this is a chance to make sure it won't be problem for users.

@h-mayorquin
Copy link
Copy Markdown
Collaborator Author

I think it is a good idea to test this. I will read more about it. I do feel though that we have other overwrites somewhere else that are not tested but probably they should be tested as well.

Comment thread src/spikeinterface/core/base.py Outdated
@h-mayorquin
Copy link
Copy Markdown
Collaborator Author

@zm711 @alejoe91
I suspect that most of the times the problem with windows comes because they have different policy for deletion. In linux, you can mark a file for removal but it is not removed and in windows this is not possible. So, in linux if the file is being used the remove command will work but it wiill remove it later while in windows it will break.

Creating a test for this is definitley out of the scope in this PR. In fact, I think that most of these problems will appear because of our own in-house of multi-processing and IO (the ChunkRecordingExeccutor) and it should be tested in a multi plataform in that context.

Another possibility is that ther are issues with file permissions in which case we can try to use an onerror call back:
https://docs.python.org/3/library/shutil.html#shutil.rmtree

If you have seen some issue where it was a permission issues please point it to me but I also believe this out of scope for this PR as well as the solution should be more general.

@zm711
Copy link
Copy Markdown
Member

zm711 commented Nov 3, 2023

Another possibility is that ther are issues with file permissions in which case we can try to use an onerror call back:
https://docs.python.org/3/library/shutil.html#shutil.rmtree

That will be complicated because they recently changed that moving forward for 3.12 (onerror will become onexec). I tested that error on a Win32 error and it failed (while running the test suite on my machine and I tried to fix one test folder by incorporating that), but I think it works for Win5 errors. So I don't think that will be the overall solution.

If you have seen some issue where it was a permission issues please point it to me but I also believe this out of scope for this PR as well as the solution should be more general.

I'll track down those errors. By my recollection we have 3-5 issues where this has come up. Do you want me to just open an issue and link them all so that it is not in this PR?

out of scope for this PR as well as the solution should be more general.

As far as being out of scope for this PR. Fair enough for me.

@h-mayorquin
Copy link
Copy Markdown
Collaborator Author

h-mayorquin commented Nov 3, 2023

I'll track down those errors. By my recollection we have 3-5 issues where this has come up. Do you want me to just open an issue and link them all so that it is not in this PR?

That would be great. I just want to see if they are permission errors or in-use errors.

That will be complicated because they recently changed that moving forward for 3.12 (onerror will become onexec). I tested that error on a Win32 error and it failed (while running the test suite on my machine and I tried to fix one test folder by incorporating that), but I think it works for Win5 errors. So I don't think that will be the overall solution.

I am aware of the change but I thought that they only changed the signature of the last parameter. Can you desribe (in the issue that you will open) what error failed.

I will switch to windows later today to test some errors and I can take a look into it.

@zm711
Copy link
Copy Markdown
Member

zm711 commented Nov 3, 2023

I've got to work on some other stuff first, but I'll open the issue sometime this weekend when I have time to comb the issues.

@alejoe91
Copy link
Copy Markdown
Member

alejoe91 commented Nov 3, 2023

Hey guys! I agree that correctly handling Windows policies for file/folder deletion is super important but outside of the scope of this PR. Let's move the discussion on a specific issue as @zm711 suggested

@alejoe91 alejoe91 merged commit d92ae92 into SpikeInterface:main Nov 3, 2023
@h-mayorquin h-mayorquin deleted the add_overwrite_to_save_to_folder branch November 29, 2023 06:44
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

Successfully merging this pull request may close these issues.

3 participants