[python-package] require scikit-learn>=0.24.2, make scikit-learn estimators compatible with scikit-learn>=1.6.0dev

[vnherdeiro]

Fixes #6653

(edit: taken over by @jameslamb, description re-written below)

• raises minimum supported version to scikit-learn>=0.24.2
• implements __sklearn_tags__() (replacement for _more_tags()) for scikit-learn estimators
• starts using sklearn.utils.validation.validate_data() in fit() and predict()
• adds tests confirming that scikit-learn estimators reject inputs with the wrong number of features

Notes for Reviewers

see https://scikit-learn.org/dev/whats_new/v1.6.html and scikit-learn/scikit-learn#29677

[vnherdeiro]

Update:

The change introduced in scikit-learn/scikit-learn#29677 makes it hard to subclass a sklearn estimator in a codebase while being compatible with sklearn < 1.6.0 and sklearn >= 1.6.0. Essentially the former looks up ._more_tags() and ignore __sklearn_tags__() while the former looks up __sklearn_tags__() and forbids existence of a
._more_tags() tags method.

The issue is discussed here:
scikit-learn/scikit-learn#29801

and it looks like a relaxation of the impossibility of having both ._more_tags() and __sklearn_tags__() simulatenously will be relaxed. If it goes through let's park this MR until lightgbm decides to force a scikit-learn>=1.6.0 dependency.

[adrinjalali]

@vnherdeiro note that it's possible already to support both with this method (scikit-learn/scikit-learn#29677 (comment)), however, the version check and @available_if are going to be unnecessary once we merge scikit-learn/scikit-learn#29801

[vnherdeiro]

Correct I am waiting for that PR to go in to bring back _more_tags Using @available_if would require another sklearn import and make the code less readable I reckon

…

On Thu, 12 Sept 2024, 11:34 am Adrin Jalali, ***@***.***> wrote: @vnherdeiro <https://github.com/vnherdeiro> note that it's possible already to support both with this method (scikit-learn/scikit-learn#29677 (comment) <scikit-learn/scikit-learn#29677 (comment)>), however, the version check and @available_if are going to be unnecessary once we merge scikit-learn/scikit-learn#29801 <scikit-learn/scikit-learn#29801> — Reply to this email directly, view it on GitHub <#6651 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE4CNVUURU6AMLDYUXKPFTTZWFU2TAVCNFSM6AAAAABOAVNTLSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBVHA4DCOJWGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jameslamb]

Thanks for starting on this @vnherdeiro . I've documented it in an issue: #6653 (and added that to the PR description).

Note there that I intentionally put the exact errors messages in plain text instead of just referring to _more_tags() ... that helps people to find this work from search engines.

Note also that the _more_tags() thing is only 1 of 3 breaking changes in scikit-learn that lightgbm will have to adjust to to get those tests passing again with scikit-learn==1.6.0.

[jameslamb]

Thanks for starting on this! Please see scikit-learn/scikit-learn#29801 (comment):

The story becomes "If you want to support multiple scikit-learn versions, define both."

I think we should leave _more_tags() untouched and add __sklearn_tags__(). And have self.__sklearn_tags__() call self._more_tags() to get its data, so we don't define things like _xfail_checks twice.

Do you have time to do that in the next few days? We need to fix this to unblock CI here, so if you don't have time to fix it this week please let me know and I will work on this.

[vnherdeiro]

@jameslamb Have just pushe a sklearn_tags trying a conversion from _more_tags. I added a out of current argument scope warning to catch a change from the arguments in _more_tags (they don't seem to change much though).

[adrinjalali]

Not a maintainer here, but coming from sklearn side. Leaving thoughts hoping it'd help.

[jameslamb]

Thanks for this.

I've reviewed the dataclasses at https://github.com/scikit-learn/scikit-learn/blob/e2ee93156bd3692722a39130c011eea313628690/sklearn/utils/_tags.py and agree with the choices you've made about how to map the dictionary-formatted values from _more_tags() to the dataclass attributes scikit-learn now prefers.

Please see the other comments about simplifying this.

[jameslamb]

Ok, this is ready for another review!

But understand if reviewers would like to wait until CI is fixed first before reviewing (#6663).

[trivialfis]

The change looks great! Thank you for the heads-up

[StrikerRUS]

Very impressive work!
I left some minor comments below:

[StrikerRUS]

_num_features() was added in 0.24 version:
scikit-learn/scikit-learn@b4d5ad6
I think we can move this into compat.py and try to import _num_features() firstly, then in case of ImportError emulate it with this function.

This approach will benefit from auto upstream updates of _num_features() in future versions.

[jameslamb]

Very good suggestion, thanks! I attempted this and found that it exposed some other complexity, which I've tried to describe in code comments and these inline GitHub comments:

• https://github.com/microsoft/LightGBM/pull/6651/files#r1788923387
• https://github.com/microsoft/LightGBM/pull/6651/files#r1788920665

To simplify the implementation a bit, I'm now also proposing:

• calling validate_data(reset=True), which will internally call _num_features() on scikit-learn>=1.6
• directly and unconditionally importing _num_features() and calling it in the pre-1.6 validate_data() implemented in compat.py (so no separate implementation to maintain!)
• raising lightgbm's scikit-learn floor to >=0.24.2 so users will always have a version at runtime with _num_features() defined

The new floor on scikit-learn>=0.24.2 should not impact users much. That version was released in April 2021 and did not have wheels for Python versions newer than 3.9 (PyPi release page), so I think it's unlikely many people will try to be using the next release of lightgbm with such an old version of scikit-learn.

But this is the first time we've had a floor on that dependency, so for awareness: cc @borchero @jmoralez @guolinke @shiyu1994

[StrikerRUS]

Sounds reasonable for me!

[StrikerRUS]

I think we can safely import this in compat.py.

__version__ was in __init__.py at least in 2011 year:
https://github.com/scikit-learn/scikit-learn/blob/dacdd3ad7b455a46b5e344ecfeaf5a369b554860/sklearn/__init__.py#L50

[jameslamb]

I was originally thinking that it'd be good for users to not incur the cost of this import when it's only needed in an error message... but I guess since it's a top-level attribute of sklearn, it will already have been imported anyway by the time any other sklearn imports have run.

I've moved this to compat.py, thanks for the suggestion.

[jameslamb]

If you pass reset=True to sklearn.utils.validation.validate_data(), it will try to:

• set estimator.n_features_in_ (code link)
• delete estimator.feature_names_in_ (code link)

We want the "set estimator.n_features_in_" behavior, because without it we have to manually set estimator.n_features_in_ in fit().

Doing that requires determining the number of features in X, which requires either re-implementing something like sklearn.utils.validation._num_features() (as I originally tried to do) or just calling that function directly. But that function can't safely be called directly before calling check_array(), because it raises a TypeError on 1-D inputs, which violates the check_fit1d estimator check (code link).

So here, I'm proposing that we do the following:

• add a setter for n_features_in_ and a deleter for feature_names_in_
• pass reset=True at fit() time to validate_data()
• modify the pre-1.6 implementation of validate_data() in compat.py to match

[jameslamb]

@StrikerRUS your comments were definitely not "minor", they really helped a lot! I've re-thought a lot of this PR based on trying to implement those suggestions.

This is ready for another review. Thank you for all your reviewing effort here, I know this change has become quite complex and there are many competing constraints it's trying to satisfy.

[StrikerRUS]

Hope this time my review comments will be really minor 😄

[StrikerRUS]

Adds ensure_min_samples=ensure_min_samples,

Suggested change

	X = check_array(X, accept_sparse=accept_sparse, force_all_finite=ensure_all_finite)
	X = check_array(
	X,
	accept_sparse=accept_sparse,
	force_all_finite=ensure_all_finite,
	ensure_min_samples=ensure_min_samples,
	)

[jameslamb]

I intentionally omitted ensure_min_samples. It's already not being passed in the one place it's used on master:

LightGBM/python-package/lightgbm/sklearn.py

Line 1007 in 0643230

X = _LGBMCheckArray(X, accept_sparse=True, force_all_finite=False)

This call to check_array() only happens in predict(), so I also think we should avoid any more validation than absolutely necessary to comply with the scikit-learn API, since applications calling predict() might care more about latency than those calling fit().

[StrikerRUS]

OK. Fine with me. However, I think you should be aware that the default argument is 1, not None

[jameslamb]

I did not realize that! I'm glad you mentioned it, just checked and it looks like omitting this argument from the call still results in that validation being performed.

    if ensure_min_samples > 0:
        n_samples = _num_samples(array)
        if n_samples < ensure_min_samples:
            raise ValueError(
                "Found array with %d sample(s) (shape=%s) while a"
                " minimum of %d is required%s."
                % (n_samples, array.shape, ensure_min_samples, context)
            )

https://github.com/scikit-learn/scikit-learn/blob/be52df50f1e9e9a6546248ccd7160a0a289f482c/sklearn/utils/validation.py#L1125-L1132

If that's the case, then my point about avoiding the overhead at predict() time doesn't matter... we're getting that overhead anyway. I guess the scikit-learn interface is probably not what you'd choose for low-latency predictions anyway... I will change this to pass through ensure_min_samples and set that to 1 in the call in sklearn.py, to make it explicit.

[jameslamb]

I just made this change in 8ef1deb. Now ensure_min_samples=1 will be passed at predict() time.

Thanks for the suggestion and talking through it with me.

[StrikerRUS]

LGTM!

That was really challenging!

@vnherdeiro Thank you for starting the work!

@adrinjalali Thanks for your help here!

@jameslamb Thanks a ton for the huge work done here!

[jameslamb]

Thanks everyone for the help, and especially @StrikerRUS for a thorough review of a very complex change!

[vnherdeiro]

Thanks for all the work @jameslamb Feeling glad this went in!