fix+tst: ensure no pybids does not break testing #2248

mgxd · 2017-10-23T22:15:33Z

Fixes #2246 .

Changes proposed in this pull request

skip doctest so pybids dependency isn't required
move actual testing to tests/test_bids.py, with skips for Raise exception if BIDSDataGrabber is imported using Python 2 #2207
new util for checking if packages are installed in editable mode
skip nipype/interfaces/tests/test_resource_monitor.py for the time being

satra · 2017-10-23T23:13:11Z

nipype/interfaces/bids_utils.py

-    >>> results = bg.run()
-    >>> basename(results.outputs.anat[0]) # doctest: +ALLOW_UNICODE
-    'sub-01_T1w.nii.gz'
+    >>> bg = BIDSDataGrabber() # doctest: +SKIP


you don't need doctest skip on everything.

satra · 2017-10-23T23:13:57Z

nipype/interfaces/bids_utils.py

-    >>> basename(results.outputs.anat[0]) # doctest: +ALLOW_UNICODE
-    'sub-01_T1w.nii.gz'
+    >>> bg = BIDSDataGrabber() # doctest: +SKIP
+    >>> bg.inputs.base_dir = 'ds005/' # doctest: +SKIP


you could add a dummy directory with a fake file or reuse an existing directory from the testing/data folder

we want to add more testing data to nipype? we'll need a dedicated bids layout for grabbids

satra · 2017-10-23T23:14:20Z

nipype/interfaces/bids_utils.py

->>> import bids
->>> filepath = os.path.realpath(os.path.dirname(bids.__file__))
->>> datadir = os.path.realpath(os.path.join(filepath, 'grabbids/tests/data/'))
->>> os.chdir(datadir)


you can replace this with the standard header that changes to the testing/data folder.

Aren't we skipping these tests anyways since it would require pybids to be installed?

correct, but we can make a directory ds005 with a dummy file (since git does not store empty directories), just not call the bidsgrabber run function. we won't actually call pybids here.

satra · 2017-10-23T23:15:45Z

nipype/interfaces/tests/test_bids.py

+                    reason="Pybids is not installed")
+@pytest.mark.skipif(sys.version_info < (3, 0),
+                    reason="Pybids no longer supports Python 2")
+@pytest.mark.skipif(not dist_is_editable('pybids'),


why is this bit necessary?

from the initial #2174 - that's how the testing data was done

also #2207

satra · 2017-10-23T23:16:03Z

nipype/interfaces/bids_utils.py

+    >>> bg.inputs.output_query['dwi'] = dict(modality='dwi') # doctest: +SKIP
+    >>> results = bg.run() # doctest: +SKIP
+    >>> basename(results.outputs.dwi[0]) # doctest: +SKIP
+    'sub-01_dwi.nii.gz' # doctest: +SKIP


same comments as above.

codecov-io · 2017-10-24T02:08:50Z

Codecov Report

Merging #2248 into master will increase coverage by 0.01%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master    #2248      +/-   ##
==========================================
+ Coverage   72.32%   72.33%   +0.01%     
==========================================
  Files        1183     1184       +1     
  Lines       59008    59048      +40     
  Branches     8490     8492       +2     
==========================================
+ Hits        42679    42715      +36     
- Misses      14945    14948       +3     
- Partials     1384     1385       +1

Flag	Coverage Δ
#smoketests	`72.33% <90.47%> (+0.01%)`	⬆️
#unittests	`69.91% <90.47%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
nipype/interfaces/bids_utils.py	`81.03% <100%> (ø)`	⬆️
nipype/utils/filemanip.py	`84.71% <66.66%> (-0.36%)`	⬇️
nipype/interfaces/tests/test_bids.py	`94.11% <94.11%> (ø)`

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 96c359d...78c15e4. Read the comment docs.

codecov-io · 2017-10-24T02:09:19Z

Codecov Report

Merging #2248 into master will increase coverage by 0.01%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #2248      +/-   ##
==========================================
+ Coverage   72.32%   72.34%   +0.01%     
==========================================
  Files        1185     1186       +1     
  Lines       59036    59077      +41     
  Branches     8491     8493       +2     
==========================================
+ Hits        42700    42739      +39     
- Misses      14952    14953       +1     
- Partials     1384     1385       +1

Flag	Coverage Δ
#smoketests	`72.34% <87.5%> (+0.01%)`	⬆️
#unittests	`69.92% <87.5%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
nipype/interfaces/tests/test_resource_monitor.py	`60.97% <100%> (+0.97%)`	⬆️
nipype/utils/filemanip.py	`84.71% <66.66%> (-0.36%)`	⬇️
nipype/interfaces/bids_utils.py	`81.03% <71.42%> (ø)`	⬆️
nipype/interfaces/tests/test_bids.py	`94.11% <94.11%> (ø)`
nipype/utils/profiler.py	`41.56% <0%> (+1.2%)`	⬆️

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 1a74013...20eccd3. Read the comment docs.

mgxd · 2017-10-24T14:57:03Z

There seems to be a mismatch in Travis' 3.4 build, which looks to be silently failing in older PRs as well...
perhaps the conda-forge channel should be added earlier to ensure no download of ipaddress

The following packages will be UPDATED:
 conda: 4.3.27-py36h2866c0b_0 --> 4.3.30-py36h5d9f9f4_0
Fetching package metadata ...........
Solving package specifications: .
UnsatisfiableError: The following specifications were found to be in conflict:
 - python 3.4*
 - urllib3 -> ipaddress -> python 2.7*
Use "conda info <package>" to see the dependencies for each package.

satra · 2017-10-24T15:47:25Z

.travis.yml

  conda config --add channels conda-forge &&
+  conda install python=${TRAVIS_PYTHON_VERSION} &&


now that we can install almost any python dependencies with pip, would it make sense to simplify this for the time being to just installing with pip -y -r requirements.txt instead of using conda install in the next step.

satra · 2017-10-24T15:49:33Z

nipype/interfaces/bids_utils.py

    >>> bg.inputs.base_dir = 'ds005/'
    >>> bg.inputs.subject = '01'
-    >>> results = bg.run()
-    >>> basename(results.outputs.anat[0]) # doctest: +ALLOW_UNICODE
+    >>> results = bg.run() # doctest


skip this and remove the next step.

satra · 2017-10-24T15:49:41Z

nipype/interfaces/bids_utils.py

    By default, the BIDSDataGrabber fetches anatomical and functional images
    from a project, and makes BIDS entities (e.g. subject) available for
    filtering outputs.

-    >>> bg = BIDSDataGrabber()
+    >>> bg = BIDSDataGrabber() # doctest: +SKIP


don't skip this.

bidsdatagrabber should initialize without pybids, just not run without it.

talking of which it would be useful in run_interface to do:

if not have_pybids: raise ImportError('Could not import pybids')

…ncies from requirements.txt

satra · 2017-10-26T19:28:33Z

nipype/interfaces/bids_utils.py


    """
    input_spec = BIDSDataGrabberInputSpec
    output_spec = DynamicTraitedSpec
    _always_run = True

-    def __init__(self, infields=None, **kwargs):
+    def __init__(self, infields=[], **kwargs):


i would revert this and check for not infields is None

satra · 2017-10-27T12:10:14Z

nipype/interfaces/bids_utils.py

@@ -123,13 +107,17 @@ def __init__(self, infields=None, **kwargs):

        # used for mandatory inputs check
        undefined_traits = {}
-        for key in infields:
+        for key in infields or []:


this should be done in list outputs as well.

and travis tests are not testing the have_bids==True branch.

@satra this might be required here, since it's using **kwargs and setting traits

…test

fix+tst: ensure no pybids does not break testing

78c15e4

satra reviewed Oct 23, 2017

View reviewed changes

mgxd added 2 commits October 24, 2017 11:44

tst: add condaforge before installing python

3fda1c7

doctest: less SKIP comments

77fb3c4

satra reviewed Oct 24, 2017

View reviewed changes

mgxd mentioned this pull request Oct 24, 2017

Test Errors on a readonly file system #1916

Closed

mgxd added this to the 0.14.0 milestone Oct 24, 2017

mgxd added 2 commits October 24, 2017 17:23

ref+tst: allow bidsgrabber to initialize without pybids, grab depende…

08ec2b3

…ncies from requirements.txt

tst: fix travis file

af459a8

satra reviewed Oct 26, 2017

View reviewed changes

rev: initiate fields as None

0b16c85

satra mentioned this pull request Oct 27, 2017

testing the install: base.py:1600: RuntimeError and others #2258

Closed

satra reviewed Oct 27, 2017

View reviewed changes

mgxd added 4 commits October 27, 2017 11:48

tst+rf: run pybids with travis, minor tweaks to interface

bb0ecff

fix: skip resource monitor test until consistent

ac45369

rev: set traits inside __init__

c87399e

Merge branch 'master' of git://github.com/nipy/nipype into fix/nipype…

20eccd3

…test

satra merged commit 1a4efff into nipy:master Oct 28, 2017

satra modified the milestone: 0.14.0 Oct 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix+tst: ensure no pybids does not break testing #2248

fix+tst: ensure no pybids does not break testing #2248

mgxd commented Oct 23, 2017 •

edited

Loading

satra Oct 23, 2017

satra Oct 23, 2017

mgxd Oct 24, 2017

satra Oct 23, 2017

mgxd Oct 24, 2017

satra Oct 24, 2017

satra Oct 23, 2017

mgxd Oct 24, 2017

satra Oct 23, 2017

codecov-io commented Oct 24, 2017 •

edited

Loading

codecov-io commented Oct 24, 2017 •

edited

Loading

mgxd commented Oct 24, 2017 •

edited

Loading

satra Oct 24, 2017

satra Oct 24, 2017

satra Oct 24, 2017

satra Oct 24, 2017

satra Oct 26, 2017

satra Oct 27, 2017

mgxd Oct 27, 2017

		conda config --add channels conda-forge &&
		conda install python=${TRAVIS_PYTHON_VERSION} &&

fix+tst: ensure no pybids does not break testing #2248

fix+tst: ensure no pybids does not break testing #2248

Conversation

mgxd commented Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 24, 2017 • edited Loading

Codecov Report

codecov-io commented Oct 24, 2017 • edited Loading

Codecov Report

mgxd commented Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgxd commented Oct 23, 2017 •

edited

Loading

codecov-io commented Oct 24, 2017 •

edited

Loading

codecov-io commented Oct 24, 2017 •

edited

Loading

mgxd commented Oct 24, 2017 •

edited

Loading