tee-ar-ex / trx-python

Python implementation of the TRX file format
https://tee-ar-ex.github.io/trx-python/
BSD 2-Clause "Simplified" License
20 stars 16 forks source link

Try_except with warning #68

Closed frheault closed 9 months ago

frheault commented 9 months ago

@arokem I don't understand why tempfile.gettempdir() is returning a path for which we can't delete (PermissionError) in the Dipy test (but our pass).

What do you think? @skoudoro ? What I added in this PR is probably a good practice (trying to delete the folder, catch an error, and give a better error message).

On top of this for Dipy in my tests, I could set the environment variable (os.getenv('TRX_TMPDIR')) to some path I do have permissions (any suggestion of a path directory that I could use?)

arokem commented 9 months ago

Regarding differences with DIPY, I agree that it's very strange.

Maybe you can initialize the temporary directory in the same place that we put data? op.join(os.expanduser('~'), '.dipy')

frheault commented 9 months ago

I just remove the concurrency lines to investiguate, if this is the source maybe changing the folder is the easiest solution and still safe (since we will know what is the source)

Hopefully, tomorrow this will be solved. Getting close!

skoudoro commented 9 months ago

it would be great to reproduce the error here.

I do not believe that the user will try to find the folder and delete manually. Actually, there are some chances that even manually, he will not be able to delete the folder. He will have to kill the terminal and then be able to delete the folder.

I think the file do not close because he still have an active reference somewhere. In the case of DIPY, sft reference is still active when you close, and you still use sft which have a reference to the file. (due to memory map)

it might be some design issue, I am not sure. I did not check your test here but do you have such an example.

  1. load trx
  2. create a reference
  3. close TRX
  4. still doing something with the reference.
  5. check if TRX was closed or still open because of the reference.
frheault commented 9 months ago

@skoudoro I just added test_close_tmp_file to do explicitly what you said and it is still passing.

When loading the trx zip file, it creates a tmp folder, then I create a SFT, close the TRX and then I

arokem commented 9 months ago

Do you want to add a test with a context manager here, to see if that's somehow messing things up?

frheault commented 9 months ago

I am not sure how to do that since I create the tmp directory in the class, and I want to keep it active outside of the scope of the __init__ and pass the trx around freely.

skoudoro commented 9 months ago

I think your test is not complete. In your test.

skoudoro commented 9 months ago

Sorry, I missed the second loading on my previous description

frheault commented 9 months ago

I added a second loading (of the same trx), checking the data, then closing it again.

I also made the deepcopy of the tmp_dir.name (just a str) clearer so it is safer (just a string)

frheault commented 9 months ago

@arokem @skoudoro my new test is much more direct. test_close_tmp_files

Earlier I was checking if the folder stopped existing, now I am using psutil to actually check if there are any opened files by the process.

And they are passing (on github, on my linux & windows). So I think its all working as intended, but all of these new tests are very good to have!

(even if I have no idea why it is not working in Dipy)