search

populse / populse_mia

Multiparametric Image Analysis
Other
10 stars 9 forks source link

It seems there is currently a bug in the pipelineManager's process list building #196

Closed servoz closed 3 years ago

servoz commented 3 years ago

Classes that are not bricks/processes are considered as bricks/processes in the pipelineManager library. I will take a look at this.

denisri commented 3 years ago

That's possible, I wanted to allow other kinds of Nodes (like switches and other custom non-process nodes) in pipelines. I can check when I find time...

servoz commented 3 years ago

no problem, I do it. I have plenty of time ..

denisri commented 3 years ago

and processes appear twice, right now. I'm looking at it.

denisri commented 3 years ago

I have pushed a modif. Some nodes (capsul switch for instance) cannot be dragged/dropped yet (because they are older nodes and don't comply to the newer API to instantiate nodes, I'm fixing that also).

servoz commented 3 years ago

Well ... I wanted to do it but it's been several days since I had the time to do what I had planned to do several days ago ... I was just about to start ... so i will see your fix !

denisri commented 3 years ago

Switches and other nodes have been fixed: you can use them using drag & drop in the editor.

servoz commented 3 years ago

OK, it seems to work, but before there was no need to define an object with things not to be added, _already_loaded (which will have to be maintained if we add new things not to be added), there was no need to exclude tests and there was no need to define counter-intuitive things like 'populse_mia.user_interface.pipeline_manager.process_mia.ProcessMIA', But why not! Maybe we should put everything in mia_processes?

denisri commented 3 years ago

The way "packages" were walked through is different, and I admit I have not understood what it was doing. The thing is that it was parsing "packages" directories, but not regular modules (? or not all of them). Now every submodule is parsed, and classes are sometimes found at several levels of the module three. For instance module.__init__ may import some of its sub-modules classes, like in mia_processes/bricks/preprocess/spm/__init__.py:

from .spatial_preprocessing import (Coregister, GM_WM_Normalize, NewSegment,
                                    Normalize12, Realign, SliceTiming, Smooth)

Thus the Coregister class is found both as mia_processes.bricks.preprocess.spm.Coregister and as mia_processes.bricks.preprocess.spm.spatial_preprocessing.Coregister. So I had to do something to avoid duplicates. The parser was not duplicating things before, but it was also missing things... But if you have a way of making it work as we need, I agree...

Note: we also don't want to import all sub-modules classes in packages __init__ files, because we want to keep processes modular and avoid loading things that are not necessary for the execution of a single process.

servoz commented 3 years ago

I definitely don't have the time. Maybe I'll look at this later, I think I was the one who coded the previous version. But I don't remember how it worked at all ..... I'm not sure we're really effective if we change things without consulting each other (that's why I always do a PR on capsul or others)

denisri commented 3 years ago

The older code was:

                for _, modname, ispkg in pkgutil.iter_modules(pkg.__path__):

                    if ispkg:
                        print('\nExploring subpackages of {0} ...'
                              .format(module_name))
                        print('- ', str(module_name + '.' + modname))
                        self.add_package(str(module_name + '.' + modname),
                                         class_name)

Thus it was not parsing non-package modules (terminal, .py modules) because of the if ispkg clause and the unconditional use of pkg.__path__ (terminal modules don't have a __path__ attribute). I changed that, so now classes were found several times when they were imported in packages __init__.py from sub-modules.

I'm not sure we're really effective if we change things without consulting each other (that's why I always do a PR on capsul or others)

Yes you're totally right. I have bad habits because I'm working in like 50 projects and don't have time to review all PRs, and am used that PRs are never read and reviewed in other projects, thus most of them stay indefinietly and are never merged; but here in populse_mia I should make this effort...