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

Move Pod*Exceptions to separate module to avoid unnecessary slow imports in CLI #45759

Merged
merged 1 commit into from
Jan 18, 2025

Conversation

IKholopov
Copy link
Contributor

Pod*Exceptions has been moved from core to providers back in de92a81, but kept in airflow/exceptions.py under try-case for backwards compatibility and allow for usage of an older k8s provider with a newer core airflow.

This resulted in 2 unintended side-effects:

  1. Any CLI command started taking ~ 1s longer to start. That is because importing anything from airflow resulted in importing kubernetes client. It is effectively the most expensive import out of all CLI does for any command, even though it is used by very few commands. Here are the timings of a trivial airflow dag-processor --help
  1. Some time ago the compatibility import broke altogether. Because of the amount of imports in pod_generator.py, the import from exceptions.py failed even if providers were up to date with recursive import. But it was caught by exception handling, resulting in from airflow.exceptions import PodMutationHookException and from airflow.providers.cncf.kubernetes.pod_generator import PodMutationHookException pointing to different classes defeating the purpose of the fallback.

This PR addresses both by moving PodGenerator exceptions to the separate module that only import its base class. This keeps imports backwards compatible, doesn't attempt to load k8s modules and fixes the divergence of exception classes in airflow.providers.cncf.kubernetes.pod_generator and airflow.exceptions.

It is worth noting, that all usages of those exceptions in core Airflow also has been cleaned up since then, so if you believe that we need to remove those exceptions from airflow/exceptions.py, please let me know.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:providers provider:cncf-kubernetes Kubernetes provider related issues labels Jan 17, 2025
@IKholopov IKholopov marked this pull request as ready for review January 17, 2025 18:13
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Nice one! Love the timing results

@potiuk potiuk added the full tests needed We need to run full set of tests for this PR to merge label Jan 17, 2025
@potiuk
Copy link
Member

potiuk commented Jan 17, 2025

I applied "full tests needed" and rebased it - I think that one should run full suite of tests due to potential compatibility issues.

@IKholopov
Copy link
Contributor Author

Looks like the only failure is unrelated one (already failing on parent commit, same failure as here: #45727 (comment) - providers/tests/common/sql/hooks/test_dbapi.py::TestDbApiHook::test_run_no_log ).

Copy link
Contributor

@o-nikolas o-nikolas left a comment

Choose a reason for hiding this comment

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

Ahh, I love these kinds of PRs :)

@kaxil kaxil merged commit 702c0e0 into apache:main Jan 18, 2025
90 of 91 checks passed
@potiuk
Copy link
Member

potiuk commented Jan 18, 2025

Added an issue for the flaky stuff #45774

@potiuk
Copy link
Member

potiuk commented Jan 18, 2025

It looks like another "caplog issue"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers full tests needed We need to run full set of tests for this PR to merge provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants