Skip to content

FIX: Disallow returning None in pipeline.utils.load_resultfile #3023

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

Merged
merged 6 commits into from
Sep 11, 2019

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Sep 10, 2019

Summary

Purpose: make the handling of result files more robust.

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

Prevents #3009 and #3014 from happening - although this might not solve those issues,
this patch will help find their origin by making load_resultfile more strict (and
letting it raise exceptions).

The try .. except structure is moved to the only place is was being used within the Node code.

Acknowledgment

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

@codecov
Copy link

codecov bot commented Sep 10, 2019

Codecov Report

Merging #3023 into master will decrease coverage by 3.34%.
The diff coverage is 44.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3023      +/-   ##
==========================================
- Coverage   67.52%   64.18%   -3.35%     
==========================================
  Files         344      342       -2     
  Lines       44045    43988      -57     
  Branches     5552     5546       -6     
==========================================
- Hits        29740    28232    -1508     
- Misses      13566    14639    +1073     
- Partials      739     1117     +378
Flag Coverage Δ
#smoketests ?
#unittests 64.18% <44.11%> (-0.79%) ⬇️
Impacted Files Coverage Δ
nipype/pipeline/engine/utils.py 71.05% <100%> (-9.88%) ⬇️
nipype/pipeline/engine/nodes.py 77.1% <22.22%> (-7.01%) ⬇️
nipype/pipeline/plugins/tools.py 81.81% <54.54%> (-3.12%) ⬇️
nipype/interfaces/nilearn.py 40% <0%> (-56.67%) ⬇️
nipype/utils/spm_docs.py 25.92% <0%> (-44.45%) ⬇️
nipype/interfaces/freesurfer/base.py 50% <0%> (-30.51%) ⬇️
nipype/utils/logger.py 59.7% <0%> (-29.86%) ⬇️
nipype/algorithms/rapidart.py 35% <0%> (-29.42%) ⬇️
nipype/interfaces/spm/base.py 57.94% <0%> (-29.14%) ⬇️
nipype/utils/provenance.py 55.73% <0%> (-28.35%) ⬇️
... and 43 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 0470641...e8c408a. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 10, 2019

Codecov Report

Merging #3023 into master will decrease coverage by 3.33%.
The diff coverage is 44.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3023      +/-   ##
==========================================
- Coverage   67.51%   64.18%   -3.34%     
==========================================
  Files         344      342       -2     
  Lines       44045    43988      -57     
  Branches     5550     5546       -4     
==========================================
- Hits        29739    28232    -1507     
- Misses      13567    14639    +1072     
- Partials      739     1117     +378
Flag Coverage Δ
#smoketests ?
#unittests 64.18% <44.11%> (-0.79%) ⬇️
Impacted Files Coverage Δ
nipype/pipeline/engine/utils.py 71.05% <100%> (-9.88%) ⬇️
nipype/pipeline/engine/nodes.py 77.1% <22.22%> (-7.01%) ⬇️
nipype/pipeline/plugins/tools.py 81.81% <54.54%> (-3.12%) ⬇️
nipype/interfaces/nilearn.py 40% <0%> (-56.67%) ⬇️
nipype/utils/spm_docs.py 25.92% <0%> (-44.45%) ⬇️
nipype/interfaces/freesurfer/base.py 50% <0%> (-30.51%) ⬇️
nipype/utils/logger.py 59.7% <0%> (-29.86%) ⬇️
nipype/algorithms/rapidart.py 35% <0%> (-29.42%) ⬇️
nipype/interfaces/spm/base.py 57.94% <0%> (-29.14%) ⬇️
nipype/utils/provenance.py 55.73% <0%> (-28.35%) ⬇️
... and 42 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 2162c84...a200bc5. Read the comment docs.

@oesteban oesteban marked this pull request as ready for review September 10, 2019 16:49
@oesteban oesteban requested review from satra and effigies September 10, 2019 16:49
Copy link
Member

@satra satra left a comment

Choose a reason for hiding this comment

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

the rest of the changes look like reasonable replacements. but the change in report_crash i think requires checking what result is.

as part of the clean_node subfunction in multiproc, result could be set to None.

@oesteban
Copy link
Contributor Author

oesteban commented Sep 11, 2019

I caught one more bug (rebase False not being honored).

This PR should be ready to merge.

Please note that I'm marking one test condition as xfail with python 2 - not sure it is worth digging up the pickling of Files with Jan 2020 this close.

Prevents nipy#3009 and nipy#3014 from happening - although this might not solve those issues,
this patch will help find their origin by making ``load_resultfile`` more strict (and
letting it raise exceptions).

The try .. except structure is moved to the only place is was being used within the Node code.
@oesteban oesteban force-pushed the fix/revise-load_resultfile branch from 6a13f90 to a200bc5 Compare September 11, 2019 04:52
@oesteban oesteban merged commit 01de656 into nipy:master Sep 11, 2019
@oesteban oesteban deleted the fix/revise-load_resultfile branch September 11, 2019 14:06
@effigies effigies added this to the 1.2.3 milestone Sep 20, 2019
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.

3 participants