Skip to content

DOC reference parallel_config instead of parallel_backend #1457

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

Merged
merged 19 commits into from
Jun 27, 2023

Conversation

tomMoral
Copy link
Contributor

@tomMoral tomMoral commented Jun 22, 2023

Follow up for #1451 to use parallel_config as much as possible in examples and in the tests.

In doing so, I realized that it actually has a different behavior than parallel_backend.
This PR does:

  • make it possible to specify the default_n_jobs per backend (in particular to have default n_jobs=-1 for dask).
  • Fix the nested parallel_config usage. Calling the two following examples is equivalent while the first one would give LokyBackend with 2 jobs with the current version on master.
with parallel_config(backend='threading'):
    with parallel_config(n_jobs=2):
        p = Parallel()

with parallel_config(backend='loky', n_jobs=2):
        p = Parallel()

@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Patch coverage: 98.13% and project coverage change: -0.10 ⚠️

Comparison is base (2edd849) 94.93% compared to head (ddcbebe) 94.83%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1457      +/-   ##
==========================================
- Coverage   94.93%   94.83%   -0.10%     
==========================================
  Files          44       45       +1     
  Lines        7439     7471      +32     
==========================================
+ Hits         7062     7085      +23     
- Misses        377      386       +9     
Impacted Files Coverage Δ
joblib/test/test_dask.py 91.19% <96.15%> (ø)
joblib/test/test_config.py 97.59% <97.59%> (ø)
joblib/_dask.py 92.71% <100.00%> (+0.03%) ⬆️
joblib/_parallel_backends.py 93.47% <100.00%> (+0.02%) ⬆️
joblib/parallel.py 95.90% <100.00%> (-1.34%) ⬇️
joblib/test/test_parallel.py 96.11% <100.00%> (-0.09%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tomMoral tomMoral requested a review from jeremiedbb June 22, 2023 16:30
tomMoral and others added 3 commits June 23, 2023 13:43
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Copy link
Contributor

@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

Copy link
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

I did not have time to fully review the PR carefully but what I read looks good.

tomMoral and others added 3 commits June 26, 2023 09:30
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@ogrisel
Copy link
Contributor

ogrisel commented Jun 26, 2023

@tomMoral I do not understand where the doc problems come from when reading the readthedocs output. Do you get more informative feedback when building the doc locally?

Readthedocs is also failing on unrelated PRs (e.g. #1463) but it would be great to fix it here to see the updated doc for the parallel_config API.

@ogrisel
Copy link
Contributor

ogrisel commented Jun 27, 2023

I tried locally and the output folders stay mostly empty but there is no error message and the return code is 0...

@ogrisel
Copy link
Contributor

ogrisel commented Jun 27, 2023

So the sphinx crash is probably caused by 2 offending examples:

  • examples/serialization_and_wrappers.py
  • examples/parallel_random_state.py (only when ran via sphinx, it works fine otherwise...)

Deleting both example files makes the sphinx generation complete successfully.

But those failures are not related to this PR. I think they also fail on master and even with joblib 1.2.0 but I want to double check.

EDIT: I opened #1465 for the first problem.

@ogrisel
Copy link
Contributor

ogrisel commented Jun 27, 2023

Actually the above comment is macos only. On the output of the CI it fails because of a warning:

/home/docs/checkouts/readthedocs.org/user_builds/joblib/envs/1457/lib/python3.7/site-packages/joblib/parallel.py:docstring of joblib.parallel.parallel_config:1: WARNING: duplicate object description of joblib.parallel_config, other instance in generated/joblib.parallel_config, use :noindex: for one of them

@ogrisel
Copy link
Contributor

ogrisel commented Jun 27, 2023

@tomMoral I added the :noindex: and it fixed the problem and the function is still indexed and reachable from the right location in the left navigation menu:

https://joblib--1457.org.readthedocs.build/en/1457/generated/joblib.parallel_config.html

@tomMoral
Copy link
Contributor Author

Yeah! thanks @ogrisel :)

I don't think the broken build on sklearn is due to this PR as this seems to be an update on scipy\ so merging this one :)

@tomMoral tomMoral merged commit 5d88860 into joblib:master Jun 27, 2023
@tomMoral tomMoral deleted the DOC_parallel_config branch June 27, 2023 22:55
@ogrisel
Copy link
Contributor

ogrisel commented Jun 28, 2023

Indeed, i think the scipy solver problem was fixed in scikit-learn main branch and will be released as part of 1.3.

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.

3 participants