Merge pull request #2971 from oesteban/fix/2944-3

FIX: Resolving/rebasing paths from/to results files

Author: oesteban

nipype/caching/tests/test_memory.py

@@ -15,8 +15,7 @@ class SideEffectInterface(EngineTestInterface):
     def _run_interface(self, runtime):
         global nb_runs
         nb_runs += 1
-        runtime.returncode = 0
-        return runtime
+        return super(SideEffectInterface, self)._run_interface(runtime)
 
 
 def test_caching(tmpdir):

nipype/interfaces/base/traits_extension.py

@@ -526,7 +526,7 @@ def _recurse_on_path_traits(func, thistrait, value, cwd):
     elif thistrait.is_trait_type(traits.List):
         innertrait, = thistrait.inner_traits
         if not isinstance(value, (list, tuple)):
-            value = [value]
+            return _recurse_on_path_traits(func, innertrait, value, cwd)
 
         value = [_recurse_on_path_traits(func, innertrait, v, cwd)
                  for v in value]

nipype/pipeline/engine/nodes.py

@@ -587,7 +587,9 @@ def _load_results(self):
                     runtime=runtime,
                     inputs=self._interface.inputs.get_traitsfree(),
                     outputs=aggouts)
-                _save_resultfile(result, cwd, self.name)
+                _save_resultfile(
+                    result, cwd, self.name,
+                    rebase=str2bool(self.config['execution']['use_relative_paths']))
             else:
                 logger.debug('aggregating mapnode results')
                 result = self._run_interface()
@@ -634,7 +636,9 @@ def _run_command(self, execute, copyfiles=True):
             except Exception as msg:
                 result.runtime.stderr = '{}\n\n{}'.format(
                     getattr(result.runtime, 'stderr', ''), msg)
-                _save_resultfile(result, outdir, self.name)
+                _save_resultfile(
+                    result, outdir, self.name,
+                    rebase=str2bool(self.config['execution']['use_relative_paths']))
                 raise
             cmdfile = op.join(outdir, 'command.txt')
             with open(cmdfile, 'wt') as fd:
@@ -646,7 +650,9 @@ def _run_command(self, execute, copyfiles=True):
         except Exception as msg:
             result.runtime.stderr = '%s\n\n%s'.format(
                 getattr(result.runtime, 'stderr', ''), msg)
-            _save_resultfile(result, outdir, self.name)
+            _save_resultfile(
+                result, outdir, self.name,
+                rebase=str2bool(self.config['execution']['use_relative_paths']))
             raise
 
         dirs2keep = None
@@ -660,7 +666,9 @@ def _run_command(self, execute, copyfiles=True):
             self.needed_outputs,
             self.config,
             dirs2keep=dirs2keep)
-        _save_resultfile(result, outdir, self.name)
+        _save_resultfile(
+            result, outdir, self.name,
+            rebase=str2bool(self.config['execution']['use_relative_paths']))
 
         return result
 
@@ -1260,7 +1268,7 @@ def _run_interface(self, execute=True, updatehash=False):
                 stop_first=str2bool(
                     self.config['execution']['stop_on_first_crash'])))
         # And store results
-        _save_resultfile(result, cwd, self.name)
+        _save_resultfile(result, cwd, self.name, rebase=False)
         # remove any node directories no longer required
         dirs2remove = []
         for path in glob(op.join(cwd, 'mapflow', '*')):

nipype/pipeline/engine/tests/test_base.py

@@ -20,19 +20,15 @@ class OutputSpec(nib.TraitedSpec):
     output1 = nib.traits.List(nib.traits.Int, desc='outputs')
 
 
-class EngineTestInterface(nib.BaseInterface):
+class EngineTestInterface(nib.SimpleInterface):
     input_spec = InputSpec
     output_spec = OutputSpec
 
     def _run_interface(self, runtime):
         runtime.returncode = 0
+        self._results['output1'] = [1, self.inputs.input1]
         return runtime
 
-    def _list_outputs(self):
-        outputs = self._outputs().get()
-        outputs['output1'] = [1, self.inputs.input1]
-        return outputs
-
 
 @pytest.mark.parametrize(
     'name', ['valid1', 'valid_node', 'valid-node', 'ValidNode0'])

nipype/pipeline/engine/tests/test_utils.py

@@ -224,3 +224,62 @@ def test_mapnode_crash3(tmpdir):
     wf.config["execution"]["crashdump_dir"] = os.getcwd()
     with pytest.raises(RuntimeError):
         wf.run(plugin='Linear')
+
+class StrPathConfuserInputSpec(nib.TraitedSpec):
+    in_str = nib.traits.String()
+
+
+class StrPathConfuserOutputSpec(nib.TraitedSpec):
+    out_tuple = nib.traits.Tuple(nib.File, nib.traits.String)
+    out_dict_path = nib.traits.Dict(nib.traits.String, nib.File(exists=True))
+    out_dict_str = nib.traits.DictStrStr()
+    out_list = nib.traits.List(nib.traits.String)
+    out_str = nib.traits.String()
+    out_path = nib.File(exists=True)
+
+
+class StrPathConfuser(nib.SimpleInterface):
+    input_spec = StrPathConfuserInputSpec
+    output_spec = StrPathConfuserOutputSpec
+
+    def _run_interface(self, runtime):
+        out_path = os.path.abspath(os.path.basename(self.inputs.in_str) + '_path')
+        open(out_path, 'w').close()
+        self._results['out_str'] = self.inputs.in_str
+        self._results['out_path'] = out_path
+        self._results['out_tuple'] = (out_path, self.inputs.in_str)
+        self._results['out_dict_path'] = {self.inputs.in_str: out_path}
+        self._results['out_dict_str'] = {self.inputs.in_str: self.inputs.in_str}
+        self._results['out_list'] = [self.inputs.in_str] * 2
+        return runtime
+
+
+def test_modify_paths_bug(tmpdir):
+    """
+    There was a bug in which, if the current working directory contained a file with the name
+    of an output String, the string would get transformed into a path, and generally wreak havoc.
+    This attempts to replicate that condition, using an object with strings and paths in various
+    trait configurations, to ensure that the guards added resolve the issue.
+    Please see https://github.com/nipy/nipype/issues/2944 for more details.
+    """
+    tmpdir.chdir()
+
+    spc = pe.Node(StrPathConfuser(in_str='2'), name='spc')
+
+    open('2', 'w').close()
+
+    outputs = spc.run().outputs
+
+    # Basic check that string was not manipulated
+    out_str = outputs.out_str
+    assert out_str == '2'
+
+    # Check path exists and is absolute
+    out_path = outputs.out_path
+    assert os.path.isabs(out_path)
+
+    # Assert data structures pass through correctly
+    assert outputs.out_tuple == (out_path, out_str)
+    assert outputs.out_dict_path == {out_str: out_path}
+    assert outputs.out_dict_str == {out_str: out_str}
+    assert outputs.out_list == [out_str] * 2

nipype/pipeline/engine/utils.py

@@ -25,13 +25,13 @@
 from ... import logging, config, LooseVersion
 from ...utils.filemanip import (
     Path,
+    indirectory,
     relpath,
     makedirs,
     fname_presuffix,
     to_str,
     ensure_list,
     get_related_files,
-    FileNotFoundError,
     save_json,
     savepkl,
     loadpkl,
@@ -41,8 +41,10 @@
 )
 from ...utils.misc import str2bool
 from ...utils.functions import create_function_from_source
-from ...interfaces.base import (Bunch, CommandLine, isdefined, Undefined,
-                                InterfaceResult, traits)
+from ...interfaces.base.traits_extension import (
+    rebase_path_traits, resolve_path_traits, OutputMultiPath, isdefined, Undefined, traits)
+from ...interfaces.base.support import Bunch, InterfaceResult
+from ...interfaces.base import CommandLine
 from ...interfaces.utility import IdentityInterface
 from ...utils.provenance import ProvStore, pm, nipype_ns, get_id
 
@@ -227,52 +229,101 @@ def write_report(node, report_type=None, is_mapnode=False):
     return
 
 
-def save_resultfile(result, cwd, name):
-    """Save a result pklz file to ``cwd``"""
+def save_resultfile(result, cwd, name, rebase=None):
+    """Save a result pklz file to ``cwd``."""
+    if rebase is None:
+        rebase = config.getboolean('execution', 'use_relative_paths')
+
+    cwd = os.path.abspath(cwd)
     resultsfile = os.path.join(cwd, 'result_%s.pklz' % name)
-    savepkl(resultsfile, result)
-    logger.debug('saved results in %s', resultsfile)
+    logger.debug("Saving results file: '%s'", resultsfile)
 
+    if result.outputs is None:
+        logger.warn('Storing result file without outputs')
+        savepkl(resultsfile, result)
+        return
+    try:
+        output_names = result.outputs.copyable_trait_names()
+    except AttributeError:
+        logger.debug('Storing non-traited results, skipping rebase of paths')
+        savepkl(resultsfile, result)
+        return
 
-def load_resultfile(results_file):
+    backup_traits = {}
+    try:
+        with indirectory(cwd):
+            # All the magic to fix #2944 resides here:
+            for key in output_names:
+                old = getattr(result.outputs, key)
+                if isdefined(old):
+                    if result.outputs.trait(key).is_trait_type(OutputMultiPath):
+                        old = result.outputs.trait(key).handler.get_value(
+                            result.outputs, key)
+                    backup_traits[key] = old
+                    val = rebase_path_traits(result.outputs.trait(key), old, cwd)
+                    setattr(result.outputs, key, val)
+        savepkl(resultsfile, result)
+    finally:
+        # Restore resolved paths from the outputs dict no matter what
+        for key, val in list(backup_traits.items()):
+            setattr(result.outputs, key, val)
+
+
+def load_resultfile(results_file, resolve=True):
     """
-    Load InterfaceResult file from path
+    Load InterfaceResult file from path.
 
     Parameter
     ---------
-
     path : base_dir of node
     name : name of node
 
     Returns
     -------
-
     result : InterfaceResult structure
     aggregate : boolean indicating whether node should aggregate_outputs
     attribute error : boolean indicating whether there was some mismatch in
         versions of traits used to store result and hence node needs to
         rerun
+
     """
-    aggregate = True
     results_file = Path(results_file)
+    aggregate = True
     result = None
     attribute_error = False
-    if results_file.exists():
+
+    if not results_file.exists():
+        return result, aggregate, attribute_error
+
+    with indirectory(str(results_file.parent)):
         try:
             result = loadpkl(results_file)
-        except (traits.TraitError, AttributeError, ImportError,
-                EOFError) as err:
-            if isinstance(err, (AttributeError, ImportError)):
-                attribute_error = True
-                logger.debug('attribute error: %s probably using '
-                             'different trait pickled file', str(err))
-            else:
-                logger.debug(
-                    'some file does not exist. hence trait cannot be set')
+        except (traits.TraitError, EOFError):
+            logger.debug(
+                'some file does not exist. hence trait cannot be set')
+        except (AttributeError, ImportError) as err:
+            attribute_error = True
+            logger.debug('attribute error: %s probably using '
+                         'different trait pickled file', str(err))
         else:
             aggregate = False
 
-    logger.debug('Aggregate: %s', aggregate)
+        if resolve and result.outputs:
+            try:
+                outputs = result.outputs.get()
+            except TypeError:  # This is a Bunch
+                return result, aggregate, attribute_error
+
+            logger.debug('Resolving paths in outputs loaded from results file.')
+            for trait_name, old in list(outputs.items()):
+                if isdefined(old):
+                    if result.outputs.trait(trait_name).is_trait_type(OutputMultiPath):
+                        old = result.outputs.trait(trait_name).handler.get_value(
+                            result.outputs, trait_name)
+                    value = resolve_path_traits(result.outputs.trait(trait_name), old,
+                                                results_file.parent)
+                    setattr(result.outputs, trait_name, value)
+
     return result, aggregate, attribute_error