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: Add pytest plugins pytest-xdist and pytest-rerunfailures #3193

Merged
merged 19 commits into from
May 19, 2024
Merged

Conversation

seisman
Copy link
Member

@seisman seisman commented Apr 23, 2024

Our "Tests" workflow sometimes fails due to a few crashed tests, especially on Windows and macOS. The crashes are likely caused by unfixed upstream bugs, and they cause the CI to fail immediately, so other tests are not executed at all, which is very annoying.

Inspired by this post (https://stackoverflow.com/a/32140354), we can use the pytest-xdist to let other tests continue even if one or more tests crash (https://pytest-xdist.readthedocs.io/en/stable/crash.html). The crashed tests are marked as failures.

In addition to pytest-xdist, we can also use the pytest-rerunfailures plugin, which can rerun failing tests multiple times. So, if a test crashes but passes when rerun, then the test should be good.

Initially, I thought we have to use -n 1 to run the tests using one process because PyGMT uses a single global session and can't be run parallel without import pygmt in each process (xref #217). However, it turns out -n auto works, which is unexpected. Here, -n auto means using as many processes as the number of cores (2 for Linux/Windows and 3 for macOS with the GitHub-hosted agents), which speed up the CI runs significantly.

Here is the comparison of the "Run tests" step of the latest runs of the main branch (https://github.com/GenericMappingTools/pygmt/actions/runs/8825374546) and this PR (https://github.com/GenericMappingTools/pygmt/actions/runs/8825127559):

Job Main Branch This PR
Linux Python 3.10 84 s 54 s
Linux Python 3.12 100 s 64 s
macOS Python 3.10 90 s 37 s
macOS Python 3.12 100 s 50 s
Windows Python 3.10 230 s 135 s
Windows Python 3.12 crash at 150 s 180 s

Looking at the detailed log in https://github.com/GenericMappingTools/pygmt/actions/runs/8825127559/job/24228863150, we can see that the test_tilemap_no_clip test crashed but passed in rerun, so the CI run still passes.

@seisman seisman marked this pull request as ready for review April 23, 2024 10:36
@seisman seisman force-pushed the pytest/xdist branch 3 times, most recently from d4cd959 to d451df7 Compare April 24, 2024 01:35
@seisman seisman changed the title WIP: Use the pytest-xdist plugin WIP: Add pytest plugins pytest-xdist and pytest-rerunfailures Apr 25, 2024
@seisman seisman changed the title WIP: Add pytest plugins pytest-xdist and pytest-rerunfailures POC: Add pytest plugins pytest-xdist and pytest-rerunfailures Apr 25, 2024
@seisman seisman added maintenance Boring but important stuff for the core devs needs review This PR has higher priority and needs review. labels Apr 25, 2024
@seisman seisman added this to the 0.12.0 milestone Apr 25, 2024
@seisman
Copy link
Member Author

seisman commented Apr 25, 2024

Ping @GenericMappingTools/pygmt-maintainers for comments on adding two more pytest plugins. If agreed, then maybe we should also add them to other workflows, like:

  • .github/workflows/ci_tests_legacy.yaml
  • .github/workflows/ci_tests_dev.yaml
  • .github/workflows/ci_doctests.yaml
  • .github/workflows/benchmarks.yml

@seisman seisman added run/benchmark Trigger the benchmark workflow in PRs run/test-gmt-dev Trigger the GMT Dev Tests workflow in PR labels Apr 28, 2024
@seisman
Copy link
Member Author

seisman commented Apr 28, 2024

Doesn't work well in the Benchmarks workflow (https://github.com/GenericMappingTools/pygmt/actions/runs/8866542049/job/24344321212).

Edit: Works after the patch PR #3241.

@seisman seisman removed the needs review This PR has higher priority and needs review. label Apr 28, 2024
@seisman seisman removed this from the 0.12.0 milestone Apr 28, 2024
Copy link

codspeed-hq bot commented May 10, 2024

CodSpeed Performance Report

Merging #3193 will improve performances by ×3.4

Comparing pytest/xdist (a2e85cd) with main (5787a34)

Summary

⚡ 5 improvements
✅ 94 untouched benchmarks

Benchmarks breakdown

Benchmark main pytest/xdist Change
test_contour_matrix[DataFrame] 274.5 ms 82.9 ms ×3.3
test_contour_matrix[Dataset] 287 ms 95.4 ms ×3
test_contour_matrix[array] 272.3 ms 80.6 ms ×3.4
test_grdlandmask_no_outgrid 763.8 ms 342.1 ms ×2.2
test_sph2grd_no_outgrid 2.3 s 1.6 s +46.54%

@seisman
Copy link
Member Author

seisman commented May 11, 2024

The two pytest plugins are only installed to the "Benchmarks", "Tests" and "Dev Tests" workflows, because we only care about the speed of these three workflows.

@seisman seisman changed the title POC: Add pytest plugins pytest-xdist and pytest-rerunfailures CI: Add pytest plugins pytest-xdist and pytest-rerunfailures May 13, 2024
@seisman seisman added the needs review This PR has higher priority and needs review. label May 13, 2024
@seisman seisman added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels May 18, 2024
@seisman seisman requested a review from a team May 18, 2024 02:57
@seisman seisman merged commit 861f454 into main May 19, 2024
13 of 22 checks passed
@seisman seisman deleted the pytest/xdist branch May 19, 2024 01:40
@seisman seisman removed final review call This PR requires final review and approval from a second reviewer run/benchmark Trigger the benchmark workflow in PRs run/test-gmt-dev Trigger the GMT Dev Tests workflow in PR labels May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant