Use weakref.proxy where parent=self.

[nbiederbeck]

We dug a little to Fix #2218.

We found that the garbage collector of python does not clean up after tests have run. This seems to be the case because the tools specify parent=self, and when the tools are out of scope, somehow their children stay in memory and thus the references to the tools via the parent are still valid, which is why it cannot be garbage-collected.

We replaced parent=self with parent=weakref.proxy(self), which does not increment the reference counter. This makes it possible to be collected once the tools are out of scope.

We needed to fix a single test, where before we asserted that the parent of a children is the object itself, which is no longer valid, because it now is a Proxy.

These changes have two great outcomes: the total test time decreases in fact a little, and the total memory consumption is decreased more than 50% !

The root cause is probably in the configsystem, where we need the references to the parents. Changing this would likely fix the problem, and not just the symptoms.

The black line in the plot is this branch, the blue line is the current master.

Co-authored-by: Lukas Nickel lukas.nickel@tu-dortmund.de
Co-authored-by: Rune Dominik rune.dominik@tu-dortmund.de

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[maxnoe]

Nice!

[maxnoe]

Very good catch!

While I think it is the right thing to do, this needs a better usability. Can we somehow do this automatically in Component.__init__? E.g. do super().__init__(parent=weakref.proxy(parent)) ?

Any downside to this?

[nbiederbeck]

Any downside to this?

didn't think of that, I will check if that works.

[nbiederbeck]

diff --git a/ctapipe/core/component.py b/ctapipe/core/component.py
index 09294c38..0525f57e 100644
--- a/ctapipe/core/component.py
+++ b/ctapipe/core/component.py
@@ -1,5 +1,6 @@
 """ Class to handle configuration for algorithms """
 import warnings
+import weakref
 from abc import ABCMeta
 from inspect import cleandoc, isabstract
 from logging import getLogger
@@ -155,6 +156,8 @@ class Component(Configurable, metaclass=AbstractConfigurableMeta):
         with warnings.catch_warnings():
             warnings.filterwarnings("error", message=".*Config option.*not recognized")
             try:
+                if parent is not None:
+                    parent = weakref.proxy(parent)
                 super().__init__(parent=parent, config=config, **kwargs)
             except UserWarning as e:
                 raise TraitError(e) from None

black: master, blue: this branch, red: only the changes (relative to master) in the diff above.

might be the better approach.

[nbiederbeck]

what doesn't get proxied is when calling Reconstructor.read, because there we don't go via the __init__ but attach the parent via kwargs and setattr.

But I still like the two-line change much more and will implement it.

This will then fix the symptoms for quite some time, until we can fix the root cause.

[maxnoe]

@nbiederbeck Ok, but special-casing that only in Reconstructor.read is I think the much better solution and that method is already very "by hand" anyway

[nbiederbeck]

@nbiederbeck Ok, but special-casing that only in Reconstructor.read is I think the much better solution and that method is already very "by hand" anyway

I didn't even want to special-case that, just leave it as is. Still has a great improvement vs. master

[nbiederbeck]

Here are the changes from Component.__init__, additional including those in Reconstructor.read and using proxy in run_tool. As you can see, those additional changes don't make any difference.

I need to partially withdraw my earlier comment.

[maxnoe]

Still, using weakref is the correct solution I think. So we should add it also to the Reconstructor.read method.

[nbiederbeck]

Still, using weakref is the correct solution I think. So we should add it also to the Reconstructor.read method.

done

[kosack]

nice fix! This is much better than requiring component devs to understand that they need a weakref.