[MRG+1] MacOS X testing improvements #2982
Conversation
n_jobs variable was unused previously, leaving n_jobs=-1 case untested for BaggingRegressor.
| @@ -338,7 +338,7 @@ def test_parallel(): | |||
|
|
|||
| for n_jobs in [-1, 3]: | |||
There was a problem hiding this comment.
I think we should not test for n_jobs=-1. If you run the tests on a box with 64 cores you might disrupt the regular operation of the machine. +1 for setting the range to [1, 3] instead.
There was a problem hiding this comment.
Does it apply to all cases where n_jobs=-1 is used in tests? I believe it occurs more than once in scikit-learn testing suite.
There was a problem hiding this comment.
I would say so but AFAIK there is no official guideline. @GaelVaroquaux any opinion? No need to hunt them all in this PR though, this is not a big issue.
There was a problem hiding this comment.
There are occurances of n_jobs=-1 in the same function, in other functions in this testing module, in other testing modules from the same testing package, and in other tests. It looks like a bad idea to change just this line.
There was a problem hiding this comment.
correction: ther are no n_jobs=-1 in other functions from this testing module
There was a problem hiding this comment.
I agree that we probably shouldn't have n_jobs=-1 in the test suite.
|
Once my comment is addressed, +1 for merge. |
|
The 'n_jobs=-1' aspect needs to be address, and after that 👍 for merge. That said, in the long run, we would need to address the fact that parallel computing hangs on mac. @ogrisel, any clues on what would be the right direction to fix that? |
|
There are also |
Based on past discussions on the NumPy ML and github issues, it seems that NumPy devs will advocate that users should either use Atlas or OpenBLAS (built from master branch as of today) instead of using the OSX system BLAS. Hopefully PyPI will have .whl binary packages for OSX for NumPy and SciPy with OpenBLAS included at some point in the future. |
[MRG+1] MacOS X testing improvements
|
Merged, thanks @kmike! |
That's what I was hoping for. In which case we can reenable these tests. |
|
Didn't know OpenBLAS became fork-safe, great news - thanks @ogrisel! |
|
Well it did not happen just by chance ;) OpenMathLib/OpenBLAS#343 |
|
Yep, my "thanks" was not just for the comment :) Great job! |
Hanging multiprocessing tests are skipped on Mac OS X 10.9 in this PR. See also: #636.
test_k_means_plus_plus_init_2_jobsdoesn't fail for me on 10.9, but I've added skipping code anyways because it was intended to be skipped before, and failures could be non-deterministic.Also, I'm not sure customization hooks in
if_not_mac_os(versions and message arguments) worth keeping. But at least it is better than old_is_mac_os_versionfunction which had incorrect docstring.Not Mac specific:
BaggingRegressorwas untested with n_jobs=-1 (most likely because of a typo); this is also fixed.