populse / capsul

Collaborative Analysis Platform : Simple, Unifying, Lean
Other
7 stars 14 forks source link

tests on populse_mia are currently failing #283

Closed servoz closed 1 year ago

servoz commented 1 year ago

and only for the windows build ... (fine with linux and mac) .

======================================================================
ERROR: test_z_init_pipeline (__main__.TestMIAPipelineManagerTab)
Adds a process, mocks several parameters from the pipeline
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".\\python\\populse_mia\\test.py", line 10286, in test_z_init_pipeline
  File "C:\projects\populse-mia\python\populse_mia\user_interface\pipeline_manager\pipeline_manager_tab.py", line 1670, in init_pipeline
    print(study_config.engine.settings.select_configurations("global"))
  File "C:\projects\capsul\capsul\engine\settings.py", line 191, in select_configurations
    len(docs), module))
OSError: Cannot create configurations for environment "global" because settings returned 2 instances for module capsul.engine.module.spm

it seems to be from commit 153dab3 (fine with 8179433).

To fix this problem I'll have to use RDP from a windows virtual machine .... to see what's going on in the appveyor build ... If there is an obvious idea following changes in capsul, I'm interested !!!!

denisri commented 1 year ago

The capsul StudyConfig was missing something for a SPM standalone config with a MCR directory not inside the SPM directory. As the SPM install in our lab has recently changed in this way, I had to fix it quickly, but I have likely not done it the right way. I decided to use the spm_directory to rather represent the MCR directory (because there was no place it was configured, and spm_directory was containing info already in spm_executable), but doing this I changed the meaning of spm_directory, which is not right, and some code is not expecting that. I have to redo it a better way. I's a bit a mess because we are changing StudyConfig here, which is obsolete and is supposed not to be maintained any longer, then we must maintain sync with CapsulEngine, then with Axon config, and Populse MIA config... In other words it's probably going to take me the day to fix it in an obsolete and unmaintained API... well it's life...

servoz commented 1 year ago

Okay, I see and I sympathize!

But apart from that, I'm very surprised that the tests only crash for Windows in this case... Do you have any idea? If not, never mind!

Anyway, thanks for letting me know because I was about to start introspection in the windows build appveyor (which is always a pain!) ... for nothing!

Let me know when it's fixed on your end and I'll restart the populse_mia unit tests.

servoz commented 1 year ago

for populse_mia don't hesitate to let me know if I can help ... !

denisri commented 1 year ago

No, no idea why it fails only on windows. But our linux tests for Morphologist are also broken, so I hope fixing them (correctly this time!) will also fix your problem.

denisri commented 1 year ago

I have reworked the SPM config in master branch. Tests in brainvisa have not completely run however (because the test machine has been reinstalled with a new system) so I can't guarantee that it is OK (but manual tests did pass on linux for capsul and morphologist). But I'm not sure it will fix your issue either. If you could give it a try and tell me if anything has changed ? I have not backported to branch brainvisa-5.1 yet because I want to make sure it is OK in master, first.

servoz commented 1 year ago

I just restarted a test and it still crashes for Windows. Ok for macos and linux. I'll have to check in RDP on the windows build of appveyor. I'm currently doing something else. As soon as I've finished, I'll check in the windows appveyor build.

denisri commented 1 year ago

So the problem here is probably not related to my last fix. It seems related to 153dab346f3e8ea1b8635ed0739f986922973973 as you have noted, since this change removes the requirement "spm": "any' (because the FOM module does not depend on the spm module), but here, by doing this, it also removes the selection ("any" means the first matching config, while here we have 2). The real problems are thus:

denisri commented 1 year ago

The error raises "just" because of a print:

  File "C:\projects\populse-mia\python\populse_mia\user_interface\pipeline_manager\pipeline_manager_tab.py", line 1670, in init_pipeline
    print(study_config.engine.settings.select_configurations("global"))

and engine.settings.select_configurations("global") actually performs a selection where each module config should be unique, whereas we have two here. The code in the print shoud rather be replaced with:

    print(study_config.engine.settings.export_config_dict("global"))

export_config_dict does not perform a selection. Of course in the long run, the print should be removed.

The remaining question is: why do we have 2 configs for SPM on windows in this test, and only one on other platforms ?

servoz commented 1 year ago

As you say, there are actually two questions.

  1. Why the difference between linux like and windaube?
  2. The corresponding code in populse_mia (print(...)).

Regarding 1. I struggled today, in vain, with the RDP build. In fact, I'm able to enter the build but nothing seems to be initialised. In short, I can't get anywhere (although I'm sure I've debugged in the past directly in the appveyor build). I've opened a ticket at appveyor, but so far there's been no response.

On the one hand, my curiosity is driving me to go and find the reason for this difference, but on the other hand, I spend too much time being curious... Especially since :

point 2. I've just looked at the populse_mia's code snippet and I've got the impression that it's no longer necessary since I've reworked the requirements tests in mia (we talked about this in another ticket). In fact, it's a part I forgot to modify, or even delete, because now the requirement can't take None as value (so the test is looking at something that can't happen in real life ....)

So I'd rather fix point 2 in mia and save the curiosity point for later (humm ... it's usually ignored ...!).

I'll close this ticket in capsul and fix it in mia