[MRG] Fix DeprecationWarning due to collections.abc in Python 3.7 #11431
Conversation
sklearn/tests/test_init.py
Outdated
| # Test that importing scikit-learn doesn't raise any warnings. | ||
|
|
||
| message = subprocess.check_output(['python', '-Wdefault', | ||
| '-c', 'import sklearn'], |
There was a problem hiding this comment.
Not sure there is a better way of testing this? Trying to run the import in a separate process with multiprocessing but I couldn't see the deprecation warning. Using importlib.reload + "always" warning filter also didn't work. Besides I wasn't sure how it might interact with pytest warning capture, so using subprocess seems easier to isolate the environment.
There was a problem hiding this comment.
Does this really fail at master? Surely sklearn/__init__.py is light enough that no warning is triggered...?
There was a problem hiding this comment.
It does fail on master with Python 3.7 without this PR as described in the original issue #11121 (comment)
sklearn/__init__.py imports base.py which imports sklearn/utils/__init__.py which imports collections.abc.Sequence. There is second place it is raised from the __init__ I think as well.
There was a problem hiding this comment.
I don't really see the point in testing the import of sklearn, and not testing the import of every module for either an internal DeprecationWarning or no warning.
|
This pull request introduces 4 alerts when merging 3018864 into 6f9a8df - view on LGTM.com new alerts:
Comment posted by LGTM.com |
These are false positive for unused imports in fixes.. |
Good point. The last commit tests that no warning is raised at import by any of the top level modules. Another point I was concerned about is that this is quite easily breakable. For instance, future numpy release in 1 year might still work with 0.20 but raise some DeprecationWarning, which will break this test. So I have skipped this test at failure, while printing an informative skipped message. For instance, introducing a DeprecationWarning in |
|
why don't you just run it as a separate ci script if you want it to happen
in ci but not at home?
|
|
This pull request introduces 4 alerts when merging 128d460 into 6f9a8df - view on LGTM.com new alerts:
Comment posted by LGTM.com |
I want this to be tested both in CI and when running tests locally. I just don't want this be reported as a critical test failure, when it does fail (e.g. with outdated or future versions of dependencies). |
|
how do you anticipate it being useful if it skips
|
|
The error message will still be visible in the "short test summary info" pytest section where skipped tests are listed. To give you an example, in https://travis-ci.org/scikit-learn/scikit-learn/builds/400205299 this tests used to fail,
these are now handled as special case and are ignored, but there may be other combinations of systems / dependencies would produce other warnings. The point I think is that we don't want to be in a situation where this test makes the tests suite fail during downstream packaging of scikit-learn (e.g. for debian, using PyPy etc) due to some peculiarities of the environment. So IMO this test is inherently not robust enough to be included as a mandatory test, but at the same time it's useful to know information as currently we have no mechanism to detect issues like #11121. This provides such mechanism with a test that is allowed to fail. |
|
Also it's not ideal to make it mandatory in CI and optional locally (for instance looking with Unless you see a better approach? |
|
Is there anything else I can do to make a more convincing case for this PR @jnothman ? I can remove the added test if you are not enthusiastic about it, but then this deprecation warning filter won't be tested. I think it might be worth backporting this fix to 0.19.2. Maybe @lesteve would have an opinion about this PR as well. |
|
And yes, backporting to 0.19.2 makes sense. @ogrisel should probably handle
that, as he is managing the wheel building.
|
| # Test that importing scikit-learn main modules doesn't raise any warnings. | ||
|
|
||
| try: | ||
| pkgs = pkgutil.iter_modules(path=sklearn.__path__, prefix='sklearn.') |
There was a problem hiding this comment.
wait what happens if a submodule is deprecated? then this fails, right?
|
lgtm apart from the test which is a bit confusing to me. |
|
so we backport to 0.19.2, release, and then remove this again in 0.21-dev? |
|
hm these are public so we would need to deprecate them to remove them again? or we make fixes private? :-/ can we import as _Thing so it's explicit they are private maybe? |
|
Thanks for the review!
Yes, it means that when new modules are deprecated, they would need to be explicitly excluded from this check.
If py2.7 support is dropped in 0.21-dev, we could indeed remove this fix, and just use
Good point. Will make them private in fixes (e.g.
I agree the test is a bit convoluted but I couldn't find a simpler solution. What should I do then: remove it altogether, considering that the fix will not be needed in 0.21? |
|
This pull request introduces 8 alerts when merging c9dc88f into 6f9a8df - view on LGTM.com new alerts:
Comment posted by LGTM.com |
sklearn/tests/test_init.py
Outdated
| for _, modname, _ in pkgs | ||
| if not modname.startswith('_')]) | ||
|
|
||
| message = subprocess.check_output(['python', '-Wdefault', |
There was a problem hiding this comment.
we should probably do this in tests/test_init.py where we check that importing sklearn raises no warning.
sklearn/tests/test_init.py
Outdated
| message = message.decode("utf-8") | ||
| message = '\n'.join([line for line in message.splitlines() | ||
| if not ( # ignore ImportWarning | ||
| "ImportWarning" in line or |
There was a problem hiding this comment.
In my case, I get,
$ python -Wdefault -c "import sklearn" [collections-abc-py37]
/home/rth/.miniconda3/envs/sklearn-dev/lib/python3.6/importlib/_bootstrap.py:219: ImportWarning: can't resolve package from __spec__ or __package__, falling back on __name__ and __path__
return f(*args, **kwds)
/home/rth/.miniconda3/envs/sklearn-dev/lib/python3.6/importlib/_bootstrap.py:219: ImportWarning: can't resolve package from __spec__ or __package__, falling back on __name__ and __path__
return f(*args, **kwds)
so this ignores these ImportWarnings
There was a problem hiding this comment.
do you know what that means? And does that mean we want to ignore all import warnings?
There was a problem hiding this comment.
There is a related discussion in astropy/astropy#6025 suggesting some of it could come from Cython. In my case, it seems to be due to importing a pyx file in MKL,
PYTHONWARNINGS='error::ImportWarning' python -c "import sklearn"
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "/home/rth/src/scikit-learn/sklearn/__init__.py", line 64, in <module>
from .base import clone
File "/home/rth/src/scikit-learn/sklearn/base.py", line 10, in <module>
import numpy as np
File "/home/rth/.miniconda3/envs/sklearn-dev/lib/python3.6/site-packages/numpy/__init__.py", line 180, in <module>
from . import fft
File "/home/rth/.miniconda3/envs/sklearn-dev/lib/python3.6/site-packages/numpy/fft/__init__.py", line 14, in <module>
import mkl_fft._numpy_fft as _nfft
File "/home/rth/.miniconda3/envs/sklearn-dev/lib/python3.6/site-packages/mkl_fft/__init__.py", line 27, in <module>
from ._pydfti import (fft, ifft, fft2, ifft2, fftn, ifftn, rfft, irfft,
File "mkl_fft/_pydfti.pyx", line 27, in init mkl_fft._pydfti
ImportWarning: can't resolve package from __spec__ or __package__, falling back on __name__ and __path__in any case it seems to be upstream issues, that will not be seen by users (as ImportWarning is ignored by default), I'm not sure it's worth the effort investigating further.
There was a problem hiding this comment.
fair, maybe add a comment to this effect?
sklearn/tests/test_init.py
Outdated
| # ignore DeprecationWarnings due to | ||
| 10000 # numpy.oldnumeric | ||
| "oldnumeric" in line or | ||
| # ignore FutureWarnings |
There was a problem hiding this comment.
Why? which ones? from sklearn or external? If internal, maybe we should have our own subclass?
|
seems good to me. |
|
This pull request introduces 8 alerts when merging a2f208d into 6f9a8df - view on LGTM.com new alerts:
Comment posted by LGTM.com |
…ikit-learn#11431) Closes scikit-learn#11121 This PR removes the deprecation warning about ABC being moved from `collections` to `collections.abc` when importing scikit-learn in Python 3.7. In the end, I put `collections.abc.{Sequence, Iterable, Mapping, Sized}` in the namespace of `sklearn.utils.fixes`. This was the simplest way I could find, and while it has the drawback of obfuscating the real module name, other approached appeared more problematic and a similar approach is currently used e.g. for `utils.fixes.signature` which is an alias for `inspect.signature`. We can't just patch six with benjaminp/six#241, because sklearn uses six from 5 years ago, which would need updating and I'm not sure if it could have side effects (e.g. for pickling backward compatibility etc). **Edit**: This adds a test checking that generally no warnings are raised when importing scikit-learn top-level modules.
Closes #11121
This PR removes the deprecation warning about ABC being moved from
collectionstocollections.abcwhen importing scikit-learn in Python 3.7.In the end, I put
collections.abc.{Sequence, Iterable, Mapping, Sized}in the namespace ofsklearn.utils.fixes. This was the simplest way I could find, and while it has the drawback of obfuscating the real module name, other approached appeared more problematic and a similar approach is currently used e.g. forutils.fixes.signaturewhich is an alias forinspect.signature.We can't just patch six with benjaminp/six#241, because sklearn uses six from 5 years ago, which would need updating and I'm not sure if it could have side effects (e.g. for pickling backward compatibility etc).
Edit: This adds a test checking that generally no warnings are raised when importing scikit-learn top-level modules.