FEAT add scikit-learn wrappers #20599

adrinjalali · 2024-12-05T11:23:09Z

Fixes #20399

This adds a minimal wrapper under keras.wrappers. It delegates all model construction parameters to the function generating the model, and therefore doesn't require much __init__ params at all.

There are a lot of useful features under https://github.com/adriangb/scikeras which I haven't included here to make the review much easier. Happy to work on more features in this PR or after, if they're not covered.

As for the CI, would we want to test this in a separate job in actions.yml or do we want to include it in the build job? Also, should we test against multiple scikit-learn versions in the CI?

also cc @adriangb @clstaudt @fchollet

codecov-commenter · 2024-12-05T13:15:24Z

Codecov Report

Attention: Patch coverage is 76.63043% with 43 lines in your changes missing coverage. Please review.

Project coverage is 82.52%. Comparing base (90d36dc) to head (eb7a893).
Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
keras/src/wrappers/fixes.py	54.76%	18 Missing and 1 partial ⚠️
keras/src/wrappers/sklearn_wrapper.py	86.11%	12 Missing and 3 partials ⚠️
keras/src/wrappers/utils.py	71.42%	3 Missing and 3 partials ⚠️
keras/api/_tf_keras/keras/wrappers/__init__.py	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #20599      +/-   ##
==========================================
- Coverage   82.55%   82.52%   -0.03%     
==========================================
  Files         518      525       +7     
  Lines       48682    48948     +266     
  Branches     7592     7615      +23     
==========================================
+ Hits        40188    40393     +205     
- Misses       6669     6719      +50     
- Partials     1825     1836      +11

Flag	Coverage Δ
keras	`82.36% <76.63%> (-0.04%)`	⬇️
keras-jax	`65.69% <76.63%> (+0.08%)`	⬆️
keras-numpy	`60.66% <69.02%> (+0.07%)`	⬆️
keras-tensorflow	`66.52% <76.63%> (+0.04%)`	⬆️
keras-torch	`65.59% <76.63%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

adriangb

This is amazing! You've made some great improvements over the version in SciKeras.

I agree that the approach you're proposing here of not just copying over SciKeras or trying to reproduce all of the features out of the gate and instead build it up better as a new version over time.

fchollet

Thanks for the PR!

fchollet · 2024-12-06T17:06:52Z

keras/api/wrappers/__init__.py

+
+from keras.src.wrappers._sklearn import KerasClassifier
+from keras.src.wrappers._sklearn import KerasRegressor
+from keras.src.wrappers._sklearn import KerasTransformer


I have reservations about "KerasTransformer" due to the very specific meaning that Transformer has in deep learning. This is going to confuse the hell out of people who search "Transformer" on keras.io. This was also not part of the original implementation of sklearn wrappers we had in Keras.

Fair enough.

I've included this here since, in the meantime, using pre-trained models as a step in a pipeline to get embeddings or any other kind of transformation has become much more popular than a couple of years ago.

But I do understand the name is quite confusing. What do you think of naming these estimators SKLearn{Regressor, Classifier, Transformer}? That should make it clear that the space/scope of these estimators are scikit-learn. We can also add a clear note in the docstring that these have nothing to do with "transformers".

keras/src/wrappers/_sklearn.py

fchollet · 2024-12-06T17:07:55Z

keras/src/wrappers/_sklearn.py

+            deterministic state using this seed. Pass an int for reproducible
+            results across multiple function calls.
+
+    Attributes:


You can remove this

Sure, but how should I document public attributes which users can use to inspect the object after calling fit?

Why not plain model btw?

scikit-learn convention / API is that given estimator arguments are never changed and est.fit(X,y) is (ignoring randomness) the same as est.fit(X, y); est.fit(X, y). Object attributes with no trailing underscore are the ones given to __init__, and the ones set during fit are with a trailing underscore.

keras/src/wrappers/_sklearn.py

fchollet · 2024-12-06T17:09:03Z

keras/src/wrappers/_sklearn.py

+            arguments. Other arguments must be accepted if passed as
+            `model_args` by the user.
+
+        warm_start: bool, default=False


Please use the same arg description format as the rest of the codebase

Hope this is okay now:

warm_start: bool, defaults to False. Whether to reuse the model weights from the previous fit. If `True`, the given model won't be cloned and the weights from the previous fit will be reused.

keras/src/wrappers/_sklearn.py

fchollet · 2024-12-06T17:10:20Z

keras/src/wrappers/utils.py

+    For use in pipelines with transformers that only accept
+    2D inputs, like OneHotEncoder and OrdinalEncoder.
+
+    Attributes


Please use the same docstring format as in the rest of the codebase.

clstaudt · 2024-12-09T11:06:55Z

Thank you @adrinjalali. I use these wrappers all the time for bridging keras and scikit-learn. From my point of view they're an essential feature for keras.

adrinjalali

Thanks for the review. A couple of questions here.

adrinjalali · 2024-12-09T12:28:12Z

keras/api/wrappers/__init__.py

+
+from keras.src.wrappers._sklearn import KerasClassifier
+from keras.src.wrappers._sklearn import KerasRegressor
+from keras.src.wrappers._sklearn import KerasTransformer


Fair enough.

I've included this here since, in the meantime, using pre-trained models as a step in a pipeline to get embeddings or any other kind of transformation has become much more popular than a couple of years ago.

But I do understand the name is quite confusing. What do you think of naming these estimators SKLearn{Regressor, Classifier, Transformer}? That should make it clear that the space/scope of these estimators are scikit-learn. We can also add a clear note in the docstring that these have nothing to do with "transformers".

adrinjalali · 2024-12-09T12:33:25Z

keras/src/wrappers/_sklearn.py

+            deterministic state using this seed. Pass an int for reproducible
+            results across multiple function calls.
+
+    Attributes:


Sure, but how should I document public attributes which users can use to inspect the object after calling fit?

adrinjalali · 2024-12-09T12:34:09Z

keras/src/wrappers/_sklearn.py

+            arguments. Other arguments must be accepted if passed as
+            `model_args` by the user.
+
+        warm_start: bool, default=False


Hope this is okay now:

warm_start: bool, defaults to False. Whether to reuse the model weights from the previous fit. If `True`, the given model won't be cloned and the weights from the previous fit will be reused.

fchollet · 2024-12-09T20:09:36Z

But I do understand the name is quite confusing. What do you think of naming these estimators SKLearn{Regressor, Classifier, Transformer}? That should make it clear that the space/scope of these estimators are scikit-learn. We can also add a clear note in the docstring that these have nothing to do with "transformers".

Yes, let's do that. This is much better.

fchollet

Thanks for the update!

fchollet · 2024-12-10T17:49:01Z

keras/src/wrappers/_sklearn.py

+            deterministic state using this seed. Pass an int for reproducible
+            results across multiple function calls.
+
+    Attributes:


Why not plain model btw?

keras/src/wrappers/_sklearn.py

fchollet · 2024-12-10T18:00:08Z

keras/src/wrappers/sklearn_test.py

+
+EXPECTED_FAILED_CHECKS = {
+    "SKLearnClassifier": {
+        "check_classifiers_regression_target": ("not an issue in sklearn>=1.6"),


Why are strings wrapped in tuples?

it's not a tuple, here cause it used to be longer and needed to be in a new line 😅

fchollet · 2024-12-10T18:04:10Z

keras/src/wrappers/random_state.py

+import numpy as np
+
+try:
+    import tensorflow as tf


We should not unconditionally try to import tensorflow since that could cause issues with other backends

fchollet · 2024-12-10T18:04:55Z

keras/src/wrappers/random_state.py

+
+
+@contextmanager
+def tensorflow_random_state(seed: int) -> Generator[None, None, None]:


This should really not be necessary, instead just use set_random_seed from Keras. If users need TF_DETERMINISTIC_OPS that is something they should set manually, separately.

keras/src/wrappers/_sklearn.py

fchollet · 2024-12-10T18:06:23Z

keras/src/wrappers/_sklearn.py

+            values passed directly to the `fit` method take precedence over
+            these.
+        random_state : int, np.random.RandomState, or None, defaults to None.
+            Set the Tensorflow random number generators to a reproducible


This should work with all backends, not just TF. So no mention of TF and no special casing of TF in the code

fchollet · 2024-12-10T18:06:49Z

keras/src/wrappers/_sklearn.py

+            If callable, it must accept at least `X` and `y` as keyword
+            arguments. Other arguments must be accepted if passed as
+            `model_args` by the user.
+        warm_start: bool, defaults to False.


Use backticks around code keywords like False, True, None

fchollet · 2024-12-10T18:07:13Z

keras/src/wrappers/_sklearn.py

+            Whether to reuse the model weights from the previous fit. If `True`,
+            the given model won't be cloned and the weights from the previous
+            fit will be reused.
+        model_args: dict, defaults to None.


These are actually kwargs rather than args (args would be a tuple of values)

technically yes, but I'm not sure if model_kwargs is a better name here; renamed to model_kwargs

fchollet · 2024-12-10T18:08:00Z

keras/src/wrappers/_sklearn.py

+            directly to the `fit` method of the scikit-learn wrapper. The
+            values passed directly to the `fit` method take precedence over
+            these.
+        random_state : int, np.random.RandomState, or None, defaults to None.


Why not seed (which is the standard arg name for this in Keras)?

adrinjalali

Regarding randomness, I don't think it's a good idea to recommend keras.utils.set_random_seed since it has a global side effect. Setting randomness should ideally be local to the object. Right now with the current design of keras and TF it's not clear to me what the best recommendation is. But since this is a larger issue than this PR, I've removed random_seed and the docs now point to the example where users can control the randomness themselves.

keras/src/wrappers/_sklearn.py

adrinjalali · 2024-12-10T18:46:24Z

keras/src/wrappers/_sklearn.py

+            Whether to reuse the model weights from the previous fit. If `True`,
+            the given model won't be cloned and the weights from the previous
+            fit will be reused.
+        model_args: dict, defaults to None.


technically yes, but I'm not sure if model_kwargs is a better name here; renamed to model_kwargs

adrinjalali · 2024-12-10T18:50:39Z

keras/src/wrappers/_sklearn.py

+            deterministic state using this seed. Pass an int for reproducible
+            results across multiple function calls.
+
+    Attributes:


scikit-learn convention / API is that given estimator arguments are never changed and est.fit(X,y) is (ignoring randomness) the same as est.fit(X, y); est.fit(X, y). Object attributes with no trailing underscore are the ones given to __init__, and the ones set during fit are with a trailing underscore.

adrinjalali · 2024-12-10T19:37:49Z

keras/src/wrappers/sklearn_test.py

+
+EXPECTED_FAILED_CHECKS = {
+    "SKLearnClassifier": {
+        "check_classifiers_regression_target": ("not an issue in sklearn>=1.6"),


it's not a tuple, here cause it used to be longer and needed to be in a new line 😅

keras/src/wrappers/sklearn_wrapper.py

fchollet · 2024-12-11T18:39:44Z

keras/src/wrappers/fixes.py

+
+sklearn_version = parse_version(parse_version(sklearn.__version__).base_version)
+
+if sklearn_version < parse_version("1.6"):


If seems like it would be much easier and maintainable to simply require a minimum sklearn version?

it's not the worst. And I've included all version specific code in a single fixes.py so that we know later where to clean up. Also, 1.6 was just released a few days ago, so I don't think it's a good idea to have that as a minimum required version. WDYT?

Ok, that's fine

fchollet · 2024-12-12T17:57:49Z

keras/src/wrappers/utils.py

+    def inverse_transform(self, y):
+        """Revert the transformation of transform.
+
+        Parameters


Please update the docstrings in this file to use the standard format

should be fixed now.

fchollet

LGTM -- thank you for the neat contribution!

adrinjalali · 2024-12-12T20:48:25Z

Oh wow 🥳 thanks for the reviews. Happy to contribute docs, or more features, or be pinged on issues about this ❤️

glemaitre · 2024-12-12T20:54:04Z

Thanks @adrinjalali and everyone involved in this contribution.

adriangb · 2024-12-12T20:57:03Z

Amazing work folks!

I'll update SciKeras to point folks towards these wrappers.

Very happy to see this come full circle 🥳.

FEAT add scikit-learn wrappers

6178e35

google-ml-butler bot added the size:L label Dec 5, 2024

google-ml-butler bot assigned gbaned Dec 5, 2024

adrinjalali mentioned this pull request Dec 5, 2024

Maintainence status (tests failing for the past 6 months) adriangb/scikeras#329

Open

adrinjalali added 7 commits December 5, 2024 12:25

import cleanup

9a4b999

run black

e14ec2b

linters

77d0975

lint

5b28b90

add scikit-learn to requirements-common

e450bf1

generate public api

f73c5e2

fix tests for sklearn 1.5

c5aa0ea

adrinjalali added 2 commits December 5, 2024 16:23

check fixes

999c598

Merge remote-tracking branch 'upstream/master' into wrapper

5d9bd01

adriangb reviewed Dec 5, 2024

View reviewed changes

adrinjalali mentioned this pull request Dec 6, 2024

MNT Replace the Pytorch and Tensorflow test mocks with actual instances fairlearn/fairlearn#1417

Open

adrinjalali added 2 commits December 6, 2024 10:54

skip numpy tests

a91b475

xfail instead of skip

ab1d0ea

fchollet reviewed Dec 6, 2024

View reviewed changes

adrinjalali added 2 commits December 9, 2024 13:45

apply review comments

4f3d7ad

Merge remote-tracking branch 'upstream/master' into wrapper

425b940

adrinjalali commented Dec 9, 2024

View reviewed changes

adrinjalali added 4 commits December 10, 2024 17:14

change names to SKL* and add transformer example

7f9dd79

fix API and imports

9eebe2d

fix for new sklearn

8e784fa

sklearn1.6 test

4c26931

fchollet reviewed Dec 10, 2024

View reviewed changes

review comments and remove random_state

6289a8f

Merge remote-tracking branch 'upstream/master' into wrapper

a958d98

adrinjalali commented Dec 10, 2024

View reviewed changes

adrinjalali added 3 commits December 10, 2024 21:15

add another skipped test

3cad37f

rename file

70b44e8

change imports

f506f97

fchollet reviewed Dec 11, 2024

View reviewed changes

keras/src/wrappers/sklearn_wrapper.py Outdated Show resolved Hide resolved

fchollet reviewed Dec 11, 2024

View reviewed changes

adrinjalali added 2 commits December 12, 2024 09:55

unindent

a33501e

Merge remote-tracking branch 'upstream/master' into wrapper

be8835f

fchollet reviewed Dec 12, 2024

View reviewed changes

docstrings

eb7a893

fchollet approved these changes Dec 12, 2024

View reviewed changes

google-ml-butler bot added kokoro:force-run ready to pull Ready to be merged into the codebase labels Dec 12, 2024

fchollet merged commit 32a642d into keras-team:master Dec 12, 2024
6 checks passed

google-ml-butler bot removed ready to pull Ready to be merged into the codebase kokoro:force-run labels Dec 12, 2024

adrinjalali deleted the wrapper branch December 12, 2024 20:48

sampathweb mentioned this pull request Dec 13, 2024

Adds scikit-learn dependency to setup #20644

Merged



		@contextmanager
		def tensorflow_random_state(seed: int) -> Generator[None, None, None]:


		sklearn_version = parse_version(parse_version(sklearn.__version__).base_version)

		if sklearn_version < parse_version("1.6"):

FEAT add scikit-learn wrappers #20599

FEAT add scikit-learn wrappers #20599

Conversation

adrinjalali commented Dec 5, 2024

codecov-commenter commented Dec 5, 2024 • edited Loading

Codecov Report

adriangb left a comment

Choose a reason for hiding this comment

fchollet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clstaudt commented Dec 9, 2024

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fchollet commented Dec 9, 2024

fchollet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fchollet left a comment

Choose a reason for hiding this comment

adrinjalali commented Dec 12, 2024

glemaitre commented Dec 12, 2024

adriangb commented Dec 12, 2024

codecov-commenter commented Dec 5, 2024 •

edited

Loading