Merge pull request #105 from oesteban/enh/refactor-base-dwi-workflow

MAINT: Refactor the workflow to use Nipype iterables

Author: oesteban

dmriprep/cli/parser.py

@@ -18,30 +18,33 @@ def main():
     if not config.execution.notrack:
         import popylar
         from ..__about__ import __ga_id__
+
         config.loggers.cli.info(
             "Your usage of dmriprep is being recorded using popylar (https://popylar.github.io/). ",  # noqa
             "For details, see https://nipreps.github.io/dmriprep/usage.html. ",
-            "To opt out, call dmriprep with a `--notrack` flag")
-        popylar.track_event(__ga_id__, 'run', 'cli_run')
+            "To opt out, call dmriprep with a `--notrack` flag",
+        )
+        popylar.track_event(__ga_id__, "run", "cli_run")
 
     # CRITICAL Save the config to a file. This is necessary because the execution graph
     # is built as a separate process to keep the memory footprint low. The most
     # straightforward way to communicate with the child process is via the filesystem.
-    config_file = config.execution.work_dir / '.dmriprep.toml'
+    config_file = config.execution.work_dir / ".dmriprep.toml"
     config.to_filename(config_file)
 
     # CRITICAL Call build_workflow(config_file, retval) in a subprocess.
     # Because Python on Linux does not ever free virtual memory (VM), running the
     # workflow construction jailed within a process preempts excessive VM buildup.
     with Manager() as mgr:
         from .workflow import build_workflow
+
         retval = mgr.dict()
         p = Process(target=build_workflow, args=(str(config_file), retval))
         p.start()
         p.join()
 
-        retcode = p.exitcode or retval.get('return_code', 0)
-        dmriprep_wf = retval.get('workflow', None)
+        retcode = p.exitcode or retval.get("return_code", 0)
+        dmriprep_wf = retval.get("workflow", None)
 
     # CRITICAL Load the config from the file. This is necessary because the ``build_workflow``
     # function executed constrained in a process may change the config (and thus the global
@@ -52,7 +55,7 @@ def main():
         sys.exit(int(retcode > 0))
 
     if dmriprep_wf and config.execution.write_graph:
-        dmriprep_wf.write_graph(graph2use="colored", format='svg', simple_form=True)
+        dmriprep_wf.write_graph(graph2use="colored", format="svg", simple_form=True)
 
     retcode = retcode or (dmriprep_wf is None) * os.EX_SOFTWARE
     if retcode != 0:
@@ -61,8 +64,8 @@ def main():
     # Generate boilerplate
     with Manager() as mgr:
         from .workflow import build_boilerplate
-        p = Process(target=build_boilerplate,
-                    args=(str(config_file), dmriprep_wf))
+
+        p = Process(target=build_boilerplate, args=(str(config_file), dmriprep_wf))
         p.start()
         p.join()
 
@@ -73,39 +76,52 @@ def main():
     gc.collect()
 
     if popylar is not None:
-        popylar.track_event(__ga_id__, 'run', 'started')
+        popylar.track_event(__ga_id__, "run", "started")
 
-    config.loggers.workflow.log(15, '\n'.join(
-        ['dMRIPrep config:'] + ['\t\t%s' % s for s in config.dumps().splitlines()])
+    config.loggers.workflow.log(
+        15,
+        "\n".join(
+            ["dMRIPrep config:"] + ["\t\t%s" % s for s in config.dumps().splitlines()]
+        ),
     )
-    config.loggers.workflow.log(25, 'dMRIPrep started!')
+    config.loggers.workflow.log(25, "dMRIPrep started!")
     errno = 1  # Default is error exit unless otherwise set
     try:
         dmriprep_wf.run(**config.nipype.get_plugin())
     except Exception as e:
         if not config.execution.notrack:
-            popylar.track_event(__ga_id__, 'run', 'error')
-        config.loggers.workflow.critical('dMRIPrep failed: %s', e)
+            popylar.track_event(__ga_id__, "run", "error")
+        config.loggers.workflow.critical("dMRIPrep failed: %s", e)
         raise
     else:
-        config.loggers.workflow.log(25, 'dMRIPrep finished successfully!')
+        config.loggers.workflow.log(25, "dMRIPrep finished successfully!")
 
         # Bother users with the boilerplate only iff the workflow went okay.
-        if (config.execution.output_dir / 'dmriprep' / 'logs' / 'CITATION.md').exists():
+        if (config.execution.output_dir / "dmriprep" / "logs" / "CITATION.md").exists():
             config.loggers.workflow.log(
-                25, 'Works derived from this dMRIPrep execution should '
-                'include the following boilerplate:\n\n%s',
-                (config.execution.output_dir / 'dmriprep' / 'logs' / 'CITATION.md').read_text()
+                25,
+                "Works derived from this dMRIPrep execution should "
+                "include the following boilerplate:\n\n%s",
+                (
+                    config.execution.output_dir / "dmriprep" / "logs" / "CITATION.md"
+                ).read_text(),
             )
 
         if config.workflow.run_reconall:
             from templateflow import api
             from niworkflows.utils.misc import _copy_any
-            dseg_tsv = str(api.get('fsaverage', suffix='dseg', extension=['.tsv']))
-            _copy_any(dseg_tsv,
-                      str(config.execution.output_dir / 'dmriprep' / 'desc-aseg_dseg.tsv'))
-            _copy_any(dseg_tsv,
-                      str(config.execution.output_dir / 'dmriprep' / 'desc-aparcaseg_dseg.tsv'))
+
+            dseg_tsv = str(api.get("fsaverage", suffix="dseg", extension=[".tsv"]))
+            _copy_any(
+                dseg_tsv,
+                str(config.execution.output_dir / "dmriprep" / "desc-aseg_dseg.tsv"),
+            )
+            _copy_any(
+                dseg_tsv,
+                str(
+                    config.execution.output_dir / "dmriprep" / "desc-aparcaseg_dseg.tsv"
+                ),
+            )
         errno = 0
     finally:
         from niworkflows.reports import generate_reports
@@ -117,17 +133,20 @@ def main():
             config.execution.output_dir,
             config.execution.work_dir,
             config.execution.run_uuid,
-            config=pkgrf('dmriprep', 'config/reports-spec.yml'),
-            packagename='dmriprep')
+            config=pkgrf("dmriprep", "config/reports-spec.yml"),
+            packagename="dmriprep",
+        )
         write_derivative_description(
-            config.execution.bids_dir,
-            config.execution.output_dir / 'dmriprep')
+            config.execution.bids_dir, config.execution.output_dir / "dmriprep"
+        )
 
         if failed_reports and not config.execution.notrack:
-            popylar.track_event(__ga_id__, 'run', 'reporting_error')
+            popylar.track_event(__ga_id__, "run", "reporting_error")
         sys.exit(int((errno + failed_reports) > 0))
 
 
-if __name__ == '__main__':
-    raise RuntimeError("dmriprep/cli/run.py should not be run directly;\n"
-                       "Please `pip install` dmriprep and use the `dmriprep` command")
+if __name__ == "__main__":
+    raise RuntimeError(
+        "dmriprep/cli/run.py should not be run directly;\n"
+        "Please `pip install` dmriprep and use the `dmriprep` command"
+    )

dmriprep/cli/run.py

@@ -5,15 +5,18 @@
 from .. import version as _version
 from ... import config
 
-MIN_ARGS = ['data/', 'out/', 'participant']
-
-
-@pytest.mark.parametrize('args,code', [
-    ([], 2),
-    (MIN_ARGS, 2),  # bids_dir does not exist
-    (MIN_ARGS + ['--fs-license-file'], 2),
-    (MIN_ARGS + ['--fs-license-file', 'fslicense.txt'], 2),
-])
+MIN_ARGS = ["data/", "out/", "participant"]
+
+
+@pytest.mark.parametrize(
+    "args,code",
+    [
+        ([], 2),
+        (MIN_ARGS, 2),  # bids_dir does not exist
+        (MIN_ARGS + ["--fs-license-file"], 2),
+        (MIN_ARGS + ["--fs-license-file", "fslicense.txt"], 2),
+    ],
+)
 def test_parser_errors(args, code):
     """Check behavior of the parser."""
     with pytest.raises(SystemExit) as error:
@@ -22,96 +25,93 @@ def test_parser_errors(args, code):
     assert error.value.code == code
 
 
-@pytest.mark.parametrize('args', [
-    MIN_ARGS,
-    MIN_ARGS + ['--fs-license-file'],
-])
+@pytest.mark.parametrize("args", [MIN_ARGS, MIN_ARGS + ["--fs-license-file"]])
 def test_parser_valid(tmp_path, args):
     """Check valid arguments."""
-    datapath = (tmp_path / 'data').absolute()
+    datapath = (tmp_path / "data").absolute()
     datapath.mkdir(exist_ok=True)
     args[0] = str(datapath)
 
-    if '--fs-license-file' in args:
-        _fs_file = tmp_path / 'license.txt'
-        _fs_file.write_text('')
-        args.insert(args.index('--fs-license-file') + 1,
-                    str(_fs_file.absolute()))
+    if "--fs-license-file" in args:
+        _fs_file = tmp_path / "license.txt"
+        _fs_file.write_text("")
+        args.insert(args.index("--fs-license-file") + 1, str(_fs_file.absolute()))
 
     opts = _build_parser().parse_args(args)
 
     assert opts.bids_dir == datapath
 
 
-@pytest.mark.parametrize('argval,gb', [
-    ('1G', 1),
-    ('1GB', 1),
-    ('1000', 1),    # Default units are MB
-    ('32000', 32),  # Default units are MB
-    ('4000', 4),    # Default units are MB
-    ('1000M', 1),
-    ('1000MB', 1),
-    ('1T', 1000),
-    ('1TB', 1000),
-    ('%dK' % 1e6, 1),
-    ('%dKB' % 1e6, 1),
-    ('%dB' % 1e9, 1),
-])
+@pytest.mark.parametrize(
+    "argval,gb",
+    [
+        ("1G", 1),
+        ("1GB", 1),
+        ("1000", 1),  # Default units are MB
+        ("32000", 32),  # Default units are MB
+        ("4000", 4),  # Default units are MB
+        ("1000M", 1),
+        ("1000MB", 1),
+        ("1T", 1000),
+        ("1TB", 1000),
+        ("%dK" % 1e6, 1),
+        ("%dKB" % 1e6, 1),
+        ("%dB" % 1e9, 1),
+    ],
+)
 def test_memory_arg(tmp_path, argval, gb):
     """Check the correct parsing of the memory argument."""
-    datapath = (tmp_path / 'data').absolute()
+    datapath = (tmp_path / "data").absolute()
     datapath.mkdir(exist_ok=True)
-    _fs_file = tmp_path / 'license.txt'
-    _fs_file.write_text('')
+    _fs_file = tmp_path / "license.txt"
+    _fs_file.write_text("")
 
-    args = MIN_ARGS + ['--fs-license-file', str(_fs_file)] \
-        + ['--mem', argval]
+    args = MIN_ARGS + ["--fs-license-file", str(_fs_file)] + ["--mem", argval]
     args[0] = str(datapath)
     opts = _build_parser().parse_args(args)
 
     assert opts.memory_gb == gb
 
 
-@pytest.mark.parametrize('current,latest', [
-    ('1.0.0', '1.3.2'),
-    ('1.3.2', '1.3.2')
-])
+@pytest.mark.parametrize("current,latest", [("1.0.0", "1.3.2"), ("1.3.2", "1.3.2")])
 def test_get_parser_update(monkeypatch, capsys, current, latest):
     """Make sure the out-of-date banner is shown."""
     expectation = Version(current) < Version(latest)
 
     def _mock_check_latest(*args, **kwargs):
         return Version(latest)
 
-    monkeypatch.setattr(config.environment, 'version', current)
-    monkeypatch.setattr(_version, 'check_latest', _mock_check_latest)
+    monkeypatch.setattr(config.environment, "version", current)
+    monkeypatch.setattr(_version, "check_latest", _mock_check_latest)
 
     _build_parser()
     captured = capsys.readouterr().err
 
     msg = """\
 You are using dMRIPrep-%s, and a newer version of dMRIPrep is available: %s.
 Please check out our documentation about how and when to upgrade:
-https://dmriprep.readthedocs.io/en/latest/faq.html#upgrading""" % (current, latest)
+https://dmriprep.readthedocs.io/en/latest/faq.html#upgrading""" % (
+        current,
+        latest,
+    )
 
     assert (msg in captured) is expectation
 
 
-@pytest.mark.parametrize('flagged', [
-    (True, None),
-    (True, 'random reason'),
-    (False, None),
-])
+@pytest.mark.parametrize(
+    "flagged", [(True, None), (True, "random reason"), (False, None)]
+)
 def test_get_parser_blacklist(monkeypatch, capsys, flagged):
     """Make sure the blacklisting banner is shown."""
+
     def _mock_is_bl(*args, **kwargs):
         return flagged
 
-    monkeypatch.setattr(_version, 'is_flagged', _mock_is_bl)
+    monkeypatch.setattr(_version, "is_flagged", _mock_is_bl)
 
     _build_parser()
     captured = capsys.readouterr().err
 
-    assert ('FLAGGED' in captured) is flagged[0]
+    assert ("FLAGGED" in captured) is flagged[0]
     if flagged[0]:
-        assert ((flagged[1] or 'reason: unknown') in captured)
+        assert (flagged[1] or "reason: unknown") in captured