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

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

Open
adrinjalali opened this issue Oct 23, 2024 · 21 comments
Open

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

adrinjalali opened this issue Oct 23, 2024 · 21 comments

Comments

@adrinjalali
Copy link

Looking at the jobs run everyday, it seems the last time tests passed was about 6 months ago, after which, there hasn't been any activity on the repo.

I was wondering if the repo is still active and y'all intend to maintain it. I'm also not sure if keras intends to put back these wrappers now that it's a standalone package.

@adriangb
Copy link
Owner

adriangb commented Oct 23, 2024

The issues are that I don't really use TensorFlow anymore and the TensorFlow team has historically made breaking changes in minor releases.

I've been giving this some thought myself and would really love for someone else who is actively using this software to take over maintenance of it.

@adrinjalali
Copy link
Author

Those are very fair points. I'll wait and see what the Keras team says, and I'll raise this internally with our team to see if we'd have capacity to help.

@clstaudt
Copy link

clstaudt commented Nov 3, 2024

I have found the scikeras wrappers very useful in my ML work, which regularly combines deep learning and "shallow learning" ("classical ML") models. I was surprised that the tensorflow project has removed them, happy to find them again in scikeras, and I'd be happy to help with keeping this project maintained.

@adriangb
Copy link
Owner

adriangb commented Nov 3, 2024

@clstaudt amazing thank you! maybe a good place to start would be to get nightly CI back to passing?

@clstaudt
Copy link

clstaudt commented Nov 3, 2024

@adriangb Here's a start - please review
#330

@adrinjalali
Copy link
Author

Note that in keras-team/keras#20399 folks agreed to have these wrappers in keras itself, and I was working on cleaning up the code from here to port it there. Would we rather do that and maintain the code there (which means we'd have to support a single version of keras then), or fix the issues here?

@clstaudt
Copy link

clstaudt commented Nov 4, 2024

@adrinjalali Why would they first remove them and then want them back in keras? Am I missing something or is this just regular chaos?

I think it makes sense that the keras project maintains a scikit-learn compatibility layer.

@adrinjalali
Copy link
Author

Keras used to be an independent project, providing a nice API layer on top of TF and Caffe (if I'm not mistaken). Then it was adopted by and moved inside TF by google, and became a TF thingy.

Now TF is moving things out of the TF repo I suppose since it's become a VERY large project and it makes sense to have different modules in their own repos. Keras is also supposed to now support TF, PyTorch, and Jax.

So I can imagine the vibe in keras is now very different and understand that decisions are made differently. I'm personally happy about this development.

@clstaudt
Copy link

clstaudt commented Nov 4, 2024

@adrinjalali Since you have agreed to work on the migration back to keras: Thanks, and feel free to delegate some of the easier tasks if you need help.

@adriangb
Copy link
Owner

adriangb commented Nov 4, 2024

I think it would be best if this moved back into Keras, as long as they're willing to accept it. Thank you @adrinjalali for getting the ball rolling on that!

@adriangb
Copy link
Owner

adriangb commented Nov 4, 2024

@clstaudt I do think your PR is still worth pursuing. It will be easier to port this to Keras if we know things are working and passing with the latest version of Keras, which will be easier to do here if nothing else because CI is faster, etc.

@clstaudt
Copy link

clstaudt commented Nov 5, 2024

@adriangb I agree, but after trying to fix some of the failing tests I realize that I am not able to complete this independently. I could suggest incremental fixes and work with you through review, but that requires your time as well. Perhaps more than you would need to fix the couple of errors yourself, knowing the code base.

@adriangb
Copy link
Owner

adriangb commented Nov 5, 2024

okay no problem 😄, I'll see if I have time to pick it up in the next couple weeks otherwise we'll see how the conversation goes on the Keras side.

@adrinjalali
Copy link
Author

PR on the keras side: keras-team/keras#20599

@adrinjalali
Copy link
Author

Since keras-team/keras#20599 is now merged, we might want to slowly refer people to there in this repo?

@adriangb
Copy link
Owner

Yes agreed! Do you have any suggestions on how to do that? I propose:

  • Put a warning on import that explains that this package will no longer be maintained
  • Put a warning in the README and docs that explains that this package will no longer be maintained
  • Mark the repository as archived

@adrinjalali
Copy link
Author

That sounds like a perfect plan to me. I'm only not sure if we should wait till the next Keras release to have the wrappers available to users?

@adriangb
Copy link
Owner

Sure no harm in waiting 😄 at least to release a version with the warning. I've set a reminder for 2 weeks from now (taking a guess at it based on release cadence) but feel free to remind me. Nothing stopping us from preparing the PR to merge here though.

@adriangb
Copy link
Owner

#334

@clstaudt
Copy link

Thanks @adrinjalali - any idea about when this will be in a keras relase?

@adrinjalali
Copy link
Author

looking at the cadence, probably early Jan. But I'm not sure since I'm not a keras maintainer.

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

No branches or pull requests

3 participants