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.

Sign up for GitHub

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

Jump to bottom

Remove mlflow.keras raise deprecation warning to avoid keras==2.4.3 tensorflow==2.5.1 issue #4790

Merged
merged 9 commits into from
Sep 22, 2021
Merged

Remove mlflow.keras raise deprecation warning to avoid keras==2.4.3 tensorflow==2.5.1 issue #4790

merged 9 commits into from
Sep 22, 2021

Conversation

eedeleon
Copy link
Contributor

@eedeleon eedeleon commented Sep 10, 2021 •
edited
Loading

What changes are proposed in this pull request?

Adding a protection against attribute errors that arise when tensorflow.keras and keras are both installed.
https://stackoverflow.com/questions/61137954/attributeerror-module-tensorflow-python-keras-utils-generic-utils-has-no-attr

keras==2.4.3
tensorflow==2.5.1

How is this patch tested?

The above error is now silent in the original code path. The keras_module is set to tensorflow.keras and successfully logs the model

Release Notes

Patch bug due to incompatible keras and tensorflow.keras versions.

Is this a user-facing change?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release notes for MLflow users.

What component(s), interfaces, languages, and integrations does this PR affect?

Components

  • area/artifacts: Artifact stores and artifact logging
  • area/build: Build and test infrastructure for MLflow
  • area/docs: MLflow documentation pages
  • area/examples: Example code
  • area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
  • area/models: MLmodel format, model serialization/deserialization, flavors
  • area/projects: MLproject format, project running backends
  • area/scoring: MLflow Model server, model deployment tools, Spark UDFs
  • area/server-infra: MLflow Tracking server backend
  • area/tracking: Tracking Service, tracking client APIs, autologging

Interface

  • area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
  • area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
  • area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
  • area/windows: Windows support

Language

  • language/r: R APIs and clients
  • language/java: Java APIs and clients
  • language/new: Proposals for new client languages

Integrations

  • integrations/azure: Azure and Azure ML integrations
  • integrations/sagemaker: SageMaker integrations
  • integrations/databricks: Databricks integrations

How should the PR be classified in the release notes? Choose one:

  • rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
  • rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
  • rn/feature - A new user-facing feature worth mentioning in the release notes
  • rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
  • rn/documentation - A user-facing documentation change worth mentioning in the release notes

@github-actions
Copy link

@eedeleon Thanks for the contribution! The DCO check failed. Please sign off your commits by following the instructions here: https://github.com/mlflow/mlflow/runs/3569645554. See https://github.com/mlflow/mlflow/blob/master/CONTRIBUTING.rst#sign-your-work for more details.

WeichenXu123 and others added 2 commits September 10, 2021 13:19
@WeichenXu123
Fix RoxygenNote version (mlflow#4789)
b37a91d 
Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>
Also ignore attribute error due to tf.keras and keras
700e0d0 
Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>
@github-actions github-actions bot added rn/bug-fix Mention under Bug Fixes in Changelogs. rn/none List under Small Changes in Changelogs. and removed rn/bug-fix Mention under Bug Fixes in Changelogs. labels Sep 10, 2021
Silence keras warning if module is set
f985c7e 
Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

Handle load_model

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

Handle load_model

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>
Eduardo De Leon added 2 commits September 10, 2021 14:38
Fix ordering
ca3762d 
Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>
Add missing space
3bedde9 
Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>
@eedeleon eedeleon changed the title [WIP] mlflow.keras should not call import keras mlflow.keras should not call import keras Sep 10, 2021
sidhartha-roy
sidhartha-roy approved these changes Sep 10, 2021
View reviewed changes
Copy link

@sidhartha-roy sidhartha-roy left a comment

Choose a reason for hiding this comment

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

looks good

@eedeleon
Copy link
Contributor Author

eedeleon commented Sep 10, 2021 •
edited
Loading

I ran into this issue with keras import failing for the provided versions

../../../.local/lib/python3.8/site-packages/mlflow/keras.py:404: in log_model
Model.log(
../../../.local/lib/python3.8/site-packages/mlflow/models/model.py:187: in log
flavor.save_model(path=local_path, mlflow_model=mlflow_model, **kwargs)
../../../.local/lib/python3.8/site-packages/mlflow/keras.py:193: in save_model
_raise_deprecation_warning()
../../../.local/lib/python3.8/site-packages/mlflow/keras.py:68: in _raise_deprecation_warning
import keras
../../../.local/lib/python3.8/site-packages/keras/init.py:20: in
from . import initializers
../../../.local/lib/python3.8/site-packages/keras/initializers/init.py:124: in
populate_deserializable_objects()

AttributeError: module 'keras.utils.generic_utils' has no attribute 'populate_dict_with_module_objects'

../../../.local/lib/python3.8/site-packages/keras/initializers/init.py:82: AttributeError

@eedeleon eedeleon changed the title mlflow.keras should not call import keras mlflow.keras should not call import keras for tensorflow.keras module Sep 12, 2021
Eduardo De Leon added 2 commits September 11, 2021 22:11
Change to ignore tensorflow module
a91542e 
Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>
Protect against None
0017bd2 
Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>
harupy
harupy reviewed Sep 13, 2021
View reviewed changes
mlflow/keras.py Outdated
@@ -61,9 +61,11 @@
_PIP_ENV_SUBPATH = "requirements.txt"


def _raise_deprecation_warning():
def _raise_deprecation_warning(keras_module=None):
Copy link
Member

Choose a reason for hiding this comment

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

@eedeleon We're planning to remove this function and drop support for keras < 2.3.0 and tensorflow < 2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. However, currently I get a failure for

keras==2.4.3
tensorflow==2.5.1

Would the correct PR be to remove it altogether?

Copy link
Member

@harupy harupy Sep 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@eedeleon Thanks for the reply. Got it. If we remove mlflow.keras._raise_deprecation_warning that contains import keras, does that solve the issue or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you mean remove. If so, yes

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, "remove" was missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you like me to make a different PR removing the function and all calls to it? Or is someone else doing so.

Copy link
Member

Choose a reason for hiding this comment

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

That'd be great!

harupy
harupy reviewed Sep 15, 2021
View reviewed changes
mlflow/R/mlflow/DESCRIPTION Outdated
@@ -61,7 +61,7 @@ Suggests:
testthat (>= 2.0.0),
xgboost
Encoding: UTF-8
RoxygenNote: 7.1.1
RoxygenNote: 7.1.2
Copy link
Member

Choose a reason for hiding this comment

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

Can we revert this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure why it was added to my branch but I reverted it

Eduardo De Leon added 2 commits September 20, 2021 21:24
Revert "Fix RoxygenNote version (mlflow#4789)"
ac0e2fe 
This reverts commit b37a91d.

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>
Remove usage of raise dep warning
3499b1d 
Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>
@eedeleon eedeleon changed the title mlflow.keras should not call import keras for tensorflow.keras module Remove mlflow.keras raise deprecation warning to avoid keras==2.4.3 tensorflow==2.5.1 issue Sep 22, 2021
harupy
harupy approved these changes Sep 22, 2021
View reviewed changes
Copy link
Member

@harupy harupy left a comment

Choose a reason for hiding this comment

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

LGTM!

@harupy harupy merged commit 76c2205 into mlflow:master Sep 22, 2021
zbloss pushed a commit to zbloss/mlflow that referenced this pull request Sep 28, 2021
@eedeleon @WeichenXu123
Remove mlflow.keras raise deprecation warning to avoid keras==2.4.3 t…
1116c11 
…ensorflow==2.5.1 issue (mlflow#4790)

* Fix RoxygenNote version (mlflow#4789)

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Also ignore attribute error due to tf.keras and keras

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Silence keras warning if module is set

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

Handle load_model

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

Handle load_model

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Fix ordering

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Add missing space

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Change to ignore tensorflow module

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Protect against None

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Revert "Fix RoxygenNote version (mlflow#4789)"

This reverts commit b37a91d.

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Remove usage of raise dep warning

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

Co-authored-by: WeichenXu <weichen.xu@databricks.com>
Co-authored-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>
Signed-off-by: Zach Bloss <ZachBloss@RockCentralDetroit.com>
zbloss pushed a commit to zbloss/mlflow that referenced this pull request Sep 28, 2021
@eedeleon @WeichenXu123 @zbloss
Remove mlflow.keras raise deprecation warning to avoid keras==2.4.3 t…
6c88e68 
…ensorflow==2.5.1 issue (mlflow#4790)

* Fix RoxygenNote version (mlflow#4789)

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Also ignore attribute error due to tf.keras and keras

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Silence keras warning if module is set

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

Handle load_model

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

Handle load_model

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Fix ordering

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Add missing space

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Change to ignore tensorflow module

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Protect against None

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Revert "Fix RoxygenNote version (mlflow#4789)"

This reverts commit b37a91d.

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

* Remove usage of raise dep warning

Signed-off-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>

Co-authored-by: WeichenXu <weichen.xu@databricks.com>
Co-authored-by: Eduardo De Leon <eedeleon@Eduardos-MBP.attlocal.net>
Signed-off-by: Zach Bloss <ZachBloss@RockCentralDetroit.com>
Signed-off-by: Zach Bloss <zacharybloss@gmail.com>
@harupy harupy mentioned this pull request Oct 12, 2021
Merged
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harupy harupy harupy approved these changes

@sidhartha-roy sidhartha-roy sidhartha-roy approved these changes

Assignees
No one assigned
Labels
rn/none List under Small Changes in Changelogs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@eedeleon @harupy @sidhartha-roy @WeichenXu123