Skip to content

Commit 28d59fe

Browse files
oestebanoesteban
authored
Merge pull request #2971 from oesteban/fix/2944-3
FIX: Resolving/rebasing paths from/to results files
2 parents 2b6b368 + 6dee607 commit 28d59fe

File tree

6 files changed

+150
-37
lines changed

6 files changed

+150
-37
lines changed

‎nipype/caching/tests/test_memory.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,7 @@ class SideEffectInterface(EngineTestInterface):
1515
def _run_interface(self, runtime):
1616
global nb_runs
1717
nb_runs += 1
18-
runtime.returncode = 0
19-
return runtime
18+
return super(SideEffectInterface, self)._run_interface(runtime)
2019

2120

2221
def test_caching(tmpdir):

‎nipype/interfaces/base/traits_extension.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -526,7 +526,7 @@ def _recurse_on_path_traits(func, thistrait, value, cwd):
526526
elif thistrait.is_trait_type(traits.List):
527527
innertrait, = thistrait.inner_traits
528528
if not isinstance(value, (list, tuple)):
529-
value = [value]
529+
return _recurse_on_path_traits(func, innertrait, value, cwd)
530530

531531
value = [_recurse_on_path_traits(func, innertrait, v, cwd)
532532
for v in value]

‎nipype/pipeline/engine/nodes.py

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -587,7 +587,9 @@ def _load_results(self):
587587
runtime=runtime,
588588
inputs=self._interface.inputs.get_traitsfree(),
589589
outputs=aggouts)
590-
_save_resultfile(result, cwd, self.name)
590+
_save_resultfile(
591+
result, cwd, self.name,
592+
rebase=str2bool(self.config['execution']['use_relative_paths']))
591593
else:
592594
logger.debug('aggregating mapnode results')
593595
result = self._run_interface()
@@ -634,7 +636,9 @@ def _run_command(self, execute, copyfiles=True):
634636
except Exception as msg:
635637
result.runtime.stderr = '{}\n\n{}'.format(
636638
getattr(result.runtime, 'stderr', ''), msg)
637-
_save_resultfile(result, outdir, self.name)
639+
_save_resultfile(
640+
result, outdir, self.name,
641+
rebase=str2bool(self.config['execution']['use_relative_paths']))
638642
raise
639643
cmdfile = op.join(outdir, 'command.txt')
640644
with open(cmdfile, 'wt') as fd:
@@ -646,7 +650,9 @@ def _run_command(self, execute, copyfiles=True):
646650
except Exception as msg:
647651
result.runtime.stderr = '%s\n\n%s'.format(
648652
getattr(result.runtime, 'stderr', ''), msg)
649-
_save_resultfile(result, outdir, self.name)
653+
_save_resultfile(
654+
result, outdir, self.name,
655+
rebase=str2bool(self.config['execution']['use_relative_paths']))
650656
raise
651657

652658
dirs2keep = None
@@ -660,7 +666,9 @@ def _run_command(self, execute, copyfiles=True):
660666
self.needed_outputs,
661667
self.config,
662668
dirs2keep=dirs2keep)
663-
_save_resultfile(result, outdir, self.name)
669+
_save_resultfile(
670+
result, outdir, self.name,
671+
rebase=str2bool(self.config['execution']['use_relative_paths']))
664672

665673
return result
666674

@@ -1260,7 +1268,7 @@ def _run_interface(self, execute=True, updatehash=False):
12601268
stop_first=str2bool(
12611269
self.config['execution']['stop_on_first_crash'])))
12621270
# And store results
1263-
_save_resultfile(result, cwd, self.name)
1271+
_save_resultfile(result, cwd, self.name, rebase=False)
12641272
# remove any node directories no longer required
12651273
dirs2remove = []
12661274
for path in glob(op.join(cwd, 'mapflow', '*')):

‎nipype/pipeline/engine/tests/test_base.py

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,19 +20,15 @@ class OutputSpec(nib.TraitedSpec):
2020
output1 = nib.traits.List(nib.traits.Int, desc='outputs')
2121

2222

23-
class EngineTestInterface(nib.BaseInterface):
23+
class EngineTestInterface(nib.SimpleInterface):
2424
input_spec = InputSpec
2525
output_spec = OutputSpec
2626

2727
def _run_interface(self, runtime):
2828
runtime.returncode = 0
29+
self._results['output1'] = [1, self.inputs.input1]
2930
return runtime
3031

31-
def _list_outputs(self):
32-
outputs = self._outputs().get()
33-
outputs['output1'] = [1, self.inputs.input1]
34-
return outputs
35-
3632

3733
@pytest.mark.parametrize(
3834
'name', ['valid1', 'valid_node', 'valid-node', 'ValidNode0'])

‎nipype/pipeline/engine/tests/test_utils.py

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,3 +224,62 @@ def test_mapnode_crash3(tmpdir):
224224
wf.config["execution"]["crashdump_dir"] = os.getcwd()
225225
with pytest.raises(RuntimeError):
226226
wf.run(plugin='Linear')
227+
228+
class StrPathConfuserInputSpec(nib.TraitedSpec):
229+
in_str = nib.traits.String()
230+
231+
232+
class StrPathConfuserOutputSpec(nib.TraitedSpec):
233+
out_tuple = nib.traits.Tuple(nib.File, nib.traits.String)
234+
out_dict_path = nib.traits.Dict(nib.traits.String, nib.File(exists=True))
235+
out_dict_str = nib.traits.DictStrStr()
236+
out_list = nib.traits.List(nib.traits.String)
237+
out_str = nib.traits.String()
238+
out_path = nib.File(exists=True)
239+
240+
241+
class StrPathConfuser(nib.SimpleInterface):
242+
input_spec = StrPathConfuserInputSpec
243+
output_spec = StrPathConfuserOutputSpec
244+
245+
def _run_interface(self, runtime):
246+
out_path = os.path.abspath(os.path.basename(self.inputs.in_str) + '_path')
247+
open(out_path, 'w').close()
248+
self._results['out_str'] = self.inputs.in_str
249+
self._results['out_path'] = out_path
250+
self._results['out_tuple'] = (out_path, self.inputs.in_str)
251+
self._results['out_dict_path'] = {self.inputs.in_str: out_path}
252+
self._results['out_dict_str'] = {self.inputs.in_str: self.inputs.in_str}
253+
self._results['out_list'] = [self.inputs.in_str] * 2
254+
return runtime
255+
256+
257+
def test_modify_paths_bug(tmpdir):
258+
"""
259+
There was a bug in which, if the current working directory contained a file with the name
260+
of an output String, the string would get transformed into a path, and generally wreak havoc.
261+
This attempts to replicate that condition, using an object with strings and paths in various
262+
trait configurations, to ensure that the guards added resolve the issue.
263+
Please see https://github.com/nipy/nipype/issues/2944 for more details.
264+
"""
265+
tmpdir.chdir()
266+
267+
spc = pe.Node(StrPathConfuser(in_str='2'), name='spc')
268+
269+
open('2', 'w').close()
270+
271+
outputs = spc.run().outputs
272+
273+
# Basic check that string was not manipulated
274+
out_str = outputs.out_str
275+
assert out_str == '2'
276+
277+
# Check path exists and is absolute
278+
out_path = outputs.out_path
279+
assert os.path.isabs(out_path)
280+
281+
# Assert data structures pass through correctly
282+
assert outputs.out_tuple == (out_path, out_str)
283+
assert outputs.out_dict_path == {out_str: out_path}
284+
assert outputs.out_dict_str == {out_str: out_str}
285+
assert outputs.out_list == [out_str] * 2

‎nipype/pipeline/engine/utils.py

Lines changed: 74 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,13 @@
2525
from ... import logging, config, LooseVersion
2626
from ...utils.filemanip import (
2727
Path,
28+
indirectory,
2829
relpath,
2930
makedirs,
3031
fname_presuffix,
3132
to_str,
3233
ensure_list,
3334
get_related_files,
34-
FileNotFoundError,
3535
save_json,
3636
savepkl,
3737
loadpkl,
@@ -41,8 +41,10 @@
4141
)
4242
from ...utils.misc import str2bool
4343
from ...utils.functions import create_function_from_source
44-
from ...interfaces.base import (Bunch, CommandLine, isdefined, Undefined,
45-
InterfaceResult, traits)
44+
from ...interfaces.base.traits_extension import (
45+
rebase_path_traits, resolve_path_traits, OutputMultiPath, isdefined, Undefined, traits)
46+
from ...interfaces.base.support import Bunch, InterfaceResult
47+
from ...interfaces.base import CommandLine
4648
from ...interfaces.utility import IdentityInterface
4749
from ...utils.provenance import ProvStore, pm, nipype_ns, get_id
4850

@@ -227,52 +229,101 @@ def write_report(node, report_type=None, is_mapnode=False):
227229
return
228230

229231

230-
def save_resultfile(result, cwd, name):
231-
"""Save a result pklz file to ``cwd``"""
232+
def save_resultfile(result, cwd, name, rebase=None):
233+
"""Save a result pklz file to ``cwd``."""
234+
if rebase is None:
235+
rebase = config.getboolean('execution', 'use_relative_paths')
236+
237+
cwd = os.path.abspath(cwd)
232238
resultsfile = os.path.join(cwd, 'result_%s.pklz' % name)
233-
savepkl(resultsfile, result)
234-
logger.debug('saved results in %s', resultsfile)
239+
logger.debug("Saving results file: '%s'", resultsfile)
235240

241+
if result.outputs is None:
242+
logger.warn('Storing result file without outputs')
243+
savepkl(resultsfile, result)
244+
return
245+
try:
246+
output_names = result.outputs.copyable_trait_names()
247+
except AttributeError:
248+
logger.debug('Storing non-traited results, skipping rebase of paths')
249+
savepkl(resultsfile, result)
250+
return
236251

237-
def load_resultfile(results_file):
252+
backup_traits = {}
253+
try:
254+
with indirectory(cwd):
255+
# All the magic to fix #2944 resides here:
256+
for key in output_names:
257+
old = getattr(result.outputs, key)
258+
if isdefined(old):
259+
if result.outputs.trait(key).is_trait_type(OutputMultiPath):
260+
old = result.outputs.trait(key).handler.get_value(
261+
result.outputs, key)
262+
backup_traits[key] = old
263+
val = rebase_path_traits(result.outputs.trait(key), old, cwd)
264+
setattr(result.outputs, key, val)
265+
savepkl(resultsfile, result)
266+
finally:
267+
# Restore resolved paths from the outputs dict no matter what
268+
for key, val in list(backup_traits.items()):
269+
setattr(result.outputs, key, val)
270+
271+
272+
def load_resultfile(results_file, resolve=True):
238273
"""
239-
Load InterfaceResult file from path
274+
Load InterfaceResult file from path.
240275
241276
Parameter
242277
---------
243-
244278
path : base_dir of node
245279
name : name of node
246280
247281
Returns
248282
-------
249-
250283
result : InterfaceResult structure
251284
aggregate : boolean indicating whether node should aggregate_outputs
252285
attribute error : boolean indicating whether there was some mismatch in
253286
versions of traits used to store result and hence node needs to
254287
rerun
288+
255289
"""
256-
aggregate = True
257290
results_file = Path(results_file)
291+
aggregate = True
258292
result = None
259293
attribute_error = False
260-
if results_file.exists():
294+
295+
if not results_file.exists():
296+
return result, aggregate, attribute_error
297+
298+
with indirectory(str(results_file.parent)):
261299
try:
262300
result = loadpkl(results_file)
263-
except (traits.TraitError, AttributeError, ImportError,
264-
EOFError) as err:
265-
if isinstance(err, (AttributeError, ImportError)):
266-
attribute_error = True
267-
logger.debug('attribute error: %s probably using '
268-
'different trait pickled file', str(err))
269-
else:
270-
logger.debug(
271-
'some file does not exist. hence trait cannot be set')
301+
except (traits.TraitError, EOFError):
302+
logger.debug(
303+
'some file does not exist. hence trait cannot be set')
304+
except (AttributeError, ImportError) as err:
305+
attribute_error = True
306+
logger.debug('attribute error: %s probably using '
307+
'different trait pickled file', str(err))
272308
else:
273309
aggregate = False
274310

275-
logger.debug('Aggregate: %s', aggregate)
311+
if resolve and result.outputs:
312+
try:
313+
outputs = result.outputs.get()
314+
except TypeError: # This is a Bunch
315+
return result, aggregate, attribute_error
316+
317+
logger.debug('Resolving paths in outputs loaded from results file.')
318+
for trait_name, old in list(outputs.items()):
319+
if isdefined(old):
320+
if result.outputs.trait(trait_name).is_trait_type(OutputMultiPath):
321+
old = result.outputs.trait(trait_name).handler.get_value(
322+
result.outputs, trait_name)
323+
value = resolve_path_traits(result.outputs.trait(trait_name), old,
324+
results_file.parent)
325+
setattr(result.outputs, trait_name, value)
326+
276327
return result, aggregate, attribute_error
277328

278329

0 commit comments

Comments
 (0)