-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
CI Uses pytest-xdist to parallelize tests #13041
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
The same LGTM error happening on master as well: https://lgtm.com/projects/g/scikit-learn/scikit-learn/rev/3cb0b3bc22b0a6f99f80c025fd378660896a2c68 |
I merge #13044 PR. Could merge master. It seems that Travis did not launch last time. |
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.
The changes look good, but unless I'm mistaken there is (almost) no improvement in the Travis CI run time with this change.
What's the speed-up when you run these tests locally with pytest-xdist?
Not being able to speed up the travis build is concerning. I suspect running coverage slows it down. I am not content with how On my system, |
BTW This PR only enables |
The error in lgtm is a unrelated. I will fix it in another PR. |
@@ -31,10 +31,15 @@ test-doc: | |||
ifeq ($(BITS),64) | |||
$(PYTEST) $(shell find doc -name '*.rst' | sort) | |||
endif | |||
test-code-parallel: in | |||
$(PYTEST) -n auto --showlocals -v sklearn --durations=20 |
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.
Maybe add a comment that this requires pytest-xdist. Else pytest fails with
pytest: error: unrecognized arguments: -n
IF "%PYTHON_ARCH%"=="64" ( | ||
call activate %VIRTUALENV% | ||
set PYTEST_ARGS=%PYTEST_ARGS% -n2 |
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.
Briefly describe -n2 for future us?
@@ -123,11 +123,14 @@ def test_normalized_output(metric_name): | |||
assert not (score < 0).any() | |||
|
|||
|
|||
SUPERVISED_UNSUPERVISED_METRICS = {**SUPERVISED_METRICS, |
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.
has it been reverted @thomasjpfan ?
sklearn/utils/tests/test_testing.py
Outdated
@@ -446,6 +446,7 @@ def fit(self, X, y): | |||
"""Incorrect docstring but should not be tested""" | |||
|
|||
|
|||
@ignore_warnings(category=DeprecationWarning) |
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.
why is this needed? add a comment?
Looks like some of the sorted parameters are still dicts. Since xdist will only work on python >= 3.6 this is not needed. |
If you think this is finished, feel free to merge, @thomasjpfan |
I get 3.1x speed-up for the test suite on 4 cores (using |
Reference Issues/PRs
Fixes #13016
What does this implement/fix? Explain your changes.
Overall the changes are made to address how dictionaries are not ordered in Python <= 3.5. The major changes are as follows:
OrderedDict
in test to make sure the values passedpytest.mark.parametrize
has a deterministic order.sorted
on sets for a deterministic order.sklearn/linear_model/coordinate_descent.py
were to ensure that staticmethodpath
has a deterministic order in their respective classes. Only the two classes that useslasso_path
as a staticmethod are affected.pytest-xdist
.Any other comments?
When
pytest-xdist
spawns processes, the processes independently look through all files insklearn
to test. If the order of discovered test functions are not the same for all processes,pytest-xdist
will return an error.Travis-CI does not look like it gets that much faster. Locally, this PR does speed up running our tests suite.