Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Nov 23, 2024
Merged

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

merged 18 commits into from
Nov 23, 2024

Conversation

glemaitre
Copy link
Member

@glemaitre glemaitre commented Nov 21, 2024 •
edited
Loading

closes #30298
closes #30302
closes #30324
closes #30257
Alternative to #30302

This PR is re-introduce back _get_tags, _more_tags, and _safe_tags. This is raising a deprecation warning.

To achieve backward compatibility, we convert the __sklearn_tags__ on the fly to the _more_tags previous api by checking what tag a specific class is setting. Then, we can use the MRO walk through as in the previous version.

I added tests for several combination of estimators old-new with several mixins to ensure that we cover most of the cases that currently exists in the wild.

@github-actions GitHub Actions
Copy link

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

✔️ Linting Passed

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

Generated for commit: 82367e1. Link to the linter CI: here

@glemaitre glemaitre mentioned this pull request Nov 21, 2024
Closed
This was referenced Nov 21, 2024
Closed
Closed
@glemaitre
Copy link
Member Author

OK it looks good for be reviewed. I'll push a last commit to cover the uncovered line.
ping @ogrisel @adrinjalali @jeremiedbb @lesteve

adrinjalali
adrinjalali approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits, but LGTM.

@adrinjalali
Copy link
Member

do you understand the doc build failure here?

@glemaitre
Copy link
Member Author

do you understand the doc build failure here?

No, is let's try to restart since the azure build went through at install :)

@glemaitre
Copy link
Member Author 

      [43/251] Compiling Cython source /home/circleci/project/sklearn/svm/_newrand.pyx
      FAILED: sklearn/svm/_newrand.cpython-39-x86_64-linux-gnu.so.p/sklearn/svm/_newrand.pyx.cpp
      cython -M --fast-fail -3 --cplus '-X language_level=3' '-X boundscheck=False' '-X wraparound=False' '-X initializedcheck=False' '-X nonecheck=False' '-X cdivision=True' '-X profile=False' --include-dir /home/circleci/project/build/cp39 /home/circleci/project/sklearn/svm/_newrand.pyx -o sklearn/svm/_newrand.cpython-39-x86_64-linux-gnu.so.p/sklearn/svm/_newrand.pyx.cpp
      Killed

I'm not sure what happen first but I suspect that something killed the process.

@glemaitre
Copy link
Member Author

OK this is reproducible. Let me check if there is an unpinned dependency that was bumped yesterday.

@glemaitre
Copy link
Member Author

I cannot spot a single difference in the versions of packages, this is really weird.

@adrinjalali
Copy link
Member

yeah it's odd, and it's not a timeout

thomasjpfan
thomasjpfan approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being backward compatible adds so much complexity. I mostly have nits, otherwise LGTM

sklearn/utils/_tags.py
"`sklearn.base.ClassifierMixin`, `sklearn.base.RegressorMixin`, and "
"`sklearn.base.OutlierMixin`. From scikit-learn 1.7, not defining "
"`__sklearn_tags__` will raise an error.",
category=FutureWarning,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally, I would go with a DeprecationWarning for developer focused changes. It's unfortunate that users will see this warning too if a library has a custom _more_tags. I do not see a good way around it because users can also write their own estimators with custom _more_tags.

In any case, I am okay with the current implementation.

glemaitre and others added 3 commits November 22, 2024 16:10
@glemaitre @thomasjpfan
Update sklearn/utils/_tags.py
ad4fe79 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@glemaitre @thomasjpfan
Update sklearn/utils/_tags.py
6e89afe 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@glemaitre @thomasjpfan
Update sklearn/utils/_tags.py
1764ea9 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@glemaitre
Copy link
Member Author

Being backward compatible adds so much complexity.

This is the story of my life in imbalanced-learn :)

@lesteve lesteve mentioned this pull request Nov 22, 2024
Merged
@glemaitre glemaitre mentioned this pull request Nov 22, 2024
Closed
@thomasjpfan thomasjpfan added this to the 1.6 milestone Nov 22, 2024
@thomasjpfan thomasjpfan added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Nov 22, 2024
@thomasjpfan thomasjpfan merged commit 46a7c9a into scikit-learn:main Nov 23, 2024
34 checks passed
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Dec 4, 2024
@glemaitre @adrinjalali
@thomasjpfan @lesteve @jeremiedbb
MAINT conversion old->new/new->old tags (bis) (scikit-learn#30327)
171c373 
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
jeremiedbb pushed a commit that referenced this pull request Dec 6, 2024
@glemaitre @adrinjalali
@thomasjpfan @lesteve @jeremiedbb
MAINT conversion old->new/new->old tags (bis) (#30327)
bfd91d7 
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
virchan pushed a commit to virchan/scikit-learn that referenced this pull request Dec 9, 2024
@glemaitre @adrinjalali
@thomasjpfan @lesteve @virchan
MAINT conversion old->new/new->old tags (bis) (scikit-learn#30327)
8796079 
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
@ozguraslank ozguraslank mentioned this pull request Dec 10, 2024
Merged
@StefanieSenger StefanieSenger mentioned this pull request Feb 16, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrinjalali adrinjalali adrinjalali approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
module:utils No Changelog Needed To backport PR merged in master that need a backport to a release branch defined based on the milestone.
Projects
None yet
Milestone
1.6
4 participants
@glemaitre @adrinjalali @thomasjpfan @lesteve