Skip to content

MAINT: Simplify interface execution and better error handling of Node - #3349

Merged
effigies merged 10 commits into
nipy:masterfrom
oesteban:enh/robuster-node
Sep 29, 2021
Merged

MAINT: Simplify interface execution and better error handling of Node#3349
effigies merged 10 commits into
nipy:masterfrom
oesteban:enh/robuster-node

Conversation

@oesteban

Copy link
Copy Markdown
Contributor

Revises the implementation of Node simplifying some details and improves the error handling of plugins. Introduces a NodeExecutionError (custom alias of RuntimeError) that allows more specificity with errors, if needed.

Requires: #3347.

Summary

Fixes # .

List of changes proposed in this PR (pull-request)

Acknowledgment

  • (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.
@codecov

codecov Bot commented Jun 24, 2021

Copy link
Copy Markdown

Codecov Report

Merging #3349 (c0f49c7) into master (bb125ab) will increase coverage by 0.89%.
The diff coverage is 81.39%.

❗ Current head c0f49c7 differs from pull request most recent head bbcd4ff. Consider uploading reports for the commit bbcd4ff to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3349      +/-   ##
==========================================
+ Coverage   64.23%   65.13%   +0.89%     
==========================================
  Files         307      307              
  Lines       40373    40364       -9     
  Branches     5326     5333       +7     
==========================================
+ Hits        25935    26290     +355     
+ Misses      13385    13005     -380     
- Partials     1053     1069      +16     
Flag Coverage Δ
unittests 64.85% <81.39%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
nipype/pipeline/plugins/tools.py 76.00% <ø> (-0.32%) ⬇️
nipype/pipeline/plugins/base.py 58.15% <50.00%> (-0.02%) ⬇️
nipype/interfaces/matlab.py 78.02% <66.66%> (+4.02%) ⬆️
nipype/pipeline/engine/nodes.py 78.93% <84.21%> (+1.56%) ⬆️
nipype/interfaces/base/core.py 87.95% <100.00%> (-0.23%) ⬇️
nipype/interfaces/base/support.py 82.19% <100.00%> (+1.26%) ⬆️
nipype/pipeline/plugins/linear.py 90.69% <100.00%> (+12.12%) ⬆️
nipype/interfaces/ants/registration.py 73.09% <0.00%> (ø)
nipype/interfaces/spm/preprocess.py 50.44% <0.00%> (+0.88%) ⬆️
... and 19 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 bb125ab...bbcd4ff. Read the comment docs.

@effigies

Copy link
Copy Markdown
Member

@oesteban Rebase to clean up the diff?

@effigies
effigies force-pushed the enh/robuster-node branch from 104c3cc to 5210a3b Compare July 29, 2021 17:24
@effigies

Copy link
Copy Markdown
Member

@oesteban Rebased on your behalf.

@effigies effigies left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think it looks good. A couple initial thoughts, and please bug me to do another review.

Comment thread nipype/interfaces/base/support.py Outdated
Comment thread nipype/pipeline/engine/nodes.py
Comment thread nipype/pipeline/engine/nodes.py Outdated
Comment thread nipype/pipeline/plugins/base.py Outdated
Comment thread nipype/pipeline/plugins/linear.py Outdated
Comment thread nipype/pipeline/plugins/tests/test_sgelike.py Outdated
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Comment thread nipype/pipeline/plugins/linear.py Outdated

@effigies effigies left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Suggested updating the linear to have the same exception behavior as distributed, but don't feel very strongly.

oesteban and others added 2 commits September 29, 2021 10:08
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@effigies

Copy link
Copy Markdown
Member

Failing test fixed in #3378. Merging.

@effigies
effigies merged commit e3a1192 into nipy:master Sep 29, 2021
@oesteban
oesteban deleted the enh/robuster-node branch September 29, 2021 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants