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

Use context manager to avoid permission error on Windows #865

Merged

Conversation

jgerityneurala
Copy link
Contributor

@jgerityneurala jgerityneurala commented Nov 21, 2023

Pull Request Description:

The Model Compression Toolkit appears to have a bug on Windows caused by trying to remove a file that is still open. Linux allows for deleting a file that is still open, but Windows is much more particular about this operation and in general trying to remove a file that is still open somewhere will result in a PermissionError. Here's a standalone reproduction of the fundamental problem:

PS C:\Users\James> py -3.9
Python 3.9.9 (tags/v3.9.9:ccb0e6a, Nov 15 2021, 18:08:50) [MSC v.1929 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os, tempfile
>>> handle, tmpfile = tempfile.mkstemp()
>>> os.remove(tmpfile)  # File handle is still open, Windows will refuse to remove the file
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\James\\AppData\\Local\\Temp\\tmp8m6l666s'  
>>> os.close(handle)
>>> os.remove(tmpfile)  # If the file is first closed, it can be safely removed

I can confirm that changing the affected code inside of MCT to os.close() the handle before calling os.remove() does resolve the problem, but I have written this PR to use tempfile.NamedTemporaryFile as a context manager, which handles close/removal of the file automatically, and will also clean up the file if an error occurs during export.

Checklist before requesting a review:

  • I set the appropriate labels on the pull request.
  • [N/A] I have added/updated the release note draft (if necessary).
  • [N/A] I have updated the documentation to reflect my changes (if necessary).
  • [N/A] All function and files are well documented.
  • [N/A] All function and classes have type hints.
  • There is a licenses in all file.
  • The function and variable names are informative.
  • I have checked for code duplications.
  • [N/A] I have added new unittest (if necessary).

@reuvenperetz
Copy link
Collaborator

Hello @jgerityneurala,
Thanks for your contribution! :)

@reuvenperetz reuvenperetz merged commit c1551a4 into sony:main Nov 27, 2023
24 checks passed
@jgerityneurala jgerityneurala deleted the bugfix/fix-windows-file-contention branch November 27, 2023 14:33
jraynal added a commit to jraynal/model_optimization that referenced this pull request Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants