-
Notifications
You must be signed in to change notification settings - Fork 179
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
TEST: Run all tests through pytest
#2090
Changes from all commits
075e016
c5e3f4b
3ba6819
a2e1e70
1026e4c
6e421b7
639d0fd
b295a44
c56f5bd
6ce5300
a8c3009
02caa5b
de145cc
accad99
6c7a161
a30eb50
a209abb
aa22bc5
5bb9c50
cac58b1
aa57ba0
5653e3f
fc09a8e
ef03240
d4a06eb
5a6e450
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,55 +15,136 @@ | |
# limitations under the License. | ||
# =============================================================================== | ||
|
||
import os | ||
import subprocess | ||
import sys | ||
|
||
# test patching from command line | ||
err_code = subprocess.call( | ||
[sys.executable, "-m", "sklearnex.glob", "patch_sklearn", "-a", "svc"] | ||
) | ||
assert not err_code | ||
from sklearn.svm import SVC, SVR | ||
import pytest | ||
|
||
assert SVC.__module__.startswith("daal4py") or SVC.__module__.startswith("sklearnex") | ||
assert not SVR.__module__.startswith("daal4py") and not SVR.__module__.startswith( | ||
"sklearnex" | ||
) | ||
# This is a workaround for older versions of Python on Windows | ||
# which didn't have it as part of the built-in 'os' module. | ||
EX_OK = os.EX_OK if hasattr(os, "EX_OK") else 0 | ||
|
||
# Note: from the structure of this file, one might think of adding a test | ||
|
||
from sklearnex import patch_sklearn, unpatch_sklearn | ||
# along the lines of 'test_patching_all_from_command_line'. There is however | ||
# an issue in that, after the first time a scikit-learn module is imported, | ||
# further calls to 'patch_sklearn' with different arguments will have no effect | ||
# since sklearn is already imported. Reloading it through 'importlib.reload' | ||
# or deleting it from 'sys.modules' doesn't appear to have the intended effect | ||
# either. This also makes this first command-line fixture and tests that use | ||
# it not entirely idempotent, given that they need to import sklearn modules. | ||
|
||
# test unpatching from command line | ||
err_code = subprocess.call([sys.executable, "-m", "sklearnex.glob", "unpatch_sklearn"]) | ||
assert not err_code | ||
unpatch_sklearn() | ||
from sklearn.svm import SVC, SVR | ||
# Note 2: don't try to change these into 'yield' fixtures, because otherwise | ||
# some test runners on windows which use multi-processing will throw errors | ||
# about the fixtures not being serializable. | ||
|
||
assert not SVC.__module__.startswith("daal4py") and not SVC.__module__.startswith( | ||
"sklearnex" | ||
) | ||
assert not SVR.__module__.startswith("daal4py") and not SVR.__module__.startswith( | ||
"sklearnex" | ||
) | ||
|
||
@pytest.fixture | ||
def patch_svc_from_command_line(request): | ||
err_code = subprocess.call( | ||
[sys.executable, "-m", "sklearnex.glob", "patch_sklearn", "-a", "svc"] | ||
) | ||
assert err_code == EX_OK | ||
|
||
# test patching from function | ||
patch_sklearn(name=["svc"], global_patch=True) | ||
from sklearn.svm import SVC, SVR | ||
def finalizer(): | ||
err_code = subprocess.call( | ||
[sys.executable, "-m", "sklearnex.glob", "unpatch_sklearn", "-a", "svc"] | ||
) | ||
assert err_code == EX_OK | ||
|
||
assert SVC.__module__.startswith("daal4py") or SVC.__module__.startswith("sklearnex") | ||
assert not SVR.__module__.startswith("daal4py") and not SVR.__module__.startswith( | ||
"sklearnex" | ||
) | ||
request.addfinalizer(finalizer) | ||
return | ||
|
||
|
||
# test unpatching from function | ||
unpatch_sklearn(global_unpatch=True) | ||
from sklearn.svm import SVC, SVR | ||
def test_patching_svc_from_command_line(patch_svc_from_command_line): | ||
from sklearn.svm import SVC, SVR | ||
|
||
assert not SVC.__module__.startswith("daal4py") and not SVC.__module__.startswith( | ||
"sklearnex" | ||
) | ||
assert not SVR.__module__.startswith("daal4py") and not SVR.__module__.startswith( | ||
"sklearnex" | ||
) | ||
assert SVC.__module__.startswith("daal4py") or SVC.__module__.startswith("sklearnex") | ||
assert not SVR.__module__.startswith("daal4py") and not SVR.__module__.startswith( | ||
"sklearnex" | ||
) | ||
|
||
|
||
def test_unpatching_svc_from_command_line(patch_svc_from_command_line): | ||
err_code = subprocess.call( | ||
[sys.executable, "-m", "sklearnex.glob", "unpatch_sklearn"] | ||
) | ||
assert err_code == EX_OK | ||
from sklearnex import unpatch_sklearn | ||
|
||
unpatch_sklearn() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/intel/scikit-learn-intelex/blob/main/sklearnex/tests/test_monkeypatch.py#L65 Just be careful with these functions since they impact global state and aren't local to the test (anything with a patch_sklearn call should be with a 'try-finally' with an unpatch_sklearn) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's the idea behind the fixtures with finalizers. |
||
from sklearn.svm import SVC, SVR | ||
|
||
assert not SVC.__module__.startswith("daal4py") and not SVC.__module__.startswith( | ||
"sklearnex" | ||
) | ||
assert not SVR.__module__.startswith("daal4py") and not SVR.__module__.startswith( | ||
"sklearnex" | ||
) | ||
|
||
|
||
@pytest.fixture | ||
def patch_svc_from_function(request): | ||
from sklearnex import patch_sklearn, unpatch_sklearn | ||
|
||
patch_sklearn(name=["svc"], global_patch=True) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again possibly yield? |
||
|
||
def finalizer(): | ||
unpatch_sklearn(name=["svc"], global_unpatch=True) | ||
|
||
request.addfinalizer(finalizer) | ||
return | ||
|
||
|
||
def test_patching_svc_from_function(patch_svc_from_function): | ||
from sklearn.svm import SVC, SVR | ||
|
||
assert SVC.__module__.startswith("daal4py") or SVC.__module__.startswith("sklearnex") | ||
assert not SVR.__module__.startswith("daal4py") and not SVR.__module__.startswith( | ||
"sklearnex" | ||
) | ||
|
||
|
||
def test_unpatching_svc_from_function(patch_svc_from_function): | ||
from sklearnex import unpatch_sklearn | ||
|
||
unpatch_sklearn(global_unpatch=True) | ||
from sklearn.svm import SVC, SVR | ||
|
||
assert not SVC.__module__.startswith("daal4py") | ||
assert not SVC.__module__.startswith("sklearnex") | ||
assert not SVR.__module__.startswith("daal4py") | ||
assert not SVR.__module__.startswith("sklearnex") | ||
|
||
|
||
@pytest.fixture | ||
def patch_all_from_function(request): | ||
from sklearnex import patch_sklearn, unpatch_sklearn | ||
|
||
patch_sklearn(global_patch=True) | ||
|
||
def finalizer(): | ||
unpatch_sklearn(global_unpatch=True) | ||
|
||
request.addfinalizer(finalizer) | ||
return | ||
|
||
|
||
def test_patching_svc_from_function(patch_all_from_function): | ||
from sklearn.svm import SVC, SVR | ||
|
||
assert SVC.__module__.startswith("daal4py") or SVC.__module__.startswith("sklearnex") | ||
assert SVR.__module__.startswith("daal4py") or SVR.__module__.startswith("sklearnex") | ||
|
||
|
||
def test_unpatching_all_from_function(patch_all_from_function): | ||
from sklearnex import unpatch_sklearn | ||
|
||
unpatch_sklearn(global_unpatch=True) | ||
from sklearn.svm import SVC, SVR | ||
|
||
assert not SVC.__module__.startswith("daal4py") | ||
assert not SVC.__module__.startswith("sklearnex") | ||
assert not SVR.__module__.startswith("daal4py") | ||
assert not SVR.__module__.startswith("sklearnex") |
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 state of the unpatch_sklearn() test was previously after a
subprocess.call([sys.executable, "-m", "sklearnex.glob", "unpatch_sklearn"])
not asubprocess.call(
[sys.executable, "-m", "sklearnex.glob", "patch_sklearn", "-a", "svc"] call, meaning the state of the test is different in the new implementation. This is okay if this was what you desired
#2090 (comment)
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.
Thanks, I had missed that detail entirely.
So I tried adding a new test for global state separately from just
SVC
, but the test actually ends up failing:should that be expected to succeed? same happens if checking them with "or", and happens for both SVC and SVR.
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 assume that its failing on
assert SVC.__module__.startswith("daal4py")
. You can removeassert SVC.__module__.startswith("daal4py")
andassert SVR.__module__.startswith("daal4py")
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 also fails for
sklearnex
.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 added it this new test as an extra commit here, let's see if it also fails in the CI.
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.
Could be a legit failure then, lets see
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.
Looks like it also fails in the CI jobs:
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.
Yeah this is bad, i'll admit I don't know much about the
glob
use. I wonder if sklearn has been pre-imported via sklearnex imports, and the change in glob not affecting it. There is definitely something here.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.
Yes, looks like that does indeed make a difference. If leaving it as the only test then it actually succeeds. Will try to find a workaround.
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 didn't manage to find any solution so in the end I just removed the test and left a comment in the file.