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.

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

Improve performance of package load and command execution #2819

Merged
merged 63 commits into from Apr 11, 2017

Conversation

Projects
None yet
@johanste
Copy link
Member

johanste commented Apr 10, 2017


This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • The PR has modified HISTORY.rst with an appropriate description of the change (see Modifying change log).

Command Guidelines

  • [N/A] Each command and parameter has a meaningful description.
  • [N/A] Each new command has a test.

(see Authoring Command Modules)

Delay load azure modules as much as possible to improve startup time.

  • Exception and return value handling in general execution path now avoids loading msrest*, azure-mgmt-* as well azure- modules. Loading any of said modules bring in expensive dependencies such as requests.
  • Minimize the number of commands that are fed into argparse.ArgumentParser. This caused execution of az (without parameters) to be very slow, particularly on systems with less powerful CPUs.

johanste and others added some commits Apr 4, 2017

- Moved previously dead command filter from parser to application con…
…figuration.

- Removed unused configuration object/argv on application create.
Update test docs for running individual test and all tests in mod (#2763
)

* Update test docs for running individual test and all tests in mod

* Made feedback changes
Make argument parameters match up. (#2717)
Make lock command parameter aliases match up with resource commands.
[DevTestLabs] Adding scenario test to create simple Linux + Windows V…
…M in lab (#2767)

* WIP create linux + Windows vm in lab

* Adding recording
Add some more error checking/handling. (#2768)
Add more validation to resolve "lock level" for lock commands.
core: apply configured defaults on optional arg (#2770)
* Core:apply configured defaults on optional argument

* add a test

* add tests

* update history doc

* address review feedback
Fix doc references to azure.cli.commands (#2740)
* Fix doc references to azure.cli.commands

This module has moved to azure.cli.core.commands

* Fix PyLint
Add clearer guidelines on modifying changelog (#2739)
* Add clearer guidelines on modifying changelog

* A few smaller changes

* another small format change

* Code review changes
ACS Update: nulling out the windows profile so that there isn't a val…
…idation fail… (#2764)

* nulling out the windows profile so that there isn't a valdiation failure for missing password

ACS doesn't return a password on GET. az acs scale command does a GET
then PUT, but since ACS doesn't return the password the verification is
failing before the PUT is sent to ACS.

There is a bug in ACS this exposes. So this shouldn't be merged until
after the ACS rollout finishes. Should be about start of next week.

* updating history

* updating version in history

* removing white space added by editor
[Compute] Fix issues with VMSS and VM availability set update. (#2773)
* Fix issues with VMSS and VM availability set update.

* Update help. Fix #2762.

@johanste johanste requested a review from xingwu1 Apr 10, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 10, 2017

Codecov Report

Merging #2819 into master will increase coverage by 0.04%.
The diff coverage is 67.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2819      +/-   ##
==========================================
+ Coverage   62.89%   62.94%   +0.04%     
==========================================
  Files         480      481       +1     
  Lines       26012    26074      +62     
  Branches     3946     3956      +10     
==========================================
+ Hits        16361    16412      +51     
- Misses       8627     8634       +7     
- Partials     1024     1028       +4
Impacted Files Coverage Δ
...re-cli-iot/azure/cli/command_modules/iot/custom.py 82.75% <ø> (-0.1%) ⬇️
...rofile/azure/cli/command_modules/profile/custom.py 32.39% <0%> (-2.82%) ⬇️
...gure/azure/cli/command_modules/configure/custom.py 15.31% <0%> (-0.91%) ⬇️
...i-batch/azure/cli/command_modules/batch/_params.py 100% <100%> (ø) ⬆️
...rage/azure/cli/command_modules/storage/commands.py 98.77% <100%> (ø) ⬆️
...tch/azure/cli/command_modules/batch/_validators.py 75.38% <100%> (+7.51%) ⬆️
...li-role/azure/cli/command_modules/role/commands.py 86.95% <100%> (ø) ⬆️
...orage/azure/cli/command_modules/storage/_params.py 87.43% <100%> (+0.03%) ⬆️
src/azure-cli-core/azure/cli/core/parser.py 81.37% <100%> (ø) ⬆️
...azure/cli/command_modules/storage/_command_type.py 100% <100%> (ø) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de6fe79...dec0001. Read the comment docs.

@tjprescott
Copy link
Member

tjprescott left a comment

LGTM

@@ -852,6 +854,45 @@ def _load_transformed_arguments(self, handler):
help=docstring))


def validate_client_parameters(namespace):
"""Retrieves Batch connection parameters from environment variables"""
from azure.mgmt.batch import BatchManagementClient

This comment has been minimized.

Copy link
@tjprescott

tjprescott Apr 10, 2017

Member

Is this just relocated or is it refactored? If so, might want @annatisch to review.

This comment has been minimized.

Copy link
@johanste

johanste Apr 10, 2017

Author Member

It is just relocated. Adding @annatisch as a reviewer either way. Good call!

@johanste johanste requested a review from annatisch Apr 10, 2017

@derekbekoe

This comment has been minimized.

Copy link
Member

derekbekoe commented Apr 10, 2017

FYI @oakeyc this may impact you also.

@derekbekoe
Copy link
Member

derekbekoe left a comment

LGTM!

@annatisch
Copy link
Contributor

annatisch left a comment

Seems okay to me :)

@tjprescott tjprescott merged commit 4c6cc97 into Azure:master Apr 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@johanste johanste deleted the johanste:command_package_load branch Oct 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.