Skip to content

FIX short deprecation cycle for private module #31500

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 5 commits into from
Jul 8, 2025
Merged

FIX short deprecation cycle for private module #31500

merged 5 commits into from
Jul 8, 2025

Conversation

glemaitre
Copy link
Member

In #30763, we reorganize the module for the repr of estimator. The public functions associated with this module and that could be potentially used by developer of third-party libraries should not have subject to any breaking changes.

Right now, we are breaking the code of people if they were importing from the module. Let's be a bit more gentle and just warn for a cycle before to remove the module.

@glemaitre glemaitre added this to the 1.7.1 milestone Jun 6, 2025
@glemaitre
Copy link
Member Author

I added this PR to the 1.7.1 milestone.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 6, 2025 •
edited
Loading

✔️ Linting Passed

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

Generated for commit: 800e112. Link to the linter CI: here

thomasjpfan
thomasjpfan reviewed Jun 6, 2025
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I do not think we normally had deprecation cycle for a private module. Was this private module use in a library you know about?

@glemaitre
Copy link
Member Author

It was skrub. One thing that we did in the past was to be a bit more gentled with the utils when we moved things around. Here, we did not yet decide what thing of the repr should be used by third-party libraries. I thought that we could be extra nice until we converged to something a bit more stable.

@jeremiedbb
Copy link
Member

I don't know why the test fails in the CI but pass locally... Maybe we need to deprecate using __get_item__ ?

@jeremiedbb
Copy link
Member

The issue is that the module is imported by another test. Sometimes before, sometimes after, and when it's before it means that the warning was already raised in the first test so it's not during the test of interest. Thus I modified the test to reload the module if it was already loaded to make sure that the warning is raised no matter the execution order of the tests.

jeremiedbb
jeremiedbb approved these changes Jul 7, 2025
View reviewed changes
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM

@glemaitre what do you think of the changes I made to your PR before merging ?

betatim
betatim approved these changes Jul 7, 2025
View reviewed changes
Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

LGTM

@lesteve
Copy link
Member

lesteve commented Jul 8, 2025

Let's merge this! The work-around of reloading the module in the test feels a bit hacky, but this will be removed for the 1.8 release so I guess we can leave with it.

@lesteve lesteve merged commit 57df491 into scikit-learn:main Jul 8, 2025
36 checks passed
@jeremiedbb jeremiedbb added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Jul 11, 2025
jeremiedbb added a commit to jeremiedbb/scikit-learn that referenced this pull request Jul 15, 2025
@glemaitre @jeremiedbb
FIX short deprecation cycle for private module (scikit-learn#31500)
526738e 
Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
@jeremiedbb jeremiedbb mentioned this pull request Jul 15, 2025
Merged
13 tasks
jeremiedbb added a commit to jeremiedbb/scikit-learn that referenced this pull request Jul 16, 2025
@glemaitre @jeremiedbb
FIX short deprecation cycle for private module (scikit-learn#31500)
b35ee54 
Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
jeremiedbb added a commit to jeremiedbb/scikit-learn that referenced this pull request Jul 16, 2025
@glemaitre @jeremiedbb
FIX short deprecation cycle for private module (scikit-learn#31500)
e5dce82 
Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
jeremiedbb added a commit to jeremiedbb/scikit-learn that referenced this pull request Jul 16, 2025
@glemaitre @jeremiedbb
FIX short deprecation cycle for private module (scikit-learn#31500)
9ef6593 
Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
jeremiedbb added a commit to jeremiedbb/scikit-learn that referenced this pull request Jul 16, 2025
@glemaitre @jeremiedbb
FIX short deprecation cycle for private module (scikit-learn#31500)
c443f7b 
Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
jeremiedbb added a commit to jeremiedbb/scikit-learn that referenced this pull request Jul 16, 2025
@glemaitre @jeremiedbb
FIX short deprecation cycle for private module (scikit-learn#31500)
0a13699 
Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betatim betatim betatim approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@jeremiedbb jeremiedbb jeremiedbb 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.7.1
Development

Successfully merging this pull request may close these issues.
5 participants
@glemaitre @jeremiedbb @lesteve @betatim @thomasjpfan