-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
TST improve error message on _more_tags and _get_tags deprecation #29801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
sklearn/utils/estimator_checks.py
Outdated
from sklearn.utils.metaestimators import available_if | ||
|
||
def check_version(estimator): | ||
return version.parse(sklearn.__version__) < version.parse("1.6.dev") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we can get the base
version to avoid to have to deal with the .dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be nice to have that in the user guide as well for the transition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, simplified the message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM also. Thanks @adrinjalali
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:cc @adam2392 @glemaitre @larsoner