-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
TST Speed up some of the slowest tests #27383
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
Conversation
…the slowest multiprocessing backend
…y using n_jobs=2 instead of n_jobs=3
@@ -2044,10 +2043,10 @@ def test_same_radius_neighbors_parallel(algorithm): | |||
assert_allclose(graph, graph_parallel) | |||
|
|||
|
|||
@pytest.mark.parametrize("backend", JOBLIB_BACKENDS) | |||
@pytest.mark.parametrize("backend", ["threading", "loky"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't want to test multiprocessing and sequential backends?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The multiprocessing case is very slow on macOS (and maybe windows as well): this backend creates new processes from scratch each time which is can take several tens of seconds to run on the overloaded macOS Azure CI.
I think the loky backend is enough to test for multiprocess-related problems in our Cython code. The main difference between multiprocessing and loky is that loky reuses its workers across calls making it much more efficient on overloaded machines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also sequential is already tested everywhere else in test_neighbors.py
(as it's the default backend for the default value of n_jobs
).
... n_samples=100, n_components=15, n_features=20, n_nonzero_coefs=10, | ||
... n_samples=30, n_components=15, n_features=20, n_nonzero_coefs=10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious, how much speedup are we getting here? are we THAT slow? 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the macOS CI build, this test is sometimes one of the slowest and can last more than 20s on an overloaded CI runner.
Locally it's fast but decreasing this makes it ~3x faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the description of the PR to show the impact on the macOS CI between main
and this PR.
I observed in the latest run than the docstring of MiniBatchDictionaryLearning
has a similar problem (and a bug in the direction on the assertion about the sparsity of the transformed data). I pushed a96dee2 to fix it and improve the speed. We will have to wait for the end of this CI run to see the concrete impact of that last commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the description to take a96dee2 into account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to my comments, I am fine with the changes.
I assume that all the merging regarding the sparse containers hurts the testing performance in terms of time. I tried to be sure that we were not duplicating tests for no reason while merging.
I am wondering if we could have been slightly more lenient with the test: be sure that fit/predict/transform works but not always running statistical checking for all tests.
In the long term, we will remove the tests for scipy sparse matrices and only keep the scipy sparse arrays. In the short term, we could maybe change the defintion of the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@ogrisel ready to merge or anything else? |
Merged. There are more tests to optimize down the line but with diminishing returns. Another PR that should greatly speed-up the tests is the test refactoring of pairwise argkmin & radius reductions: #27281. |
Mostly by reducing the dataset sizes.
pylatest_conda_forge_mkl
onmain
: