Skip to content

[WIP,ENH] Conditional Nodes and Workflows #1299

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 62 commits into from

Conversation

oesteban
Copy link
Contributor

This is a first take on conditional execution of nodes in workflows. Runtime decisions in nipype are old feature requests (i.e. #819 and #878 for nodes, and #1081 for workflows).

Edit: this PR is documented here: https://github.com/oesteban/nipype/blob/enh/ControlNodes/doc/users/runtime_decisions.rst

The new ConditionalNode adds an special input to the inner interface named donotrun that can be used to disable the node. It is the user's responsibility to maintain the consistency along the whole graph.

The new ConditionalWorkflow adds an special input node: when setting checknode.donotrun to True, the workflow is disabled. These workflows automatically cast all its nodes to ConditionalNodes and connected to the checknode.donotrun input.

Finally, a new CachedWorkflow is derived from ConditionalWorkflow, with the addition of a cache_map (list of tuples indicating the input name and the output to which it is mapped). If all the inputs of a cachenode (as listed in the first element of the cache_map list of tuples) are defined, the donotrun of the workflow is activated and all the nodes skipped. Additionally, the workflows outputs are mapped from the cachenode. Otherwise, they are filled with the normal execution of the workflow.

Example:

[example moved to documentation]

This will return outputnode.out1 = 5 and outputnode.out2 = 5
If the line # cwf.inputs.cachenode.c = 1 is uncommented, then outputnode.out1 = 1 and outputnode.out2 = 7 since all the cache inputs are set and mapped.

This PR is labeled as WIP because there are some missing elements:

  • Include this example in the documentation, improve documentation
  • Analyze side-effects of skipping nodes, currently this is just a hard decision. Presumably, something should be cached when donotrun is True.
  • Convert sub-workflows of ConditionalWorkflow and CachedWorkflow in ConditionalWorkflows and connect the donotrun input.

This PR requires #1298. [Already merged]

@satra
Copy link
Member

satra commented Dec 15, 2015

this is great. one thing is that i have been contemplating rewriting the workflow engine code to integrate node and mapnode into the same construct. i was hoping to work on this towards the end of this month. should i outline the changes in a separate issue? i haven't completely thought through the changes, but i was hoping that it would at least implement all these patterns: http://www.workflowpatterns.com/ and be scalable and expandable completely at runtime. it would also allow additional execution semantics to run subgraphs or chains of nodes using node/storage/memory isolation.

@oesteban
Copy link
Contributor Author

Hi @satra I really agree with you on the convenience of attaching to those patterns you propose (great resource!, btw).

In this case, this could be a kind of pattern of data precondition adding signaling inputs to the nodes (the ConditionalNode special input donotrun, in this case). I don't think my approach implements the pattern very well. An alternate approach I've tried is graph editing (see https://github.com/oesteban/nipype/tree/enh/ConditionalWorkflow) but I found that route a bit unreliable.

I think the redesign you propose deserves a separate issue, identifying which patterns are currently necessary (to start implementation from them) and then organize the remaining patterns in order of priority. What do you think?

@satra
Copy link
Member

satra commented Dec 15, 2015

@oesteban - thanks - perhaps we can have a google hangout next week or the week after to go over this. that will give me a little bit of time to even attempt a basic refactor.

@oesteban
Copy link
Contributor Author

Hi there, I think this is ready to be reviewed. So yes, @blakedewey, I'd really appreciate some help with testing :)

@satra
Copy link
Member

satra commented Dec 23, 2015

@oesteban - thanks!

i'll start taking a look in a day. is that ok?

@chrisgorgo
Copy link
Member

@sgiavasis and @carlohamalainen could you also have a look - this might be useful/relevant to C-PAC and QAP.

@sgiavasis
Copy link
Contributor

@chrisfilo Interesting..! I will also take a look.

@oesteban
Copy link
Contributor Author

Basically, this PR consists on several patches. I think we should definitely go for a refactoring sooner or later. But for now, this can be useful.

@effigies
Copy link
Member

I'm a little inclined to call this an orphan. Is there any interest in reviving this or splitting it into PRs that provide partial functionality, or is this wholly superceded by the 2.0 refactor?

@oesteban
Copy link
Contributor Author

Yeah, I think this is orphaned. We can cherry pick from it if we wanted something in either 1.0 or 2.0.

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

Successfully merging this pull request may close these issues.

6 participants