From c8a37979456b75db0c6e0c1c65bba559a59131cd Mon Sep 17 00:00:00 2001 From: oesteban Date: Thu, 1 Aug 2019 12:48:02 -0700 Subject: [PATCH 1/3] FIX: Correctly pickle ``OuputMulti{Object,Path}`` traits It seems that #2944 has uncovered a rats-nest hidden in the engine. In resolving that issue, I found out that a great deal of boilerplate was set in place when loading/saving results to deal with ``OutputMulti{Object,Path}`` traits. The reason being that these traits flatten single-element-list values. This PR fixes the pickling behavior of traited specs containing these types of traits. Additionally, this PR also avoids the ``modify_paths`` function that was causing problems originally in #2944. Therefore, this PR effectively make results files static, meaning: caching if the ``base_dir`` of the workflow is changed will not work anymore. I plan to re-insert this feature (results file mobility) with #2971. This PR is just to split that one in more digestible bits. All the boilerplate mentioned above has been cleaned up. --- nipype/interfaces/base/specs.py | 28 ++++++++++ nipype/pipeline/engine/utils.py | 91 ++------------------------------- 2 files changed, 31 insertions(+), 88 deletions(-) diff --git a/nipype/interfaces/base/specs.py b/nipype/interfaces/base/specs.py index 5028d5c30a..30d191163f 100644 --- a/nipype/interfaces/base/specs.py +++ b/nipype/interfaces/base/specs.py @@ -30,6 +30,8 @@ Undefined, isdefined, has_metadata, + OutputMultiObject, + OutputMultiPath, ) from ... import config, __version__ @@ -316,6 +318,32 @@ def _get_sorteddict(self, def __all__(self): return self.copyable_trait_names() + def __getstate__(self): + """ + Override __getstate__ so that OutputMultiObjects are correctly pickled. + + >>> class OutputSpec(TraitedSpec): + ... out = OutputMultiObject(traits.List(traits.Int)) + >>> spec = OutputSpec() + >>> spec.out = [[4]] + >>> spec.out + [4] + + >>> spec.__getstate__()['out'] + [[4]] + + >>> spec.__setstate__(spec.__getstate__()) + >>> spec.out + [4] + + """ + state = super(BaseTraitedSpec, self).__getstate__() + for key in self.__all__: + _trait_spec = self.trait(key) + if _trait_spec.is_trait_type((OutputMultiObject, OutputMultiPath)): + state[key] = _trait_spec.handler.get_value(self, key) + return state + def _deepcopypatch(self, memo): """ diff --git a/nipype/pipeline/engine/utils.py b/nipype/pipeline/engine/utils.py index 3d961126d5..b6c0afda64 100644 --- a/nipype/pipeline/engine/utils.py +++ b/nipype/pipeline/engine/utils.py @@ -226,87 +226,12 @@ def write_report(node, report_type=None, is_mapnode=False): return -def _identify_collapses(hastraits): - """ Identify traits that will collapse when being set to themselves. - - ``OutputMultiObject``s automatically unwrap a list of length 1 to directly - reference the element of that list. - If that element is itself a list of length 1, then the following will - result in modified values. - - hastraits.trait_set(**hastraits.trait_get()) - - Cloning performs this operation on a copy of the original traited object, - allowing us to identify traits that will be affected. - """ - raw = hastraits.trait_get() - cloned = hastraits.clone_traits().trait_get() - - collapsed = set() - for key in cloned: - orig = raw[key] - new = cloned[key] - # Allow numpy to handle the equality checks, as mixed lists and arrays - # can be problematic. - if isinstance(orig, list) and len(orig) == 1 and ( - not np.array_equal(orig, new) and np.array_equal(orig[0], new)): - collapsed.add(key) - - return collapsed - - -def _uncollapse(indexable, collapsed): - """ Wrap collapsible values in a list to prevent double-collapsing. - - Should be used with _identify_collapses to provide the following - idempotent operation: - - collapsed = _identify_collapses(hastraits) - hastraits.trait_set(**_uncollapse(hastraits.trait_get(), collapsed)) - - NOTE: Modifies object in-place, in addition to returning it. - """ - - for key in indexable: - if key in collapsed: - indexable[key] = [indexable[key]] - return indexable - - -def _protect_collapses(hastraits): - """ A collapse-protected replacement for hastraits.trait_get() - - May be used as follows to provide an idempotent trait_set: - - hastraits.trait_set(**_protect_collapses(hastraits)) - """ - collapsed = _identify_collapses(hastraits) - return _uncollapse(hastraits.trait_get(), collapsed) - - def save_resultfile(result, cwd, name): """Save a result pklz file to ``cwd``""" resultsfile = os.path.join(cwd, 'result_%s.pklz' % name) - if result.outputs: - try: - collapsed = _identify_collapses(result.outputs) - outputs = _uncollapse(result.outputs.trait_get(), collapsed) - # Double-protect tosave so that the original, uncollapsed trait - # is saved in the pickle file. Thus, when the loading process - # collapses, the original correct value is loaded. - tosave = _uncollapse(outputs.copy(), collapsed) - except AttributeError: - tosave = outputs = result.outputs.dictcopy() # outputs was a bunch - for k, v in list(modify_paths(tosave, relative=True, basedir=cwd).items()): - setattr(result.outputs, k, v) - savepkl(resultsfile, result) logger.debug('saved results in %s', resultsfile) - if result.outputs: - for k, v in list(outputs.items()): - setattr(result.outputs, k, v) - def load_resultfile(path, name): """ @@ -349,20 +274,10 @@ def load_resultfile(path, name): logger.debug( 'some file does not exist. hence trait cannot be set') else: - if result.outputs: - try: - outputs = _protect_collapses(result.outputs) - except AttributeError: - outputs = result.outputs.dictcopy() # outputs == Bunch - try: - for k, v in list(modify_paths(outputs, relative=False, - basedir=path).items()): - setattr(result.outputs, k, v) - except FileNotFoundError: - logger.debug('conversion to full path results in ' - 'non existent file') aggregate = False - pkl_file.close() + finally: + pkl_file.close() + logger.debug('Aggregate: %s', aggregate) return result, aggregate, attribute_error From 9c374e4bebddc142709c682f71eceaab1b4fc92a Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Thu, 1 Aug 2019 13:32:34 -0700 Subject: [PATCH 2/3] Update nipype/interfaces/base/specs.py [skip ci] Co-Authored-By: Chris Markiewicz --- 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 30d191163f..bbafda607b 100644 --- a/nipype/interfaces/base/specs.py +++ b/nipype/interfaces/base/specs.py @@ -340,7 +340,7 @@ def __getstate__(self): state = super(BaseTraitedSpec, self).__getstate__() for key in self.__all__: _trait_spec = self.trait(key) - if _trait_spec.is_trait_type((OutputMultiObject, OutputMultiPath)): + if _trait_spec.is_trait_type(OutputMultiObject): state[key] = _trait_spec.handler.get_value(self, key) return state From 0cd60b6d66a572a7a3e7b065c99234e4f7750211 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Thu, 1 Aug 2019 14:06:39 -0700 Subject: [PATCH 3/3] Update nipype/interfaces/base/specs.py [skip ci] Co-Authored-By: Chris Markiewicz --- 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 bbafda607b..c99620e9d8 100644 --- a/nipype/interfaces/base/specs.py +++ b/nipype/interfaces/base/specs.py @@ -31,7 +31,6 @@ isdefined, has_metadata, OutputMultiObject, - OutputMultiPath, ) from ... import config, __version__