FIX: DataSink to S3 buckets

[kimsin98]

Due to how pathlib.Path handles path strings, S3 bucket paths are missing a /.

Summary

Fixes #3105.

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

• Changes DataSink input specs for output paths to be Str rather than Directory.

Acknowledgment

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

[effigies]

I agree with @oesteban that I think converting base_directory to Str is probably more correct in terms of keeping the inputs consistent with what is passed by the user. On the other hand, this also doesn't seem egregious to me, as s3:/ is not really going to happen when s3:// is not intended. So I'm fine with either approach, myself.

In any event, could you rebase onto maint/1.4.x and update the base branch? As this is a bug fix, we can aim for the 1.4.1 release.

[codecov]

Codecov Report

Merging #3130 into maint/1.4.x will increase coverage by 0.12%.
The diff coverage is 57.14%.

@@               Coverage Diff               @@
##           maint/1.4.x    #3130      +/-   ##
===============================================
+ Coverage        67.59%   67.72%   +0.12%     
===============================================
  Files              299      299              
  Lines            39499    39834     +335     
  Branches          5220     5364     +144     
===============================================
+ Hits             26700    26976     +276     
- Misses           12086    12125      +39     
- Partials           713      733      +20

Flag	Coverage Δ
#smoketests	53.04% <57.14%> (ø)	⬆️
#unittests	65.01% <42.85%> (+0.17%)	⬆️

Impacted Files	Coverage Δ
nipype/interfaces/io.py	59.82% <57.14%> (+0.15%)	⬆️
nipype/interfaces/dcm2nii.py	49.76% <0%> (-8.93%)	⬇️
nipype/interfaces/base/core.py	87.73% <0%> (+0.76%)	⬆️
nipype/pipeline/plugins/tools.py	80.72% <0%> (+1.77%)	⬆️
nipype/pipeline/plugins/legacymultiproc.py	64.98% <0%> (+2.16%)	⬆️
nipype/pipeline/plugins/multiproc.py	85.96% <0%> (+4.71%)	⬆️

---

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 7dd3b37...847553e. Read the comment docs.

[effigies]

I'm good with this. @oesteban, @satra How do you feel about this change?

[kimsin98]

As all the reviewers noted, there seems to be little to no advantage of keeping base_directory to be a Directory. If anything goes wrong, the problem is probably in the user's inputs, and keeping them as Str would make the problem more transparent.

Changed the DataSink input spec instead.

[effigies]

LGTM. A quick question and a suggestion.

[effigies]

What is the effect of this change? Was something failing?

[kimsin98]

No, I don't think this will make any difference in proper use cases.

But since we are setting base_directory trait to Str to be faithful to the user's inputs, it would not make sense to disable input processing on one path input but not the other.