-
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
Tensorflow 2.11 support #1016
Tensorflow 2.11 support #1016
Conversation
Documentation preview |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@pytest.mark.skipif( | ||
version.parse(tf.__version__) < version.parse("2.9.0"), | ||
reason="tf.keras.optimizers.legacy is not available in TF <= 2.8", | ||
) |
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.
Since we need to use legacy
optimizers in MultiOptimizer, I updated the incremental training notebook to use tf.keras.optimizers.legacy
optimizers. And since this namespace was not available in TF 2.8, I opted to skip testing for TF 2.8, effectively dropping support for TF 2.8 for this notebook. Open questions: Is there a better way to handle this? It's likely not feasible to support all previous versions forever. Do we need an SLA or something for when to drop support for old dependency versions?
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.
Maybe the last 4 releases of TensorFlow or something would be reasonable. Might depend a bit on the level of complexity in supporting each one
elif hasattr(optimizer, "variables") and callable( | ||
optimizer.variables | ||
): # Tensorflow >= 2.11 | ||
weights += optimizer_blocks.optimizer.variables() |
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.
if variables()
was present in earlier versions of tensorflow (2.8, 2.9, 2.10) and equivalent to .weights
? then we might be able to simplify to be only this line?
Goals ⚽
Support Tensorflow 2.11
Implementation Details 🚧
tf.keras.optimizers.legacy
. Most of the changes in this PR involve checking the Tensorflow version and using the correct namespace for the version.MultiOptimizer
only works with legacy optimizers, and it does not work with newly introduced Keras optimizers. This PR opted to throw an error if the user tries to use it with the new optimizers. Supporting new optimizers in MultiOptimizer is left as future work.Testing Details 🔍
Needs horovod 0.27. Related: NVIDIA-Merlin/Merlin#883
For GPU test, manually built docker image with NVIDIA-Merlin/Merlin#883 and ran tests.