Skip to content

Improve speed to run airflow by 6x#21438

Merged
ashb merged 3 commits into
apache:mainfrom
astronomer:speed-up-cli-importtime
Feb 9, 2022
Merged

Improve speed to run airflow by 6x#21438
ashb merged 3 commits into
apache:mainfrom
astronomer:speed-up-cli-importtime

Conversation

@ashb

@ashb ashb commented Feb 8, 2022

Copy link
Copy Markdown
Member

By delaying expensive/slow imports to where they are needed, this gets airflow printing it's usage information in under 0.5s, down from almost 3s which makes it feel much much snappier.

I can't really think of any way of adding tests to prevent regressions here. Ideas welcome.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

By delaying expensive/slow imports to where they are needed, this gets
`airflow` printing it's usage information in under 0.8s, down from almost
3s which makes it feel much much snappier.
Comment thread airflow/exceptions.py
status_code = 400


class AirflowNotFoundException(AirflowException, ApiConnexionNotFound):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have a feeling that this might break something. Lets see.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here. Let's see :)

@ashb

ashb commented Feb 8, 2022

Copy link
Copy Markdown
Member Author

This was profiled using python -X importtime -m airflow and then looking for high cumulative modules times (3rd column)

@ashb ashb changed the title Improve speed to run airflow by 3x Improve speed to run airflow by 6x Feb 8, 2022
@ashb
ashb requested a review from potiuk February 8, 2022 21:48
log = logging.getLogger(__name__)

if TYPE_CHECKING:
from airflow.executors.base_executor import BaseExecutor

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

By moving this here it prevents loading all the models and saves a goodly chunk of time.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fantastic!

@potiuk

potiuk commented Feb 8, 2022

Copy link
Copy Markdown
Member

I love the "perceived speed" factor. You can't imagine how many tricks mobile apps do to load "percivably faster". And to be perfectly hones this is one of the most important factors that improve "quality" of the apps.

@ashb

ashb commented Feb 8, 2022

Copy link
Copy Markdown
Member Author

(This was totally not what I "meant" to be work on tonight. But sometimes you have to roll with it)

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's see - but it looks good.

BTW. I have a vague feeling this is really a band-aid and we need to refactor the modules to avoid local imports altogether. For me the need to add "TYPE_CHECKING" and local imports is a sign we have the structure of "airflow" package wrong.

However it is likely unsolvable until airfliow as it might lead to breaking compatibility, so for now it is "good enoug".

@github-actions github-actions Bot added the full tests needed We need to run full set of tests for this PR to merge label Feb 8, 2022
@github-actions

github-actions Bot commented Feb 8, 2022

Copy link
Copy Markdown
Contributor

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@ashb

ashb commented Feb 8, 2022

Copy link
Copy Markdown
Member Author

Let's see - but it looks good.

BTW. I have a vague feeling this is really a band-aid and we need to refactor the modules to avoid local imports altogether. For me the need to add "TYPE_CHECKING" and local imports is a sign we have the structure of "airflow" package wrong.

However it is likely unsolvable until airfliow as it might lead to breaking compatibility, so for now it is "good enoug".

Yeah no disagreement from me -- and primarirly one of the thngs I have always not liked about the code is the number of side-effects we get from just import airflow

@ashb
ashb merged commit 1a8a897 into apache:main Feb 9, 2022
@ashb
ashb deleted the speed-up-cli-importtime branch February 9, 2022 08:59
@jedcunningham jedcunningham added the type:improvement Changelog: Improvements label Mar 1, 2022
@potiuk

potiuk commented Mar 28, 2022

Copy link
Copy Markdown
Member

@ashb - seems like you've been ahead of our users reporting this: #21802 (reply in thread) (connected to the fact that someone's filesystem is SLOW)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:logging area:serialization full tests needed We need to run full set of tests for this PR to merge type:improvement Changelog: Improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants