Repository: scikit-learn/scikit-learn
Pull request: MNT Tags: quality of life improvements
Fixes https://github.com/scikit-learn/scikit-learn/issues/30011

This adds tests, and makes it more clear on how to extend tags.

It also does the right check on whether `_more_tags` is implemented but `__sklearn_tags__` is missing.

cc @glemaitre @thomasjpfan 

Problem statement:
Extending tags infrastructure is not easy anymore
In the context of bringing `imbalanced-learn` compatible with changes done in `scikit-learn` 1.6, I could see that now this is quite difficult to extend the tags infrastructure.

For instance, we added a `"dataframe"` entry to the previous `X_type` to check whether or not we should run our own test of sampler supporting dataframe.

However, because we switch from Python dictionary to Python dataclasses, mutating one of the dataclass is not possible.

@adrinjalali did we overlook at this side of the tag.

Hints from issue discussion:
The idea is that you need to subclass and add an attribute that way.
But we haven't tested this 
OK so I'll give a try.
So I'm playing with the tag in `imbalanced-learn`. Right now, I would like to extend `Tags` such that it contains a `SamplerTags`. There is two problems:

- I cannot subclass `Tags` and add `SamplerTags` because this is a default key coming after a non-default one (`InputTags` is the parent class). So I need to rewrite the class.
- The `check_estimator_get_tags_default_keys` will check that `__sklearn_tags__` provide a `sklearn.utils.Tags`. However, I'm providing a `imblearn.utils.Tags` because I don't have choice.

So it seems that this is still tricky to extend the tags for the moment.
@glemaitre Can you do the following to add a new dataframe input tag?

```python
from dataclasses import dataclass, field
from sklearn.utils import Tags, InputTags, TargetTags

@dataclass
class MyInputTags(InputTags):
    dataframe: bool = False
    
@dataclass
class MyTags(Tags):
    input_tags: MyInputTags = field(default_factory=MyInputTags)

t = MyTags(
    target_tags=TargetTags(required=True),
    transformer_tags=None,
    classifier_tags=None,
    regressor_tags=None,
    input_tags=MyInputTags(dataframe=True)
)

print(t.input_tags.dataframe)
```
It is indeed what I did here: https://github.com/scikit-learn-contrib/imbalanced-learn/pull/1104/files#diff-7d71613d30ea8043ed9cf51cca1d90489ad70d7e1192a3c1116342c56967f17cR10-R101

Commit message: add imports to doc

Patch to review:
diff --git a/doc/developers/develop.rst b/doc/developers/develop.rst
index ace3fbbcfa9c6..1e4d82c8d0425 100644
--- a/doc/developers/develop.rst
+++ b/doc/developers/develop.rst
@@ -519,7 +519,27 @@ which returns the new values for your estimator's tags. For example::
             return tags
 
 You can create a new subclass of :class:`~sklearn.utils.Tags` if you wish to add new
-tags to the existing set.
+tags to the existing set. Note that all attributes that you add in a child class need
+to have a default value. It can be of the form::
+
+    from dataclasses import dataclass, asdict
+
+    @dataclass
+    class MyTags(Tags):
+        my_tag: bool = True
+
+    class MyEstimator(BaseEstimator):
+        def __sklearn_tags__(self):
+            tags_orig = super().__sklearn_tags__()
+            tags = MyTags(**asdict(tags_orig))
+            tags.input_tags = tags_orig.input_tags
+            tags.target_tags = tags_orig.target_tags
+            tags.classifier_tags = tags_orig.classifier_tags
+            tags.regressor_tags = tags_orig.regressor_tags
+            tags.transformer_tags = tags_orig.transformer_tags
+            tags.my_tag = True
+            return tags
+
 
 .. _developer_api_set_output:
 
diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py
index 1191b9ed8bd42..d54916059c163 100644
--- a/sklearn/tests/test_common.py
+++ b/sklearn/tests/test_common.py
@@ -35,14 +35,6 @@
     StandardScaler,
 )
 from sklearn.utils import all_estimators
-from sklearn.utils._tags import (
-    ClassifierTags,
-    InputTags,
-    RegressorTags,
-    TargetTags,
-    TransformerTags,
-    get_tags,
-)
 from sklearn.utils._test_common.instance_generator import (
     _get_check_estimator_ids,
     _get_expected_failed_checks,
@@ -228,55 +220,6 @@ def test_class_support_removed():
         parametrize_with_checks([LogisticRegression])
 
 
-@pytest.mark.parametrize(
-    "estimator", _tested_estimators(), ids=_get_check_estimator_ids
-)
-def test_valid_tag_types(estimator):
-    """Check that estimator tags are valid."""
-    tags = get_tags(estimator)
-    assert isinstance(tags.estimator_type, (str, type(None)))
-    assert isinstance(tags.target_tags, TargetTags)
-    assert isinstance(tags.classifier_tags, (ClassifierTags, type(None)))
-    assert isinstance(tags.regressor_tags, (RegressorTags, type(None)))
-    assert isinstance(tags.transformer_tags, (TransformerTags, type(None)))
-    assert isinstance(tags.input_tags, InputTags)
-    assert isinstance(tags.array_api_support, bool)
-    assert isinstance(tags.no_validation, bool)
-    assert isinstance(tags.non_deterministic, bool)
-    assert isinstance(tags.requires_fit, bool)
-    assert isinstance(tags._skip_test, bool)
-
-    assert isinstance(tags.target_tags.required, bool)
-    assert isinstance(tags.target_tags.one_d_labels, bool)
-    assert isinstance(tags.target_tags.two_d_labels, bool)
-    assert isinstance(tags.target_tags.positive_only, bool)
-    assert isinstance(tags.target_tags.multi_output, bool)
-    assert isinstance(tags.target_tags.single_output, bool)
-
-    assert isinstance(tags.input_tags.pairwise, bool)
-    assert isinstance(tags.input_tags.allow_nan, bool)
-    assert isinstance(tags.input_tags.sparse, bool)
-    assert isinstance(tags.input_tags.categorical, bool)
-    assert isinstance(tags.input_tags.string, bool)
-    assert isinstance(tags.input_tags.dict, bool)
-    assert isinstance(tags.input_tags.one_d_array, bool)
-    assert isinstance(tags.input_tags.two_d_array, bool)
-    assert isinstance(tags.input_tags.three_d_array, bool)
-    assert isinstance(tags.input_tags.positive_only, bool)
-
-    if tags.classifier_tags is not None:
-        assert isinstance(tags.classifier_tags.poor_score, bool)
-        assert isinstance(tags.classifier_tags.multi_class, bool)
-        assert isinstance(tags.classifier_tags.multi_label, bool)
-
-    if tags.regressor_tags is not None:
-        assert isinstance(tags.regressor_tags.poor_score, bool)
-        assert isinstance(tags.regressor_tags.multi_label, bool)
-
-    if tags.transformer_tags is not None:
-        assert isinstance(tags.transformer_tags.preserves_dtype, list)
-
-
 def _estimators_that_predict_in_fit():
     for estimator in _tested_estimators():
         est_params = set(estimator.get_params())
diff --git a/sklearn/utils/_tags.py b/sklearn/utils/_tags.py
index 161ceb9e992fd..419bcfd6a46b5 100644
--- a/sklearn/utils/_tags.py
+++ b/sklearn/utils/_tags.py
@@ -232,9 +232,9 @@ class Tags:
 
     estimator_type: str | None
     target_tags: TargetTags
-    transformer_tags: TransformerTags | None
-    classifier_tags: ClassifierTags | None
-    regressor_tags: RegressorTags | None
+    transformer_tags: TransformerTags | None = None
+    classifier_tags: ClassifierTags | None = None
+    regressor_tags: RegressorTags | None = None
     array_api_support: bool = False
     no_validation: bool = False
     non_deterministic: bool = False
@@ -313,6 +313,7 @@ def get_tags(estimator) -> Tags:
     tags : :class:`~.sklearn.utils.Tags`
         The estimator tags.
     """
+
     if hasattr(estimator, "__sklearn_tags__"):
         tags = estimator.__sklearn_tags__()
     else:
diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py
index 604719896e413..6dc382a6e8f40 100644
--- a/sklearn/utils/estimator_checks.py
+++ b/sklearn/utils/estimator_checks.py
@@ -78,7 +78,14 @@
 from . import shuffle
 from ._missing import is_scalar_nan
 from ._param_validation import Interval, StrOptions, validate_params
-from ._tags import Tags, get_tags
+from ._tags import (
+    ClassifierTags,
+    InputTags,
+    RegressorTags,
+    TargetTags,
+    TransformerTags,
+    get_tags,
+)
 from ._test_common.instance_generator import (
     CROSS_DECOMPOSITION,
     _get_check_estimator_ids,
@@ -107,6 +114,8 @@
 def _yield_api_checks(estimator):
     tags = get_tags(estimator)
     yield check_estimator_cloneable
+    yield check_estimator_tags_renamed
+    yield check_valid_tag_types
     yield check_estimator_repr
     yield check_no_attributes_set_in_init
     yield check_fit_score_takes_y
@@ -166,9 +175,6 @@ def _yield_checks(estimator):
     yield check_estimators_pickle
     yield partial(check_estimators_pickle, readonly_memmap=True)
 
-    yield check_estimator_get_tags_default_keys
-    yield check_estimator_tags_renamed
-
     if tags.array_api_support:
         for check in _yield_array_api_checks(estimator):
             yield check
@@ -4336,15 +4342,58 @@ def {method}(self, X):
         estimator.partial_fit(X_bad, y)
 
 
-def check_estimator_get_tags_default_keys(name, estimator_orig):
-    # check that if __sklearn_tags__ is implemented, it's an instance of Tags
-    estimator = clone(estimator_orig)
-    if not hasattr(estimator, "__sklearn_tags__"):
-        return
-
-    assert isinstance(
-        estimator.__sklearn_tags__(), Tags
-    ), f"{name}.__sklearn_tags__() must be an instance of Tags"
+def check_valid_tag_types(name, estimator):
+    """Check that estimator tags are valid."""
+    assert hasattr(estimator, "__sklearn_tags__"), (
+        f"Estimator {name} does not have `__sklearn_tags__` method. This method is"
+        " implemented in BaseEstimator and returns a sklearn.utils.Tags instance."
+    )
+    err_msg = (
+        "Tag values need to be of a certain type. "
+        "Please refer to the documentation of `sklearn.utils.Tags` for more details."
+    )
+    tags = get_tags(estimator)
+    assert isinstance(tags.estimator_type, (str, type(None))), err_msg
+    assert isinstance(tags.target_tags, TargetTags), err_msg
+    assert isinstance(tags.classifier_tags, (ClassifierTags, type(None))), err_msg
+    assert isinstance(tags.regressor_tags, (RegressorTags, type(None))), err_msg
+    assert isinstance(tags.transformer_tags, (TransformerTags, type(None))), err_msg
+    assert isinstance(tags.input_tags, InputTags), err_msg
+    assert isinstance(tags.array_api_support, bool), err_msg
+    assert isinstance(tags.no_validation, bool), err_msg
+    assert isinstance(tags.non_deterministic, bool), err_msg
+    assert isinstance(tags.requires_fit, bool), err_msg
+    assert isinstance(tags._skip_test, bool), err_msg
+
+    assert isinstance(tags.target_tags.required, bool), err_msg
+    assert isinstance(tags.target_tags.one_d_labels, bool), err_msg
+    assert isinstance(tags.target_tags.two_d_labels, bool), err_msg
+    assert isinstance(tags.target_tags.positive_only, bool), err_msg
+    assert isinstance(tags.target_tags.multi_output, bool), err_msg
+    assert isinstance(tags.target_tags.single_output, bool), err_msg
+
+    assert isinstance(tags.input_tags.pairwise, bool), err_msg
+    assert isinstance(tags.input_tags.allow_nan, bool), err_msg
+    assert isinstance(tags.input_tags.sparse, bool), err_msg
+    assert isinstance(tags.input_tags.categorical, bool), err_msg
+    assert isinstance(tags.input_tags.string, bool), err_msg
+    assert isinstance(tags.input_tags.dict, bool), err_msg
+    assert isinstance(tags.input_tags.one_d_array, bool), err_msg
+    assert isinstance(tags.input_tags.two_d_array, bool), err_msg
+    assert isinstance(tags.input_tags.three_d_array, bool), err_msg
+    assert isinstance(tags.input_tags.positive_only, bool), err_msg
+
+    if tags.classifier_tags is not None:
+        assert isinstance(tags.classifier_tags.poor_score, bool), err_msg
+        assert isinstance(tags.classifier_tags.multi_class, bool), err_msg
+        assert isinstance(tags.classifier_tags.multi_label, bool), err_msg
+
+    if tags.regressor_tags is not None:
+        assert isinstance(tags.regressor_tags.poor_score, bool), err_msg
+        assert isinstance(tags.regressor_tags.multi_label, bool), err_msg
+
+    if tags.transformer_tags is not None:
+        assert isinstance(tags.transformer_tags.preserves_dtype, list), err_msg
 
 
 def check_estimator_tags_renamed(name, estimator_orig):
@@ -4353,13 +4402,20 @@ def check_estimator_tags_renamed(name, estimator_orig):
 scikit-learn versions.
 """
 
-    if not hasattr(estimator_orig, "__sklearn_tags__"):
-        assert not hasattr(estimator_orig, "_more_tags"), help.format(
-            tags_func="_more_tags"
-        )
-        assert not hasattr(estimator_orig, "_get_tags"), help.format(
-            tags_func="_get_tags"
-        )
+    for klass in type(estimator_orig).mro():
+        if (
+            # Here we check vars(...) because we want to check if the method is
+            # explicitly defined in the class instead of inherited from a parent class.
+            ("_more_tags" in vars(klass) or "_get_tags" in vars(klass))
+            and "__sklearn_tags__" not in vars(klass)
+        ):
+            raise TypeError(
+                f"Estimator {name} has defined either `_more_tags` or `_get_tags`,"
+                " but not `__sklearn_tags__`. If you're customizing tags, and need to"
+                " support multiple scikit-learn versions, you can implement both"
+                " `__sklearn_tags__` and `_more_tags` or `_get_tags`. This change was"
+                " introduced in scikit-learn=1.6"
+            )
 
 
 def check_dataframe_column_names_consistency(name, estimator_orig):
diff --git a/sklearn/utils/tests/test_estimator_checks.py b/sklearn/utils/tests/test_estimator_checks.py
index 003ec488de81a..cd95af2c1145c 100644
--- a/sklearn/utils/tests/test_estimator_checks.py
+++ b/sklearn/utils/tests/test_estimator_checks.py
@@ -1535,10 +1535,10 @@ def __sklearn_tags__(self):
         def _more_tags(self):
             return None  # pragma: no cover
 
-    msg = "was removed in 1.6. Please use __sklearn_tags__ instead."
-    with raises(AssertionError, match=msg):
+    msg = "has defined either `_more_tags` or `_get_tags`"
+    with raises(TypeError, match=msg):
         check_estimator_tags_renamed("BadEstimator1", BadEstimator1())
-    with raises(AssertionError, match=msg):
+    with raises(TypeError, match=msg):
         check_estimator_tags_renamed("BadEstimator2", BadEstimator2())
 
     # This shouldn't fail since we allow both __sklearn_tags__ and _more_tags
diff --git a/sklearn/utils/tests/test_tags.py b/sklearn/utils/tests/test_tags.py
index 5768a0d2b6b27..846e7346320ee 100644
--- a/sklearn/utils/tests/test_tags.py
+++ b/sklearn/utils/tests/test_tags.py
@@ -1,3 +1,5 @@
+from dataclasses import asdict, dataclass
+
 import pytest
 
 from sklearn.base import (
@@ -5,7 +7,12 @@
     RegressorMixin,
     TransformerMixin,
 )
+from sklearn.utils import Tags
 from sklearn.utils._tags import get_tags
+from sklearn.utils.estimator_checks import (
+    check_estimator_tags_renamed,
+    check_valid_tag_types,
+)
 
 
 class NoTagsEstimator:
@@ -38,3 +45,38 @@ class EmptyRegressor(RegressorMixin, BaseEstimator):
 )
 def test_requires_y(estimator, value):
     assert get_tags(estimator).target_tags.required == value
+
+
+def test_no___sklearn_tags__with_more_tags():
+    """Test that calling `get_tags` on a class that defines `_more_tags` but not
+    `__sklearn_tags__` raises an error.
+    """
+
+    class MoreTagsEstimator(BaseEstimator):
+        def _more_tags(self):
+            return {"requires_y": True}
+
+    with pytest.raises(
+        TypeError, match="has defined either `_more_tags` or `_get_tags`"
+    ):
+        check_estimator_tags_renamed("MoreTagsEstimator", MoreTagsEstimator())
+
+
+def test_tag_test_passes_with_inheritance():
+    @dataclass
+    class MyTags(Tags):
+        my_tag: bool = True
+
+    class MyEstimator(BaseEstimator):
+        def __sklearn_tags__(self):
+            tags_orig = super().__sklearn_tags__()
+            tags = MyTags(**asdict(tags_orig))
+            tags.input_tags = tags_orig.input_tags
+            tags.target_tags = tags_orig.target_tags
+            tags.classifier_tags = tags_orig.classifier_tags
+            tags.regressor_tags = tags_orig.regressor_tags
+            tags.transformer_tags = tags_orig.transformer_tags
+            tags.my_tag = True
+            return tags
+
+    check_valid_tag_types("MyEstimator", MyEstimator())


Code review comments:
diff_hunk: @@ -1,11 +1,18 @@
+from dataclasses import asdict, dataclass
+
 import pytest
 
 from sklearn.base import (
     BaseEstimator,
     RegressorMixin,
     TransformerMixin,
 )
+from sklearn.utils import Tags
 from sklearn.utils._tags import get_tags
path: sklearn/utils/tests/test_tags.py
text: @user1:
```suggestion
from sklearn.utils import get_tags
```