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

FIX: Improve error handling of CommandLine interfaces #3395

Merged
merged 3 commits into from
Oct 26, 2021

Conversation

oesteban
Copy link
Contributor

Currently, errors arising from interpolating the command line of these interfaces is handled poorly at the Node level.
If the command line cannot be built, the error is printed in the logfile but the exception is caught and never raised (i.e., likely leading to an infinite loop as the execution is not stopped).

I have experienced that while debugging fMRIPrep. To learn which of the inputs of a faulty interface derived from
CommandLine was not being formatted, I had to also add the error annotation proposed for the _parse_inputs inner loop.

Acknowledgment

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

Currently, errors arising from interpolating the command line of these
interfaces is handled poorly at the ``Node`` level.
If the command line cannot be built, the error is printed in the logfile
but the exception is caught and never raised (i.e., likely leading to an
infinite loop as the execution is not stopped).

I have experienced that while debugging fMRIPrep.
To learn which of the inputs of a faulty interface derived from
``CommandLine`` was not being formatted, I had to also add the error
annotation proposed for the ``_parse_inputs`` inner loop.
@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #3395 (5d9adbb) into master (eab4b2a) will increase coverage by 0.09%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3395      +/-   ##
==========================================
+ Coverage   65.20%   65.29%   +0.09%     
==========================================
  Files         307      307              
  Lines       40457    40926     +469     
  Branches     5350     5515     +165     
==========================================
+ Hits        26379    26724     +345     
- Misses      13003    13102      +99     
- Partials     1075     1100      +25     
Flag Coverage Δ
unittests 64.94% <91.66%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
nipype/interfaces/base/core.py 87.91% <88.88%> (-0.04%) ⬇️
nipype/pipeline/engine/nodes.py 78.93% <100.00%> (ø)
nipype/utils/subprocess.py 86.66% <100.00%> (+0.30%) ⬆️
nipype/interfaces/spm/preprocess.py 57.01% <0.00%> (+6.77%) ⬆️

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 eab4b2a...5d9adbb. Read the comment docs.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Fine to merge with or without my suggestion.

@effigies
Copy link
Member

Oh, looks like there's a style check failing...

oesteban and others added 2 commits October 26, 2021 11:16
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@effigies effigies merged commit 3c9c64f into nipy:master Oct 26, 2021
@oesteban oesteban deleted the fix/commandline-nodes-issue branch November 5, 2021 12:10
effigies added a commit to effigies/niworkflows that referenced this pull request Mar 9, 2022
effigies added a commit to effigies/niworkflows that referenced this pull request Mar 9, 2022
@effigies effigies added this to the 1.7.1 milestone Apr 3, 2022
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.

2 participants