Skip to content

{Perf} Delay importing MSIAuthenticationWrapper and check_version_compatibility #14331

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

Merged
jiasli merged 3 commits into from
Jul 15, 2020
Merged

{Perf} Delay importing MSIAuthenticationWrapper and check_version_compatibility #14331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a876e9
Delay importing MSIAuthenticationWrapper and requests
jiasli Jul 13, 2020
0e6082e
Add comments
jiasli Jul 13, 2020
e8efdce
Fix tests
jiasli Jul 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/azure-cli-core/azure/cli/core/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,6 @@ def _update_command_table_from_extensions(ext_suppressions, extension_modname=No
Otherwise, the list will be extended using ALWAYS_LOADED_EXTENSIONS.
If the extensions in the list are not installed, it will be skipped.
"""

from azure.cli.core.extension.operations import check_version_compatibility

def _handle_extension_suppressions(extensions):
filtered_extensions = []
for ext in extensions:
Expand Down Expand Up @@ -299,6 +296,9 @@ def _filter_modname(extensions):

for ext in allowed_extensions:
try:
# Import in the `for` loop because `allowed_extensions` can be []. In such case we
# don't need to import `check_version_compatibility` at all.
from azure.cli.core.extension.operations import check_version_compatibility
Copy link
Contributor

@haroldrandom haroldrandom Jul 15, 2020

Choose a reason for hiding this comment

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

Importing a module in a for loop is not a good practice.
How much can it benefit from importing in this for loop?

Copy link
Member

@jsntcy jsntcy Jul 15, 2020

Choose a reason for hiding this comment

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

You can import module as many times as you want in one Python program, no matter what module it is. Every subsequent import after the first accesses the cached module instead of re-evaluating it. So there is no performance penalty.

In reply to: 454774982 [](ancestors = 454774982)

Copy link
Member Author

@jiasli jiasli Jul 15, 2020

Choose a reason for hiding this comment

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

This is a tricky part. I just want to eliminate an extra if allowed_extensions:. Python has an import cache which will handle "import in a for loop" nicely.

check_version_compatibility(ext.get_metadata())
except CLIError as ex:
# issue warning and skip loading extensions that aren't compatible with the CLI core
Expand Down
5 changes: 3 additions & 2 deletions src/azure-cli-core/azure/cli/core/_profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
is_windows, is_wsl
from azure.cli.core.cloud import get_active_cloud, set_cloud_subscription

from .adal_authentication import MSIAuthenticationWrapper
Copy link
Member Author

@jiasli jiasli Jul 15, 2020

Choose a reason for hiding this comment

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

@jsntcy, relative import it is not recommended according to PEP 8:

Absolute imports are recommended, as they are usually more readable and tend to be better behaved (or at least give better error messages)...


from knack.log import get_logger
from knack.util import CLIError

Expand Down Expand Up @@ -309,6 +307,7 @@ def find_subscriptions_in_vm_with_msi(self, identity_id=None, allow_no_subscript
import jwt
from requests import HTTPError
from msrestazure.tools import is_valid_resource_id
from azure.cli.core.adal_authentication import MSIAuthenticationWrapper
resource = self.cli_ctx.cloud.endpoints.active_directory_resource_id

if identity_id:
Expand Down Expand Up @@ -388,6 +387,7 @@ def find_subscriptions_in_cloud_console(self):
return deepcopy(consolidated)

def _get_token_from_cloud_shell(self, resource): # pylint: disable=no-self-use
from azure.cli.core.adal_authentication import MSIAuthenticationWrapper
auth = MSIAuthenticationWrapper(resource=resource)
auth.set_token()
token_entry = auth.token
Expand Down Expand Up @@ -773,6 +773,7 @@ def valid_msi_account_types():

@staticmethod
def msi_auth_factory(cli_account_name, identity, resource):
from azure.cli.core.adal_authentication import MSIAuthenticationWrapper
if cli_account_name == MsiAccountTypes.system_assigned:
return MSIAuthenticationWrapper(resource=resource)
if cli_account_name == MsiAccountTypes.user_assigned_client_id:
Expand Down
2 changes: 1 addition & 1 deletion src/azure-cli-core/azure/cli/core/extension/_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License. See License.txt in the project root for license information.
# --------------------------------------------------------------------------------------------
import requests

from knack.log import get_logger
from knack.util import CLIError
Expand All @@ -22,6 +21,7 @@

# pylint: disable=inconsistent-return-statements
def get_index(index_url=None):
import requests
from azure.cli.core.util import should_disable_connection_verify
index_url = index_url or DEFAULT_INDEX_URL

Expand Down
3 changes: 2 additions & 1 deletion src/azure-cli-core/azure/cli/core/extension/operations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
from subprocess import check_output, STDOUT, CalledProcessError
from six.moves.urllib.parse import urlparse # pylint: disable=import-error

import requests
from pkg_resources import parse_version

from azure.cli.core import CommandIndex
Expand Down Expand Up @@ -64,6 +63,7 @@ def _run_pip(pip_exec_args, extension_path=None):


def _whl_download_from_url(url_parse_result, ext_file):
import requests
from azure.cli.core.util import should_disable_connection_verify
url = url_parse_result.geturl()
r = requests.get(url, stream=True, verify=(not should_disable_connection_verify()))
Expand Down Expand Up @@ -106,6 +106,7 @@ def _add_whl_ext(cmd, source, ext_sha256=None, pip_extra_index_urls=None, pip_pr
tmp_dir = tempfile.mkdtemp()
ext_file = os.path.join(tmp_dir, whl_filename)
logger.debug('Downloading %s to %s', source, ext_file)
import requests
try:
cmd.cli_ctx.get_progress_controller().add(message='Downloading')
_whl_download_from_url(url_parse_result, ext_file)
Expand Down
16 changes: 8 additions & 8 deletions src/azure-cli-core/azure/cli/core/tests/test_profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,7 @@ def test_get_login_credentials_aux_tenants(self, mock_get_token, mock_read_cred_
aux_tenants=[test_tenant_id2])

@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
def test_get_login_credentials_msi_system_assigned(self, mock_msi_auth, mock_read_cred_file):
mock_read_cred_file.return_value = []

Expand Down Expand Up @@ -676,7 +676,7 @@ def test_get_login_credentials_msi_system_assigned(self, mock_msi_auth, mock_rea
self.assertTrue(cred.token_read_count)

@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
def test_get_login_credentials_msi_user_assigned_with_client_id(self, mock_msi_auth, mock_read_cred_file):
mock_read_cred_file.return_value = []

Expand Down Expand Up @@ -707,7 +707,7 @@ def test_get_login_credentials_msi_user_assigned_with_client_id(self, mock_msi_a
self.assertTrue(cred.client_id, test_client_id)

@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
def test_get_login_credentials_msi_user_assigned_with_object_id(self, mock_msi_auth, mock_read_cred_file):
mock_read_cred_file.return_value = []

Expand Down Expand Up @@ -738,7 +738,7 @@ def test_get_login_credentials_msi_user_assigned_with_object_id(self, mock_msi_a
self.assertTrue(cred.object_id, test_object_id)

@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
def test_get_login_credentials_msi_user_assigned_with_res_id(self, mock_msi_auth, mock_read_cred_file):
mock_read_cred_file.return_value = []

Expand Down Expand Up @@ -849,7 +849,7 @@ def test_get_raw_token_for_sp(self, mock_get_token, mock_read_cred_file):
self.assertEqual(tenant, self.tenant_id)

@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
def test_get_raw_token_msi_system_assigned(self, mock_msi_auth, mock_read_cred_file):
mock_read_cred_file.return_value = []

Expand Down Expand Up @@ -884,7 +884,7 @@ def test_get_raw_token_msi_system_assigned(self, mock_msi_auth, mock_read_cred_f

@mock.patch('azure.cli.core._profile.in_cloud_console', autospec=True)
@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
def test_get_raw_token_in_cloud_console(self, mock_msi_auth, mock_read_cred_file, mock_in_cloud_console):
mock_read_cred_file.return_value = []
mock_in_cloud_console.return_value = True
Expand Down Expand Up @@ -1037,7 +1037,7 @@ def test_find_subscriptions_thru_username_non_password(self, mock_auth_context):
# assert
self.assertEqual([], subs)

@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile._get_cloud_console_token_endpoint', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder', autospec=True)
Expand Down Expand Up @@ -1186,7 +1186,7 @@ def __init__(self, *args, **kwargs):
self.assertEqual(s['id'], self.id1.split('/')[-1])
self.assertEqual(s['tenantId'], '54826b22-38d6-4fb2-bad9-b7b93a3e9c5a')

@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder', autospec=True)
def test_find_subscriptions_in_vm_with_msi_user_assigned_with_object_id(self, mock_subscription_finder, mock_get_client_class,
Expand Down
14 changes: 7 additions & 7 deletions src/azure-cli-core/azure/cli/core/tests/test_profile_v2016_06_01.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ def test_get_login_credentials_aux_subscriptions(self, mock_get_token, mock_read
self.assertEqual(mock_get_token.call_count, 2)

@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
def test_get_login_credentials_msi_system_assigned(self, mock_msi_auth, mock_read_cred_file):
mock_read_cred_file.return_value = []

Expand Down Expand Up @@ -602,7 +602,7 @@ def test_get_login_credentials_msi_system_assigned(self, mock_msi_auth, mock_rea
self.assertTrue(cred.token_read_count)

@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
def test_get_login_credentials_msi_user_assigned_with_client_id(self, mock_msi_auth, mock_read_cred_file):
mock_read_cred_file.return_value = []

Expand Down Expand Up @@ -633,7 +633,7 @@ def test_get_login_credentials_msi_user_assigned_with_client_id(self, mock_msi_a
self.assertTrue(cred.client_id, test_client_id)

@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
def test_get_login_credentials_msi_user_assigned_with_object_id(self, mock_msi_auth, mock_read_cred_file):
mock_read_cred_file.return_value = []

Expand Down Expand Up @@ -664,7 +664,7 @@ def test_get_login_credentials_msi_user_assigned_with_object_id(self, mock_msi_a
self.assertTrue(cred.object_id, test_object_id)

@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
def test_get_login_credentials_msi_user_assigned_with_res_id(self, mock_msi_auth, mock_read_cred_file):
mock_read_cred_file.return_value = []

Expand Down Expand Up @@ -753,7 +753,7 @@ def test_get_raw_token_for_sp(self, mock_get_token, mock_read_cred_file):
self.assertEqual(tenant, self.tenant_id)

@mock.patch('azure.cli.core._profile._load_tokens_from_file', autospec=True)
@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
def test_get_raw_token_msi_system_assigned(self, mock_msi_auth, mock_read_cred_file):
mock_read_cred_file.return_value = []

Expand Down Expand Up @@ -899,7 +899,7 @@ def test_find_subscriptions_thru_username_non_password(self, mock_auth_context):
# assert
self.assertEqual([], subs)

@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile._get_cloud_console_token_endpoint', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder', autospec=True)
Expand Down Expand Up @@ -1048,7 +1048,7 @@ def __init__(self, *args, **kwargs):
self.assertEqual(s['id'], self.id1.split('/')[-1])
self.assertEqual(s['tenantId'], '54826b22-38d6-4fb2-bad9-b7b93a3e9c5a')

@mock.patch('azure.cli.core._profile.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.adal_authentication.MSIAuthenticationWrapper', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder', autospec=True)
def test_find_subscriptions_in_vm_with_msi_user_assigned_with_object_id(self, mock_subscription_finder, mock_get_client_class,
Expand Down