-
Notifications
You must be signed in to change notification settings - Fork 30.3k
Improved keras imports #24448
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
Improved keras imports #24448
Conversation
The documentation is not available anymore as the PR was closed or merged. |
It's tensor-flow ... that's probably the reason. |
I tested this with a spread of TF versions going back to 2.6. Even at 2.6, it was pretty hard to get an environment that worked with modern In all versions I tested with this patch, our test suite runs well and the issue identified in #24437 is fixed, so this should be ready to go after it's reviewed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the cleanup. I will agree to not put a maximum pin on TensorFlow when they stop making breaking changes in minor releases (so never 😅 )
setup.py
Outdated
"tensorflow-cpu>=2.4,<2.13", | ||
"tensorflow>=2.4,<2.13", | ||
"tensorflow-text<2.13", | ||
"tensorflow-cpu>=2.6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No let's leave a maximum pin. I do not want any surprises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deal, but I'm bumping the cap to <2.14
, since 2.13 works fine!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
A sneaky bug was hiding in our Keras imports, where an import for
call_context
appeared to succeed on some TF versions, but actually got an older, unusable version of the function. This causedbuild()
to behave improperly in some cases.I went on a quest to fix this, and generally clean up our version-specific imports for TensorFlow to stop things like this from happening in future. I also bumped the minimum version for TF to 2.6 (2.6 should be 2 years old by the time of our next release), and eliminated the version cap in our dependency table because TF 2.13 should also be fully supported now.
This involved a partial rewrite of some code, where we checked for
KerasTensor
in a lot of places. However, this is an internal class for Keras and they keep moving it around, so trying to import it feels like a bad idea. Instead, I'm relying ontf.is_tensor()
, which returnsTrue
for anything tensor-y, including symbolic tensors and KerasInput
placeholders.Fixes #24437