Skip to content
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

ENH Adds lazy module at the top level #29793

Merged

Conversation

thomasjpfan
Copy link
Member

Reference Issues/PRs

Simpler version of #26440

What does this implement/fix? Explain your changes.

This PR implements lazy loading at the top level. With this PR, it enables users to write code like this:

import sklearn as sk

pipe = sk.pipeline.Pipeline(...)

Copy link

github-actions bot commented Sep 6, 2024

✔️ Linting Passed

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

Generated for commit: 3d07932. Link to the linter CI: here

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

I think the added complexity/magic is minimal and the ability to do a single import sklearn line in an IPython session followed by code composing classes looked from different modules by qualnames is quite convenient.

So +1 on my side.

I am not sure about how to describe this in the changelog entry, but maybe this is better to introduce this with an example in the release highlights.

Before merging this PR, we might want to get more feedback than usual though, either by more reviews / votes or by validating it at the next monthly dev meeting.

EDIT: __getattr__ magic (even if quite straightforward in this case) and introducing multiple usage/import styles for sklearn make me ponder the above. So I am +0 instead of +1.

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. I like that now ipython can auto complete sklearn submodules.

Comment on lines 38 to 39
- |Enhancement| Scikit-learn classes and functions can be imported from the root
`sklearn` module. :pr:`29793` by `Thomas Fan`_.
Copy link
Member

Choose a reason for hiding this comment

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

I find the formulation a bit misleading. One can think that he can now do from sklearn import LogisticRegression.

@betatim
Copy link
Member

betatim commented Sep 30, 2024

For me a key was to think about this PR as enabling a new/different style of Python, and not so much "lazy loading to make imports faster".

I'm undecided if I like adding the "new style" or not. Not so much because I like/dislike the technical implementation but more because it adds a different way of doing the same thing. And on top of that I don't experience the problem that this solves.

@ogrisel
Copy link
Member

ogrisel commented Sep 30, 2024

I don't experience the problem that this solves.

When working in interactive IPython sessions, it often takes as many import statements as useful code lines to reproduce a toy experiment interactively to prove a point during an educational live coding session or craft a minimal reproducer.

@glemaitre
Copy link
Member

Actually I like the point raised by @ogrisel. This is indeed something that I experienced.

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.

Other than the changelog, LGTM.

@@ -35,6 +35,9 @@ Changes impacting many modules
More details in :ref:`estimator_tags`.
:pr:`22606` by `Thomas Fan`_ and :pr:`29677` by `Adrin Jalali`_.

- |Enhancement| Scikit-learn classes and functions can be used while only having a
Copy link
Member

@glemaitre glemaitre Oct 17, 2024

Choose a reason for hiding this comment

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

You will need to add a fragment in doc/whats_new/upcoming_changes/many-modules/29793.enhancement.rst and have the following entry:

- Scikit-learn classes and functions can be used while only having a
  `import sklearn` import line. For example, `import sklearn; sklearn.svm.SVC()` now works.
  By :user:`Thomas Fan <thomasjpfan>`

@adrinjalali adrinjalali merged commit fd07977 into scikit-learn:main Oct 21, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants