Revert "{Core} using threads to build command table for performance" #32694
Conversation
DanielMicrosoft
requested review from
bebound,
calvinhzy,
jiasli,
jsntcy,
kairu-ms,
necusjz and
zhoxing-ms
as code owners
🔄AzureCLI-FullTest
|
|
Hi @DanielMicrosoft, |
️✔️AzureCLI-BreakingChangeTest
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
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>
|
microsoft-github-policy-service
bot
added
Auto-Assign
There was a problem hiding this comment.
Pull request overview
This PR reverts PR #32518, which introduced threading for building the command table. The revert removes the parallel module loading implementation and returns to sequential module loading for better reliability and stability.
Changes:
- Removed threading infrastructure (ThreadPoolExecutor, ModuleLoadResult class, and related methods) from command table loading
- Reverted to sequential module loading in
MainCommandsLoader.load_command_table - Updated function signatures to remove the third return value (command_loader) from
_load_command_loader
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/azure-cli-core/azure/cli/core/init.py | Removed threading imports, constants, and helper classes/methods; reverted to sequential module loading logic |
| src/azure-cli-core/azure/cli/core/commands/init.py | Updated _load_command_loader to return 2 values instead of 3, moved cmd_to_loader_map population into the function |
| src/azure-cli-core/azure/cli/core/tests/test_parser.py | Updated mock to match new function signature |
| src/azure-cli-core/azure/cli/core/tests/test_command_registration.py | Updated mock and test assertions to match new function signature and behavior |
| src/azure-cli-core/azure/cli/core/tests/test_command_table_integrity.py | Removed test file that was specific to threading implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| group_table = command_loader.command_group_table if command_loader else {} | ||
| return command_table, group_table, command_loader | ||
| return command_table, command_loader.command_group_table |
There was a problem hiding this comment.
The variable command_loader is only defined inside the if loader_cls: block (line 1139), but is referenced unconditionally in the return statement at line 1152. If loader_cls is None (when the module doesn't have COMMAND_LOADER_CLS or get_command_loader), this will raise an UnboundLocalError.
The correct fix should initialize command_loader = None before the if statement, and then return an empty dictionary for the command_group_table when command_loader is None, similar to what was removed:
group_table = command_loader.command_group_table if command_loader else {}
return command_table, group_table| cmd_tbl = loader.load_command_table(["extra", "extra", "positional_argument"]) | ||
| self.assertDictEqual(INDEX[CommandIndex._COMMAND_INDEX], self.expected_command_index) | ||
| self.assertSetEqual(set(cmd_tbl), {'hello mod-only', 'hello overridden', 'extra final', 'hello ext-only'}) | ||
| self.assertEqual(list(cmd_tbl), ['hello mod-only', 'hello overridden', 'extra final', 'hello ext-only']) |
There was a problem hiding this comment.
Changing from assertSetEqual(set(cmd_tbl), ...) to assertEqual(list(cmd_tbl), ...) makes the test depend on the insertion order of commands in the dictionary. While Python 3.7+ guarantees dict order preservation, this change makes the test more brittle and less robust to implementation changes that might affect the loading order of commands. Unless there's a specific requirement to test the ordering, using assertSetEqual (or assertEqual(set(cmd_tbl), set(...))) would be more appropriate for testing command presence regardless of order.
| cmd_tbl = loader.load_command_table(["hello", "mod-only", "positional_argument"]) | ||
| self.assertDictEqual(INDEX[CommandIndex._COMMAND_INDEX], self.expected_command_index) | ||
| self.assertSetEqual(set(cmd_tbl), {'hello mod-only', 'hello overridden', 'hello ext-only'}) | ||
| self.assertEqual(list(cmd_tbl), ['hello mod-only', 'hello overridden', 'hello ext-only']) |
There was a problem hiding this comment.
Changing from assertSetEqual(set(cmd_tbl), ...) to assertEqual(list(cmd_tbl), ...) makes the test depend on the insertion order of commands in the dictionary. While Python 3.7+ guarantees dict order preservation, this change makes the test more brittle and less robust to implementation changes that might affect the loading order of commands. Unless there's a specific requirement to test the ordering, using assertSetEqual (or assertEqual(set(cmd_tbl), set(...))) would be more appropriate for testing command presence regardless of order.
| module_command_table.update(command_table) | ||
| loader.loaders.append(command_loader) # this will be used later by the load_arguments method | ||
| return module_command_table, command_loader.command_group_table, command_loader | ||
| return module_command_table, command_loader.command_group_table |
There was a problem hiding this comment.
The variable command_loader may be undefined if the for loop doesn't execute (e.g., if command_loaders is empty). While the current test setup ensures the loop always executes, this is fragile. Consider initializing command_loader = None before the loop and returning an empty dict for command_group_table when it's None, similar to the fix needed in the main implementation.
| module_command_table.update(command_table) | ||
| loader.loaders.append(command_loader) # this will be used later by the load_arguments method | ||
| return module_command_table, command_loader.command_group_table, command_loader | ||
| return module_command_table, command_loader.command_group_table |
There was a problem hiding this comment.
The variable command_loader may be undefined if the for loop doesn't execute (e.g., if no loader matches the name filter or if command_loaders is empty). While the current test setup ensures at least one loader is processed, this is fragile. Consider initializing command_loader = None before the loop and returning an empty dict for command_group_table when it's None, similar to the fix needed in the main implementation.
| command_table = command_loader.load_command_table(args) | ||
| if command_table: | ||
| for cmd in list(command_table.keys()): | ||
| # TODO: If desired to for extension to patch module, this can be uncommented |
There was a problem hiding this comment.
Grammar issue in TODO comment. The phrase "If desired to for extension to patch module" should be corrected to either "If desired for extension to patch module" or "If it's desired to allow extension to patch module".
| # TODO: If desired to for extension to patch module, this can be uncommented | |
| # TODO: If desired for extension to patch module, this can be uncommented |
Reverts #32518