TST improve error message on _more_tags and _get_tags deprecation #29801

adrinjalali · 2024-09-07T08:44:52Z

This PR improves the error message so that estimator developers have a better idea of how they can support multiple sklearn versions (ref: #29677 (comment))

Example pytest run:

$ pytest ~/Projects/sklearn/bugs/1.py
==================================================================================================================== test session starts =====================================================================================================================
platform linux -- Python 3.12.3, pytest-8.2.0, pluggy-1.5.0
rootdir: /home/adrin
plugins: xdist-3.5.0, anyio-4.4.0
collected 33 items                                                                                                                                                                                                                                           

Projects/sklearn/bugs/1.py ..................F..............                                                                                                                                                                                           [100%]

========================================================================================================================== FAILURES ==========================================================================================================================
_______________________________________________________________________________________________ test_my_estimator[MyEstimator()-check_estimator_tags_renamed] ________________________________________________________________________________________________

estimator = MyEstimator(), check = functools.partial(<function check_estimator_tags_renamed at 0x74f6290a14e0>, 'MyEstimator')

    @parametrize_with_checks([MyEstimator()])
    def test_my_estimator(estimator, check):
>       check(estimator)

Projects/sklearn/bugs/1.py:30: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

name = 'MyEstimator', estimator_orig = MyEstimator()

    def check_estimator_tags_renamed(name, estimator_orig):
        help = """_more_tags() was removed in 1.6. Please use __sklearn_tags__ instead.
    Use the following template to migrate if you wish to support multiple versions of
    scikit-learn:
    
    import sklearn
    from packaging import version
    from sklearn.base import BaseEstimator
    from sklearn.utils.metaestimators import available_if
    
    def check_version(estimator):
        return version.parse(sklearn.__version__) < version.parse("1.6.dev")
    
    class MyEstimator(BaseEstimator):
        ...
    
        @available_if(check_version)
        def {tags_func}(self):
            tags = dict(key=value, ...)
            return tags
    
        def __sklearn_tags__(self):
            tags = super().__sklearn_tags__()
            tags.some_key = False
            return tags
        ...
    """
    
>       assert not hasattr(estimator_orig, "_more_tags"), help.format(
            tags_func="_more_tags"
        )
E       AssertionError: _more_tags() was removed in 1.6. Please use __sklearn_tags__ instead.
E       Use the following template to migrate if you wish to support multiple versions of
E       scikit-learn: 
E       
E       import sklearn
E       from packaging import version
E       from sklearn.base import BaseEstimator
E       from sklearn.utils.metaestimators import available_if
E       
E       def check_version(estimator):
E           return version.parse(sklearn.__version__) < version.parse("1.6.dev")
E       
E       class MyEstimator(BaseEstimator):
E           ...
E       
E           @available_if(check_version)
E           def _more_tags(self):
E               tags = dict(key=value, ...)
E               return tags
E           
E           def __sklearn_tags__(self):
E               tags = super().__sklearn_tags__()
E               tags.some_key = False
E               return tags
E           ...

Projects/gh/me/scikit-learn/sklearn/utils/estimator_checks.py:3963: AssertionError
====================================================================================================================== warnings summary ======================================================================================================================
Projects/gh/me/scikit-learn/sklearn/utils/_array_api.py:123
  /home/adrin/Projects/gh/me/scikit-learn/sklearn/utils/_array_api.py:123: UserWarning: Some scikit-learn array API features might rely on enabling SciPy's own support for array API to function properly. Please set the SCIPY_ARRAY_API=1 environment variable before importing sklearn or scipy. More details at: https://docs.scipy.org/doc/scipy/dev/api-dev/array_api.html
    warnings.warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================================================================== short test summary info ===================================================================================================================
FAILED Projects/sklearn/bugs/1.py::test_my_estimator[MyEstimator()-check_estimator_tags_renamed] - AssertionError: _more_tags() was removed in 1.6. Please use __sklearn_tags__ instead.
========================================================================================================== 1 failed, 32 passed, 1 warning in 1.28s ===========================================================================================================

cc @adam2392 @glemaitre @larsoner

github-actions · 2024-09-07T08:46:10Z

✔️ Linting Passed

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

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

glemaitre · 2024-09-09T19:44:26Z

sklearn/utils/estimator_checks.py

+from sklearn.utils.metaestimators import available_if
+
+def check_version(estimator):
+    return version.parse(sklearn.__version__) < version.parse("1.6.dev")


I think that we can get the base version to avoid to have to deal with the .dev

I think it could be nice to have that in the user guide as well for the transition.

I'm reluctant to put this in the docs since it's about private API. I think this error message in reality is where users would see it, rather than the docs.

And as for the version, the issue is that 1.5.2 < 1.6.dev < 1.6 and the change really happens in 1.6.dev. If we use any other version, we might need to change it. I've also seen code in other repos where they reference some unreleased version of python.

Is it safe just to include _more_tags without checking the scikit-learn version? Specifically, can scikit-learn 1.6 ignore _more_tags if __sklearn_tags__ is defined?

I would guess the issue is that users would define _more_tags (but not __sklearn_tags__), but not have things work as expected once they transition to v1.6?

I think it's fine to have both methods. So, if the user defines both _more_tags and __sklearn_tags__ they most probably know what they're doing.

I've changed the PR to reflect that.

If they can safely define both, then can we remove the need for available_if? The story becomes "If you want to support multiple scikit-learn versions, define both."

Done, simplified the message.

glemaitre

LGTM also. Thanks @adrinjalali

TST improve error message on _more_tags and _get_tags deprecation

b70a12f

adrinjalali added the No Changelog Needed label Sep 7, 2024

github-actions bot added the module:utils label Sep 7, 2024

ogrisel added the Developer API Third party developer API related label Sep 9, 2024

glemaitre reviewed Sep 9, 2024

View reviewed changes

adrinjalali added 4 commits September 10, 2024 21:20

Merge remote-tracking branch 'upstream/main' into tags/message

87b1794

don't warn if __sklearn_tags__ defined

dcd4c21

test the check

28ce296

Merge remote-tracking branch 'upstream/main' into tags/message

7377d3d

vnherdeiro mentioned this pull request Sep 11, 2024

[python-package] require scikit-learn>=0.24.2, make scikit-learn estimators compatible with scikit-learn>=1.6.0dev microsoft/LightGBM#6651

Merged

adrinjalali added 3 commits September 11, 2024 21:25

relax condition and simplify message

46ce31f

Merge remote-tracking branch 'upstream/main' into tags/message

2065273

imrpove test

6ca460c

thomasjpfan approved these changes Sep 12, 2024

View reviewed changes

glemaitre self-requested a review September 12, 2024 13:06

glemaitre approved these changes Sep 12, 2024

View reviewed changes

glemaitre merged commit d4ab9ed into scikit-learn:main Sep 12, 2024
30 checks passed

adrinjalali deleted the tags/message branch September 12, 2024 13:17

jameslamb mentioned this pull request Sep 15, 2024

[ci] [python-package] scikit-learn compatibility tests fail with scikit-learn 1.6.dev0 microsoft/LightGBM#6653

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

TST improve error message on _more_tags and _get_tags deprecation #29801

TST improve error message on _more_tags and _get_tags deprecation #29801

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TST improve error message on _more_tags and _get_tags deprecation #29801

TST improve error message on _more_tags and _get_tags deprecation #29801

Uh oh!

Conversation

Uh oh!

Uh oh!

✔️ Linting Passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!