Fix file contention issues in parallel tests #2175
Conversation
a499617 to
8760997
Compare
…llelization & change test distribution
8760997 to
03c5371
Compare
| @@ -482,7 +484,9 @@ def test_mode_solver_custom_medium(mock_remote_api, local, tmp_path): | |||
| freqs=[freq0], | |||
| direction="+", | |||
| ) | |||
| modes = ms.solve() if local else msweb.run(ms) | |||
| modes = ( | |||
| ms.solve() if local else msweb.run(ms, results_file=tmp_path / "ms_custom_medium.hdf5") | |||
There was a problem hiding this comment.
What is the takehome message here, we shouldn't have multiple tests writing something to the same file? Is it ok if it's across different test files?
There was a problem hiding this comment.
Not sure whether it's ok across different test files, the problem occurs in tests that write to the same object from multiple workers. I think this is more likely to happen with parameterized tests.
There was a problem hiding this comment.
What we really need to do is get rid of these global default paths in the tests, I think those are the root cause of the issue, and have each test always use its tmp_path. Currently (some of) the tests end up cluttering the base dir.
There was a problem hiding this comment.
Maybe we can come up with a generic path management solution we can share for the back-end too re test file paths? Something on these lines and we'd just document it in some developers guide.
There was a problem hiding this comment.
Isn't that what the built-in tmp_path fixture is for? Or do you have something else in mind? I think we just need to do a better job of actually using it 😄
There was a problem hiding this comment.
Thanks Yannick! This is great.
Mainly I'm thinking should we track this issue of making sure that all our tests paths follow this tmp_path convention both on the frontend and backend? Couldn't seem to find one already. If this can work nicely for the backend path management too. Happy to help on this if we want.
|
I am tracking general test refactoring but haven't managed to port this to the new projects tracking, just did not get to that yet. So yeah will tag you too. It's a big project 😄 |
This is a follow-up to #2165
Some test were sporadically failing due to I/O contention, so this fixes that. The PR also:
close_matplotlibfixture that closes all matplotlib figures after testing a module so that these don't hang around for the whole test suite. Gets rid of a few warnings and reduces overall memory consumption.reset_loggerthat reset the logger for each test module, since most modules modify logger state.pytest-xdistdistribution algorithm to "worksteal", which brings the time for the whole test suite further down from 160s to 50s (so ~8x improvement in total since Test parallelization #2165). This one should also be significant for CI. Let's see.rngfixture in the autograd plugin.This also includes the change to the test workflow we discussed @daquintero