Skip to content

[ENH/WIP] WorkflowInterface #1835

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

Closed
wants to merge 17 commits into from

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Feb 22, 2017

This PR creates a new type of interface that wraps workflows into a nipype interface. A naive addendum has been done to the BaseWorkflow class, in order to post-process the executed graph to retrieve the outputs that were connected to an "outputnode". This addition only happens for synchronous runners. For now, only the Linear and MultiProc plugins are set as synchronous.

The inner workflow is then run using either Linear or Multiproc.

The doctest reads like:

    >>> from nipype.pipeline import engine as pe
    >>> from nipype.interfaces.utility.base import IdentityInterface
    >>> wf = pe.Workflow('test_workflow')
    >>> node1 = pe.Node(IdentityInterface(fields=['a', 'b']), name='inputnode')
    >>> func = 'def func(a, b): return a + b'
    >>> node2 = pe.Node(Function(input_names=['a', 'b'], output_names=['out'],
    ...                 function=func), name='strconcat')
    >>> node3 = pe.Node(IdentityInterface(fields=['out']), name='outputnode')
    >>> wf.connect([
    ...     (node1, node2, [('a', 'a'), ('b', 'b')]),
    ...     (node2, node3, [('out', 'out')])
    ... ])
    >>> wi = WorkflowInterface(workflow=wf)
    >>> wi.inputs.a = 'b input '
    >>> wi.inputs.b = 'was concatenated to a'
    >>> res = wi.run()
    >>> res.outputs.out
    'b input was concatenated to a'

Before this PR can be merged, these tasks should be ticked :

  • Merge [ENH] Refactoring of nipype.interfaces.utility #1828 (this refactoring was a preliminary work to this PR)
  • Write some additional documentation (besides the Function interface)
  • Pass a config object to the inner workflow (probably overwriting the stop_at_first_crash option to True, maybe @satra can think of other options that should be set and potential problems of this approach)
  • Add more tests:
    • workflow that fails for some traits setting errors
    • workflow that fails for some runtime error (maybe a Function interface that just raises)
    • embed the new interface in a mapnode
    • run the interface with iterables

@satra
Copy link
Member

satra commented Feb 22, 2017

@oesteban - the new api we are working on basically has the following subclassing structure Node -> Workflow, so by definition a Workflow can be a Node. we have a discussion with @effigies and @djarecka scheduled for tomorrow and hoping we have a draft to present after that.

we are considering input_map and output_map as parameters to Workflow that can basically route inputs and outputs, without creating explicit inputspec and outputspec identity nodes. by default input_map and output_map would be a flattened key_value object for all nodes of the workflow, but a user could constrain that space by explicitly specifying these mappings, or sub-selecting from them.

however, this would be a good interface to implement that functionality.

@codecov-io
Copy link

codecov-io commented Feb 23, 2017

Codecov Report

Merging #1835 into master will increase coverage by 0.81%.
The diff coverage is 96.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1835      +/-   ##
==========================================
+ Coverage   72.31%   73.13%   +0.81%     
==========================================
  Files        1180     1064     -116     
  Lines       58885    53546    -5339     
  Branches     8474        0    -8474     
==========================================
- Hits        42585    39160    -3425     
+ Misses      14921    14386     -535     
+ Partials     1379        0    -1379
Flag Coverage Δ
#smoketests ?
#unittests 73.13% <96.84%> (+3.23%) ⬆️
Impacted Files Coverage Δ
nipype/pipeline/plugins/linear.py 92.1% <100%> (+3.53%) ⬆️
nipype/pipeline/plugins/base.py 65.76% <100%> (+7.24%) ⬆️
nipype/pipeline/plugins/multiproc.py 75.55% <100%> (-1.21%) ⬇️
nipype/interfaces/utility/__init__.py 100% <100%> (ø) ⬆️
nipype/pipeline/engine/workflows.py 79.47% <100%> (+0.68%) ⬆️
nipype/interfaces/utility/wrappers.py 91.83% <93.87%> (+6.65%) ⬆️
nipype/interfaces/utility/tests/test_wrappers.py 93.58% <97.36%> (+6.55%) ⬆️
nipype/interfaces/spm/model.py 41.76% <0%> (-29.35%) ⬇️
nipype/algorithms/rapidart.py 37.46% <0%> (-27.14%) ⬇️
nipype/interfaces/fsl/model.py 54.81% <0%> (-25.55%) ⬇️
... and 967 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56b7c81...41388eb. Read the comment docs.

@effigies
Copy link
Member

@oesteban is this one orphaned?

@satra
Copy link
Member

satra commented Jan 21, 2018

@effigies and @oesteban - i wouldn't mind seeing this in the 1.0 branch, but this would be explicitly supported in 2.0 engine.

@oesteban
Copy link
Contributor Author

I can recheck in two weeks. If re-syncing is not too hard (better said: very easy), we can add it as a patch for 1.0. As Satra mentioned, this does not make sense in 2.0.

@effigies effigies closed this Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants