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

CI: Test Python 3.8 #3154

Merged
merged 5 commits into from Apr 20, 2020
Merged

CI: Test Python 3.8 #3154

merged 5 commits into from Apr 20, 2020

Conversation

effigies
Copy link
Member

@effigies effigies commented Jan 15, 2020

Closes #3098.

@effigies effigies added this to the 1.4.1 milestone Jan 15, 2020
@effigies
Copy link
Member Author

@effigies effigies modified the milestones: 1.4.1, 1.5.0 Jan 26, 2020
@effigies effigies mentioned this pull request Feb 23, 2020
17 tasks
@codecov
Copy link

codecov bot commented Feb 23, 2020

Codecov Report

Merging #3154 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3154      +/-   ##
==========================================
+ Coverage   64.89%   65.01%   +0.11%     
==========================================
  Files         301      301              
  Lines       39717    39833     +116     
  Branches     5268     5269       +1     
==========================================
+ Hits        25775    25896     +121     
+ Misses      12884    12880       -4     
+ Partials     1058     1057       -1     
Flag Coverage Δ
#unittests 65.01% <100.00%> (+0.11%) ⬆️
Impacted Files Coverage Δ
nipype/info.py 92.30% <100.00%> (ø)
nipype/interfaces/minc/minc.py 85.18% <0.00%> (+0.01%) ⬆️
nipype/interfaces/fsl/preprocess.py 71.74% <0.00%> (+0.03%) ⬆️
nipype/interfaces/ants/registration.py 73.09% <0.00%> (+0.05%) ⬆️
nipype/interfaces/base/specs.py 89.28% <0.00%> (+0.05%) ⬆️
nipype/interfaces/base/traits_extension.py 88.29% <0.00%> (+0.06%) ⬆️
nipype/utils/filemanip.py 72.94% <0.00%> (+0.06%) ⬆️
nipype/testing/fixtures.py 98.70% <0.00%> (+0.09%) ⬆️
nipype/interfaces/niftyreg/regutils.py 80.76% <0.00%> (+0.09%) ⬆️
nipype/interfaces/niftyreg/reg.py 86.00% <0.00%> (+0.09%) ⬆️
... and 26 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 28ef1af...ab4298a. Read the comment docs.

@effigies effigies changed the base branch from maint/1.4.x to master February 23, 2020 03:11
@effigies
Copy link
Member Author

It seems that that test actually breaks the multiprocessing pool. Locally it will pass, but the next test will fail to start up jobs. Might also be causing problems with pytest-xdist.

@oesteban Just in case you have ideas.

@effigies effigies removed this from the 1.5.0 milestone Feb 24, 2020
@oesteban
Copy link
Contributor

Sorry, I was away for the weekend. I'm marking this unread and will come back to it later today.

@yarikoptic
Copy link
Member

observing the stall of test_run_multiproc_nondaemon_false while building 1.4.2 on Debian (unstable/sid) as of yesterday Mar 22.

@yarikoptic
Copy link
Member

FWIW test_run_multiproc_nondaemon_true hangs too ;-/

@yarikoptic
Copy link
Member

and test_run_multiproc

@effigies
Copy link
Member Author

effigies commented Mar 23, 2020

To clarify: All for Python 3.8? Or is this a Python 3.7 issue as well?

@oesteban I'm considering marking these as skip if Python >= 3.8.

@yarikoptic
Copy link
Member

I believe it is 3.8 according to process trace:

                               /bin/sh -c export PYTHONPATH=/build/nipype-1.4.2/.pybuild/cpython3_3.8_nipype/build; \ cd build && \          MPLCONFIGDIR=/build/nipype-1.4.2/build LC_ALL=C.UTF-8 py.test-3 -v --doctest-modules ../nipype
                                 /usr/bin/python3 /usr/bin/py.test-3 -v --doctest-modules ../nipype
                                   /usr/bin/python3 /usr/bin/py.test-3 -v --doctest-modules ../nipype
                                   /usr/bin/python3 /usr/bin/py.test-3 -v --doctest-modules ../nipype

also test_hold_job_until_procs_available stalls.

My current diff with skips now
# quilt diff
Index: nipype-1.4.2/nipype/pipeline/plugins/tests/test_legacymultiproc_nondaemon.py
===================================================================
--- nipype-1.4.2.orig/nipype/pipeline/plugins/tests/test_legacymultiproc_nondaemon.py
+++ nipype-1.4.2/nipype/pipeline/plugins/tests/test_legacymultiproc_nondaemon.py
@@ -134,6 +134,7 @@ def run_multiproc_nondaemon_with_flag(no
     return result
 
 
+@pytest.mark.skipif(True, reason="hangs, known upstream")
 def test_run_multiproc_nondaemon_false():
     """
     This is the entry point for the test. Two times a pipe of several
@@ -152,6 +153,7 @@ def test_run_multiproc_nondaemon_false()
     assert shouldHaveFailed
 
 
+@pytest.mark.skipif(True, reason="hangs, known upstream")
 def test_run_multiproc_nondaemon_true():
     # with nondaemon_flag = True, the execution should succeed
     result = run_multiproc_nondaemon_with_flag(True)
Index: nipype-1.4.2/nipype/pipeline/plugins/tests/test_multiproc.py
===================================================================
--- nipype-1.4.2.orig/nipype/pipeline/plugins/tests/test_multiproc.py
+++ nipype-1.4.2/nipype/pipeline/plugins/tests/test_multiproc.py
@@ -33,6 +33,7 @@ class MultiprocTestInterface(nib.BaseInt
         return outputs
 
 
+@pytest.mark.skipif(True, reason="hangs, known upstream")
 def test_run_multiproc(tmpdir):
     tmpdir.chdir()
 
@@ -114,6 +115,7 @@ def test_no_more_threads_than_specified(
         pipe.run(plugin="MultiProc", plugin_args={"n_procs": max_threads})
 
 
+@pytest.mark.skipif(True, reason="hangs, known upstream")
 def test_hold_job_until_procs_available(tmpdir):
     tmpdir.chdir()

@effigies effigies force-pushed the ci/py38 branch 2 times, most recently from fa21602 to c17de57 Compare March 23, 2020 20:01
@effigies
Copy link
Member Author

This seems concerning, as I think this means the multiprocessing plugins can't be tested with Python 3.8. The best case scenario is that this is an interaction with pytest, but that would be very bad...

@yarikoptic
Copy link
Member

eh, I am just an unlucky in general, even nipype/tests/test_nipype.py::test_no_et seems to stall (no etelemetry client is installed if that is relevant), although I believe that on previous run it went through.

@yarikoptic
Copy link
Member

yarikoptic commented Mar 24, 2020

and now they hang on test_provenance... and if I skip that one test_provenance_exists hangs.
given that it is at 99% I wonder if that is just not a side effect of some previous test ...

edit: if I drop my patch altogether and use this PR diff, I am back at hanging at test_run_pbsgraph
edit 2: if I get back to my skips, and also skip test_provenance_exists, I get to the end but the whole process seems to not quit.

@effigies
Copy link
Member Author

Do you have qsub installed on your test rig?

@yarikoptic
Copy link
Member

yarikoptic commented Mar 24, 2020

qsub

I hope (checked to be true) not - there is no any pbs/slurm in the build env. Shouldn't tests requiring it skip if no qsub?

@effigies
Copy link
Member Author

It's currently being marked xfail. I'll propose a PR to make it skipif.

@effigies effigies mentioned this pull request Mar 24, 2020
1 task
@yarikoptic
Copy link
Member

ok, if I take that patch too, I still get stalling on no_et - if I skip it -- on test_provenance... didn't try yet again to extend skips. I will check first if it is etelemetry altogether disabling/removing it.

@effigies
Copy link
Member Author

I can't replicate the provenance one. What versions of prov and rdflib are you using? Are you aware of the weird hack of needing to install neurdflib after prov?

@satra
Copy link
Member

satra commented Mar 24, 2020

hopefully that hack will be resolved after rdflib 5.0.0 is released.

@effigies
Copy link
Member Author

Agreed. Does installing from rdflib@master have any issues?

@satra
Copy link
Member

satra commented Mar 24, 2020

yes, at this point the neurdflib PRs are now in master. so rdflib@master can be used instead of neurdflib.

@effigies
Copy link
Member Author

effigies commented Mar 25, 2020

I've replaced those warnings with the more appropriate raise SkipTest in 858d2dd, and conditioned testing prov on a good version of rdflib in c190a26. (neurdflib claims version 5.0, and the next release of rdflib will be 5.0, so this should be fine.)

@effigies
Copy link
Member Author

Rebased on #3195 to consolidate. @yarikoptic How are things now?

@effigies effigies force-pushed the ci/py38 branch 2 times, most recently from b554ee7 to c33f0e6 Compare March 31, 2020 14:29
@effigies
Copy link
Member Author

I guess we should go ahead with this, as some tests are better than none, but it's probably not a good thing that we can't test (Legacy)MultiProc on Python 3.8+. Do we know if it works at all? Has anybody tried using nipype with Python 3.8 yet?

@effigies
Copy link
Member Author

effigies commented Apr 1, 2020

The prov issues seem to be stochastic... I'm going to try dropping the rdflib/neurdflib dependency and switch to master. That will postpone any inclusion for after the 5.0.0 release, but at this point it seems worth it.

@effigies
Copy link
Member Author

effigies commented Apr 1, 2020

Not resolved by rdflib@master.

@effigies
Copy link
Member Author

effigies commented Apr 1, 2020

Now that I've put a timeout that might give us some debugging output if it does hang again, it won't hang... Rerunning again...

nipype/info.py Outdated Show resolved Hide resolved
@yarikoptic
Copy link
Member

FWIW, I thought that I might just avoid "stall" by simply using older nipype -- nope, elderly nipype 1.1.9 also gets stuck there, so should add skips.

@yarikoptic
Copy link
Member

re test_run_pbsgraph:

It's currently being marked xfail. I'll propose a PR to make it skipif.

could/should it also get a timeout then, since xfail is no good if it just hangs there.

@effigies effigies force-pushed the ci/py38 branch 2 times, most recently from a982342 to 5579985 Compare April 18, 2020 14:00
@effigies
Copy link
Member Author

@yarikoptic Timeouts added to OAR/PBS tests.

doc/requirements.txt Outdated Show resolved Hide resolved
@yarikoptic
Copy link
Member

FWIW, I have not tried this specific version, but I think I have all the skips - but then good old 1.1.9 still hangs at some later tests (at about 98% completness of the tests). If someone gets down to the underlying issue which causes the stalls -- would be very appreciated and would open possibility to upload recent nipype to debian proper.

@effigies
Copy link
Member Author

This gets things going, so I'm going to merge. I'll create an issue for Python 3.8 + multiprocessing, and feel free to open more outstanding issues.

@effigies effigies merged commit 9a7db4d into nipy:master Apr 20, 2020
@effigies effigies deleted the ci/py38 branch April 20, 2020 18:20
@effigies effigies added this to the 1.5.0 milestone May 19, 2020
shnizzedy added a commit to FCP-INDI/C-PAC that referenced this pull request Nov 30, 2020
> we had to skip some tests in Python 3.8 due to changes in how multiprocessing works: nipy/nipype#3154
> I'm not sure whether Nipype will work in general after Python 3.7.
― @effigies, https://mattermost.brainhack.org/brainhack/pl/kxjyek8wk3nwjmcrdyw969kfde
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.

Test on Python 3.8
4 participants