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

Test suite clean up #3385

Open
wants to merge 72 commits into
base: master
Choose a base branch
from
Open

Test suite clean up #3385

wants to merge 72 commits into from

Conversation

JDBetteridge
Copy link
Member

@JDBetteridge JDBetteridge commented Feb 2, 2024

Description

  • Switches to having MPI executed 'on the outside'. This means we are no longer bound to MPICH (the source of huge build times and MacOS bugs) and also makes things much faster.
  • Parallel tests do not oversubscribe the runners.
  • Various associated bug fixes since parallel tests are not run in isolation.

TODOs

  • Fix remaining failing tests
  • Add make check and clean up Makefile (and use in CI + mention on install page)
  • Remove --download-mpich from everywhere, means that MPI is a system dependency

@connorjward
Copy link
Contributor

connorjward commented Feb 2, 2024

This is cool, but isn't it a bad idea to effectively remove test coverage? If CI doesn't run all the tests no one will.

I can see this being useful in the context of a bigger change where we run the test suite with a number of Firedrake configurations and only one of them would run these slow tests.

@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch from ecb9628 to f3685b7 Compare February 9, 2024 19:21
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch from f3685b7 to f848c6f Compare March 5, 2024 13:40
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch 2 times, most recently from 93a0aae to a20fc9d Compare June 7, 2024 13:52
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch from a5f614f to 27b9af3 Compare July 19, 2024 16:25
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch from 2a7f468 to 6ed1774 Compare August 18, 2024 13:44
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch 2 times, most recently from 8132b64 to 1122a3b Compare August 31, 2024 18:37
@JDBetteridge JDBetteridge self-assigned this Sep 4, 2024
tests/regression/test_ensembleparallelism.py Outdated Show resolved Hide resolved
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch 2 times, most recently from 9d5f056 to df4aea3 Compare September 12, 2024 13:04
Copy link

github-actions bot commented Sep 12, 2024

TestsPassed ✅Skipped ⏭️Failed ❌
Firedrake complex31 ran27 passed4 skipped0 failed

Copy link

github-actions bot commented Sep 12, 2024

TestsPassed ✅Skipped ⏭️Failed ❌
Firedrake real29 ran27 passed2 skipped0 failed

@JDBetteridge JDBetteridge marked this pull request as ready for review September 24, 2024 14:23
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch from bf317f5 to ef87021 Compare October 2, 2024 21:57
@connorjward connorjward changed the title Mark and skip slow tests Test suite clean up Oct 3, 2024
Copy link
Contributor

@connorjward connorjward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally very happy with this.

.github/workflows/build.yml Outdated Show resolved Hide resolved
.test_durations Outdated Show resolved Hide resolved
firedrake/parameters.py Outdated Show resolved Hide resolved
firedrake/slate/slac/compiler.py Show resolved Hide resolved
firedrake/tsfc_interface.py Outdated Show resolved Hide resolved
firedrake/tsfc_interface.py Outdated Show resolved Hide resolved
tests/demos/test_demos_run.py Outdated Show resolved Hide resolved
tests/output/test_io_mesh.py Outdated Show resolved Hide resolved
tests/slate/test_hdg_poisson.py Outdated Show resolved Hide resolved
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch from 8435a69 to 10f7f0c Compare October 8, 2024 14:02
Copy link
Contributor

@connorjward connorjward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving notes for someone (most likely me) to refer to in future. In summary:

  • Need to rebase/merge in master.
  • Tweaks to Makefile and build.yml.

-o faulthandler_timeout=1860 \
--junit-xml=firedrake2_\$MPISPAWN_TASK_ID1.xml \
-m "parallel[\$MPISPAWN_WORLD_SIZE] and not broken" \
-v tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: "dogfood" (bleh) Makefile and use a matrix to massively cut down on boilerplate

.PHONY: test_smoke
test_smoke:
@echo " Running the bare minimum smoke tests"
@python -m pytest -k "poisson_strong or stokes_mini or dg_advection" -v tests/regression/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to use MPI on the "outside" here for the parallel tests so this can be run to check things on HPC

endif
# Requires pytest and pytest-mpi only
.PHONY: test_serial
test_serial:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Terrible name! This runs all the parallel tests too!


# Requires pytest and pytest-mpi only
.PHONY: test_smoke
test_smoke:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bikeshedding: I prefer make smoke_tests or make smoketests

done

.PHONY: _test_large_world_test
_test_large_world_tests:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we have small_world and large_world tests separately.

firedrake/slate/slac/kernel_builder.py Outdated Show resolved Hide resolved
tests/firedrake/conftest.py Outdated Show resolved Hide resolved
tests/firedrake/conftest.py Show resolved Hide resolved
tests/firedrake/conftest.py Show resolved Hide resolved
tests/firedrake/multigrid/test_netgen_gmg.py Outdated Show resolved Hide resolved
tests/firedrake/equation_bcs/test_equation_bcs.py Outdated Show resolved Hide resolved
tests/firedrake/equation_bcs/test_equation_bcs.py Outdated Show resolved Hide resolved
tests/firedrake/demos/test_notebooks_run.py Outdated Show resolved Hide resolved
tests/firedrake/conftest.py Outdated Show resolved Hide resolved
tests/firedrake/conftest.py Outdated Show resolved Hide resolved
@JDBetteridge
Copy link
Member Author

Why would you remove the slow markers? They are really useful if you want to run the test suite faster (especially in parallel!) and are also there to highlight tests that can be sped up.

@connorjward
Copy link
Contributor

Why would you remove the slow markers?

Sorry to undo your hard work but I don't see the benefit in having them:

They are really useful if you want to run the test suite faster (especially in parallel!)

But it doesn't run all of the tests. It is useful to be able to run only a few fast tests (a la make check) but I can't imagine a situation where I would want to run most-but-not-all of the tests.

and are also there to highlight tests that can be sped up.

We already know which tests are slow from .test_durations and from the

============================ slowest 200 durations =============================

output from pytest. This is another source of truth and I think it will immediately bit-rot.

@@ -61,17 +60,15 @@ def adjoint_example(mesh):


@pytest.mark.skipcomplex
# Waiting on stable parallel decompositions through disk checkpointing.
@pytest.mark.xfail
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: What is going on here

@connorjward connorjward dismissed stale reviews from JHopeCollins and themself January 10, 2025 17:28

I'm in charge now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

INSTALL: Tests not passing on fresh install M2 Mac
4 participants