FIX propagate configuration to workers in parallel #25363

glemaitre · 2023-01-11T16:39:30Z

closes #25242
closes #25239
closes #25290

This is an alternative to #25290. The issue in #25290 is that we change the public API for 1.2.1. Making the change in a private _delayed is not really possible since we would warn our user or developer to use a private function.

This PR proposes to overload Parallel and propagate the config using the thread that calls Parallel. It only requires changing the import without changing the Parallel or delayed calls.

ogrisel

I like this approach the best. Here is a first pass of feedback on phrasing.

sklearn/utils/fixes.py

sklearn/utils/tests/test_fixes.py

thomasjpfan

Thank you for the PR! I do like this option a bit more than the other ones.

sklearn/utils/fixes.py

Co-authored-by: Olivier Grisel <[email protected]>

…rn#25361)

….py (scikit-learn#25281)

…#25357)

…ed attributes (scikit-learn#24882) Co-authored-by: Julien Jerphanion <[email protected]> Co-authored-by: Guillaume Lemaitre <[email protected]>

…atch (scikit-learn#25297) Co-authored-by: Guillaume Lemaitre <[email protected]>

Co-authored-by: Thomas J. Fan <[email protected]> Co-authored-by: Christian Lorentzen <[email protected]> Co-authored-by: Guillaume Lemaitre <[email protected]> Co-authored-by: Chiara Marmo <[email protected]>

build_tools/azure/linting.sh

sklearn/utils/parallel.py

ogrisel

Reading the generated API doc for the sklearn.fixes module I see that we also expose utils.parallel_backend and utils.register_parallel_backend.

I believe that we should also deprecated those and move them to sklearn.utils.parallel for consistency.

We should also probably included versionchanged or versionchanged info in the docstring of sklearn.utils.parallel.delayed and versionadded for .Parallel.

sklearn/utils/parallel.py

sklearn/utils/tests/test_parallel.py

ogrisel · 2023-01-18T09:32:55Z

Other than the comments above, LGTM by the way. Thanks very much for this work @glemaitre!

thomasjpfan · 2023-01-18T13:09:58Z

Reading the generated API doc for the sklearn.fixes module I see that we also expose utils.parallel_backend and utils.register_parallel_backend.

I believe that we should also deprecated those and move them to sklearn.utils.parallel for consistency.

Does the following comment still apply?

scikit-learn/sklearn/utils/__init__.py

Lines 44 to 50 in f1340c7

    
           # Do not deprecate parallel_backend and register_parallel_backend as they are 
        
           # needed to tune `scikit-learn` behavior and have different effect if called 
        
           # from the vendored version or or the site-package version. The other are 
        
           # utilities that are independent of scikit-learn so they are not part of 
        
           # scikit-learn public API. 
        
           parallel_backend = _joblib.parallel_backend 
        
           register_parallel_backend = _joblib.register_parallel_backend

I do not think we need to provide parallel_backend or register_parallel_backend anywhere if they are already in joblib.

Co-authored-by: Olivier Grisel <[email protected]>

ogrisel · 2023-01-18T16:54:09Z

I do not think we need to provide parallel_backend or register_parallel_backend anywhere if they are already in joblib.

Good catch, I should have checked that. I would be in favor of deprecating those two in favor of the matching functions from joblib.

glemaitre · 2023-01-18T16:55:22Z

Good catch, I should have checked that. I would be in favor of deprecating those two in favor of the matching functions from joblib.

I can do that in a subsequent PR since it is independent of this PR.

glemaitre · 2023-01-18T16:58:32Z

I think that I addressed the last comments. I will now open a PR for the deprecation of the joblib helper functions.

ogrisel

LGTM! +1 when CI is green.

ogrisel · 2023-01-18T17:15:05Z

Hum, sphinx chokes in the ..rubric:: directive used in the Parallel.__doc__ attribute:

https://github.com/scikit-learn/scikit-learn/actions/runs/3951111594/jobs/6764541521#step:3:2324

Maybe we could instead do a simple docstring for sklearn.utils.parallel.Parallel that only present how it is different from joblib.Parallel (config propagation) and refer the reader to the docstring the parent class the rest.

ogrisel · 2023-01-19T11:08:10Z

Green!

@thomasjpfan any final comment?

glemaitre · 2023-01-19T16:00:31Z

I swear it was green before :)

thomasjpfan

Minor comment about the changelog, otherwise LGTM.

thomasjpfan · 2023-01-20T01:37:22Z

doc/whats_new/v1.2.rst

  boolean. The type is maintained, instead of converting to `float64.`
  :pr:`25147` by :user:`Tim Head <betatim>`.

+- |API| :func:`utils.fixes.delayed` is deprecated in 1.2.1 and will be removed


I think we need a changelog entry to introduce utils.parallel.Parallel as well.

Rather than a new entry I think we can just expand this entry as suggested below to keep related changes together:

doc/whats_new/v1.2.rst

Co-authored-by: Olivier Grisel <[email protected]>

Co-authored-by: Olivier Grisel <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]>

glemaitre added 2 commits January 11, 2023 17:35

FIX propagate configuration to workers in parallel

c9fcaa1

update PR

a10e6da

ogrisel reviewed Jan 11, 2023

View reviewed changes

thomasjpfan reviewed Jan 11, 2023

View reviewed changes

sklearn/utils/fixes.py Outdated Show resolved Hide resolved

glemaitre and others added 17 commits January 12, 2023 11:30

Apply suggestions from code review

790baa1

Co-authored-by: Olivier Grisel <[email protected]>

MAINT always seed make_classification in tests (scikit-learn#25353)

3ed8832

MNT minor clean-up of sklearn/conftests.py (scikit-learn#25358)

c9b5d02

CI Remove unused env var (scikit-learn#25359)

ef3ef2f

MNT [scipy-dev] [pypy] [azure parallel] update lock files (scikit-lea…

9b2eb17

…rn#25361)

TST use global_random_seed in sklearn/cluster/tests/test_hierarchical…

a1fb2be

….py (scikit-learn#25281)

CI Use latest Cython sources in scipy-dev build (scikit-learn#25300)

689792f

MAINT Clean deprecations in MiniBatchDictionaryLearning (scikit-learn…

5b3c9a1

…#25357)

FEA Store the GradientBoosting OOB Scores in the oob_score(s)_ fitt…

4a4eee4

…ed attributes (scikit-learn#24882) Co-authored-by: Julien Jerphanion <[email protected]> Co-authored-by: Guillaume Lemaitre <[email protected]>

ENH Adds InconsistentVersionWarning when unpickling version doesn't m…

4af933b

…atch (scikit-learn#25297) Co-authored-by: Guillaume Lemaitre <[email protected]>

ENH Add feature_name_combiner to OneHotEncoder (scikit-learn#22506)

ea03b3e

Co-authored-by: Thomas J. Fan <[email protected]> Co-authored-by: Christian Lorentzen <[email protected]> Co-authored-by: Guillaume Lemaitre <[email protected]> Co-authored-by: Chiara Marmo <[email protected]>

TST update warning message issued

7676555

TST update warning message issued

90ce7d1

create a new module

b80d5df

DEP deprecate sklearn.utils.fixes.delayed

a25add1

Merge remote-tracking branch 'origin/main' into is/25290_alternative

eddea10

DOC add deprecation in changelog

1fd6a88

thomasjpfan reviewed Jan 12, 2023

View reviewed changes

build_tools/azure/linting.sh Show resolved Hide resolved

sklearn/utils/parallel.py Show resolved Hide resolved

glemaitre added 4 commits January 13, 2023 11:52

MAINT/DOC add Parallel to linter and document parallel module

4015591

Merge remote-tracking branch 'origin/main' into is/25290_alternative

358e33e

missing import Parallel

32cda57

Merge branch 'main' into is/25290_alternative

a5c83a8

ogrisel mentioned this pull request Jan 18, 2023

Library specific global options are not passed to jobs joblib/joblib#1071

Open

ogrisel reviewed Jan 18, 2023

View reviewed changes

sklearn/utils/parallel.py Show resolved Hide resolved

sklearn/utils/parallel.py Show resolved Hide resolved

sklearn/utils/parallel.py Outdated Show resolved Hide resolved

sklearn/utils/parallel.py Show resolved Hide resolved

sklearn/utils/tests/test_parallel.py Show resolved Hide resolved

glemaitre and others added 2 commits January 18, 2023 17:44

Apply suggestions from code review

2b92410

Co-authored-by: Olivier Grisel <[email protected]>

update warning

b16203e

ogrisel mentioned this pull request Jan 18, 2023

FIX get config from dispatcher thread in delayed by default #25242

Closed

ogrisel added this to the 1.2.1 milestone Jan 18, 2023

DOC add versionadded and versionchanged

d2fb6e6

Merge remote-tracking branch 'origin/main' into is/25290_alternative

8f9e99f

ogrisel approved these changes Jan 18, 2023

View reviewed changes

glemaitre added 3 commits January 19, 2023 10:41

Merge remote-tracking branch 'origin/main' into is/25290_alternative

19661ee

DOC reference joblib.Parallel instead of overwrite

4f7bd37

fix Parallel import

9320a51

Merge branch 'main' into is/25290_alternative

dbdb2cd

jjerphan mentioned this pull request Jan 19, 2023

FIX Support readonly sparse datasets for manhattan_distances #25432

Merged

thomasjpfan approved these changes Jan 20, 2023

View reviewed changes

ogrisel reviewed Jan 20, 2023

View reviewed changes

doc/whats_new/v1.2.rst Outdated Show resolved Hide resolved

glemaitre and others added 2 commits January 20, 2023 11:06

Update doc/whats_new/v1.2.rst

1bd78b7

Co-authored-by: Olivier Grisel <[email protected]>

Update v1.2.rst

15526e8

thomasjpfan added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Jan 20, 2023

thomasjpfan merged commit a7cd0ca into scikit-learn:main Jan 20, 2023

jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 23, 2023

FIX propagate configuration to workers in parallel (scikit-learn#25363)

904c2ec

Co-authored-by: Olivier Grisel <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]>

adrinjalali pushed a commit that referenced this pull request Jan 24, 2023

FIX propagate configuration to workers in parallel (#25363)

79da465

Co-authored-by: Olivier Grisel <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]>

colibrisson mentioned this pull request Mar 7, 2023

Workers don't respect global configuration and spawn too many threads mittagessen/kraken#468

Closed

Uh oh!

FIX propagate configuration to workers in parallel #25363

FIX propagate configuration to workers in parallel #25363

Uh oh!

Conversation

glemaitre commented Jan 11, 2023

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ogrisel commented Jan 18, 2023

Uh oh!

thomasjpfan commented Jan 18, 2023

Uh oh!

ogrisel commented Jan 18, 2023

Uh oh!

glemaitre commented Jan 18, 2023

Uh oh!

glemaitre commented Jan 18, 2023

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Jan 18, 2023

Uh oh!

ogrisel commented Jan 19, 2023

Uh oh!

glemaitre commented Jan 19, 2023

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

thomasjpfan Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

ogrisel Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants