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.

Sign up for GitHub

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

Add a test to catch regressions in cli run time #32612

Merged
merged 2 commits into from
Jul 15, 2023
Merged

Add a test to catch regressions in cli run time #32612

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af5b7ea
Add a test to catch regressions in cli run time
o-nikolas Jul 14, 2023
d34964c
Merge branch 'main' into onikolas/cli_time_test
o-nikolas Jul 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions tests/cli/test_cli_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import contextlib
import io
import re
import timeit
from collections import Counter
from unittest.mock import patch

Expand Down Expand Up @@ -268,3 +269,13 @@ def test_invalid_choice_raises_for_export_format_in_db_export_archived_command(
f"--export-format: invalid choice: '{export_format}' "
"(choose from 'csv'), see help above.\n"
)

def test_cli_run_time(self):
setup_code = "import subprocess"
timing_code = 'subprocess.run(["airflow", "--help"])'
# Limit the number of samples otherwise the test will take a very long time
num_samples = 3
threshold = 3.5
timing_result = timeit.timeit(stmt=timing_code, number=num_samples, setup=setup_code) / num_samples
# Average run time of Airflow CLI should at least be within 3.5s
assert timing_result < threshold
Comment on lines +278 to +281
Copy link
Member

Choose a reason for hiding this comment

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

Each airflow --help currently takes more than three seconds? That’s bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each command must complete in < less than 3.5 seconds. On my machine it usually takes are 2-2.5 seconds in rare cases spiking up to 3 if you CPU is under load for something, etc.

But yeah, the CLI module lives in the same code base as everything else, so to run it all of Airflow must be loaded (including the heavy hitters of conf/settings, etc)

Copy link
Member

@potiuk potiuk Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We should have different timings for different commands IMHO. And I agree with @uranusjr that if basic commands take > 3 seconds, this is wrong. Some of them ("config" that I worked on recently in #32604 need to initialize providers when it will be merged and I deliberately introduced lazy-loading the providers-manager for config in my change to only run it when actually needed. But maybe this is an indication that we still need to improve some of that . @uranusjr had done a lot of those lazy-loading things to make the time smaller for CLIs and some initialization, but 3 seconds for "most" commands seems really off.

Copy link
Contributor Author

@o-nikolas o-nikolas Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@potiuk @uranusjr I agree with all of this, but we can also agree that it is out of scope for this PR, right? I just want to establish some kind of threshold now in the most basic sense (i.e. just a simple --help) and we can have much more follow up work to optimize it and lower the threshold once we make more optimizations.

Copy link
Contributor

Choose a reason for hiding this comment

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

A different threshold for each command sounds great but also sounds like a lot of work and way more chances to break something for what feels like marginal benefit to me. Either way, I'd propose we get the test implemented then work on improvements and adjust the test thresholds (hopefully down/tighter) as improvements are implemented, right?

Copy link
Member

Choose a reason for hiding this comment

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

This is the matter of risk. How likely it is that this test will break things for people running regular PRs on public runners.

We do not know. We - for now - know that it seems to work on self-hosted. Let's try few times on public runners. I will add label and close/reopem to check.