-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
{Core} Build and use command index #13294
Conversation
add to S169 |
@@ -305,6 +304,7 @@ def assemble_json(ids): | |||
if full_id_list: | |||
setattr(namespace, '_ids', full_id_list) | |||
|
|||
from msrestazure.tools import parse_resource_id, is_valid_resource_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should move to azure-mgmt-core, since it's where future update will be. This is the same code I copy pasted without any changes:
from azure.mgmt.core.tools import parse_resource_id, is_valid_resource_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@jsntcy Could you help address this, and do necessary tests in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the change in this PR already.
@@ -47,6 +43,8 @@ def handle_exception(ex): # pylint: disable=too-many-return-statements | |||
from msrestazure.azure_exceptions import CloudError | |||
from msrest.exceptions import HttpOperationError, ValidationError, ClientRequestError | |||
from azure.cli.core.azlogging import CommandLoggerContext | |||
from azure.common import AzureException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is only ONE SDK that is still using azure.common.AzureException
and it's storage. If that helps, you should be able to move that to storage data-plane specific commands (until you can move to full track2 storage)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, as what Laurent said, we are still using AzureExeption with existing track 1 storage commands and we cannot fully move to track 2 now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what I mean is that this import is not necessary in cli-core, if only on particular set of SDKs is using it (just trying to save some more :))
When an extension is removed, will the command index be updated or invalidated? |
This also drastically shortens the running time of CI. Previously CLI Automation Full Test takes 1h 4m 2s. Now it only takes 27m 36s. https://github.com/Azure/azure-cli/runs/648374336 |
Yes. In such case, the second check will fail and the index will be refreshed. https://github.com/Azure/azure-cli/blob/90c4ba188c87bec8ee634c63748d1dc4fac2be3b/src/azure-cli-core/azure/cli/core/__init__.py#L413 |
# Extensions that will always be loaded if installed. These extensions don't expose commands but hook into CLI core. | ||
ALWAYS_LOADED_EXTENSION_MODNAMES = ['azext_ai_examples', 'azext_ai_did_you_mean_this'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mirdaki, @christopher-o-toole, I have allowed azext_ai_examples
(Azure/azure-cli-extensions#1345) and azext_ai_did_you_mean_this
(Azure/azure-cli-extensions#1536) to be always loaded. Please member to change this const list in CLI core if you are going to change the namespace in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the extensions not installed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only a filter applied to installed extensions. If they are not installed, they will simply be ignored. https://github.com/Azure/azure-cli/blob/3632dc0fa87a4c9c928f204cfb61d448c304a208/src/azure-cli-core/azure/cli/core/__init__.py#L272-L279
cumulative_elapsed_time = 0 | ||
for mod in [m for m in installed_command_modules if m not in BLACKLISTED_MODS]: | ||
for mod in [m for m in command_modules if m not in BLACKLISTED_MODS]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove if m not in BLACKLISTED_MODS
as it is already done in line 224?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, but the index may be out-dated and contains a blocked mod. I will remove it from L224 instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I have changed the name BLACKLISTED_MODS
to the neutral BLOCKED_MODS
, as per https://bugs.chromium.org/p/chromium/issues/detail?id=981129
# Found modules from index | ||
logger.debug("Modules found from index for '%s': %s", top_command, index_modules) | ||
for m in index_modules: | ||
command_module_prefix = 'azure.cli.command_modules.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can move it outside the for loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Changed.
# Extensions that will always be loaded if installed. These extensions don't expose commands but hook into CLI core. | ||
ALWAYS_LOADED_EXTENSION_MODNAMES = ['azext_ai_examples', 'azext_ai_did_you_mean_this'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the extensions not installed ?
add to S170 |
Where represent the 3rd point, |
How do we maintain the commandIndex.json? (and considering the features/objective we discussed at brain storm) |
We don't need to maintain it. It is transparent and gets updated totally automatically. |
index_version = INDEX[_COMMAND_INDEX_VERSION] | ||
if not (index_version and index_version == __version__): | ||
logger.debug("Command index version is invalid or outdated.") | ||
invalidate_command_index() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added version information in the command index.
{
"version": "2.7.0",
"commandIndex": {
"acr": [
"azure.cli.command_modules.acr"
],
CLI version may change when
- Users update or downgrade Azure CLI to another version.
- Users install multiple versions and use them alternately.
- This includes a dev environment configured side-by-side with the released version.
When a command is run with an incompatible version (invalid or mismatch), we manually call invalidate_command_index
to force an update to avoid all possible edge cases.
@fengzhou-msft to help verify this is the correct way to use __version__
.
@@ -295,15 +350,102 @@ def _get_extension_suppressions(mod_loaders): | |||
res.append(sup) | |||
return res | |||
|
|||
def _update_command_index(): | |||
start_time = timeit.default_timer() | |||
INDEX[_COMMAND_INDEX_VERSION] = __version__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_COMMAND_INDEX_VERSION
is set here.
# Set fallback=False to turn off command index in case of regression | ||
use_command_index = self.cli_ctx.config.getboolean('core', 'use_command_index', fallback=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the switch to disable command index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
A command index is built and saved in file
.azure/commandIndex.json
. It contains a mapping from top-level commands / command group names to their command module namespaces.Both built-in modules and extensions are included in the index:
CLI checks if a command can be found from the index (either from built-in modules, extensions, or both):
This significantly decreases the run time of commands, since unnecessary modules are not loaded anymore.
Together with other PRs, Azure CLI's run time are improved from 2.3s to 0.6s (
az version
, without the loading time ofpkg_resources
):Fix built-in timer ({Misc} Fix timer for __main__.py #13844): Previous it only takes
invoke
duration into account, while forgetting about theinit
duration. The timing logic is fixed to include bothinit
andinvoke
duration.Delay unnecessary imports ({Core} Delay
import
#13843): Previously during CLI initialization, importingazure.core
andmsrestazure.tools
are very time-consuming, but actually unnecessary. This PR delays the import untilazure.core
andmsrestazure.tools
are actually used.Remove unnecessary imports of msrest: Follow EAFP (Easier to Ask for Forgiveness than Permission) style to remove unnecessary imports of msrest. Also see https://stackoverflow.com/a/11360880/2199657
Add
util
module foraz rest
andaz version
: ({Util} Add util module for version and rest #13783): Peel offaz rest
andaz version
fromresource
module. A new moduleutil
is created. Thus they don't carry the burden of mgmt SDK initialization, thus making them faster.Testing Guide
Test with the built-in timer
First time run:
Second time run:
Test with PowerShell
Before:
After:
Test with Linux
Before:
After:
Profile the import time
az version -h
Before:
After:
az version
Before:
After:
Remark
pkg_resources
is still causing significant delay (0.272s) during the package initialization. That's where the extra time came from, compared to the built-in timer. This needs to be addressed separately in Support PEP 420 -- Implicit Namespace Packages #13293See Command line and environment for the usage of
python -X importtime
To quit
tuna
, on Window press Ctrl+Break, on Linux press Ctrl+C. See this stackoverflow thread