Skip to content
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

Feat: Upgrade tensorflow to 2.16.1 and keras to 3.4.0 #1385

Merged

Conversation

RollerKnobster
Copy link
Contributor

@RollerKnobster RollerKnobster commented Jun 26, 2024

To accommodate the upgrade, next changes to the code were made:

  • tensorflow no longer has keras wrappers, which are now used from scikeras library, recommended for use by them. They have largely the same interface, but slightly different innerworkings, which were accomodated for.
  • Since keras 3.* now clones the input regressor that is provided to the BaseEstimator into a regressor_ attribute, and ONLY updates the regressor_ one after fitting, metadata extraction in gordo.builder.build_model.ModelBuilder was changed to seek the regressor_ in the steps.
  • Accessing keras.optimizers has changed in keras 3.* to having to use the keras.optimizers.get method and providing a class_name and config to deserialize. Relevant for lstm and feedforward autoencoders.
  • Model config now requires for tensorflow.keras.models.Sequential to define input_shape in its layers, otherwise model is not properly built after compiling and prior to fitting. Relevant for KerasRawModelRegressor.

KerasBaseEstimator underwent the most changes.

  • We now need to run the __init__ of the KerasRegressor with the expected kwargs for proper initialisation of the object, but the kwargs will always take precedence for fit, predict and compile, so they are mostly for making keras happy.
  • Saving model for pickling was changed ƒrom h5 format, which is now considered legacy to keras native zip format, and the file is now stored to a temporary file and read into a buffer instead of using the h5py library, since save_model and load_model now exclusively expect an actual file instead of an io buffer.
  • Current implementation of model preparation for fit depends on the __call__ method, which is no longer used in keras 3.*, and was changed to _prepare_model to replicate the same behaviour right before we call our own fit since it requires the X and Y to be present to set the n_features and n_features_out.
  • History is no longer returned from calling fit and must be extracted from under model.history.
  • Manually stored history now resides in self._history instead of self.history to avoid attribute name clash.

Adjustments to tests were made:

  • input_shape now resides under input.shape in model.layers.
  • optimizer_kwargs.lr is now optimizer_kwargs.learning_rate

To accommodate the upgrade, next changes to the code were made:

* Since `keras 3.*` now clones the input `regressor` that is provided to the `BaseEstimator` into a `regressor_` attribute, and ONLY updates the `regressor_` one after fitting, metadata extraction in `gordo.builder.build_model.ModelBuilder` was changed to seek the `regressor_` in the `steps`.
* Accessing `keras.optimizers` has changed in `keras 3.*` to having to use the `keras.optimizers.get` method and providing a `class_name` and `config` to deserialize. Relevant for lstm and feedforward autoencoders.
* Model config now requires for `tensorflow.keras.models.Sequential` to define `input_shape` in its layers, otherwise model is not properly built after compiling and prior to fitting. Relevant for `KerasRawModelRegressor`.

`KerasBaseEstimator` underwent the most changes.

* We now need to run the `__init__` of the KerasRegressor with the expected `kwargs` for proper initialisation of the object, but the `kwargs` will always take precedence for `fit`, `predict` and `compile`, so they are mostly for making `keras` happy.
* Saving model for pickling was changed ƒrom `h5` format, which is now considered legacy to `keras` native zip format, and the file is now stored to a temporary file and read into a buffer instead of using the `h5py` library, since `save_model` and `load_model` now exclusively expect an actual file instead of an io buffer.
* Current implementation of model preparation for fit depends on the `__call__` method, which is no longer used in `keras 3.*`, and was changed to `_prepare_model` to replicate the same behaviour right before we call our own `fit` since it requires the `X` and `Y` to be present to set the `n_features` and `n_features_out`.
* `History` is no longer returned from calling `fit` and must be extracted from under `model.history`.
* Manually stored history now resides in `self._history` instead of `self.history` to avoid attribute name clash.

Adjustments to tests were made:

* `input_shape` now resides under `input.shape` in `model.layers`.
* `optimizer_kwargs.lr` is now `optimizer_kwargs.learning_rate`
Comment on lines 559 to 561
if key.endswith(
"_"
): # keras3 clones the regressor into regressor_ and never updates original regressor
Copy link
Contributor

@koropets koropets Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if key.endswith(
"_"
): # keras3 clones the regressor into regressor_ and never updates original regressor
if key == "regressor": # keras3 clones the regressor into regressor_ and never updates original regressor
continue

endwith is too broad to fix one back-compatibility issue

Comment on lines 87 to 89
*self._fit_kwargs,
*self._predict_kwargs,
*self._compile_kwargs,
Copy link
Contributor

@koropets koropets Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*self._fit_kwargs,
*self._predict_kwargs,
*self._compile_kwargs,
*KerasRegressor._fit_kwargs,
*KerasRegressor._predict_kwargs,
*KerasRegressor._compile_kwargs,

in case of self not clear what an exact source of these constants

@@ -301,25 +313,25 @@ def get_params(self, **params):
Parameters used in this estimator
"""
params = super().get_params(**params)
params.pop("build_fn", None)
params.pop("model", None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
params.pop("model", None)

unnecessary call

Comment on lines 84 to 85
input_shape:
- 4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
input_shape:
- 4
input_shape: [4]

just suggestion

Comment on lines 88 to 89
input_shape:
- 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
input_shape:
- 1

@RollerKnobster RollerKnobster merged commit b327587 into master Jun 26, 2024
17 checks passed
@RollerKnobster RollerKnobster deleted the feature/rivan/163526-upgrade-tensorflow-and-keras branch June 26, 2024 11:59
@@ -88,8 +88,9 @@ class (e.x. Adam(lr=0.01,beta_1=0.9, beta_2=0.999)). If no arguments are

# Instantiate optimizer with kwargs
if isinstance(optimizer, str):
Optim = getattr(keras.optimizers, optimizer)
optimizer = Optim(**optimizer_kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this previous approach still works though. It's what we have implemented in aion and runs fine. But I'm ok with changing it.

import tensorflow.keras.models
from tensorflow.keras.models import load_model, save_model
from tensorflow.keras.preprocessing.sequence import pad_sequences, TimeseriesGenerator
from tensorflow.keras.wrappers.scikit_learn import KerasRegressor as BaseWrapper
from tensorflow.keras.callbacks import History
from scikeras.wrappers import KerasRegressor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting I wasn't aware about this. Good you found this out.

*KerasRegressor._predict_kwargs,
*KerasRegressor._compile_kwargs,
}
KerasRegressor.__init__(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed a bit strange, but oh well, as long as it works. Fortunately we won't need this wrapping business in aion.

Copy link
Contributor

@hugobettmach hugobettmach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow quite a lot of tricky changes to make, good work @RollerKnobster in finding these out. It's hard to have an opinion if they will work or not but once we have a pre-release of gordo-anomaly I will use my local dieta to test it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants