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

ENH: Allow Nipype configuration directory to be specified with NIPYPE_CONFIG_DIR environment variable #3073

Merged
merged 5 commits into from
Dec 18, 2019

Conversation

stilley2
Copy link
Contributor

@stilley2 stilley2 commented Oct 8, 2019

Summary

Allow overwriting default location of nipype.cfg (~/.nipype) with the environment variable NIPYPECONFDIR. This is potentially useful for containerized pipelines which require a default nipype.cfg.

Acknowledgment

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

@codecov
Copy link

codecov bot commented Oct 8, 2019

Codecov Report

Merging #3073 into master will increase coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3073      +/-   ##
==========================================
+ Coverage   67.86%   68.05%   +0.18%     
==========================================
  Files         295      295              
  Lines       39260    39747     +487     
  Branches     5171     5293     +122     
==========================================
+ Hits        26644    27050     +406     
- Misses      11911    11979      +68     
- Partials      705      718      +13
Flag Coverage Δ
#smoketests 49.55% <100%> (-1.68%) ⬇️
#unittests 67.75% <100%> (+2.66%) ⬆️
Impacted Files Coverage Δ
nipype/utils/config.py 65.19% <100%> (ø) ⬆️
nipype/pipeline/plugins/legacymultiproc.py 63.31% <0%> (+0.5%) ⬆️
nipype/interfaces/fsl/preprocess.py 83.88% <0%> (+2.16%) ⬆️
nipype/interfaces/spm/model.py 47.11% <0%> (+4.84%) ⬆️

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 1a86999...2026787. Read the comment docs.

Copy link
Contributor

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

Could we test this updating the circleci jobs?

@stilley2
Copy link
Contributor Author

stilley2 commented Oct 9, 2019

Sorry @oesteban , I'm not sure exactly what you mean. Do you want me to add some tests for this?

@effigies
Copy link
Member

effigies commented Oct 9, 2019

I'm not sure what a good test would be. I guess you could mock the environment and validate that the expected contents are loaded when the variable is set or not set.

In any event, you should add a note about the environment variable to, once this is merged: https://github.com/miykael/nipype_tutorial/blob/master/notebooks/basic_execution_configuration.ipynb

@effigies
Copy link
Member

Hi @stilley2, this PR is also affected by our recent merges. Could you re-apply these changes on the latest master? You'll want to re-run make specs entirely, and we're now using the Black formatter. You can run it manually, or use pre-commit to have it automatically run every time you commit.

Please let me know if you need any help or clarifications.

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.

There are two cases of NIPYPE* environment variables:

if plugin == "MultiProc":
plugin_args["n_procs"] = int(os.getenv("NIPYPE_NUMBER_OF_CPUS", cpu_count()))

if "NIPYPE_NO_MATLAB" in os.environ or Info.version() is None:
return True

Both have the form with underscores separating words, so let's do something in that direction.

Thanks for your patience, btw.

nipype/utils/config.py Outdated Show resolved Hide resolved
@effigies effigies added this to the 1.4.0 milestone Dec 14, 2019
@effigies
Copy link
Member

Hi @stilley2, do you have a couple minutes to consider the above proposal?

Co-Authored-By: Chris Markiewicz <[email protected]>
@stilley2
Copy link
Contributor Author

Sorry last week was pretty busy. Change looks good!

@effigies effigies changed the title Allow nipype configuration directory to be specified with NIPYPECONFDIR ENH: Allow Nipype configuration directory to be specified with NIPYPE_CONFIG_DIR Dec 16, 2019
@effigies effigies changed the title ENH: Allow Nipype configuration directory to be specified with NIPYPE_CONFIG_DIR ENH: Allow Nipype configuration directory to be specified with NIPYPE_CONFIG_DIR environment variable Dec 16, 2019
@effigies
Copy link
Member

Thanks for your patience.

@effigies effigies merged commit b5cb2aa into nipy:master Dec 18, 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