🐛 FIX: Cleanup process event hooks

[chrisjsewell]

fixes #197

[chrisjsewell]

something like this @muhrin?

[muhrin]

Yes, this looks good. I also think that the class should just explicitly clear all event hooks callbacks when it terminates. The specification is quite clear about there being no more transitions after termination. If you do this it would be good to make sure it just ignore anyone 'unhooking' themselves and not being found in this case.

[chrisjsewell]

ok cheers, I'll have a think; the current solution is not quite working at the moment anyway, because cleanups are "initialised" in the init method, but hooks are initialised earlier in the __init__

[chrisjsewell]

perhaps this then: e0596e5? or is there a better place to put it?

[codecov-io]

Codecov Report

Merging #200 (bfa4011) into develop (33f4e9f) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #200      +/-   ##
===========================================
+ Coverage    90.91%   90.92%   +0.01%     
===========================================
  Files           22       22              
  Lines         2926     2927       +1     
===========================================
+ Hits          2660     2661       +1     
  Misses         266      266              

Impacted Files	Coverage Δ
plumpy/processes.py	92.46% <100.00%> (+0.02%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33f4e9f...bfa4011. Read the comment docs.

[chrisjsewell]

If you do this it would be good to make sure it just ignore anyone 'unhooking' themselves and not being found in this case.

not quite sure what you mean by this 😬 do you think this needs additional logic?

[ltalirz]

not quite sure what you mean by this 😬 do you think this needs additional logic?

I guess he meant that if you kept your original implementation where you registered the cleanup functions, one would need to make sure those don't error when you explicitly clear the event callbacks.
But it looks to me like your modified approach removes the need for this. ;-)

Just to check: this change does remove the references to the process in your test case, right?

[muhrin]

Ah, I just meant that if the class automatically clear all clients that have registered themselves for transition hooks it should not cause an error if a client then asks to unregister themselves after termination (which I presume it would it the callback method isn't found in the dictionary of registered clients).

[muhrin]

So, if I'm reading this current implementation this doesn't achieve what I'm getting at because for the moment you've just removed the call that would unregister the client. This is totally fine (as a client can now assume it is unregistered automatically on termination), however it should also be fine for a client to have 'dumb' code that unconditionally unregisters and this should always work (whether the process has terminated or not).

[chrisjsewell]

Just to check: this change does remove the references to the process in your test case, right?

Yes, applying this and also setting self.state_machine = None in Finished.__init__:

root@0d026cab8612:~# verdi shell
Python 3.8.5 (default, Jul 28 2020, 12:59:40) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.13.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import gc 
   ...: from aiida_sleep.cli import run_calc 
   ...: from aiida_sleep.sleep_job import SleepCalculation 
   ...: from pympler import muppy, refbrowser 
   ...:  
   ...: run_calc(payload=int(1e5), output=int(1e5))                                                                                   
Out[1]: <CalcJobNode: uuid: 0fc71c1f-b870-4fa4-b1e9-51e49443a377 (pk: 81) (aiida.calculations:sleep)>

In [2]: gc.collect() 
   ...: all_objects = muppy.get_objects() 
   ...: calcs = [o for o in all_objects if hasattr(o, "__class__") and isinstance(o, SleepCalculation)] 
   ...: len(calcs)                                                                                                                    
Out[2]: 1

In [3]: cb = refbrowser.ConsoleBrowser(calcs[0], maxdepth=3) 
   ...: tree = cb.get_tree() 
   ...: cb.print_tree(tree)                                                                                                           
aiida_sleep.sleep_job.SleepCalculation-+-list--dict-+-list
                                       |            +-dict
                                       |            +-dict
                                       |            +-dict
                                       |            +-dict
                                       |            +-dict
                                       |            +-IPython.core.interactiveshell.DummyMod
                                       |            +-dict
                                       |            +-dict
                                       |
                                       +-list--dict-+-list
                                       |            +-dict
                                       |            +-dict
                                       |            +-dict
                                       |            +-dict
                                       |            +-dict
                                       |            +-IPython.core.interactiveshell.DummyMod
                                       |            +-dict
                                       |            +-dict
                                       |
                                       +-list-+-list--dict
                                              +-hamt_bitmap_node-+-list
                                                                 +-hamt

so still not releasing, but the tree is getting smaller!

[chrisjsewell]

however it should also be fine for a client to have 'dumb' code that unconditionally unregisters and this should always work (whether the process has terminated or not).

so do you mean that in:

    def remove_state_event_callback(self, hook: Hashable, callback: EVENT_CALLBACK_TYPE) -> None:
        try:
            self._event_callbacks[hook].remove(callback)
        except (KeyError, ValueError):
            raise ValueError("Callback not set for hook '{}'".format(hook))

we should ignore a key error?

[ltalirz]

so still not releasing, but the tree is getting smaller!

If I understand correctly, then the first two references in the tree are the two lists you created in the ipython session (calcs and tree.children) as they are referenced by the ipython DummyMod.

That would leave only one list to be eliminated...

[chrisjsewell]

That would leave only one list to be eliminated...

It turns out this seems to be the big issue: aiidateam/aiida-core#4603 (comment)

[muhrin]

however it should also be fine for a client to have 'dumb' code that unconditionally unregisters and this should always work (whether the process has terminated or not).

so do you mean that in:

    def remove_state_event_callback(self, hook: Hashable, callback: EVENT_CALLBACK_TYPE) -> None:
        try:
            self._event_callbacks[hook].remove(callback)
        except (KeyError, ValueError):
            raise ValueError("Callback not set for hook '{}'".format(hook))

we should ignore a key error?

Yes, but only if it's in the terminal state.

[chrisjsewell]

Yes, but only if it's in the terminal state.

@muhrin see 4d5f1a5?

As a general update: #197 and #198 are not strictly necessary to fix the memory link, since python will clean up these cyclic references (once no other references remain). However, they do block automated garbage collection by cpython, and so without them (and perhaps anyway) we may need to think of some place to directly call garbage collection after a process has finished or something similar
(see aiidateam/aiida-core#4699 (comment) and aiidateam/aiida-core#4699 (comment))

[muhrin]

All good, if everyone is happy with this one please merge away.