Run tests in parallel with pytest-xdist.

[bdice]

Description

This PR adds the plugin pytest-xdist to run tests in parallel. Each CircleCI machine has two cores. This cuts off around 45 seconds from each CI job, which adds up to a decent amount of time across all jobs. The output is a little more verbose.

Before:

After (ignore pypy, I pushed another commit before it finished):

Motivation and Context

Might as well use all of the CI resources we are given. Saves a bit of time and probably reduces the energy needed by the server since it can sleep sooner.

Types of Changes

• Documentation update
• Bug fix
• New feature
• Breaking change¹

¹The change breaks (or has the potential to break) existing functionality.

Checklist:

• I am familiar with the Contributing Guidelines.
• I agree with the terms of the Contributor Agreement.
• My name is on the list of contributors.
• My code follows the code style guideline of this project.
• The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:

• I have updated the API documentation as part of the package doc-strings.
• I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
• I have updated the changelog and added all related issue and pull request numbers for future reference (if applicable). See example below.

[codecov]

Codecov Report

Merging #705 (e7edb89) into master (dfc28cb) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #705   +/-   ##
=======================================
  Coverage   78.54%   78.54%           
=======================================
  Files          65       65           
  Lines        7141     7141           
  Branches     1565     1565           
=======================================
  Hits         5609     5609           
  Misses       1228     1228           
  Partials      304      304           

---

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 dfc28cb...e7edb89. Read the comment docs.

[mikemhenry]

Maybe do -n auto so that if we ever have more cores/different CI we don't need to change the number?

[bdice]

Maybe do -n auto so that if we ever have more cores/different CI we don't need to change the number?

Unfortunately the CI machine claims to have 36 cores but only schedules 2. Auto ends up oversubscribing really badly.

[mikemhenry]

Maybe do -n auto so that if we ever have more cores/different CI we don't need to change the number?

Unfortunately the CI machine claims to have 36 cores but only schedules 2. Auto ends up oversubscribing really badly.

Yikes! Okay that makes sense!
(sorry I see you tried that here: e77fcdc)

[joaander]

Maybe do -n auto so that if we ever have more cores/different CI we don't need to change the number?

Unfortunately the CI machine claims to have 36 cores but only schedules 2. Auto ends up oversubscribing really badly.

Yikes! Okay that makes sense! (sorry I see you tried that here: e77fcdc)

As far as I know, this is specific to circleci. GitHub Actions and Azure pipelines use VMs that advertise the correct number of cores.

[vyasr]

I think we can probably run with this change for now and revisit if we ever switch to a different CI executor, then?

[joaander]

I think we can probably run with this change for now and revisit if we ever switch to a different CI executor, then?

Yes. I wasn't suggesting action - just providing information for context.

[cbkerr]

Nice speedup! Make the changelog entry more specific then this is good to go!

[csadorf]

Great! Thanks a lot.

Do you know what is going on with the failing py38 test? Looks like some multi-threaded tests are failing?

[vyasr]

Great! Thanks a lot.

Do you know what is going on with the failing py38 test? Looks like some multi-threaded tests are failing?

It looks like one of the errors is that this line in the test should be modified to catch a KeyError. The existing except clause isn't broad enough in detecting the ways in which multithreaded execution can fail when multithreading support is disabled.

My best guess is that the other failure is due to #397, specifically the case that I decided was out of scope when closing that issue: process-parallelism safe buffering. I suspect that we only see failures on PyPy because it executes much faster than CPython. I observed the same type of issue many times when constructing this test, specifically because sequences of operations that could fail when multithreading support was disabled would succeed in PyPy because threads completed too quickly to be preempted and so data was never corrupted even without acquiring mutexes.

Buffering safe for multiple processes is still out of scope (especially in conjunction with PyPy), so I don't think diagnosing the exact failure here is worth much further investigation. My recommendation is to check if we're using pytest-xdist (either using the PYTEST_XDIST_WORKER environment variable or another mechanism described in the pytest-xdist docs) and whether we're running in PyPy (the platform check is already being done for other things in this file), and if both of those conditions are true just skip the multithreaded test in buffered mode. Note that we do not want to skip the multithreaded test in unbuffered collections, so you'll probably need to do something like override the test defined in SyncedCollectionTest with another test defined in BufferedJSONCollectionTest that skips the xdist+PyPy case and otherwise calls the super() test.

[vyasr]

The same failure of the multithreaded test with PyPy is showing up on #706. It's possible that tests are suddenly detecting a failure in the multithreading logic that they haven't for the past year, although I don't know why that would be. A possible culprit is #658, which bumped our testing environment for pypy from 3.7 to 3.8. Perhaps something is different in PyPy 3.8 vs 3.7.

Now that we've seen this failure on two PRs, my recommendation would be to merge this PR as is and to investigate further in a separate PR. The easiest solution would just be to disable threading support on PyPy if we can't find an underlying cause. I invested a lot of time trying to get to the bottom of this issue in the past and I am unlikely to do so again in the near future.

[vyasr]

It looks like the first issue that I diagnosed was the underlying problem. I pushed a fix in #710 that resolved the problem for me locally, let's see if tests pass on CI.

EDIT
CI passed. I think we can merge this (with failing tests) after #710 and be fine.