-
Notifications
You must be signed in to change notification settings - Fork 254
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
Fix Keras 3 version check #1328
Conversation
keras_nlp/backend/config.py
Outdated
@@ -65,7 +65,18 @@ | |||
|
|||
def detect_if_tensorflow_uses_keras_3(): | |||
# We follow the version of keras that tensorflow is configured to use. | |||
from tensorflow import keras | |||
import keras |
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.
I am not sure this actually keep any use case working, so might not be worth the extra complexity. tensorflow-text
will use tf.keras
, so if tf.keras
is broken, our preprocessing is broken.
I would somewhat consider this an expected fail state, and maybe instead do something like
try:
from tensorflow import keras
except:
raise ValueError(
"Unable to import `keras` with `tensorflow`. Please check your Keras and "
"Tensorflow version are compatible; Keras 3 requires TensorFlow 2.15 or later. "
"See keras.io/getting_started for more information on installing Keras."
)
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.
Addressed it. Can you check if the updates make sense?
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.
Looks fine!
I think we could also just keep the try/catch
logic I posted above for simplicity. No version parsing. Might catch other weird edge cases we haven't thought of and still give the helpful link to keras.io. But ok with this too. Is there a reason we are avoiding trying from tensorflow import keras
here?
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.
Makes sense. Reverted back to try/except
with from tensorflow import keras
keras_nlp/backend/config.py
Outdated
@@ -65,7 +65,18 @@ | |||
|
|||
def detect_if_tensorflow_uses_keras_3(): | |||
# We follow the version of keras that tensorflow is configured to use. | |||
from tensorflow import keras | |||
import keras |
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.
Looks fine!
I think we could also just keep the try/catch
logic I posted above for simplicity. No version parsing. Might catch other weird edge cases we haven't thought of and still give the helpful link to keras.io. But ok with this too. Is there a reason we are avoiding trying from tensorflow import keras
here?
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.
lgtm! thank you
* Fix Keras 3 version check * Fix Keras 3 version check * Update version check * Raise error if Keras is not compatible with TF
* Fix Keras 3 version check * Fix Keras 3 version check * Update version check * Raise error if Keras is not compatible with TF
* Fix Keras 3 version check * Fix Keras 3 version check * Update version check * Raise error if Keras is not compatible with TF
keras-nlp
has a function to check if TensorFlow uses Keras 3 via -https://github.com/keras-team/keras-nlp/blob/master/keras_nlp/backend/config.py#L66
This works when using
tf-nightly
orTensorFlow 2.15
, but breaks onTensorFlow 2.14
which is the default in Colab. To fix it, useimport keras
instead offrom tensorflow import keras
when doing this check.Colab to repro the issue:
https://colab.research.google.com/drive/17AwBX5S29d-hjUDPLN8roHSNzrjXricG?usp=sharing