[iteration enhancement] exception when iterating a renamed pipeline

[LStruber]

Minimum steps to reproduce:

• Add a spatial_preprocessing brick in the pipeline manager
• Rename it (ex. spatial_preproc)
• Try to iterate the pipeline

MIA close with following standard output:

Traceback (most recent call last):
  File "/home/lucas/DEV/populse_dev/populse_mia/python/populse_mia/user_interface/pipeline_manager/pipeline_manager_tab.py", line 2640, in update_scans_list
    new_pipeline = self.build_iterated_pipeline()
  File "/home/lucas/DEV/populse_dev/populse_mia/python/populse_mia/user_interface/pipeline_manager/pipeline_manager_tab.py", line 956, in build_iterated_pipeline
    pipeline.remove_trait(plug)
  File "/casa/host/src/capsul/master/capsul/pipeline/pipeline.py", line 366, in remove_trait
    super(Pipeline, self).remove_trait(name)
  File "/casa/host/build/python/soma/controller/controller.py", line 347, in remove_trait
    super(Controller, self).remove_trait(name)
  File "/usr/local/lib/python3.6/dist-packages/traits/has_traits.py", line 2963, in remove_trait
    self.remove_trait(name + "_items")
  File "/casa/host/src/capsul/master/capsul/pipeline/pipeline.py", line 366, in remove_trait
    super(Pipeline, self).remove_trait(name)
  File "/casa/host/build/python/soma/controller/controller.py", line 352, in remove_trait
    self.user_traits_changed = True
  File "/usr/local/lib/python3.6/dist-packages/traits/trait_notifiers.py", line 478, in __call__
    self.notify_listener(self, object, trait_name, old, new)
  File "/usr/local/lib/python3.6/dist-packages/traits/trait_notifiers.py", line 553, in _notify_method_listener
    object, trait_name, old, new, listener
  File "/usr/local/lib/python3.6/dist-packages/traits/trait_notifiers.py", line 532, in _dispatch_change_event
    handle_exception(object, trait_name, old, new)
  File "/usr/local/lib/python3.6/dist-packages/traits/trait_notifiers.py", line 149, in _handle_exception
    raise excp
  File "/usr/local/lib/python3.6/dist-packages/traits/trait_notifiers.py", line 524, in _dispatch_change_event
    self.dispatch(handler, *args)
  File "/usr/local/lib/python3.6/dist-packages/traits/trait_notifiers.py", line 619, in dispatch
    handler(*args)
  File "/casa/host/src/capsul/master/capsul/qt_gui/widgets/pipeline_developper_view.py", line 395, in update_parameters
    self.update_node()
  File "/casa/host/src/capsul/master/capsul/qt_gui/widgets/pipeline_developper_view.py", line 1026, in update_node
    self._set_brush()
  File "/casa/host/src/capsul/master/capsul/qt_gui/widgets/pipeline_developper_view.py", line 823, in _set_brush
    node = pipeline.nodes[self.name]
KeyError: 'spatial_preprocessing_1_1'

Note that I do not reproduce this issue when iterating a simple brick (ex. smooth_1 brick renamed my_smooth) but the new iterated brick is named "iterated_smooth_1" instead of "iterated_my_smooth")

[LStruber]

1. Regarding the Exception: I had hard times understanding what was going on here... I'll try to explain it as simple as possible. In MIA when renaming a node, a value_changed signal is emited by the node_controller, calling the controller_value_changed function of pipeline_manager_tab.py. This function recover the current pipeline (pipeline = self.pipelineEditorTabs.get_current_pipeline()) and set it to the current editor (editor = self.pipelineEditorTabs.get_current_editor() and editor.set_pipeline(pipeline)). Here is the thing: set_pipeline function of Capsul pipeline_developper_view.py create a new PipelineScene (via release_pipeline function) without properly deleting the former. Then the old scene of our current editor has residuals connections with the nodes. Then, when iterating a pipeline in MIA (build_iterated_pipeline function in pipeline_manager_tab.py), the remove_trait function calls the update_parameters function of Capsul pipeline_developper_view.py (update_parameters is connected to user_trait_changed) BOTH in current PipelineScene and in the former one ! In the former, self.name is still assigned to the old name of the node throwing the Exception describe above.

The more simple and elegant way I found to fix it is to properly delete the previous scene (if it exists) before creating a new one. This translate by adding those lines (l.2750) in release_pipeline function of pipeline_developper_view.py:

if self.scene:
    self.scene.__del__() 

Note that to remove properly all remaining connections, I also had to add:

self.changed.disconnect()

in the __del__() method of the PipelineScene (pipeline_developper_view.py l.1524).

As these are modifications in Capsul, I'm not sure of all their implications, but it seems to solve the problem in MIA. If @denisri is OK, I'll go for a PR.

2. Regarding the error name when iterating a simple brick: This problem was far more simpler, we just needed to properly rename the context_name attributes of the renamed node. Nevertheless, working on this point raised a question: when renaming a pipeline (ex. Spatial_preprocessing pipeline), do we want to also update all child nodes full names ? It is currently not done, and to my mind it should be (I coded a small function in the node controller doing that in my branch) Currently, when renaming spatial_preprocessing_1_1 node spatial_preproc, all child nodes are still named after the old name, for example Pipeline.spatial_preprocessing_1_1.new_segment_1. The consequence is that after completion, brick name in Data Browser is spatial_preprocessing_1_1.new_segment_1 and not spatial_preproc.new_segment_1.

3. Note that all described problems and fixes were observed and performed using CapsulNodeController (V2 controller) and I still have to check what happen when using V1 controller

[servoz]

great !

[LStruber]

I just checked with the V1 controller. For 1. the Capsul modifications also solved the issue For 2. renaming child nodes can be done using the same function than for V2 controller

[servoz]

For the second point, I completely agree with you, we should also change the name of the children!

[denisri]

I agree with most, except that I suspect a more subtle problem for 1. Totally OK for 2. For 1., the thing that puzzles me is that you are calling self.scene.__del__() manually: this should not be done "by hand": when there are no remaining references on a python object, then it should be deleted automatically, and its __del__ method will be called automatically if it has one. If it is not called then

• either there are still references on the scene object - then calling its __del__ method on a still living object is definitely not good... Moreover calling __del__ does not destroy the object at all (__del__ is just the cleanup for an object which is about to be deleted) - basically you can't force an object deletion in python...
• or it's not deleted due to references cycles in objects. Some times a gc.collect() can help, but it may be very difficult to track why an objects still has references when it should not. To help debugging this the module objgraph is very helpful (it saved my life several times)

[LStruber]

I agree with that, my only concern is : does it worth to look into why scene object is not automatically deleted for a 'minor' issue of renaming ? It seems that looking into that won't be a happy time :)

[denisri]

For you it's possibly not worth ;) For us (me) it's quite important to guarantee that the library works well and without errors...

[LStruber]

Of course it is important ! The only thing I wanted to point out is that we can either fix the issue by a simple "patch" ensuring that the library works, or digging more deeply to understand precisely the why it happens and maybe find a better solution. Obviously the second solution is always better for the library, but as we have a lot to do, sometimes the first solution allows to focus on higher priorities...

To get back to our issue, without calling directly __del__ function, we can clean the old scene manually in release_pipeline function :

if self.scene:
    for gnode in self.scene.gnodes.values():
        gnode._release()
    del self.scene.gnodes
    self.scene.changed.disconnect()
    del self.scene
    import gc
    gc.collect()

[servoz]

if self.scene:
    for gnode in self.scene.gnodes.values():
        gnode._release()
    del self.scene.gnodes
    self.scene.changed.disconnect()
    del self.scene
    import gc
    gc.collect()

it looks almost clean (for a patch), doesn't it?

[denisri]

If the scene is actually deleted after del self.scene, or after gc.collect(), then it's completely good. If not, then we still have a memory leak but it won't crash at once: it will be OK for the current issue, but we should open another one in capsul.

[servoz]

did you try to see if after this patch, self.scene still exists (by doing for example just an if hasattr()) ?

[LStruber]

It prevents the crash but the __del__() method of PipelineScene is not called, then I guess it is not really deleted and the memory leak remains

[denisri]

Then there is still a leak, yes. We should open another ticket for that in Capsul. And you can push your modif to fix the current issue.

[LStruber]

Closed with commit 793f1d26f157a2b1decc495ded9202f3800484ef (Capsul) and commit e7c2a206fe3e79bc581a5f8cf931cee0e59a4b24. The memory leak remains and the corresponding ticket has been open in Capsul.