[Undo / Redo] Undo (Ctrl+z) changes in New Pipeline after adding a file in a input node makes Mia craches

[ghost]

---

I wanted to undo changes un the New Pipeline windows and it has gone wrong! I added a smooth brick from spm in mia_processes packages, exported all unconnected plugs and added a file to process in the node controller. In the New Pipeline windows, when I hit once on Ctrl+Z , It deletes the added file. If I hit a second time on Ctrl + Z, Mia crashes.

Down this read is the generated error:

Traceback (most recent call last):
  File "/casa/home/DEV/populse_dev/populse_mia/python/populse_mia/user_interface/main_window.py", line 1567, in undo
    self.pipeline_manager.undo()
  File "/casa/home/DEV/populse_dev/populse_mia/python/populse_mia/user_interface/pipeline_manager/pipeline_manager_tab.py", line 2458, in undo
    c_e.update_plug_value(node_name, old_value, plug_name,
  File "/casa/home/DEV/populse_dev/populse_mia/python/populse_mia/user_interface/pipeline_manager/pipeline_editor.py", line 1120, in update_plug_value
    self.scene.pipeline.nodes[node_name].set_plug_value(
  File "/casa/host/src/capsul/master/capsul/pipeline/pipeline_nodes.py", line 791, in set_plug_value
    self.process.set_parameter(plug_name, value, protected)
  File "/casa/host/src/capsul/master/capsul/process/process.py", line 1043, in set_parameter
    setattr(self, name, value)
  File "/usr/local/lib/python3.10/dist-packages/nipype/interfaces/base/traits_extension.py", line 425, in validate
    value = super(MultiObject, self).validate(objekt, name, newvalue)
  File "/usr/lib/python3/dist-packages/traits/trait_types.py", line 2699, in validate
    return TraitListObject(self, object, name, value)
  File "/usr/lib/python3/dist-packages/traits/trait_list_object.py", line 582, in __init__
    super().__init__(
  File "/usr/lib/python3/dist-packages/traits/trait_list_object.py", line 213, in __init__
    super().__init__(self.item_validator(item) for item in iterable)
  File "/usr/lib/python3/dist-packages/traits/trait_list_object.py", line 213, in <genexpr>
    super().__init__(self.item_validator(item) for item in iterable)
  File "/usr/lib/python3/dist-packages/traits/trait_list_object.py", line 865, in _item_validator
    return trait_validator(object, self.name, value)
  File "/usr/lib/python3/dist-packages/traits/trait_handlers.py", line 873, in validate
    return self.slow_validate(object, name, value)
  File "/usr/lib/python3/dist-packages/traits/trait_handlers.py", line 881, in slow_validate
    self.error(object, name, value)
  File "/usr/lib/python3/dist-packages/traits/base_trait_handler.py", line 74, in error
    raise TraitError(
traits.trait_errors.TraitError: Each element of the 'in_files' trait of a Pipeline instance must be a pathlike object or string representing a file or a _Undefined or None, but a value of "['/casa/home/Projects/test_0/data/raw_data/alej170316-IRMFonct_+perfusion-2016-03-17083444-01-T13DSENSE-T1TFE-000425_000.nii']" <class 'str'> was specified.

To Reproduce Steps to reproduce the behavior:

1. Go to Pipeline Manager
2. In the packages tab , select mia_processes-> bricks -> processes -> spm.
3. Drag and drop smooth brick in New Pipeline windows
4. Right click on smooth_1 node then export unconnected mandatory inputs
5. Left click on inputs node and on the node controller choose Filter and add the file to smooth.
6. Click on New Pipeline windows and hit Ctrl + Z once and twice : here it is ! Mia crashes!

Before the fist Ctrl+Z

After hitting Ctrl+Z you see in the node controller that the added file just desapeared

By hitting again Ctrl+Z it is Mia it selt which desapear

Mia shouldn't crash. When hitting Ctrl+Z it should simply undo an action.

EDIT It seems to me that the minimum procedure to reproduce is not strictly the right one. On my station the procedure that produces the bug is:

1. Go to Pipeline Manager
2. In the packages tab , select mia_processes-> bricks -> processes -> spm
3. Drag and drop smooth brick in New Pipeline windows
4. Right click on smooth_1 node then export unconnected mandatory inputs
5. Left click on the the smooth_1 node, then left click on the inputs node
6. In the node controller hit the Filter button then select the file to smooth and click Ok button
7. Press (Ctrl + Z) keys once and twice : here it is ! Mia crashes!

[servoz]

It seems that this solution is not effective. It doesn't really do an undo. Did you notice that the undo doesn't really do the operations in reverse order anymore?

It does: 1- put the brick in the editor pipeline 2- add the nodes 3- add/modify the value of a plug.

So undo should do : 3- return to the initial value of the plug 2- delete the nodes 1- delete the brick

Now undo does : (3+2)- at the same time (not what is desired) 1- delete the brick

I don't think the problem is with on_trait_change().

As we discussed in a private message, I think to fix this issue, we need to understand and track what happens when we do an action (mainly how the undos object is constructed during that action). Then we need to track exactly what happens when we do an undo (what happens from the undos object? how does it evolve? how is the redos object, that comes from the undo, handled? etc). From there, it should be possible to fix this issue efficiently.

While testing just now I noticed that there are other problems when we are with version 1 of the node controller (for example when we do a redo). As I told you, I have a limited confidence, for various reasons, in the current codes managing undo and redo functions. I think it's the opportunity to examine undo/redo from all side and to make the modifications with a global vision of the processes.

[ghost]

You are right. I also noticed that the modifications did not resolve the issue. Undo Object is okay when the process node is not clicked. Things start going wrong once we click on the process brick before a undo action, leading to an irregular filling of the undo object. I'm examinig this last one.

[servoz]

I am working on this ticket. It seems to me that the minimum procedure to reproduce is not strictly the right one. With the one proposed I do not observe the bug. I will make an edit of the first post with the procedure that produces the bug on my station

[servoz]

In the Mia operation, we clearly have a weakness that causes a big issue in the management of the undo/redo, in the case of a change of node plug value, using the Filter button of the Mia GUI controller (this ticket), or not (more generally).

It seems that this weakness stems from populse_mia.user_interface.pipeline_manager.node_controller.CapsulNodeController.display_parameters().

This method is triggered when the user clicks on a node. In this method, the Mia GUI for the node controller will be created and at the end of this method there is a notification to self.parameters_changed() when the values of a process (traits) are changed.

The purpose of self.parameters_changed is only to send a signal to the controller_value_changed() slot of the populse_mia.user_interface.pipeline_manager.pipeline_manager_tab.PipelineManageTab class. The job of controller_value_changed() is to update the undo history (the populse_mia.user_interface.pipeline_manager.pipeline_editor.PipelineEditorTabs.undos object). So this is a very elegant way to automatically update the undo history (as in general what @denisri codes! - unless I'm mistaken, it's you @denisri who coded this part ?).

However, this has a weakness because if the user clicks on two nodes that share the same plug (e.g. a node at the beginning or end of a pipeline, which may have plugs linked to the "inputs" and "outputs" pipeline nodes), there will be two passes through parameters_changed(), so the same undo history is added to the undos object twice.

So if I do the following procedure:

• 0 Load a project in data browser (data).
• 1 Go to Pipeline Manager.
• 2 In the packages tab, select mia_processes-> bricks -> processes -> spm.
• 3 Drag and drop smooth brick in New Pipeline windows.
• 4 Right click on smooth_1 node then hit export all unconnected plugs.
• 5 Left click on the inputs node -> we observe the GUI controller on the right part of the pipeline editor, with the Filter button for the in_files plug.
• 6 In the node controller GUI hit the Filter button then select the file to smooth and click Ok button.

In this case we observe: self.pipelineEditorTabs.undos: {<populse_mia.user_interface.pipeline_manager.pipeline_editor.PipelineEditor object at 0x7fdf960eb8b0>: [ ['add_process', 'smooth_1', <class 'mia_processes.bricks.preprocess.spm.spatial_preprocessing.Smooth'>], ['export_plugs', ['in_files', 'fwhm', 'data_type', 'implicit_masking', 'out_prefix', 'smoothed_files'], 'smooth_1'], ['update_plug_value', '', , 'in_files', <class 'str'>, ['/home/econdami/Data/IRM/projects_mia/test/data/raw_data/alej170316test24042018-IRMFonct+perfusion-2016-03-17083444-00-T13DSENSE-T1TFE-000425_000.nii']]]} So what is expected (one add_process, one export_plugs et one update_plug_value) !

Now if we do the following procedure:

• 0 Load a project in data browser (data).
• 1 Go to Pipeline Manager.
• 2 In the packages tab, select mia_processes-> bricks -> processes -> spm.
• 3 Drag and drop smooth brick in New Pipeline windows.
• 4 Right click on smooth_1 node then hit export all unconnected plugs.
• 5 Left click on the smooth_1 node.
• 6 Left click on the inputs node -> we observe the GUI controller on the right part of the pipeline editor, with the Filter button for the in_files plug.
• 7 In the node controller GUI hit the Filter button then select the file to smooth and click Ok button.

In this case we observe: self.pipelineEditorTabs.undos: {<populse_mia.user_interface.pipeline_manager.pipeline_editor.PipelineEditor object at 0x7f99680a28b0>: [ ['add_process', 'smooth_1', <class 'mia_processes.bricks.preprocess.spm.spatial_preprocessing.Smooth'>], ['export_plugs', ['in_files', 'fwhm', 'data_type', 'implicit_masking', 'out_prefix', 'smoothed_files'], 'smooth_1'], ['update_plug_value', '', , 'in_files', <class 'str'>, ['/home/econdami/Data/IRM/projects_mia/test/data/raw_data/alej170316test24042018-IRMFonct+perfusion-2016-03-17083444-00-T13DSENSE-T1TFE-000425_000.nii']], ['update_plug_value', '', , 'in_files', <class 'str'>, ['/home/econdami/Data/IRM/projects_mia/test/data/raw_data/alej170316test24042018-IRMFonct+perfusion-2016-03-17083444-00-T13DSENSE-T1TFE-000425_000.nii']]]} So twice the value change for the in_file plug (one for the mia_processes.bricks.preprocess.spm.spatial_preprocessing.Smooth object and one for the capsul.pipeline.pipeline.Pipeline object), while it only had one value change action. This can only lead to a mess later on during the undo step ...

In fact, all cases of plug value change in a node (process parameter) can be impacted (not only using the Filter button, as in the first post),

For example by following the procedure below:

• 1 Go to Pipeline Manager.
• 2 In the packages tab, select mia_processes-> bricks -> processes -> spm.
• 3 Drag and drop smooth brick in New Pipeline windows.
• 4 Right click on smooth_1 node then hit export all unconnected plugs.
• 5 Left click on the inputs node.
• 6 Left click on the smooth_1 node.
• 7 In the GUI controller on the right part of the pipeline editor, change the value for out_prefix from 's' to 't'.
• 8 In the GUI controller on the right part of the pipeline editor, change the value for out_prefix from 't' to 'u'.

In this case we observe: self.pipelineEditorTabs.undos : {<populse_mia.user_interface.pipeline_manager.pipeline_editor.PipelineEditor object at 0x7f710c84da60>: [ ['add_process', 'smooth_1', <class 'mia_processes.bricks.preprocess.spm.spatial_preprocessing.Smooth'>], ['export_plugs', ['in_files', 'fwhm', 'data_type', 'implicit_masking', 'out_prefix', 'smoothed_files'], 'smooth_1'], ['update_plug_value', 'smooth_1', 's', 'out_prefix', <class 'str'>, 't'], ['update_plug_value', 'smooth_1', 's', 'out_prefix', <class 'str'>, 't'], ['update_plug_value', 'smooth_1', 't', 'out_prefix', <class 'str'>, 'u'], ['update_plug_value', 'smooth_1', 't', 'out_prefix', <class 'str'>, 'u']]} When there were only two actions: from s to t and from t to u.

As a result, in this case, Mia doesn't crash at all ... but by pressing Ctrl + Z, we will go from u to t and from t to u just at the end of time without ever coming back to s!!!

This ticket is a bit tricky. I have some vague ideas (not sure yet if they work without testing) to fix it. I still need to think about it a bit. @denisri , I'm very interested in your point of view on the subject.

[servoz]

Ok, I think I'm starting to understand the issue.

The idea of the populse_mia.user_interface.pipeline_manager.node_controller.CapsulNodeController.display_parameters() method is to check if the node controller GUI exists, if so to destroy it and create a new one.

The issue is that currently the destroyed signal for self.process_widget is not caught due to a typo and the result is that the handler is not "unhooked" (never passed by static_release()).

I think we should use self.process_widget.destroyed.connect(partial(self.static_release, process, self.parameters_changed)) instead of self.destroyed.connect(partial(self.static_release, process, self.parameters_changed))

This way, it goes through static_release() and the self.parameters_changed is "unhooked".

This solves the original reason for this ticket (procedure in EDIT of the first post).

But ... This does not solve other problems.

For example, if we click twice on the smooth_1 node, if we change a parameter (say out_prefix) the undo history is not updated (we can't undo using Ctrl + Z).

So I think that we have to change self.process_widget.destroyed.connect(partial(self.static_release, process, self.parameters_changed)) right after deleting self.process_widget (as in 47b4421).

I think this fix is effective for this ticket. I also think we can close this ticket. @denisri, I look forward to hearing your thoughts on this story. Feel free to comment and why not reopen this ticket if necessary.

While working on this ticket, I realised that there are quite a few things wrong with undo/redo. I think we need to carefully inspect the operation of undo/redo in order to resolve any issues with this feature.