Skip to content

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Dec 8, 2021

Alternative to #21907.
Towards: #21407.

Do not test for the threading backend which was not involved in the original problem.
Do not test for the multitask variants that are much more computationally intensive. LassoCV and ElasticNetCV should be enough to cover as a non-regression test for the original problem.
Use the minimal dataset for the fewest number of features to trigger memmaping in joblib.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

I left comment about future API for joblib.

Overall this PR LGTM.

Comment on lines +1640 to +1641
# Unfortunately the scikit-learn and joblib APIs do not make it possible to
# change the max_nbyte of the inner Parallel call.
Copy link
Member

Choose a reason for hiding this comment

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

Do you think there should be an API for changing max_nbytes in the inner Parallel call? Something like:

with parallel_backend("loky", max_nbytes=1000):
    results = Parallel(n_jobs=4)(delayed(func)(x, y) for x, y in data)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that would be nice but unfortunately this conflicts with the current joblib backend API design and dealing with backward compat is...

Copy link
Member

Choose a reason for hiding this comment

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

If it is hard for joblib API wise, what do you think about adding a parallel_kwargs parameter to estimators that creates a Parallel object?

(I know this is a little counter to how we have been removing pre_dispatch from the estimator's __init__.)

Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably find a way to change the parallel_backend to detect Parallel kwargs and treat time specifically instead of passing them as constructor arguments for the backend.

@ogrisel
Copy link
Member Author

ogrisel commented Dec 9, 2021

@jeremiedbb you might want to give this PR a second review.

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

This test is a non regression test for a fix regarding using the loky backend. By default these estimators use the threading backend which has always been working. We need to keep testing with the "loky" backend

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@ogrisel
Copy link
Member Author

ogrisel commented Dec 9, 2021

This test is a non regression test for a fix regarding using the loky backend. By default these estimators use the threading backend which has always been working. We need to keep testing with the "loky" backend

Indeed, good catch, I had not realized. Let's see if the CI stays green.

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM. Do you know how much time it saves ?

@ogrisel
Copy link
Member Author

ogrisel commented Dec 10, 2021

When running sklearn/linear_model/tests/test_coordinate_descent.py in with pytest-xdist in oversubscription context (-n 8 on a 8 core machine):

  • on main:
================================================================== slowest 10 durations ===================================================================
7.37s call     sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_models_cv_fit_for_all_backends[MultiTaskElasticNetCV-threading]
6.97s call     sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_models_cv_fit_for_all_backends[MultiTaskLassoCV-threading]
5.18s call     sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_models_cv_fit_for_all_backends[MultiTaskElasticNetCV-loky]
4.70s call     sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_models_cv_fit_for_all_backends[MultiTaskLassoCV-loky]
1.10s call     sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_models_cv_fit_for_all_backends[ElasticNetCV-loky]
... other unrelated tests
  • on this branch:
================================================================== slowest 10 durations ===================================================================
0.89s call     sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_models_cv_fit_with_loky[ElasticNetCV]
0.88s call     sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_models_cv_fit_with_loky[LassoCV]
... other unrelated tests

@ogrisel ogrisel merged commit 670133d into scikit-learn:main Dec 10, 2021
@ogrisel ogrisel deleted the maint-test_linear_models_cv_fit_for_joblib branch December 10, 2021 01:43
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Dec 24, 2021
…educe CI usage (scikit-learn#21918)



Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
glemaitre pushed a commit that referenced this pull request Dec 25, 2021
…educe CI usage (#21918)



Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
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.

3 participants