-
Notifications
You must be signed in to change notification settings - Fork 614
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 imports. #2829
Fix Keras imports. #2829
Conversation
You are owner of some files modified in this pull request. |
CC @bhack |
try: | ||
# New versions of Keras require importing from `keras.src` when | ||
# importing internal symbols. | ||
from keras.src import backend |
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.
Do we need this line? Shouldn't from keras import backend
work since its public API?
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.
Below, on line 144, backend._current_graph()
is called. Since this is an internal symbol, it requires the keras.src
import. from keras import backend
will only allow public symbols to be accessed on backend
, while from keras.src import backend
allows all symbols from backend.py
to be accessed.
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 details. Sounds good.
Are you using try/except as it is faster instead of checking the version? |
No, I use try-except because the issue only affects recent |
Is this exported? |
I think, but that version only refers to the stable version. The try-except will work on any given commit of Keras, which allows any version of |
Right, It is that keras-nightly is a quite ambiguous concept. See my old point (2021) at: I am ok with both. I am only worried that in the future users will be surprised by the wrong fallback import if the env is broken. /cc @seanpmorgan what do you think? |
@bhack I switched to checking The issue is causing various things to break, such as the official models, so it would be great if this could get merged quickly. Since @seanpmorgan hasn't responded yet, can you review this without @seanpmorgan's approval? |
No problem, I am re-running two failed jobs, then I'll merge it. |
Honestly I think no CI on Keras, TF Add-ons or TF is testing this so for me it is ok as I will not claim any compatibility without a CI coverage. |
Thanks @bhack! Agreed, without CI coverage we cannot make any compatibility guarantees. |
Description
Brief Description of the PR:
Recently, Keras changed how you must import internal symbols. Now, you must import private symbols from
keras.src
instead ofkeras
, e.g. you must usefrom keras.src.utils import tf_utils
instead offrom keras.utils import tf_utils
. See slide 12 of the April Keras community meeting slides for details Because old versions of Keras, including the current stable version, require the old import form, this PR changes every file importing internal Keras symbols to try both import forms: first it tries importing the new version (withkeras.src
), and if that fails, it tries importing the old version (withkeras
)Without this change, TF Addons cannot be imported with the latest
keras-nightly
pip package.Fixes # (issue)
Type of change
Checklist:
How Has This Been Tested?
If you're adding a bugfix or new feature please describe the tests that you ran to verify your changes: