Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[general need] Synchronisation between mia and capsul engine configuration is suboptimal, in V2 #185

Closed
servoz opened this issue May 10, 2021 · 84 comments
Assignees
Labels
enhancement New feature or request To discuss A decision is to be taken

Comments

@servoz
Copy link
Contributor

servoz commented May 10, 2021

Currently by going to File > Mia preferences it is possible to access the configuration settings for mia but also for capsul engine. In some cases this is completely redundant.
The synchronisation between the two is currently sub-optimal. We are getting there but I think that a simple user would be lost at the moment.
It would be necessary to have a perfect synchronisation (instantaneous) in both directions and that one adds or removes a configuration of module.
Currently it is not all the time easy and faster to do.

In fact we did not invest too much effort in this direction because this duplication should disappear (keep only mia prefrence and a synchronisation invisible to the user would be made at each change, or keep only the display of the capsul engine).

Another alternative, which goes in the direction of a reflection I had started and which I already mentioned, but which we never settled, would be to have a local or a remote mode of operation (soma-workflow does it, but we don't currently have a defined working mode in mia).
In this case, we could keep both.
The mia preferences would be keep for local use (where it is possible to test the validity of the config, which is currently done for spm and fsl) and capsul engine would be keep for remote use (where it may not be possible to check the config at the time of its definition). But this is beyond the scope of this ticket.

@servoz servoz added enhancement New feature or request To discuss A decision is to be taken labels May 10, 2021
@servoz servoz changed the title Synchronisation between mia and capsul engine configuration is suboptimal, in V2 [general need] Synchronisation between mia and capsul engine configuration is suboptimal, in V2 Aug 23, 2021
@servoz
Copy link
Contributor Author

servoz commented Jan 4, 2022

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

  • Until we can do better (perfect synchronisation between Mia's preferences and capsul engine configuration, this ticket), we comment out the CAPSUL part (including the Edit CAPSUL config button) in the Pipeline tab of the Mia's preferences.

The Mia's preferences synchronisation with capsul engine configuration works correctly now, but the opposite does not seem to be optimal. So, by commenting on the CAPSUL part, we won't have any more problems on that side. This is only a temporary solution, the best will be to synchronise properly in both directions.

@servoz servoz changed the title [general need] Synchronisation between mia and capsul engine configuration is suboptimal, in V2 [general need *] Synchronisation between mia and capsul engine configuration is suboptimal, in V2 Jan 4, 2022
@LStruber
Copy link
Collaborator

CAPSUL configuration has been commented in commit 0e8c820. I let the ticket open until the synchronisation is completely fixed.

@LStruber LStruber changed the title [general need *] Synchronisation between mia and capsul engine configuration is suboptimal, in V2 [general need] Synchronisation between mia and capsul engine configuration is suboptimal, in V2 Jan 11, 2022
@denisri
Copy link
Contributor

denisri commented Jan 12, 2022

Do you have an example / test of bad sync between capsul and mi configs ?
Well, I have added the capsul config because we use it to edit some settings not managed by mia, and now we cannot any longer :/ What's the reason to remove it ? To avoid confusing users ? Can we rather, at least, make it available in admin mode (since this mode exists, let's use it)
Mia is based on Capsul, thus should not reimplement config settings that are already managed by capsul... In a more general way I think all the config should go in capsul config, and have only one config system (whatever GUI we use on top of it, it's another question). Then sync between configs will not be a problem at all anymore.

@servoz
Copy link
Contributor Author

servoz commented Feb 3, 2022

I'm sorry I left this ticket out, but I'm running out of time.

That there are two configurations (capsul config and mia config) in Mia's preferences doesn't bother me at all, I even think it's good.

Indeed, since the beginning of the Mia project, I have always put as the most important parameter the simplicity, the ergonomics and the documented character of the things proposed to the user.

I have a high regard for capsul and everything that comes from BrainVisa in general, but I think it's generally difficult to access, to use, in short, a little bit elitist.

I admit, while I'm not a complete beginner, having a little trouble understanding exactly how the capsul config works (I speak of the GUI that was in the mia preferences).

At the risk of looking simple, I just want Mia to be as easy to use and ergonomic as possible.

That's why I think both currently have their places in Mia's preferences (to caricature a little, one - mia preference - for clinical use and the other - capsul config - for more experienced users). However, hopefully both should lead to the same result.

Anyway, the Mia config is now perfectly synchronised with the capsul config (I know this because I worked quite a bit on a now closed ticket where the goal was to make the mia config working!) and ultimately during the run it is the capsul config that is used!

However when I was working on this ticket I noticed that the opposite was not perfect (I change in capsul config and it is not reflected or badly in the config of mia). I could have continued the work in this sense (synchronises capsul config with Mia config) but I admit I didn't have the courage at this time... I don't think it's a big job, but at that moment I got a bit fed up with fix things that didn't work in V2...

Now to see what doesn't sync from capsul config to mia config, it's quite simple, just change the capsul config and see the result in the mia config... It's very quick to see, my remember that it works very badly!

I can of course help to give "minimum procedure to reproduce", but honestly if I remember correctly, we just have to make a change in capsul config and we quickly see that it does not synchronise in mia's preferences (or badly, in confess that I don't exactly remember). Another solution is for me to make the sync working. It's true that now that V2 is starting to work, I'm less worried about the large number of things that don't work, and i can plane to do it ...

@servoz
Copy link
Contributor Author

servoz commented Feb 8, 2022

I take this ticket.

@servoz servoz self-assigned this Feb 8, 2022
@servoz
Copy link
Contributor Author

servoz commented Feb 10, 2022

I found a little time to come back on this ticket.

  • playing around fsl config, no system environment variables defined:

    • Nothing defined in Mia preferences:
      Screenshot from 2022-02-10 15-30-15

    • So nothing is defined in capsul config (good synchronisation):
      Screenshot from 2022-02-10 15-30-32

    • In this case it is not possible to run a FSL process, this is normal.
      Screenshot from 2022-02-10 15-35-18

    • We define the FSL config file in Mia preferences, then we click on the OK button:
      toto

    • If we go back to Mia preferences, then to capsul config (click in the Edit CAPSU config), we can see that the synchronisation has been done with capsul config:
      Screenshot from 2022-02-10 15-58-24

    • Moreover, the FSL processes are now running very well:
      Screenshot from 2022-02-10 16-00-23

    • OK, let's say we don't want to do the configuration from Mia preferences but from capsul config. We start from no configuration for FSL:
      Screenshot from 2022-02-10 16-09-49

    • We click on the Edit CAPSUL config, then we take the FSL tab. Nothing is defined in capsul config (good synchronisation):
      Screenshot from 2022-02-10 16-13-04

    • So we fill in the necessary fields (normally it seems to me that capsul automatically deducts the other parameters from the config field, but to avoid any error here the first two are filled in):
      Screenshot from 2022-02-10 16-21-05

    • Then we click on the OK button:
      Screenshot from 2022-02-10 16-24-16

  • So no synchronisation is done with Mia preference. Maybe after clicking on the OK button? Let's click on the OK button, then we exit the Mia preferences. Let's go back to the MIA preferences:

    • No synchronisation with Mia preferences was done:
      Screenshot from 2022-02-10 16-27-56

    • In fact even in capsul config the FSL config is not here:
      Screenshot from 2022-02-10 16-28-16

Moreover it seems that the configuration is not kept when the GUI of capsul config is closed by clicking on the OK button because if, without leaving the Mia preferences, we click again on Edit CAPSUL config button, the FSL configuration is already lost. I will investigate this. However, I am away for a fortnight from tomorrow.

EDIT: This is why we decided to comment the capsul config GUI, as it is, it is just useless!

@servoz
Copy link
Contributor Author

servoz commented Feb 12, 2022

It's fixed for FSL.
If I remember well (I haven't retested yet) there was also a problem with matlab /spm.
I'll have a look later, in two weeks.

@servoz
Copy link
Contributor Author

servoz commented Feb 28, 2022

I start to see for Matlab/SPM.
Currently,

-1 if nothing is defined in Mia preferences,
rien
we observe in the GUI of capsul config:
matlab_rien
spm_rien
This looks good!

-2 Now if we define Matlab/SPM in mia preferences:
mia_pref_matlab
we observe in the GUI of capsul config:
matlab
spm
This looks good!

-3 Now if we define MCR/SPM standalone in mia preferences:
mia_pref_mcr
we observe in the GUI of capsul config:
matlabstandalone
spmstandalone
This looks good for spm and wrong for matlab. Am I wrong?
How should the value of the matlab executable field be in this case? /home/econdami/MATLAB/MATLAB_Runtime/v95 ?

@servoz
Copy link
Contributor Author

servoz commented Mar 1, 2022

In fact in capsul/study_config/config_modules/matlab_config.py, matlab_exec is defined as a File trait. So it is not possible actually to give here the path to the MCR (a Dir). So it is not possible to define the MCR path in the GUI of capsul config and it happens elsewhere in populse_mia, under the hood:

-> In populse_mia/software_properties.py (line 863):

       if capsul_config.get('use_spm', False):
            spm_dir = capsul_config.get('spm_directory')
            spm_standalone = capsul_config.get('spm_standalone')
            #TODO: I thing the following is wrong
            if spm_standalone:
                mcr = os.path.join(spm_dir, 'mcr', 'v713')
                if os.path.isdir(mcr) and os.path.isdir(spm_dir):
                    self.set_spm_standalone_path(spm_dir)
                    self.set_matlab_standalone_path(mcr)
                    self.set_use_spm_standalone(True)
                    self.set_use_matlab_standalone(True)
                    self.set_use_matlab(False)
            else:
                self.set_use_spm_standalone(False)
                if self.get_use_matlab():
                    self.set_spm_path(spm_dir)
                    self.set_use_spm(True)
                else:
                    self.set_use_spm(False)

As it is, it is not possible to synchronise capsul config with mia preferences, and worse, we have to use only mia pref to define the MCR path.

This is where I reach the limit of what I can do to close this ticket. I think the capsul config GUI should be able to accept a file or a directory for the "executable" field in the matlab tab. In this case, I'm not sure if "executable" is really suitable in the GUI ("path" would be better?).
Anyway, I need you to tell me what you have imagined on the subject. For now the whole thing works by chance for a bad reason (the hard-coded line: mcr = os.path.join(spm_dir, 'mcr', 'v713', see above)). If we want to put the capsul config GUI back in use we have to fix this problem. Then the synchronisation should work, with some modifications of the populse_mia codes.
I work in the with_capsul_config branch, where the Edit capsul config GUI is available.

@denisri
Copy link
Contributor

denisri commented Mar 1, 2022

Sorry for not following this earlier...
I think the key of the problem is that parameters have different meanings in MIA and in Capsul. In Capsul, matlab executable is just what it says: the Matlab program executable. When using a MCR version of SPM, there is no matlab executable. A MCR is something different, and should not be set instead of matlab executable. On the other hand, in Capsul we have not set a config variable for the MCR, because up to now we always have found only one MCR in the SPM standalone distribution, and have somewhat hard-coded it, which I agree is probably not enough. I think the solution here is to add a config variable in Capsul, rather than reusing matlab executable.

@servoz
Copy link
Contributor Author

servoz commented Mar 1, 2022

Of course! This is exactly what we did in mia pref. There is a matlab_exec and a matlab_standalone.
I respect your point of view, but, concerning matlab, whether it is an executable or not it is a file for an executable or a path in case of an MCR. It is more or less the same for SPM. In the case of standalone it is a file and in the case of "normal" SPM it is a directory ...
What would be nice is to be able to define in the GUI either the matlab_exec or the matlab_standalone.

@servoz
Copy link
Contributor Author

servoz commented Mar 1, 2022

For SPM we use in both cases the "directory" field, so it's ok for the file (.sh) or the directory (in fact we add the .sh, under the hood, to the directory, in case of a standalone spm). But for matlab as you want to keep the notion of "executable" it's less obvious (for me we need the directory for the matlab executable and like for spm we could add, under the hood). With this, a "directory" field is enough instead of the "executable" field. But if you have another quick solution, I am interested.
For the moment I'm stuck until we know how to do it in the capsul side, to define the bin/matlab in the case of matlab or the path to the MCR in the case of the Compiler Runtime).
Also, I believe that it is necessary to make simple for lambda users. For us the notions of executable or MCR are important. But for a lambda user I am not sure that it is very important!

@denisri
Copy link
Contributor

denisri commented Mar 1, 2022

Why use the same field for matlab executable and the MCR ? If both can be used to run SPM, in a more general way it is not the same at all: you can run matlab alone, you can't run the MCR (I guess); one is a file, the other is a directory. Thus it seems simpler and clearer for me to use two config fields. However if you really want to, we can use the traits.Either type to allow a file or a directory, but I'm not convinced it its the best solution.
For me I would add a field matlab_mcr in capsul config, and if it were just for me I would completely remove it from MIA's config, to avoid redundancies, even if we keep MIA's GUI to edit it: its specialized GUI is obviously better than the generic GUI in Capsul, but it could allow to edit the same single config.
But as you like... Tell me if you want me to add the missing field in Capsul...

@servoz
Copy link
Contributor Author

servoz commented Mar 1, 2022

I think that MCR can be used without spm standalone (for example if we want to use compiled matlab codes, I have to look into it as soon as possible because I have matlab codes that I want to use without access to the source codes). But this is not the question.
if it's possible to define both, it's ok for me. it's just that i don't see yet exactly how it will work to find it (for spm it's easy, you check standalone or not, so you know exactly what it means) ...

@servoz
Copy link
Contributor Author

servoz commented Mar 1, 2022

I don't think it's a good idea to remove matlab_mcr from mia pref. The idea is to be perfectly synchronised, that is to say to be able to define the same things in mia pref and capsul config .... and for the moment to define the MCR we have to use mia pref, capsul config does not allow it.

@denisri
Copy link
Contributor

denisri commented Mar 1, 2022

I don't think it's a good idea to remove matlab_mcr from mia pref

Why not ? Why is it a good thing to duplicate config options ?

capsul config does not allow it.

that's exactly why I ask if I should add it in capsul config.

The reason I insist is, in addition to not liking duplicating things, that, for me MIA GUI should be a graphical interface to run Capsul pipelines, which we should normally be able to run also outside of the GUI, typically to run them on a computing resource, from a script or something else. Capsul may allow extensions like the features of MIAProcesses, databasing and provenance tracking etc. There are many things that were developed inside MIA's GUI (see the pipeline_management_tab.py which is both the GUI and in a large part also processing infrastructure. I would like to separate them, probably not in the short term, but we want, at the end, to be able to run any processing from the commandline python -m capsul (or what it becomes in the future). If we keep things separated like today, with a config for capsul, and another for MIA, a separate processing/run engine/loop, etc, we will not reach this goal, which is for me a deal-breaker for the whole project.

@servoz
Copy link
Contributor Author

servoz commented Mar 1, 2022

To tell the truth, if the GUI of capsul config works as well as mia pref, I have no problem to delete mia pref ... The actual problem goes beyond the synchronisation issue ... The problem is that capsul config doesn't work for the MCR. It's mandatory to use mia pref ... That's why we have unplugged the GUI of capsul config in mia pref in master actually ...

@denisri
Copy link
Contributor

denisri commented Mar 1, 2022

I have added the matlab mcr_directory field in capsul config.

@servoz
Copy link
Contributor Author

servoz commented Mar 1, 2022

Thanks @denisri ! I will see ASAP how to deal with ...

@servoz
Copy link
Contributor Author

servoz commented Mar 28, 2022

Just tested! This is very nice.

Just one thing:
Is it possible to protect by a try the tab.accept() in SettingsEditor.accept() ?

I had protected it to avoid crashing Mia if a bad parameter is entered (typo, etc ...).

For example in caspul config, put an error in the spm path, then do ok -> Mia crash.

Then if we change by:

     try:
         tab.accept()
     except Exception as e:
         print(e)

by doing the typo, it doesn't crash anymore and a small message appears for the user in the standard output.

Is it possible to protect again the tab.accept() ?

@LStruber
Copy link
Collaborator

@servoz synchronization seems to work in both ways for AFNI and ANTS. I also added an AFNI example path in the MIA config window, but not for ANTS since ANTS puts all files in one big folder, that you choose in installation.

@servoz
Copy link
Contributor Author

servoz commented Mar 29, 2022

OK, thank you @LStruber !

@denisri
Copy link
Contributor

denisri commented Mar 29, 2022

Is it possible to protect by a try the tab.accept() in SettingsEditor.accept() ?

yes, sure. I'm doing it.

@denisri
Copy link
Contributor

denisri commented Mar 29, 2022

done.

@servoz
Copy link
Contributor Author

servoz commented Mar 29, 2022

Thanks @denisri !

servoz added a commit that referenced this issue Sep 19, 2022
servoz added a commit that referenced this issue Sep 19, 2022
- working on MATLAB / SPM
servoz pushed a commit that referenced this issue Sep 19, 2022
fixes in sync between capsul and mia
avoid using obsolete studyconfig
servoz pushed a commit that referenced this issue Sep 19, 2022
servoz pushed a commit that referenced this issue Sep 19, 2022
servoz added a commit that referenced this issue Sep 19, 2022
servoz added a commit that referenced this issue Sep 19, 2022
      - Sync from Mia pref to Capsul config: OK
servoz added a commit that referenced this issue Sep 19, 2022
          - Sync from Mia pref to Capsul config: OK
		- typo fix for ANTS
servoz added a commit that referenced this issue Sep 19, 2022
              - Sync from Capsul config to Mia pref: OK for ANTS, AFNI
		and FSL
servoz added a commit that referenced this issue Sep 19, 2022
                  - Sync from Capsul config to Mia pref: working on matlab/spm.

                    Because there are two parameters for matlab (executable and
                    mcr_directory) if the user defines both, we don't know which
                    one to choose! Here we choose to favour matlab in front
                    of MCR if both are chosen. This is suboptimal
servoz added a commit that referenced this issue Sep 19, 2022
                  - Sync from Capsul config to Mia pref: MATLAB / SPM OK
servoz added a commit that referenced this issue Sep 19, 2022
   - fix wrong checked status if spm/matlab
servoz pushed a commit that referenced this issue Sep 19, 2022
instead of recreating it without a version. This way we don't lose the
SPM version anymore. (#185)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request To discuss A decision is to be taken
Projects
None yet
Development

No branches or pull requests

4 participants