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] Issue 128: unregister TwinModel.__del__ from atexit and enhance PR116 #134

Merged
merged 13 commits into from
Dec 11, 2023

Conversation

chrpetre
Copy link
Collaborator

@chrpetre chrpetre commented Dec 7, 2023

Multiple enhancements to Fix #128:

  • Remove registration of TwinModel.del() to atexit module which was preventing garbage collector to call it at the end of a loop (see Issue TwinModel instance keep allocating memory when used in a loop #128 details), remove the TwinModel.del() method
  • Modify current logic to prevent issue with settings module trying to delete pytwin process temporary folder while some TwinRuntime objects have not closed their log file stored in the model directory. To do that, let TwinModel manages its model directory deletion and ensures its TwinRuntime log file has been released before deleting its model folder. This provides a cleaner fix to Issue [FIX] Register __del__ method of TwinModel to avoid issue atexit #116
  • Add an explicit close method to TwinModel.

chrpetre and others added 4 commits December 6, 2023 10:31
it covers all unit testing, examples as well as issue reported in the bug (for loop handled either via explicit load with no need of close but corresponding API available if needed, or using 'with' context)
@chrpetre chrpetre requested a review from lboucin December 7, 2023 11:05
@chrpetre chrpetre changed the title Fix/memory leak Fix issue #128 Dec 7, 2023
@github-actions github-actions bot added the bug Something isn't working label Dec 7, 2023
@lboucin lboucin changed the title Fix issue #128 [FIX] Issue 128: unregister TwinModel.__del__ from atexit and enhance PR116 Dec 7, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2023

Codecov Report

Merging #134 (c3c791b) into main (b4fe3d3) will increase coverage by 0.09%.
The diff coverage is 72.22%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #134      +/-   ##
==========================================
+ Coverage   88.68%   88.77%   +0.09%     
==========================================
  Files          11       11              
  Lines        2350     2360      +10     
==========================================
+ Hits         2084     2095      +11     
+ Misses        266      265       -1     

@chrpetre
Copy link
Collaborator Author

chrpetre commented Dec 7, 2023

@lboucin all tests are now green, ready to review

Copy link
Collaborator

@lboucin lboucin left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @chrpetre

@chrpetre chrpetre merged commit 3187e0b into main Dec 11, 2023
20 checks passed
@chrpetre chrpetre deleted the fix/memory_leak branch December 11, 2023 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TwinModel instance keep allocating memory when used in a loop
3 participants