[WIP] Major refactor (nipype-1.5 ?)

[oesteban]

Starting from #1240, the PR got too large so I think it is best to open a new fresh discussion, with an eye on #1301. This PR can be seen as a transitional work.

The original purpose of my refactor was to extend the name_source behaviour of the File traits in the CommandLine interfaces to any interface. Very soon I arrived to a quirky code with lots of branches for handling names. This was not very reliable and started to modify base things.

What is new in this refactor:

• Use of the Interfaces of Enthought traits, for both the interfaces definition (https://github.com/oesteban/nipype/blob/enh/nipype-1.5/nipype/interfaces/base/interfaces.py#L62) and inputs / outputs (https://github.com/oesteban/nipype/blob/enh/nipype-1.5/nipype/interfaces/base/specs.py#L35). In general, this does not affect the former way of working (breaks partially the generation of documentation, though), but gives reliability and moves the problems of type checking to traits (as they are intended for).
• BaseInterface now is a HasTraits object that implements (provides, in the traits jargon) the IBase interface. Now, inputs and outputs are traits.Instances of the corresponding input/output interfaces (https://github.com/oesteban/nipype/blob/enh/nipype-1.5/nipype/interfaces/base/specs.py#L35). This is good because we forget about type checking, and the access is pretty convenient. It also enables the exist check for files at the output, bringing earlier potential failures.
• environ, terminal_output, redirect_x, always_run, ignore_exceptions, etc. are now traits of the interfaces (not inputs of some of them or private properties). I have found this pretty convenient so far.
• File trait improved so that exists is also metadata, and now it can be queried.
• New traits GenFile and GenMultiFile, including a new mini-language to create replacements. Some very cool things can be done now, like generate a range of names of files with string formatting patterns (https://github.com/oesteban/nipype/blob/enh/nipype-1.5/nipype/interfaces/fsl/preprocess.py#L245) or generate one file name for each input file in an InputMultiPath trait (https://github.com/oesteban/nipype/blob/enh/nipype-1.5/nipype/interfaces/fsl/preprocess.py#L238), or substring patterns (https://github.com/oesteban/nipype/blob/enh/nipype-1.5/nipype/interfaces/fsl/utils.py#L826). Side note: maybe this should call these StrTemplate and List(StrTemplate) and then add the implementation for GenFile, since these generated strings do not need to be files (for instance, it could be possible to generate a title string including a file name for a report).
• New trait Command that encapsulates the code to check if the command is executable and saves the path in a metadata entry.
• Dry-runs: added a dry_run argument to run(), default False, that checks the expected outputs and makes the equivalent to bash touch on those files missing. This way, we can fake the running of interfaces, making debugging of workflows incredibly easier.
• parse_args (now public) and _format_args now are inside the CommandLineInputSpec since they only are affected and affect the inputs.

What I would propose to include:

• [easy] InterfaceResult and Runtime should also be traited objects (IMHO). Particularly, the runtime could be an Instance in the interface type. Allowing the use of self.runtime.value all along the interface. We would also replace the home-made implementation of Bunch.
• [medium] The comprehensive use of traits allows us to start using traits.Event and to code the logic of interfaces with observable events by the runners... This will ease the runner codes significantly and reduce the busy waits.
• [medium] I'm not against the Workflow class. Actually, we can make it IBase compliant. If Workflows fulfilled this interface, they would be interchangeable with unitary interfaces. The concepts would be now split as follows: running plugins only see Nodes or Tasks to execute. A Node has inside an IBase instance (the actual interface) that could also be a workflow. Some little logic can be added to compute the subgraph to split tasks or run the workflow in bulk. This will also make almost immediate to implement the iterfields of workflows.

Caveats and problems

• Even if this PR wouldn't get through, I think we need to rethink the way outputs are investigated. I would suggest to define a list of rules or similar (https://github.com/oesteban/nipype/blob/enh/nipype-1.5/nipype/interfaces/base/interfaces.py#L200). But I think we would need to discuss about this. This should be as automatic as possible, so we reduce to the minimum the code in the old _list_outputs (now should be implemented in a method called post_run, which is a more intuitive name of what you should expect to happen and when).
• Backwards compatibility: it is rather poor at this moment. Do we want it? Makes sense a transitional 1.5 towards a 2.0?

Feedback

Right now I'm a bit stuck with the automatic investigation of outputs. Also with the handling of these automatedly generated inputs, because there are many situations in which we may want to improve the code and avoid superfluous definitions of inputs (i.e. commands that accept a prefix for each output will need an extra input to append the extension, like in some afni interfaces). Many times you find boolean switches that operate on files flags, or even traits.Either(traits.Bool, File). All this logic can get complicated. I haven't tried to port ants to the new style... I bet it can be very much shortened with the new templates, but still a lot of extra definitions will be required.

I'm to give myself a couple of days off this PR, so that I can come back with fresh ideas and I give you some time to get through it. Please let me know any feedback you can provide @chrisfilo, @satra, @mwaskom .

[mwaskom]

Backwards compatibility: it is rather poor at this moment. Do we want it? Makes sense a transitional 1.5 towards a 2.0?

I don't fully understand what's going on here, but as someone with a lot of code that depends on nipype, this PR and this statement make me very nervous. It seems like a lot of stuff is changing (breaking?) without much rationale for why that is happening.

[oesteban]

That's why I called this transitional. With that question I just wanted to gauge how much effort we want to dedicate to backwards compatibility.

If that code you (or any nipype user) have consists of, basically, workflows, it will work with some changes for certain interfaces.

We can spend some time in writing the compatibility elements (mainly those changes in inputs/outputs of interfaces).

If you have a lot of custom Interfaces, then changes are a bit more delicate. We could provide a version with support for the legacy code for some time, though. I would just need to re-include an empty _list_outputs where needed, redirect the _format_args and _parse_inputs calls, etc.

Please recall that these changes are oriented to provide an easier access to new users (since it simplifies the inputs/outputs and automates a lot of stuff) and gives us a more reliable base towards #1301.

EDIT: Please recall that these changes are oriented to give us a more reliable base towards #1301 and additionally provide an easier access to new users (since it simplifies the inputs/outputs and automates a lot of stuff).

I agree with you in not to forget about our current users, but this PR is a look into the future. And also, it is far from ready to be merged, this is also important. I early posted it because I needed to ask you for help and to open the development of this branch to discussion.

In an effort to refocus the conversation on the current problems: what are your ideas regarding:

• an automated way of aggregating outputs?
• a neat way to specify inputs and outputs and their interaction avoiding very complex constructs like those in the current interfaces for ants?

[mwaskom]

Please recall that these changes are oriented to provide an easier access to new users

I find this a little confusing, since the rationale you seem to have provided appears focused entirely on the internals (i.e., not having to write a _list_outputs method for every interface). It's less clear to me how that helps users?

[oesteban]

Sorry for my misleading statement. I have edited it in my last post.

This would help new users since it will be a lot easier to create an interface in nipype for your custom software. Also, the experience when debugging workflows is a lot better using the dry_runs, for instance. There are more elements, but I think this all should go in the future documentation.

[chrisgorgo]

Issue found: Too many positional arguments for method call

[oesteban]

@satra, do you think we can schedule that meeting we were considering 2 months ago?

[satra]

@oesteban - let's do it - perhaps send out a doodle poll on the mailing list. i've the theoretical refactoring of nipype engine worked out and plan to start implementing it this week.

[oesteban]

BTW, I haven't tried it yet, but if we make traits for everything (interfaces and workflows) as I propose, I think that we could get the FASTR-like expressions very easily using the sync_trait feature (and overloading the gettatr and setattr of the traits).

If we finally don't get that far, using the sync_trait would be the most reliable way to create the actual connections between interfaces (and workflows if we make them traited), and a good coding practice.

None of these changes would necessarily affect the current ways of connecting interfaces and workflows.

If we wanted to move to traitlets, this is called linking (https://ipython.org/ipython-doc/3/api/generated/IPython.utils.traitlets.html#IPython.utils.traitlets.link). This is actually pretty the same thing we are doing in the deepest core of the connect function.

But honestly, I think the enthought traits are a bit more powerful.

[satra]

@oesteban - we need to follow up on this after the current release. i like many of the ideas here. let's make a concrete todo list after a discussion.