ENH: REF: SSHDataGrabber should grab related file

[ashgillman]

SSHDataGrabber will optionally download files, however if requested file is a header file, the corresponding data file won't be downloaded. This PR fixes this issue.

Changes

• Refactored current file downloading, which was replicated in two areas, to one function
• Added test for SSHDataGrabber, demonstrating the issue and its correction
• When downloading files, and files with a related file type in the same directory are also downloaded

[ashgillman]

The CircleCI failure looks to be unrelated: an issue w/ dependency resolution for FSL and AFNI

[jdkent]

Looks like the same/similar issue we had here: nipreps/fmriprep#566, probably need to update the docker file in a separate pull request.

[effigies]

This was fixed in #2075. Just merge or rebase on master.

[ashgillman]

Ah thanks, I thought the CI would have run against what the code would be after a merge, not the pre-merge. I have rebased

[codecov-io]

Codecov Report

Merging #2104 into master will increase coverage by 23.58%.
The diff coverage is 12.5%.

@@             Coverage Diff             @@
##           master    #2104       +/-   ##
===========================================
+ Coverage   48.56%   72.15%   +23.58%     
===========================================
  Files         116     1144     +1028     
  Lines       23598    57629    +34031     
  Branches        0     8246     +8246     
===========================================
+ Hits        11461    41582    +30121     
- Misses      12137    14746     +2609     
- Partials        0     1301     +1301

Flag	Coverage Δ
#smoketests	72.15% <12.5%> (+23.58%)	⬆️
#unittests	69.78% <12.5%> (?)

Impacted Files	Coverage Δ
nipype/interfaces/tests/test_io.py	69.33% <17.64%> (ø)
nipype/interfaces/io.py	53.55% <7.89%> (+18.62%)	⬆️
...ilities/tests/test_auto_BRAINSLinearModelerEPCA.py	78.57% <0%> (ø)
nipype/interfaces/camino/calib.py	80.7% <0%> (ø)
...terfaces/camino/tests/test_auto_TrackBayesDirac.py	85.71% <0%> (ø)
nipype/interfaces/niftyfit/dwi.py	95.86% <0%> (ø)
...pype/algorithms/tests/test_auto_SimpleThreshold.py	85.71% <0%> (ø)
...es/diffusion_toolkit/tests/test_auto_DTITracker.py	85.71% <0%> (ø)
.../utilities/tests/test_auto_BRAINSSnapShotWriter.py	85.71% <0%> (ø)
...filtering/tests/test_auto_SubtractScalarVolumes.py	100% <0%> (ø)
... and 1085 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 af2b7aa...69c948a. Read the comment docs.

[codecov-io]

Codecov Report

Merging #2104 into master will increase coverage by 0.01%.
The diff coverage is 7.89%.

@@            Coverage Diff             @@
##           master    #2104      +/-   ##
==========================================
+ Coverage   66.62%   66.63%   +0.01%     
==========================================
  Files         328      328              
  Lines       42508    42497      -11     
  Branches     5280     5275       -5     
==========================================
- Hits        28321    28319       -2     
+ Misses      13505    13496       -9     
  Partials      682      682

Flag	Coverage Δ
#smoketests	50.75% <7.89%> (+0.02%)	⬆️
#unittests	63.97% <7.89%> (+0.01%)	⬆️

Impacted Files	Coverage Δ
nipype/info.py	84.61% <ø> (ø)	⬆️
nipype/interfaces/io.py	54.16% <7.89%> (+0.3%)	⬆️

---

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 04d0159...7b5d667. Read the comment docs.

[ashgillman]

I'm actually surprised the SSH tests ran already... The CI must be able to SSH back to itself by default.

But I went through the logs and this line exists:

../src/nipype/nipype/interfaces/io.py::nipype.interfaces.io.SSHDataGrabber PASSED

And I confirmed that the test fails locally without the final commit locally.

[ashgillman]

Uh oh, my bad:

../src/nipype/nipype/interfaces/tests/test_io.py::test_SSHDataGrabber SKIPPED

I guess I need to add paramiko to the CI dependencies.

[ashgillman]

So it looks like SSHing to localhost on CI might be a little tricky, I thought it would be a simple way to add tests of the functionality.

What do others think of leaving the test, but marking it not to be checked my CI? The functionality is currently not tested at all, at least this way it can be manually tested.

[effigies]

Sorry I completely dropped the ball on this. If I'm understanding the issue, it looks like there's no SSH server? If so, I guess there are three options:

1. Skip the test, and make sure it works on your own machine.
2. Install openssh-server in the nipype containers, so you can login to localhost.
3. Spin up an SSH server with parmiko (a quick look at the docs suggests this is possible, but I have no familiarity with the library and may have misinterpreted)

Obviously the first is the least work. I think in principle the third is ideal. But I don't see any particular harm in (2), if configured only to accept connections from localhost. Managing keys could get fiddly.

In any event, you'll need to merge master and resolve conflicts to resume this PR. Apologies again.

[ashgillman]

Hey. Thanks for responding, I was actually just looking for this yesterday, I have been rebasing a few changes. I think 2 would be a good solution. If I rebase, are you able to help with the container side?

[effigies]

Sure.

[ashgillman]

Okay, rebased and test_io.py running successfully locally

[effigies]

Great, thanks. I spent some time last night playing with installing and running openssh-server on nipype Docker images. It's somewhat complicated by the fact that there doesn't seem to be a good way to run a daemon in a container directly. It can be done, but I'm not sure that it's a good approach, since it'll need to be scripted from outside Docker.

I'm going to have a look at creating servers with paramiko. If we can simulate a server from within the tests, I think that would be ideal.

In the short term, I think it would be useful to have another pytest skip criterion that checks whether there is an SSH server running on localhost (with a port parameter that defaults to 22). Given that we need paramiko installed anyway, perhaps there is functionality there that will make it easy.

[ashgillman]

Good temp fix.

Python2:

[nix-shell:~/dev/nipy@github.com/nipype]$ sudo systemctl start ssh                                                                       
                                                                                                                                         
[nix-shell:~/dev/nipy@github.com/nipype]$ pytest nipype/interfaces/tests/test_io.py                                                      
========================================================== test session starts ===========================================================
platform linux2 -- Python 2.7.14, pytest-3.2.5, py-1.4.34, pluggy-0.4.0                                                                  
rootdir: /home/gil2a4/dev/nipy@github.com/nipype/nipype, inifile: pytest.ini                                                             
collected 28 items                                                                                                                        
                                                                                                                                         
nipype/interfaces/tests/test_io.py .s......s..ss...........sss.                                                                          
                                                                                                                                         
================================================== 21 passed, 7 skipped in 0.92 seconds ==================================================
                                                                                                                                         
[nix-shell:~/dev/nipy@github.com/nipype]$ sudo systemctl stop ssh                                                                        
                                                                                                                                         
[nix-shell:~/dev/nipy@github.com/nipype]$ pytest nipype/interfaces/tests/test_io.py                                                      
========================================================== test session starts ===========================================================
platform linux2 -- Python 2.7.14, pytest-3.2.5, py-1.4.34, pluggy-0.4.0
rootdir: /home/gil2a4/dev/nipy@github.com/nipype/nipype, inifile: pytest.ini                                                             
collected 28 items                                                                                                                        
                                                                                                                                         
nipype/interfaces/tests/test_io.py .s......s..ss...........ssss                                                                          
                                                                                                                                         
================================================== 20 passed, 8 skipped in 0.12 seconds ==================================================

And Python 3

[nix-shell:~/dev/nipy@github.com/nipype]$ sudo systemctl start ssh                                                                        
[sudo] password for gil2a4:                                                                                                                                                                                                                                                         
[nix-shell:~/dev/nipy@github.com/nipype]$ pytest nipype/interfaces/tests/test_io.py                                                       ========================================================== test session starts ===========================================================
platform linux -- Python 3.6.4, pytest-3.2.5, py-1.4.34, pluggy-0.4.0                                                                     rootdir: /home/gil2a4/dev/nipy@github.com/nipype/nipype, inifile: pytest.ini                                                              
collected 28 items                                                                                                                                                                                                                                                                  
nipype/interfaces/tests/test_io.py .s......s..ss...........sss.                                                                                                                                                                                                                     
================================================== 21 passed, 7 skipped in 0.78 seconds ==================================================
                                                                                                                                          
[nix-shell:~/dev/nipy@github.com/nipype]$ sudo systemctl stop ssh                                                                         
                                                                                                                                          
[nix-shell:~/dev/nipy@github.com/nipype]$ pytest nipype/interfaces/tests/test_io.py                                                       
========================================================== test session starts ===========================================================
platform linux -- Python 3.6.4, pytest-3.2.5, py-1.4.34, pluggy-0.4.0                                                                     
rootdir: /home/gil2a4/dev/nipy@github.com/nipype/nipype, inifile: pytest.ini                                                              
collected 28 items                                                                                                                        
                                                                                                                                          
nipype/interfaces/tests/test_io.py .s......s..ss...........ssss                                                                           
                                                                                                                                         
================================================== 20 passed, 8 skipped in 0.14 seconds ==================================================

[effigies]

Hey, thanks for poking at this some more. Just as an update, I've taken a couple shots at creating an SSH server with paramiko and have failed pretty hard. I'm a little inclined to verify your tests work locally, skip properly on circle, and add an issue to hack together an SSH server in a separate PR.

[ashgillman]

Yeah, my /very limited/ search revealed that it might not be as trivial as it at first seems

[effigies]

Okay, I can verify tests pass locally, and turning my SSH server off and on toggles the number of skipped tests. Pushing a fix for the Dockerfile (we now edit a Dockerfile-generating script), and I think this should be good to go, as far as I'm concerned.

[ashgillman]

Yep, assuming CI doesn't fail again.

Not sure why Circle CI threw a different error (OSError) to locally. I guess maybe it isn't so much refusing to connect as refusing to try to connect (ssh server vs ssh client error?).

[ashgillman]

Ah sorry, I was wondering what the go with that Dockerfile was when I was rebasing. I guess I accidentally added it back in

[ashgillman]

Not sure about that 1 Travis error... Some sort of time out? Can you ask it to run that test again?

[effigies]

Looks like a Python 2 issue.

[effigies]

The CircleCI build had a Cannot assign requested address socket error, which suggests that possibly localhost isn't bound to an address, so I switched to using 127.0.0.1. And the Travis error was a timeout, so I added a 10s timeout to the SSH connect statements. Let's see if those work.

[effigies]

Waiting on the examples to run, but the tests are passing, so I'm calling this ready.

Will merge at the end of the day unless tests fail or someone else has comments. A second pair of eyes would be appreciated.

[ashgillman]

Strange that you'd only get such errors with Python 2, but glad the tests ran. Thank for adding the last bit of work, I ran out of time on Friday afternoon :)