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

Remove core Airflow support for static hybrid executors #47322

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

o-nikolas
Copy link
Contributor

Remove all the handholding and custom logic we have in core airflow which allows the use of static hybrid executors like LocalKubernetesExecutor and CeleryKubernetesExecutor. These executors will still work on 2.X versions of Airflow, but moving forward they will not be supported on Airflow 3


^ 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.

Remove all the handholding and custom logic we have in core airflow
which allows the use of static hybrid executors like
LocalKubernetesExecutor and CeleryKubernetesExecutor. These executors
will still work on 2.X versions of Airflow, but moving forward they will
not be supported on Airflow 3
@boring-cyborg boring-cyborg bot added area:CLI area:core-operators Operators, Sensors and hooks within Core Airflow area:Executors-core LocalExecutor & SequentialExecutor labels Mar 4, 2025
executor_constants.CELERY_KUBERNETES_EXECUTOR,
executor_constants.LOCAL_KUBERNETES_EXECUTOR,
}
IS_K8S_OR_K8SCELERY_EXECUTOR = conf.get("core", "EXECUTOR") == executor_constants.KUBERNETES_EXECUTOR
Copy link
Member

Choose a reason for hiding this comment

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

This variable name probably needs some work. Also probably means this should be a flag on the interface? Not sure what it actually controls on the ui though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @jedcunningham!

I decided to stop the scope of the change here, I'll explain why, but it's a fair call out.

I can update the name of the variable at least. But whether or not it requires an interface change I'd like to push back on. That was true even before these changes, all the way back to AIP-51 (here is the issue covering this coupling) and I don't think it should be contingent on this PR being merged.

It takes a lot of code to support it in the interface. We'd have to build a mechanism for executors to vend UI pages, and that's a lot of logic in a space where there's been lots of changes. K8s is the only executor that has a UI page. The solution I proposed the last couple times this has come up is to actually just remove this code altogether since I think that's the easiest path (and we love removing code for Airflow 3.0 😃 ). And I'm not sure how many folks really even make use of the UI endpoint to render the config file.

I'd love to hear your thoughts on that. If it sounds agreeable I'll open a separate 3.0 PR for that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI area:core-operators Operators, Sensors and hooks within Core Airflow area:Executors-core LocalExecutor & SequentialExecutor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants