feat: add callback support to aws batch executor#62984
feat: add callback support to aws batch executor#62984dondaum wants to merge 1 commit intoapache:mainfrom
Conversation
| fallback=CONFIG_DEFAULTS[AllBatchConfigKeys.MAX_SUBMIT_JOB_ATTEMPTS], | ||
| ) | ||
|
|
||
| def queue_workload(self, workload: workloads.All, session: Session | None) -> None: |
There was a problem hiding this comment.
If you don't mind can you narrow down the type for workload here? I think there is a new type that @ferruzzi made basically anywhere you need task | callback as a type.
There was a problem hiding this comment.
Thank you for the review.
Are you referring to types ExecuteTask and ExecuteCallback? https://github.com/apache/airflow/blob/main/airflow-core/src/airflow/executors/workloads/__init__.py
I think we can only use it if we pin the provider's Airflow core dependency to when this change was implemented. This happened here #61153.
If we want to maintain backwards compatibility, I can't use the new types.. Currently the Amazon provider is apache-airflow>=2.11.0
There was a problem hiding this comment.
@onikolas is likely thinking of SchedulerWorkload from workloads/types.py, but @dondaum may be right, that isn't going to be in 2.11 so we can't use that until we bunmp the min_ver.
I think we could do a conditional import though:
workload_type_hint = workloads.All
if airflow version > 3.2:
from airflow.executors.workloads.types import SchedulerWorkload
workload_type_hint = SchedulerWorkloadAnd that will force it to get cleaned up later when we pin the versions up??
There was a problem hiding this comment.
Let's say we make this conditional input work here. We are still limited by the BaseExecutor type having this
def queue_workload(self, workload: workloads.All, session: Session) -> None:
...There was a problem hiding this comment.
Since #62645 is now merged, I can narrow down the types.
There was a problem hiding this comment.
What is the state of this thread? Is it still open or can we resolve this one?
There was a problem hiding this comment.
Thanks for another look at this.
In my opinion we can solve it.
The relevant comment can be found at https://github.com/apache/airflow/pull/63657/changes#r3113924410.
|
@dondaum A few things need addressing before review — see our Pull Request quality criteria.
No rush. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
571de3f to
c591fbe
Compare
I changed the reference from task to workload where I could make a change. There is a method |
67332d4 to
00c71ae
Compare
639de36 to
a651768
Compare
a651768 to
f5a8db8
Compare
f5a8db8 to
3867506
Compare
Implements executor callback support for the AWS BatchExecutor.
related: #62887
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.