ENH: Add resolve/rebase `BasePath` traits methods & tests #2970

oesteban · 2019-07-19T05:47:50Z

List of changes proposed in this PR (pull-request)

Two new methods resolve_path_traits and rebase_path_traits are being included.

They take trait instances from a spec (selected via spec.trait('traitname')), the value and a base path. Returns the modified value (if necessary).

Summary

These two functions will be useful to progress towards #2944.

Acknowledgment

(Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

effigies

My biggest concern is with monkey-patching traits. I think providing replacements in the nipype namespace is fine.

A couple potential corner cases for tests:

rebase_path_traits: cwd not a prefix of value
resolve_path_traits: value already absolute

nipype/interfaces/base/traits_extension.py

nipype/interfaces/base/tests/test_traits_extension.py

nipype/interfaces/base/traits_extension.py

codecov-io · 2019-07-19T19:23:15Z

Codecov Report

Merging #2970 into master will decrease coverage by 3.31%.
The diff coverage is 92.4%.

@@            Coverage Diff             @@
##           master    #2970      +/-   ##
==========================================
- Coverage   67.61%   64.29%   -3.32%     
==========================================
  Files         344      342       -2     
  Lines       43798    43822      +24     
  Branches     5471     5498      +27     
==========================================
- Hits        29612    28174    -1438     
- Misses      13475    14558    +1083     
- Partials      711     1090     +379

Flag	Coverage Δ
#smoketests	`?`
#unittests	`64.29% <92.4%> (-0.75%)`	⬇️

Impacted Files	Coverage Δ
nipype/interfaces/base/traits_extension.py	`89.09% <92.4%> (-5.24%)`	⬇️
nipype/interfaces/nilearn.py	`40% <0%> (-56.67%)`	⬇️
nipype/utils/spm_docs.py	`25.92% <0%> (-44.45%)`	⬇️
nipype/interfaces/freesurfer/base.py	`50% <0%> (-30.51%)`	⬇️
nipype/utils/logger.py	`59.7% <0%> (-29.86%)`	⬇️
nipype/algorithms/rapidart.py	`35% <0%> (-29.42%)`	⬇️
nipype/interfaces/spm/base.py	`57.94% <0%> (-29.14%)`	⬇️
nipype/utils/provenance.py	`55.73% <0%> (-28.35%)`	⬇️
nipype/interfaces/fsl/model.py	`55.26% <0%> (-25.35%)`	⬇️
nipype/testing/fixtures.py	`77.33% <0%> (-21.34%)`	⬇️
... and 41 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...4544b34. Read the comment docs.

codecov-io · 2019-07-19T19:23:31Z

Codecov Report

Merging #2970 into master will decrease coverage by 2.84%.
The diff coverage is 98.97%.

@@            Coverage Diff             @@
##           master    #2970      +/-   ##
==========================================
- Coverage   67.12%   64.28%   -2.85%     
==========================================
  Files         344      342       -2     
  Lines       43785    43776       -9     
  Branches     5469     5483      +14     
==========================================
- Hits        29391    28140    -1251     
- Misses      13648    14547     +899     
- Partials      746     1089     +343

Flag	Coverage Δ
#smoketests	`?`
#unittests	`64.28% <98.97%> (-0.21%)`	⬇️

Impacted Files	Coverage Δ
nipype/interfaces/spm/base.py	`57.94% <ø> (-25.83%)`	⬇️
nipype/interfaces/base/__init__.py	`100% <ø> (ø)`	⬆️
nipype/interfaces/base/specs.py	`87.11% <100%> (-6.19%)`	⬇️
nipype/interfaces/dipy/base.py	`77.19% <100%> (+1.75%)`	⬆️
nipype/interfaces/utility/base.py	`86.36% <100%> (-1.14%)`	⬇️
nipype/interfaces/afni/preprocess.py	`82.01% <100%> (+0.23%)`	⬆️
nipype/interfaces/afni/model.py	`81.95% <100%> (ø)`	⬆️
nipype/interfaces/nipy/model.py	`25.37% <100%> (ø)`	⬆️
nipype/interfaces/freesurfer/registration.py	`78.19% <100%> (ø)`	⬆️
nipype/algorithms/misc.py	`45.57% <100%> (-0.12%)`	⬇️
... and 76 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 81231c8...b0c80d7. Read the comment docs.

- [x] Removed traits.api patches - [x] Deduplicated code with a _recurse_on_path_traits proxy (nipy#2970 (comment)) - [x] Added the two proposed test cases - [x] Optimized loop (see nipy#2970 (comment))

effigies · 2019-07-26T14:06:34Z

Sorry for taking so long to get back to this one. I'm a little torn on the Tuple/Either issue. I see that the point of adding the inner_traits option is to have a uniform access method, but it does mean that our methods won't work with standard traits.Tuple and traits.Either, which is evidenced by the need to mass-replace those with the traits_extension variants.

Does it not make more sense to have an accessor function that doesn't require either monkey-patching or replacing? This way we're not breaking downstream tools that define their own interface.

If not, then I think monkey-patching the pre-existing traits will be necessary.

nipype/interfaces/base/traits_extension.py

oesteban · 2019-07-26T16:13:53Z

Sorry for taking so long to get back to this one. I'm a little torn on the Tuple/Either issue. I see that the point of adding the inner_traits option is to have a uniform access method, but it does mean that our methods won't work with standard traits.Tuple and traits.Either, which is evidenced by the need to mass-replace those with the traits_extension variants.

Correct, the original Tuple and Either won't work.

Does it not make more sense to have an accessor function that doesn't require either monkey-patching or replacing? This way we're not breaking downstream tools that define their own interface.

I tried, but accessing traits without their prescribed methods has a huge boilerplate. I finally opted for this solution of least changes. We should consider streaming them up to traits - at least for the Tuple (not so convinced about Either). Reading through their code, it seems to me that the inner_traits was left there to implement this behavior but they never finally did.

If not, then I think monkey-patching the pre-existing traits will be necessary.

That was my initial solution, but after reading your position about File, I went ahead and changed everything for these other two. Sorry if I misunderstood your previous comment.

effigies · 2019-07-26T16:18:20Z

That was my initial solution, but after reading your position about File, I went ahead and changed everything for these other two. Sorry if I misunderstood your previous comment.

Well, with File and Directory, we're creating them and throwing them into that namespace, which feels strange. Especially because we haven't done it before, so everybody who has been using them has been importing them from nipype.

While I would rather just create an accessor method to contain the boilerplate to one place, e.g.:

def get_inner_traits(trait_type):
    try:
        return trait_type.inner_traits
    except:
        pass
    # Tuple hack
    # Either hack
    # Other hacks?

If that's out of the question, and we're producing traits that are otherwise identical to the ones we're replacing, I think monkey-patching is a cleaner solution than forcing all downstream projects to change their import targets.

oesteban · 2019-07-26T16:25:47Z

Well, with File and Directory, we're creating them and throwing them into that namespace, which feels strange.

https://github.com/enthought/traits/blob/master/traits/trait_types.py#L1659
https://github.com/enthought/traits/blob/master/traits/trait_types.py#L1762

I don't see the difference with Either and Tuple.

Especially because we haven't done it before, so everybody who has been using them has been importing them from nipype.

I disagree - f1f4cf2

effigies · 2019-07-26T16:30:48Z

Huh. I didn't realize there already were File/Directory traits. Sorry about that.

So I don't know. It's annoying to push these changes downstream, but we haven't been monkey-patching traits, so this is a pretty big change to do that.

oesteban · 2019-07-26T17:12:05Z

I would bring monkeypatching back, it is very likely that people have their interfaces defined using traits.(File|Directory|Tuple|Either), but keep all those conversions to nipype's definitions for the codebase. WDYT?

effigies · 2019-07-26T17:15:31Z

That seems reasonable, but I feel like I want to get @satra's opinion on monkeypatching before I ask you to do anything big again...

satra · 2019-07-26T17:35:45Z

@oesteban and @effigies - File/Directory were overwritten to provide better error messages. nothing about the functionality of File/Directory was changed. and ImageFile is a new trait, so that is unique to nipype itself.

the current change that we are making is to change the behavior of File to not only accept/provide Pathlib objects but also to create additional semantics. if we were to forget backwards compatibility we would simply consider two new nipype traits:

PathX, string representing input/output files which need to be content hashed
StrX, strings representing Paths/FileNames (typically outputs), which need to be string hashed (these would have the metadata hash_files=False.

all places where we use File could be replaced by one of these two traits.

the functionality of the new File trait that @oesteban implemented would mostly correspond to PathX. the names PathX and StrX can be decided to be something different.

this way we are not monkeypatching traits, and the introduced functionality is within the nipype context and people are not confused.

oesteban · 2019-07-26T20:12:35Z

The problem I see @satra is that people having interfaces not contributed to nipype are potentially using traits.File and co. If we don't want to break compatibility with those users, we'll need to monkeypatch.

One thing you did not mention is what we do with the new Tuple/Either traits (so that they return their inner trait types).

oesteban · 2019-07-26T20:14:16Z

That said, I would not revert back all those traits.File (at least we now have all of them from nipype, regardless we monkeypatch or not).

oesteban · 2019-07-31T06:46:40Z

I believe I've found the way so that not to shadow traits.Either and traits.Tuple - I'll revert back those changes (maybe opening a new PR?)
Should we patch File and Directory back into traits? We were using traits.File, so it is likely users are using it themselves.
The hash_files and the config use_relative_paths are tangential to this PR and FIX: Resolving/rebasing paths from/to results files #2971, but could be easily addressed with the new features implemented here.

- [x] Removed traits.api patches - [x] Deduplicated code with a _recurse_on_path_traits proxy (nipy#2970 (comment)) - [x] Added the two proposed test cases - [x] Optimized loop (see nipy#2970 (comment))

effigies · 2019-07-31T15:05:50Z

@oesteban I think you rebased on an out-of-date master.

- [x] Removed traits.api patches - [x] Deduplicated code with a _recurse_on_path_traits proxy (nipy#2970 (comment)) - [x] Added the two proposed test cases - [x] Optimized loop (see nipy#2970 (comment))

effigies · 2019-07-31T16:04:07Z

Sorry, can you remove the commits with @josephmje? I don't think we want them and they add a min_ver back in that had been removed from his PR.

effigies

Assuming tests pass, this looks reasonable. One minor suggestion, and one quick question about the tests.

effigies · 2019-07-31T16:38:45Z

nipype/interfaces/dipy/base.py

@@ -100,7 +100,7 @@ def convert_to_traits_type(dipy_type, is_file=False):
    elif "string" in dipy_type and not is_file:
        return traits.Str, is_mandatory


While we're switching from traits.File:

Suggested change

return traits.Str, is_mandatory

return Str, is_mandatory

This will need an import.

@skoudoro Could you verify that this is fine and that you didn't explicitly intend to use traits.Str over nipype.interfaces.base.Str for some reason?

I just check it and everything is fine.

@oesteban Did we not want to change this?

nipype/interfaces/base/traits_extension.py

oesteban · 2019-07-31T19:03:59Z

Three questions remain open (only 1 is relevant to this PR):

Whether to patch File and Directory back in traits - I guess @effigies' would be against (?)
Whether we want to simplify/refactor the implementation of hash_files (ENH: Add resolve/rebase BasePath traits methods & tests #2970 (comment)): this PR is a drop-in replacement of traits that should allow FIX: Resolving/rebasing paths from/to results files #2971 to remain strictly scoped into reading/writing of results files (i.e., no need to rework hash_files).
Whether we want to implement the config use_relative_paths, which I think fully pertains to the Interface level (i.e., it would be independent of the internal format of results files).

cc @satra

effigies

LGTM. 🤞 for the tests.

Closes nipy#2959

Two new methods ``resolve_path_traits`` and ``rebase_path_traits`` are being included. They take trait instances from a spec (selected via ``spec.trait('traitname')``, the value and a base path. These two functions will be usefull to progress towards nipy#2944.

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

- [x] Removed traits.api patches - [x] Deduplicated code with a _recurse_on_path_traits proxy (nipy#2970 (comment)) - [x] Added the two proposed test cases - [x] Optimized loop (see nipy#2970 (comment))

… paths

…ases

oesteban · 2019-08-01T13:08:57Z

Ready to go :)

oesteban mentioned this pull request Jul 19, 2019

FIX: Resolving/rebasing paths from/to results files #2971

Merged

1 task

oesteban requested review from effigies and satra July 19, 2019 16:01

oesteban mentioned this pull request Jul 19, 2019

FIX: Resolving absolute to relative paths in output #2966

Closed

effigies reviewed Jul 19, 2019

View reviewed changes

effigies reviewed Jul 26, 2019

View reviewed changes

nipype/interfaces/base/traits_extension.py Outdated Show resolved Hide resolved

oesteban force-pushed the enh/resolve-rebase-paths branch from cfdb14c to a1eae5a Compare July 31, 2019 14:51

oesteban force-pushed the enh/resolve-rebase-paths branch from a1eae5a to b2adade Compare July 31, 2019 15:02

oesteban force-pushed the enh/resolve-rebase-paths branch from b2adade to 1a67e63 Compare July 31, 2019 15:22

oesteban force-pushed the enh/resolve-rebase-paths branch from 1896f8b to e3a6fd3 Compare July 31, 2019 16:09

effigies approved these changes Jul 31, 2019

View reviewed changes

oesteban and others added 12 commits July 31, 2019 23:28

ENH: Modify Directory and File traits to get along with pathlib

ca5cae3

Closes nipy#2959

Apply suggestions from code review

5f74273

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

Update nipype/interfaces/base/tests/test_traits_extension.py

8a2a4e3

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

fix: addressed @effigies' comments:

cb66a49

- [x] Removed traits.api patches - [x] Deduplicated code with a _recurse_on_path_traits proxy (nipy#2970 (comment)) - [x] Added the two proposed test cases - [x] Optimized loop (see nipy#2970 (comment))

fix: replace all traits.File -> File

2c70fa8

fix: potential fix for nipy#2968 in the context of resolving/rebasing…

ad10517

… paths

fix(Either): better implementation that allows make specs

83380fb

fix(traits): avoid redefining Either and Tuple and new test-c…

cf0403b

…ases

fix: final cleanup after rebase

676f566

fix(tests): combine rebase/resolve tests to check idempotence

17e48d7

tests: add real idempotence tests

eb24c6b

oesteban force-pushed the enh/resolve-rebase-paths branch from a1d6301 to 699b6cf Compare August 1, 2019 06:28

fix: cover one more edge case (nipype.interfaces.base.DictStrStr)

b0c80d7

oesteban force-pushed the enh/resolve-rebase-paths branch from 699b6cf to b0c80d7 Compare August 1, 2019 06:31

effigies merged commit 5965d45 into nipy:master Aug 1, 2019

oesteban deleted the enh/resolve-rebase-paths branch August 1, 2019 16:18

effigies added this to the 1.2.1 milestone Aug 16, 2019

		@@ -100,7 +100,7 @@ def convert_to_traits_type(dipy_type, is_file=False):
		elif "string" in dipy_type and not is_file:
		return traits.Str, is_mandatory

ENH: Add resolve/rebase BasePath traits methods & tests #2970

ENH: Add resolve/rebase BasePath traits methods & tests #2970

Uh oh!

Conversation

oesteban commented Jul 19, 2019

List of changes proposed in this PR (pull-request)

Summary

Acknowledgment

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Jul 19, 2019

Codecov Report

Uh oh!

codecov-io commented Jul 19, 2019 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

effigies commented Jul 26, 2019

Uh oh!

Uh oh!

oesteban commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

effigies commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oesteban commented Jul 26, 2019

Uh oh!

effigies commented Jul 26, 2019

Uh oh!

oesteban commented Jul 26, 2019

Uh oh!

effigies commented Jul 26, 2019

Uh oh!

satra commented Jul 26, 2019

Uh oh!

oesteban commented Jul 26, 2019

Uh oh!

oesteban commented Jul 26, 2019

Uh oh!

oesteban commented Jul 31, 2019

Uh oh!

effigies commented Jul 31, 2019

Uh oh!

effigies commented Jul 31, 2019

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

effigies Jul 31, 2019

Choose a reason for hiding this comment

Uh oh!

skoudoro Jul 31, 2019

Choose a reason for hiding this comment

Uh oh!

effigies Jul 31, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oesteban commented Jul 31, 2019

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

oesteban commented Aug 1, 2019

Uh oh!

Uh oh!

ENH: Add resolve/rebase `BasePath` traits methods & tests #2970

ENH: Add resolve/rebase `BasePath` traits methods & tests #2970

codecov-io commented Jul 19, 2019 •

edited by codecov bot

Loading

oesteban commented Jul 26, 2019 •

edited

Loading

effigies commented Jul 26, 2019 •

edited

Loading