search

tensorflow / addons

Useful extra functionality for TensorFlow 2.x maintained by SIG-addons
Apache License 2.0
1.69k stars 610 forks source link

Fix Keras imports. #2829

Closed reedwm closed 1 year ago

reedwm commented 1 year ago

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 of keras, e.g. you must use from keras.src.utils import tf_utils instead of from 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 (with keras.src), and if that fails, it tries importing the old version (with keras)

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:

bot-of-gabrieldemarmiesse commented 1 year ago

@hyang0129

You are owner of some files modified in this pull request. Would you kindly review the changes whenever you have the time to? Thank you very much.

reedwm commented 1 year ago

CC @bhack

bhack commented 1 year ago

Are you using try/except as it is faster instead of checking the version?

reedwm commented 1 year ago

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 keras-nightly versions, and users with older keras-nightly versions need the old import. I don't think there's an easy way to check if the keras-nightly commit is newer than a certain commit.

bhack commented 1 year ago

No, I use try-except because the issue only affects recent keras-nightly versions, and users with older keras-nightly versions need the old import. I don't think there's an easy way to check if the keras-nightly commit is newer than a certain commit.

Is this exported? https://github.com/keras-team/keras/blob/master/keras/__init__.py#L31-L33

reedwm commented 1 year ago

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 keras-nightly to work. This is a fairly minor use case, so I can switch to a version check if you prefer.

bhack commented 1 year ago

Right, It is that keras-nightly is a quite ambiguous concept.

See my old point (2021) at:

https://github.com/keras-team/tf-keras/issues/86

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?

reedwm commented 1 year ago

@bhack I switched to checking tf.__version__ instead of using a try-except statement. I'm happy to switch it back if you prefer the try-except statement. The tf.__version__ check means older version of keras-nightly will not be supported, but this is fine as users should expect to have to update the nightly TF and Keras packages if they also update TF-Addons.

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?

bhack commented 1 year ago

No problem, I am re-running two failed jobs, then I'll merge it.

bhack commented 1 year ago

....but this is fine as users should expect to have to update the nightly TF and Keras packages if they also update TF-Addons.

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.

reedwm commented 1 year ago

Thanks @bhack! Agreed, without CI coverage we cannot make any compatibility guarantees.