diff --git a/nipype/caching/tests/test_memory.py b/nipype/caching/tests/test_memory.py index 3ea594f22a..642fee363d 100644 --- a/nipype/caching/tests/test_memory.py +++ b/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): diff --git a/nipype/interfaces/base/traits_extension.py b/nipype/interfaces/base/traits_extension.py index 395ae0732e..27c4103a74 100644 --- a/nipype/interfaces/base/traits_extension.py +++ b/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] diff --git a/nipype/pipeline/engine/nodes.py b/nipype/pipeline/engine/nodes.py index 6314411a07..2c441a5c57 100644 --- a/nipype/pipeline/engine/nodes.py +++ b/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', '*')): diff --git a/nipype/pipeline/engine/tests/test_base.py b/nipype/pipeline/engine/tests/test_base.py index 3513152c06..5b072e9ee6 100644 --- a/nipype/pipeline/engine/tests/test_base.py +++ b/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']) diff --git a/nipype/pipeline/engine/tests/test_utils.py b/nipype/pipeline/engine/tests/test_utils.py index 4f4383f169..c462ea1533 100644 --- a/nipype/pipeline/engine/tests/test_utils.py +++ b/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 diff --git a/nipype/pipeline/engine/utils.py b/nipype/pipeline/engine/utils.py index 638c91ed97..65170f14c9 100644 --- a/nipype/pipeline/engine/utils.py +++ b/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