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: Correctly pickle OuputMulti{Object,Path} traits #2983

Merged
merged 3 commits into from
Aug 2, 2019

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Aug 1, 2019

It seems that #2944 has uncovered a rats-nest hidden in the engine. In
resolving that issue, I found out that a great deal of boilerplate was
set in place when loading/saving results to deal with
OutputMulti{Object,Path} traits. The reason being that these traits
flatten single-element-list values.

This PR fixes the pickling behavior of traited specs containing these
types of traits.

Additionally, this PR also avoids the modify_paths function that was
causing problems originally in #2944. Therefore, this PR effectively
make results files static, meaning: caching if the base_dir of the
workflow is changed will not work anymore.

I plan to re-insert this feature (results file mobility) with #2971.
This PR is just to split that one in more digestible bits.

All the boilerplate mentioned above has been cleaned up.

Fixes #2968

Acknowledgment

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

It seems that nipy#2944 has uncovered a rats-nest hidden in the engine. In
resolving that issue, I found out that a great deal of boilerplate was
set in place when loading/saving results to deal with
``OutputMulti{Object,Path}`` traits. The reason being that these traits
flatten single-element-list values.

This PR fixes the pickling behavior of traited specs containing these
types of traits.

Additionally, this PR also avoids the ``modify_paths`` function that was
causing problems originally in nipy#2944. Therefore, this PR effectively
make results files static, meaning: caching if the ``base_dir`` of the
workflow is changed will not work anymore.

I plan to re-insert this feature (results file mobility) with nipy#2971.
This PR is just to split that one in more digestible bits.

All the boilerplate mentioned above has been cleaned up.
@effigies
Copy link
Member

effigies commented Aug 1, 2019

Love to see it
Screen Shot 2019-08-01 at 16 23 30

@oesteban
Copy link
Contributor Author

oesteban commented Aug 1, 2019

And most of the green are doctests 🥇

@oesteban
Copy link
Contributor Author

oesteban commented Aug 1, 2019

make tests works locally for python 2.7 and 3.7 - we'll see what happens with workflows on circle

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>
Co-Authored-By: Chris Markiewicz <effigies@gmail.com>
@oesteban
Copy link
Contributor Author

oesteban commented Aug 1, 2019

@satra this is something you may want to have a quick look before it gets in.

@mgxd, @djarecka your feedback would be very welcome too!

@oesteban oesteban requested a review from satra August 1, 2019 22:16
@oesteban
Copy link
Contributor Author

oesteban commented Aug 1, 2019

All green (c8a3797)

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.

LGTM

@effigies effigies merged commit 865adc9 into nipy:master Aug 2, 2019
@codecov
Copy link

codecov bot commented Aug 2, 2019

Codecov Report

Merging #2983 into master will decrease coverage by 3.38%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2983      +/-   ##
==========================================
- Coverage   67.65%   64.26%   -3.39%     
==========================================
  Files         344      342       -2     
  Lines       43835    43747      -88     
  Branches     5486     5475      -11     
==========================================
- Hits        29657    28115    -1542     
- Misses      13466    14537    +1071     
- Partials      712     1095     +383
Flag Coverage Δ
#smoketests ?
#unittests 64.26% <100%> (-0.82%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/base/specs.py 87.56% <100%> (-5.74%) ⬇️
nipype/pipeline/engine/utils.py 69.48% <100%> (-12.78%) ⬇️
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%) ⬇️
nipype/interfaces/fsl/model.py 55.26% <0%> (-25.35%) ⬇️
... and 44 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 7262b24...0cd60b6. Read the comment docs.

4 similar comments
@codecov
Copy link

codecov bot commented Aug 2, 2019

Codecov Report

Merging #2983 into master will decrease coverage by 3.38%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2983      +/-   ##
==========================================
- Coverage   67.65%   64.26%   -3.39%     
==========================================
  Files         344      342       -2     
  Lines       43835    43747      -88     
  Branches     5486     5475      -11     
==========================================
- Hits        29657    28115    -1542     
- Misses      13466    14537    +1071     
- Partials      712     1095     +383
Flag Coverage Δ
#smoketests ?
#unittests 64.26% <100%> (-0.82%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/base/specs.py 87.56% <100%> (-5.74%) ⬇️
nipype/pipeline/engine/utils.py 69.48% <100%> (-12.78%) ⬇️
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%) ⬇️
nipype/interfaces/fsl/model.py 55.26% <0%> (-25.35%) ⬇️
... and 44 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 7262b24...0cd60b6. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 2, 2019

Codecov Report

Merging #2983 into master will decrease coverage by 3.38%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2983      +/-   ##
==========================================
- Coverage   67.65%   64.26%   -3.39%     
==========================================
  Files         344      342       -2     
  Lines       43835    43747      -88     
  Branches     5486     5475      -11     
==========================================
- Hits        29657    28115    -1542     
- Misses      13466    14537    +1071     
- Partials      712     1095     +383
Flag Coverage Δ
#smoketests ?
#unittests 64.26% <100%> (-0.82%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/base/specs.py 87.56% <100%> (-5.74%) ⬇️
nipype/pipeline/engine/utils.py 69.48% <100%> (-12.78%) ⬇️
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%) ⬇️
nipype/interfaces/fsl/model.py 55.26% <0%> (-25.35%) ⬇️
... and 44 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 7262b24...0cd60b6. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 2, 2019

Codecov Report

Merging #2983 into master will decrease coverage by 3.38%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2983      +/-   ##
==========================================
- Coverage   67.65%   64.26%   -3.39%     
==========================================
  Files         344      342       -2     
  Lines       43835    43747      -88     
  Branches     5486     5475      -11     
==========================================
- Hits        29657    28115    -1542     
- Misses      13466    14537    +1071     
- Partials      712     1095     +383
Flag Coverage Δ
#smoketests ?
#unittests 64.26% <100%> (-0.82%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/base/specs.py 87.56% <100%> (-5.74%) ⬇️
nipype/pipeline/engine/utils.py 69.48% <100%> (-12.78%) ⬇️
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%) ⬇️
nipype/interfaces/fsl/model.py 55.26% <0%> (-25.35%) ⬇️
... and 44 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 7262b24...0cd60b6. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 2, 2019

Codecov Report

Merging #2983 into master will decrease coverage by 3.38%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2983      +/-   ##
==========================================
- Coverage   67.65%   64.26%   -3.39%     
==========================================
  Files         344      342       -2     
  Lines       43835    43747      -88     
  Branches     5486     5475      -11     
==========================================
- Hits        29657    28115    -1542     
- Misses      13466    14537    +1071     
- Partials      712     1095     +383
Flag Coverage Δ
#smoketests ?
#unittests 64.26% <100%> (-0.82%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/base/specs.py 87.56% <100%> (-5.74%) ⬇️
nipype/pipeline/engine/utils.py 69.48% <100%> (-12.78%) ⬇️
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%) ⬇️
nipype/interfaces/fsl/model.py 55.26% <0%> (-25.35%) ⬇️
... and 44 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 7262b24...0cd60b6. Read the comment docs.

@oesteban oesteban deleted the fix/pickling-outputmultiobjects branch August 2, 2019 18:53
@effigies effigies added this to the 1.2.1 milestone Aug 16, 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.

Obscure behavior of OutputMultiObject traits
3 participants