Fix tensorflow 2.5.2 cross test failure #4995
Conversation
github-actions
bot
added
area/tracking
| @@ -912,7 +912,7 @@ def test_import_tensorflow_with_fluent_autolog_enables_tf_autologging(): | |||
|
|
|||
| # NB: For backwards compatibility, fluent autologging enables TensorFlow and | |||
| # Keras autologging upon tensorflow import in TensorFlow 2.5.1 | |||
There was a problem hiding this comment.
The comments are also outdated here.
| @@ -927,7 +927,7 @@ def test_import_tf_keras_with_fluent_autolog_enables_tf_autologging(): | |||
|
|
|||
| # NB: For backwards compatibility, fluent autologging enables TensorFlow and | |||
| # Keras autologging upon tf.keras import in TensorFlow 2.5.1 | |||
| @@ -912,7 +912,7 @@ def test_import_tensorflow_with_fluent_autolog_enables_tf_autologging(): | |||
|
|
|||
| # NB: For backwards compatibility, fluent autologging enables TensorFlow and | |||
| # Keras autologging upon tensorflow import in TensorFlow 2.5.1 | |||
| if Version(tf.__version__) != Version("2.5.1"): | |||
| if Version(tf.__version__) >= Version("2.6"): | |||
There was a problem hiding this comment.
Why do we also want to disable Version(tf.__version__) < Version("2.5.1")? it looks like only 2.5.1 and 2.5.2 are causing failures.
There was a problem hiding this comment.
@dbczumar
I think previous code logic here is wrong. Only >= 2.6 version will redirect keras autologging to tensorflow autologging.
There was a problem hiding this comment.
CC @liangz1
For more context, you can read this PR:
#4766
Simple explanation:
In Tensorflow >= 2.6, the Tensorflow embedded keras will be linked to the solely installed keras (in contrast, previous Tensorflow embedded keras was a different module with the solely installed keras)
The change in Tensorflow >= 2.6, cause the mlflow.tensorflow.autolog (including patching on tf and embeded tf.keras) and mlflow.keras.autolog (patching on keras ) conflicts. So, as a workaround, for TF >= 2.6, we disable mlflow.keras.autolog, but instead, we only trigger mlflow.tensorflow.autolog
There was a problem hiding this comment.
LGTM! Thanks @WeichenXu123 ! Can we update the comments in the tests as well, as @liangz1 pointed out?
Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
| if Version(tf.__version__) >= Version("2.6"): | ||
| assert autologging_is_disabled(mlflow.keras.FLAVOR_NAME) |
There was a problem hiding this comment.
@WeichenXu123 @dbczumar
In tensorflow < 2.6, does import tensorflow.keras enable keras autologging?
| # NB: In Tensorflow >= 2.6, we redirect keras autologging to tensorflow autologging | ||
| # so the original keras autologging is disabled | ||
| if Version(tf.__version__) >= Version("2.6"): | ||
| import keras # pylint: disable=unused-variable,unused-import |
There was a problem hiding this comment.
Add import keras to make test following test autologging_is_disabled(mlflow.keras.FLAVOR_NAME) meaningful
There was a problem hiding this comment.
Does import tensorflow.keras run import keras or not?
There was a problem hiding this comment.
I'd prefer to create a separate test (e.g. test_import_keras_with_fluent_autolog_does_not_enable_keras_autologging)
There was a problem hiding this comment.
Does import tensorflow.keras run import keras or not?
For TF >= 2.6, Yes.
Remove import keras under this case.
There was a problem hiding this comment.
I'd prefer to create a separate test (e.g. test_import_keras_with_fluent_autolog_does_not_enable_keras_autologging)
We already has it. "test_import_keras_with_fluent_autolog_enables_tensorflow_autologging"
The test test_import_tensorflow_with_fluent_autolog_enables_tf_autologging is a bit different, it want to test we import tensorflow first (invoke set_up_tensorflow_autologging), then import keras( invoke conditionally_set_up_keras_autologging), different invoking order leads to different code path coverage.
There was a problem hiding this comment.
We already has it. "test_import_keras_with_fluent_autolog_enables_tensorflow_autologging"
Ah I missed that.
|
Seems a new error (irrelative to this PR) happen: This happen under TF 2.6 installed (it will install a incompatible keras version (keras 2.7) as dependency) |
Found related issue: tensorflow/tensorflow#52922 |
|
TF 2.6.2 released to pypi about 2 hours ago with a version fix in dependencies. |
Signed-off-by: Weichen Xu weichen.xu@databricks.com
What changes are proposed in this pull request?
Fix tensorflow 2.5.2 cross test failure
Tensorflow release 2.5.2 today https://pypi.org/project/tensorflow/2.5.2/ , and it cause this failure happen.
How is this patch tested?
Unit tests.
Release Notes
Is this a user-facing change?
(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)
What component(s), interfaces, languages, and integrations does this PR affect?
Components
area/artifacts: Artifact stores and artifact loggingarea/build: Build and test infrastructure for MLflowarea/docs: MLflow documentation pagesarea/examples: Example codearea/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registryarea/models: MLmodel format, model serialization/deserialization, flavorsarea/projects: MLproject format, project running backendsarea/scoring: MLflow Model server, model deployment tools, Spark UDFsarea/server-infra: MLflow Tracking server backendarea/tracking: Tracking Service, tracking client APIs, autologgingInterface
area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev serverarea/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Modelsarea/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registryarea/windows: Windows supportLanguage
language/r: R APIs and clientslanguage/java: Java APIs and clientslanguage/new: Proposals for new client languagesIntegrations
integrations/azure: Azure and Azure ML integrationsintegrations/sagemaker: SageMaker integrationsintegrations/databricks: Databricks integrationsHow should the PR be classified in the release notes? Choose one:
rn/breaking-change- The PR will be mentioned in the "Breaking Changes" sectionrn/none- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/feature- A new user-facing feature worth mentioning in the release notesrn/bug-fix- A user-facing bug fix worth mentioning in the release notesrn/documentation- A user-facing documentation change worth mentioning in the release notes