MAINT conversion old->new/new->old tags #30302

glemaitre · 2024-11-18T21:50:59Z

Towards #30298

This PR provides a way to convert old tags into new tags and new tags into old tags.
We should make sure that:

An estimator implementing _get_tags or _more_tags get a warning but that the check_estimator should be working reasonably
_safe_tags should raise a deprecation warning
we should make sure that get_tags can get old tags and convert it to new tags. The tricky part is to detect whether __sklearn_tags__ if present is implemented by the BaseEstimator from scikit-learn or the child class. If this is only the BaseEstimator, we should raise a warning to move to the new API and we should temporary use the _get_tags/_safe_tags, otherwise we are fine.

github-actions · 2024-11-18T21:52:19Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: b41d1a6. Link to the linter CI: here}

jeremiedbb · 2024-11-19T15:46:43Z

sklearn/utils/_tags.py

@@ -318,6 +318,14 @@ def get_tags(estimator) -> Tags:

    if hasattr(estimator, "__sklearn_tags__"):
        tags = estimator.__sklearn_tags__()
+    elif hasattr(estimator, "_get_tags"):
+        warnings.warn("BROKEN SOON, IT WILL BE", FutureWarning)


I'm entertaining myself as I can ;)

glemaitre · 2024-11-20T18:50:40Z

OK, so I'm converging with something working. I'm sorry for the reviewers of this PR... Trust the tests ;)

ping @jeremiedbb @ogrisel @adrinjalali

adrinjalali

a few comments

adrinjalali · 2024-11-21T07:34:53Z

sklearn/base.py

+        # TODO(1.7): Remove this block
+        if _find_tags_provider(self) == "_get_tags":
+            # one of the children classes only implements `_get_tags` so we need to
+            # warn and and mix old-style and new-style tags.


Suggested change

# warn and and mix old-style and new-style tags.

# warn and mix old-style and new-style tags.

adrinjalali · 2024-11-21T08:34:01Z

sklearn/base.py

+
+        collected_tags = {}
+        for base_class in reversed(inspect.getmro(self.__class__)):
+            if hasattr(base_class, "_more_tags"):


is this equivalent to checking vars?

This is the old implementation of _get_tags. So I would not touch it.
The only change here is to fallback to __sklearn_tags__ in case someone in the chain of inheritance implemented it (like scikit-learn implementing the new tags and removing the old tags).

Now, I get your remark. I think that _more_tags was the API trick to avoid going through the MRO and thus an equivalent of vars(__sklearn_tags__) but by design. This should solve my bug below :) (edit: or not because I don't know how to avoid the climbing of the inheritance).

adrinjalali · 2024-11-21T08:35:14Z

sklearn/base.py

+                more_tags = base_class.__sklearn_tags__(self)
+                collected_tags.update(_to_old_tags(more_tags))


pretty sure this is gonna be buggy since __sklearn_tags__ collects all tags from base_class upwards.

Can you provide me a case where it would break?

Since this is not covered, I'm not doing the case that I was thinking of :).

So this would be:

class Mixin: def __sklearn_tags__(self): tags = super().__sklearn_tags__() tags.input_tags.allow_nan = True return tags class MyClass(Mixin, BaseEstimator): def _more_tags(self): return { "multioutput": True }

and indeed it end up with a recursion error.

adrinjalali · 2024-11-21T08:37:20Z

sklearn/feature_extraction/text.py

+    def __sklearn_tags__(self):
+        tags = super().__sklearn_tags__()
+        tags.transformer_tags = TransformerTags(preserves_dtype=[])
+        return tags


this class is not a transformer, why do we attach transformer tags here?

A transformer is something that has a fit + transform method, isn't it?

Yes, and the _VectorizerMixin mixin has neither of them.

adrinjalali · 2024-11-21T08:39:30Z

sklearn/manifold/_mds.py

@@ -572,6 +573,7 @@ def __init__(

    def __sklearn_tags__(self):
        tags = super().__sklearn_tags__()
+        tags.transformer_tags = TransformerTags(preserves_dtype=["float64"])


why isn't this inheriting from TransformerMixin?

Because we don't want to implement transform but only fit_transform such that you can only provide the same data at fit and transform.

At least this is what I recall.

TransformerMixin doesn't do anything to transform. Here we'd use its tags, and override fit_transform from the mixin.

adrinjalali · 2024-11-21T08:39:59Z

sklearn/manifold/_spectral_embedding.py

@@ -654,6 +655,7 @@ def __sklearn_tags__(self):
            "precomputed",
            "precomputed_nearest_neighbors",
        ]
+        tags.transformer_tags = TransformerTags(preserves_dtype=["float64"])


same answer than above ;)

adrinjalali · 2024-11-21T08:41:20Z

sklearn/model_selection/tests/test_search.py

 # Neither of the following two estimators inherit from BaseEstimator,
 # to test hyperparameter search on user-defined classifiers.
-class MockClassifier(ClassifierMixin, BaseEstimator):
+class MockClassifier(TransformerMixin, ClassifierMixin, BaseEstimator):


the comment is not valid anymore, and we have the same estimator in test_rfe

adrinjalali · 2024-11-21T08:52:40Z

sklearn/utils/_tags.py

@@ -243,7 +245,7 @@ class Tags:
    input_tags: InputTags = field(default_factory=InputTags)


-# TODO(1.8): Remove this function
+# TODO(1.7): Remove this function


then we also need to change the warnings / comments about _estimator_type as well

sklearn/utils/_tags.py

glemaitre · 2024-11-21T13:01:00Z

I'll add more test to cover the missing bits. I'll do it in a way to emulate old estimators.

glemaitre · 2024-11-21T19:58:15Z

I open #30327 that is a simpler alternative: when we cannot rely solely on __sklearn_tags__, we patch all classes in the inheritance tree by detecting the tags to be added in a _more_tags old API.

MAINT conversion old->new new->old tags

d74c0e8

glemaitre force-pushed the is/30298_tags branch from f14ac4f to d74c0e8 Compare November 18, 2024 21:52

glemaitre changed the title ~~MAINT conversion old->new new->old tags~~ MAINT conversion old->new/new->old tags Nov 18, 2024

jeremiedbb reviewed Nov 19, 2024

View reviewed changes

glemaitre added 10 commits November 19, 2024 19:30

iter

4d70067

versioon

6366366

correct version removal

4ea106d

iter

109c214

iter

b3c7be9

iter

bb24091

lint

3d107eb

call new API in old function when it makes sens

ae20c68

lint

fe55288

iter

7aa9981

glemaitre added the No Changelog Needed label Nov 19, 2024

glemaitre added 13 commits November 19, 2024 23:55

iter

98ac133

silence some warning and make some tests

7f6b14b

lint

c68f368

iter

a46e593

lint

55d7603

fix pipeline

822e9c8

fix vectorizer

c914e09

more fixes

521718c

add comments

dfe7c1f

put back try/except

5c9f23f

comment

8dbb358

Merge remote-tracking branch 'origin/main' into is/30298_tags

424ed90

last fix

2bf876e

fix

b41d1a6

adrinjalali reviewed Nov 21, 2024

View reviewed changes

glemaitre mentioned this pull request Nov 21, 2024

MAINT conversion old->new/new->old tags (bis) #30327

Merged

thomasjpfan closed this in #30327 Nov 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT conversion old->new/new->old tags #30302

MAINT conversion old->new/new->old tags #30302

glemaitre commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading

jeremiedbb Nov 19, 2024

glemaitre Nov 19, 2024

glemaitre commented Nov 20, 2024

adrinjalali left a comment

adrinjalali Nov 21, 2024

adrinjalali Nov 21, 2024

glemaitre Nov 21, 2024

glemaitre Nov 21, 2024 •

edited

Loading

adrinjalali Nov 21, 2024

glemaitre Nov 21, 2024

glemaitre Nov 21, 2024

glemaitre Nov 21, 2024

adrinjalali Nov 21, 2024

glemaitre Nov 21, 2024

adrinjalali Nov 21, 2024

adrinjalali Nov 21, 2024

glemaitre Nov 21, 2024

adrinjalali Nov 21, 2024

adrinjalali Nov 21, 2024

glemaitre Nov 21, 2024

adrinjalali Nov 21, 2024

adrinjalali Nov 21, 2024

glemaitre commented Nov 21, 2024

glemaitre commented Nov 21, 2024

	# warn and and mix old-style and new-style tags.
	# warn and mix old-style and new-style tags.

		more_tags = base_class.__sklearn_tags__(self)
		collected_tags.update(_to_old_tags(more_tags))

MAINT conversion old->new/new->old tags #30302

MAINT conversion old->new/new->old tags #30302

Conversation

glemaitre commented Nov 18, 2024 • edited Loading

github-actions bot commented Nov 18, 2024 • edited Loading

✔️ Linting Passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemaitre commented Nov 20, 2024

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemaitre Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemaitre commented Nov 21, 2024

glemaitre commented Nov 21, 2024

glemaitre commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading

glemaitre Nov 21, 2024 •

edited

Loading