AIP - 51 Make CeleryKubernetesExecutor extends BaseExecutor for a common API.

Make the CeleryKubernetesExecutor extends the BaseExecutor. This would allow for a common API and not having to handle this particular executor differently.

A solution could be to rework a bit the BaseExecutor via property to allow more control for subclasses without breaking backward comp.

Or at least have some ancestor in common to be able to share some logic.

Committer

I acknowledge that I am a maintainer/committer of the Apache Airflow project.

@o-nikolas

@o-nikolas

@dstandish made me notice that. The consequence is that changes we bring to the BaseExecutor are not propagated to the CeleryKubernetesExecutor. (is_local property for instance, which is pretty counter intuitive and prone to error)

@dstandish

@dstandish made me notice that. The consequence is that changes we bring to the BaseExecutor are not propagated to the CeleryKubernetesExecutor. (is_local property for instance, which is pretty counter intuitive and prone to error)

Hey Pierre! Yeah, there is some overlap here. I don't love the way multiple executors are stuffed together. I have plans in the works for an AIP to properly support multiple executors, stay tuned for that :)

@o-nikolas

@o-nikolas,

Even better, can't wait!

Should I leave this open until your proposal come through, just to keep track of this specificity ? I can remove the AIP-51 if this is considered out of scope, let me know :)

@pierrejeambrun

Should I leave this open until your proposal come through, just to keep track of this specificity ? Maybe I can remove the AIP-51 if this is considered out of scope ?

We'll still need some short term solution which this ticket can capture? At least to fix what's been released so far (which I think are only the is_local and supports_ad_hoc_ti_run fields) and then whatever we decide here will need to be done for future PRs too.

Two approaches I see:

The fields that have been added/modified so far (e.g. is_local and supports_ad_hoc_ti_run) can be added directly to the executors which don't inherit from the base executor (e.g. CeleryKubernetesExecutor, LocalKubernetesExecutor, etc)
1. Pros: fewer code changes; encapsulates the fix to code that will eventually be deprecated/heavily modified.
2. Cons: only works for executors within Airflow core. Any custom 3rd party executors which don't inherit from BaseExecutor will also break once this code is released (what level of support and backwards compatibility do we owe these 3rd party executors?)
All the locations which use the new fields added to the baseExecutor (e.g. is_local and supports_ad_hoc_ti_run) should first check if the executor object has that attribute set, and if not, use the default from the BaseExceutor class (this is approximately how supports_ad_hoc_ti_run was originally implemented)
1. Pros: Will be backwards compatible for all executors both in Airflow core and public/3rd party executors
2. Cons: much uglier code and more brittle to maintain and eventually deprecate

WDYT @pierrejeambrun? (also cc: @potiuk @eladkal)

I think you resumed pretty well the options we have.

I naively went for 1) in a quickfix #28277 for is_local. I missed the LocalKubernetesExecutor (follow up pr #28288). For supports_ad_hoc_ti_run, both LocalKubernetesExecutor and CeleryKubernetesExecutor already implement it.

I don't have a lot of context/background on custom executor and how our users leverage them. I would say that for maintainability, having third party executor extends the base executor would be much easier. (and make sense to me). Otherwise we have to keep in mind that 'Any' type can be passed as an executor to airflow core, and this is pretty ugly to handle. (would require changing type and using 'hasattr' everywhere in the codebase where we have an 'Executor' type).

I would lean for 1 with a follow up task to make all core airflow executor extends the base class, and asking custom executor to also inherit this. (but the breaking change is a fair concern)

Yupp, I think that's the way I'm leaning as well. I'll hold off on reviewing those PRs and give some time for others to weigh in since it will change how we fix the inconsistency depending on the discussion here.

Edit: It looks like those are being reviewed/merged despite the open discussion here, so I suppose I'll review the remaining open PR.

@potiuk

Checking in on this one.

I'll leave it open for another day or so in case someone wants to weigh in . But after doing even more thinking I'm even more convinced we should go with option 1. Especially after reviewing and thinking about #28300 from @potiuk, the only part of executor logic that is explicitly public is the BaseExecutor interface. I don't think it's functionally possible for us to support any 3rd party executor which is not a child of this class (i.e. implementing its interface). So while it's possible that someone out there has written an executor which doesn't implement this interface and this project may break that and require them to move over to this public interface, I think it's a fair ask.

As dicussed in #28288 (comment), I think this functionality should be split to different properties of executor - each of them with much narrower meaning but eventually we (or those who implemented their own executor) should be able to choose the right combination of flags for the perticular executor they implement.

I am commenting here because I already merged #28288, but possibly the decision here might improve the merged code as well

The "is_local" flag is rather meaningless and does not reflect it's true meaning, simply because there are multiple things "is_local" flag tried to achieve:

serving logs - > I think this should be a "serve_logs" flag. There are several reasons why the executor would like to serve logs, and while for example Local Executor should not do that by default, but LocalCeleryExecutor should for example (if we have it). I propose to use serve_logs_by_worker property for that one.
Supporting standalone airflow - another place where is_local is used is to determine fallback choice when standalone Airflow is used. There are two options here: a) we only suport LocalExecutor/SequentialExecutor in Standalone airflow and if any other executor is used, we fallback to one of those. Or we allowe to mark custom executor as supporting "Standalone" mode. I would be for the latter option and for adding supports_standalone property. This is more versatile and allows (in the future) to promote production-ready "standalone airflow" with custom executor - I see some interesting use cases for that one actually - you could have a fully standalone airflow with UI and scheduler and Triggerer, while doing a fully remote execution of tasks elsewhere. This is very appealing to have a much simpler deployment of Airflow - where you would just need pip install airflow and single command to run the whole of Airflow.
determine if we need to copy a configuration - there is this check:

      if executor_class.is_local:
          cfg_path = tmp_configuration_copy()

Which I believe is only needed because of the impersonation - we need to copy a configuration file when backfilling. Since the backfill is done

And here as well I would propose to use new property freeze_configuration_on_backfill to control that one. I think there might be some executors that would like to do the same.

I think if we split is_local flag to those three values, we should be able to determine the right set of properties that each executor should have.

@potiuk

I think if we split is_local flag to those three values, we should be able to determine the right set of properties that each executor should have.

Hey @potiuk thanks for weighing in!

The original thread/discussion here was actually more around what to do about executors which do not implement the BaseExecutor class (either the composite executors we have or external 3rd party ones folks may have written), which causes issues because the base executor provides all the defaults for the new flags we're adding and if an executor does not implement the base class it will fail to run with the core Airflow code since that code expects all executors to know about all these new compatibility flags. This comment summarizes that issue pretty well and I think @pierrejeambrun and I are fairly comfortable with going with option one.

But as for your suggestion for more granular executor compatibility flags/properties I think this is a super interesting discussion! I originally made the flags to be as general as possible but still be compatible with the current core code behaviours ("current' as it was before the AIP-51 project), since I expected people to balk at the idea of having tens of these properties. But if you and others are in support of it, I'm actually more than happy to go with more granular flags. It will decrease the likelihood of future changes to the API.
I'll cut a new ticket to break apart is_local into multiple individual flags and add it to the project board 👍

I'm going to resolve this one since we settled on a short term approach to this and the long term fix is to support true/native hybrid executors (AIP incoming soon) and then deprecating these hand crafted ones. Until then we'll continue supporting them as we do now

Navigation Menu

Repository navigation

AIP - 51 Make CeleryKubernetesExecutor extends BaseExecutor for a common API. #28276

Committer

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

AIP - 51 Make CeleryKubernetesExecutor extends BaseExecutor for a common API. #28276

Description

Committer

Activity

pierrejeambrun commented on Dec 9, 2022

o-nikolas commented on Dec 9, 2022

pierrejeambrun commented on Dec 9, 2022

o-nikolas commented on Dec 9, 2022

pierrejeambrun commented on Dec 10, 2022

o-nikolas commented on Dec 10, 2022

o-nikolas commented on Dec 15, 2022

potiuk commented on Jan 2, 2023

o-nikolas commented on Jan 4, 2023

o-nikolas commented on Feb 7, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions