-
Notifications
You must be signed in to change notification settings - Fork 432
Add environment variable to change default parallel backend #1356
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
Codecov ReportBase: 93.96% // Head: 93.73% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1356 +/- ##
==========================================
- Coverage 93.96% 93.73% -0.24%
==========================================
Files 52 52
Lines 7291 7291
==========================================
- Hits 6851 6834 -17
- Misses 440 457 +17
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
The things I am wondering about:
|
I think so, otherwise the test suite will become loky specific again without us realizing it. |
I think it's fine to only check it at joblib import time. |
55a5511
to
657d331
Compare
The windows error does not seem related, maybe similar to #1275 |
@GaelVaroquaux raised the concern that the environment variable could be set for other purposes than testing and that users in some environments would not realize that system administrator might change the joblib behaviors without the users being aware causing hard to debug and support situations. Maybe we would instead change the default backend via a environment variable that would only impact a dedicated pytest configuration (e.g. module level setup function in a top level |
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 had this comment in a pending for at least a week. Other than that LGTM!
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! Thanks @lesteve
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.
lint fixes
Thanks for the improvement @lesteve! |
We now need something similar to run the scikit-learn tests with |
Yep this is on the list, as an intermediary step I tried a while ago to run the joblib tests with |
The main motivation for this is to be able to run joblib and later scikit-learn tests with Python nogil with the default backend set to threading