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

fix win #924

Closed
wants to merge 5 commits into from
Closed

fix win #924

wants to merge 5 commits into from

Conversation

eaidova
Copy link
Collaborator

@eaidova eaidova commented Oct 4, 2024

What does this PR do?

Fixes # openvinotoolkit/openvino#26844

force deleting openvino model and garbage collector during model export to prevent locking files in tmp dir due to mmap.
Additionally fixed installation from source on windows (in setup.py, subprocess.run raises FileNotFound error if can not find git) and refactoring weights compression part moving model size calculation during export and read model only if weights compression should be applied (based on model size or explicitly specified configuration, prevents OOM and invalid IR errors if nncf not installed)

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

@eaidova eaidova added the openvino-test Trigger OpenVINO slow tests label Oct 4, 2024
@HuggingFaceDocBuilderDev

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.

@eaidova eaidova changed the title Ea/fix win fix win Oct 4, 2024
Copy link
Collaborator

@nikita-savelyevv nikita-savelyevv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines +741 to +747
def __del__(self):
if hasattr(self, "request"):
del self.request
if hasattr(self, "model"):
del self.model
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this required or just to be safe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added for experiment to check that process exit issues will be resolved if we remove model and request in destructor. checking that attribute exists added for safety (if somebody already remove them before)

Comment on lines 407 to 419
def __del__(self):
if hasattr(self, "request"):
del self.request
if hasattr(self, "_original_model"):
del self._original_model
if hasattr(self, "model"):
del self.model
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question

@eaidova
Copy link
Collaborator Author

eaidova commented Oct 4, 2024

I still getting segmentation fault (probably caused double free memory corruption in my experiments with resolving tmp dir destruction, so I moved changes required for compression into separated PR to unblock people that waiting this issue resolution #925

@eaidova eaidova marked this pull request as draft October 4, 2024 14:29
@eaidova eaidova closed this Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openvino-test Trigger OpenVINO slow tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants