Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

changing Node._output_dir to realpath #2639

Merged
merged 1 commit into from
Jul 18, 2018
Merged

Conversation

djarecka
Copy link
Collaborator

I changed abspath to realpath in Node._output_dir to fix problems with temporary directories on OSX. abspath gives /var/... and realpath returns /private/var.... In clean_working_directory we had needed_files that used /private/var/... and cwd used /var/..., so many files were removed.

I'm not sure if this is the best place to change the path, I can do it also in clean_working_directory, if for any reason we should keep abspath in _output_dir.

There is a chance that this will solve the general issue from #2395. For now it fixed all tests from heudiconv, so should also solve this issue

… (abspath returns /var/.. and realpath returns /private/var/... and some files are removed by clean_working_directory
@codecov-io
Copy link

codecov-io commented Jul 16, 2018

Codecov Report

Merging #2639 into master will decrease coverage by 3.54%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2639      +/-   ##
==========================================
- Coverage    67.6%   64.06%   -3.55%     
==========================================
  Files         340      338       -2     
  Lines       43003    42952      -51     
  Branches     5321     5318       -3     
==========================================
- Hits        29073    27516    -1557     
- Misses      13231    14383    +1152     
- Partials      699     1053     +354
Flag Coverage Δ
#smoketests ?
#unittests 64.06% <100%> (-1%) ⬇️
Impacted Files Coverage Δ
nipype/pipeline/engine/nodes.py 77.52% <100%> (-6.78%) ⬇️
nipype/interfaces/nilearn.py 40% <0%> (-56.67%) ⬇️
nipype/utils/spm_docs.py 25.92% <0%> (-44.45%) ⬇️
nipype/interfaces/nitime/analysis.py 43.15% <0%> (-36.85%) ⬇️
nipype/interfaces/freesurfer/base.py 49.59% <0%> (-30.9%) ⬇️
nipype/utils/logger.py 59.7% <0%> (-29.86%) ⬇️
nipype/algorithms/rapidart.py 35.39% <0%> (-29.21%) ⬇️
nipype/interfaces/spm/base.py 58.41% <0%> (-29.05%) ⬇️
nipype/utils/provenance.py 55.73% <0%> (-28.99%) ⬇️
nipype/interfaces/fsl/model.py 55.26% <0%> (-25.35%) ⬇️
... and 45 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 6c00f85...d51b534. Read the comment docs.

@mgxd
Copy link
Member

mgxd commented Jul 17, 2018

@djarecka are unused outputs still removed within a workflow (by default)? if so, i'm good with this change - nice catch

@djarecka
Copy link
Collaborator Author

djarecka commented Jul 17, 2018

@mgxd - yes, unused output is still removed, you can check that you don't have all dcm files at the end. My changes just ensure that we run walk_files using path that is a realpath and not an abspath.

Dcm2niix that we were testing yesterday uses abspath in out_file, but fnam contains ., so abspath is equivalent to realpath.

I hope that we don't have interfaces that return abspath that is different than realpath in output (this might lead to similar problems). If we notice any new problems, I'd suggest adding realpath in File class: here and here (and similar changes in Directory and ImageFile).

@mgxd
Copy link
Member

mgxd commented Jul 18, 2018

OK, let's merge this and see!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants