From 737dd6d0ab4a409b142a0d160c1cd613c8946697 Mon Sep 17 00:00:00 2001 From: oesteban Date: Wed, 21 Nov 2018 12:25:27 -0800 Subject: [PATCH 01/42] [RF] Detachable ``run_command`` process **Introduction**: the current implementation of ``run_command`` makes use of ``Stream``s to redirect output in a sort of inflexible way, and does not allow running the processes in the background. **This PR**: modifies the behavior of ``run_command`` opening a new self-control thread, that is in charge of printing to the terminal if the ``terminal_output`` is set to ``'stream'``. Output streams are redirected to files by default, trying to reduce the amount of data buffered in memory and also to ensure that there is a trace of nipype's execution. Additionally, a new argument ``background`` can be set to ``True``, detaching from the process. This lends itself to important improvements on the ``MultiProc`` variants, as all these processes can be launched directly from the main thread and run in the background. I hope this will drastically reduce the memory fingerprint of workers (if we actually need to maintain a ``Pool``, which is probably not the case anymore!). **Tests**: I've added a couple of tests, but I'm happy to add more as I am aware that this may be too big of a change to the nipype interfaces. **Extra**: removed a couple of deprecated objects that we promised to trash past nipype-1.1.0. --- nipype/interfaces/base/core.py | 44 +--- nipype/utils/subprocess.py | 389 ++++++++++++++++++--------------- 2 files changed, 219 insertions(+), 214 deletions(-) diff --git a/nipype/interfaces/base/core.py b/nipype/interfaces/base/core.py index 2a0e2e32f8..4fef4e0a10 100644 --- a/nipype/interfaces/base/core.py +++ b/nipype/interfaces/base/core.py @@ -22,7 +22,7 @@ import os import re import platform -import subprocess as sp +from tempfile import TemporaryDirectory import shlex import sys from textwrap import wrap @@ -845,26 +845,6 @@ def raise_exception(self, runtime): def _get_environ(self): return getattr(self.inputs, 'environ', {}) - def version_from_command(self, flag='-v', cmd=None): - iflogger.warning('version_from_command member of CommandLine was ' - 'Deprecated in nipype-1.0.0 and deleted in 1.1.0') - if cmd is None: - cmd = self.cmd.split()[0] - - env = dict(os.environ) - if which(cmd, env=env): - out_environ = self._get_environ() - env.update(out_environ) - proc = sp.Popen( - ' '.join((cmd, flag)), - shell=True, - env=env, - stdout=sp.PIPE, - stderr=sp.PIPE, - ) - o, e = proc.communicate() - return o - def _run_interface(self, runtime, correct_return_codes=(0, )): """Execute command via subprocess @@ -881,10 +861,9 @@ def _run_interface(self, runtime, correct_return_codes=(0, )): out_environ = self._get_environ() # Initialize runtime Bunch - runtime.stdout = None - runtime.stderr = None runtime.cmdline = self.cmdline runtime.environ.update(out_environ) + runtime.shell = True # which $cmd executable_name = shlex.split(self._cmd_prefix + self.cmd)[0] @@ -900,7 +879,7 @@ def _run_interface(self, runtime, correct_return_codes=(0, )): runtime.dependencies = (get_dependencies(executable_name, runtime.environ) if self._ldd else '') - runtime = run_command(runtime, output=self.terminal_output) + runtime = run_command(runtime, output=self.terminal_output).runtime if runtime.returncode is None or \ runtime.returncode not in correct_return_codes: self.raise_exception(runtime) @@ -1203,15 +1182,16 @@ class PackageInfo(object): def version(klass): if klass._version is None: if klass.version_cmd is not None: - try: - clout = CommandLine( - command=klass.version_cmd, - resource_monitor=False, - terminal_output='allatonce').run() - except IOError: - return None + with TemporaryDirectory() as tmp_folder: + runtime = run_command( + Bunch(cmdline=klass.version_cmd, + cwd=tmp_folder)).runtime + + if runtime.returncode != 0: + return None - raw_info = clout.runtime.stdout + with open(runtime.stdout) as stdout: + raw_info = stdout.read() elif klass.version_file is not None: try: with open(klass.version_file, 'rt') as fobj: diff --git a/nipype/utils/subprocess.py b/nipype/utils/subprocess.py index 5516482936..70a373fe56 100644 --- a/nipype/utils/subprocess.py +++ b/nipype/utils/subprocess.py @@ -9,195 +9,220 @@ import sys import gc import errno -import select -import locale -import datetime -from subprocess import Popen, STDOUT, PIPE -from .filemanip import canonicalize_env, read_stream +import threading +from time import time, sleep +from subprocess import Popen +from .filemanip import canonicalize_env from .. import logging -from builtins import range, object - iflogger = logging.getLogger('nipype.interface') -class Stream(object): - """Function to capture stdout and stderr streams with timestamps +def run_command(runtime, background=False, output=None, period=0.01, + callback_fn=None, callback_args=None, callback_kwargs=None): + r""" + Run a command in a subprocess, handling output and allowing for + background processes. + + Parameters + ---------- + runtime: :class:`~nipype.interfaces.support.Bunch` + runtime object encapsulating the command line, current working + directory, environment, etc. + background: bool + whether the command line should be waited for, or run in background + otherwise. + output: str or None + accepts the keyword ``stream`` when the command's outputs should + be also printed out to the terminal. + period: float + process polling period (in seconds). + callback_fn: callable + a function to be called when the process has terminated. + callback_args: tuple or None + positional arguments to be passed over to ``callback_fn()``. + callback_kwargs: dict or None + keyword arguments to be passed over to ``callback_fn()``. + + Returns + ------- + + rtmon: :class:`.RuntimeMonitor` + the runtime monitor thread + + + >>> from time import sleep + >>> from nipype.interfaces.base.support import Bunch + >>> from nipype.utils.subprocess import run_command + >>> rt = run_command(Bunch(cmdline='echo hello!')).runtime + >>> rt.returncode + 0 + >>> with open(rt.stdout) as stdout: + ... data = stdout.read() + >>> data + 'hello!\n' + >>> rt = run_command(Bunch(cmdline='sleep 2'), background=True).runtime + >>> rt.returncode is None + True + >>> sleep(5) + >>> rt.returncode + 0 - stackoverflow.com/questions/4984549/merge-and-sync-stdout-and-stderr/5188359 """ - def __init__(self, name, impl): - self._name = name - self._impl = impl - self._buf = '' - self._rows = [] - self._lastidx = 0 - self.default_encoding = locale.getdefaultlocale()[1] or 'UTF-8' - - def fileno(self): - "Pass-through for file descriptor." - return self._impl.fileno() - - def read(self, drain=0): - "Read from the file descriptor. If 'drain' set, read until EOF." - while self._read(drain) is not None: - if not drain: - break - - def _read(self, drain): - "Read from the file descriptor" - fd = self.fileno() - buf = os.read(fd, 4096).decode(self.default_encoding) - if not buf and not self._buf: - return None - if '\n' not in buf: - if not drain: - self._buf += buf - return [] - - # prepend any data previously read, then split into lines and format - buf = self._buf + buf - if '\n' in buf: - tmp, rest = buf.rsplit('\n', 1) - else: - tmp = buf - rest = None - self._buf = rest - now = datetime.datetime.now().isoformat() - rows = tmp.split('\n') - self._rows += [(now, '%s %s:%s' % (self._name, now, r), r) - for r in rows] - for idx in range(self._lastidx, len(self._rows)): - iflogger.info(self._rows[idx][1]) - self._lastidx = len(self._rows) - - -def run_command(runtime, output=None, timeout=0.01): - """Run a command, read stdout and stderr, prefix with timestamp. - - The returned runtime contains a merged stdout+stderr log with timestamps - """ + rtmon = RuntimeMonitor(runtime, output, + period=period, + callback_fn=callback_fn, + callback_args=callback_args, + callback_kwargs=callback_kwargs) + rtmon.start() + + if not background: + rtmon.join() + + return rtmon - # Init variables - cmdline = runtime.cmdline - env = canonicalize_env(runtime.environ) - - errfile = None - outfile = None - stdout = PIPE - stderr = PIPE - - if output == 'file': - outfile = os.path.join(runtime.cwd, 'output.nipype') - stdout = open(outfile, 'wb') # t=='text'===default - stderr = STDOUT - elif output == 'file_split': - outfile = os.path.join(runtime.cwd, 'stdout.nipype') - stdout = open(outfile, 'wb') - errfile = os.path.join(runtime.cwd, 'stderr.nipype') - stderr = open(errfile, 'wb') - elif output == 'file_stdout': - outfile = os.path.join(runtime.cwd, 'stdout.nipype') - stdout = open(outfile, 'wb') - elif output == 'file_stderr': - errfile = os.path.join(runtime.cwd, 'stderr.nipype') - stderr = open(errfile, 'wb') - - proc = Popen( - cmdline, - stdout=stdout, - stderr=stderr, - shell=True, - cwd=runtime.cwd, - env=env, - close_fds=(not sys.platform.startswith('win')), - ) - - result = { - 'stdout': [], - 'stderr': [], - 'merged': [], - } - - if output == 'stream': - streams = [ - Stream('stdout', proc.stdout), - Stream('stderr', proc.stderr) - ] - - def _process(drain=0): - try: - res = select.select(streams, [], [], timeout) - except select.error as e: - iflogger.info(e) - if e[0] == errno.EINTR: - return - else: - raise - else: - for stream in res[0]: - stream.read(drain) - - while proc.returncode is None: - proc.poll() - _process() - - _process(drain=1) - - # collect results, merge and return - result = {} - temp = [] - for stream in streams: - rows = stream._rows - temp += rows - result[stream._name] = [r[2] for r in rows] - temp.sort() - result['merged'] = [r[1] for r in temp] - - if output.startswith('file'): - proc.wait() - if outfile is not None: - stdout.flush() - stdout.close() - with open(outfile, 'rb') as ofh: - stdoutstr = ofh.read() - result['stdout'] = read_stream(stdoutstr, logger=iflogger) - del stdoutstr - - if errfile is not None: - stderr.flush() - stderr.close() - with open(errfile, 'rb') as efh: - stderrstr = efh.read() - result['stderr'] = read_stream(stderrstr, logger=iflogger) - del stderrstr - - if output == 'file': - result['merged'] = result['stdout'] - result['stdout'] = [] - else: - stdout, stderr = proc.communicate() - if output == 'allatonce': # Discard stdout and stderr otherwise - result['stdout'] = read_stream(stdout, logger=iflogger) - result['stderr'] = read_stream(stderr, logger=iflogger) - - runtime.returncode = proc.returncode - try: - proc.terminate() # Ensure we are done - except OSError as error: - # Python 2 raises when the process is already gone - if error.errno != errno.ESRCH: - raise - - # Dereference & force GC for a cleanup - del proc - del stdout - del stderr - gc.collect() - - runtime.stderr = '\n'.join(result['stderr']) - runtime.stdout = '\n'.join(result['stdout']) - runtime.merged = '\n'.join(result['merged']) - return runtime + +class RuntimeMonitor(threading.Thread): + """ + A ``Thread`` to monitor a subprocess with a certain polling + period + """ + __slots__ = ['_proc', '_output', '_stdoutfh', '_stderrfh', '_runtime', + '_callback_fn', '_calback_args', '_callback_kwargs'] + + def __init__(self, runtime, output=None, period=0.1, + callback_fn=None, callback_args=None, callback_kwargs=None): + """ + Initialize a self-monitored process. + + + Parameters + ---------- + runtime: :class:`~nipype.interfaces.support.Bunch` + runtime object encapsulating the command line, current working + directory, environment, etc. + output: str or None + accepts the keyword ``stream`` when the command's outputs should + be also printed out to the terminal. + period: float + process polling period (in seconds). + callback_fn: callable + a function to be called when the process has terminated. + callback_args: tuple or None + positional arguments to be passed over to ``callback_fn()``. + callback_kwargs: dict or None + keyword arguments to be passed over to ``callback_fn()``. + + + """ + self._proc = None + self._output = output + self._period = period + self._stdoutfh = None + self._stderrfh = None + self._runtime = runtime + self._runtime.returncode = None + self._callback_fn = callback_fn + self._callback_args = callback_args or tuple() + self._callback_kwargs = callback_kwargs or {} + + cwd = getattr(self._runtime, 'cwd', os.getcwd()) + self._runtime.cwd = cwd + self._runtime.stdout = getattr( + self._runtime, 'stdout', os.path.join(cwd, '.nipype.out')) + self._runtime.stderr = getattr( + self._runtime, 'stderr', os.path.join(cwd, '.nipype.err')) + + # Open file descriptors and get number + self._stdoutfh = open(self._runtime.stdout, 'wb') + self._stderrfh = open(self._runtime.stderr, 'wb') + + # Start thread + threading.Thread.__init__(self) + + @property + def runtime(self): + return self._runtime + + def _update_output(self, tracker=None): + """When the ``stream`` output is selected, just keeps + track of the logs backing up the process' outputs and + sends them to the standard i/o streams""" + if self._output == 'stream': + self._stdoutfh.flush() + self._stderrfh.flush() + if tracker is None: + tracker = (0, 0) + + out_size = os.stat(self._runtime.stdout).st_size + err_size = os.stat(self._runtime.stderr).st_size + + if out_size > tracker[0]: + data = None + with open(self._runtime.stdout) as out: + out.seek(tracker[0] + int(tracker[0] > 0)) + data = out.read() + tracker = (out_size, tracker[1]) + if data: + print(data) + + if err_size > tracker[1]: + data = None + with open(self._runtime.stderr) as err: + err.seek(tracker[1] + int(tracker[1] > 0)) + data = err.read() + tracker = (tracker[0], err_size) + if data: + print(err.read(), file=sys.stderr) + return tracker + + def run(self): + """Monitor the process and fill in the runtime object""" + + # Init variables + cmdline = self._runtime.cmdline + env = canonicalize_env( + getattr(self._runtime, 'environ', os.environ)) + + tracker = None + start_time = time() + self._proc = Popen( + cmdline, + stdout=self._stdoutfh.fileno(), + stderr=self._stderrfh.fileno(), + shell=getattr(self._runtime, 'shell', True), + cwd=self._runtime.cwd, + env=env, + close_fds=False, + ) + wait_til = start_time + while self._proc.returncode is None: + self._proc.poll() + tracker = self._update_output(tracker) + wait_til += self._period + sleep(max(0, wait_til - time())) + self._runtime.returncode = self._proc.returncode + + try: + self._proc.terminate() # Ensure we are done + except OSError as error: + # Python 2 raises when the process is already gone + if error.errno != errno.ESRCH: + raise + + # Close open file descriptors + self._stdoutfh.flush() + self._stdoutfh.close() + self._stderrfh.flush() + self._stderrfh.close() + + # Run callback + if self._callback_fn and hasattr(self._callback_fn, '__call__'): + self._callback_fn(*self._callback_args, + **self._callback_kwargs) + + # Dereference & force GC for a cleanup + del self._proc + gc.collect() From 27a999b028c0980e5c20ff00c9e22ef1d9bdade1 Mon Sep 17 00:00:00 2001 From: oesteban Date: Wed, 21 Nov 2018 12:55:06 -0800 Subject: [PATCH 02/42] be more restrictive about ``shell=True`` --- nipype/interfaces/base/core.py | 2 +- nipype/utils/subprocess.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nipype/interfaces/base/core.py b/nipype/interfaces/base/core.py index 4fef4e0a10..74616eda7d 100644 --- a/nipype/interfaces/base/core.py +++ b/nipype/interfaces/base/core.py @@ -1185,7 +1185,7 @@ def version(klass): with TemporaryDirectory() as tmp_folder: runtime = run_command( Bunch(cmdline=klass.version_cmd, - cwd=tmp_folder)).runtime + cwd=tmp_folder, shell=True)).runtime if runtime.returncode != 0: return None diff --git a/nipype/utils/subprocess.py b/nipype/utils/subprocess.py index 70a373fe56..34bd67d351 100644 --- a/nipype/utils/subprocess.py +++ b/nipype/utils/subprocess.py @@ -192,7 +192,7 @@ def run(self): cmdline, stdout=self._stdoutfh.fileno(), stderr=self._stderrfh.fileno(), - shell=getattr(self._runtime, 'shell', True), + shell=getattr(self._runtime, 'shell', False), cwd=self._runtime.cwd, env=env, close_fds=False, From 942a927e4f772a7a6a3248deb37f4b321b25a348 Mon Sep 17 00:00:00 2001 From: oesteban Date: Wed, 21 Nov 2018 17:01:10 -0800 Subject: [PATCH 03/42] fix error --- nipype/utils/subprocess.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nipype/utils/subprocess.py b/nipype/utils/subprocess.py index 34bd67d351..1a1c8bc96e 100644 --- a/nipype/utils/subprocess.py +++ b/nipype/utils/subprocess.py @@ -175,7 +175,7 @@ def _update_output(self, tracker=None): data = err.read() tracker = (tracker[0], err_size) if data: - print(err.read(), file=sys.stderr) + print(data, file=sys.stderr) return tracker def run(self): From fae15009c473a0f570a581c009fe2dd2512e4c20 Mon Sep 17 00:00:00 2001 From: oesteban Date: Wed, 21 Nov 2018 17:10:51 -0800 Subject: [PATCH 04/42] normalizing terminal_output, reading in standard i/o --- doc/devel/interface_specs.rst | 54 +++------------ doc/devel/matlab_example2.py | 4 +- examples/nipype_tutorial.ipynb | 2 +- examples/rsfmri_vol_surface_preprocessing.py | 2 +- .../rsfmri_vol_surface_preprocessing_nipy.py | 2 +- nipype/interfaces/afni/base.py | 12 ++-- nipype/interfaces/afni/preprocess.py | 20 ++---- nipype/interfaces/afni/utils.py | 10 +-- nipype/interfaces/ants/registration.py | 6 +- nipype/interfaces/ants/resampling.py | 16 +++-- nipype/interfaces/ants/segmentation.py | 16 +++-- nipype/interfaces/base/core.py | 19 +++++- nipype/interfaces/base/tests/test_core.py | 68 ++++--------------- nipype/interfaces/diffusion_toolkit/base.py | 5 +- nipype/interfaces/dtitk/registration.py | 13 ++-- nipype/interfaces/dynamic_slicer.py | 8 ++- nipype/interfaces/freesurfer/utils.py | 29 +++++--- nipype/interfaces/fsl/dti.py | 29 +++++--- nipype/interfaces/fsl/epi.py | 26 ++++--- nipype/interfaces/fsl/model.py | 13 ++-- nipype/interfaces/fsl/preprocess.py | 13 ++-- nipype/interfaces/matlab.py | 23 ++++--- nipype/interfaces/minc/base.py | 6 +- nipype/interfaces/slicer/generate_classes.py | 7 -- nipype/interfaces/spm/base.py | 13 ++-- nipype/pipeline/engine/nodes.py | 10 +-- nipype/pipeline/engine/utils.py | 39 ++++++----- nipype/pipeline/plugins/condor.py | 12 ++-- nipype/pipeline/plugins/dagman.py | 2 +- nipype/pipeline/plugins/legacymultiproc.py | 4 +- nipype/pipeline/plugins/lsf.py | 17 ++--- nipype/pipeline/plugins/multiproc.py | 2 +- nipype/pipeline/plugins/oar.py | 6 +- nipype/pipeline/plugins/pbs.py | 13 ++-- nipype/pipeline/plugins/pbsgraph.py | 2 +- nipype/pipeline/plugins/sge.py | 6 +- nipype/pipeline/plugins/sgegraph.py | 2 +- nipype/pipeline/plugins/slurm.py | 12 ++-- nipype/pipeline/plugins/slurmgraph.py | 2 +- nipype/utils/docparse.py | 12 ++-- 40 files changed, 293 insertions(+), 264 deletions(-) diff --git a/doc/devel/interface_specs.rst b/doc/devel/interface_specs.rst index 56e1be9ed4..64beb4c396 100644 --- a/doc/devel/interface_specs.rst +++ b/doc/devel/interface_specs.rst @@ -12,8 +12,8 @@ In case of trouble, we encourage you to post on `NeuroStars `_ channel or in the +Alternatively, you're welcome to chat with us in the Nipype +`Gitter `_ channel or in the BrainHack `Slack `_ channel. (Click `here `_ to join the Slack workspace.) @@ -163,62 +163,28 @@ Controlling outputs to terminal It is very likely that the software wrapped within the interface writes to the standard output or the standard error of the terminal. -Interfaces provide a means to access and retrieve these outputs, by -using the ``terminal_output`` attribute: :: +Interfaces redirect both streams to logfiles under the interface's working +directory (by default: ``.nipype.out`` and ``.nipype.err``, respectively) :: import nipype.interfaces.fsl as fsl mybet = fsl.BET(from_file='bet-settings.json') - mybet.terminal_output = 'file_split' -In the example, the ``terminal_output = 'file_split'`` will redirect the -standard output and the standard error to split files (called -``stdout.nipype`` and ``stderr.nipype`` respectively). -The possible values for ``terminal_output`` are: - -*file* - Redirects both standard output and standard error to the same file - called ``output.nipype``. - Messages from both streams will be overlapped as they arrive to - the file. - -*file_split* - Redirects the output streams separately, to ``stdout.nipype`` - and ``stderr.nipype`` respectively, as described in the example. - -*file_stdout* - Only the standard output will be redirected to ``stdout.nipype`` - and the standard error will be discarded. - -*file_stderr* - Only the standard error will be redirected to ``stderr.nipype`` - and the standard output will be discarded. +By default, the contents of both logs are copied to the standard output +and error streams. *stream* - Both output streams are redirected to the current logger printing - their messages interleaved and immediately to the terminal. - -*allatonce* - Both output streams will be forwarded to a buffer and stored - separately in the `runtime` object that the `run()` method returns. - No files are written nor streams printed out to terminal. + Both standard output and standard error are copied to the + terminal. -*none* - Both outputs are discarded -In all cases, except for the ``'none'`` setting of ``terminal_output``, -the ``run()`` method will return a "runtime" object that will contain -the streams in the corresponding properties (``runtime.stdout`` -for the standard output, ``runtime.stderr`` for the standard error, and -``runtime.merged`` for both when streams are mixed, eg. when using the -*file* option). :: +The ``runtime`` object will keep the location of both logging files: import nipype.interfaces.fsl as fsl mybet = fsl.BET(from_file='bet-settings.json') - mybet.terminal_output = 'file_split' ... result = mybet.run() result.runtime.stdout - ' ... captured standard output ...' + '/path/to/interface/cwd/.nipype.out' diff --git a/doc/devel/matlab_example2.py b/doc/devel/matlab_example2.py index 8d683ea45f..7aabd7f113 100644 --- a/doc/devel/matlab_example2.py +++ b/doc/devel/matlab_example2.py @@ -44,7 +44,9 @@ def run(self, **inputs): # Inject your script self.inputs.script = self._my_script() results = super(MatlabCommand, self).run(**inputs) - stdout = results.runtime.stdout + + with open(results.runtime.stdout, 'rt') as f: + stdout = f.read() # Attach stdout to outputs to access matlab results results.outputs.matlab_output = stdout return results diff --git a/examples/nipype_tutorial.ipynb b/examples/nipype_tutorial.ipynb index 9a7678dfd1..d7d97467f9 100644 --- a/examples/nipype_tutorial.ipynb +++ b/examples/nipype_tutorial.ipynb @@ -635,7 +635,7 @@ "convert = MRIConvert(in_file='../ds107/sub001/BOLD/task001_run001/bold.nii.gz',\n", " out_file='ds107.nii')\n", "print(convert.cmdline)\n", - "results = convert.run(terminal_output='none') # allatonce, stream (default), file" + "results = convert.run(terminal_output='default') # default, stream" ], "language": "python", "metadata": {}, diff --git a/examples/rsfmri_vol_surface_preprocessing.py b/examples/rsfmri_vol_surface_preprocessing.py index 20b150b149..50c1ccaf3d 100644 --- a/examples/rsfmri_vol_surface_preprocessing.py +++ b/examples/rsfmri_vol_surface_preprocessing.py @@ -49,7 +49,7 @@ import os from nipype.interfaces.base import CommandLine -CommandLine.set_default_terminal_output('allatonce') +CommandLine.set_default_terminal_output('default') from dicom import read_file diff --git a/examples/rsfmri_vol_surface_preprocessing_nipy.py b/examples/rsfmri_vol_surface_preprocessing_nipy.py index d3d9887cc6..75a661d3fc 100644 --- a/examples/rsfmri_vol_surface_preprocessing_nipy.py +++ b/examples/rsfmri_vol_surface_preprocessing_nipy.py @@ -51,7 +51,7 @@ import os from nipype.interfaces.base import CommandLine -CommandLine.set_default_terminal_output('allatonce') +CommandLine.set_default_terminal_output('default') # https://github.com/moloney/dcmstack from dcmstack.extract import default_extractor diff --git a/nipype/interfaces/afni/base.py b/nipype/interfaces/afni/base.py index d4b8e474ff..24be6405f7 100644 --- a/nipype/interfaces/afni/base.py +++ b/nipype/interfaces/afni/base.py @@ -87,13 +87,14 @@ def standard_image(img_name): Could be made more fancy to allow for more relocatability''' clout = CommandLine( 'which afni', + terminal_output='default', ignore_exception=True, - resource_monitor=False, - terminal_output='allatonce').run() + resource_monitor=False).run() if clout.runtime.returncode is not 0: return None - out = clout.runtime.stdout + with open(clout.runtime.stdout, 'rt') as f: + out = f.read().strip() basedir = os.path.split(out)[0] return os.path.join(basedir, img_name) @@ -104,10 +105,11 @@ class AFNICommandBase(CommandLine): See http://afni.nimh.nih.gov/afni/community/board/read.php?1,145346,145347#msg-145347 """ - def _run_interface(self, runtime): + def _run_interface(self, runtime, correct_return_codes=(0, )): if platform == 'darwin': runtime.environ['DYLD_FALLBACK_LIBRARY_PATH'] = '/usr/local/afni/' - return super(AFNICommandBase, self)._run_interface(runtime) + return super(AFNICommandBase, self)._run_interface( + runtime, correct_return_codes=correct_return_codes) class AFNICommandInputSpec(CommandLineInputSpec): diff --git a/nipype/interfaces/afni/preprocess.py b/nipype/interfaces/afni/preprocess.py index 0ecbe4b347..43cbb76fd1 100644 --- a/nipype/interfaces/afni/preprocess.py +++ b/nipype/interfaces/afni/preprocess.py @@ -1013,7 +1013,9 @@ def aggregate_outputs(self, runtime=None, needed_outputs=None): return self.run().outputs else: clip_val = [] - for line in runtime.stdout.split('\n'): + with open(runtime.stdout) as f: + stdout = f.read() + for line in stdout.splitlines(): if line: values = line.split() if len(values) > 1: @@ -1688,27 +1690,18 @@ class OutlierCount(CommandLine): _cmd = '3dToutcount' input_spec = OutlierCountInputSpec output_spec = OutlierCountOutputSpec - _terminal_output = 'file_split' def _parse_inputs(self, skip=None): if skip is None: skip = [] - # This is not strictly an input, but needs be - # set before run() is called. - if self.terminal_output == 'none': - self.terminal_output = 'file_split' - if not self.inputs.save_outliers: skip += ['outliers_file'] return super(OutlierCount, self)._parse_inputs(skip) def _run_interface(self, runtime): + runtime.stdout = op.abspath(self.inputs.out_file) runtime = super(OutlierCount, self)._run_interface(runtime) - - # Read from runtime.stdout or runtime.merged - with open(op.abspath(self.inputs.out_file), 'w') as outfh: - outfh.write(runtime.stdout or runtime.merged) return runtime def _list_outputs(self): @@ -1922,7 +1915,6 @@ class ROIStats(AFNICommandBase): """ _cmd = '3dROIstats' - _terminal_output = 'allatonce' input_spec = ROIStatsInputSpec output_spec = ROIStatsOutputSpec @@ -3055,7 +3047,9 @@ def _run_interface(self, runtime): if self.inputs.save_warp: import numpy as np warp_file = self._list_outputs()['warp_file'] - np.savetxt(warp_file, [runtime.stdout], fmt=str('%s')) + with open(runtime.stdout) as f: + stdout = f.read().strip() + np.savetxt(warp_file, [stdout], fmt=str('%s')) return runtime def _list_outputs(self): diff --git a/nipype/interfaces/afni/utils.py b/nipype/interfaces/afni/utils.py index 987fcec135..1a32cccefb 100644 --- a/nipype/interfaces/afni/utils.py +++ b/nipype/interfaces/afni/utils.py @@ -211,10 +211,12 @@ class Autobox(AFNICommand): def aggregate_outputs(self, runtime=None, needed_outputs=None): outputs = super(Autobox, self).aggregate_outputs( runtime, needed_outputs) - pattern = 'x=(?P-?\d+)\.\.(?P-?\d+) '\ - 'y=(?P-?\d+)\.\.(?P-?\d+) '\ - 'z=(?P-?\d+)\.\.(?P-?\d+)' - for line in runtime.stderr.split('\n'): + pattern = r'x=(?P-?\d+)\.\.(?P-?\d+) '\ + r'y=(?P-?\d+)\.\.(?P-?\d+) '\ + r'z=(?P-?\d+)\.\.(?P-?\d+)' + with open(runtime.stderr) as f: + stderr = f.read() + for line in stderr.splitlines(): m = re.search(pattern, line) if m: d = m.groupdict() diff --git a/nipype/interfaces/ants/registration.py b/nipype/interfaces/ants/registration.py index 9d3b07e96a..185d4a9fe2 100644 --- a/nipype/interfaces/ants/registration.py +++ b/nipype/interfaces/ants/registration.py @@ -940,9 +940,11 @@ def _run_interface(self, runtime, correct_return_codes=(0, )): runtime = super(Registration, self)._run_interface(runtime) # Parse some profiling info - output = runtime.stdout or runtime.merged + with open(runtime.stdout) as f: + output = f.read().strip() + if output: - lines = output.split('\n') + lines = output.splitlines() for l in lines[::-1]: # This should be the last line if l.strip().startswith('Total elapsed time:'): diff --git a/nipype/interfaces/ants/resampling.py b/nipype/interfaces/ants/resampling.py index e26a48ed6a..ed04cc7913 100644 --- a/nipype/interfaces/ants/resampling.py +++ b/nipype/interfaces/ants/resampling.py @@ -136,12 +136,16 @@ def _list_outputs(self): (name, self.inputs.out_postfix, ext))) return outputs - def _run_interface(self, runtime, correct_return_codes=[0]): - runtime = super(WarpTimeSeriesImageMultiTransform, - self)._run_interface( - runtime, correct_return_codes=[0, 1]) - if "100 % complete" not in runtime.stdout: - self.raise_exception(runtime) + def _run_interface(self, runtime, correct_return_codes=(0, 1)): + runtime = super( + WarpTimeSeriesImageMultiTransform, self)._run_interface( + runtime, correct_return_codes=correct_return_codes) + + with open(runtime.stdout) as stdoutfh: + output_str = stdoutfh.read() + + if "100 % complete" not in output_str: + self.raise_exception(runtime) return runtime diff --git a/nipype/interfaces/ants/segmentation.py b/nipype/interfaces/ants/segmentation.py index 04d212ec0f..ccf0c45843 100644 --- a/nipype/interfaces/ants/segmentation.py +++ b/nipype/interfaces/ants/segmentation.py @@ -818,8 +818,11 @@ def _run_interface(self, runtime, correct_return_codes=(0, )): runtime = super(BrainExtraction, self)._run_interface(runtime) # Still, double-check if it didn't found N4 - if 'we cant find' in runtime.stdout: - for line in runtime.stdout.split('\n'): + with open(runtime.stdout) as stdoutfh: + stdout = stdoutfh.read() + + if 'we cant find' in stdout: + for line in stdout.split('\n'): if line.strip().startswith('we cant find'): tool = line.strip().replace('we cant find the', '').split(' ')[0] @@ -828,10 +831,11 @@ def _run_interface(self, runtime, correct_return_codes=(0, )): errmsg = ( 'antsBrainExtraction.sh requires "%s" to be found in $ANTSPATH ' '($ANTSPATH="%s").') % (tool, ants_path) - if runtime.stderr is None: - runtime.stderr = errmsg - else: - runtime.stderr += '\n' + errmsg + + # Append errmsg to stderr file + with open(runtime.stderr, 'w+') as stderrfh: + stderrfh.write(errmsg) + runtime.returncode = 1 self.raise_exception(runtime) diff --git a/nipype/interfaces/base/core.py b/nipype/interfaces/base/core.py index 74616eda7d..9fdf9f5dfb 100644 --- a/nipype/interfaces/base/core.py +++ b/nipype/interfaces/base/core.py @@ -51,8 +51,8 @@ PY35 = sys.version_info >= (3, 5) PY3 = sys.version_info[0] > 2 VALID_TERMINAL_OUTPUT = [ - 'stream', 'allatonce', 'file', 'file_split', 'file_stdout', 'file_stderr', - 'none' + 'stream', 'default', + 'allatonce', 'file', 'file_split', 'file_stdout', 'file_stderr', 'none' ] __docformat__ = 'restructuredtext' @@ -837,10 +837,23 @@ def terminal_output(self, value): self._terminal_output = value def raise_exception(self, runtime): + with open(runtime.stdout) as stdoutfh: + stdout = stdoutfh.read() + + if not stdout: + stdout = '' + + with open(runtime.stderr) as stderrfh: + stderr = stderrfh.read() + + if not stderr: + stderr = '' + raise RuntimeError( ('Command:\n{cmdline}\nStandard output:\n{stdout}\n' 'Standard error:\n{stderr}\nReturn code: {returncode}' - ).format(**runtime.dictcopy())) + ).format(cmdline=runtime.cmdline, stdout=stdout, + stderr=stderr, returncode=runtime.returncode)) def _get_environ(self): return getattr(self.inputs, 'environ', {}) diff --git a/nipype/interfaces/base/tests/test_core.py b/nipype/interfaces/base/tests/test_core.py index 392d38706f..db63c78062 100644 --- a/nipype/interfaces/base/tests/test_core.py +++ b/nipype/interfaces/base/tests/test_core.py @@ -2,10 +2,11 @@ # emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*- # vi: set ft=python sts=4 ts=4 sw=4 et: from __future__ import print_function, unicode_literals -from future import standard_library -from builtins import open import os +from builtins import open import simplejson as json +from future import standard_library +from IPython.utils.io import capture_output import pytest @@ -406,59 +407,20 @@ def test_CommandLine_output(tmpdir): file.write('123456\n') name = os.path.basename(file.strpath) + # Default: output written to stdout file. ci = nib.CommandLine(command='ls -l') - ci.terminal_output = 'allatonce' - res = ci.run() - assert res.runtime.merged == '' - assert name in res.runtime.stdout - - # Check stdout is written - ci = nib.CommandLine(command='ls -l') - ci.terminal_output = 'file_stdout' - res = ci.run() - assert os.path.isfile('stdout.nipype') - assert name in res.runtime.stdout - tmpdir.join('stdout.nipype').remove(ignore_errors=True) - - # Check stderr is written - ci = nib.CommandLine(command='ls -l') - ci.terminal_output = 'file_stderr' + ci.terminal_output = 'default' res = ci.run() - assert os.path.isfile('stderr.nipype') - tmpdir.join('stderr.nipype').remove(ignore_errors=True) + with open(res.runtime.stdout) as f: + stdout = f.read().strip() + assert name in stdout - # Check outputs are thrown away - ci = nib.CommandLine(command='ls -l') - ci.terminal_output = 'none' - res = ci.run() - assert res.runtime.stdout == '' and \ - res.runtime.stderr == '' and \ - res.runtime.merged == '' - - # Check that new interfaces are set to default 'stream' - ci = nib.CommandLine(command='ls -l') - res = ci.run() - assert ci.terminal_output == 'stream' - assert name in res.runtime.stdout and \ - res.runtime.stderr == '' + # Test streamed output + with capture_output() as captured: + ci.terminal_output = 'stream' + res = ci.run() - # Check only one file is generated - ci = nib.CommandLine(command='ls -l') - ci.terminal_output = 'file' - res = ci.run() - assert os.path.isfile('output.nipype') - assert name in res.runtime.merged and \ - res.runtime.stdout == '' and \ - res.runtime.stderr == '' - tmpdir.join('output.nipype').remove(ignore_errors=True) - - # Check split files are generated - ci = nib.CommandLine(command='ls -l') - ci.terminal_output = 'file_split' - res = ci.run() - assert os.path.isfile('stdout.nipype') - assert os.path.isfile('stderr.nipype') - assert name in res.runtime.stdout + assert name in captured.stdout def test_global_CommandLine_output(tmpdir): @@ -471,9 +433,9 @@ def test_global_CommandLine_output(tmpdir): ci = BET() assert ci.terminal_output == 'stream' # default case - nib.CommandLine.set_default_terminal_output('allatonce') + nib.CommandLine.set_default_terminal_output('none') ci = nib.CommandLine(command='ls -l') - assert ci.terminal_output == 'allatonce' + assert ci.terminal_output == 'none' nib.CommandLine.set_default_terminal_output('file') ci = nib.CommandLine(command='ls -l') diff --git a/nipype/interfaces/diffusion_toolkit/base.py b/nipype/interfaces/diffusion_toolkit/base.py index c8e3a17c61..e7c50307f5 100644 --- a/nipype/interfaces/diffusion_toolkit/base.py +++ b/nipype/interfaces/diffusion_toolkit/base.py @@ -49,12 +49,13 @@ def version(): """ clout = CommandLine( - command='dti_recon', terminal_output='allatonce').run() + command='dti_recon', terminal_output='default').run() if clout.runtime.returncode is not 0: return None - dtirecon = clout.runtime.stdout + with open(clout.runtime.stdout, 'rt') as f: + dtirecon = f.read() result = re.search('dti_recon (.*)\n', dtirecon) version = result.group(0).split()[1] return version diff --git a/nipype/interfaces/dtitk/registration.py b/nipype/interfaces/dtitk/registration.py index 6aa40d4201..3298b59788 100644 --- a/nipype/interfaces/dtitk/registration.py +++ b/nipype/interfaces/dtitk/registration.py @@ -81,10 +81,15 @@ class Rigid(CommandLineDtitk): value = 1 return super(Rigid, self)._format_arg(name, spec, value)''' - def _run_interface(self, runtime): - runtime = super(Rigid, self)._run_interface(runtime) - if '''.aff doesn't exist or can't be opened''' in runtime.stderr: - self.raise_exception(runtime) + def _run_interface(self, runtime, correct_return_codes=(0, )): + runtime = super(Rigid, self)._run_interface( + runtime, correct_return_codes=correct_return_codes) + + with open(runtime.stderr) as stderrfh: + stderr = stderrfh.read() + + if ".aff doesn't exist or can't be opened" in stderr: + runtime.returncode = 1 return runtime def _list_outputs(self): diff --git a/nipype/interfaces/dynamic_slicer.py b/nipype/interfaces/dynamic_slicer.py index 5d3a3c1899..eebd939617 100644 --- a/nipype/interfaces/dynamic_slicer.py +++ b/nipype/interfaces/dynamic_slicer.py @@ -28,13 +28,16 @@ class SlicerCommandLine(CommandLine): def _grab_xml(self, module): cmd = CommandLine( command="Slicer3", + terminal_output='default', resource_monitor=False, args="--launch %s --xml" % module) ret = cmd.run() if ret.runtime.returncode == 0: - return xml.dom.minidom.parseString(ret.runtime.stdout) + with open(ret.runtime.stdout) as f: + stdout = f.read() + return xml.dom.minidom.parseString(stdout) else: - raise Exception(cmd.cmdline + " failed:\n%s" % ret.runtime.stderr) + self.raise_exception(ret.runtime) def _outputs(self): base = super(SlicerCommandLine, self)._outputs() @@ -221,5 +224,4 @@ def _format_arg(self, name, spec, value): # test.inputs.warpTransform = "/home/filo/workspace/nipype/nipype/interfaces/outputTransform.mat" # print test.cmdline # ret = test.run() -# print ret.runtime.stderr # print ret.runtime.returncode diff --git a/nipype/interfaces/freesurfer/utils.py b/nipype/interfaces/freesurfer/utils.py index b258d41720..1c9f8b9955 100644 --- a/nipype/interfaces/freesurfer/utils.py +++ b/nipype/interfaces/freesurfer/utils.py @@ -919,7 +919,7 @@ def _format_arg(self, name, spec, value): return "-annotation %s" % value return super(SurfaceSnapshots, self)._format_arg(name, spec, value) - def _run_interface(self, runtime): + def _run_interface(self, runtime, correct_return_codes=(0, )): if not isdefined(self.inputs.screenshot_stem): stem = "%s_%s_%s" % (self.inputs.subject_id, self.inputs.hemi, self.inputs.surface) @@ -935,7 +935,8 @@ def _run_interface(self, runtime): "Graphics are not enabled -- cannot run tksurfer") runtime.environ["_SNAPSHOT_STEM"] = stem self._write_tcl_script() - runtime = super(SurfaceSnapshots, self)._run_interface(runtime) + runtime = super(SurfaceSnapshots, self)._run_interface( + runtime, correct_return_codes=correct_return_codes) # If a display window can't be opened, this will crash on # aggregate_outputs. Let's try to parse stderr and raise a # better exception here if that happened. @@ -943,9 +944,16 @@ def _run_interface(self, runtime): "surfer: failed, no suitable display found", "Fatal Error in tksurfer.bin: could not open display" ] + + with open(runtime.stderr) as stderrfh: + stderr = stderrfh.read() + for err in errors: - if err in runtime.stderr: + if err in stderr: + if runtime.returncode == 0: + runtime.returncode = 1 self.raise_exception(runtime) + # Tksurfer always (or at least always when you run a tcl script) # exits with a nonzero returncode. We have to force it to 0 here. runtime.returncode = 0 @@ -1622,12 +1630,15 @@ def _gen_outfilename(self): _, name, ext = split_filename(self.inputs.in_file) return os.path.abspath(name + '_smoothed' + ext) - def _run_interface(self, runtime): - # The returncode is meaningless in BET. So check the output - # in stderr and if it's set, then update the returncode - # accordingly. - runtime = super(SmoothTessellation, self)._run_interface(runtime) - if "failed" in runtime.stderr: + def _run_interface(self, runtime, correct_return_codes=(0, )): + runtime = super(SmoothTessellation, self)._run_interface( + runtime, correct_return_codes=correct_return_codes) + + with open(runtime.stderr) as stderrfh: + stderr = stderrfh.read() + + if "failed" in stderr: + runtime.returncode = 1 self.raise_exception(runtime) return runtime diff --git a/nipype/interfaces/fsl/dti.py b/nipype/interfaces/fsl/dti.py index c842ff05cf..a16c9eb148 100644 --- a/nipype/interfaces/fsl/dti.py +++ b/nipype/interfaces/fsl/dti.py @@ -279,11 +279,16 @@ class FSLXCommand(FSLCommand): input_spec = FSLXCommandInputSpec output_spec = FSLXCommandOutputSpec - def _run_interface(self, runtime): - self._out_dir = os.getcwd() - runtime = super(FSLXCommand, self)._run_interface(runtime) - if runtime.stderr: - self.raise_exception(runtime) + def _run_interface(self, runtime, correct_return_codes=(0, )): + self._out_dir = runtime.cwd + runtime = super(FSLXCommand, self)._run_interface( + runtime, correct_return_codes=correct_return_codes) + + with open(runtime.stderr) as stderrfh: + stderr = stderrfh.read() + + if stderr.strip(): + runtime.returncode = 1 return runtime def _list_outputs(self, out_dir=None): @@ -755,7 +760,7 @@ def __init__(self, **inputs): "instead"), DeprecationWarning) return super(ProbTrackX, self).__init__(**inputs) - def _run_interface(self, runtime): + def _run_interface(self, runtime, correct_return_codes=(0, )): for i in range(1, len(self.inputs.thsamples) + 1): _, _, ext = split_filename(self.inputs.thsamples[i - 1]) copyfile( @@ -787,9 +792,15 @@ def _run_interface(self, runtime): f.write("%s\n" % seed) f.close() - runtime = super(ProbTrackX, self)._run_interface(runtime) - if runtime.stderr: - self.raise_exception(runtime) + runtime = super(ProbTrackX, self)._run_interface( + runtime, correct_return_codes=correct_return_codes) + + with open(runtime.stderr) as stderrfh: + stderr = stderrfh.read() + + if stderr.strip(): + runtime.returncode = 1 + return runtime def _format_arg(self, name, spec, value): diff --git a/nipype/interfaces/fsl/epi.py b/nipype/interfaces/fsl/epi.py index 84bd9e8dba..d99d1913e9 100644 --- a/nipype/interfaces/fsl/epi.py +++ b/nipype/interfaces/fsl/epi.py @@ -1111,10 +1111,15 @@ def __init__(self, **inputs): DeprecationWarning) return super(EPIDeWarp, self).__init__(**inputs) - def _run_interface(self, runtime): - runtime = super(EPIDeWarp, self)._run_interface(runtime) - if runtime.stderr: - self.raise_exception(runtime) + def _run_interface(self, runtime, correct_return_codes=(0, )): + runtime = super(EPIDeWarp, self)._run_interface( + runtime, correct_return_codes=correct_return_codes) + + with open(runtime.stderr) as stderrfh: + stderr = stderrfh.read() + + if stderr.strip(): + runtime.returncode = 1 return runtime def _gen_filename(self, name): @@ -1209,8 +1214,13 @@ def __init__(self, **inputs): "instead"), DeprecationWarning) return super(EddyCorrect, self).__init__(**inputs) - def _run_interface(self, runtime): - runtime = super(EddyCorrect, self)._run_interface(runtime) - if runtime.stderr: - self.raise_exception(runtime) + def _run_interface(self, runtime, correct_return_codes=(0, )): + runtime = super(EddyCorrect, self)._run_interface( + runtime, correct_return_codes=correct_return_codes) + + with open(runtime.stderr) as stderrfh: + stderr = stderrfh.read() + + if stderr.strip(): + runtime.returncode = 1 return runtime diff --git a/nipype/interfaces/fsl/model.py b/nipype/interfaces/fsl/model.py index 113f785120..7a441d447e 100644 --- a/nipype/interfaces/fsl/model.py +++ b/nipype/interfaces/fsl/model.py @@ -1216,13 +1216,18 @@ class ContrastMgr(FSLCommand): input_spec = ContrastMgrInputSpec output_spec = ContrastMgrOutputSpec - def _run_interface(self, runtime): + def _run_interface(self, runtime, correct_return_codes=(0, )): # The returncode is meaningless in ContrastMgr. So check the output # in stderr and if it's set, then update the returncode # accordingly. - runtime = super(ContrastMgr, self)._run_interface(runtime) - if runtime.stderr: - self.raise_exception(runtime) + runtime = super(ContrastMgr, self)._run_interface( + runtime, correct_return_codes=correct_return_codes) + + with open(runtime.stderr) as stderrfh: + stderr = stderrfh.read() + + if stderr.strip(): + runtime.returncode = 1 return runtime def _format_arg(self, name, trait_spec, value): diff --git a/nipype/interfaces/fsl/preprocess.py b/nipype/interfaces/fsl/preprocess.py index da06a5c574..e927298a3e 100644 --- a/nipype/interfaces/fsl/preprocess.py +++ b/nipype/interfaces/fsl/preprocess.py @@ -135,13 +135,18 @@ class BET(FSLCommand): input_spec = BETInputSpec output_spec = BETOutputSpec - def _run_interface(self, runtime): + def _run_interface(self, runtime, correct_return_codes=(0, )): # The returncode is meaningless in BET. So check the output # in stderr and if it's set, then update the returncode # accordingly. - runtime = super(BET, self)._run_interface(runtime) - if runtime.stderr: - self.raise_exception(runtime) + runtime = super(BET, self)._run_interface( + runtime, correct_return_codes=correct_return_codes) + + with open(runtime.stderr) as stderrfh: + stderr = stderrfh.read() + + if stderr.strip(): + runtime.returncode = 1 return runtime def _gen_outfilename(self): diff --git a/nipype/interfaces/matlab.py b/nipype/interfaces/matlab.py index fed7bfeb57..88f1aa29a2 100644 --- a/nipype/interfaces/matlab.py +++ b/nipype/interfaces/matlab.py @@ -26,8 +26,9 @@ def get_matlab_command(): command='which', args=matlab_cmd, resource_monitor=False, - terminal_output='allatonce').run() - matlab_path = res.runtime.stdout.strip() + terminal_output='default').run() + with open(res.runtime.stdout, 'rt') as f: + matlab_path = f.read().strip() except Exception: return None return matlab_cmd @@ -120,9 +121,6 @@ def __init__(self, matlab_cmd=None, **inputs): not isdefined(self.inputs.uses_mcr): if config.getboolean('execution', 'single_thread_matlab'): self.inputs.single_comp_thread = True - # For matlab commands force all output to be returned since matlab - # does not have a clean way of notifying an error - self.terminal_output = 'allatonce' @classmethod def set_default_matlab_cmd(cls, matlab_cmd): @@ -157,17 +155,22 @@ def set_default_paths(cls, paths): """ cls._default_paths = paths - def _run_interface(self, runtime): - self.terminal_output = 'allatonce' - runtime = super(MatlabCommand, self)._run_interface(runtime) + def _run_interface(self, runtime, correct_return_codes=(0, )): + runtime = super(MatlabCommand, self)._run_interface( + runtime, correct_return_codes=correct_return_codes) try: # Matlab can leave the terminal in a barbbled state os.system('stty sane') except: # We might be on a system where stty doesn't exist pass - if 'MATLAB code threw an exception' in runtime.stderr: - self.raise_exception(runtime) + + with open(runtime.stderr) as stderrfh: + stderr = stderrfh.read() + + if 'MATLAB code threw an exception' in stderr: + runtime.returncode = 1 + return runtime def _format_arg(self, name, trait_spec, value): diff --git a/nipype/interfaces/minc/base.py b/nipype/interfaces/minc/base.py index 67b7938176..61af5a23cf 100644 --- a/nipype/interfaces/minc/base.py +++ b/nipype/interfaces/minc/base.py @@ -57,11 +57,13 @@ def version(): clout = CommandLine( command='mincinfo', args='-version', - terminal_output='allatonce').run() + resource_monitor=False, + terminal_output='default').run() except IOError: return None - out = clout.runtime.stdout + with open(clout.runtime.stdout, 'rt') as f: + out = f.read() def read_program_version(s): if 'program' in s: diff --git a/nipype/interfaces/slicer/generate_classes.py b/nipype/interfaces/slicer/generate_classes.py index 6fe3ae927f..2d1037cb9e 100644 --- a/nipype/interfaces/slicer/generate_classes.py +++ b/nipype/interfaces/slicer/generate_classes.py @@ -433,13 +433,6 @@ def grab_xml(module, launcher, mipav_hacks=False): raise e return dom - -# if ret.runtime.returncode == 0: -# return xml.dom.minidom.parseString(ret.runtime.stdout) -# else: -# raise Exception(cmd.cmdline + " failed:\n%s"%ret.runtime.stderr) - - def parse_params(params): list = [] for key, value in params.items(): diff --git a/nipype/interfaces/spm/base.py b/nipype/interfaces/spm/base.py index 214a6e7a2f..8555661c19 100644 --- a/nipype/interfaces/spm/base.py +++ b/nipype/interfaces/spm/base.py @@ -370,18 +370,19 @@ def _check_mlab_inputs(self): if not isdefined(self.inputs.use_mcr) and self._use_mcr: self.inputs.use_mcr = self._use_mcr - def _run_interface(self, runtime): + def _run_interface(self, runtime, correct_return_codes=(0,)): """Executes the SPM function using MATLAB.""" self.mlab.inputs.script = self._make_matlab_command( deepcopy(self._parse_inputs())) results = self.mlab.run() runtime.returncode = results.runtime.returncode if self.mlab.inputs.uses_mcr: - if 'Skipped' in results.runtime.stdout: - self.raise_exception(runtime) - runtime.stdout = results.runtime.stdout - runtime.stderr = results.runtime.stderr - runtime.merged = results.runtime.merged + with open(runtime.stdout) as stdoutfh: + stdout = stdoutfh.read() + + if 'Skipped' in stdout: + runtime.returncode = 1 + return runtime def _list_outputs(self): diff --git a/nipype/pipeline/engine/nodes.py b/nipype/pipeline/engine/nodes.py index af93fd140b..35eb0fabf5 100644 --- a/nipype/pipeline/engine/nodes.py +++ b/nipype/pipeline/engine/nodes.py @@ -622,8 +622,9 @@ def _run_command(self, execute, copyfiles=True): with indirectory(outdir): cmd = self._interface.cmdline except Exception as msg: - result.runtime.stderr = '{}\n\n{}'.format( - getattr(result.runtime, 'stderr', ''), msg) + # append cmdline exception to errorlog + with open(result.runtime.stderr, 'wt+') as f: + f.write('\n\n%s' % msg) _save_resultfile(result, outdir, self.name) raise cmdfile = op.join(outdir, 'command.txt') @@ -634,8 +635,9 @@ def _run_command(self, execute, copyfiles=True): try: result = self._interface.run(cwd=outdir) except Exception as msg: - result.runtime.stderr = '%s\n\n%s'.format( - getattr(result.runtime, 'stderr', ''), msg) + # append cmdline exception to errorlog + with open(result.runtime.stderr, 'wt+') as f: + f.write('\n\n%s' % msg) _save_resultfile(result, outdir, self.name) raise diff --git a/nipype/pipeline/engine/utils.py b/nipype/pipeline/engine/utils.py index 672e482959..e454be57d5 100644 --- a/nipype/pipeline/engine/utils.py +++ b/nipype/pipeline/engine/utils.py @@ -205,21 +205,24 @@ def write_report(node, report_type=None, is_mapnode=False): lines.append(write_rst_dict(rst_dict)) # Collect terminal output - if hasattr(result.runtime, 'merged'): - lines += [ - write_rst_header('Terminal output', level=2), - write_rst_list(result.runtime.merged), - ] if hasattr(result.runtime, 'stdout'): - lines += [ - write_rst_header('Terminal - standard output', level=2), - write_rst_list(result.runtime.stdout), - ] + with open(result.runtime.stdout) as f: + stdout = f.read().strip() + + if stdout: + lines += [ + write_rst_header('Terminal - standard output', level=2), + write_rst_list(stdout), + ] if hasattr(result.runtime, 'stderr'): - lines += [ - write_rst_header('Terminal - standard error', level=2), - write_rst_list(result.runtime.stderr), - ] + with open(result.runtime.stderr) as f: + stderr = f.read().strip() + + if stderr: + lines += [ + write_rst_header('Terminal - standard error', level=2), + write_rst_list(stderr), + ] # Store environment if hasattr(result.runtime, 'environ'): @@ -1363,7 +1366,9 @@ def export_graph(graph_in, # Convert .dot if format != 'dot' outfname, res = _run_dot(out_dot, format_ext=format) if res is not None and res.runtime.returncode: - logger.warning('dot2png: %s', res.runtime.stderr) + with open(res.runtime.stderr, 'rt') as f: + errmsg = f.read() + logger.warning('dot2png: %s', errmsg) pklgraph = _create_dot_graph(graph, show_connectinfo, simple_form) simple_dot = fname_presuffix( @@ -1373,7 +1378,9 @@ def export_graph(graph_in, # Convert .dot if format != 'dot' simplefname, res = _run_dot(simple_dot, format_ext=format) if res is not None and res.runtime.returncode: - logger.warning('dot2png: %s', res.runtime.stderr) + with open(res.runtime.stderr, 'rt') as f: + errmsg = f.read() + logger.warning('dot2png: %s', errmsg) if show: pos = nx.graphviz_layout(pklgraph, prog='dot') @@ -1403,7 +1410,7 @@ def _run_dot(dotfilename, format_ext): dot_base = os.path.splitext(dotfilename)[0] formatted_dot = '{}.{}'.format(dot_base, format_ext) cmd = 'dot -T{} -o"{}" "{}"'.format(format_ext, formatted_dot, dotfilename) - res = CommandLine(cmd, terminal_output='allatonce', + res = CommandLine(cmd, terminal_output='default', resource_monitor=False).run() return formatted_dot, res diff --git a/nipype/pipeline/plugins/condor.py b/nipype/pipeline/plugins/condor.py index 9f5ca632e5..3c136c7886 100644 --- a/nipype/pipeline/plugins/condor.py +++ b/nipype/pipeline/plugins/condor.py @@ -49,14 +49,16 @@ def __init__(self, **kwargs): def _is_pending(self, taskid): cmd = CommandLine( - 'condor_q', resource_monitor=False, terminal_output='allatonce') + 'condor_q', resource_monitor=False, terminal_output='default') cmd.inputs.args = '%d' % taskid # check condor cluster oldlevel = iflogger.level iflogger.setLevel(logging.getLevelName('CRITICAL')) result = cmd.run(ignore_exception=True) iflogger.setLevel(oldlevel) - if result.runtime.stdout.count('\n%d' % taskid): + with open(result.runtime.stdout, 'rt') as f: + stdout = f.read() + if stdout.count('\n%d' % taskid): return True return False @@ -65,7 +67,7 @@ def _submit_batchtask(self, scriptfile, node): 'condor_qsub', environ=dict(os.environ), resource_monitor=False, - terminal_output='allatonce') + terminal_output='default') path = os.path.dirname(scriptfile) qsubargs = '' if self._qsub_args: @@ -111,7 +113,9 @@ def _submit_batchtask(self, scriptfile, node): break iflogger.setLevel(oldlevel) # retrieve condor clusterid - taskid = int(result.runtime.stdout.split(' ')[2]) + with open(result.runtime.stdout, 'rt') as f: + stdout = f.read() + taskid = int(stdout.split(' ')[2]) self._pending[taskid] = node.output_dir() logger.debug('submitted condor cluster: %d for node %s' % (taskid, node._id)) diff --git a/nipype/pipeline/plugins/dagman.py b/nipype/pipeline/plugins/dagman.py index 28b766f2ea..237b040ea8 100644 --- a/nipype/pipeline/plugins/dagman.py +++ b/nipype/pipeline/plugins/dagman.py @@ -159,7 +159,7 @@ def _submit_graph(self, pyfiles, dependencies, nodes): 'condor_submit_dag', environ=dict(os.environ), resource_monitor=False, - terminal_output='allatonce') + terminal_output='default') # needs -update_submit or re-running a workflow will fail cmd.inputs.args = '%s -update_submit %s' % (self._dagman_args, dagfilename) diff --git a/nipype/pipeline/plugins/legacymultiproc.py b/nipype/pipeline/plugins/legacymultiproc.py index bfc1773a92..3affff805e 100644 --- a/nipype/pipeline/plugins/legacymultiproc.py +++ b/nipype/pipeline/plugins/legacymultiproc.py @@ -81,7 +81,7 @@ class NonDaemonMixin(object): @property def daemon(self): return False - + @daemon.setter def daemon(self, val): pass @@ -227,7 +227,7 @@ def _submit_job(self, node, updatehash=False): # Don't allow streaming outputs if getattr(node.interface, 'terminal_output', '') == 'stream': - node.interface.terminal_output = 'allatonce' + node.interface.terminal_output = 'default' self._task_obj[self._taskid] = self.pool.apply_async( run_node, (node, updatehash, self._taskid), diff --git a/nipype/pipeline/plugins/lsf.py b/nipype/pipeline/plugins/lsf.py index bdaabc31e6..79ac735aed 100644 --- a/nipype/pipeline/plugins/lsf.py +++ b/nipype/pipeline/plugins/lsf.py @@ -49,7 +49,7 @@ def _is_pending(self, taskid): ready to be checked for completeness. So return True if status is either 'PEND' or 'RUN'""" cmd = CommandLine( - 'bjobs', resource_monitor=False, terminal_output='allatonce') + 'bjobs', resource_monitor=False, terminal_output='default') cmd.inputs.args = '%d' % taskid # check lsf task oldlevel = iflogger.level @@ -57,17 +57,16 @@ def _is_pending(self, taskid): result = cmd.run(ignore_exception=True) iflogger.setLevel(oldlevel) # logger.debug(result.runtime.stdout) - if 'DONE' in result.runtime.stdout or 'EXIT' in result.runtime.stdout: - return False - else: - return True + with open(result.runtime.stdout, 'rt') as f: + stdout = f.read() + return 'DONE' not in stdout and 'EXIT' not in stdout def _submit_batchtask(self, scriptfile, node): cmd = CommandLine( 'bsub', environ=dict(os.environ), resource_monitor=False, - terminal_output='allatonce') + terminal_output='default') bsubargs = '' if self._bsub_args: bsubargs = self._bsub_args @@ -113,12 +112,14 @@ def _submit_batchtask(self, scriptfile, node): break iflogger.setLevel(oldlevel) # retrieve lsf taskid - match = re.search('<(\d*)>', result.runtime.stdout) + with open(result.runtime.stdout, 'rt') as f: + stdout = f.read() + match = re.search(r'<(\d*)>', stdout) if match: taskid = int(match.groups()[0]) else: raise ScriptError("Can't parse submission job output id: %s" % - result.runtime.stdout) + stdout) self._pending[taskid] = node.output_dir() logger.debug('submitted lsf task: %d for node %s' % (taskid, node._id)) return taskid diff --git a/nipype/pipeline/plugins/multiproc.py b/nipype/pipeline/plugins/multiproc.py index d2ef363a34..80e2dae0c5 100644 --- a/nipype/pipeline/plugins/multiproc.py +++ b/nipype/pipeline/plugins/multiproc.py @@ -162,7 +162,7 @@ def _submit_job(self, node, updatehash=False): # Don't allow streaming outputs if getattr(node.interface, 'terminal_output', '') == 'stream': - node.interface.terminal_output = 'allatonce' + node.interface.terminal_output = 'default' result_future = self.pool.submit(run_node, node, updatehash, self._taskid) result_future.add_done_callback(self._async_callback) diff --git a/nipype/pipeline/plugins/oar.py b/nipype/pipeline/plugins/oar.py index c68b42379f..84d1b88aae 100644 --- a/nipype/pipeline/plugins/oar.py +++ b/nipype/pipeline/plugins/oar.py @@ -70,7 +70,7 @@ def _submit_batchtask(self, scriptfile, node): 'oarsub', environ=dict(os.environ), resource_monitor=False, - terminal_output='allatonce') + terminal_output='default') path = os.path.dirname(scriptfile) oarsubargs = '' if self._oarsub_args: @@ -127,7 +127,9 @@ def _submit_batchtask(self, scriptfile, node): o = '' add = False - for line in result.runtime.stdout.splitlines(): + with open(result.runtime.stdout, 'rt') as f: + stdout = f.read() + for line in stdout.splitlines(): if line.strip().startswith('{'): add = True if add: diff --git a/nipype/pipeline/plugins/pbs.py b/nipype/pipeline/plugins/pbs.py index 0738638765..c32e0eb54d 100644 --- a/nipype/pipeline/plugins/pbs.py +++ b/nipype/pipeline/plugins/pbs.py @@ -51,12 +51,13 @@ def __init__(self, **kwargs): def _is_pending(self, taskid): result = CommandLine('qstat -f {}'.format(taskid), environ=dict(os.environ), - terminal_output='file_split', resource_monitor=False, ignore_exception=True).run() - stdout = result.runtime.stdout - stderr = result.runtime.stderr + with open(result.runtime.stdout, 'rt') as f: + stdout = f.read() + with open(result.runtime.stderr, 'rt') as f: + stderr = f.read() errmsg = 'Unknown Job Id' success = 'Job has finished' if (success in stderr) or ('job_state = C' in stdout): @@ -69,7 +70,7 @@ def _submit_batchtask(self, scriptfile, node): 'qsub', environ=dict(os.environ), resource_monitor=False, - terminal_output='allatonce') + terminal_output='default') path = os.path.dirname(scriptfile) qsubargs = '' if self._qsub_args: @@ -115,7 +116,9 @@ def _submit_batchtask(self, scriptfile, node): break iflogger.setLevel(oldlevel) # retrieve pbs taskid - taskid = result.runtime.stdout.split('.')[0] + with open(result.runtime.stdout, 'rt') as f: + stdout = f.read() + taskid = stdout.split('.')[0] self._pending[taskid] = node.output_dir() logger.debug('submitted pbs task: {} for node {}'.format( taskid, node._id)) diff --git a/nipype/pipeline/plugins/pbsgraph.py b/nipype/pipeline/plugins/pbsgraph.py index 68fc651f5f..4e557a86ec 100644 --- a/nipype/pipeline/plugins/pbsgraph.py +++ b/nipype/pipeline/plugins/pbsgraph.py @@ -59,7 +59,7 @@ def _submit_graph(self, pyfiles, dependencies, nodes): 'sh', environ=dict(os.environ), resource_monitor=False, - terminal_output='allatonce') + terminal_output='default') cmd.inputs.args = '%s' % submitjobsfile cmd.run() logger.info('submitted all jobs to queue') diff --git a/nipype/pipeline/plugins/sge.py b/nipype/pipeline/plugins/sge.py index a4ce28297c..fbca11996c 100644 --- a/nipype/pipeline/plugins/sge.py +++ b/nipype/pipeline/plugins/sge.py @@ -393,7 +393,7 @@ def _submit_batchtask(self, scriptfile, node): 'qsub', environ=dict(os.environ), resource_monitor=False, - terminal_output='allatonce') + terminal_output='default') path = os.path.dirname(scriptfile) qsubargs = '' if self._qsub_args: @@ -439,7 +439,9 @@ def _submit_batchtask(self, scriptfile, node): break iflogger.setLevel(oldlevel) # retrieve sge taskid - lines = [line for line in result.runtime.stdout.split('\n') if line] + with open(result.runtime.stdout, 'rt') as f: + stdout = f.read() + lines = [line.strip() for line in stdout.splitlines() if line.strip()] taskid = int( re.match("Your job ([0-9]*) .* has been submitted", lines[-1]).groups()[0]) diff --git a/nipype/pipeline/plugins/sgegraph.py b/nipype/pipeline/plugins/sgegraph.py index fa07d6a436..7006b65ff4 100644 --- a/nipype/pipeline/plugins/sgegraph.py +++ b/nipype/pipeline/plugins/sgegraph.py @@ -156,7 +156,7 @@ def make_job_name(jobnumber, nodeslist): 'bash', environ=dict(os.environ), resource_monitor=False, - terminal_output='allatonce') + terminal_output='default') cmd.inputs.args = '%s' % submitjobsfile cmd.run() logger.info('submitted all jobs to queue') diff --git a/nipype/pipeline/plugins/slurm.py b/nipype/pipeline/plugins/slurm.py index 4645e52fba..c1b84dad7a 100644 --- a/nipype/pipeline/plugins/slurm.py +++ b/nipype/pipeline/plugins/slurm.py @@ -67,8 +67,10 @@ def _is_pending(self, taskid): 'squeue', args=' '.join(['-j', '%s' % taskid]), resource_monitor=False, - terminal_output='allatonce').run() - return res.runtime.stdout.find(str(taskid)) > -1 + terminal_output='default').run() + with open(result.runtime.stdout, 'rt') as f: + stdout = f.read() + return stdout.find(str(taskid)) > -1 except RuntimeError as e: if any(ss in str(e) for ss in ['Socket timed out', 'not available at the moment']): @@ -92,7 +94,7 @@ def _submit_batchtask(self, scriptfile, node): 'sbatch', environ=dict(os.environ), resource_monitor=False, - terminal_output='allatonce') + terminal_output='default') path = os.path.dirname(scriptfile) sbatch_args = '' @@ -140,7 +142,9 @@ def _submit_batchtask(self, scriptfile, node): logger.debug('Ran command ({0})'.format(cmd.cmdline)) iflogger.setLevel(oldlevel) # retrieve taskid - lines = [line for line in result.runtime.stdout.split('\n') if line] + with open(result.runtime.stdout, 'rt') as f: + stdout = f.read() + lines = [line.strip() for line in stdout.splitlines() if line.strip()] taskid = int(re.match(self._jobid_re, lines[-1]).groups()[0]) self._pending[taskid] = node.output_dir() logger.debug('submitted sbatch task: %d for node %s' % (taskid, diff --git a/nipype/pipeline/plugins/slurmgraph.py b/nipype/pipeline/plugins/slurmgraph.py index b4013163cb..49b6d28de8 100644 --- a/nipype/pipeline/plugins/slurmgraph.py +++ b/nipype/pipeline/plugins/slurmgraph.py @@ -156,7 +156,7 @@ def make_job_name(jobnumber, nodeslist): 'bash', environ=dict(os.environ), resource_monitor=False, - terminal_output='allatonce') + terminal_output='default') cmd.inputs.args = '%s' % submitjobsfile cmd.run() logger.info('submitted all jobs to queue') diff --git a/nipype/utils/docparse.py b/nipype/utils/docparse.py index 1df779f2ce..7a7a0e8698 100644 --- a/nipype/utils/docparse.py +++ b/nipype/utils/docparse.py @@ -255,8 +255,10 @@ def get_doc(cmd, opt_map, help_flag=None, trap_error=True): res = CommandLine( 'which %s' % cmd.split(' ')[0], resource_monitor=False, - terminal_output='allatonce').run() - cmd_path = res.runtime.stdout.strip() + terminal_output='default').run() + with open(res.runtime.stdout, 'rt') as f: + stdout = f.read() + cmd_path = stdout.strip() if cmd_path == '': raise Exception('Command %s not found' % cmd.split(' ')[0]) if help_flag: @@ -335,8 +337,10 @@ def get_params_from_doc(cmd, style='--', help_flag=None, trap_error=True): res = CommandLine( 'which %s' % cmd.split(' ')[0], resource_monitor=False, - terminal_output='allatonce').run() - cmd_path = res.runtime.stdout.strip() + terminal_output='default').run() + with open(res.runtime.stdout, 'rt') as f: + stdout = f.read() + cmd_path = stdout.strip() if cmd_path == '': raise Exception('Command %s not found' % cmd.split(' ')[0]) if help_flag: From 283769a3cc988da7f4178aa9a4e3cb84fc6817a9 Mon Sep 17 00:00:00 2001 From: oesteban Date: Wed, 21 Nov 2018 17:32:14 -0800 Subject: [PATCH 05/42] fix missing terminal_output in pbs plugin --- nipype/pipeline/plugins/pbs.py | 1 + 1 file changed, 1 insertion(+) diff --git a/nipype/pipeline/plugins/pbs.py b/nipype/pipeline/plugins/pbs.py index c32e0eb54d..9a87ebdf5a 100644 --- a/nipype/pipeline/plugins/pbs.py +++ b/nipype/pipeline/plugins/pbs.py @@ -51,6 +51,7 @@ def __init__(self, **kwargs): def _is_pending(self, taskid): result = CommandLine('qstat -f {}'.format(taskid), environ=dict(os.environ), + terminal_output='default', resource_monitor=False, ignore_exception=True).run() From 18a9e733928bc30d4542701d766c1c9f663c0c57 Mon Sep 17 00:00:00 2001 From: oesteban Date: Wed, 21 Nov 2018 20:44:29 -0800 Subject: [PATCH 06/42] fixing tests --- nipype/interfaces/afni/utils.py | 8 +++- nipype/interfaces/ants/registration.py | 6 ++- nipype/interfaces/base/core.py | 43 ++++++++++++++++++++-- nipype/interfaces/dcm2nii.py | 14 ++++--- nipype/interfaces/freesurfer/base.py | 6 +-- nipype/interfaces/freesurfer/utils.py | 3 +- nipype/interfaces/fsl/model.py | 3 +- nipype/interfaces/fsl/utils.py | 18 ++++++--- nipype/interfaces/niftyreg/base.py | 11 ++---- nipype/interfaces/niftyseg/base.py | 13 +++++-- nipype/interfaces/niftyseg/label_fusion.py | 6 ++- nipype/interfaces/niftyseg/stats.py | 6 ++- nipype/interfaces/spm/base.py | 4 +- nipype/interfaces/spm/model.py | 8 +++- nipype/utils/spm_docs.py | 4 +- 15 files changed, 109 insertions(+), 44 deletions(-) diff --git a/nipype/interfaces/afni/utils.py b/nipype/interfaces/afni/utils.py index 1a32cccefb..613494944e 100644 --- a/nipype/interfaces/afni/utils.py +++ b/nipype/interfaces/afni/utils.py @@ -299,7 +299,9 @@ def aggregate_outputs(self, runtime=None, needed_outputs=None): return self.run().outputs else: min_val = [] - for line in runtime.stdout.split('\n'): + with open(runtime.stdout, 'rt') as f: + stdout = f.read() + for line in stdout.splitlines(): if line: values = line.split() if len(values) > 1: @@ -3049,8 +3051,10 @@ class GCOR(CommandLine): def _run_interface(self, runtime): runtime = super(GCOR, self)._run_interface(runtime) + with open(runtime.stdout, 'rt') as f: + stdout = f.read() gcor_line = [ - line.strip() for line in runtime.stdout.split('\n') + line.strip() for line in stdout.splitlines() if line.strip().startswith('GCOR = ') ][-1] setattr(self, '_gcor', float(gcor_line[len('GCOR = '):])) diff --git a/nipype/interfaces/ants/registration.py b/nipype/interfaces/ants/registration.py index 185d4a9fe2..b9ee98d35d 100644 --- a/nipype/interfaces/ants/registration.py +++ b/nipype/interfaces/ants/registration.py @@ -1491,8 +1491,10 @@ def _format_arg(self, opt, spec, val): def aggregate_outputs(self, runtime=None, needed_outputs=None): outputs = self._outputs() - stdout = runtime.stdout.split('\n') - outputs.similarity = float(stdout[0]) + if runtime is not None and runtime.stdout: + with open(runtime.stdout, 'rf') as f: + stdout = f.read().splitlines()[0] + outputs.similarity = float(stdout[0]) return outputs diff --git a/nipype/interfaces/base/core.py b/nipype/interfaces/base/core.py index 9fdf9f5dfb..5852e97179 100644 --- a/nipype/interfaces/base/core.py +++ b/nipype/interfaces/base/core.py @@ -625,10 +625,12 @@ def aggregate_outputs(self, runtime=None, needed_outputs=None): @property def version(self): - if self._version is None: - if str2bool(config.get('execution', 'stop_on_unknown_version')): - raise ValueError('Interface %s has no version information' % - self.__class__.__name__) + if self._version: + return self._version + + if str2bool(config.get('execution', 'stop_on_unknown_version')): + raise ValueError('Interface %s has no version information' % + self.__class__.__name__) return self._version def load_inputs_from_json(self, json_file, overwrite=True): @@ -767,6 +769,7 @@ class must be instantiated with a command argument _cmd_prefix = '' _cmd = None _version = None + _version_cmd_flag = None _terminal_output = 'stream' @classmethod @@ -858,6 +861,38 @@ def raise_exception(self, runtime): def _get_environ(self): return getattr(self.inputs, 'environ', {}) + @property + def version(self): + if self._version: + return self._version + + if self._version_cmd_flag: + self._version = self._version_from_command( + flag=self._version_cmd_flag) + + if self._version is None: + if str2bool(config.get('execution', 'stop_on_unknown_version')): + raise ValueError('Interface %s has no version information' % + self.__class__.__name__) + return self._version + + def _version_from_command(self, flag='-v', cmd=None): + env = dict(os.environ) + env.update(self._get_environ()) + with TemporaryDirectory() as tmp_folder: + cmdline = ' '.join([cmd or self.cmd.split()[0], flag]) + rt = run_command( + Bunch(cmdline=cmdline, environ=env, + cwd=tmp_folder, shell=True)).runtime + + if rt.returncode != 0: + return None + + with open(rt.stdout, 'rt') as f: + ver_str = f.read() + + return ver_str + def _run_interface(self, runtime, correct_return_codes=(0, )): """Execute command via subprocess diff --git a/nipype/interfaces/dcm2nii.py b/nipype/interfaces/dcm2nii.py index 65771873a5..d608c0b0dc 100644 --- a/nipype/interfaces/dcm2nii.py +++ b/nipype/interfaces/dcm2nii.py @@ -147,7 +147,7 @@ def _run_interface(self, runtime): os.remove('config.ini') return new_runtime - def _parse_stdout(self, stdout): + def _parse_stdout(self, stdout_file): files = [] reoriented_files = [] reoriented_and_cropped_files = [] @@ -155,7 +155,9 @@ def _parse_stdout(self, stdout): bvals = [] skip = False last_added_file = None - for line in stdout.split("\n"): + with open(stdout_file, 'rt') as f: + stdout = f.read() + for line in stdout.splitlines(): if not skip: out_file = None if line.startswith("Saving "): @@ -374,7 +376,7 @@ class Dcm2niix(CommandLine): converts any files in the directory containing the files in the list. We also do not support nested filenames with this option. **Thus all files must have a common root directory.** - + >>> converter = Dcm2niix() >>> converter.inputs.source_names = ['functional_1.dcm', 'functional_2.dcm'] >>> converter.inputs.compression = 5 @@ -418,14 +420,16 @@ def _run_interface(self, runtime): runtime.stdout) return runtime - def _parse_stdout(self, stdout): + def _parse_stdout(self, stdout_file): files = [] bvecs = [] bvals = [] bids = [] skip = False find_b = False - for line in stdout.split("\n"): + with open(stdout_file, 'rt') as f: + stdout = f.read() + for line in stdout.splitlines(): if not skip: out_file = None if line.startswith("Convert "): # output diff --git a/nipype/interfaces/freesurfer/base.py b/nipype/interfaces/freesurfer/base.py index cda527a5ea..1903081185 100644 --- a/nipype/interfaces/freesurfer/base.py +++ b/nipype/interfaces/freesurfer/base.py @@ -279,8 +279,4 @@ def no_freesurfer(): """Checks if FreeSurfer is NOT installed used with skipif to skip tests that will fail if FreeSurfer is not installed""" - - if Info.version() is None: - return True - else: - return False + return Info.version() is None diff --git a/nipype/interfaces/freesurfer/utils.py b/nipype/interfaces/freesurfer/utils.py index 1c9f8b9955..03b50d79c8 100644 --- a/nipype/interfaces/freesurfer/utils.py +++ b/nipype/interfaces/freesurfer/utils.py @@ -1041,7 +1041,8 @@ def info_regexp(self, info, field, delim="\n"): def aggregate_outputs(self, runtime=None, needed_outputs=None): outputs = self._outputs() - info = runtime.stdout + with open(runtime.stdout, 'rt') as f: + info = f.read() outputs.info = info # Pulse sequence parameters diff --git a/nipype/interfaces/fsl/model.py b/nipype/interfaces/fsl/model.py index 7a441d447e..0301a28c9c 100644 --- a/nipype/interfaces/fsl/model.py +++ b/nipype/interfaces/fsl/model.py @@ -1827,7 +1827,8 @@ class SmoothEstimate(FSLCommand): def aggregate_outputs(self, runtime=None, needed_outputs=None): outputs = self._outputs() - stdout = runtime.stdout.split('\n') + with open(runtime.stdout, 'rt') as f: + stdout = f.read().splitlines() outputs.dlh = float(stdout[0].split()[1]) outputs.volume = int(stdout[1].split()[1]) outputs.resels = float(stdout[2].split()[1]) diff --git a/nipype/interfaces/fsl/utils.py b/nipype/interfaces/fsl/utils.py index f4ef73c0e9..747c8d9fbe 100644 --- a/nipype/interfaces/fsl/utils.py +++ b/nipype/interfaces/fsl/utils.py @@ -788,7 +788,9 @@ def aggregate_outputs(self, runtime=None, needed_outputs=None): return self.run().outputs else: out_stat = [] - for line in runtime.stdout.split('\n'): + with open(runtime.stdout, 'rt') as f: + stdout = f.read() + for line in stdout.splitlines(): if line: values = line.split() if len(values) > 1: @@ -863,7 +865,9 @@ def _run_interface(self, runtime): '(?P[0-9\.\ \n-]+)[\s\n]*' 'Backward\ half\ transform\ =[\s]*\n' '(?P[0-9\.\ \n-]+)[\s\n]*') - out = expr.search(runtime.stdout).groupdict() + with open(runtime.stdout, 'rt') as f: + stdout = f.read() + out = expr.search(stdout).groupdict() outputs = {} outputs['rotation_translation_matrix'] = [[ float(v) for v in r.strip().split(' ') @@ -2488,8 +2492,10 @@ def _run_interface(self, runtime): self._trk_to_coords(fname, out_file=tmpfile) runtime = super(WarpPoints, self)._run_interface(runtime) + with open(runtime.stdout, 'rt') as f: + stdout = f.read() newpoints = np.fromstring( - '\n'.join(runtime.stdout.split('\n')[1:]), sep=' ') + '\n'.join(stdout.splitlines()[1:]), sep=' ') if tmpfile is not None: try: @@ -2624,9 +2630,9 @@ class WarpPointsFromStd(CommandLine): output_spec = WarpPointsOutputSpec _cmd = 'std2imgcoord' - def _list_outputs(self): - outputs = self.output_spec().get() - outputs['out_file'] = op.abspath('stdout.nipype') + def aggregate_outputs(self, runtime=None, needed_outputs=None): + outputs = self._outputs() + outputs['out_file'] = runtime.stdout return outputs diff --git a/nipype/interfaces/niftyreg/base.py b/nipype/interfaces/niftyreg/base.py index bd8a280aa5..9b29c799ff 100644 --- a/nipype/interfaces/niftyreg/base.py +++ b/nipype/interfaces/niftyreg/base.py @@ -49,6 +49,7 @@ class NiftyRegCommand(CommandLine): """ _suffix = '_nr' _min_version = '1.5.30' + _version_cmd_flag = '-v' input_spec = NiftyRegCommandInputSpec @@ -56,7 +57,7 @@ def __init__(self, required_version=None, **inputs): self.num_threads = 1 super(NiftyRegCommand, self).__init__(**inputs) self.required_version = required_version - _version = self.version_from_command() + _version = self.version if _version: _version = _version.decode("utf-8") if self._min_version is not None and \ @@ -93,7 +94,7 @@ def _environ_update(self): self.inputs.omp_core_val = Undefined def check_version(self): - _version = self.version_from_command() + _version = self.version if not _version: raise Exception('Niftyreg not found') # Decoding to string: @@ -107,12 +108,8 @@ def check_version(self): err += '(%s != %s)' raise ValueError(err % (_version, self.required_version)) - @property - def version(self): - return self.version_from_command() - def exists(self): - return self.version_from_command() is not None + return self.version is not None def _format_arg(self, name, spec, value): if name == 'omp_core_val': diff --git a/nipype/interfaces/niftyseg/base.py b/nipype/interfaces/niftyseg/base.py index d68bbcc73b..8790c02255 100644 --- a/nipype/interfaces/niftyseg/base.py +++ b/nipype/interfaces/niftyseg/base.py @@ -27,10 +27,17 @@ class NiftySegCommand(NiftyFitCommand): """ _suffix = '_ns' _min_version = None + _version_cmd_flag = '--version' def __init__(self, **inputs): super(NiftySegCommand, self).__init__(**inputs) - def get_version(self): - return super(NiftySegCommand, self).version_from_command( - cmd='seg_EM', flag='--version') + @property + def version(self): + if self._version: + return self._version + + if self._version_cmd_flag: + self._version = self._version_from_command( + cmd='seg_EM') + return self._version diff --git a/nipype/interfaces/niftyseg/label_fusion.py b/nipype/interfaces/niftyseg/label_fusion.py index 1b0237d37c..484201796e 100644 --- a/nipype/interfaces/niftyseg/label_fusion.py +++ b/nipype/interfaces/niftyseg/label_fusion.py @@ -318,14 +318,16 @@ def aggregate_outputs(self, runtime=None, needed_outputs=None): outputs = self._outputs() # local caching for backward compatibility outfile = os.path.join(os.getcwd(), 'CalcTopNCC.json') - if runtime is None or not runtime.stdout: + if runtime is None: try: out_files = load_json(outfile)['files'] except IOError: return self.run().outputs else: out_files = [] - for line in runtime.stdout.split('\n'): + with open(runtime.stdout, 'rt') as f: + stdout = f.read() + for line in stdout.splitlines(): if line: values = line.split() if len(values) > 1: diff --git a/nipype/interfaces/niftyseg/stats.py b/nipype/interfaces/niftyseg/stats.py index 796e07410c..d0602a8801 100644 --- a/nipype/interfaces/niftyseg/stats.py +++ b/nipype/interfaces/niftyseg/stats.py @@ -55,9 +55,11 @@ class StatsCommand(NiftySegCommand): input_spec = StatsInput output_spec = StatsOutput - def _parse_stdout(self, stdout): + def _parse_stdout(self, stdout_file): out = [] - for string_line in stdout.split("\n"): + with open(stdout_file, 'rt') as f: + stdout = f.read() + for string_line in stdout.splitlines(): if string_line.startswith('#'): continue if len(string_line) <= 1: diff --git a/nipype/interfaces/spm/base.py b/nipype/interfaces/spm/base.py index 8555661c19..3735aab077 100644 --- a/nipype/interfaces/spm/base.py +++ b/nipype/interfaces/spm/base.py @@ -228,7 +228,9 @@ def getinfo(klass, matlab_cmd=None, paths=None, use_mcr=None): klass._paths = paths return None - out = sd._strip_header(out.runtime.stdout) + with open(out.runtime.stdout, 'rt') as f: + stdout = f.read() + out = sd._strip_header(stdout) out_dict = {} for part in out.split('|'): key, val = part.split(':') diff --git a/nipype/interfaces/spm/model.py b/nipype/interfaces/spm/model.py index 3e26ab6e2a..5c43d5e531 100644 --- a/nipype/interfaces/spm/model.py +++ b/nipype/interfaces/spm/model.py @@ -713,7 +713,9 @@ def aggregate_outputs(self, runtime=None): setattr(outputs, 'thresholded_map', self._gen_thresholded_map_filename()) setattr(outputs, 'pre_topo_fdr_map', self._gen_pre_topo_map_filename()) - for line in runtime.stdout.split('\n'): + with open(runtime.stdout, 'rt') as f: + stdout = f.read() + for line in stdout.splitlines(): if line.startswith("activation_forced = "): setattr(outputs, 'activation_forced', line[len("activation_forced = "):].strip() == "1") @@ -836,7 +838,9 @@ def _make_matlab_command(self, _): def aggregate_outputs(self, runtime=None, needed_outputs=None): outputs = self._outputs() cur_output = "" - for line in runtime.stdout.split('\n'): + with open(runtime.stdout, 'rt') as f: + stdout = f.read() + for line in stdout.splitlines(): if cur_output != "" and len(line.split()) != 0: setattr(outputs, cur_output, float(line)) cur_output = "" diff --git a/nipype/utils/spm_docs.py b/nipype/utils/spm_docs.py index 12169b482b..7c75197442 100644 --- a/nipype/utils/spm_docs.py +++ b/nipype/utils/spm_docs.py @@ -36,7 +36,9 @@ def grab_doc(task_name): cmd.inputs.script_lines = mcmd # Run the command and get the documentation out of the result. out = cmd.run() - return _strip_header(out.runtime.stdout) + with open(out.runtime.stdout, 'rt') as f: + stdout = f.read() + return _strip_header(stdout) def _strip_header(doc): From 8446d64938d705e0c4bd7c6bf1e27bf1ff75b398 Mon Sep 17 00:00:00 2001 From: oesteban Date: Wed, 21 Nov 2018 23:51:26 -0800 Subject: [PATCH 07/42] fix doctest --- nipype/utils/subprocess.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/nipype/utils/subprocess.py b/nipype/utils/subprocess.py index 1a1c8bc96e..d6d3bb2da9 100644 --- a/nipype/utils/subprocess.py +++ b/nipype/utils/subprocess.py @@ -55,14 +55,16 @@ def run_command(runtime, background=False, output=None, period=0.01, >>> from time import sleep >>> from nipype.interfaces.base.support import Bunch >>> from nipype.utils.subprocess import run_command - >>> rt = run_command(Bunch(cmdline='echo hello!')).runtime + >>> rt = run_command(Bunch(cmdline='echo hello!', + ... shell=True)).runtime >>> rt.returncode 0 >>> with open(rt.stdout) as stdout: ... data = stdout.read() >>> data 'hello!\n' - >>> rt = run_command(Bunch(cmdline='sleep 2'), background=True).runtime + >>> rt = run_command(Bunch(cmdline='sleep 2', shell=True), + ... background=True).runtime >>> rt.returncode is None True >>> sleep(5) From 8c7d6880bb3db3d2f96d53c504c3e731f80e28bc Mon Sep 17 00:00:00 2001 From: oesteban Date: Thu, 22 Nov 2018 00:31:16 -0800 Subject: [PATCH 08/42] undo traits_set --- nipype/pipeline/engine/utils.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nipype/pipeline/engine/utils.py b/nipype/pipeline/engine/utils.py index 37b11af847..e454be57d5 100644 --- a/nipype/pipeline/engine/utils.py +++ b/nipype/pipeline/engine/utils.py @@ -307,13 +307,13 @@ def save_resultfile(result, cwd, name): tosave = _uncollapse(outputs.copy(), collapsed) except AttributeError: tosave = outputs = result.outputs.dictcopy() # outputs was a bunch - result.outputs.trait_set(**modify_paths(tosave, relative=True, basedir=cwd)) + result.outputs.set(**modify_paths(tosave, relative=True, basedir=cwd)) savepkl(resultsfile, result) logger.debug('saved results in %s', resultsfile) if result.outputs: - result.outputs.trait_set(**outputs) + result.outputs.set(**outputs) def load_resultfile(path, name): @@ -363,7 +363,7 @@ def load_resultfile(path, name): except AttributeError: outputs = result.outputs.dictcopy() # outputs == Bunch try: - result.outputs.trait_set( + result.outputs.set( **modify_paths(outputs, relative=False, basedir=path)) except FileNotFoundError: logger.debug('conversion to full path results in ' From 70aa9ce67f3f7075b534291f6672beb0c52361e2 Mon Sep 17 00:00:00 2001 From: oesteban Date: Thu, 22 Nov 2018 09:06:47 -0800 Subject: [PATCH 09/42] do not discard first character --- nipype/utils/subprocess.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nipype/utils/subprocess.py b/nipype/utils/subprocess.py index d6d3bb2da9..9fac180593 100644 --- a/nipype/utils/subprocess.py +++ b/nipype/utils/subprocess.py @@ -164,7 +164,7 @@ def _update_output(self, tracker=None): if out_size > tracker[0]: data = None with open(self._runtime.stdout) as out: - out.seek(tracker[0] + int(tracker[0] > 0)) + out.seek(tracker[0]) data = out.read() tracker = (out_size, tracker[1]) if data: @@ -173,7 +173,7 @@ def _update_output(self, tracker=None): if err_size > tracker[1]: data = None with open(self._runtime.stderr) as err: - err.seek(tracker[1] + int(tracker[1] > 0)) + err.seek(tracker[1]) data = err.read() tracker = (tracker[0], err_size) if data: From 132cddf65f3f83dd30e999cc2009a1374df1377b Mon Sep 17 00:00:00 2001 From: oesteban Date: Thu, 22 Nov 2018 09:19:50 -0800 Subject: [PATCH 10/42] [MAINT] Import Sequence from collections to avoid DeprecationWarning --- nipype/interfaces/base/traits_extension.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nipype/interfaces/base/traits_extension.py b/nipype/interfaces/base/traits_extension.py index a98ec020c8..7a464cc557 100644 --- a/nipype/interfaces/base/traits_extension.py +++ b/nipype/interfaces/base/traits_extension.py @@ -25,7 +25,7 @@ from builtins import str, bytes import os -import collections +from collections import Sequence # perform all external trait imports here from traits import __version__ as traits_version @@ -323,7 +323,7 @@ def validate(self, object, name, value): # want to treat range and other sequences (except str) as list if not isinstance(value, (str, bytes)) and isinstance( - value, collections.Sequence): + value, Sequence): value = list(value) if not isdefined(value) or \ From 1873bfb87b2b94296211d19a5b6ad05259c339a1 Mon Sep 17 00:00:00 2001 From: oesteban Date: Thu, 22 Nov 2018 11:15:22 -0800 Subject: [PATCH 11/42] test passing with python 3.7.1, 60 skipped tests --- nipype/pipeline/engine/nodes.py | 9 +++------ nipype/utils/subprocess.py | 18 ++++++++++++++++-- nipype/utils/tests/test_nipype2boutiques.py | 4 +++- 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/nipype/pipeline/engine/nodes.py b/nipype/pipeline/engine/nodes.py index 35eb0fabf5..c4089f7880 100644 --- a/nipype/pipeline/engine/nodes.py +++ b/nipype/pipeline/engine/nodes.py @@ -622,9 +622,8 @@ def _run_command(self, execute, copyfiles=True): with indirectory(outdir): cmd = self._interface.cmdline except Exception as msg: - # append cmdline exception to errorlog - with open(result.runtime.stderr, 'wt+') as f: - f.write('\n\n%s' % msg) + # insert cmdline exception to runtime object + result.runtime.cmderr = msg _save_resultfile(result, outdir, self.name) raise cmdfile = op.join(outdir, 'command.txt') @@ -635,9 +634,7 @@ def _run_command(self, execute, copyfiles=True): try: result = self._interface.run(cwd=outdir) except Exception as msg: - # append cmdline exception to errorlog - with open(result.runtime.stderr, 'wt+') as f: - f.write('\n\n%s' % msg) + result.runtime.exception = msg _save_resultfile(result, outdir, self.name) raise diff --git a/nipype/utils/subprocess.py b/nipype/utils/subprocess.py index 9fac180593..8264d6b71b 100644 --- a/nipype/utils/subprocess.py +++ b/nipype/utils/subprocess.py @@ -12,6 +12,9 @@ import threading from time import time, sleep from subprocess import Popen +from tempfile import mkdtemp +from shutil import rmtree + from .filemanip import canonicalize_env from .. import logging @@ -92,7 +95,7 @@ class RuntimeMonitor(threading.Thread): period """ __slots__ = ['_proc', '_output', '_stdoutfh', '_stderrfh', '_runtime', - '_callback_fn', '_calback_args', '_callback_kwargs'] + '_callback_fn', '_calback_args', '_callback_kwargs', '_tmpdir'] def __init__(self, runtime, output=None, period=0.1, callback_fn=None, callback_args=None, callback_kwargs=None): @@ -129,8 +132,13 @@ def __init__(self, runtime, output=None, period=0.1, self._callback_fn = callback_fn self._callback_args = callback_args or tuple() self._callback_kwargs = callback_kwargs or {} + self._tmpdir = False + + cwd = getattr(runtime, 'cwd', None) + if not cwd: + self._tmpdir = True + cwd = mkdtemp() - cwd = getattr(self._runtime, 'cwd', os.getcwd()) self._runtime.cwd = cwd self._runtime.stdout = getattr( self._runtime, 'stdout', os.path.join(cwd, '.nipype.out')) @@ -146,8 +154,14 @@ def __init__(self, runtime, output=None, period=0.1, @property def runtime(self): + """The nipype runtime object""" return self._runtime + @property + def tmpdir(self): + """Whether a temporal directory was created""" + return self._tmpdir + def _update_output(self, tracker=None): """When the ``stream`` output is selected, just keeps track of the logs backing up the process' outputs and diff --git a/nipype/utils/tests/test_nipype2boutiques.py b/nipype/utils/tests/test_nipype2boutiques.py index f1d0c46eed..73c48c196b 100644 --- a/nipype/utils/tests/test_nipype2boutiques.py +++ b/nipype/utils/tests/test_nipype2boutiques.py @@ -1,13 +1,15 @@ # -*- coding: utf-8 -*- # emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*- # vi: set ft=python sts=4 ts=4 sw=4 et: +import pytest from future import standard_library standard_library.install_aliases() from ..nipype2boutiques import generate_boutiques_descriptor -def test_generate(): +def test_generate(tmpdir): + tmpdir.chdir() generate_boutiques_descriptor(module='nipype.interfaces.ants.registration', interface_name='ANTS', ignored_template_inputs=(), From a0f26be4dfe419dbb9e944ce4c49f169347e74b2 Mon Sep 17 00:00:00 2001 From: oesteban Date: Thu, 22 Nov 2018 21:21:06 -0800 Subject: [PATCH 12/42] add terminal output to matlab subcommands --- nipype/interfaces/spm/base.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/nipype/interfaces/spm/base.py b/nipype/interfaces/spm/base.py index 3735aab077..0732d040e1 100644 --- a/nipype/interfaces/spm/base.py +++ b/nipype/interfaces/spm/base.py @@ -196,7 +196,8 @@ def getinfo(klass, matlab_cmd=None, paths=None, use_mcr=None): 'release': klass._version } logger.debug('matlab command or path has changed. recomputing version.') - mlab = MatlabCommand(matlab_cmd=matlab_cmd, resource_monitor=False) + mlab = MatlabCommand(matlab_cmd=matlab_cmd, resource_monitor=False, + terminal_output='default') mlab.inputs.mfile = False if paths: mlab.inputs.paths = paths @@ -336,7 +337,8 @@ def _matlab_cmd_update(self): matlab_cmd=self.inputs.matlab_cmd, mfile=self.inputs.mfile, paths=self.inputs.paths, - resource_monitor=False) + resource_monitor=False, + terminal_output='default') self.mlab.inputs.script_file = 'pyscript_%s.m' % \ self.__class__.__name__.split('.')[-1].lower() if isdefined(self.inputs.use_mcr) and self.inputs.use_mcr: From adec5cfe4b9a47735b59fd40757c2f84fd4d4558 Mon Sep 17 00:00:00 2001 From: oesteban Date: Fri, 23 Nov 2018 08:25:19 -0800 Subject: [PATCH 13/42] use our capture std i/o context --- nipype/interfaces/base/tests/test_core.py | 9 +++++---- nipype/testing/utils.py | 13 +++++++++++++ nipype/utils/tests/test_cmd.py | 21 ++++----------------- 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/nipype/interfaces/base/tests/test_core.py b/nipype/interfaces/base/tests/test_core.py index db63c78062..631cfa03d2 100644 --- a/nipype/interfaces/base/tests/test_core.py +++ b/nipype/interfaces/base/tests/test_core.py @@ -6,12 +6,12 @@ from builtins import open import simplejson as json from future import standard_library -from IPython.utils.io import capture_output import pytest from .... import config from ....testing import example_data +from ....testing.utils import capture_sys_output from ... import base as nib standard_library.install_aliases() @@ -308,7 +308,8 @@ def _list_outputs(self): obj.run() -def test_Commandline(): +def test_Commandline(tmpdir): + tmpdir.chdir() with pytest.raises(Exception): nib.CommandLine() ci = nib.CommandLine(command='which') @@ -416,11 +417,11 @@ def test_CommandLine_output(tmpdir): assert name in stdout # Test streamed output - with capture_output() as captured: + with capture_sys_output() as (stdout, stderr): ci.terminal_output = 'stream' res = ci.run() - assert name in captured.stdout + assert name in stdout.getvalue() def test_global_CommandLine_output(tmpdir): diff --git a/nipype/testing/utils.py b/nipype/testing/utils.py index 716b16da78..9011cac0bd 100644 --- a/nipype/testing/utils.py +++ b/nipype/testing/utils.py @@ -8,10 +8,13 @@ from builtins import range, object, open import os +import sys import time import shutil import signal import subprocess +from contextlib import contextmanager +from io import StringIO from subprocess import CalledProcessError from tempfile import mkdtemp from future.utils import raise_from @@ -23,6 +26,16 @@ import nibabel as nb +@contextmanager +def capture_sys_output(): + caputure_out, capture_err = StringIO(), StringIO() + current_out, current_err = sys.stdout, sys.stderr + try: + sys.stdout, sys.stderr = caputure_out, capture_err + yield caputure_out, capture_err + finally: + sys.stdout, sys.stderr = current_out, current_err + class TempFATFS(object): def __init__(self, size_in_mbytes=8, delay=0.5): """Temporary filesystem for testing non-POSIX filesystems on a POSIX diff --git a/nipype/utils/tests/test_cmd.py b/nipype/utils/tests/test_cmd.py index 0e16e0aad8..9d27462a3e 100644 --- a/nipype/utils/tests/test_cmd.py +++ b/nipype/utils/tests/test_cmd.py @@ -2,30 +2,17 @@ from __future__ import (print_function, division, unicode_literals, absolute_import) -from future import standard_library -standard_library.install_aliases() - -import pytest import sys -from contextlib import contextmanager +import pytest +from future import standard_library -from io import StringIO +from ...testing.utils import capture_sys_output from ...utils import nipype_cmd +standard_library.install_aliases() PY2 = sys.version_info[0] < 3 -@contextmanager -def capture_sys_output(): - caputure_out, capture_err = StringIO(), StringIO() - current_out, current_err = sys.stdout, sys.stderr - try: - sys.stdout, sys.stderr = caputure_out, capture_err - yield caputure_out, capture_err - finally: - sys.stdout, sys.stderr = current_out, current_err - - class TestNipypeCMD(): maxDiff = None From 1cbf064d7c49d6f1eef61c7f770d0a2f8418f6f7 Mon Sep 17 00:00:00 2001 From: oesteban Date: Fri, 23 Nov 2018 08:54:18 -0800 Subject: [PATCH 14/42] changedir before test --- nipype/interfaces/fsl/tests/test_base.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/nipype/interfaces/fsl/tests/test_base.py b/nipype/interfaces/fsl/tests/test_base.py index 71022997b6..dc9ebcb61d 100644 --- a/nipype/interfaces/fsl/tests/test_base.py +++ b/nipype/interfaces/fsl/tests/test_base.py @@ -34,9 +34,10 @@ def test_outputtype_to_ext(): @pytest.mark.skipif(no_fsl(), reason="fsl is not installed") -def test_FSLCommand(): +def test_FSLCommand(tmpdir): # Most methods in FSLCommand are tested in the subclasses. Only # testing the one item that is not. + tmpdir.chdir() cmd = fsl.FSLCommand(command='ls') res = cmd.run() assert type(res) == InterfaceResult From 25a7030d4407db9f5f0918734d45ca98e9d9bab5 Mon Sep 17 00:00:00 2001 From: oesteban Date: Fri, 23 Nov 2018 10:28:38 -0800 Subject: [PATCH 15/42] fix python 2.7 tests --- nipype/interfaces/base/core.py | 3 +-- nipype/utils/filemanip.py | 38 +++++++++++++++++++++++++++++++--- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/nipype/interfaces/base/core.py b/nipype/interfaces/base/core.py index ab372b9c52..aee8d54c6e 100644 --- a/nipype/interfaces/base/core.py +++ b/nipype/interfaces/base/core.py @@ -22,7 +22,6 @@ import os import re import platform -from tempfile import TemporaryDirectory import shlex import sys from textwrap import wrap @@ -33,7 +32,7 @@ from ...utils.provenance import write_provenance from ...utils.misc import trim, str2bool, rgetcwd from ...utils.filemanip import (FileNotFoundError, split_filename, - which, get_dependencies) + which, get_dependencies, TemporaryDirectory) from ...utils.subprocess import run_command from ...external.due import due diff --git a/nipype/utils/filemanip.py b/nipype/utils/filemanip.py index d3f9b2fc6a..c7142dcbc6 100644 --- a/nipype/utils/filemanip.py +++ b/nipype/utils/filemanip.py @@ -21,16 +21,15 @@ import contextlib import posixpath import simplejson as json -import numpy as np from builtins import str, bytes, open from .. import logging, config from .misc import is_container from future import standard_library -standard_library.install_aliases() -fmlogger = logging.getLogger('nipype.utils') + +standard_library.install_aliases() related_filetype_sets = [ ('.hdr', '.img', '.mat'), @@ -39,6 +38,39 @@ ] PY3 = sys.version_info[0] >= 3 +fmlogger = logging.getLogger('nipype.utils') + +try: + from tempfile import TemporaryDirectory +except ImportError: + from tempfile import mkdtemp + from builtins import object + + class TemporaryDirectory(object): + """Create and return a temporary directory. This has the same + behavior as mkdtemp but can be used as a context manager. For + example: + with TemporaryDirectory() as tmpdir: + ... + Upon exiting the context, the directory and everything contained + in it are removed. + """ + + def __init__(self, suffix=None, prefix=None, dir=None): + self.name = mkdtemp(suffix, prefix, dir) + + def __repr__(self): + return "<{} {!r}>".format(self.__class__.__name__, self.name) + + def __enter__(self): + return self.name + + def __exit__(self, exc, value, tb): + self.cleanup() + + def cleanup(self): + shutil.rmtree(self.name) + class FileNotFoundError(Exception): pass From e2ac4c729ff549c18a07f52131902f1ca1919a7e Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sat, 24 Nov 2018 10:03:13 -0800 Subject: [PATCH 16/42] [MAINT] Offload interfaces with help formatting This is the first on a series of PRs to clean up Nipype interfaces. --- nipype/interfaces/base/__init__.py | 3 +- nipype/interfaces/base/core.py | 200 ++++------------------------- nipype/interfaces/base/support.py | 150 ++++++++++++++++++++-- 3 files changed, 164 insertions(+), 189 deletions(-) diff --git a/nipype/interfaces/base/__init__.py b/nipype/interfaces/base/__init__.py index f617064b2f..2284c1763a 100644 --- a/nipype/interfaces/base/__init__.py +++ b/nipype/interfaces/base/__init__.py @@ -22,5 +22,4 @@ OutputMultiObject, InputMultiObject, OutputMultiPath, InputMultiPath) -from .support import (Bunch, InterfaceResult, load_template, - NipypeInterfaceError) +from .support import (Bunch, InterfaceResult, NipypeInterfaceError) diff --git a/nipype/interfaces/base/core.py b/nipype/interfaces/base/core.py index 40efa1f33a..3eaec3d7d6 100644 --- a/nipype/interfaces/base/core.py +++ b/nipype/interfaces/base/core.py @@ -20,18 +20,17 @@ from copy import deepcopy from datetime import datetime as dt import os -import re import platform import subprocess as sp import shlex import sys -from textwrap import wrap import simplejson as json from dateutil.parser import parse as parseutc +from future import standard_library from ... import config, logging, LooseVersion from ...utils.provenance import write_provenance -from ...utils.misc import trim, str2bool, rgetcwd +from ...utils.misc import str2bool, rgetcwd from ...utils.filemanip import (FileNotFoundError, split_filename, which, get_dependencies) from ...utils.subprocess import run_command @@ -41,9 +40,9 @@ from .traits_extension import traits, isdefined, TraitError from .specs import (BaseInterfaceInputSpec, CommandLineInputSpec, StdOutCommandLineInputSpec, MpiCommandLineInputSpec) -from .support import (Bunch, InterfaceResult, NipypeInterfaceError) +from .support import (Bunch, InterfaceResult, NipypeInterfaceError, + format_help) -from future import standard_library standard_library.install_aliases() iflogger = logging.getLogger('nipype.interface') @@ -67,38 +66,22 @@ class Interface(object): input_spec = None # A traited input specification output_spec = None # A traited output specification - - # defines if the interface can reuse partial results after interruption - _can_resume = False + _can_resume = False # See property below + _always_run = False # See property below @property def can_resume(self): + """defines if the interface can reuse partial results after interruption""" return self._can_resume - # should the interface be always run even if the inputs were not changed? - _always_run = False - @property def always_run(self): + """should the interface be always run even if the inputs were not changed?""" return self._always_run - def __init__(self, **inputs): - """Initialize command with given args and inputs.""" - raise NotImplementedError - - @classmethod - def help(cls): - """ Prints class help""" - raise NotImplementedError - - @classmethod - def _inputs_help(cls): - """ Prints inputs help""" - raise NotImplementedError - - @classmethod - def _outputs_help(cls): - """ Prints outputs help""" + @property + def version(self): + """interfaces should implement a version property""" raise NotImplementedError @classmethod @@ -106,9 +89,14 @@ def _outputs(cls): """ Initializes outputs""" raise NotImplementedError - @property - def version(self): - raise NotImplementedError + @classmethod + def help(cls, returnhelp=False): + """ Prints class help """ + allhelp = format_help(cls) + if returnhelp: + return allhelp + print(allhelp) + return None # R1710 def run(self): """Execute the command.""" @@ -185,142 +173,6 @@ def __init__(self, from_file=None, resource_monitor=None, for name, value in list(inputs.items()): setattr(self.inputs, name, value) - @classmethod - def help(cls, returnhelp=False): - """ Prints class help - """ - - if cls.__doc__: - # docstring = cls.__doc__.split('\n') - # docstring = [trim(line, '') for line in docstring] - docstring = trim(cls.__doc__).split('\n') + [''] - else: - docstring = [''] - - allhelp = '\n'.join(docstring + cls._inputs_help( - ) + [''] + cls._outputs_help() + [''] + cls._refs_help() + ['']) - if returnhelp: - return allhelp - else: - print(allhelp) - - @classmethod - def _refs_help(cls): - """ Prints interface references. - """ - if not cls.references_: - return [] - - helpstr = ['References::'] - - for r in cls.references_: - helpstr += ['{}'.format(r['entry'])] - - return helpstr - - @classmethod - def _get_trait_desc(self, inputs, name, spec): - desc = spec.desc - xor = spec.xor - requires = spec.requires - argstr = spec.argstr - - manhelpstr = ['\t%s' % name] - - type_info = spec.full_info(inputs, name, None) - - default = '' - if spec.usedefault: - default = ', nipype default value: %s' % str( - spec.default_value()[1]) - line = "(%s%s)" % (type_info, default) - - manhelpstr = wrap( - line, - 70, - initial_indent=manhelpstr[0] + ': ', - subsequent_indent='\t\t ') - - if desc: - for line in desc.split('\n'): - line = re.sub("\s+", " ", line) - manhelpstr += wrap( - line, 70, initial_indent='\t\t', subsequent_indent='\t\t') - - if argstr: - pos = spec.position - if pos is not None: - manhelpstr += wrap( - 'flag: %s, position: %s' % (argstr, pos), - 70, - initial_indent='\t\t', - subsequent_indent='\t\t') - else: - manhelpstr += wrap( - 'flag: %s' % argstr, - 70, - initial_indent='\t\t', - subsequent_indent='\t\t') - - if xor: - line = '%s' % ', '.join(xor) - manhelpstr += wrap( - line, - 70, - initial_indent='\t\tmutually_exclusive: ', - subsequent_indent='\t\t ') - - if requires: - others = [field for field in requires if field != name] - line = '%s' % ', '.join(others) - manhelpstr += wrap( - line, - 70, - initial_indent='\t\trequires: ', - subsequent_indent='\t\t ') - return manhelpstr - - @classmethod - def _inputs_help(cls): - """ Prints description for input parameters - """ - helpstr = ['Inputs::'] - - inputs = cls.input_spec() - if len(list(inputs.traits(transient=None).items())) == 0: - helpstr += ['', '\tNone'] - return helpstr - - manhelpstr = ['', '\t[Mandatory]'] - mandatory_items = inputs.traits(mandatory=True) - for name, spec in sorted(mandatory_items.items()): - manhelpstr += cls._get_trait_desc(inputs, name, spec) - - opthelpstr = ['', '\t[Optional]'] - for name, spec in sorted(inputs.traits(transient=None).items()): - if name in mandatory_items: - continue - opthelpstr += cls._get_trait_desc(inputs, name, spec) - - if manhelpstr: - helpstr += manhelpstr - if opthelpstr: - helpstr += opthelpstr - return helpstr - - @classmethod - def _outputs_help(cls): - """ Prints description for output parameters - """ - helpstr = ['Outputs::', ''] - if cls.output_spec: - outputs = cls.output_spec() - for name, spec in sorted(outputs.traits(transient=None).items()): - helpstr += cls._get_trait_desc(outputs, name, spec) - if len(helpstr) == 2: - helpstr += ['\tNone'] - return helpstr - def _outputs(self): """ Returns a bunch containing output fields for the class """ @@ -653,7 +505,7 @@ def save_inputs_to_json(self, json_file): A convenient way to save current inputs to a JSON file. """ inputs = self.inputs.get_traitsfree() - iflogger.debug('saving inputs {}', inputs) + iflogger.debug('saving inputs %s', inputs) with open(json_file, 'w' if PY3 else 'wb') as fhandle: json.dump(inputs, fhandle, indent=4, ensure_ascii=False) @@ -785,14 +637,6 @@ def set_default_terminal_output(cls, output_type): raise AttributeError( 'Invalid terminal output_type: %s' % output_type) - @classmethod - def help(cls, returnhelp=False): - allhelp = 'Wraps command **{cmd}**\n\n{help}'.format( - cmd=cls._cmd, help=super(CommandLine, cls).help(returnhelp=True)) - if returnhelp: - return allhelp - print(allhelp) - def __init__(self, command=None, terminal_output=None, **inputs): super(CommandLine, self).__init__(**inputs) self._environ = None @@ -812,6 +656,10 @@ def __init__(self, command=None, terminal_output=None, **inputs): @property def cmd(self): """sets base command, immutable""" + if not self._cmd: + raise NotImplementedError( + 'CommandLineInterface should wrap an executable, but ' + 'none has been set.') return self._cmd @property diff --git a/nipype/interfaces/base/support.py b/nipype/interfaces/base/support.py index 543d4b6c40..305a0a13b5 100644 --- a/nipype/interfaces/base/support.py +++ b/nipype/interfaces/base/support.py @@ -13,12 +13,15 @@ import os from copy import deepcopy +from textwrap import wrap +import re from ... import logging from ...utils.misc import is_container from ...utils.filemanip import md5, to_str, hash_infile iflogger = logging.getLogger('nipype.interface') +HELP_LINEWIDTH = 70 class NipypeInterfaceError(Exception): """Custom error for interfaces""" @@ -235,14 +238,139 @@ def version(self): return self._version -def load_template(name): - """ - Deprecated stub for backwards compatibility, - please use nipype.interfaces.fsl.model.load_template - - """ - from ..fsl.model import load_template - iflogger.warning( - 'Deprecated in 1.0.0, and will be removed in 1.1.0, ' - 'please use nipype.interfaces.fsl.model.load_template instead.') - return load_template(name) +def format_help(cls): + """Prints help text of a Nipype interface""" + from ...utils.misc import trim + + docstring = [] + cmd = getattr(cls, '_cmd', None) + if cmd: + docstring += ['Wraps the executable command ``%s``.' % cmd, ''] + + if cls.__doc__: + docstring += trim(cls.__doc__).split('\n') + [''] + + allhelp = '\n'.join( + docstring + + _inputs_help(cls) + [''] + + _outputs_help(cls) + [''] + + _refs_help(cls) + ) + return allhelp.expandtabs(4) + + +def _inputs_help(cls): + """Prints description for input parameters""" + helpstr = ['Inputs::', ''] + mandatory_keys = [] + optional_items = [] + + if cls.input_spec: + inputs = cls.input_spec() + mandatory_items = list(inputs.traits(mandatory=True).items()) + if mandatory_items: + helpstr += ['\t[Mandatory]'] + for name, spec in sorted(mandatory_items): + helpstr += _get_trait_desc(inputs, name, spec) + + mandatory_keys = [item[0] for item in mandatory_items] + optional_items = ['\n'.join(_get_trait_desc(inputs, name, val)) + for name, val in inputs.traits(transient=None).items() + if name not in mandatory_keys] + if optional_items: + helpstr += ['\t[Optional]'] + optional_items + + if not mandatory_keys and not optional_items: + helpstr += ['\tNone'] + return helpstr + + +def _outputs_help(cls): + """Prints description for output parameters""" + helpstr = ['Outputs::', '', '\tNone'] + if cls.output_spec: + outputs = cls.output_spec() + outhelpstr = [ + '\n'.join(_get_trait_desc(outputs, name, spec)) + for name, spec in sorted(outputs.traits(transient=None).items())] + if outhelpstr: + helpstr = helpstr[:-1] + outhelpstr + return helpstr + + +def _refs_help(cls): + """Prints interface references.""" + references = getattr(cls, '_references', None) + if not references: + return [] + + helpstr = ['References:', '-----------'] + for r in references: + helpstr += ['{}'.format(r['entry'])] + + return helpstr + + +def _get_trait_desc(inputs, name, spec): + """Parses a HasTraits object into a nipype documentation string""" + desc = spec.desc + xor = spec.xor + requires = spec.requires + argstr = spec.argstr + + manhelpstr = ['\t%s' % name] + + type_info = spec.full_info(inputs, name, None) + + default = '' + if spec.usedefault: + default = ', nipype default value: %s' % str( + spec.default_value()[1]) + line = "(%s%s)" % (type_info, default) + + manhelpstr = wrap( + line, + HELP_LINEWIDTH, + initial_indent=manhelpstr[0] + ': ', + subsequent_indent='\t\t ') + + if desc: + for line in desc.split('\n'): + line = re.sub(r"\s+", " ", line) + manhelpstr += wrap( + line, HELP_LINEWIDTH, + initial_indent='\t\t', + subsequent_indent='\t\t') + + if argstr: + pos = spec.position + if pos is not None: + manhelpstr += wrap( + 'flag: %s, position: %s' % (argstr, pos), + HELP_LINEWIDTH, + initial_indent='\t\t', + subsequent_indent='\t\t') + else: + manhelpstr += wrap( + 'flag: %s' % argstr, + HELP_LINEWIDTH, + initial_indent='\t\t', + subsequent_indent='\t\t') + + if xor: + line = '%s' % ', '.join(xor) + manhelpstr += wrap( + line, + HELP_LINEWIDTH, + initial_indent='\t\tmutually_exclusive: ', + subsequent_indent='\t\t ') + + if requires: + others = [field for field in requires if field != name] + line = '%s' % ', '.join(others) + manhelpstr += wrap( + line, + HELP_LINEWIDTH, + initial_indent='\t\trequires: ', + subsequent_indent='\t\t ') + return manhelpstr From c9eea71a07c8fb0267c807e204b0bcb3606a486f Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sat, 24 Nov 2018 10:39:50 -0800 Subject: [PATCH 17/42] add a couple of tests --- nipype/interfaces/base/support.py | 37 ++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/nipype/interfaces/base/support.py b/nipype/interfaces/base/support.py index 305a0a13b5..d553a15fe7 100644 --- a/nipype/interfaces/base/support.py +++ b/nipype/interfaces/base/support.py @@ -239,7 +239,20 @@ def version(self): def format_help(cls): - """Prints help text of a Nipype interface""" + """ + Prints help text of a Nipype interface + + >>> from nipype.interfaces.afni import GCOR + >>> GCOR.help() # doctest: +ELLIPSIS, +NORMALIZE_WHITESPACE + Wraps the executable command ``@compute_gcor``. + + Computes the average correlation between every voxel + and ever other voxel, over any give mask. + + + For complete details, ... + + """ from ...utils.misc import trim docstring = [] @@ -260,7 +273,14 @@ def format_help(cls): def _inputs_help(cls): - """Prints description for input parameters""" + r""" + Prints description for input parameters + + >>> from nipype.interfaces.afni import GCOR + >>> _inputs_help(GCOR) # doctest: +ELLIPSIS, +NORMALIZE_WHITESPACE + ['Inputs::', '', '\t[Mandatory]', '\tin_file: (an existing file name)', ... + + """ helpstr = ['Inputs::', ''] mandatory_keys = [] optional_items = [] @@ -286,7 +306,14 @@ def _inputs_help(cls): def _outputs_help(cls): - """Prints description for output parameters""" + r""" + Prints description for output parameters + + >>> from nipype.interfaces.afni import GCOR + >>> _outputs_help(GCOR) # doctest: +ELLIPSIS, +NORMALIZE_WHITESPACE + ['Outputs::', '', '\tout: (a float)\n\t\tglobal correlation value'] + + """ helpstr = ['Outputs::', '', '\tNone'] if cls.output_spec: outputs = cls.output_spec() @@ -346,13 +373,13 @@ def _get_trait_desc(inputs, name, spec): pos = spec.position if pos is not None: manhelpstr += wrap( - 'flag: %s, position: %s' % (argstr, pos), + 'argument: ``%s``, position: %s' % (argstr, pos), HELP_LINEWIDTH, initial_indent='\t\t', subsequent_indent='\t\t') else: manhelpstr += wrap( - 'flag: %s' % argstr, + 'argument: ``%s``' % argstr, HELP_LINEWIDTH, initial_indent='\t\t', subsequent_indent='\t\t') From 454c6aca61d0f14e72ae236d98a2678a6ce7ca95 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sat, 24 Nov 2018 10:59:39 -0800 Subject: [PATCH 18/42] add documentation --- nipype/interfaces/base/core.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/nipype/interfaces/base/core.py b/nipype/interfaces/base/core.py index 3eaec3d7d6..62ba176345 100644 --- a/nipype/interfaces/base/core.py +++ b/nipype/interfaces/base/core.py @@ -71,12 +71,14 @@ class Interface(object): @property def can_resume(self): - """defines if the interface can reuse partial results after interruption""" + """Defines if the interface can reuse partial results after interruption. + Only applies to interfaces being run within a workflow context.""" return self._can_resume @property def always_run(self): - """should the interface be always run even if the inputs were not changed?""" + """Should the interface be always run even if the inputs were not changed? + Only applies to interfaces being run within a workflow context.""" return self._always_run @property From f09b3bfa2a54e27f9195bfa5f8aa7e3ad9fbe9ff Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sat, 24 Nov 2018 11:55:27 -0800 Subject: [PATCH 19/42] fix old _get_trait_desc calls --- nipype/interfaces/base/support.py | 8 ++++---- nipype/scripts/utils.py | 4 ++-- nipype/utils/nipype_cmd.py | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/nipype/interfaces/base/support.py b/nipype/interfaces/base/support.py index d553a15fe7..4e551efa73 100644 --- a/nipype/interfaces/base/support.py +++ b/nipype/interfaces/base/support.py @@ -291,10 +291,10 @@ def _inputs_help(cls): if mandatory_items: helpstr += ['\t[Mandatory]'] for name, spec in sorted(mandatory_items): - helpstr += _get_trait_desc(inputs, name, spec) + helpstr += get_trait_desc(inputs, name, spec) mandatory_keys = [item[0] for item in mandatory_items] - optional_items = ['\n'.join(_get_trait_desc(inputs, name, val)) + optional_items = ['\n'.join(get_trait_desc(inputs, name, val)) for name, val in inputs.traits(transient=None).items() if name not in mandatory_keys] if optional_items: @@ -318,7 +318,7 @@ def _outputs_help(cls): if cls.output_spec: outputs = cls.output_spec() outhelpstr = [ - '\n'.join(_get_trait_desc(outputs, name, spec)) + '\n'.join(get_trait_desc(outputs, name, spec)) for name, spec in sorted(outputs.traits(transient=None).items())] if outhelpstr: helpstr = helpstr[:-1] + outhelpstr @@ -338,7 +338,7 @@ def _refs_help(cls): return helpstr -def _get_trait_desc(inputs, name, spec): +def get_trait_desc(inputs, name, spec): """Parses a HasTraits object into a nipype documentation string""" desc = spec.desc xor = spec.xor diff --git a/nipype/scripts/utils.py b/nipype/scripts/utils.py index f4b8a86fb1..ce9acde7fd 100644 --- a/nipype/scripts/utils.py +++ b/nipype/scripts/utils.py @@ -13,6 +13,7 @@ from .instance import import_module from ..interfaces.base import InputMultiPath, traits +from ..interfaces.base.support import get_trait_desc # different context options CONTEXT_SETTINGS = dict(help_option_names=['-h', '--help']) @@ -61,8 +62,7 @@ def add_args_options(arg_parser, interface): """Add arguments to `arg_parser` to create a CLI for `interface`.""" inputs = interface.input_spec() for name, spec in sorted(interface.inputs.traits(transient=None).items()): - desc = "\n".join(interface._get_trait_desc(inputs, name, - spec))[len(name) + 2:] + desc = "\n".join(get_trait_desc(inputs, name, spec))[len(name) + 2:] # Escape any % signs with a % desc = desc.replace('%', '%%') args = {} diff --git a/nipype/utils/nipype_cmd.py b/nipype/utils/nipype_cmd.py index b31795aa92..36fa69b3c1 100644 --- a/nipype/utils/nipype_cmd.py +++ b/nipype/utils/nipype_cmd.py @@ -8,6 +8,7 @@ import sys from ..interfaces.base import Interface, InputMultiPath, traits +from ..interfaces.base.support import get_trait_desc from .misc import str2bool @@ -30,8 +31,7 @@ def add_options(parser=None, module=None, function=None): inputs = interface.input_spec() for name, spec in sorted( interface.inputs.traits(transient=None).items()): - desc = "\n".join(interface._get_trait_desc(inputs, name, - spec))[len(name) + 2:] + desc = "\n".join(get_trait_desc(inputs, name, spec))[len(name) + 2:] args = {} if spec.is_trait_type(traits.Bool): From 2575d4d632d4a273b070d6dbab0f645c4ec2e7ea Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sat, 24 Nov 2018 11:59:19 -0800 Subject: [PATCH 20/42] fix old ``_{in,out}puts_help`` calls --- nipype/interfaces/base/tests/test_core.py | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/nipype/interfaces/base/tests/test_core.py b/nipype/interfaces/base/tests/test_core.py index 392d38706f..748ac74a97 100644 --- a/nipype/interfaces/base/tests/test_core.py +++ b/nipype/interfaces/base/tests/test_core.py @@ -12,6 +12,7 @@ from .... import config from ....testing import example_data from ... import base as nib +from ..support import _inputs_help standard_library.install_aliases() @@ -42,14 +43,6 @@ def test_Interface(): assert nib.Interface.output_spec is None with pytest.raises(NotImplementedError): nib.Interface() - with pytest.raises(NotImplementedError): - nib.Interface.help() - with pytest.raises(NotImplementedError): - nib.Interface._inputs_help() - with pytest.raises(NotImplementedError): - nib.Interface._outputs_help() - with pytest.raises(NotImplementedError): - nib.Interface._outputs() class DerivedInterface(nib.Interface): def __init__(self): @@ -88,7 +81,7 @@ class DerivedInterface(nib.BaseInterface): resource_monitor = False assert DerivedInterface.help() is None - assert 'moo' in ''.join(DerivedInterface._inputs_help()) + assert 'moo' in ''.join(_inputs_help(DerivedInterface)) assert DerivedInterface()._outputs() is None assert DerivedInterface._get_filecopy_info()[0]['key'] == 'woo' assert DerivedInterface._get_filecopy_info()[0]['copy'] From 1512cc3cbef3ce9ddf3e9d1bc2e51a1545e5b0b0 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sat, 24 Nov 2018 14:59:54 -0800 Subject: [PATCH 21/42] re-introduce __init__ --- nipype/interfaces/base/core.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/nipype/interfaces/base/core.py b/nipype/interfaces/base/core.py index 62ba176345..6bf19eaa72 100644 --- a/nipype/interfaces/base/core.py +++ b/nipype/interfaces/base/core.py @@ -100,6 +100,10 @@ def help(cls, returnhelp=False): print(allhelp) return None # R1710 + def __init__(self): + """Subclasses must implement __init__""" + raise NotImplementedError + def run(self): """Execute the command.""" raise NotImplementedError From 7cb203825af13de69d2f15444e8b74ae1dffaaf0 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sat, 24 Nov 2018 20:44:28 -0800 Subject: [PATCH 22/42] [MAINT] Outsource checks of inputs from interface Another PR removing stuff from the interface class definition. --- nipype/interfaces/base/core.py | 59 +++-------------- nipype/interfaces/base/specs.py | 65 ++++++++++++++++++- nipype/interfaces/base/tests/test_core.py | 5 -- nipype/interfaces/base/tests/test_specs.py | 74 ++++++++++++++++++++-- nipype/interfaces/freesurfer/preprocess.py | 8 ++- nipype/utils/errors.py | 60 ++++++++++++++++++ 6 files changed, 208 insertions(+), 63 deletions(-) create mode 100644 nipype/utils/errors.py diff --git a/nipype/interfaces/base/core.py b/nipype/interfaces/base/core.py index 40efa1f33a..06ef43c7cf 100644 --- a/nipype/interfaces/base/core.py +++ b/nipype/interfaces/base/core.py @@ -40,7 +40,8 @@ from .traits_extension import traits, isdefined, TraitError from .specs import (BaseInterfaceInputSpec, CommandLineInputSpec, - StdOutCommandLineInputSpec, MpiCommandLineInputSpec) + StdOutCommandLineInputSpec, MpiCommandLineInputSpec, + check_mandatory_inputs) from .support import (Bunch, InterfaceResult, NipypeInterfaceError) from future import standard_library @@ -343,53 +344,6 @@ def _get_filecopy_info(cls): info.append(dict(key=name, copy=spec.copyfile)) return info - def _check_requires(self, spec, name, value): - """ check if required inputs are satisfied - """ - if spec.requires: - values = [ - not isdefined(getattr(self.inputs, field)) - for field in spec.requires - ] - if any(values) and isdefined(value): - msg = ("%s requires a value for input '%s' because one of %s " - "is set. For a list of required inputs, see %s.help()" % - (self.__class__.__name__, name, - ', '.join(spec.requires), self.__class__.__name__)) - raise ValueError(msg) - - def _check_xor(self, spec, name, value): - """ check if mutually exclusive inputs are satisfied - """ - if spec.xor: - values = [ - isdefined(getattr(self.inputs, field)) for field in spec.xor - ] - if not any(values) and not isdefined(value): - msg = ("%s requires a value for one of the inputs '%s'. " - "For a list of required inputs, see %s.help()" % - (self.__class__.__name__, ', '.join(spec.xor), - self.__class__.__name__)) - raise ValueError(msg) - - def _check_mandatory_inputs(self): - """ Raises an exception if a mandatory input is Undefined - """ - for name, spec in list(self.inputs.traits(mandatory=True).items()): - value = getattr(self.inputs, name) - self._check_xor(spec, name, value) - if not isdefined(value) and spec.xor is None: - msg = ("%s requires a value for input '%s'. " - "For a list of required inputs, see %s.help()" % - (self.__class__.__name__, name, - self.__class__.__name__)) - raise ValueError(msg) - if isdefined(value): - self._check_requires(spec, name, value) - for name, spec in list( - self.inputs.traits(mandatory=None, transient=None).items()): - self._check_requires(spec, name, getattr(self.inputs, name)) - def _check_version_requirements(self, trait_object, raise_exception=True): """ Raises an exception on version mismatch """ @@ -474,7 +428,7 @@ def run(self, cwd=None, ignore_exception=None, **inputs): enable_rm = config.resource_monitor and self.resource_monitor self.inputs.trait_set(**inputs) - self._check_mandatory_inputs() + check_mandatory_inputs(self.inputs) self._check_version_requirements(self.inputs) interface = self.__class__ self._duecredit_cite() @@ -818,7 +772,12 @@ def cmd(self): def cmdline(self): """ `command` plus any arguments (args) validates arguments and generates command line""" - self._check_mandatory_inputs() + if not check_mandatory_inputs(self.inputs, raise_exc=False): + iflogger.warning( + 'Some inputs are not valid. Please make sure all mandatory ' + 'inputs, required inputs and mutually-exclusive inputs are ' + 'set or in a sane state.') + allargs = [self._cmd_prefix + self.cmd] + self._parse_inputs() return ' '.join(allargs) diff --git a/nipype/interfaces/base/specs.py b/nipype/interfaces/base/specs.py index f02d5bd709..23beca98c0 100644 --- a/nipype/interfaces/base/specs.py +++ b/nipype/interfaces/base/specs.py @@ -19,6 +19,8 @@ from packaging.version import Version from ...utils.filemanip import md5, hash_infile, hash_timestamp, to_str +from ...utils.errors import ( + MandatoryInputError, MutuallyExclusiveInputError, RequiredInputError) from .traits_extension import ( traits, Undefined, @@ -114,9 +116,8 @@ def _xor_warn(self, obj, name, old, new): trait_change_notify=False, **{ '%s' % name: Undefined }) - msg = ('Input "%s" is mutually exclusive with input "%s", ' - 'which is already set') % (name, trait_name) - raise IOError(msg) + raise MutuallyExclusiveInputError( + self, name, name_other=trait_name) def _deprecated_warn(self, obj, name, old, new): """Checks if a user assigns a value to a deprecated trait @@ -375,3 +376,61 @@ class MpiCommandLineInputSpec(CommandLineInputSpec): n_procs = traits.Int(desc="Num processors to specify to mpiexec. Do not " "specify if this is managed externally (e.g. through " "SGE)") + + +def check_requires(inputs, spec, value): + """check if required inputs are satisfied + """ + if not spec.requires: + return True + + # Check value and all required inputs' values defined + values = [isdefined(value)] + [ + isdefined(getattr(inputs, field)) + for field in spec.requires] + return all(values) + +def check_xor(inputs, spec, value): + """ check if mutually exclusive inputs are satisfied + """ + if not spec.xor: + return True + + print(inputs) + values = [isdefined(value)] + [ + isdefined(getattr(inputs, field)) for field in spec.xor] + return sum(values) + +def check_mandatory_inputs(inputs, raise_exc=True): + """ Raises an exception if a mandatory input is Undefined + """ + # Check mandatory, not xor-ed inputs. + for name, spec in list(inputs.traits(mandatory=True).items()): + value = getattr(inputs, name) + # Mandatory field is defined, check xor'ed inputs + cxor = check_xor(inputs, spec, value) + if cxor != 1: + if raise_exc: + raise MutuallyExclusiveInputError(inputs, name, cxor) + return False + + # Simplest case: no xor metadata and not defined + if cxor is True and not isdefined(value): + if raise_exc: + raise MandatoryInputError(inputs, name) + return False + + # Check whether mandatory inputs require others + if not check_requires(inputs, spec, value): + if raise_exc: + raise RequiredInputError(inputs, name) + return False + + # Check requirements of non-mandatory inputs + for name, spec in list( + inputs.traits(mandatory=None, transient=None).items()): + if not check_requires(inputs, spec, getattr(inputs, name)): + if raise_exc: + raise RequiredInputError(inputs, name) + + return True diff --git a/nipype/interfaces/base/tests/test_core.py b/nipype/interfaces/base/tests/test_core.py index 392d38706f..19c82f34a0 100644 --- a/nipype/interfaces/base/tests/test_core.py +++ b/nipype/interfaces/base/tests/test_core.py @@ -95,11 +95,6 @@ class DerivedInterface(nib.BaseInterface): assert DerivedInterface._get_filecopy_info()[1]['key'] == 'zoo' assert not DerivedInterface._get_filecopy_info()[1]['copy'] assert DerivedInterface().inputs.foo == nib.Undefined - with pytest.raises(ValueError): - DerivedInterface()._check_mandatory_inputs() - assert DerivedInterface(goo=1)._check_mandatory_inputs() is None - with pytest.raises(ValueError): - DerivedInterface().run() with pytest.raises(NotImplementedError): DerivedInterface(goo=1).run() diff --git a/nipype/interfaces/base/tests/test_specs.py b/nipype/interfaces/base/tests/test_specs.py index f08fa68adf..4bf2dbd534 100644 --- a/nipype/interfaces/base/tests/test_specs.py +++ b/nipype/interfaces/base/tests/test_specs.py @@ -2,15 +2,18 @@ # emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*- # vi: set ft=python sts=4 ts=4 sw=4 et: from __future__ import print_function, unicode_literals -from future import standard_library import os import warnings - import pytest +from future import standard_library from ....utils.filemanip import split_filename from ... import base as nib from ...base import traits, Undefined +from ..specs import ( + check_mandatory_inputs, MandatoryInputError, + MutuallyExclusiveInputError, RequiredInputError +) from ....interfaces import fsl from ...utility.wrappers import Function from ....pipeline import Node @@ -55,8 +58,8 @@ def test_TraitedSpec_tab_completion(): bet_nd = Node(fsl.BET(), name='bet') bet_interface = fsl.BET() bet_inputs = bet_nd.inputs.class_editable_traits() - bet_outputs = bet_nd.outputs.class_editable_traits() - + bet_outputs = bet_nd.outputs.class_editable_traits() + # Check __all__ for bet node and interface inputs assert set(bet_nd.inputs.__all__) == set(bet_inputs) assert set(bet_interface.inputs.__all__) == set(bet_inputs) @@ -433,3 +436,66 @@ def test_ImageFile(): with pytest.raises(nib.TraitError): x.nocompress = 'test.nii.gz' x.nocompress = 'test.mgh' + + +def test_inputs_checks(): + + class InputSpec(nib.TraitedSpec): + goo = nib.traits.Int(desc='a random int', mandatory=True) + + class DerivedInterface(nib.BaseInterface): + input_spec = InputSpec + resource_monitor = False + + assert check_mandatory_inputs( + DerivedInterface(goo=1).inputs) + with pytest.raises(MandatoryInputError): + check_mandatory_inputs( + DerivedInterface().inputs) + with pytest.raises(MandatoryInputError): + DerivedInterface().run() + + class InputSpec(nib.TraitedSpec): + goo = nib.traits.Int(desc='a random int', mandatory=True, + requires=['woo']) + woo = nib.traits.Int(desc='required by goo') + + class DerivedInterface(nib.BaseInterface): + input_spec = InputSpec + resource_monitor = False + + assert check_mandatory_inputs( + DerivedInterface(goo=1, woo=1).inputs) + with pytest.raises(RequiredInputError): + check_mandatory_inputs( + DerivedInterface(goo=1).inputs) + with pytest.raises(RequiredInputError): + DerivedInterface(goo=1).run() + + class InputSpec(nib.TraitedSpec): + goo = nib.traits.Int(desc='a random int', mandatory=True, + xor=['woo']) + woo = nib.traits.Int(desc='a random int', mandatory=True, + xor=['goo']) + + class DerivedInterface(nib.BaseInterface): + input_spec = InputSpec + resource_monitor = False + + # If either goo or woo are set, then okay! + assert check_mandatory_inputs( + DerivedInterface(goo=1).inputs) + assert check_mandatory_inputs( + DerivedInterface(woo=1).inputs) + + # None are set, raise MandatoryInputError + with pytest.raises(MutuallyExclusiveInputError): + check_mandatory_inputs( + DerivedInterface().inputs) + + # Both are set, raise MutuallyExclusiveInputError + with pytest.raises(MutuallyExclusiveInputError): + check_mandatory_inputs( + DerivedInterface(goo=1, woo=1).inputs) + with pytest.raises(MutuallyExclusiveInputError): + DerivedInterface(goo=1, woo=1).run() diff --git a/nipype/interfaces/freesurfer/preprocess.py b/nipype/interfaces/freesurfer/preprocess.py index 2941968f85..b25c7fc6f6 100644 --- a/nipype/interfaces/freesurfer/preprocess.py +++ b/nipype/interfaces/freesurfer/preprocess.py @@ -21,6 +21,7 @@ from ..base import (TraitedSpec, File, traits, Directory, InputMultiPath, OutputMultiPath, CommandLine, CommandLineInputSpec, isdefined) +from ..base.specs import check_mandatory_inputs from .base import (FSCommand, FSTraitedSpec, FSTraitedSpecOpenMP, FSCommandOpenMP, Info) from .utils import copy2subjdir @@ -634,7 +635,12 @@ def _get_filelist(self, outdir): def cmdline(self): """ `command` plus any arguments (args) validates arguments and generates command line""" - self._check_mandatory_inputs() + if not check_mandatory_inputs(self.inputs, raise_exc=False): + iflogger.warning( + 'Some inputs are not valid. Please make sure all mandatory ' + 'inputs, required inputs and mutually-exclusive inputs are ' + 'set or in a sane state.') + outdir = self._get_outdir() cmd = [] if not os.path.exists(outdir): diff --git a/nipype/utils/errors.py b/nipype/utils/errors.py new file mode 100644 index 0000000000..74cf1340c6 --- /dev/null +++ b/nipype/utils/errors.py @@ -0,0 +1,60 @@ +# -*- coding: utf-8 -*- +# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*- +# vi: set ft=python sts=4 ts=4 sw=4 et: +"""Errors and exceptions +""" +from __future__ import (print_function, division, unicode_literals, + absolute_import) + + +class MandatoryInputError(ValueError): + """Raised when one input with the ``mandatory`` metadata set to ``True`` is + not defined.""" + def __init__(self, inputspec, name): + classname = inputspec.__class__.__name__ + if classname.endswith('InputSpec'): + classname = classname[:len('InputSpec')] + msg = ( + 'Interface "{classname}" requires a value for input {name}. ' + 'For a list of required inputs, see {classname}.help().').format( + classname=classname, name=name) + super(MandatoryInputError, self).__init__(msg) + +class MutuallyExclusiveInputError(ValueError): + """Raised when none or more than one mutually-exclusive inputs are set.""" + def __init__(self, inputspec, name, values_defined=None, name_other=None): + classname = inputspec.__class__.__name__ + if classname.endswith('InputSpec') and classname != 'InputSpec': + classname = classname[:-len('InputSpec')] + + if values_defined: + xor = [name]+ inputspec.traits()[name].xor + msg = ('Interface "{classname}" has mutually-exclusive inputs. ' + 'Exactly one of ({xor}) should be set, but {n:d} were set. ' + 'For a list of mutually-exclusive inputs, see ' + '{classname}.help().').format(classname=classname, + xor='|'.join(xor), + n=values_defined) + + else: + msg = ('Interface "{classname}" has mutually-exclusive inputs. ' + 'Input "{name}" is mutually exclusive with input ' + '"{name_other}", which is already set').format( + classname=classname, name=name, name_other=name_other) + super(MutuallyExclusiveInputError, self).__init__(msg) + +class RequiredInputError(ValueError): + """Raised when one input requires some other and those or some of + those are ``Undefined``.""" + def __init__(self, inputspec, name): + classname = inputspec.__class__.__name__ + if classname.endswith('InputSpec'): + classname = classname[:-len('InputSpec')] + requires = inputspec.traits()[name].requires + + msg = ('Interface "{classname}" requires a value for input {name} ' + 'because one of ({requires}) is set. For a list of required ' + 'inputs, see {classname}.help().').format( + classname=classname, name=name, + requires=', '.join(requires)) + super(RequiredInputError, self).__init__(msg) From a15c6fb5e9786f5911f39eccbf56ad7049e0e0f7 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sat, 24 Nov 2018 21:05:50 -0800 Subject: [PATCH 23/42] remove forgotten print --- nipype/interfaces/base/specs.py | 1 - 1 file changed, 1 deletion(-) diff --git a/nipype/interfaces/base/specs.py b/nipype/interfaces/base/specs.py index 23beca98c0..107a90672c 100644 --- a/nipype/interfaces/base/specs.py +++ b/nipype/interfaces/base/specs.py @@ -396,7 +396,6 @@ def check_xor(inputs, spec, value): if not spec.xor: return True - print(inputs) values = [isdefined(value)] + [ isdefined(getattr(inputs, field)) for field in spec.xor] return sum(values) From a3600d910ba3487e138dc3a1f0532f4f21e2d005 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sat, 24 Nov 2018 22:49:04 -0800 Subject: [PATCH 24/42] fix mutually-exclusive check --- nipype/interfaces/base/specs.py | 23 ++++++++++++----------- nipype/utils/errors.py | 4 ++-- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/nipype/interfaces/base/specs.py b/nipype/interfaces/base/specs.py index 107a90672c..1e5bca3494 100644 --- a/nipype/interfaces/base/specs.py +++ b/nipype/interfaces/base/specs.py @@ -378,26 +378,25 @@ class MpiCommandLineInputSpec(CommandLineInputSpec): "SGE)") -def check_requires(inputs, spec, value): +def check_requires(inputs, requires, value): """check if required inputs are satisfied """ - if not spec.requires: + if not requires: return True # Check value and all required inputs' values defined values = [isdefined(value)] + [ isdefined(getattr(inputs, field)) - for field in spec.requires] + for field in requires] return all(values) -def check_xor(inputs, spec, value): +def check_xor(inputs, xor): """ check if mutually exclusive inputs are satisfied """ - if not spec.xor: + if len(xor) == 1: return True - values = [isdefined(value)] + [ - isdefined(getattr(inputs, field)) for field in spec.xor] + values = [isdefined(getattr(inputs, field)) for field in xor] return sum(values) def check_mandatory_inputs(inputs, raise_exc=True): @@ -407,10 +406,12 @@ def check_mandatory_inputs(inputs, raise_exc=True): for name, spec in list(inputs.traits(mandatory=True).items()): value = getattr(inputs, name) # Mandatory field is defined, check xor'ed inputs - cxor = check_xor(inputs, spec, value) + xor = list(set([name] + spec.xor)) + cxor = check_xor(inputs, xor) if cxor != 1: if raise_exc: - raise MutuallyExclusiveInputError(inputs, name, cxor) + raise MutuallyExclusiveInputError( + inputs, name, values_defined=cxor) return False # Simplest case: no xor metadata and not defined @@ -420,7 +421,7 @@ def check_mandatory_inputs(inputs, raise_exc=True): return False # Check whether mandatory inputs require others - if not check_requires(inputs, spec, value): + if not check_requires(inputs, spec.requires, value): if raise_exc: raise RequiredInputError(inputs, name) return False @@ -428,7 +429,7 @@ def check_mandatory_inputs(inputs, raise_exc=True): # Check requirements of non-mandatory inputs for name, spec in list( inputs.traits(mandatory=None, transient=None).items()): - if not check_requires(inputs, spec, getattr(inputs, name)): + if not check_requires(inputs, spec.requires, getattr(inputs, name)): if raise_exc: raise RequiredInputError(inputs, name) diff --git a/nipype/utils/errors.py b/nipype/utils/errors.py index 74cf1340c6..a55f3179f2 100644 --- a/nipype/utils/errors.py +++ b/nipype/utils/errors.py @@ -27,8 +27,8 @@ def __init__(self, inputspec, name, values_defined=None, name_other=None): if classname.endswith('InputSpec') and classname != 'InputSpec': classname = classname[:-len('InputSpec')] - if values_defined: - xor = [name]+ inputspec.traits()[name].xor + if values_defined is not None: + xor = list(set([name]+ inputspec.traits()[name].xor)) msg = ('Interface "{classname}" has mutually-exclusive inputs. ' 'Exactly one of ({xor}) should be set, but {n:d} were set. ' 'For a list of mutually-exclusive inputs, see ' From c095d65e867415341883fbcf77cc3243a20232fa Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sat, 24 Nov 2018 23:19:49 -0800 Subject: [PATCH 25/42] fix bug trying to append a spec.xor that is None --- nipype/interfaces/base/specs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nipype/interfaces/base/specs.py b/nipype/interfaces/base/specs.py index 1e5bca3494..dc476965c5 100644 --- a/nipype/interfaces/base/specs.py +++ b/nipype/interfaces/base/specs.py @@ -406,7 +406,7 @@ def check_mandatory_inputs(inputs, raise_exc=True): for name, spec in list(inputs.traits(mandatory=True).items()): value = getattr(inputs, name) # Mandatory field is defined, check xor'ed inputs - xor = list(set([name] + spec.xor)) + xor = list(set([name] + spec.xor or [])) cxor = check_xor(inputs, xor) if cxor != 1: if raise_exc: From 61d553c42c24bc27b0977cabccabccec84b2b92c Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sat, 24 Nov 2018 23:45:40 -0800 Subject: [PATCH 26/42] fix precedence --- nipype/interfaces/base/specs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nipype/interfaces/base/specs.py b/nipype/interfaces/base/specs.py index dc476965c5..a62cdec437 100644 --- a/nipype/interfaces/base/specs.py +++ b/nipype/interfaces/base/specs.py @@ -406,7 +406,7 @@ def check_mandatory_inputs(inputs, raise_exc=True): for name, spec in list(inputs.traits(mandatory=True).items()): value = getattr(inputs, name) # Mandatory field is defined, check xor'ed inputs - xor = list(set([name] + spec.xor or [])) + xor = list(set([name] + (spec.xor or []))) cxor = check_xor(inputs, xor) if cxor != 1: if raise_exc: From 1ade70c18b0b033a2fc9216a018cea38549846ea Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sun, 25 Nov 2018 00:42:48 -0800 Subject: [PATCH 27/42] outsource ``_check_version_requirements`` --- nipype/interfaces/base/core.py | 50 +------- nipype/interfaces/base/specs.py | 51 ++++++++- nipype/interfaces/base/tests/test_core.py | 127 +-------------------- nipype/interfaces/base/tests/test_specs.py | 90 ++++++++++++++- nipype/utils/errors.py | 28 ++++- 5 files changed, 171 insertions(+), 175 deletions(-) diff --git a/nipype/interfaces/base/core.py b/nipype/interfaces/base/core.py index 06ef43c7cf..dcc4580191 100644 --- a/nipype/interfaces/base/core.py +++ b/nipype/interfaces/base/core.py @@ -29,7 +29,7 @@ import simplejson as json from dateutil.parser import parse as parseutc -from ... import config, logging, LooseVersion +from ... import config, logging from ...utils.provenance import write_provenance from ...utils.misc import trim, str2bool, rgetcwd from ...utils.filemanip import (FileNotFoundError, split_filename, @@ -41,7 +41,7 @@ from .traits_extension import traits, isdefined, TraitError from .specs import (BaseInterfaceInputSpec, CommandLineInputSpec, StdOutCommandLineInputSpec, MpiCommandLineInputSpec, - check_mandatory_inputs) + check_mandatory_inputs, check_version) from .support import (Bunch, InterfaceResult, NipypeInterfaceError) from future import standard_library @@ -344,46 +344,6 @@ def _get_filecopy_info(cls): info.append(dict(key=name, copy=spec.copyfile)) return info - def _check_version_requirements(self, trait_object, raise_exception=True): - """ Raises an exception on version mismatch - """ - unavailable_traits = [] - # check minimum version - check = dict(min_ver=lambda t: t is not None) - names = trait_object.trait_names(**check) - - if names and self.version: - version = LooseVersion(str(self.version)) - for name in names: - min_ver = LooseVersion( - str(trait_object.traits()[name].min_ver)) - if min_ver > version: - unavailable_traits.append(name) - if not isdefined(getattr(trait_object, name)): - continue - if raise_exception: - raise Exception( - 'Trait %s (%s) (version %s < required %s)' % - (name, self.__class__.__name__, version, min_ver)) - - # check maximum version - check = dict(max_ver=lambda t: t is not None) - names = trait_object.trait_names(**check) - if names and self.version: - version = LooseVersion(str(self.version)) - for name in names: - max_ver = LooseVersion( - str(trait_object.traits()[name].max_ver)) - if max_ver < version: - unavailable_traits.append(name) - if not isdefined(getattr(trait_object, name)): - continue - if raise_exception: - raise Exception( - 'Trait %s (%s) (version %s > required %s)' % - (name, self.__class__.__name__, version, max_ver)) - return unavailable_traits - def _run_interface(self, runtime): """ Core function that executes interface """ @@ -429,7 +389,7 @@ def run(self, cwd=None, ignore_exception=None, **inputs): enable_rm = config.resource_monitor and self.resource_monitor self.inputs.trait_set(**inputs) check_mandatory_inputs(self.inputs) - self._check_version_requirements(self.inputs) + check_version(self.inputs, version=self.version) interface = self.__class__ self._duecredit_cite() @@ -555,8 +515,8 @@ def aggregate_outputs(self, runtime=None, needed_outputs=None): if predicted_outputs: _unavailable_outputs = [] if outputs: - _unavailable_outputs = \ - self._check_version_requirements(self._outputs()) + _unavailable_outputs = check_version( + self._outputs(), self.version) for key, val in list(predicted_outputs.items()): if needed_outputs and key not in needed_outputs: continue diff --git a/nipype/interfaces/base/specs.py b/nipype/interfaces/base/specs.py index a62cdec437..c6e860ff51 100644 --- a/nipype/interfaces/base/specs.py +++ b/nipype/interfaces/base/specs.py @@ -20,7 +20,8 @@ from ...utils.filemanip import md5, hash_infile, hash_timestamp, to_str from ...utils.errors import ( - MandatoryInputError, MutuallyExclusiveInputError, RequiredInputError) + MandatoryInputError, MutuallyExclusiveInputError, RequiredInputError, + VersionIOError) from .traits_extension import ( traits, Undefined, @@ -434,3 +435,51 @@ def check_mandatory_inputs(inputs, raise_exc=True): raise RequiredInputError(inputs, name) return True + +def check_version(traited_spec, version=None, raise_exc=True): + """ Raises an exception on version mismatch + """ + + # no version passed on to check against + if not version: + return [] + + # check minimum version + names = traited_spec.trait_names( + min_ver=lambda t: t is not None) + \ + traited_spec.trait_names( + max_ver=lambda t: t is not None) + + # no traits defined any versions + if not names: + return [] + + version = Version(str(version)) + unavailable_traits = [] + for name in names: + value_set = isdefined(getattr(traited_spec, name)) + min_ver = traited_spec.traits()[name].min_ver + if min_ver: + min_ver = Version(str(min_ver)) + + max_ver = traited_spec.traits()[name].max_ver + if max_ver: + max_ver = Version(str(max_ver)) + + if min_ver and max_ver: + if max_ver < min_ver: + raise AssertionError( + 'Trait "%s" (%s) has incongruent version metadata ' + '(``max_ver`` is lower than ``min_ver``).' % ( + traited_spec.__class__.__name__, name)) + + if min_ver and (min_ver > version): + unavailable_traits.append(name) + if value_set and raise_exc: + raise VersionIOError(traited_spec, name, version) + if max_ver and (max_ver < version): + unavailable_traits.append(name) + if value_set and raise_exc: + raise VersionIOError(traited_spec, name, version) + + return list(set(unavailable_traits)) diff --git a/nipype/interfaces/base/tests/test_core.py b/nipype/interfaces/base/tests/test_core.py index 19c82f34a0..cba88be0a5 100644 --- a/nipype/interfaces/base/tests/test_core.py +++ b/nipype/interfaces/base/tests/test_core.py @@ -172,136 +172,15 @@ def __init__(self, **inputs): assert '8562a5623562a871115eb14822ee8d02' == hashvalue -class MinVerInputSpec(nib.TraitedSpec): - foo = nib.traits.Int(desc='a random int', min_ver='0.9') - -class MaxVerInputSpec(nib.TraitedSpec): - foo = nib.traits.Int(desc='a random int', max_ver='0.7') - - -def test_input_version_1(): - class DerivedInterface1(nib.BaseInterface): - input_spec = MinVerInputSpec - - obj = DerivedInterface1() - obj._check_version_requirements(obj.inputs) - +def test_stop_on_unknown_version(): config.set('execution', 'stop_on_unknown_version', True) + ci = nib.CommandLine(command='which') with pytest.raises(ValueError) as excinfo: - obj._check_version_requirements(obj.inputs) + _ = ci.version assert "no version information" in str(excinfo.value) - config.set_default_config() - -def test_input_version_2(): - class DerivedInterface1(nib.BaseInterface): - input_spec = MinVerInputSpec - _version = '0.8' - - obj = DerivedInterface1() - obj.inputs.foo = 1 - with pytest.raises(Exception) as excinfo: - obj._check_version_requirements(obj.inputs) - assert "version 0.8 < required 0.9" in str(excinfo.value) - - -def test_input_version_3(): - class DerivedInterface1(nib.BaseInterface): - input_spec = MinVerInputSpec - _version = '0.10' - - obj = DerivedInterface1() - obj._check_version_requirements(obj.inputs) - - -def test_input_version_4(): - class DerivedInterface1(nib.BaseInterface): - input_spec = MinVerInputSpec - _version = '0.9' - - obj = DerivedInterface1() - obj.inputs.foo = 1 - obj._check_version_requirements(obj.inputs) - - -def test_input_version_5(): - class DerivedInterface2(nib.BaseInterface): - input_spec = MaxVerInputSpec - _version = '0.8' - - obj = DerivedInterface2() - obj.inputs.foo = 1 - with pytest.raises(Exception) as excinfo: - obj._check_version_requirements(obj.inputs) - assert "version 0.8 > required 0.7" in str(excinfo.value) - - -def test_input_version_6(): - class DerivedInterface1(nib.BaseInterface): - input_spec = MaxVerInputSpec - _version = '0.7' - - obj = DerivedInterface1() - obj.inputs.foo = 1 - obj._check_version_requirements(obj.inputs) - - -def test_output_version(): - class InputSpec(nib.TraitedSpec): - foo = nib.traits.Int(desc='a random int') - - class OutputSpec(nib.TraitedSpec): - foo = nib.traits.Int(desc='a random int', min_ver='0.9') - - class DerivedInterface1(nib.BaseInterface): - input_spec = InputSpec - output_spec = OutputSpec - _version = '0.10' - resource_monitor = False - - obj = DerivedInterface1() - assert obj._check_version_requirements(obj._outputs()) == [] - - class InputSpec(nib.TraitedSpec): - foo = nib.traits.Int(desc='a random int') - - class OutputSpec(nib.TraitedSpec): - foo = nib.traits.Int(desc='a random int', min_ver='0.11') - - class DerivedInterface1(nib.BaseInterface): - input_spec = InputSpec - output_spec = OutputSpec - _version = '0.10' - resource_monitor = False - - obj = DerivedInterface1() - assert obj._check_version_requirements(obj._outputs()) == ['foo'] - - class InputSpec(nib.TraitedSpec): - foo = nib.traits.Int(desc='a random int') - - class OutputSpec(nib.TraitedSpec): - foo = nib.traits.Int(desc='a random int', min_ver='0.11') - - class DerivedInterface1(nib.BaseInterface): - input_spec = InputSpec - output_spec = OutputSpec - _version = '0.10' - resource_monitor = False - - def _run_interface(self, runtime): - return runtime - - def _list_outputs(self): - return {'foo': 1} - - obj = DerivedInterface1() - with pytest.raises(KeyError): - obj.run() - - def test_Commandline(): with pytest.raises(Exception): nib.CommandLine() diff --git a/nipype/interfaces/base/tests/test_specs.py b/nipype/interfaces/base/tests/test_specs.py index 4bf2dbd534..9e3fba03de 100644 --- a/nipype/interfaces/base/tests/test_specs.py +++ b/nipype/interfaces/base/tests/test_specs.py @@ -11,8 +11,9 @@ from ... import base as nib from ...base import traits, Undefined from ..specs import ( - check_mandatory_inputs, MandatoryInputError, - MutuallyExclusiveInputError, RequiredInputError + check_mandatory_inputs, check_version, + MandatoryInputError, MutuallyExclusiveInputError, + RequiredInputError, VersionIOError ) from ....interfaces import fsl from ...utility.wrappers import Function @@ -137,7 +138,7 @@ class MyInterface(nib.BaseInterface): myif.inputs.foo = 1 assert myif.inputs.foo == 1 set_bar = lambda: setattr(myif.inputs, 'bar', 1) - with pytest.raises(IOError): + with pytest.raises(MutuallyExclusiveInputError): set_bar() assert myif.inputs.foo == 1 myif.inputs.kung = 2 @@ -499,3 +500,86 @@ class DerivedInterface(nib.BaseInterface): DerivedInterface(goo=1, woo=1).inputs) with pytest.raises(MutuallyExclusiveInputError): DerivedInterface(goo=1, woo=1).run() + + + +def test_input_version(): + class MinVerInputSpec(nib.TraitedSpec): + foo = nib.traits.Int(desc='a random int', min_ver='0.5') + + + assert check_version(MinVerInputSpec(), '0.6') == [] + assert check_version(MinVerInputSpec(), '0.4') == ['foo'] + with pytest.raises(VersionIOError): + check_version(MinVerInputSpec(foo=1), '0.4') + + + class MaxVerInputSpec(nib.TraitedSpec): + foo = nib.traits.Int(desc='a random int', max_ver='0.7') + + + assert check_version(MaxVerInputSpec(), '0.6') == [] + assert check_version(MaxVerInputSpec(), '0.8') == ['foo'] + with pytest.raises(VersionIOError): + check_version(MaxVerInputSpec(foo=1), '0.8') + + + class MinMaxVerInputSpec(nib.TraitedSpec): + foo = nib.traits.Int(desc='a random int', max_ver='0.7', + min_ver='0.5') + + + assert check_version(MinMaxVerInputSpec(), '0.6') == [] + assert check_version(MinMaxVerInputSpec(), '0.4') == ['foo'] + assert check_version(MinMaxVerInputSpec(), '0.8') == ['foo'] + with pytest.raises(VersionIOError): + check_version(MinMaxVerInputSpec(foo=1), '0.8') + with pytest.raises(VersionIOError): + check_version(MinMaxVerInputSpec(foo=1), '0.4') + + + class FixedVerInputSpec(nib.TraitedSpec): + foo = nib.traits.Int(desc='a random int', max_ver='0.6.2', + min_ver='0.6.2') + + + assert check_version(FixedVerInputSpec(), '0.6.2') == [] + assert check_version(FixedVerInputSpec(), '0.6.1') == ['foo'] + assert check_version(FixedVerInputSpec(), '0.6.3') == ['foo'] + with pytest.raises(VersionIOError): + check_version(FixedVerInputSpec(foo=1), '0.6.1') + with pytest.raises(VersionIOError): + check_version(FixedVerInputSpec(foo=1), '0.6.3') + + + class IncongruentVerInputSpec(nib.TraitedSpec): + foo = nib.traits.Int(desc='a random int', max_ver='0.5', + min_ver='0.7') + + with pytest.raises(AssertionError): + check_version(IncongruentVerInputSpec(), '0.6') + with pytest.raises(AssertionError): + check_version(IncongruentVerInputSpec(foo=1), '0.6') + + + class InputSpec(nib.TraitedSpec): + foo = nib.traits.Int(desc='a random int') + + class OutputSpec(nib.TraitedSpec): + foo = nib.traits.Int(desc='a random int', min_ver='0.11') + + class DerivedInterface1(nib.BaseInterface): + input_spec = InputSpec + output_spec = OutputSpec + _version = '0.10' + resource_monitor = False + + def _run_interface(self, runtime): + return runtime + + def _list_outputs(self): + return {'foo': 1} + + obj = DerivedInterface1() + with pytest.raises(KeyError): + obj.run() diff --git a/nipype/utils/errors.py b/nipype/utils/errors.py index a55f3179f2..c1ceb38b5c 100644 --- a/nipype/utils/errors.py +++ b/nipype/utils/errors.py @@ -12,7 +12,7 @@ class MandatoryInputError(ValueError): not defined.""" def __init__(self, inputspec, name): classname = inputspec.__class__.__name__ - if classname.endswith('InputSpec'): + if classname.endswith('InputSpec') and classname != 'InputSpec': classname = classname[:len('InputSpec')] msg = ( 'Interface "{classname}" requires a value for input {name}. ' @@ -48,7 +48,7 @@ class RequiredInputError(ValueError): those are ``Undefined``.""" def __init__(self, inputspec, name): classname = inputspec.__class__.__name__ - if classname.endswith('InputSpec'): + if classname.endswith('InputSpec') and classname != 'InputSpec': classname = classname[:-len('InputSpec')] requires = inputspec.traits()[name].requires @@ -58,3 +58,27 @@ def __init__(self, inputspec, name): classname=classname, name=name, requires=', '.join(requires)) super(RequiredInputError, self).__init__(msg) + +class VersionIOError(ValueError): + """Raised when one input with the ``mandatory`` metadata set to ``True`` is + not defined.""" + def __init__(self, spec, name, version): + classname = spec.__class__.__name__ + if classname.endswith('InputSpec') and classname != 'InputSpec': + classname = classname[:len('InputSpec')] + if classname.endswith('OutputSpec') and classname != 'OutputSpec': + classname = classname[:len('OutputSpec')] + + max_ver = spec.traits()[name].max_ver + min_ver = spec.traits()[name].min_ver + + msg = ('Interface "{classname}" has version requirements for ' + '{name}, but version {version} was found. ').format( + classname=classname, name=name, version=version) + + if min_ver: + msg += 'Minimum version is %s. ' % min_ver + if max_ver: + msg += 'Maximum version is %s. ' % max_ver + + super(VersionIOError, self).__init__(msg) From 94c08234de3dec4309bc1614d653e9e79602a4b5 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sun, 25 Nov 2018 01:38:04 -0800 Subject: [PATCH 28/42] fix multiple xor check --- nipype/interfaces/base/specs.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/nipype/interfaces/base/specs.py b/nipype/interfaces/base/specs.py index c6e860ff51..4da090c8c5 100644 --- a/nipype/interfaces/base/specs.py +++ b/nipype/interfaces/base/specs.py @@ -397,7 +397,9 @@ def check_xor(inputs, xor): if len(xor) == 1: return True - values = [isdefined(getattr(inputs, field)) for field in xor] + values = [isdefined(getattr(inputs, xor[0]))] + values += [any([isdefined(getattr(inputs, field)) + for field in xor[1:]])] return sum(values) def check_mandatory_inputs(inputs, raise_exc=True): From fddec6968caddb227627ee686b4ab992a8d71e28 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sun, 25 Nov 2018 01:55:42 -0800 Subject: [PATCH 29/42] fix check_requires for non-mandatory requires --- nipype/interfaces/base/specs.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/nipype/interfaces/base/specs.py b/nipype/interfaces/base/specs.py index 4da090c8c5..2b130463f0 100644 --- a/nipype/interfaces/base/specs.py +++ b/nipype/interfaces/base/specs.py @@ -386,9 +386,8 @@ def check_requires(inputs, requires, value): return True # Check value and all required inputs' values defined - values = [isdefined(value)] + [ - isdefined(getattr(inputs, field)) - for field in requires] + values = [isdefined(getattr(inputs, field)) + for field in requires] return all(values) def check_xor(inputs, xor): @@ -432,7 +431,8 @@ def check_mandatory_inputs(inputs, raise_exc=True): # Check requirements of non-mandatory inputs for name, spec in list( inputs.traits(mandatory=None, transient=None).items()): - if not check_requires(inputs, spec.requires, getattr(inputs, name)): + value = getattr(inputs, name) # value must be set to follow requires + if isdefined(value) and not check_requires(inputs, spec.requires, value): if raise_exc: raise RequiredInputError(inputs, name) From 34cb9c9e79e15f73da9399dba6cf3a9e94430232 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sun, 25 Nov 2018 02:39:44 -0800 Subject: [PATCH 30/42] fix massaging xored inputs --- nipype/interfaces/base/specs.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/nipype/interfaces/base/specs.py b/nipype/interfaces/base/specs.py index 2b130463f0..f65ffd4008 100644 --- a/nipype/interfaces/base/specs.py +++ b/nipype/interfaces/base/specs.py @@ -408,7 +408,10 @@ def check_mandatory_inputs(inputs, raise_exc=True): for name, spec in list(inputs.traits(mandatory=True).items()): value = getattr(inputs, name) # Mandatory field is defined, check xor'ed inputs - xor = list(set([name] + (spec.xor or []))) + xor = spec.xor or [] + xor = list(xor) if isinstance(xor, (list, tuple)) \ + else [xor] + xor = list(set([name] + xor)) cxor = check_xor(inputs, xor) if cxor != 1: if raise_exc: From 41a48bdd946adea929eaeb60897c3bfc3257c7c7 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sun, 25 Nov 2018 08:10:10 -0800 Subject: [PATCH 31/42] fix test_extra_Registration --- .../ants/tests/test_extra_Registration.py | 7 ++-- nipype/utils/errors.py | 33 ++++++++++--------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/nipype/interfaces/ants/tests/test_extra_Registration.py b/nipype/interfaces/ants/tests/test_extra_Registration.py index 745b825c65..c65bb445be 100644 --- a/nipype/interfaces/ants/tests/test_extra_Registration.py +++ b/nipype/interfaces/ants/tests/test_extra_Registration.py @@ -1,9 +1,10 @@ # emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*- # vi: set ft=python sts=4 ts=4 sw=4 et: from __future__ import unicode_literals -from nipype.interfaces.ants import registration import os import pytest +from nipype.interfaces.ants import registration +from nipype.utils.errors import MandatoryInputError def test_ants_mand(tmpdir): @@ -17,6 +18,6 @@ def test_ants_mand(tmpdir): ants.inputs.fixed_image = [os.path.join(datadir, 'T1.nii')] ants.inputs.metric = ['MI'] - with pytest.raises(ValueError) as er: + with pytest.raises(MandatoryInputError) as er: ants.run() - assert "ANTS requires a value for input 'radius'" in str(er.value) + assert 'Interface "ANTS" requires a value for input radius.' in str(er.value) diff --git a/nipype/utils/errors.py b/nipype/utils/errors.py index c1ceb38b5c..18c4a25116 100644 --- a/nipype/utils/errors.py +++ b/nipype/utils/errors.py @@ -11,9 +11,7 @@ class MandatoryInputError(ValueError): """Raised when one input with the ``mandatory`` metadata set to ``True`` is not defined.""" def __init__(self, inputspec, name): - classname = inputspec.__class__.__name__ - if classname.endswith('InputSpec') and classname != 'InputSpec': - classname = classname[:len('InputSpec')] + classname = _classname_from_spec(inputspec) msg = ( 'Interface "{classname}" requires a value for input {name}. ' 'For a list of required inputs, see {classname}.help().').format( @@ -23,9 +21,7 @@ def __init__(self, inputspec, name): class MutuallyExclusiveInputError(ValueError): """Raised when none or more than one mutually-exclusive inputs are set.""" def __init__(self, inputspec, name, values_defined=None, name_other=None): - classname = inputspec.__class__.__name__ - if classname.endswith('InputSpec') and classname != 'InputSpec': - classname = classname[:-len('InputSpec')] + classname = _classname_from_spec(inputspec) if values_defined is not None: xor = list(set([name]+ inputspec.traits()[name].xor)) @@ -47,9 +43,7 @@ class RequiredInputError(ValueError): """Raised when one input requires some other and those or some of those are ``Undefined``.""" def __init__(self, inputspec, name): - classname = inputspec.__class__.__name__ - if classname.endswith('InputSpec') and classname != 'InputSpec': - classname = classname[:-len('InputSpec')] + classname = _classname_from_spec(inputspec) requires = inputspec.traits()[name].requires msg = ('Interface "{classname}" requires a value for input {name} ' @@ -63,12 +57,7 @@ class VersionIOError(ValueError): """Raised when one input with the ``mandatory`` metadata set to ``True`` is not defined.""" def __init__(self, spec, name, version): - classname = spec.__class__.__name__ - if classname.endswith('InputSpec') and classname != 'InputSpec': - classname = classname[:len('InputSpec')] - if classname.endswith('OutputSpec') and classname != 'OutputSpec': - classname = classname[:len('OutputSpec')] - + classname = _classname_from_spec(spec) max_ver = spec.traits()[name].max_ver min_ver = spec.traits()[name].min_ver @@ -82,3 +71,17 @@ def __init__(self, spec, name, version): msg += 'Maximum version is %s. ' % max_ver super(VersionIOError, self).__init__(msg) + +def _classname_from_spec(spec): + classname = spec.__class__.__name__ + + kind = 'Output' if 'Output' in classname else 'Input' + # General pattern is that spec ends in KindSpec + if classname.endswith(kind + 'Spec') and classname != (kind + 'Spec'): + classname = classname[:-len(kind + 'Spec')] + + # Catch some special cases such as ANTS + if classname.endswith(kind) and classname != kind: + classname = classname[:-len(kind)] + + return classname From 8f8ceb178e4115401630983c285342a7c11129e5 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sun, 25 Nov 2018 08:21:47 -0800 Subject: [PATCH 32/42] fixed fsl.utils doctests --- nipype/interfaces/fsl/utils.py | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/nipype/interfaces/fsl/utils.py b/nipype/interfaces/fsl/utils.py index f4ef73c0e9..45d09b53e6 100644 --- a/nipype/interfaces/fsl/utils.py +++ b/nipype/interfaces/fsl/utils.py @@ -238,16 +238,31 @@ class Smooth(FSLCommand): >>> sm.cmdline # doctest: +ELLIPSIS 'fslmaths functional2.nii -kernel gauss 3.397 -fmean functional2_smooth.nii.gz' - One of sigma or fwhm must be set: + One of sigma or fwhm must be set. Printing the ``cmdline`` will issue + a warning: >>> from nipype.interfaces.fsl import Smooth >>> sm = Smooth() >>> sm.inputs.output_type = 'NIFTI_GZ' >>> sm.inputs.in_file = 'functional2.nii' - >>> sm.cmdline #doctest: +ELLIPSIS + >>> sm.cmdline # doctest: +ELLIPSIS + 'fslmaths functional2.nii functional2_smooth.nii.gz' + + The warning is: :: + 181125-08:12:09,489 nipype.interface WARNING: + Some inputs are not valid. Please make sure all mandatory + inputs, required inputs and mutually-exclusive inputs are + set or in a sane state. + + Attempting to run the interface without the necessary inputs will + lead to an error (in this case, a ``MutuallyExclusiveInputError``): + + >>> sm.run() # doctest: +ELLIPSIS Traceback (most recent call last): ... - ValueError: Smooth requires a value for one of the inputs ... + nipype.utils.errors.MutuallyExclusiveInputError: \ + Interface "Smooth" has mutually-exclusive inputs. \ + Exactly one of (fwhm|sigma) should be set... """ From d0c6ab852d1fd566e45ad52080abe84f8df7da78 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sun, 25 Nov 2018 08:52:22 -0800 Subject: [PATCH 33/42] cmdline returns ``None`` instead of raising if error on inputs --- nipype/interfaces/base/core.py | 8 ++- .../freesurfer/tests/test_preprocess.py | 70 ++++++++++++------- nipype/interfaces/fsl/utils.py | 8 +-- 3 files changed, 52 insertions(+), 34 deletions(-) diff --git a/nipype/interfaces/base/core.py b/nipype/interfaces/base/core.py index 335c6fecae..77f1991df6 100644 --- a/nipype/interfaces/base/core.py +++ b/nipype/interfaces/base/core.py @@ -726,9 +726,11 @@ def cmdline(self): validates arguments and generates command line""" if not check_mandatory_inputs(self.inputs, raise_exc=False): iflogger.warning( - 'Some inputs are not valid. Please make sure all mandatory ' - 'inputs, required inputs and mutually-exclusive inputs are ' - 'set or in a sane state.') + 'Command line could not be generated because some inputs ' + 'are not valid. Please make sure all mandatory inputs, ' + 'required inputs and mutually-exclusive inputs are set ' + 'or in a sane state.') + return None allargs = [self._cmd_prefix + self.cmd] + self._parse_inputs() return ' '.join(allargs) diff --git a/nipype/interfaces/freesurfer/tests/test_preprocess.py b/nipype/interfaces/freesurfer/tests/test_preprocess.py index f9fc09515a..6853bb7015 100644 --- a/nipype/interfaces/freesurfer/tests/test_preprocess.py +++ b/nipype/interfaces/freesurfer/tests/test_preprocess.py @@ -7,24 +7,26 @@ import pytest from nipype.testing.fixtures import create_files_in_directory -from nipype.interfaces import freesurfer +from nipype.interfaces import freesurfer as fs from nipype.interfaces.freesurfer import Info from nipype import LooseVersion +from nipype.utils import errors as nue @pytest.mark.skipif( - freesurfer.no_freesurfer(), reason="freesurfer is not installed") + fs.no_freesurfer(), reason="freesurfer is not installed") def test_robustregister(create_files_in_directory): filelist, outdir = create_files_in_directory - reg = freesurfer.RobustRegister() + reg = fs.RobustRegister() cwd = os.getcwd() # make sure command gets called assert reg.cmd == 'mri_robust_register' + assert reg.cmdline is None # test raising error with mandatory args absent - with pytest.raises(ValueError): + with pytest.raises(nue.MandatoryInputError): reg.run() # .inputs based parameters setting @@ -36,7 +38,7 @@ def test_robustregister(create_files_in_directory): (cwd, filelist[0][:-4], filelist[0], filelist[1])) # constructor based parameter setting - reg2 = freesurfer.RobustRegister( + reg2 = fs.RobustRegister( source_file=filelist[0], target_file=filelist[1], outlier_sens=3.0, @@ -49,17 +51,18 @@ def test_robustregister(create_files_in_directory): @pytest.mark.skipif( - freesurfer.no_freesurfer(), reason="freesurfer is not installed") + fs.no_freesurfer(), reason="freesurfer is not installed") def test_fitmsparams(create_files_in_directory): filelist, outdir = create_files_in_directory - fit = freesurfer.FitMSParams() + fit = fs.FitMSParams() # make sure command gets called assert fit.cmd == 'mri_ms_fitparms' + assert fit.cmdline is None # test raising error with mandatory args absent - with pytest.raises(ValueError): + with pytest.raises(nue.MandatoryInputError): fit.run() # .inputs based parameters setting @@ -69,7 +72,7 @@ def test_fitmsparams(create_files_in_directory): filelist[1], outdir) # constructor based parameter setting - fit2 = freesurfer.FitMSParams( + fit2 = fs.FitMSParams( in_files=filelist, te_list=[1.5, 3.5], flip_list=[20, 30], @@ -80,17 +83,18 @@ def test_fitmsparams(create_files_in_directory): @pytest.mark.skipif( - freesurfer.no_freesurfer(), reason="freesurfer is not installed") + fs.no_freesurfer(), reason="freesurfer is not installed") def test_synthesizeflash(create_files_in_directory): filelist, outdir = create_files_in_directory - syn = freesurfer.SynthesizeFLASH() + syn = fs.SynthesizeFLASH() # make sure command gets called assert syn.cmd == 'mri_synthesize' + assert syn.cmdline is None # test raising error with mandatory args absent - with pytest.raises(ValueError): + with pytest.raises(nue.MandatoryInputError): syn.run() # .inputs based parameters setting @@ -105,7 +109,7 @@ def test_synthesizeflash(create_files_in_directory): os.path.join(outdir, 'synth-flash_30.mgz'))) # constructor based parameters setting - syn2 = freesurfer.SynthesizeFLASH( + syn2 = fs.SynthesizeFLASH( t1_image=filelist[0], pd_image=filelist[1], flip_angle=20, te=5, tr=25) assert syn2.cmdline == ('mri_synthesize 25.00 20.00 5.000 %s %s %s' % (filelist[0], filelist[1], @@ -113,16 +117,17 @@ def test_synthesizeflash(create_files_in_directory): @pytest.mark.skipif( - freesurfer.no_freesurfer(), reason="freesurfer is not installed") + fs.no_freesurfer(), reason="freesurfer is not installed") def test_mandatory_outvol(create_files_in_directory): filelist, outdir = create_files_in_directory - mni = freesurfer.MNIBiasCorrection() + mni = fs.MNIBiasCorrection() # make sure command gets called assert mni.cmd == "mri_nu_correct.mni" + assert mni.cmdline is None # test raising error with mandatory args absent - with pytest.raises(ValueError): + with pytest.raises(nue.MandatoryInputError): mni.cmdline # test with minimal args @@ -141,7 +146,7 @@ def test_mandatory_outvol(create_files_in_directory): 'mri_nu_correct.mni --i %s --n 4 --o new_corrected_file.mgz' % (filelist[0])) # constructor based tests - mni2 = freesurfer.MNIBiasCorrection( + mni2 = fs.MNIBiasCorrection( in_file=filelist[0], out_file='bias_corrected_output', iterations=2) assert mni2.cmdline == ( 'mri_nu_correct.mni --i %s --n 2 --o bias_corrected_output' % @@ -149,17 +154,24 @@ def test_mandatory_outvol(create_files_in_directory): @pytest.mark.skipif( - freesurfer.no_freesurfer(), reason="freesurfer is not installed") -def test_bbregister(create_files_in_directory): + fs.no_freesurfer(), reason="freesurfer is not installed") +def test_bbregister(capsys, create_files_in_directory): filelist, outdir = create_files_in_directory - bbr = freesurfer.BBRegister() + bbr = fs.BBRegister() # make sure command gets called assert bbr.cmd == "bbregister" + # cmdline issues a warning in this stats + assert bbr.cmdline is None + captured = capsys.readouterr() + + assert 'WARNING' in captured.out + assert 'Some inputs are not valid.' in captured.out + # test raising error with mandatory args absent - with pytest.raises(ValueError): - bbr.cmdline + with pytest.raises(nue.MandatoryInputError): + bbr.run() bbr.inputs.subject_id = 'fsaverage' bbr.inputs.source_file = filelist[0] @@ -167,10 +179,14 @@ def test_bbregister(create_files_in_directory): # Check that 'init' is mandatory in FS < 6, but not in 6+ if Info.looseversion() < LooseVersion("6.0.0"): - with pytest.raises(ValueError): - bbr.cmdline + assert bbr.cmdline is None + captured = capsys.readouterr() + assert 'Some inputs are not valid.' in captured.out + + with pytest.raises(nue.VersionIOError): + bbr.run() else: - bbr.cmdline + assert bbr.cmdline is not None bbr.inputs.init = 'fsl' @@ -187,5 +203,5 @@ def test_bbregister(create_files_in_directory): def test_FSVersion(): """Check that FSVersion is a string that can be compared with LooseVersion """ - assert isinstance(freesurfer.preprocess.FSVersion, str) - assert LooseVersion(freesurfer.preprocess.FSVersion) >= LooseVersion("0") + assert isinstance(fs.preprocess.FSVersion, str) + assert LooseVersion(fs.preprocess.FSVersion) >= LooseVersion("0") diff --git a/nipype/interfaces/fsl/utils.py b/nipype/interfaces/fsl/utils.py index 45d09b53e6..fac48dcec5 100644 --- a/nipype/interfaces/fsl/utils.py +++ b/nipype/interfaces/fsl/utils.py @@ -238,15 +238,15 @@ class Smooth(FSLCommand): >>> sm.cmdline # doctest: +ELLIPSIS 'fslmaths functional2.nii -kernel gauss 3.397 -fmean functional2_smooth.nii.gz' - One of sigma or fwhm must be set. Printing the ``cmdline`` will issue - a warning: + One of sigma or fwhm must be set. Accessing the ``cmdline`` property + will return ``None`` and issue a warning: >>> from nipype.interfaces.fsl import Smooth >>> sm = Smooth() >>> sm.inputs.output_type = 'NIFTI_GZ' >>> sm.inputs.in_file = 'functional2.nii' - >>> sm.cmdline # doctest: +ELLIPSIS - 'fslmaths functional2.nii functional2_smooth.nii.gz' + >>> sm.cmdline is None # doctest: +ELLIPSIS + True The warning is: :: 181125-08:12:09,489 nipype.interface WARNING: From 1b948cfedf02202b0e2d2b8ffff2c1bd1fd48883 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sun, 25 Nov 2018 11:35:43 -0800 Subject: [PATCH 34/42] fix one more use-case --- nipype/interfaces/base/specs.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/nipype/interfaces/base/specs.py b/nipype/interfaces/base/specs.py index 96289f47a0..434cf0e6fa 100644 --- a/nipype/interfaces/base/specs.py +++ b/nipype/interfaces/base/specs.py @@ -427,6 +427,15 @@ def check_mandatory_inputs(inputs, raise_exc=True): value = getattr(inputs, name) # Mandatory field is defined, check xor'ed inputs xor = spec.xor or [] + has_xor = bool(xor) + has_value = isdefined(value) + + # Simplest case: no xor metadata and not defined + if not has_xor and not has_value: + if raise_exc: + raise MandatoryInputError(inputs, name) + return False + xor = list(xor) if isinstance(xor, (list, tuple)) \ else [xor] xor = list(set([name] + xor)) @@ -437,14 +446,8 @@ def check_mandatory_inputs(inputs, raise_exc=True): inputs, name, values_defined=cxor) return False - # Simplest case: no xor metadata and not defined - if cxor is True and not isdefined(value): - if raise_exc: - raise MandatoryInputError(inputs, name) - return False - # Check whether mandatory inputs require others - if not check_requires(inputs, spec.requires): + if has_value and not check_requires(inputs, spec.requires): if raise_exc: raise RequiredInputError(inputs, name) return False From 1a558a4d0250191afec8f13927810cb863c1f51e Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Sun, 25 Nov 2018 11:48:35 -0800 Subject: [PATCH 35/42] fixed tests --- .../freesurfer/tests/test_preprocess.py | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/nipype/interfaces/freesurfer/tests/test_preprocess.py b/nipype/interfaces/freesurfer/tests/test_preprocess.py index 6853bb7015..3d908914b8 100644 --- a/nipype/interfaces/freesurfer/tests/test_preprocess.py +++ b/nipype/interfaces/freesurfer/tests/test_preprocess.py @@ -11,6 +11,7 @@ from nipype.interfaces.freesurfer import Info from nipype import LooseVersion from nipype.utils import errors as nue +from nipype.utils.tests.test_cmd import capture_sys_output @pytest.mark.skipif( @@ -128,7 +129,7 @@ def test_mandatory_outvol(create_files_in_directory): assert mni.cmdline is None # test raising error with mandatory args absent with pytest.raises(nue.MandatoryInputError): - mni.cmdline + mni.run() # test with minimal args mni.inputs.in_file = filelist[0] @@ -155,7 +156,7 @@ def test_mandatory_outvol(create_files_in_directory): @pytest.mark.skipif( fs.no_freesurfer(), reason="freesurfer is not installed") -def test_bbregister(capsys, create_files_in_directory): +def test_bbregister(create_files_in_directory): filelist, outdir = create_files_in_directory bbr = fs.BBRegister() @@ -163,11 +164,12 @@ def test_bbregister(capsys, create_files_in_directory): assert bbr.cmd == "bbregister" # cmdline issues a warning in this stats - assert bbr.cmdline is None - captured = capsys.readouterr() + with capture_sys_output() as (stdout, stderr): + assert bbr.cmdline is None - assert 'WARNING' in captured.out - assert 'Some inputs are not valid.' in captured.out + captured = stdout.getvalue() + assert 'WARNING' in captured + assert 'Command line could not be generated' in captured # test raising error with mandatory args absent with pytest.raises(nue.MandatoryInputError): @@ -179,9 +181,10 @@ def test_bbregister(capsys, create_files_in_directory): # Check that 'init' is mandatory in FS < 6, but not in 6+ if Info.looseversion() < LooseVersion("6.0.0"): - assert bbr.cmdline is None - captured = capsys.readouterr() - assert 'Some inputs are not valid.' in captured.out + with capture_sys_output() as (stdout, stderr): + assert bbr.cmdline is None + captured = stdout.getvalue() + assert 'Command line could not be generated' in captured with pytest.raises(nue.VersionIOError): bbr.run() From b08b5ad4acd40d8486c1bbdf62257e959e1e1d93 Mon Sep 17 00:00:00 2001 From: oesteban Date: Sun, 25 Nov 2018 15:42:18 -0800 Subject: [PATCH 36/42] fix errors checking xor --- nipype/interfaces/base/specs.py | 17 +++++++++-------- nipype/interfaces/fsl/utils.py | 4 +--- nipype/utils/errors.py | 7 +++++-- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/nipype/interfaces/base/specs.py b/nipype/interfaces/base/specs.py index 434cf0e6fa..d4c828699d 100644 --- a/nipype/interfaces/base/specs.py +++ b/nipype/interfaces/base/specs.py @@ -408,15 +408,15 @@ def check_requires(inputs, requires): for field in requires] return all(values) -def check_xor(inputs, xor): +def check_xor(inputs, name, xor): """ check if mutually exclusive inputs are satisfied """ - if len(xor) == 1: + if len(xor) == 0: return True - values = [isdefined(getattr(inputs, xor[0]))] + values = [isdefined(getattr(inputs, name))] values += [any([isdefined(getattr(inputs, field)) - for field in xor[1:]])] + for field in xor])] return sum(values) def check_mandatory_inputs(inputs, raise_exc=True): @@ -436,10 +436,11 @@ def check_mandatory_inputs(inputs, raise_exc=True): raise MandatoryInputError(inputs, name) return False - xor = list(xor) if isinstance(xor, (list, tuple)) \ - else [xor] - xor = list(set([name] + xor)) - cxor = check_xor(inputs, xor) + xor = set(list(xor) if isinstance(xor, (list, tuple)) + else [xor]) + xor.discard(name) + xor = list(xor) + cxor = check_xor(inputs, name, xor) if cxor != 1: if raise_exc: raise MutuallyExclusiveInputError( diff --git a/nipype/interfaces/fsl/utils.py b/nipype/interfaces/fsl/utils.py index fac48dcec5..dcf545525f 100644 --- a/nipype/interfaces/fsl/utils.py +++ b/nipype/interfaces/fsl/utils.py @@ -260,9 +260,7 @@ class Smooth(FSLCommand): >>> sm.run() # doctest: +ELLIPSIS Traceback (most recent call last): ... - nipype.utils.errors.MutuallyExclusiveInputError: \ - Interface "Smooth" has mutually-exclusive inputs. \ - Exactly one of (fwhm|sigma) should be set... + MutuallyExclusiveInputError: Interface ... """ diff --git a/nipype/utils/errors.py b/nipype/utils/errors.py index 18c4a25116..7051eda366 100644 --- a/nipype/utils/errors.py +++ b/nipype/utils/errors.py @@ -25,12 +25,15 @@ def __init__(self, inputspec, name, values_defined=None, name_other=None): if values_defined is not None: xor = list(set([name]+ inputspec.traits()[name].xor)) - msg = ('Interface "{classname}" has mutually-exclusive inputs. ' + msg = ('Interface "{classname}" has mutually-exclusive inputs ' + '(processing "{name}", with value={value}). ' 'Exactly one of ({xor}) should be set, but {n:d} were set. ' 'For a list of mutually-exclusive inputs, see ' '{classname}.help().').format(classname=classname, xor='|'.join(xor), - n=values_defined) + n=values_defined, + name=name, + value=getattr(inputspec, name)) else: msg = ('Interface "{classname}" has mutually-exclusive inputs. ' From 44a724d0c1053948ce1fd4efeefa51ecfe504d6c Mon Sep 17 00:00:00 2001 From: oesteban Date: Sun, 25 Nov 2018 15:48:57 -0800 Subject: [PATCH 37/42] fix fs.preprocess test --- .../interfaces/freesurfer/tests/test_preprocess.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/nipype/interfaces/freesurfer/tests/test_preprocess.py b/nipype/interfaces/freesurfer/tests/test_preprocess.py index 3d908914b8..01b99eaa2f 100644 --- a/nipype/interfaces/freesurfer/tests/test_preprocess.py +++ b/nipype/interfaces/freesurfer/tests/test_preprocess.py @@ -11,7 +11,6 @@ from nipype.interfaces.freesurfer import Info from nipype import LooseVersion from nipype.utils import errors as nue -from nipype.utils.tests.test_cmd import capture_sys_output @pytest.mark.skipif( @@ -156,7 +155,7 @@ def test_mandatory_outvol(create_files_in_directory): @pytest.mark.skipif( fs.no_freesurfer(), reason="freesurfer is not installed") -def test_bbregister(create_files_in_directory): +def test_bbregister(caplog, create_files_in_directory): filelist, outdir = create_files_in_directory bbr = fs.BBRegister() @@ -164,11 +163,9 @@ def test_bbregister(create_files_in_directory): assert bbr.cmd == "bbregister" # cmdline issues a warning in this stats - with capture_sys_output() as (stdout, stderr): - assert bbr.cmdline is None + assert bbr.cmdline is None - captured = stdout.getvalue() - assert 'WARNING' in captured + captured = caplog.text assert 'Command line could not be generated' in captured # test raising error with mandatory args absent @@ -181,9 +178,8 @@ def test_bbregister(create_files_in_directory): # Check that 'init' is mandatory in FS < 6, but not in 6+ if Info.looseversion() < LooseVersion("6.0.0"): - with capture_sys_output() as (stdout, stderr): - assert bbr.cmdline is None - captured = stdout.getvalue() + assert bbr.cmdline is None + captured = caplog.text assert 'Command line could not be generated' in captured with pytest.raises(nue.VersionIOError): From f7b5650b817da5fdb6493658c84f41cd6a9de800 Mon Sep 17 00:00:00 2001 From: oesteban Date: Sun, 25 Nov 2018 16:01:45 -0800 Subject: [PATCH 38/42] fix fsl.tests.test_preprocess --- .../freesurfer/tests/test_preprocess.py | 2 +- .../interfaces/fsl/tests/test_preprocess.py | 33 +++++++++++++------ 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/nipype/interfaces/freesurfer/tests/test_preprocess.py b/nipype/interfaces/freesurfer/tests/test_preprocess.py index 01b99eaa2f..64dd3b8379 100644 --- a/nipype/interfaces/freesurfer/tests/test_preprocess.py +++ b/nipype/interfaces/freesurfer/tests/test_preprocess.py @@ -162,7 +162,7 @@ def test_bbregister(caplog, create_files_in_directory): # make sure command gets called assert bbr.cmd == "bbregister" - # cmdline issues a warning in this stats + # cmdline issues a warning: mandatory inputs missing assert bbr.cmdline is None captured = caplog.text diff --git a/nipype/interfaces/fsl/tests/test_preprocess.py b/nipype/interfaces/fsl/tests/test_preprocess.py index 4b387201cf..578fb745a2 100644 --- a/nipype/interfaces/fsl/tests/test_preprocess.py +++ b/nipype/interfaces/fsl/tests/test_preprocess.py @@ -15,6 +15,7 @@ from nipype.interfaces.fsl import Info from nipype.interfaces.base import File, TraitError, Undefined, isdefined from nipype.interfaces.fsl import no_fsl +from nipype.utils import errors as nue def fsl_name(obj, fname): @@ -39,7 +40,7 @@ def test_bet(setup_infile): assert better.cmd == 'bet' # Test raising error with mandatory args absent - with pytest.raises(ValueError): + with pytest.raises(nue.MandatoryInputError): better.run() # Test generated outfile name @@ -195,7 +196,7 @@ def setup_flirt(tmpdir): @pytest.mark.skipif(no_fsl(), reason="fsl is not installed") -def test_flirt(setup_flirt): +def test_flirt(caplog, setup_flirt): # setup tmpdir, infile, reffile = setup_flirt @@ -230,12 +231,24 @@ def test_flirt(setup_flirt): flirter = fsl.FLIRT() # infile not specified - with pytest.raises(ValueError): - flirter.cmdline + assert flirter.cmdline is None + captured = caplog.text + assert 'Command line could not be generated' in captured + + # interface should raise error with mandatory inputs unset + with pytest.raises(nue.MandatoryInputError): + flirter.run() + flirter.inputs.in_file = infile # reference not specified - with pytest.raises(ValueError): - flirter.cmdline + assert flirter.cmdline is None + captured = caplog.text + assert 'Command line could not be generated' in captured + + # interface should raise error with reference still unset + with pytest.raises(nue.MandatoryInputError): + flirter.run() + flirter.inputs.reference = reffile # Generate outfile and outmatrix @@ -380,10 +393,10 @@ def test_mcflirt_opt(setup_flirt): def test_mcflirt_noinput(): # Test error is raised when missing required args fnt = fsl.MCFLIRT() - with pytest.raises(ValueError) as excinfo: + with pytest.raises(nue.MandatoryInputError) as excinfo: fnt.run() assert str(excinfo.value).startswith( - "MCFLIRT requires a value for input 'in_file'") + 'Interface "MCFLIRT" requires a value for input in_file.') # test fnirt @@ -441,9 +454,9 @@ def test_fnirt(setup_flirt): iout) assert fnirt.cmdline == cmd - # Test ValueError is raised when missing mandatory args + # Test nue.MandatoryInputError is raised when missing mandatory args fnirt = fsl.FNIRT() - with pytest.raises(ValueError): + with pytest.raises(nue.MandatoryInputError): fnirt.run() fnirt.inputs.in_file = infile fnirt.inputs.ref_file = reffile From 294850bc9c56668ec00c6696419a530a12195373 Mon Sep 17 00:00:00 2001 From: oesteban Date: Sun, 25 Nov 2018 16:01:45 -0800 Subject: [PATCH 39/42] fix fsl.tests.test_preprocess --- .../freesurfer/tests/test_preprocess.py | 2 +- .../interfaces/fsl/tests/test_preprocess.py | 33 +++++++++++++------ nipype/interfaces/fsl/utils.py | 2 +- nipype/pytest.ini | 2 +- nipype/utils/errors.py | 5 ++- 5 files changed, 30 insertions(+), 14 deletions(-) diff --git a/nipype/interfaces/freesurfer/tests/test_preprocess.py b/nipype/interfaces/freesurfer/tests/test_preprocess.py index 01b99eaa2f..64dd3b8379 100644 --- a/nipype/interfaces/freesurfer/tests/test_preprocess.py +++ b/nipype/interfaces/freesurfer/tests/test_preprocess.py @@ -162,7 +162,7 @@ def test_bbregister(caplog, create_files_in_directory): # make sure command gets called assert bbr.cmd == "bbregister" - # cmdline issues a warning in this stats + # cmdline issues a warning: mandatory inputs missing assert bbr.cmdline is None captured = caplog.text diff --git a/nipype/interfaces/fsl/tests/test_preprocess.py b/nipype/interfaces/fsl/tests/test_preprocess.py index 4b387201cf..578fb745a2 100644 --- a/nipype/interfaces/fsl/tests/test_preprocess.py +++ b/nipype/interfaces/fsl/tests/test_preprocess.py @@ -15,6 +15,7 @@ from nipype.interfaces.fsl import Info from nipype.interfaces.base import File, TraitError, Undefined, isdefined from nipype.interfaces.fsl import no_fsl +from nipype.utils import errors as nue def fsl_name(obj, fname): @@ -39,7 +40,7 @@ def test_bet(setup_infile): assert better.cmd == 'bet' # Test raising error with mandatory args absent - with pytest.raises(ValueError): + with pytest.raises(nue.MandatoryInputError): better.run() # Test generated outfile name @@ -195,7 +196,7 @@ def setup_flirt(tmpdir): @pytest.mark.skipif(no_fsl(), reason="fsl is not installed") -def test_flirt(setup_flirt): +def test_flirt(caplog, setup_flirt): # setup tmpdir, infile, reffile = setup_flirt @@ -230,12 +231,24 @@ def test_flirt(setup_flirt): flirter = fsl.FLIRT() # infile not specified - with pytest.raises(ValueError): - flirter.cmdline + assert flirter.cmdline is None + captured = caplog.text + assert 'Command line could not be generated' in captured + + # interface should raise error with mandatory inputs unset + with pytest.raises(nue.MandatoryInputError): + flirter.run() + flirter.inputs.in_file = infile # reference not specified - with pytest.raises(ValueError): - flirter.cmdline + assert flirter.cmdline is None + captured = caplog.text + assert 'Command line could not be generated' in captured + + # interface should raise error with reference still unset + with pytest.raises(nue.MandatoryInputError): + flirter.run() + flirter.inputs.reference = reffile # Generate outfile and outmatrix @@ -380,10 +393,10 @@ def test_mcflirt_opt(setup_flirt): def test_mcflirt_noinput(): # Test error is raised when missing required args fnt = fsl.MCFLIRT() - with pytest.raises(ValueError) as excinfo: + with pytest.raises(nue.MandatoryInputError) as excinfo: fnt.run() assert str(excinfo.value).startswith( - "MCFLIRT requires a value for input 'in_file'") + 'Interface "MCFLIRT" requires a value for input in_file.') # test fnirt @@ -441,9 +454,9 @@ def test_fnirt(setup_flirt): iout) assert fnirt.cmdline == cmd - # Test ValueError is raised when missing mandatory args + # Test nue.MandatoryInputError is raised when missing mandatory args fnirt = fsl.FNIRT() - with pytest.raises(ValueError): + with pytest.raises(nue.MandatoryInputError): fnirt.run() fnirt.inputs.in_file = infile fnirt.inputs.ref_file = reffile diff --git a/nipype/interfaces/fsl/utils.py b/nipype/interfaces/fsl/utils.py index dcf545525f..b85101f867 100644 --- a/nipype/interfaces/fsl/utils.py +++ b/nipype/interfaces/fsl/utils.py @@ -259,7 +259,7 @@ class Smooth(FSLCommand): >>> sm.run() # doctest: +ELLIPSIS Traceback (most recent call last): - ... + ... MutuallyExclusiveInputError: Interface ... """ diff --git a/nipype/pytest.ini b/nipype/pytest.ini index 70f12b64aa..5f22555598 100644 --- a/nipype/pytest.ini +++ b/nipype/pytest.ini @@ -1,6 +1,6 @@ [pytest] norecursedirs = .git build dist doc nipype/external tools examples src addopts = --doctest-modules -n auto -doctest_optionflags = ALLOW_UNICODE NORMALIZE_WHITESPACE +doctest_optionflags = ALLOW_UNICODE NORMALIZE_WHITESPACE IGNORE_EXCEPTION_DETAIL env = PYTHONHASHSEED=0 diff --git a/nipype/utils/errors.py b/nipype/utils/errors.py index 7051eda366..1fff7404c6 100644 --- a/nipype/utils/errors.py +++ b/nipype/utils/errors.py @@ -24,7 +24,10 @@ def __init__(self, inputspec, name, values_defined=None, name_other=None): classname = _classname_from_spec(inputspec) if values_defined is not None: - xor = list(set([name]+ inputspec.traits()[name].xor)) + xor = inputspec.traits()[name].xor or [] + xor = set(list(xor) if isinstance(xor, (list, tuple)) + else [xor]) + xor.add(name) msg = ('Interface "{classname}" has mutually-exclusive inputs ' '(processing "{name}", with value={value}). ' 'Exactly one of ({xor}) should be set, but {n:d} were set. ' From 89da56a19a2d3d32f4771568fda979e8a9866296 Mon Sep 17 00:00:00 2001 From: oesteban Date: Sun, 25 Nov 2018 19:10:52 -0800 Subject: [PATCH 40/42] install caplog fixture --- nipype/info.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/nipype/info.py b/nipype/info.py index 9e162524dd..c21dfbd8bf 100644 --- a/nipype/info.py +++ b/nipype/info.py @@ -157,7 +157,13 @@ def get_nipype_gitversion(): if sys.version_info <= (3, 4): REQUIRES.append('configparser') -TESTS_REQUIRES = ['pytest-cov', 'codecov', 'pytest-env', 'coverage<5'] +TESTS_REQUIRES = [ + 'pytest-capturelog', + 'pytest-cov', + 'pytest-env', + 'codecov', + 'coverage<5', +] EXTRA_REQUIRES = { 'doc': ['Sphinx>=1.4', 'numpydoc', 'matplotlib', 'pydotplus', 'pydot>=1.2.3'], From fff59eb64a43ef33a96aba257e9bbdbe7bd6ce98 Mon Sep 17 00:00:00 2001 From: oesteban Date: Sun, 25 Nov 2018 20:31:25 -0800 Subject: [PATCH 41/42] pin pytest>=3.4 for reliable caplog fixture --- nipype/info.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/nipype/info.py b/nipype/info.py index c21dfbd8bf..8d41b3c88a 100644 --- a/nipype/info.py +++ b/nipype/info.py @@ -107,7 +107,7 @@ def get_nipype_gitversion(): SCIPY_MIN_VERSION = '0.14' TRAITS_MIN_VERSION = '4.6' DATEUTIL_MIN_VERSION = '2.2' -PYTEST_MIN_VERSION = '3.0' +PYTEST_MIN_VERSION = '3.4' FUTURE_MIN_VERSION = '0.16.0' SIMPLEJSON_MIN_VERSION = '3.8.0' PROV_VERSION = '1.5.2' @@ -158,7 +158,6 @@ def get_nipype_gitversion(): REQUIRES.append('configparser') TESTS_REQUIRES = [ - 'pytest-capturelog', 'pytest-cov', 'pytest-env', 'codecov', From ea4417990dc6c582612e4ee6f3711c1cc695b1ce Mon Sep 17 00:00:00 2001 From: oesteban Date: Mon, 26 Nov 2018 09:10:17 -0800 Subject: [PATCH 42/42] avoid ``CommandLine("which")`` calls, use internal which --- nipype/interfaces/afni/base.py | 16 ++++-------- nipype/interfaces/base/tests/test_core.py | 3 ++- nipype/interfaces/matlab.py | 19 ++------------ nipype/utils/docparse.py | 30 ++++++++--------------- nipype/utils/subprocess.py | 5 ++-- 5 files changed, 22 insertions(+), 51 deletions(-) diff --git a/nipype/interfaces/afni/base.py b/nipype/interfaces/afni/base.py index 24be6405f7..57f307b3a2 100644 --- a/nipype/interfaces/afni/base.py +++ b/nipype/interfaces/afni/base.py @@ -4,7 +4,6 @@ """Provide interface to AFNI commands.""" from __future__ import (print_function, division, unicode_literals, absolute_import) -from builtins import object, str from future.utils import raise_from import os @@ -12,7 +11,7 @@ from distutils import spawn from ... import logging, LooseVersion -from ...utils.filemanip import split_filename, fname_presuffix +from ...utils.filemanip import split_filename, fname_presuffix, which from ..base import (CommandLine, traits, CommandLineInputSpec, isdefined, File, TraitedSpec, PackageInfo) @@ -85,17 +84,12 @@ def standard_image(img_name): '''Grab an image from the standard location. Could be made more fancy to allow for more relocatability''' - clout = CommandLine( - 'which afni', - terminal_output='default', - ignore_exception=True, - resource_monitor=False).run() - if clout.runtime.returncode is not 0: + + afni_path = which('afni') + if not afni_path: return None - with open(clout.runtime.stdout, 'rt') as f: - out = f.read().strip() - basedir = os.path.split(out)[0] + basedir = os.path.split(afni_path)[0] return os.path.join(basedir, img_name) diff --git a/nipype/interfaces/base/tests/test_core.py b/nipype/interfaces/base/tests/test_core.py index 1c66df14b0..060eba0a27 100644 --- a/nipype/interfaces/base/tests/test_core.py +++ b/nipype/interfaces/base/tests/test_core.py @@ -159,7 +159,8 @@ def __init__(self, **inputs): assert '8562a5623562a871115eb14822ee8d02' == hashvalue -def test_stop_on_unknown_version(): +def test_stop_on_unknown_version(tmpdir): + tmpdir.chdir() config.set('execution', 'stop_on_unknown_version', True) ci = nib.CommandLine(command='which') diff --git a/nipype/interfaces/matlab.py b/nipype/interfaces/matlab.py index 88f1aa29a2..988118c310 100644 --- a/nipype/interfaces/matlab.py +++ b/nipype/interfaces/matlab.py @@ -8,6 +8,7 @@ import os from .. import config +from ..utils.filemanip import which from .base import (CommandLineInputSpec, InputMultiPath, isdefined, CommandLine, traits, File, Directory) @@ -15,23 +16,7 @@ def get_matlab_command(): if 'NIPYPE_NO_MATLAB' in os.environ: return None - - try: - matlab_cmd = os.environ['MATLABCMD'] - except: - matlab_cmd = 'matlab' - - try: - res = CommandLine( - command='which', - args=matlab_cmd, - resource_monitor=False, - terminal_output='default').run() - with open(res.runtime.stdout, 'rt') as f: - matlab_path = f.read().strip() - except Exception: - return None - return matlab_cmd + return which(os.getenv('MATLABCMD', 'matlab')) no_matlab = get_matlab_command() is None diff --git a/nipype/utils/docparse.py b/nipype/utils/docparse.py index 7a7a0e8698..2448d4e677 100644 --- a/nipype/utils/docparse.py +++ b/nipype/utils/docparse.py @@ -15,10 +15,10 @@ """ from __future__ import (print_function, division, unicode_literals, absolute_import) -from builtins import str, open, bytes - import subprocess -from ..interfaces.base import CommandLine +from builtins import str, bytes + +from .filemanip import which from .misc import is_container @@ -252,15 +252,10 @@ def get_doc(cmd, opt_map, help_flag=None, trap_error=True): The formated docstring """ - res = CommandLine( - 'which %s' % cmd.split(' ')[0], - resource_monitor=False, - terminal_output='default').run() - with open(res.runtime.stdout, 'rt') as f: - stdout = f.read() - cmd_path = stdout.strip() - if cmd_path == '': - raise Exception('Command %s not found' % cmd.split(' ')[0]) + cmd_exec = cmd.split()[0] + cmd_path = which(cmd_exec) + if not cmd_path: + raise Exception('Command %s not found' % cmd_exec) if help_flag: cmd = ' '.join((cmd, help_flag)) doc = grab_doc(cmd, trap_error) @@ -334,14 +329,9 @@ def get_params_from_doc(cmd, style='--', help_flag=None, trap_error=True): Contains a mapping from input to command line variables """ - res = CommandLine( - 'which %s' % cmd.split(' ')[0], - resource_monitor=False, - terminal_output='default').run() - with open(res.runtime.stdout, 'rt') as f: - stdout = f.read() - cmd_path = stdout.strip() - if cmd_path == '': + cmd_exec = cmd.split()[0] + cmd_path = which(cmd_exec) + if not cmd_path: raise Exception('Command %s not found' % cmd.split(' ')[0]) if help_flag: cmd = ' '.join((cmd, help_flag)) diff --git a/nipype/utils/subprocess.py b/nipype/utils/subprocess.py index 8264d6b71b..b297935c78 100644 --- a/nipype/utils/subprocess.py +++ b/nipype/utils/subprocess.py @@ -140,10 +140,11 @@ def __init__(self, runtime, output=None, period=0.1, cwd = mkdtemp() self._runtime.cwd = cwd + name = self._runtime.cmdline.split()[0].split('/')[-1] self._runtime.stdout = getattr( - self._runtime, 'stdout', os.path.join(cwd, '.nipype.out')) + self._runtime, 'stdout', os.path.join(cwd, '.%s.out' % name)) self._runtime.stderr = getattr( - self._runtime, 'stderr', os.path.join(cwd, '.nipype.err')) + self._runtime, 'stderr', os.path.join(cwd, '.%s.err' % name)) # Open file descriptors and get number self._stdoutfh = open(self._runtime.stdout, 'wb')