Skip to content

Do not use Makefile for firedrake-check #4391

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

Merged
merged 3 commits into from
Jul 15, 2025

Conversation

connorjward
Copy link
Contributor

@connorjward connorjward commented Jun 24, 2025

  • The previous approach led to issues for a number of users for whom 'mpiexec' did not work inside the Makefile.
  • We now support a '--mpiexec' argument for those requiring extra commands like '--oversubscribe' or 'srun'.
  • Adapts to mpi-pytest change so parallel[1] covers all serial tests.

This is approved an can be merged once a new release of mpi-pytest is made. (now done)

@connorjward connorjward requested a review from JHopeCollins June 24, 2025 08:57
Copy link
Member

@JHopeCollins JHopeCollins left a comment

Choose a reason for hiding this comment

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

I've suggested refactoring how the tests to run are specified so it'd be easier to add other parallel tests with nprocs!=3 in the future. What do you think?

@connorjward
Copy link
Contributor Author

@JHopeCollins I have followed your suggestions though the code doesn't exactly match your suggested changes.

@connorjward connorjward marked this pull request as ready for review June 25, 2025 10:44
JHopeCollins
JHopeCollins previously approved these changes Jun 25, 2025
Copy link
Member

@JHopeCollins JHopeCollins left a comment

Choose a reason for hiding this comment

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

Very happy with this, thanks @connorjward!

@JHopeCollins JHopeCollins enabled auto-merge (squash) June 25, 2025 10:58
@connorjward connorjward disabled auto-merge June 25, 2025 11:22
@connorjward connorjward marked this pull request as draft June 25, 2025 11:25
@connorjward connorjward changed the title Do not use Makefile for firedrake-check Do not use Makefile for firedrake-check (needs mpi-pytest release) Jun 25, 2025
@connorjward
Copy link
Contributor Author

This is failing because the non-forking mpiexec problem is alive and kicking! I have a fix in this instance but it's interesting that this works fine on my machine.

@connorjward connorjward marked this pull request as ready for review July 11, 2025 14:42
This should avoid CI failures due to forking mpiexec after importing
Firedrake.
@connorjward connorjward force-pushed the connorjward/firedrake-check branch from 2329330 to 9ec8176 Compare July 15, 2025 09:54
@connorjward connorjward changed the base branch from master to release July 15, 2025 09:54
@connorjward connorjward changed the title Do not use Makefile for firedrake-check (needs mpi-pytest release) Do not use Makefile for firedrake-check Jul 15, 2025
@connorjward
Copy link
Contributor Author

Merging as this was previously approved and passes CI in the relevant places.

@connorjward connorjward merged commit 8f421c4 into release Jul 15, 2025
5 of 7 checks passed
@connorjward connorjward deleted the connorjward/firedrake-check branch July 15, 2025 13:36
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.

2 participants