Thanks for your effort @adriangb ! I'm sorry I couldn't write back before.

Of course, I don't have any problem with you updating the other PR, but I'm not sure if I have to do something to allow you to edit it. I've added you as a collaborator to my tf fork.

In any case, this is a nice improvement for tf.keras in my opinion, but I don't know if the development team is interested. My PR has been open for a while and the same changes were proposed in the keras repo about a year ago now.

I'm hoping that maybe the reviewers were just a bit busy?

I feel like the wrappers are used quite often, especially in beginner tutorials. I think it's important that the initial experience be seamless.

In addition to your PR, this resolves several issues: #33204, #36074, #34689 and #36137. There are/will be more, like the comment I posted on your PR.

I'm hoping we can at least get some feedback from @fchollet or @pavithrasv regarding interest in these changes.

I was able to add built-in support for all of the multi-output modes that Scikit-Learn supports, as well as a framework to easily support multi-input models. This means this would close #34689 as well.

Because of the great modularity of the Functional API, only a limited number of multi-output cases can be automatically supported (those that scikit-learn itself supports with 1-1 mapping of model outputs to y columns) and no multi-input models can be supported out of the box. If a user wants to use a more complex model with the wrappers, they would have to subclass and manually define the desired behavior. As I show in the tests, all that is needed to implement multi-input is to define a behvior for _pre_process_X (for example, columns 0-3 are one input, 4 is another input). All of this was done with no changes to the API.

Although the original problem statement ('fix compatibility') was quite large, I do feel that this PR has grown very large. I would personally prefer to split it into smaller PRs (even if that means more work for me), but I will leave that up to the reviewers.

@gbaned , is there a timeline for this review process? It would be nice to at least get tests running so that I can see if there are issues.

I realized that we actually need to re-implement not only an R^2 score, but also a classifier accuracy score: Keras does not use the sample_weight parameter for metrics, but Scikit-Learn does use it for scoring. That and the fact that Keras metrics don't support the Scikit-Learn style multi-output concept, so all of those scores are already being handled manually.

This prompted me to think: does it make sense to make scikit-learn an optional dependency that is only imported within this module? I can see pros and cons, just playing devil's advocate here.

Approving to run the presubmits.

Can you add tests to make sure that saving and loading works with the wrapper?

Thank you for kicking off those tests!

There are several tests for pickling/unpickling of Functional API models with and without Callbacks, etc. I think the only thing that is missing is a test for subclassed models. I'll add that in the next few days.

Quite a few errors:

• Pylint: my fault, I was using 4 spaces. Fixing.
• Scipy import error: for some CI builds, the scikit-learn version was pinned to a 2016 version that now has some broken functionality that these new changes were relying on. I bumped the version.
• Python 2 errors: I'm ignoring these.
• API changes: it's complaining about changes to internal parameters that should never have been public in the first place. I now appended an _ to them to fix this for the future. It also doesn't like that I capitalized X to match scikit-learn.
• Windows builds: I have no idea why these are failing. Any advice would be much appreciated.

Also, I added the test suggested by @k-w-w (it's called SerializeCustomLayers fyi).

I guess another approval is needed for tests to run again, it'd be nice to fix the windows build errors before that though.

Thanks for fixing the bugs! I'm pretty sure the windows tests are unrelated. Running the tests again

@fchollet, thank you for looking at this!

I think that could work really well. But I'd like to list out pros and cons to consider

Pros:

• More flexibility. scikit-learn can become required for the wrapper, which eliminates a lot of code duplication for this wrapper and removes the need for scikit-learn from TF CI.
• Relieves maintenance burden from Keras/TF team.

Cons:

• Improvements in Keras might take a long time to trickle over.
• When issues do crop up in the future, wrapper maintainers (i.e. me) might have to open an issue with Keras/TF to get support in fixing things.

Overall, I think the pros outweigh the cons.

I will start working on getting a separate repo with CI/publishing working. In the meantime, if you could do a brief review of this PR as it currently stands, that would be super useful to make sure the initial release is as good as possible.

Are there any preferences as far as:

• Naming of the repo/package or any project this should live under. I came up with sklearn-keras-wrap
• Documentation style or content. I planned on copying the existing docs but just keeping everying in the README.md for simplicity.
• Python versions: I think it would be easier to make this package >=Python3.5
• Linting: I would switch to flake8/black
• Testing: I would switch to pytest.

I played around with a packaging a bit, everything seems to work as far as CI/testing/releasing. It would have scikit-learn>=0.21.0 and tensorflow>=2.1.0 as dependencies.

A quick update: the package is now fully operational. I settled on the name SciKeras.
PyPi: https://pypi.org/project/scikeras/
GitHub: https://github.com/adriangb/scikeras

Some important updates since this PR:

1. Inherited BaseWrapper from sklearn.base.BaseEstimator.
2. Implemented tags interface.
3. Use OneHotEncoder/LabelEncoder instead of manual numpy work.
4. Fix model serialization bugs.
5. A lot of cleanup.

With all of this, estimators created with these wrappers now pass all of scikit-learn's estimator checks, except those that require setting a random state. As far as I understand, it is not possible to easily set a random seed in tf.

Hi @gbaned, just checking if there are any updates on this proposal/PR? Thanks!

Hi @gbaned, just checking if there are any updates on this proposal/PR? Thanks!

Hi @adriangb, It is waiting for approval. Thanks!

Thanks for the update. It's great to see that you've already released the new package. We can recommend that people start using it instead of keras.wrappers.scikit_learn.

With all of this, estimators created with these wrappers now pass all of scikit-learn's estimator checks, except those that require setting a random state. As far as I understand, it is not possible to easily set a random seed in tf.

This should be fixable: https://www.tensorflow.org/api_docs/python/tf/random/set_seed

What do you want us to do with the current PR? Should we close it?

We can recommend that people start using it instead of keras.wrappers.scikit_learn.

That sounds good.

This should be fixable: https://www.tensorflow.org/api_docs/python/tf/random/set_seed

Will take a look, thank you.

What do you want us to do with the current PR? Should we close it?

I think let's keep it open for a bit longer. Dask is looking to adopt SciKeras as a wrapper (here), so as they do their testing I expect there to be a couple of issues that crop up in the next couple of weeks that I may need input from the TF team on. Unless the TF team is willing to check the SciKeras repo if they are tagged.

I think let's keep it open for a bit longer. Dask is looking to adopt SciKeras as a wrapper (here), so as they do their testing I expect there to be a couple of issues that crop up in the next couple of weeks that I may need input from the TF team on. Unless the TF team is willing to check the SciKeras repo if they are tagged.

Ok, sounds good! Please reach out if you need anything from us (over email preferably, so we don't miss it). We'll start recommending your library as soon as it starts getting traction then 👍

@adriangb Any update on this PR? Please. Thanks!

@adriangb Any update on this PR? Please. Thanks!

Hi @gbaned, as per François' comment above, the plan is to not merge this PR and instead move this part of tf.keras to an external package. I had asked to keep this PR open for communication and help with bringup of the external package, but I've since established communication with François directly and the external package is making good progress, so I think we can close this PR 😄