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

{Misc} Fix timer for __main__.py #13844

Merged
merged 3 commits into from
Jun 8, 2020
Merged

{Misc} Fix timer for __main__.py #13844

merged 3 commits into from
Jun 8, 2020

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Jun 5, 2020

Description

Fix the incorrect timing logic. Previous it only takes invoke duration into account, while forgetting about the init duration.

This PR fixes the timing logic to include both init and invoke duration.

Testing Guide

Before:

> az version -h --verbose
...
command ran in 0.881 seconds.

After:

> az version -h --verbose
...
Command ran in 1.679 seconds (init: 0.824, invoke: 0.855)

@yonzhan yonzhan requested review from qianwens and arrownj June 5, 2020 10:36
@yonzhan yonzhan added this to the S171 milestone Jun 5, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Jun 5, 2020

add to S171

@jiasli
Copy link
Member Author

jiasli commented Jun 8, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jiasli
Copy link
Member Author

jiasli commented Jun 8, 2020

Because azdev doesn't honor azure-cli repo's .flake8 (Azure/azure-cli-dev-tools#209), disabling E402 doesn't work in the CI and causes the failure:

/home/vsts/work/1/s/src/azure-cli/azure/cli/__main__.py:13:1: E402 module level import not at top of file
/home/vsts/work/1/s/src/azure-cli/azure/cli/__main__.py:14:1: E402 module level import not at top of file
/home/vsts/work/1/s/src/azure-cli/azure/cli/__main__.py:16:1: E402 module level import not at top of file
/home/vsts/work/1/s/src/azure-cli/azure/cli/__main__.py:17:1: E402 module level import not at top of file
/home/vsts/work/1/s/src/azure-cli/azure/cli/__main__.py:18:1: E402 module level import not at top of file
/home/vsts/work/1/s/src/azure-cli/azure/cli/__main__.py:19:1: E402 module level import not at top of file
6     E402 module level import not at top of file

Thus, per-file-ignores won't work wither.

Since there is no support for file-level # flake8: noqa to disable certain errors (https://gitlab.com/pycqa/flake8/-/issues/399), temporarily comply to flake8's E402 rule to make the CI pass.

logger.info("command ran in %.3f seconds.", elapsed_time)
# Log the invoke finish time
invoke_finish_time = timeit.default_timer()
logger.info("Command ran in %.3f seconds (init: %.3f, invoke: %.3f)",
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if this running time info should be moved to logger.debug. I am open to this.

Copy link
Contributor

@haroldrandom haroldrandom left a comment

Choose a reason for hiding this comment

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

Would it better to calculate the execution time under certain situations such as debug mode, --verbose mode, etc.

But this modifcation doesn't bring any (significant) regression, So, LGTM.

@jiasli
Copy link
Member Author

jiasli commented Jun 8, 2020

Would it better to calculate the execution time under certain situations such as debug mode, --verbose mode, etc.

But this modifcation doesn't bring any (significant) regression, So, LGTM.

The execution time is calculated regardless of the execution mode (verbose, debug). --verbose and --debug only decide if the execution time is printed.

Anyway, the overhead of calling timeit.default_timer() is pretty small. So we don't need to worry about it.

@jiasli jiasli changed the title {__main__} Fix timer {Misc} Fix timer for __main__.py Jun 8, 2020
@jiasli jiasli merged commit 9d76b7a into Azure:dev Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants