-
Notifications
You must be signed in to change notification settings - Fork 294
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: Config module to replace long list of downstreamed arguments #2018
Conversation
3e59c4b
to
fd8d3ea
Compare
af5bcfb
to
2c4de8b
Compare
@oesteban Here's the merge conflict between this and 20.0.2:
Would you prefer to resolve that in this PR, or after this is done? (Trying to decide whether to merge the latest tag into |
fmriprep/cli/parser.py
Outdated
) | ||
from packaging.version import Version | ||
from .version import check_latest, is_flagged | ||
from niworkflows.utils.spaces import Reference, SpatialReferences, OutputReferencesAction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue found: Unused variable 'SpatialReferences'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I assume if Codacy tagged this, you'll take care of it?
(BTW Codacy comments are completely spamming my feed. Is there any way to have it make just one review?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have the commit done, was just waiting for Circle to finish so that the build didn't get cancelled.
I have changed Codacy's settings so that only one summary comment will be posted.
citation_files['md'].write_text(boilerplate) | ||
|
||
if not config.execution.md_only_boilerplate and citation_files['md'].exists(): | ||
from subprocess import check_call, CalledProcessError, TimeoutExpired |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I've addressed both use cases and added a test that doesn't cover all possibilities but is pretty close. I've also tested this on ls5 on 15 subjects from the ABCD Study and worked like a charm. I'll hold off until I get more feedback (cc/ @mgxd, @effigies and perhaps also interesting to check for @emdupre, @jdkent, @mattcieslak). |
|
||
|
||
@contextmanager | ||
def mock_config(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't really seem like a mock, in that it's not resetting values when finished. Is the goal to eventually move to a mock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's going to be tricky, but that would be the right thing. For now, since this is only utilized in generating documentation, it seems to work well enough.
fmriprep/cli/parser.py
Outdated
) | ||
from packaging.version import Version | ||
from .version import check_latest, is_flagged | ||
from niworkflows.utils.spaces import Reference, SpatialReferences, OutputReferencesAction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I assume if Codacy tagged this, you'll take care of it?
(BTW Codacy comments are completely spamming my feed. Is there any way to have it make just one review?)
set_start_method('forkserver') | ||
except RuntimeError: | ||
pass # context has been already set | ||
finally: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this in finally
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretical reason: it is explicit those are to be run regardless the branch flow went through.
Practical reason: so that my linter does not complain about imports after plain code.
Happy to take out from the finally if that feels more appropriate.
Co-Authored-By: Chris Markiewicz <effigies@gmail.com>
…[skip ci] addresses nipreps#2018 (comment) also address nipreps#2018 (comment)
…config At some point of this PR (nipreps#2018), I was getting pklz crashfiles. Obviously, the config wasn't working properly. Once that was addressed via nipreps@8a0bf9d, this setting should go away for us to identify any regression of the problem.
I was thinking @josephmje may want to weigh in this one too. |
I think this is good to go. Happy to send a quick PR to address #2018 (comment) if it will make code clearer. |
Merge notes: * Fix gh-2014 appears to have been made unnecessary by gh-2018. Tag message: 20.0.2 (March 6, 2020) ====================== A bug squashing release in the 20.0.x series. This release fixes the use of custom templates within the docker wrapper, remedies crashes when FreeSurfer HOME was not set, and improves the documentation for local installations. With thanks to Blaise Frederick for the contribution. * DOC: Update standalone installation requirements (#2009) * FIX: Crashes whenever FREESURFER_HOME is not set (#2014) * FIX: Local template mounting (wrapper) (#2020) * MAINT: Pin minor series of nipype, major series of nibabel (#2021)
I was getting an error on this line when running this. It looks like |
Good catch. Care to submit a quick fix? |
Yay :) - please f-strings :) |
Includes addressing #2019, althoughI think that one could be addressed separately to simplify this already huge PR.Requires: nipreps/niworkflows#475
Summary
This module implements the memory structures to keep a consistent, singleton config.
Example
The module also has a :py:func:
to_filename
function to allow writting outthe settings to hard disk in ToML format, which looks like:
This config file is used to pass the settings across processes,
using the :py:func:
load
function.Other responsibilities of the config module:
Switching Python's
multiprocessing
to forkserver mode.Set up new logger levels (25: IMPORTANT, and 15: VERBOSE).
Set up a warnings filter as soon as possible.
Initialize runtime descriptive settings (e.g., default FreeSurfer license,
execution environment, nipype and fMRIPrep versions, etc.).
Automated I/O magic operations:
Path
<-> :obj:str
<-> :obj:Path
).~niworkflows.util.spaces.SpatialReferences
<-> :obj:str
<->:py:class:
~niworkflows.util.spaces.SpatialReferences
~bids.layout.BIDSLayout
<-> :obj:str
<->:py:class:
~bids.layout.BIDSLayout
Potential utilizations of the config module
environment
) of the config file, so that those are then reported in the log folder and within sentry.