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 version checking for nodes of workflows #3152

Merged
merged 9 commits into from
Jan 16, 2020

Conversation

satra
Copy link
Member

@satra satra commented Jan 11, 2020

Reduce number of version checks for nodes of workflows run through multiprocessing .

This is a rather drastic step, and we should think through how to handle this in general. this will prevent this for other libraries which are used as well.

@effigies
Copy link
Member

What if we add a new environment variable like NIPYPE_SKIP_ET?

@codecov
Copy link

codecov bot commented Jan 14, 2020

Codecov Report

Merging #3152 into maint/1.4.x will increase coverage by 0.05%.
The diff coverage is 68.42%.

Impacted file tree graph

@@              Coverage Diff               @@
##           maint/1.4.x   #3152      +/-   ##
==============================================
+ Coverage        67.54%   67.6%   +0.05%     
==============================================
  Files              299     299              
  Lines            39499   39496       -3     
  Branches          5220    5220              
==============================================
+ Hits             26681   26701      +20     
+ Misses           12108   12087      -21     
+ Partials           710     708       -2
Flag Coverage Δ
#smoketests 53.02% <45.45%> (-0.02%) ⬇️
#unittests 64.85% <55.26%> (+0.05%) ⬆️
Impacted Files Coverage Δ
nipype/pipeline/plugins/tools.py 78.94% <ø> (ø) ⬆️
nipype/utils/__init__.py 100% <ø> (ø) ⬆️
nipype/utils/config.py 65% <ø> (-0.2%) ⬇️
nipype/algorithms/icc.py 57.53% <ø> (-0.58%) ⬇️
nipype/interfaces/dipy/preprocess.py 25.42% <0%> (-0.42%) ⬇️
nipype/interfaces/mrtrix/convert.py 17.18% <0%> (+0.17%) ⬆️
nipype/interfaces/dcmstack.py 35.1% <0%> (-0.27%) ⬇️
nipype/interfaces/fsl/epi.py 62.08% <0%> (-0.08%) ⬇️
nipype/algorithms/modelgen.py 67.6% <100%> (-0.07%) ⬇️
nipype/interfaces/spm/base.py 87% <100%> (ø) ⬆️
... and 10 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 39012c5...e2db192. 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.

I think the logic is backwards in a couple places, and I'm a bit confused by the pyscript stuff.

nipype/__init__.py Outdated Show resolved Hide resolved
nipype/interfaces/base/core.py Outdated Show resolved Hide resolved
nipype/pipeline/plugins/multiproc.py Outdated Show resolved Hide resolved
nipype/pipeline/plugins/tools.py Outdated Show resolved Hide resolved
@effigies
Copy link
Member

This seems to work. See satra#31 for a test and a couple small suggestions.

Also, we currently support two other environment variables: NIPYPE_NO_MATLAB and NIPYPE_CONFIG_DIR. My inclination would be to stick with the NIPYPE_* convention. Unless you have a strong preference for NO_NIPYPE_ET?

@effigies effigies changed the base branch from master to maint/1.4.x January 15, 2020 14:27
@effigies
Copy link
Member

effigies commented Jan 15, 2020

Looking at the test failure, I'm unable to replicate it easily. It appears to be related to matplotlib/matplotlib#15832 and https://discourse.matplotlib.org/t/issues-with-my-installation-of-matplotlib/20746.

I can fiddle more later, but I wouldn't hold up merge for it.

Only remaining comment is that, as a bug fix, we should target maint/1.4.x and rebase on it.

Edit: Failing test fixed in maint/1.4.x. Rebasing should resolve it.

@effigies
Copy link
Member

@satra Rebased cleanly, so I just force-pushed to keep this moving. Hope that's okay.

@effigies effigies merged commit e5664a2 into nipy:maint/1.4.x Jan 16, 2020
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