Skip to content
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

FIX treat n_jobs=None as if left to its default value #1475

Merged
merged 10 commits into from Jul 10, 2023

Conversation

jeremiedbb
Copy link
Contributor

Fixes #1473

Even if the new default value of n_jobs is a sentinel object, I think that we should not interpret n_jobs=None as explicitely set because it used to mean 'unset'. Instead we should treat it identically as if it were left to its default value.

@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.06 ⚠️

Comparison is base (476ff8e) 94.94% compared to head (b70321f) 94.88%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1475      +/-   ##
==========================================
- Coverage   94.94%   94.88%   -0.06%     
==========================================
  Files          45       45              
  Lines        7474     7491      +17     
==========================================
+ Hits         7096     7108      +12     
- Misses        378      383       +5     
Impacted Files Coverage Δ
joblib/parallel.py 96.91% <100.00%> (+0.01%) ⬆️
joblib/test/test_config.py 97.95% <100.00%> (+0.36%) ⬆️

... and 3 files with indirect coverage changes

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

Copy link
Contributor

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

Indeed, thanks for the PR.

@jeremiedbb
Copy link
Contributor Author

It's not fully fixed yet, i'm still working out some details

@tomMoral
Copy link
Contributor

Yes it is not fixed, it seems like we need to manage and test when n_job is not set on the context and None is passed explicitly

Copy link
Contributor

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

Better fix indeed! thanks @jeremiedbb 😁

just an extra nom regression test.

joblib/test/test_config.py Outdated Show resolved Hide resolved
@jeremiedbb
Copy link
Contributor Author

Actually, n_jobs=None should mean "unset" only for Parallel. For parallel_backend, since the default is -1, None used to mean "set" and was used to reset n_jobs to the backend default n_jobs in a nesting situation. parallel_config should behave the same.

Copy link
Contributor

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

LGTM, thx @jeremiedbb

I was quite surprised by the behavior of setting None in the context but indeed, if this is what was enforced, I think it is a good idea to keep the same behavior.

@tomMoral tomMoral merged commit 4ccc02d into joblib:master Jul 10, 2023
16 checks passed
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.

BUG: n_jobs=None not handled properly anymore in Parallel
2 participants