-
-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
[MRG+1] MAINT bump joblib to 0.9.0b4 to use forkserver for Py3 & POSIX #5199
Conversation
This should be mentioned in the whats_new, maybe? This change isn't completely trivial and might have consequences on the users. |
Aside from that, 👍 for merge. |
Sure I will update the what's new. |
f0cd2e4
to
710fa14
Compare
710fa14
to
b056a0e
Compare
Added a what's new entry. I also did a clean up of the test utils. I wanted to remove the https://github.com/chyikwei/topicModels/blob/master/LDA/test.py So I deprecated it instead. |
@@ -688,6 +692,29 @@ def func(*args, **kwargs): | |||
return decorator | |||
|
|||
|
|||
def if_safe_multiprocessing_with_blas(func): |
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.
This is much more conservative now, right? So the CI will not test the parallelization on older numpy / scipy. That is not great, but I guess the benefit is that users will have more robust testing?
But the users might actually run into this problem, so I'm not sure skipping tests is the right thing to do.
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.
It's still tested by the Python 3.4 build of travis and on all builds of appveyor. The problem is that there is no way to test when there could be a problem otherwise.
If you want we can restore the test under Linux assuming that Linux users will always use there system BLAS and as far as I know all BLAS versions (OpenBLAS and ATLAS) shipped by Linux distros are fork-safe although I am not 100% sure.
But if someone compiles their own OpenBLAS with OpenMP enabled under Linux, that will crash and there is no robust way to test that.
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 will re-enable those tests under linux and put a not in the docstring.
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.
Sorry for slow replies.
Why are you not fine with the tests crashing when people used OpenMP?
The tests test if the setup is working correctly, right? If their production system will crash, the tests should crash.
b056a0e
to
d2b668c
Compare
@amueller I have reenabled the multiprocessing tests under Linux assuming the users stick to packaged BLAS implementation that seems to be all fork-safe. |
[MRG+1] MAINT bump joblib to 0.9.0b4 to use forkserver for Py3 & POSIX
Thanks! |
Thank you @ogrisel ! |
This upgrades joblib to use the safer forkserver mode of multiprocessing under Python 3.4 and later by default.