-
Notifications
You must be signed in to change notification settings - Fork 120
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
Fix TemporaryDirectory error when exporting models on Windows #749
Fix TemporaryDirectory error when exporting models on Windows #749
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the fix @helena-intel
# This attribute is needed to keep one reference on the temporary directory, since garbage collecting | ||
# would end-up removing the directory containing the underlying OpenVINO model | ||
cls._model_save_dir_tempdirectory_instance = save_dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think this could be moved to OVBaseModel.__init__()
as done in :
optimum-intel/optimum/intel/openvino/modeling_diffusion.py
Lines 106 to 115 in 2c79d98
# This attribute is needed to keep one reference on the temporary directory, since garbage collecting | |
# would end-up removing the directory containing the underlying OpenVINO model | |
self._model_save_dir_tempdirectory_instance = None | |
if isinstance(model_save_dir, TemporaryDirectory): | |
self._model_save_dir_tempdirectory_instance = model_save_dir | |
self._model_save_dir = Path(model_save_dir.name) | |
elif isinstance(model_save_dir, str): | |
self._model_save_dir = Path(model_save_dir) | |
else: | |
self._model_save_dir = model_save_dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would require changing some more logic; it doesn't work out of the box. Do you want me to do that?
In https://github.com/huggingface/optimum-intel/blob/main/optimum/intel/openvino/modeling_base.py#L368 the TemporaryDirectory is converted to a Path, which is then past to both main_export
and _from_pretrained
. So OVBaseModel__init__()
model_save_dir
is not a TemporaryDirectory
but a Path
. And to prevent the TemporaryDirectory from automatically being deleted, _model_save_dir_tempdirectory_instance
must be set to the actual TemporaryDirectory instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the update, for now I think we can merge and will see if it can be added in a following PR
On Windows, exporting models results in a PermissionError because Python tries to delete the TemporaryDirectory containing OpenVINO model files when they were still in used. The model export works, but the error is confusing, it makes it difficult to debug issues on Windows, and the temporary directories are never deleted. This PR fixes that with the same method that is already used for stable diffusion models.