Fix creating local tensorflow model data fails with module import and tensorflow versioning #1205
Conversation
| @@ -525,6 +525,8 @@ def test_invalid_local_data(client): | |||
| ) | |||
|
|
|||
|
|
|||
| # TODO: Remove this pytest fixture on next release. | |||
There was a problem hiding this comment.
Please make a task for this and put it in the format TODO(ENG-...)
| @@ -10,7 +10,8 @@ RUN apt-get update && \ | |||
| aqueduct-ml \ | |||
| boto3 \ | |||
| pandas \ | |||
| pydantic | |||
| pydantic \ | |||
| tensorflow==2.12.0 | |||
There was a problem hiding this comment.
cc: @hsubbaraj-spiral I assume we don't want to pin an exact version?
| @@ -114,7 +114,7 @@ def _read_local_bytes_content(path: str) -> bytes: | |||
| def _read_local_tf_keras_model(path: str) -> Any: | |||
| from tensorflow import keras | |||
|
|
|||
| return keras.saving.load_model(path) | |||
| return keras.models.load_model(path) | |||
There was a problem hiding this comment.
If this is backwards compatible (works with multiple TF versions), do we need to pin the TF version in the dockerfile?
There was a problem hiding this comment.
After I took a look at their release log:
"Moved all saving-related utilities to a new namespace, keras.saving, i.e. keras.saving.load_model, keras.saving.save_model, keras.saving.custom_object_scope, keras.saving.get_custom_objects, keras.saving.register_keras_serializable,keras.saving.get_registered_name and keras.saving.get_registered_object. The previous API locations (in keras.utils and keras.models) will stay available indefinitely, but we recommend that you update your code to point to the new API locations."
I guess we don't need to pin it to a specific version? @kenxu95 @hsubbaraj-spiral
There was a problem hiding this comment.
Can we lower bound it in the dockerfile?
There was a problem hiding this comment.
Lower bounded it at the latest TF release.
There was a problem hiding this comment.
Actually wait does lower-bounding this actually matter? @hsubbaraj-spiral Should we just rid of the constraint altogether so it always uses the latest for now.
There was a problem hiding this comment.
Wait actually I just remembered - we don't want to install tensorflow in the dockerfile right since it would take forever? I'm confused as to what the point of this change is now. I think we can assume that local data doesn't work with K8s for now, and make a task for that.
There was a problem hiding this comment.
I think we'll want to just run the parameter operators locally as a long-term solution for K8s.
| @@ -62,7 +62,7 @@ runs: | |||
|
|
|||
| - name: Install packages needed for testing | |||
| shell: bash | |||
| run: python3 -m pip install nltk pytest-xdist tensorflow | |||
There was a problem hiding this comment.
Remove tensorflow as a requirement for setup-server since we are not testing tensorflow models curently.
|
I disabled the integration test for tf_keras parameter artifact. Since we don't have |
vsreekanti
deleted the
eng-2797-create-local-tensorflow-model-data-fails
branch
Describe your changes and why you are making these changes
This PR contains two changes regarding local tensorflow data parameter test:
keras.saving.load_model. Therefore, we change the loading method tokeras.models.load_modelwhich presents in both new and older version of tensorflow.Related issue number (if any)
Loom demo (if any)
Checklist before requesting a review
python3 scripts/run_linters.py -hfor usage).run_integration_test: Runs integration testsskip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)