Skip to content

Experiment: Attempt to run tests in parallel in github actions. #1407

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

Draft
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

jwboth
Copy link
Contributor

@jwboth jwboth commented Apr 13, 2025

Proposed changes

This PR aims at utilizing the parallel environment on github. Currently the tests are performed sequentially. If exploting pytest-xdist, it seems that one can invoke up to 4 workers to run the tests in parallel.

The tutorials pass the parallel tests, and the run time is cut a bit.

The remaining tests seem to have some trouble when e.g. external files need to be accessed. Four tests fail. Still the runtime seems to be cut.

@keileg @IvarStefansson if this of interest, I can try to understand what goes wrong in the failing tests.

Types of changes

What types of changes does this PR introduce to PorePy?
Put an x in the boxes that apply.

  • Minor change (e.g., dependency bumps, broken links).
  • Bugfix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • Testing (contribution related to testing of existing or new functionality).
  • Documentation (contribution related to adding, improving, or fixing documentation).
  • Maintenance (e.g., improve logic and performance, remove obsolete code).
  • Other:

Checklist

Put an x in the boxes that apply or explain briefly why the box is not relevant.

  • The documentation is up-to-date.
  • Static typing is included in the update.
  • This PR does not duplicate existing functionality.
  • The update is covered by the test suite (including tests added in the PR).
  • If new skipped tests have been introduced in this PR, pytest was run with the --run-skipped flag.

@jwboth jwboth marked this pull request as draft April 13, 2025 06:22
@keileg
Copy link
Contributor

keileg commented Apr 14, 2025

@Yuriyzabegaev and I looked into this some months ago. The problem is all tests that interact with gmsh, where the common file name gmsh_frac_file leads to all kind of trouble. A solution may be to assign random names to files during testing. I'll do some experiments now.

@jwboth
Copy link
Contributor Author

jwboth commented Apr 14, 2025

@Yuriyzabegaev and I looked into this some months ago. The problem is all tests that interact with gmsh, where the common file name gmsh_frac_file leads to all kind of trouble. A solution may be to assign random names to files during testing. I'll do some experiments now.

Cool. I am looking forward to the outcome of the currently running tests!

It all makes sense. This is consistent with my experience in running many simulations side-by-side. Seems like classical race conditions - in my simple cases there was a simple fix of including some seconds waiting times (I also used dedicated file names for gmsh files, but if I remember correctly this was not sufficient - I did not dig too deep as it worked in the end). Here, it is not so trivial I find.

@keileg
Copy link
Contributor

keileg commented Apr 14, 2025

I pushed a change that will assign random names to gmsh files during testing, but not regular use. The good news is that the time for testing is almost cut in half locally (which in a sense is disappointing from four workers, but I'll take the improvement still). The not so good news are:

  1. There are occasional random errors from tests that seem to trace back to conflicting gmsh file names. The file names are assigned a random number between 1 and 10^6 (or was it ^8), so the chance of collision should be miniscule, but it could be that the workers all start with the same random seed.
  2. There are also issues with some vtk exporting. I'll leave these to you, @jwboth.

It remains to be seen whether we get a meaningful speedup also on GH actions, but local speedups will also help.

In total, I would say this is worth pursuing a bit more, but not necessarily to be assigned very high priority right now. Thoughts?

@jwboth
Copy link
Contributor Author

jwboth commented Apr 14, 2025

I pushed a change that will assign random names to gmsh files during testing, but not regular use. The good news is that the time for testing is almost cut in half locally (which in a sense is disappointing from four workers, but I'll take the improvement still). The not so good news are:

  1. There are occasional random errors from tests that seem to trace back to conflicting gmsh file names. The file names are assigned a random number between 1 and 10^6 (or was it ^8), so the chance of collision should be miniscule, but it could be that the workers all start with the same random seed.
  2. There are also issues with some vtk exporting. I'll leave these to you, @jwboth.

It remains to be seen whether we get a meaningful speedup also on GH actions, but local speedups will also help.

In total, I would say this is worth pursuing a bit more, but not necessarily to be assigned very high priority right now. Thoughts?

I would suggest one more attempt. Look here: https://pytest-xdist.readthedocs.io/en/stable/distribution.html

It is possible to control the distribution. Without having done any deeper analysis, my intuition would say that the conflicts are if certain tests are run in parallel. The likelihood that these tests come from the same file may be quite high. It is possible to request that tests from the same file are run by the same worker. This can be done again by a simple flag in the run command.

Copy link
Contributor Author

@jwboth jwboth left a comment

Choose a reason for hiding this comment

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

Distribute same test files among same workers - should hopefully fix vtk tests.

Distribute tests according to file.
Allow for aggressive distribution of tutorials in testing.
@jwboth
Copy link
Contributor Author

jwboth commented Apr 14, 2025

Distribute same test files among same workers - should hopefully fix vtk tests.

The last test run succeeded. Indeed, checking the tutorials now with aggressive distribution of tasks seems to save ~1 minute, using 2 workers. The standard test suite at the moment with conservative distribution has no groundbreaking speed up but still 2-3 minutes. Just to test the potential, I will run one experiment with aggressive distribution (and massively many fails, I assume).

Experiment with aggressive distribution of tests
@Yuriyzabegaev
Copy link
Contributor

I pushed a change that will assign random names to gmsh files during testing, but not regular use. The good news is that the time for testing is almost cut in half locally (which in a sense is disappointing from four workers, but I'll take the improvement still). The not so good news are:

  1. There are occasional random errors from tests that seem to trace back to conflicting gmsh file names. The file names are assigned a random number between 1 and 10^6 (or was it ^8), so the chance of collision should be miniscule, but it could be that the workers all start with the same random seed.
  2. There are also issues with some vtk exporting. I'll leave these to you, @jwboth.

It remains to be seen whether we get a meaningful speedup also on GH actions, but local speedups will also help.

In total, I would say this is worth pursuing a bit more, but not necessarily to be assigned very high priority right now. Thoughts?

I was also thinking towards doing this when we were investigating this. As a sidenote, could it avoid the random collisions if naming files after the test name, not at random? The test name can be accessed from the same env variable PYTEST_CURRENT_TEST

@keileg
Copy link
Contributor

keileg commented Apr 15, 2025

I was also thinking towards doing this when we were investigating this. As a sidenote, could it avoid the random collisions if naming files after the test name, not at random? The test name can be accessed from the same env variable PYTEST_CURRENT_TEST

If so, the file name would also need to include information on test parametrization. Better then to generate a temporary file using techniques like this.

@IvarStefansson
Copy link
Contributor

When reading through the discussion, I wondered whether the times.json files might also interfere with each other. Just a thought.

@jwboth
Copy link
Contributor Author

jwboth commented May 23, 2025

I did not investigate the latest suggestions as the current setup seems to have worked (for the few test runs above). The latest failing tests were merely due to mypy which are fixed now. By simple comparison to a current PR: Indeed the gains in time are not too large. Tutorials have no true speed up (<1 min possibly of 6 minutes). The remaining tests are ca. 2-3 mins faster of 10-11 minutes for reference.

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.

4 participants