fix: Import error for cloud_profiler #869
Conversation
google-cla
bot
added
the
cla: yes
product-auto-label
bot
added
the
api: aiplatform
…atform into training_utils_fix
setup.py
Outdated
| profiler_extra_require = ["tensorboard-plugin-profile", "tensorflow >=2.4.0"] | ||
| profiler_extra_require = [ | ||
| "tensorboard-plugin-profile", | ||
| "werkzeug", |
There was a problem hiding this comment.
Please add constraints on this version. Also preferable on TB plugin profile above.
| @@ -17,6 +17,15 @@ | |||
|
|
|||
| """A plugin to handle remote tensoflow profiler sessions for Vertex AI.""" | |||
|
|
|||
| try: | |||
| from tensorboard_plugin_profile.profile_plugin import ProfilePlugin | |||
| from werkzeug import Response | |||
There was a problem hiding this comment.
initializer.py imports werkzeug before importing tf_profiler: https://github.com/googleapis/python-aiplatform/blob/main/google/cloud/aiplatform/training_utils/cloud_profiler/initializer.py#L21
Recommend catching on all optional imports or walking through all the imports to determine when they are first imported.
There was a problem hiding this comment.
Done, I went with catching the imports at where they were first imported. The tests should catch the verbiage as well.
| for mock_module in ["tensorboard_plugin_profile.profile_plugin", "werkzeug"]: | ||
| with self.subTest(): | ||
| with mock.patch.dict("sys.modules", {mock_module: None}): | ||
| self.assertRaises(ImportError, reload, tf_profiler) |
There was a problem hiding this comment.
Should this test importing cloud_profiler as that's the prescribed usage?
Also, may want to use the context manager to get the exception object and ensure the raised exception includes the intended verbiage.
| @@ -25,16 +25,22 @@ | |||
| import tensorboard.plugins.base_plugin as tensorboard_base_plugin | |||
| from typing import Callable, Dict, Optional | |||
| from urllib import parse | |||
| from werkzeug import Response | |||
|
|
|||
| from google.cloud.aiplatform.tensorboard.plugins.tf_profiler import profile_uploader | |||
There was a problem hiding this comment.
It looks like this would import Tensorflow and not be caught.
…atform into training_utils_fix
…ng cloud-profiler import errors
Circular imports involving absolute imports with binding a submodule to a name was not supported in python 3.6, as documented here. Needed to update import.
Fixes #867