👌 IMPROVE: Terminal states do not reference process #205
Conversation
This removes any cyclic references in finished processes, allowing for garbage collection to be be automatically triggered in CPython.
Codecov Report
@@ Coverage Diff @@
## develop #205 +/- ##
===========================================
- Coverage 90.66% 90.60% -0.05%
===========================================
Files 22 22
Lines 2952 2956 +4
===========================================
+ Hits 2676 2678 +2
- Misses 276 278 +2
Continue to review full report at Codecov.
|
|
pinging @muhrin, if you have time to have a look at some point cheers |
|
testing against aiida-core: aiidateam/aiida-core#4743 |
| ): | ||
| """ | ||
| :param process: The associated process | ||
| :param exception: The exception instance | ||
| :param trace_back: An optional exception traceback | ||
| """ | ||
| super().__init__(process) | ||
| super().__init__(None) # terminal state does not require process ref |
There was a problem hiding this comment.
Could you just elaborate on this comment a litle to say
'Terminal states don't get a backreference to the state machine so that it's easier for python to clear the memory'
or something to that effect just so that it's clear:
- Why this was done, and,
- So it's clear under which circumstances it's
None(which you've already stated actually)
| """ | ||
| :param state_machine: The process this state belongs to | ||
| """ | ||
| self.state_machine = state_machine | ||
| self.state_machine: Optional[StateMachine] = state_machine |
There was a problem hiding this comment.
I feel like a ValueError might be useful to be riased if state_machine is None for non terminal states. What do you think?
If it mattered more I would almost change the class hierarchy to have a TerminalState which doesn't even take it as an argument and is hardcoded to return False on is_terminal but I don't think it's worth it in this case but the exception might be useful. Just because I don't know how hardcoded the assumption is that the state_machine returns a non-None.
|
Hmm, from https://github.com/aiidateam/aiida-core/runs/1878917673?check_suite_focus=true, I think this might be causing something to hang in |
This was actually because of: aiidateam/aiida-core#4751 |
This removes any cyclic references in finished processes,
allowing for garbage collection to be be automatically triggered in CPython.
closes #198 and should be a partial fix for aiidateam/aiida-core#4603
Note, without this change, garbage collection of the process on termination is possible (i.e. CPython can handle circular references).
However, it means that it is not automatically triggered, and thus the garbage collection has to be called/triggered in some other way.
This has been shown to be an issue with the AiiDA daemons, which keep the process in memory (and can lead to large amounts of memory being held).
I can't think of any good way to trigger such a garbage collection on process termination, since the process can't call garbage collection on itself?
I am going to create an alpha release from this branch (https://pypi.org/project/plumpy/0.18.5a0/), in order to test that this does not lead to any issues in the aiida-core tests