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

Run the pytest testsuite in parallel #1418

Merged
merged 7 commits into from Jun 25, 2017

Conversation

@utkbansal
Copy link
Member

@utkbansal utkbansal commented Jun 21, 2017

Fixes #

Changes made in this Pull Request:

  • use pytest-xdist to run in parallel on 2 cores

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?
.travis.yml Outdated
@@ -24,15 +24,15 @@ env:
- COVERALLS=false
- NOSE_FLAGS="--processes=2 --process-timeout=400 --no-open-files --with-timer --timer-top-n 50"
- NOSE_TEST_LIST="analysis auxiliary coordinates core formats topology utils"
- PYTEST_FLAGS="--disable-pytest-warnings"
- PYTEST_FLAGS="--disable-pytest-warnings -n 2"

This comment has been minimized.

@kain88-de

kain88-de Jun 21, 2017
Member

can you please use --numprocesses it's more explicit and I'm trying to get upstream ci-helpers to detect that to auto install pytest-xdist.

This comment has been minimized.

@utkbansal

utkbansal Jun 21, 2017
Author Member

Sure. I'll just let this build complete and then push.

This comment has been minimized.

@utkbansal

utkbansal Jun 22, 2017
Author Member

@kain88-de That doesn't work. I don't think there is anything like --numprocesses

@utkbansal utkbansal force-pushed the utkbansal:pytest-parallel branch from 2d5fe43 to ebfc117 Jun 21, 2017
@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 21, 2017

@kain88-de Should be ready to merge. No drop in coverage.

Copy link
Member

@richardjgowers richardjgowers left a comment

Looks like coverage isn't working in parallel

Coverage.py warning: Module MDAnalysis was never imported. (module-not-imported)

Coverage.py warning: No data was collected. (no-data-collected)

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 22, 2017

This is weird, I think I saw no drop in coverage last night. Let me cross check.

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 22, 2017

I wonder how pytest shows the percentage of coverage when it did not collect data.

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 22, 2017

@richardjgowers @jbarnoud @kain88-de @mnmelo I have a wild theory - the warnings might just be meaningless.

I ran pytest on the lib module with coverage, once serially and once in parallel - both times the coverage reported was the same. I assumed that the difference might be too small and it might be a round off error since the coverage was an integer.

So, I ran the coordinates module - because it is large in terms of the number of tests. The reasoning was that the difference in coverage would be larger and there should be a tangible difference even after rounding off to the nearest integer. I still got the same coverage in both cases - 41%

Commands I used -

Parallel: py.test --cov=MDAnalysis coordinates --cov-report=html -q --disable-pytest-warnings -n 2
Serial: py.test --cov=MDAnalysis coordinates --cov-report=html -q --disable-pytest-warnings

Can someone run these commands locally (off the port coordinates PR) and confirm my findings?

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 22, 2017

So the build passed. The coverage is still at 88.837% Was there a drop? The button on the README file shows the same number.

.travis.yml Outdated
- PYTEST_LIST="testsuite/MDAnalysisTests/lib testsuite/MDAnalysisTests/formats"
- NOSE_COVERAGE_FILE="nose_coverage"
- PYTEST_COVERAGE_FILE="pytest_coverage"
- MAIN_CMD="pytest ${PYTEST_LIST} ${PYTEST_FLAGS}; python ./testsuite/MDAnalysisTests/mda_nosetests ${NOSE_TEST_LIST} ${NOSE_FLAGS}"
- SETUP_CMD=""
- BUILD_CMD="pip install -v package/ && pip install testsuite/"
- CONDA_DEPENDENCIES="mmtf-python nose=1.3.7 mock six biopython networkx cython joblib nose-timer pytest=3.1.2 pytest-cov=2.5.1"
- CONDA_ALL_DEPENDENCIES="mmtf-python nose=1.3.7 mock six biopython networkx cython joblib nose-timer matplotlib netcdf4 scikit-learn scipy seaborn coveralls clustalw=2.1 pytest=3.1.2 pytest-cov=2.5.1"
- CONDA_DEPENDENCIES="mmtf-python nose=1.3.7 mock six biopython networkx cython joblib nose-timer pytest=3.1.2 pytest-cov=2.5.1 pytest-xdist=1.17.1"

This comment has been minimized.

@kain88-de

kain88-de Jun 22, 2017
Member

Could you please remove the version pinning. I'm trying to get everything updated to we can take use of ci-helpers magic to autoinstall things for us. You should be able to remove the pytest explicit dependency as well.

I'm hijacking this PR to clean up some installations of pytest.

@utkbansal utkbansal force-pushed the utkbansal:pytest-parallel branch from 0b8f1c3 to 1546462 Jun 23, 2017
@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 23, 2017

@kain88-de @jbarnoud @richardjgowers I don't really like the way pytest displays its output (listing each file and the other stuff). I'd like it to be more like what nose does currently. Can I add the -q flag to minimize the noise?

Copy link
Member

@richardjgowers richardjgowers left a comment

Yeah, looks like we are getting two coverage reports, despite being told for the pytest version that we didn't gather coverage. Maybe it's a screw up between the parallel processes in pytest not communicating with each other.

.travis.yml Outdated
- PYTEST_LIST="testsuite/MDAnalysisTests/lib testsuite/MDAnalysisTests/formats"
- NOSE_COVERAGE_FILE="nose_coverage"
- PYTEST_COVERAGE_FILE="pytest_coverage"
- MAIN_CMD="pytest ${PYTEST_LIST} ${PYTEST_FLAGS}; python ./testsuite/MDAnalysisTests/mda_nosetests ${NOSE_TEST_LIST} ${NOSE_FLAGS}"
- SETUP_CMD=""
- BUILD_CMD="pip install -v package/ && pip install testsuite/"
- CONDA_DEPENDENCIES="mmtf-python nose=1.3.7 mock six biopython networkx cython joblib nose-timer matplotlib scipy griddataformats pytest=3.1.2 pytest-cov=2.5.1"
- CONDA_ALL_DEPENDENCIES="mmtf-python nose=1.3.7 mock six biopython networkx cython joblib nose-timer matplotlib netcdf4 scikit-learn scipy griddataformats seaborn coveralls clustalw=2.1 pytest=3.1.2 pytest-cov=2.5.1"
- CONDA_DEPENDENCIES="mmtf-python nose=1.3.7 mock six biopython networkx cython joblib nose-timer matplotlib scipy griddataformats pytest-cov pytest-xdist"

This comment has been minimized.

@kain88-de

kain88-de Jun 24, 2017
Member

@utkbansal you should also be able to drop the pytest-cov package as an explicit dependency. See astropy/ci-helpers#216

This comment has been minimized.

@utkbansal

utkbansal Jun 24, 2017
Author Member

Cool.

This comment has been minimized.

@richardjgowers

richardjgowers Jun 25, 2017
Member

So here do we need to list pytest-cov? And don't we need to list pytest?

@richardjgowers
Copy link
Member

@richardjgowers richardjgowers commented Jun 24, 2017

@utkbansal rebase this and we're good to go

@utkbansal utkbansal force-pushed the utkbansal:pytest-parallel branch from 1546462 to 575aabd Jun 24, 2017
@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 24, 2017

@richardjgowers The rebase for this depends on what we want to do about #1414

It wasn't supposed to be merged yet.

  • Should I open a new PR for the coordinates module and fix coverage in that? Then I can drop coordinates from NOSE_TEST_LIST here

  • Or if we will revert #1414 then, I have to drop coordinates from PYTEST_TEST_LIST.

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 24, 2017

@richardjgowers @jbarnoud @kain88-de Something has gone wrong while merging my PRs.

All the builds on the develop branch and my PR are failing with the following error -

import MDAnalysis as mda
E     File "/Users/utkbansal/Code/mdanalysis/package/MDAnalysis/__init__.py", line 148
E       import logging
E            ^
E   SyntaxError: invalid syntax
@jbarnoud
Copy link
Contributor

@jbarnoud jbarnoud commented Jun 24, 2017

@kain88-de @richardjgowers This is the commit responsible for the error. Line 146 of __init__.py is broken.

@jbarnoud
Copy link
Contributor

@jbarnoud jbarnoud commented Jun 24, 2017

I forgot to paste the link to the commit: d99abba

@richardjgowers
Copy link
Member

@richardjgowers richardjgowers commented Jun 24, 2017

We won't revert 1414

@utkbansal utkbansal force-pushed the utkbansal:pytest-parallel branch from f8f28a0 to d599baf Jun 25, 2017
@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 25, 2017

This should be ready too once the build completes.

@richardjgowers
Copy link
Member

@richardjgowers richardjgowers commented Jun 25, 2017

@utkbansal needs a rebase thanks to utils

utkbansal added 3 commits Jun 21, 2017
utkbansal added 2 commits Jun 22, 2017
@utkbansal utkbansal force-pushed the utkbansal:pytest-parallel branch from d599baf to 7f9cd3d Jun 25, 2017
@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 25, 2017

Now how did utils slip away!

Copy link
Member

@richardjgowers richardjgowers left a comment

This reverts utils

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 25, 2017

@richardjgowers Got it back.

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 25, 2017

@richardjgowers Looks good to go. The full build fails due to the time limit though.

@richardjgowers richardjgowers merged commit 2d969b7 into MDAnalysis:develop Jun 25, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
QuantifiedCode No new issues introduced.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants