From 2d267b810be9269e9d9b5a8bdeadc042ea3cf5fe Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Fri, 22 Oct 2021 14:21:26 +0200 Subject: [PATCH 1/3] fix: improve error handling of ``CommandLine`` interfaces Currently, errors arising from interpolating the command line of these interfaces is handled poorly at the ``Node`` level. If the command line cannot be built, the error is printed in the logfile but the exception is caught and never raised (i.e., likely leading to an infinite loop as the execution is not stopped). I have experienced that while debugging fMRIPrep. To learn which of the inputs of a faulty interface derived from ``CommandLine`` was not being formatted, I had to also add the error annotation proposed for the ``_parse_inputs`` inner loop. --- nipype/interfaces/base/core.py | 35 +++++++++++++++++++++++++++++---- nipype/pipeline/engine/nodes.py | 8 ++++---- nipype/utils/subprocess.py | 6 +++++- 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/nipype/interfaces/base/core.py b/nipype/interfaces/base/core.py index 65b35cadce..6ffa843bf4 100644 --- a/nipype/interfaces/base/core.py +++ b/nipype/interfaces/base/core.py @@ -607,6 +607,7 @@ class must be instantiated with a command argument _cmd = None _version = None _terminal_output = "stream" + _write_cmdline = False @classmethod def set_default_terminal_output(cls, output_type): @@ -623,7 +624,7 @@ def set_default_terminal_output(cls, output_type): else: raise AttributeError("Invalid terminal output_type: %s" % output_type) - def __init__(self, command=None, terminal_output=None, **inputs): + def __init__(self, command=None, terminal_output=None, write_cmdline=False, **inputs): super(CommandLine, self).__init__(**inputs) self._environ = None # Set command. Input argument takes precedence @@ -638,6 +639,8 @@ def __init__(self, command=None, terminal_output=None, **inputs): if terminal_output is not None: self.terminal_output = terminal_output + self._write_cmdline = write_cmdline + @property def cmd(self): """sets base command, immutable""" @@ -669,6 +672,14 @@ def terminal_output(self, value): ) self._terminal_output = value + @property + def write_cmdline(self): + return self._write_cmdline + + @write_cmdline.setter + def write_cmdline(self, value): + self._write_cmdline = value is True + def raise_exception(self, runtime): raise RuntimeError( ( @@ -716,9 +727,14 @@ def _run_interface(self, runtime, correct_return_codes=(0,)): adds stdout, stderr, merged, cmdline, dependencies, command_path """ - out_environ = self._get_environ() # Initialize runtime Bunch + + try: + runtime.cmdline = self.cmdline + except Exception as exc: + raise RuntimeError("Error raised when interpolating the command line") from exc + runtime.stdout = None runtime.stderr = None runtime.cmdline = self.cmdline @@ -742,7 +758,11 @@ def _run_interface(self, runtime, correct_return_codes=(0,)): if self._ldd else "" ) - runtime = run_command(runtime, output=self.terminal_output) + runtime = run_command( + runtime, + output=self.terminal_output, + write_cmdline=self.write_cmdline, + ) return runtime def _format_arg(self, name, trait_spec, value): @@ -907,7 +927,14 @@ def _parse_inputs(self, skip=None): if not isdefined(value): continue - arg = self._format_arg(name, spec, value) + + try: + arg = self._format_arg(name, spec, value) + except Exception as exc: + raise ValueError( + f"Error formatting command line argument '{name}' with value '{value}'" + ) from exc + if arg is None: continue pos = spec.position diff --git a/nipype/pipeline/engine/nodes.py b/nipype/pipeline/engine/nodes.py index bda77b1956..6e251647f6 100644 --- a/nipype/pipeline/engine/nodes.py +++ b/nipype/pipeline/engine/nodes.py @@ -209,6 +209,9 @@ def __init__( self.needed_outputs = needed_outputs self.config = None + if issubclass(self._interface.__class__, CommandLine): + self._interface.write_cmdline = True + @property def interface(self): """Return the underlying interface object""" @@ -711,16 +714,13 @@ def _run_command(self, execute, copyfiles=True): f'[Node] Executing "{self.name}" <{self._interface.__module__}' f".{self._interface.__class__.__name__}>" ) + # Invoke core run method of the interface ignoring exceptions result = self._interface.run(cwd=outdir, ignore_exception=True) logger.info( f'[Node] Finished "{self.name}", elapsed time {result.runtime.duration}s.' ) - if issubclass(self._interface.__class__, CommandLine): - # Write out command line as it happened - Path.write_text(outdir / "command.txt", f"{result.runtime.cmdline}\n") - exc_tb = getattr(result.runtime, "traceback", None) if not exc_tb: diff --git a/nipype/utils/subprocess.py b/nipype/utils/subprocess.py index f590b337a0..5611cccc2b 100644 --- a/nipype/utils/subprocess.py +++ b/nipype/utils/subprocess.py @@ -10,6 +10,7 @@ import select import locale import datetime +from pathlib import Path from subprocess import Popen, STDOUT, PIPE from .filemanip import canonicalize_env, read_stream @@ -69,7 +70,7 @@ def _read(self, drain): self._lastidx = len(self._rows) -def run_command(runtime, output=None, timeout=0.01): +def run_command(runtime, output=None, timeout=0.01, write_cmdline=False): """Run a command, read stdout and stderr, prefix with timestamp. The returned runtime contains a merged stdout+stderr log with timestamps @@ -100,6 +101,9 @@ def run_command(runtime, output=None, timeout=0.01): errfile = os.path.join(runtime.cwd, "stderr.nipype") stderr = open(errfile, "wb") + if write_cmdline: + (Path(runtime.cwd) / "command.txt").write_text(cmdline) + proc = Popen( cmdline, stdout=stdout, From e90e75b3ace9da8302fc746c3c3f1e01c6032ef9 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Tue, 26 Oct 2021 11:16:00 +0200 Subject: [PATCH 2/3] Update nipype/pipeline/engine/nodes.py Co-authored-by: Chris Markiewicz --- nipype/pipeline/engine/nodes.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nipype/pipeline/engine/nodes.py b/nipype/pipeline/engine/nodes.py index 6e251647f6..59fb2e6724 100644 --- a/nipype/pipeline/engine/nodes.py +++ b/nipype/pipeline/engine/nodes.py @@ -209,7 +209,7 @@ def __init__( self.needed_outputs = needed_outputs self.config = None - if issubclass(self._interface.__class__, CommandLine): + if hasattr(self._interface, "write_cmdline"): self._interface.write_cmdline = True @property From 5d9adbbb77b7047b9b47cd2fa079dee0094cfc91 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Tue, 26 Oct 2021 11:18:02 +0200 Subject: [PATCH 3/3] sty: run black --- nipype/interfaces/base/core.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/nipype/interfaces/base/core.py b/nipype/interfaces/base/core.py index 6ffa843bf4..69d621bbc1 100644 --- a/nipype/interfaces/base/core.py +++ b/nipype/interfaces/base/core.py @@ -624,7 +624,9 @@ def set_default_terminal_output(cls, output_type): else: raise AttributeError("Invalid terminal output_type: %s" % output_type) - def __init__(self, command=None, terminal_output=None, write_cmdline=False, **inputs): + def __init__( + self, command=None, terminal_output=None, write_cmdline=False, **inputs + ): super(CommandLine, self).__init__(**inputs) self._environ = None # Set command. Input argument takes precedence @@ -733,7 +735,9 @@ def _run_interface(self, runtime, correct_return_codes=(0,)): try: runtime.cmdline = self.cmdline except Exception as exc: - raise RuntimeError("Error raised when interpolating the command line") from exc + raise RuntimeError( + "Error raised when interpolating the command line" + ) from exc runtime.stdout = None runtime.stderr = None