populse / populse_mia

Multiparametric Image Analysis
Other
10 stars 9 forks source link

[mia preferences] To be able to change the controller version later than only at startup #239

Open servoz opened 2 years ago

servoz commented 2 years ago

Currently, the controller of mia V2 allows to use only simple types. In order to allow the use of complex types, the V1 controller has been kept (and may still be needed?: is it possible to handle all complex types?, e.g. list of lists of lists?).

We can change the controller by clicking on the Version 1 controller checkbox (checked: V1, unchecked V2) in the mia preferences.

However, in order to take into account the change it is necessary to restart! This reminds me of the great time of Windows from microsoft ...

This is not a priority because we have other more important things to deal with first but it would be nice to find a solution so that we don't have to restart mia for just this!

servoz commented 2 years ago

After a private discussion with @LStruber , it was decided :

LStruber commented 2 years ago

I see two solutions here. We can either open the popup directly when the controller checkbox is toggled, or open the popup when OK button of preferences window is clicked, only if controller checkbox was modified. What do you think is prefereable?

Another point on this ticket: restarting properly the application does not seem to be effortless. I tried it in a new branch (fix#239) with this tutorial https://wiki.qt.io/How_to_make_an_Application_restartable. Restarting application is leading to a an error:

QPixmap: Must construct a QGuiApplication before a QPixmap
Fatal Python error: Aborted

Thread 0x00007f5587096700 (most recent call first):
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 299 in wait
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 551 in wait
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/pydevd.py", line 150 in _on_run
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/_pydevd_bundle/pydevd_comm.py", line 218 in run
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 916 in _bootstrap_inner
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 884 in _bootstrap

Thread 0x00007f5587897700 (most recent call first):
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/_pydevd_bundle/pydevd_comm.py", line 292 in _on_run
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/_pydevd_bundle/pydevd_comm.py", line 218 in run
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 916 in _bootstrap_inner
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 884 in _bootstrap

Thread 0x00007f5588098700 (most recent call first):
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 299 in wait
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/queue.py", line 173 in get
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/_pydevd_bundle/pydevd_comm.py", line 367 in _on_run
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/_pydevd_bundle/pydevd_comm.py", line 218 in run
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 916 in _bootstrap_inner
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 884 in _bootstrap

Current thread 0x00007f5591a3b740 (most recent call first):
  File "/home/lucas/populse_dev/populse_mia/python/populse_mia/main.py", line 468 in launch_mia
  File "/home/lucas/populse_dev/populse_mia/python/populse_mia/main.py", line 767 in main
  File "/home/lucas/populse_dev/populse_mia/python/populse_mia/main.py", line 1293 in <module>
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/_pydev_imps/_pydev_execfile.py", line 18 in execfile
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/pydevd.py", line 1483 in _exec
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/pydevd.py", line 1476 in run
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/pydevd.py", line 2164 in main
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/pydevd.py", line 2173 in <module>

Process finished with exit code 134 (interrupted by signal 6: SIGABRT)

By deleting app in main (del app) before restarting, restarting works but if the app is closed just after restarting the similar segmentation fault happens:

Fatal Python error: Segmentation fault

Thread 0x00007f6de0d8b700 (most recent call first):
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 299 in wait
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 551 in wait
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/pydevd.py", line 150 in _on_run
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/_pydevd_bundle/pydevd_comm.py", line 218 in run
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 916 in _bootstrap_inner
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 884 in _bootstrap

Thread 0x00007f6de158c700 (most recent call first):
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/_pydevd_bundle/pydevd_comm.py", line 292 in _on_run
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/_pydevd_bundle/pydevd_comm.py", line 218 in run
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 916 in _bootstrap_inner
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 884 in _bootstrap

Thread 0x00007f6de1d8d700 (most recent call first):
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 299 in wait
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/queue.py", line 173 in get
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/_pydevd_bundle/pydevd_comm.py", line 367 in _on_run
  File "/home/lucas/casa_distro/casa-dev-5.0-7/opt/pycharm-community-2021.3/plugins/python-ce/helpers/pydev/_pydevd_bundle/pydevd_comm.py", line 218 in run
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 916 in _bootstrap_inner
  File "/home/lucas/casa_distro/casa-dev-5.0-7/usr/lib/python3.6/threading.py", line 884 in _bootstrap

Current thread 0x00007f6deb730740 (most recent call first):
  File "/casa/host/src/capsul/master/capsul/qt_gui/widgets/pipeline_developer_view.py", line 2813 in release_pipeline
  File "/casa/host/src/capsul/master/capsul/qt_gui/widgets/pipeline_developer_view.py", line 2695 in __del__

Process finished with exit code 139 (interrupted by signal 11: SIGSEGV)

Any ideas ?

denisri commented 2 years ago

Is the restart feature only needed for the controller GUI version change ? If so it's probably not worth spending time on it since this controller version change is obviously a temporary feature. Better spending efforts on implementing a controller GUI that fits all needs. The v1 controller GUI does not support attributes (for attributes completion) thus is not suitable for the needs on our side (Morphologist for instance, but also basically everything that will come from the BrainVisa world). The v2 apparently doesn't support all needed types for your side - which types exactly are missing ? I don't have a complete overview. The v2 GUI, in my opinion, is not easy to understand and customize, and has bugs (some updates are not taken into account when modifying complex types such as lists of lists, in some situations), but is generic enough to be customizable and extensible. Maybe we should reimplement something, but which takes into account all needs, and which would completely replace soma.qt_gui.controller_widget and its use/extension in capsul.qt_gui.widgets.attributed_process_widget.

LStruber commented 2 years ago

For now yes, it is only needed to change controller version. I totally agree with you but I have no idea of the workload needed to reimplement a controller that fit all needs

sapetnioc commented 2 years ago

Well, I already did it but with a complete rewriting of Controllers so the effort may be important to make the switch. At the beginning it was just a branch to evaluate the interest of using standard Python 3 features for Controllers but finally, I went far in this branch. The major reasons why I have walked this path are :

denisri commented 2 years ago

I was thinking about this while I was writing my last message, and I'm happy you talk about it. However this is far more than a GUI rewrite, the changes are deep and not complete yet:

LStruber commented 2 years ago

For now, I just added a popup indicating to the user that MIA should be restarted to take the change into account (commit f1e65590d72f8a60cef77b869ca11b47bc4cba75). It is up to the user to do so.

servoz commented 2 years ago

For now, I just added a popup indicating to the user that MIA should be restarted to take the change into account (commit f1e6559). It is up to the user to do so.

Yes, it's a good idea since we really need to be able to switch easily from controller 1 to 2 and vice versa (2 may be better than 1, but it doesn't work as well for the moment as 1, which can handle all data types).

The only thing that bothers me in this patch is that we change the controller type in the preferences of Mia, and that from this moment in these preferences the controller type is not the same as the one in use in Mia. This is not serious for a user who knows that this is just a quick and dirty patch. But for an average user, he may just be surprised and continue to work like that with always this difference between Mia's preferences and the current controller state. Icing in the cake, the next time he starts Mia, there will be a change of controller, and he may have forgotten that a change was planned ...

I think that this patch should:

servoz commented 2 years ago

Moreover I observe a regression when testing controller 1 by chance: We lose the "run pipeline" button when we left-click on a brick or on an input or output node.

Minimum procedure to reproduce:

I don't think this regression was introduced during the work on this ticket, but this regression should be removed because currently we effectively make user to use exclusively the controller 2, which is far from working in all cases!

LStruber commented 2 years ago

I'm not sure about your proposal for the patch. Let's take an example:

servoz commented 2 years ago

In my opinion the best thing is to have the preferences settings (and thus their visualisation in File > Mia preferences) as close as possible to the Mia working state in order to avoid surprises or side effects. Thus the use of an editable object at the time of the use of File > Mia preferences and reachable at start-up or shut-down would be a solution. But there are certainly other solutions.

Ex. Startup of mia. object = controller version The user click to change the version in File > Mia preferences -> object = controller version at reboot, but he controller version does not change in the preferences. If the user comes back in File > Mia preferences, he will find the controller version currently running (that's why we get bored with this last modification I propose, to keep the preference settings synchronised with the current state of Mia !). If he decides to check again the change of controller he will do again object = version of the controller, which will not change anything. At the time of the shut-down we check if object has the same state as the running controller version. If so, no problem, we shut down. If not, we open a pop-up indicating that the controller version will be changed at the next start-up (ok, cancel). depending on the result (Ok or Cancel), we actually change the controller version in Mia's preferences (which will be used at the next start-up).

Anyway, in my opinion this is not the most important issue at the moment (we are trying to put the cleanest patch possible, but it is a trick to hide that version 2 does not work in all cases). The most important issue at the moment is that version 1 doesn't work fine, which is a big problem, because it can potentially prevent the use of some bricks, and therefore prevent the use of Mia! As I think that it is not tomorrow that the version 2 will be in condition to replace definitively the version 1, I believe that it is necessary to correct the regression about the version 1, then to make a patch as clean as possible for the change of version of the controller in order to propose the easiest possible use of Mia to the average users

I admit I didn't have currently the time to look at this in details, because I'm currently very busy on other sides

LStruber commented 2 years ago

I think I found out the problem of controller V1 that disabled the run pipeline button. I also implemented a solution for the "restart patch". Everything has been pushed into master branch. @servoz I let you check on it (I don't usually use V1 controller then I may have missed something), and reclose the issue if everything is fine for you.

servoz commented 2 years ago

Great, @LStruber you've done exactly what we need! I just added some minor changes to make the version change messages more explicit. I think we can remove the star in the title (the patch looks good). I do it. We can then close this ticket if we find a solution to allow a version change without restarting Mia or when the current work on the V2 controller will handle all the types we need.