Repository: scikit-learn/scikit-learn
Pull request: Do not shadow public functions with deprecated modules
Fix #15842.

This can happen for:

- sklearn.decomposition.dict_learning
- sklearn.inspection.partial_dependence

Unfortunately this solution will always trigger the import of those two deprecated module (catching the FutureWarning). Subsequent imports (by the user) will therefore no longer see the warning as it will happen only once (at import time).

I do not see any alternative however.

Problem statement:
dict_learning and partial_dependence functions can be shadowed by deprecated module imports
Since scikit-learn 0.22, the `dict_learning` function is defined in `sklearn/decomposition/_dict_learning.py` and exposed as `sklearn.decomposition.dict_learning` in the `sklearn/decomposition/__init__.py` file.

At the same time we also have a `sklearn/decomposition/dict_learning.py` deprecated module for backward compat. This module is not imported by default in `sklearn/decomposition/__init__.py` to avoid deprecation warning.

So far so good. The problem is that if the users or a tool later import `sklearn.decomposition.dict_learning` (the deprecated module), then the symbol `sklearn.decomposition.dict_learning` points to the module instead of the function:

```python
>>> from sklearn.decomposition import dict_learning                                                                                                                             
>>> type(dict_learning)                                                                                                                                                         
<class 'function'>
>>> import sklearn.decomposition.dict_learning                                                                                                                                  
/home/ogrisel/code/scikit-learn/sklearn/utils/deprecation.py:144: FutureWarning: The sklearn.decomposition.dict_learning module is  deprecated in version 0.22 and will be removed in version 0.24. The corresponding classes / functions should instead be imported from sklearn.decomposition. Anything that cannot be imported from sklearn.decomposition is now part of the private API.
  warnings.warn(message, FutureWarning)
>>> from sklearn.decomposition import dict_learning                                                                                                                             
>>> type(dict_learning)                                                                                                                                                         
<class 'module'>
```

This can happen (semi-randomly?) when running test discovery with `pytest --pyargs sklearn` from a non-source folder (without the scikit-learn `conftest.py` file) or when calling `all_estimators()` from scikit-learn.

Note that in 0.21.3 we did not have the problem because once the `sklearn.decomposition.dict_learning` module is imported once, it is cached, and therefore, `sklearn.decomposition.dict_learning` is always referring to the function.

We have the same problem for the `sklearn.inspection.partial_dependence` function / module.

I will open a PR with a possible fix.

Hints from issue discussion:
We saw that before with @thomasjpfan

This is only an issue with pytest discovery. If you run the test files independently, you shouldn't get errors.

I thought we fixed that by bumping the minimal pytest version, but looking at the git blame and my commit history, I cannot find where/if this was actually done :/
The code snippet above is not using pytest.
If I recall correctly, it depends on the pytest version as well: https://github.com/scikit-learn/scikit-learn/blob/master/azure-pipelines.yml#L60
> The code snippet above is not using pytest.

Oh ups then
Yes but we do expect the warning to be raised when doing `import sklearn.decomposition.dict_learning`, right?
Yes. I am trying to come of with a solution. The following deprecated module snippet works:

```python

# THIS FILE WAS AUTOMATICALLY GENERATED BY deprecated_modules.py
import sys
from . import _partial_dependence
from ..externals._pep562 import Pep562
from ..utils.deprecation import _raise_dep_warning_if_not_pytest

deprecated_path = 'sklearn.inspection.partial_dependence'
correct_import_path = 'sklearn.inspection'

_raise_dep_warning_if_not_pytest(deprecated_path, correct_import_path)


def __getattr__(name):
    return getattr(_partial_dependence, name)


if not sys.version_info >= (3, 7):
    Pep562(__name__)


# Make sure that importing the deprecated module does not shadow
# the public function attribute with the same name in the parent
# module:
# https://github.com/scikit-learn/scikit-learn/issues/15842
from .. import inspection
inspection.partial_dependence = _partial_dependence.partial_dependence
```

I need to find a way to make the `deprecated_modules.py` script generate this automatically.
Actually I must have made something wrong the first time I tested it because the above snippet does not seem to work. The import mechanism change the parent attribute once the module has been fully imported hence any monkeypatching logic happening here won't work.
I opened #15846 but unfortunately it hides the warnings for those 2 modules.
Why did this issue not affect previous versions, where there was also an
import ambiguity?

Commit message: Improve phrasing

Patch to review:
diff --git a/sklearn/decomposition/__init__.py b/sklearn/decomposition/__init__.py
index 93a51b04f38d2..6646382dc1ef8 100644
--- a/sklearn/decomposition/__init__.py
+++ b/sklearn/decomposition/__init__.py
@@ -4,19 +4,31 @@
 this module can be regarded as dimensionality reduction techniques.
 """
 
-from ._nmf import NMF, non_negative_factorization
-from ._pca import PCA
-from ._incremental_pca import IncrementalPCA
-from ._kernel_pca import KernelPCA
-from ._sparse_pca import SparsePCA, MiniBatchSparsePCA
-from ._truncated_svd import TruncatedSVD
-from ._fastica import FastICA, fastica
+# TODO: remove me in 0.24 (as well as the noqa markers)
+# pre-cache the import of the deprecated module so that import
+# sklearn.decomposition.dict_learning returns the function as in
+# 0.21, instead of the module.
+# https://github.com/scikit-learn/scikit-learn/issues/15842
+import warnings
+with warnings.catch_warnings():
+    warnings.simplefilter("ignore", category=FutureWarning)
+    from . import dict_learning as _ignored
+    del _ignored
+
+
+from ._nmf import NMF, non_negative_factorization  # noqa
+from ._pca import PCA  # noqa
+from ._incremental_pca import IncrementalPCA  # noqa
+from ._kernel_pca import KernelPCA  # noqa
+from ._sparse_pca import SparsePCA, MiniBatchSparsePCA  # noqa
+from ._truncated_svd import TruncatedSVD  # noqa
+from ._fastica import FastICA, fastica  # noqa
 from ._dict_learning import (dict_learning, dict_learning_online,
                              sparse_encode, DictionaryLearning,
-                             MiniBatchDictionaryLearning, SparseCoder)
-from ._factor_analysis import FactorAnalysis
-from ..utils.extmath import randomized_svd
-from ._online_lda import LatentDirichletAllocation
+                             MiniBatchDictionaryLearning, SparseCoder)  # noqa
+from ._factor_analysis import FactorAnalysis  # noqa
+from ..utils.extmath import randomized_svd  # noqa
+from ._online_lda import LatentDirichletAllocation  # noqa
 
 __all__ = ['DictionaryLearning',
            'FastICA',
diff --git a/sklearn/inspection/__init__.py b/sklearn/inspection/__init__.py
index 04d9d84ecaf02..4a3f00006ac10 100644
--- a/sklearn/inspection/__init__.py
+++ b/sklearn/inspection/__init__.py
@@ -1,8 +1,21 @@
 """The :mod:`sklearn.inspection` module includes tools for model inspection."""
-from ._partial_dependence import partial_dependence
-from ._partial_dependence import plot_partial_dependence
-from ._partial_dependence import PartialDependenceDisplay
-from ._permutation_importance import permutation_importance
+
+# TODO: remove me in 0.24 (as well as the noqa markers)
+# pre-cache the import of the deprecated module so that import
+# sklearn.inspection.partial_dependence returns the function as in
+# 0.21, instead of the module
+# https://github.com/scikit-learn/scikit-learn/issues/15842
+import warnings
+with warnings.catch_warnings():
+    warnings.simplefilter("ignore", category=FutureWarning)
+    from . import partial_dependence as _ignored
+    del _ignored
+
+from ._partial_dependence import partial_dependence  # noqa
+from ._partial_dependence import plot_partial_dependence  # noqa
+from ._partial_dependence import PartialDependenceDisplay  # noqa
+from ._permutation_importance import permutation_importance  # noqa
+
 
 __all__ = [
     'partial_dependence',
diff --git a/sklearn/tests/test_import_deprecations.py b/sklearn/tests/test_import_deprecations.py
index 13b31e89b2862..29c4259fe1e5a 100644
--- a/sklearn/tests/test_import_deprecations.py
+++ b/sklearn/tests/test_import_deprecations.py
@@ -24,6 +24,12 @@ def test_import_is_deprecated(deprecated_path, importee):
 
     # TODO: remove in 0.24
 
+    # Special case for:
+    # https://github.com/scikit-learn/scikit-learn/issues/15842
+    if deprecated_path in ("sklearn.decomposition.dict_learning",
+                           "sklearn.inspection.partial_dependence"):
+        pytest.skip("No warning can be raised for " + deprecated_path)
+
     expected_message = (
         "The {deprecated_path} module is  deprecated in version "
         "0.22 and will be removed in version 0.24. "
diff --git a/sklearn/utils/tests/test_deprecated_utils.py b/sklearn/utils/tests/test_deprecated_utils.py
index da41e7e44ddb3..08bd95aacc284 100644
--- a/sklearn/utils/tests/test_deprecated_utils.py
+++ b/sklearn/utils/tests/test_deprecated_utils.py
@@ -1,7 +1,10 @@
 import pytest
+import types
 import numpy as np
+import warnings
 
 from sklearn.dummy import DummyClassifier
+from sklearn.utils import all_estimators
 from sklearn.utils.estimator_checks import choose_check_classifiers_labels
 from sklearn.utils.estimator_checks import NotAnArray
 from sklearn.utils.estimator_checks import enforce_estimator_tags_y
@@ -94,3 +97,35 @@ def test_safe_indexing():
     with pytest.warns(FutureWarning,
                       match="removed in version 0.24"):
         safe_indexing([1, 2], 0)
+
+
+# TODO: remove in 0.24
+def test_partial_dependence_no_shadowing():
+    # Non-regression test for:
+    # https://github.com/scikit-learn/scikit-learn/issues/15842
+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore", category=FutureWarning)
+        from sklearn.inspection.partial_dependence import partial_dependence as _  # noqa
+
+        # Calling all_estimators() also triggers a recursive import of all
+        # submodules, including deprecated ones.
+        all_estimators()
+
+    from sklearn.inspection import partial_dependence
+    assert isinstance(partial_dependence, types.FunctionType)
+
+
+# TODO: remove in 0.24
+def test_dict_learning_no_shadowing():
+    # Non-regression test for:
+    # https://github.com/scikit-learn/scikit-learn/issues/15842
+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore", category=FutureWarning)
+        from sklearn.decomposition.dict_learning import dict_learning as _  # noqa
+
+        # Calling all_estimators() also triggers a recursive import of all
+        # submodules, including deprecated ones.
+        all_estimators()
+
+    from sklearn.decomposition import dict_learning
+    assert isinstance(dict_learning, types.FunctionType)


Code review comments:
diff_hunk: @@ -1,8 +1,21 @@
 """The :mod:`sklearn.inspection` module includes tools for model inspection."""
-from ._partial_dependence import partial_dependence
-from ._partial_dependence import plot_partial_dependence
-from ._partial_dependence import PartialDependenceDisplay
-from ._permutation_importance import permutation_importance
+
+# TODO: remove me in 0.24 (as well as the noqa markers)
+# pre-cache the import of the deprecated module so that import
+# sklearn.inspection.partial_dependence returns the function as in
+# 0.21, instead of the module
+# https://github.com/scikit-learn/scikit-learn/issues/15842
+import warnings
+with warnings.catch_warnings():
+    warnings.simplefilter("ignore", category=FutureWarning)
+    from . import partial_dependence as _ignored
+    del _ignored
+
+from ._partial_dependence import partial_dependence  # noqa
path: sklearn/inspection/__init__.py
text: @user1:
For simplicity we can:

```py
with warnings.catch_warnings():
    warnings.simplefilter("ignore", category=FutureWarning)
    from .partial_dependence import partial_dependence
```

which will emulate the behavior in 0.21.

@author:
Alright.