fix: Import error for cloud_profiler#869
Conversation
…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.
|
|
||
| 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.
| 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