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

fix: Import error for cloud_profiler #869

Merged
merged 12 commits into from Dec 1, 2021
Merged

fix: Import error for cloud_profiler #869

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1368556
fix import error involving absolute imports: bpo-30024
mkovalski Nov 29, 2021
7d8ef46
fix: import error involving absolute imports: fixes #867
mkovalski Nov 29, 2021
58c5519
Merge branch 'training_utils_fix' of github.com:mkovalski/python-aipl…
mkovalski Nov 29, 2021
d76cef1
fix: Move the import checks to tf profiler module
mkovalski Nov 29, 2021
91ef8d5
fix: remove unnecessary importlib mock from tf_profiler test
mkovalski Nov 30, 2021
97d41d4
fix: add werkzeug to profiler requirements and add additional missing…
mkovalski Nov 30, 2021
b9353a8
fix: version requirements on setup.py, catching exception and checkin…
mkovalski Dec 1, 2021
54e3839
setup using cloud-profiler rather than cloud_profiler
mkovalski Dec 1, 2021
728d559
Merge branch 'main' into training_utils_fix
sasha-gitg Dec 1, 2021
71a2c06
fix: Add tensorflow to module check
mkovalski Dec 1, 2021
780cd00
Merge branch 'training_utils_fix' of github.com:mkovalski/python-aipl…
mkovalski Dec 1, 2021
c3c2c9e
fix: Adding tensorflow and tensorboard as top level import for catchi…
mkovalski Dec 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 1 addition & 7 deletions google/cloud/aiplatform/training_utils/cloud_profiler/__init__.py
View file
Open in desktop
Expand Up @@ -15,13 +15,7 @@
# limitations under the License.
#

try:
mkovalski marked this conversation as resolved.
Show resolved Hide resolved
import google.cloud.aiplatform.training_utils.cloud_profiler.initializer as initializer
except ImportError as err:
raise ImportError(
"Could not load the cloud profiler. To use the profiler, "
'install the SDK using "pip install google-cloud-aiplatform[cloud-profiler]"'
) from err
from google.cloud.aiplatform.training_utils.cloud_profiler import initializer

"""
Initialize the cloud profiler for tensorflow.
Expand Down
12 changes: 9 additions & 3 deletions google/cloud/aiplatform/training_utils/cloud_profiler/plugins/tensorflow/tf_profiler.py
View file
Open in desktop
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I went with catching the imports at where they were first imported. The tests should catch the verbiage as well.

except ImportError as err:
raise ImportError(
"Could not load the cloud profiler. To use the profiler, "
'install the SDK using "pip install google-cloud-aiplatform[cloud-profiler]"'
) from err

import argparse
from collections import namedtuple
import importlib.util
Expand All @@ -25,7 +34,6 @@
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
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this would import Tensorflow and not be caught.

from google.cloud.aiplatform.training_utils import environment_variables
Expand Down Expand Up @@ -269,8 +277,6 @@ class TFProfiler(base_plugin.BasePlugin):

def __init__(self):
"""Build a TFProfiler object."""
from tensorboard_plugin_profile.profile_plugin import ProfilePlugin

context = _create_profiling_context()
self._profile_request_sender: profile_uploader.ProfileRequestSender = tensorboard_api.create_profile_request_sender()
self._profile_plugin: ProfilePlugin = ProfilePlugin(context)
Expand Down
6 changes: 5 additions & 1 deletion setup.py
View file
Open in desktop
Expand Up @@ -36,7 +36,11 @@
tensorboard_extra_require = ["tensorflow >=2.3.0, <=2.5.0"]
metadata_extra_require = ["pandas >= 1.0.0"]
xai_extra_require = ["tensorflow >=2.3.0, <=2.5.0"]
profiler_extra_require = ["tensorboard-plugin-profile", "tensorflow >=2.4.0"]
profiler_extra_require = [
"tensorboard-plugin-profile",
"werkzeug",
Copy link
Member

Choose a reason for hiding this comment

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

Please add constraints on this version. Also preferable on TB plugin profile above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

"tensorflow >=2.4.0",
]

full_extra_require = list(
set(tensorboard_extra_require + metadata_extra_require + xai_extra_require)
Expand Down
14 changes: 6 additions & 8 deletions tests/unit/aiplatform/test_cloud_profiler.py
View file
Open in desktop
Expand Up @@ -130,6 +130,12 @@ class TestProfilerPlugin(unittest.TestCase):
def setUp(self):
setupProfilerEnvVars()

def testImportError(self):
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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


# Environment variable tests
def testCanInitializeProfilerPortUnset(self):
tf_profiler.environment_variables.tf_profiler_port = None
Expand Down Expand Up @@ -359,14 +365,6 @@ def start_response(status, headers):

# Initializer tests
class TestInitializer(unittest.TestCase):
# Tests for building the plugin
def test_init_failed_import(self):
with mock.patch.dict(
"sys.modules",
{"google.cloud.aiplatform.training_utils.cloud_profiler.initializer": None},
):
self.assertRaises(ImportError, reload, training_utils.cloud_profiler)

def test_build_plugin_fail_initialize(self):
plugin = _create_mock_plugin()
plugin.can_initialize.return_value = False
Expand Down