Add no-op _process_workloads to EdgeExecutor to improve readability#64236
Conversation
Nataneljpwd
left a comment
There was a problem hiding this comment.
Great addition!
It enhanced the readability and ease of understanding of the executor!
jscheffl
left a comment
There was a problem hiding this comment.
I am wondering a bit though why this internal method is expected to be implemented,.
Can you please adjust also the documentation in https://airflow.apache.org/docs/apache-airflow/stable/core-concepts/executor/index.html#important-baseexecutor-methods - because it is not mentioned there and I am wondering why the base executor raises an exception here. I do not see why (all) executors need to implement it. I would consider it beed to be changes in Base class rather if it is optional.
|
@jscheffl airflow/airflow-core/src/airflow/executors/base_executor.py Lines 265 to 275 in 28f7cf8 It's defined as something subclasses must implement. The reason it's enforced is that all objects inheriting from BaseExecutor are assumed to follow the pattern of queuing tasks into an internal queue via EdgeExecutor is the only case (so far) that doesn't follow this pattern. However, as long as it inherits from BaseExecutor, I believe it should still implement To be precise, I think BaseExecutor should ideally be split — something like |
And therefore I wrote earlier: Can you please adjust also the documentation in https://airflow.apache.org/docs/apache-airflow/stable/core-concepts/executor/index.html#important-baseexecutor-methods - because it is not mentioned there |
|
added! |
jscheffl
left a comment
There was a problem hiding this comment.
Thanks for adding the docs!
|
Hi maintainer, this PR was merged without a milestone set.
|
Backport successfully created: v3-2-testNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
|
Backport successfully created: v3-1-testNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
|
…pache#64236) * Add no-op _process_workloads to EdgeExecutor to improve EdgeExecutor readability * add docs
…pache#64236) * Add no-op _process_workloads to EdgeExecutor to improve EdgeExecutor readability * add docs
Why?
Unlike other Executors, EdgeExecutor does not process workloads in
trigger_tasks, so_process_workloadswas not overridden. However, this can cause confusion for people (myself included) who are accustomed to other executors when they first encounter it.I've implemented an explicit no-op implementation with a docstring explaining why the method is intentionally unused, to prevent confusion for future contributors and avoid a potential NotImplementedError if the base class flow ever changes.
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.