[GitHub] [airflow] o-nikolas commented on issue #28276: AIP - 51 Make CeleryKubernetesExecutor extends BaseExecutor for a common API.
o-nikolas commented on issue #28276: URL: https://github.com/apache/airflow/issues/28276#issuecomment-1420196980 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] o-nikolas commented on issue #28276: AIP - 51 Make CeleryKubernetesExecutor extends BaseExecutor for a common API.
o-nikolas commented on issue #28276:
URL: https://github.com/apache/airflow/issues/28276#issuecomment-1371553409
> 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](https://github.com/apache/airflow/issues/28276#issuecomment-1344899475)
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 :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] o-nikolas commented on issue #28276: AIP - 51 Make CeleryKubernetesExecutor extends BaseExecutor for a common API.
o-nikolas commented on issue #28276: URL: https://github.com/apache/airflow/issues/28276#issuecomment-1352590074 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] o-nikolas commented on issue #28276: AIP - 51 Make CeleryKubernetesExecutor extends BaseExecutor for a common API.
o-nikolas commented on issue #28276: URL: https://github.com/apache/airflow/issues/28276#issuecomment-1345405031 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] o-nikolas commented on issue #28276: AIP - 51 Make CeleryKubernetesExecutor extends BaseExecutor for a common API.
o-nikolas commented on issue #28276: URL: https://github.com/apache/airflow/issues/28276#issuecomment-1344899475 > 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: 1. 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?**) 1. 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) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] o-nikolas commented on issue #28276: AIP - 51 Make CeleryKubernetesExecutor extends BaseExecutor for a common API.
o-nikolas commented on issue #28276: URL: https://github.com/apache/airflow/issues/28276#issuecomment-1344854419 > @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 :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
