FIX: Resolving absolute to relative paths in output

[oesteban]

Building a solution to #2944, starting from a refactor of
aggregate_outputs to be robuster and perform the referrencing when
requested via the new arguiment rebase_cwd.

[codecov-io]

Codecov Report

Merging #2966 into master will decrease coverage by 3.36%.
The diff coverage is 88.49%.

@@            Coverage Diff             @@
##           master    #2966      +/-   ##
==========================================
- Coverage   67.61%   64.24%   -3.37%     
==========================================
  Files         344      342       -2     
  Lines       43798    43781      -17     
  Branches     5471     5482      +11     
==========================================
- Hits        29612    28126    -1486     
- Misses      13475    14566    +1091     
- Partials      711     1089     +378

Flag	Coverage Δ
#smoketests	?
#unittests	64.24% <88.49%> (-0.8%)	⬇️

Impacted Files	Coverage Δ
nipype/utils/filemanip.py	76.5% <100%> (-3.58%)	⬇️
nipype/interfaces/fsl/model.py	55.26% <100%> (-25.35%)	⬇️
nipype/interfaces/base/traits_extension.py	90.52% <100%> (-3.8%)	⬇️
nipype/interfaces/fsl/preprocess.py	72.7% <100%> (-9.91%)	⬇️
nipype/pipeline/engine/nodes.py	77.31% <100%> (-6.79%)	⬇️
nipype/interfaces/freesurfer/preprocess.py	64.53% <100%> (-1.58%)	⬇️
nipype/interfaces/ants/registration.py	73.92% <100%> (ø)	⬆️
nipype/pipeline/engine/utils.py	68.7% <100%> (-13.56%)	⬇️
nipype/interfaces/afni/preprocess.py	82.2% <33.33%> (+0.19%)	⬆️
nipype/interfaces/fsl/utils.py	63.8% <50%> (-6.53%)	⬇️
... and 55 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3454c9a...93f513d. Read the comment docs.

[satra]

@oesteban - just a quick thing if this new release is going to not work with previous versions of pickled files, we should provide an appropriate message. this is mostly an fyi at this point.

[oesteban]

Another issue is really implementing the use_relative_paths config option, and its scope. What is its extent?

• Mandates whether paths in the results and inputs pickles should not be relative? - I'd say no to this one.
• Does this affect only workflows or the setting is also meant for interfaces? (we could resolve/rebase in aggregate_outputs, and actually I've been playing around it).

w.r.t. the old results files, I could add support to read legacy results files too.

[oesteban]

@satra, I've split this PR into more digestible pieces, much less involved:

• ENH: Add resolve/rebase BasePath traits methods & tests #2970: Implements some general functions and their tests extending some compound traits to make it possible to check the trait type before any path manipulation.
• FIX: Resolving/rebasing paths from/to results files #2971: Addresses the original issue Workflow crashing with file not found due to strange nipype.pipeline.engine.utils.modify_path behavior #2944 in the most straightforward way - replaces the old modify_paths utility by the ones added in the other PR, which are far more selective. The original test replicating the bug by @effigies is included in that PR. The way it is done, we do not need to change the way we were pickling (and thus, it should be fully compatible with existing result files).
• MAINT: Refactor aggregate_outputs for readability #2969: a refactor of the aggregate_outputs to make it more readable.

We should also address the questions of how to really implement use_relative_paths:

$git grep use_relative_paths
nipype/pipeline/engine/utils.py:                    if config.getboolean('execution', 'use_relative_paths'):
nipype/utils/config.py:use_relative_paths = false

It seems that the original intent was indeed to prescribe whether results files contain relative or absolute paths.

IMHO results files should have relative paths for the outputs, and we could try to have so for the inputs (although that would require a smarter rebasing function that can identify parents of the base directory).

Then, we could have use_relative_paths to define user-accessible behavior - i.e., whether interfaces can resolve relative paths and they return absolute or relative paths.

Please let me know if you see something else that can be rescued from this closed PR - in particular, whether there is an appetite to change how results are pickled (without all the unroll/combine process we have right now, which was there to make the data structure amenable to modify_paths, and this is not needed anymore).

WDYT?

[satra]

@oesteban - thanks for all this work and this very descriptive comment. since this requires some extra gray cells, i will read this tonight and make any comments. in general all of this sounds reasonable.

one quick note. with relative paths, we would have to consider symlinks on different filesystems/mount points, which is why it was not fully realized. it was primarily created for moving the working directory, which only works with content hashing and not timestamp hashing.