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

Nipype1 interface wrapper and simple workflow #171

Open
effigies opened this issue Jan 10, 2020 · 17 comments
Open

Nipype1 interface wrapper and simple workflow #171

effigies opened this issue Jan 10, 2020 · 17 comments
Assignees
Labels
feature Feature requests

Comments

@effigies
Copy link
Contributor

Paths may be an issue...

@effigies effigies added the feature Feature requests label Jan 10, 2020
@effigies effigies self-assigned this Jan 10, 2020
@effigies
Copy link
Contributor Author

effigies commented Jan 14, 2020

Some initial fiddling:

import pydra
import nipype
import dataclasses as dc
import typing as ty
from nipype.interfaces import fsl

def traitedspec_to_specinfo(traitedspec):
    return pydra.specs.SpecInfo(
        name="Inputs",
        fields=[
            (name, ty.Any, dc.field(metadata={"help_string": trait.desc}))
            for name, trait in traitedspec.traits().items()
        ]
    )

I don't really see how to create a task except by subclassing TaskBase or @pydra.mark.task, so I'm going with subclassing.

class Nipype1Task(pydra.TaskBase):
    """Wrap a Python callable as a task element."""

    def __init__(
        self,
        interface: nipype.interfaces.base.BaseInterface,
        audit_flags: pydra.AuditFlag = pydra.AuditFlag.NONE,
        cache_dir=None,
        cache_locations=None,
        messenger_args=None,
        messengers=None,
        name=None,
        **kwargs,
    ):
        self.input_spec = traitedspec_to_specinfo(interface.input_spec())
        self._interface = interface
        if name is None:
            name = interface.__class__.__name__
        super(Nipype1Task, self).__init__(
            name,
            inputs=kwargs,
            audit_flags=audit_flags,
            messengers=messengers,
            messenger_args=messenger_args,
            cache_dir=cache_dir,
            cache_locations=cache_locations,
        )
        self.output_spec = traitedspec_to_specinfo(interface.output_spec())

    def _run_task(self):
        inputs = dc.asdict(self.inputs)
        res = self._interface.run(**inputs)
        self.output = res.outputs

This isn't quite ideal, because it means that Nipype1Task(fsl.BET()) is a one-use thing, when the goal would be something like

newBET = Nipype1Task(fsl.BET)
bet = newBET(...)

Anyway, I'm getting stuck on actually running the thing:

wf = pydra.Workflow(name='unifize_and_skullstrip_wf', input_spec=['in_file'])
bet = Nipype1Task(fsl.BET())
bet.in_file = wf.lzin.in_file
wf.add(bet)

with pydra.Submitter() as sub:
    sub(wf)

The results in:

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-22-a8fc839fa509> in <module>
      1 with pydra.Submitter() as sub:
----> 2     sub(wf)

~/Projects/nipype2/pydra/pydra/engine/submitter.py in __call__(self, runnable, cache_locations)
     48                 if nd.allow_cache_override:
     49                     nd.cache_dir = runnable.cache_dir
---> 50             runnable.inputs._graph_checksums = [
     51                 nd.checksum for nd in runnable.graph_sorted
     52             ]

~/Projects/nipype2/pydra/pydra/engine/submitter.py in <listcomp>(.0)
     49                     nd.cache_dir = runnable.cache_dir
     50             runnable.inputs._graph_checksums = [
---> 51                 nd.checksum for nd in runnable.graph_sorted
     52             ]
     53         if is_workflow(runnable) and runnable.state is None:

~/Projects/nipype2/pydra/pydra/engine/core.py in __getattr__(self, name)
    187         if name == "lzout":  # lazy output
    188             return LazyField(self, "output")
--> 189         return self.__getattribute__(name)
    190 
    191     def help(self, returnhelp=False):

~/Projects/nipype2/pydra/pydra/engine/core.py in checksum(self)
    207             self.inputs._graph_checksums = [nd.checksum for nd in self.graph_sorted]
    208 
--> 209         input_hash = self.inputs.hash
    210         if self.state is None:
    211             self._checksum = create_checksum(self.__class__.__name__, input_hash)

AttributeError: 'Inputs' object has no attribute 'hash'

@satra
Copy link
Contributor

satra commented Jan 14, 2020

@effigies - before we go down this rabbit hole, we should try to determine if we are going to move forward with attr #174 or not. i am hoping @djarecka can fix the issues that are ailing the 3 failing tests.

@effigies
Copy link
Contributor Author

When people get a minute, maybe we can discuss the status of this, post attr.ib migration?

@satra
Copy link
Contributor

satra commented Jun 16, 2020

    my_input_spec = SpecInfo(
        name="Input",
        fields=[
            (
                "text",
                attr.ib(
                    type=str,
                    metadata={"position": 1, "help_string": "text", "mandatory": True},
                ),
            )
        ],
    )

@dafrose
Copy link
Contributor

dafrose commented Oct 23, 2020

How is the current progress on this issue? I suggested the introduction of a yaml-interface to create tasks for command line tools (primarily, not exclusively) here #367 . A possible route to go from nipype1 to pydra could be this:

  1. Translate nipype1 TraitedSpecs to yaml files.
  2. Parse yaml files to create tasks in pydra.

Advantages:

  • Enables users to also translate their own custom-written nipype1 interfaces automatically.
  • Resulting yaml files can be kept for the future, reducing processing time when reusing the interface.
  • Resulting collection of yaml files can be easily packaged as pydra addon-packages, solving pydra interface packages from nipype 1 #170 and making it easy to completely detach from original nipype1 code.
  • The compatibility of the resulting yaml files with pydra can be tested at both ends: when serializing into yaml and when parsing into tasks, making it easy to see, where something went wrong.

Disadvantages:

  • Conversion creates new files, so file saving permission is needed and some path must be specified. However, this could be mitigated by allowing a user to directly feed the yaml stream to the parser in pydra instead of saving the files.

Since workflows are tasks in pydra, it would make sense to do the same for workflows. Regarding tests, one could rewrite original nipype1 tests to use yaml-based pydra tasks instead.

What do you think?

@dafrose
Copy link
Contributor

dafrose commented Oct 23, 2020

I just noticed that you already implemented a nipype1 task wrapper with the PR nipype/pydra-nipype1#1
Has this been tested?

@effigies
Copy link
Contributor Author

@dafrose Yes, a method to translate the nipype1 packages to YAML files that could be loaded as Pydra tasks would be fantastic. The Nipype1Task does work, though I'm not sure where I have an example workflow (this should be added in the CI), but the disadvantage is that the build-time type checking is completely missing, and all trait checks will be performed at runtime.

Converting the input and output specs automatically via a YAML representation seems useful, as it would give us a declarative, Python-independent representation. The only problem I see is with interfaces that have more complicated internal workings that cannot be determined only from input and output specs.

@dafrose
Copy link
Contributor

dafrose commented Oct 23, 2020

@effigies happy to hear that you like the idea :-)

The only problem I see is with interfaces that have more complicated internal workings that cannot be determined only from input and output specs.

Do you have something in particular in mind? Anything that strictly requires user input via the command line during runtime does not work with nipype1 anyway, does it? Regarding more complex relations between inputs and output filenames, it would always be possible to reference any Python function from the YAML spec. Anything beyond that?

@effigies
Copy link
Contributor Author

Here's an example of something that would be painful: https://github.com/nipy/nipype/blob/07af08f98/nipype/interfaces/freesurfer/preprocess.py#L1010-L1569

ANTs tools are often other examples. They may be doable within the YAML, but I think the YAML would need to be designed, not automatically generated from what currently exists.

@djarecka
Copy link
Collaborator

i just want to point you to my branch where I'm working on fsl package. My way of doing is to have a converter that needs nipype1 interface and a hand-written yml file for things that I can't automatically take from nipype.
The converted interfaces are here. I still have to add some additional fields to my yml file that I don't now how to cover automatically.

@effigies @dafrose - perhaps we could zoomeet one day, what do you think?

@effigies
Copy link
Contributor Author

Can generally call in the 9-11am (Eastern US) range. In the next two weeks, any days but Friday Oct 30 and Tuesday Nov 3 work.

If you need more precision, can you send a poll?

@djarecka
Copy link
Collaborator

ok, i've created the poll for times suggested by @effigies https://doodle.com/poll/8fei4vdawvyg66me

we can think who should we ping.
(@satra - i've also added Thursday 9am, in case this is the only time works for you we can always use this slot)

@dafrose
Copy link
Contributor

dafrose commented Oct 26, 2020

I filled in my availabilities. Sorry for the late reply. With my current situation being quite dynamic, it is a bit difficult to judge in advance, but I'll try to make it work, if you decide on a time.

@effigies

Here's an example of something that would be painful: https://github.com/nipy/nipype/blob/07af08f98/nipype/interfaces/freesurfer/preprocess.py#L1010-L1569

I had a brief look at the code and can see why this would be problematic. With pydra's workflows being subclasses of tasks: Would it work to split up something as complex as recon-all into multiple tasks and resolve all the if's and elif's within the scope of a workflow?

In principle, you could represent conditions in a YAML-based task template, but I do not see how that would be better than doing it directly in Python. Maybe for some nipype1 interfaces it makes more sense to handcraft the pydra version...


Considering, that an automatic YAML-translator might not be the best/only reasonable option, here an idea how to spice up your existing implementation that you presented above:

Instead of making the nipype1 wrapper a class that instantiates a BaseTask, you could make it a function that takes your nipype1 interface and returns a new class via type(<interface-name>, pydra.BaseTask, <__dict__>) as explained here. This could look somewhat like this:

def nipype1_wrapper(interface, name, base = pydra.BaseTask, **kwargs):
    attr_dict = magic_nipype1_parser(interface)
    attr_dict.update(kwargs)
    return type(name, base, attr_dict)

This can of course be mixed with the YAML parser, because as soon as you parse the nipype1 interface into a dict, you can probably also serialize it into valid (machine-readable) YAML. It would not look nice, but probably work.

@djarecka
Copy link
Collaborator

@dafrose @effigies - thanks for answering to the poll - let's meet this Thursday at 9am EDT / 2pm CET
we can use this zoom meeting: https://mit.zoom.us/j/91819319889 (cc: @satra @nicolocin - this is a different zoom number)

Feel free to invite others

@dafrose
Copy link
Contributor

dafrose commented Oct 27, 2020

I just want to stick this here, so it won't be forgotten in the call:

I stumbled upon issue #42 which resulted in a prototype for a nipype1topydratask function by @dPys. The function seems to solve the same problem as the code by @effigies, but differently. Instead of taking the original interface class, the function takes an instance of a nipype1 interface to create an instance of its counterpart in pydra.

@djarecka djarecka mentioned this issue Oct 29, 2020
2 tasks
@dPys
Copy link

dPys commented Oct 29, 2020

Yep, will definitely try to join in! Thanks for bumping this issue @dafrose

@djarecka
Copy link
Collaborator

this is the google doc from the meeting: https://docs.google.com/document/d/12_oJKLx58c2iF_oRgPopETIX7G9qL9IH4cAa8dky8CU/edit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature requests
Projects
None yet
Development

No branches or pull requests

5 participants