{Core} using threads to build command table for performance #32518
Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
bot
requested review from
jiasli and
yonzhan
microsoft-github-policy-service
bot
added
Auto-Assign
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
DanielMicrosoft
requested review from
bebound,
calvinhzy,
evelyn-ys,
jsntcy,
kairu-ms,
necusjz and
zhoxing-ms
as code owners
There was a problem hiding this comment.
Pull request overview
This PR introduces concurrent module loading using ThreadPoolExecutor to improve Azure CLI command table build performance. The changes parallelize the expensive operation of loading command modules, which is currently a major bottleneck.
Key changes:
- Command modules are now loaded concurrently using threads with a configurable timeout and worker count
- Module loading logic has been refactored into smaller methods for better organization
- A new test ensures command table integrity after the threading changes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/azure-cli-core/azure/cli/core/init.py | Implements ThreadPoolExecutor-based concurrent module loading with timeout protection, refactors module loading into separate methods (_load_modules, _load_single_module, _process_successful_load, etc.), and adds configuration constants for timeout and thread count |
| src/azure-cli-core/azure/cli/core/tests/test_command_table_integrity.py | Adds integration test to verify command table integrity, checking for duplicates, core command groups, structural integrity, and proper command source attribution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import traceback | ||
| try: | ||
| start_time = timeit.default_timer() | ||
| module_command_table, module_group_table = _load_module_command_loader(self, args, mod) |
There was a problem hiding this comment.
Is there a risk of race conditions when loading command_loader with multiple threads? How can we ensure that all loaders are properly registered and that each command is correctly mapped to its corresponding loader?
There was a problem hiding this comment.
great catch @MoChilia , I believe I have addressed those concerns by changing the setup to have the threads only return a result, and the parent thread only will do the object mutation.
Interested in your feedback 👍
DanielMicrosoft
force-pushed
the
35856522-improve-command-index-perf
branch
from
de7a573 to
8b414bd
Compare
| cmd_tbl = loader.load_command_table(["extra", "extra", "positional_argument"]) | ||
| self.assertDictEqual(INDEX[CommandIndex._COMMAND_INDEX], self.expected_command_index) | ||
| self.assertEqual(list(cmd_tbl), ['hello mod-only', 'hello overridden', 'extra final', 'hello ext-only']) | ||
| self.assertSetEqual(set(cmd_tbl), {'hello mod-only', 'hello overridden', 'extra final', 'hello ext-only'}) |
There was a problem hiding this comment.
As commands are loaded non-deterministically now, the order of them when converting a dict to a list may change, so changing to a set here, afaik this is not functionally important (as commands are invoked by key, not by order of registation). So changing to a set here is fine.
There are various scenarios that will trigger the rebuild:
We have added a telemetry property to track when a rebuild happens: Around 30% of |
|
The PR title can be improved:
- {Core} using threads to build command table for performance
+ {Core} Use multi-threading to build command index to improve performance |
| self.loaders.append(result.command_loader) | ||
|
|
||
| for cmd in result.command_table: | ||
| self.cmd_to_loader_map[cmd] = [result.command_loader] |
There was a problem hiding this comment.
Hi @DanielMicrosoft, I think this might not be working for extensions
In azure-cli-extensions repo I am failing with an error indicating cmd_to_loader_map is not populated for my extension commands:
=============================
| Export Command Table Meta |
=============================
Initializing with loading command table...
Commands loaded in 12.974852085113525 sec
ERROR: 'fleet create'
Traceback (most recent call last):
File "/home/runner/work/azure-cli-extensions/azure-cli-extensions/env/lib/python3.11/site-packages/knack/cli.py", line 233, in invoke
cmd_result = self.invocation.execute(args)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/azure-cli-extensions/azure-cli-extensions/env/lib/python3.11/site-packages/knack/invocation.py", line 224, in execute
cmd_result = parsed_args.func(params)
^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/azure-cli-extensions/azure-cli-extensions/env/lib/python3.11/site-packages/knack/commands.py", line 149, in __call__
return self.handler(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/azure-cli-extensions/azure-cli-extensions/env/lib/python3.11/site-packages/knack/commands.py", line 256, in _command_handler
result = op(client, **command_args) if client else op(**command_args)
^^^^^^^^^^^^^^^^^^
File "/home/runner/work/azure-cli-extensions/azure-cli-extensions/env/lib/python3.11/site-packages/azdev/operations/command_change/__init__.py", line 115, in export_command_meta
module_loader = command_loader.cmd_to_loader_map[command_name]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
KeyError: 'fleet create'
Here is my build https://github.com/Azure/azure-cli-extensions/actions/runs/21294652209/job/61297205851?pr=9511
and here is my PR link: Azure/azure-cli-extensions#9511
There was a problem hiding this comment.
I reverted this commit in my local azure-cli copy and ran azdev command-change meta-export fleet it works. But with this commit it doesn't and show above error
Similar thing happens for azdev linter fleet --min-severity medium in azure-cli-extensions repo
…zure#32518)" This reverts commit 6a70dae.
6 participants
Related command
Description
ADO task: https://msazure.visualstudio.com/One/_workitems/edit/35856522
This PR aims to speed up the time it takes to re-build the commandIndex. This is currently a major bottleneck to performance.
The most expensive operations in that code path have had concurrency applied using threads (where possible).
Testing scenarios:
az help,version,upgrade, etc).vm create, etc).commandIndex.json(when it doesnt exist)commandIndex.json(PATCH)az helpcorrectly generates command tree._update_command_table_from_modulesis unchangedTesting Guide
History Notes
[Component Name 1] BREAKING CHANGE:
az command a: Make some customer-facing breaking change[Component Name 2]
az command b: Add some customer-facing featureThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.