-
Notifications
You must be signed in to change notification settings - Fork 50
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
remove tensorflow dependency #221
Comments
Hmm, there's basically no description on the PyPi package (other than some generic |
@adriangb It is also maintained by Google and has the same api with tensorflow package. It doesnt include the compiled cuda binaries so that it takes less space and easier to deploy on machines without gpu. |
It is especially useful when deploying a ML app to Heroku: https://stackoverflow.com/a/62356779 |
Interesting, I sure wish the TensorFlow team had better descriptions. Also it would have been nice if they packaged it as an extra like The main issue with just removing the dependency is that we actually do have minimum TensorFlow versions, and will probably bump them soon. Of course we can replace the requirement with a runtime check, but that has its own disadvantages. For the time being, it should be possible to install scikeras with |
That might be worth adding to the docs. |
Agreed, I'll add it in the install section. |
@adriangb Unfortunately its not an acceptable solution in my case. Im using this package in this web app project. Heroku creates the environment using this requirements.txt. And during Heroku deployment I cant specify to install scikeras with However since scikeras has tensorflow dependency, it downloads both tensorflow-cpu and tensorflow into the environment and fails the deployment due to limited storage. Better solution would be to make tensorflow dependency optional: if tensorflow-cpu>2.4.0 is present then dont install tensorflow, else require tensorflow>2.4.0 |
I see. It seems that then we are left with two options:
|
Would the second approach bring lots of maintenance cost for you? If not, it would be very useful for the ones that are in same situation with me. |
It would carry some maintenance burden, I'm not sure how much, but I'll look into it and at least scope it out. |
Would per-requirement overrides work? |
@stsievert I tried adding
|
Just as a sanity check, I tried installing |
Yes, indeed. That results in oversized heroku deployment and failure. Trying to find a way with per-requirement overrides but not successful at the moment. |
I think maybe a good next step is to open an issue in tensorflow to look for some guidance. Would you like to do that @fcakyon or would you prefer I do it? |
I think you could express the situation better then me as a package maintainer. Would you mind opening the issue? |
Sure, no problem, I'll tag you. |
@fcakyon I openend tensorflow/tensorflow#48165 |
It looks like there was already an issue in tensorflow for the same thing: tensorflow/tensorflow#7166 It seems like indeed the move is to make tensorflow an optional dependency. I think we're just going to have to bite the bullet on this one next major version bump @stsievert |
What exactly do you mean by that? What exactly is required for installation now? |
Sorry, I should have been clearer. Accept that we will need to make a backwards incompatible change that has its own set of downsides. |
Semantic versioning lays out the following:
So sounds like the minor version should be bumped from 0.2.1 to 0.3.0. |
Ugh sorry yes, you are right, spaghetti in my brain. I meant minor. So what you are saying is spot on. |
@adriangb thats great news! |
Yep! Just decided to bite the bullet on it... If you can test off of |
Due to the tensorflow dependency, this package is not usable with tensorflow-cpu package.
Can you please make tensorflow dependency optional so that I can use it with tensorflow-cpu?
The text was updated successfully, but these errors were encountered: