Skip to content

Add ExecuteCallback support to AWS ECS Executor - #63657

Merged
o-nikolas merged 11 commits into
apache:mainfrom
shivaam:feature/ecs-executor-callback
May 21, 2026
Merged

Add ExecuteCallback support to AWS ECS Executor#63657
o-nikolas merged 11 commits into
apache:mainfrom
shivaam:feature/ecs-executor-callback

Conversation

@shivaam

@shivaamshivaam commented Mar 15, 2026

Copy link
Copy Markdown
Contributor

Implements executor callback support for the AWS ECS Executor.

Sibling to merged Lambda PR #63035; builds on #62645 (merged) and #65392 (merged).

related: #62887

Changes

  • supports_callbacks = True gated on AIRFLOW_V_3_3_PLUS so base executor routes ExecuteCallback to ECS.
  • Widens key types from TaskInstanceKey to WorkloadKey in ecs_executor.py, EcsQueuedTask, EcsTaskCollection.
  • queue_workload() override branches on ExecuteTask vs ExecuteCallback (kept until min Airflow version reaches 3.3, after which BaseExecutor.queue_workload routes both natively; mirrors merged Lambda). No tracking issue yet — happy to open one for the override removal as a follow-up.
  • _process_workloads() dispatches both workload types via execute_async().
  • Renames methods/attrs to reflect workloads, with AirflowProviderDeprecationWarning shims for the public ones (matches merged Lambda):
    • public + shim: sync_running_taskssync_running_workloads, attempt_task_runsattempt_workload_runs, pending_taskspending_workloads
    • private (no shim): __update_running_task__update_running_workload, __handle_failed_task__handle_failed_workload
  • Relies on Widen BaseExecutor method signatures to accept WorkloadKey #65392 widening BaseExecutor signatures — no cast() needed, log_task_event() skips callback keys internally.

Tests

CI green: ruff, mypy, full test suite, plus compat suites on Airflow 2.11.1 / 3.0.6 / 3.1.8 / 3.2.0.

End-to-end re-verified 2026-05-02 against rebased main on real ECS Fargate. Each DAG runs a regular ExecuteTask (slow_task 120s) followed by a deadline ExecuteCallback:

DAGCallback patternTask resultCallback result
ecs_callback_simplePlain function from S3 bundleSUCCESS ~129sSUCCESS in 3.9s
ecs_callback_contextFunction reading dag_run + deadline contextSUCCESSSUCCESS in 3.8s
ecs_callback_failureFunction raises RuntimeErrorSUCCESSFAILED after 3 retries
ecs_callback_notifier_classCallable class (notifier __call__ pattern)SUCCESSSUCCESS in 4.0s

Verified: PENDING → QUEUED → RUNNING → SUCCESS/FAILED for both workload types; __handle_failed_workload + pending_workloads.append(...) reschedule callbacks correctly; log_task_event and self.fail() with string callback keys handled correctly by #65392's base class. Callback UUIDs and ECS task ARNs from the run are available on request.

Backwards compatibility

Existing task path unchanged. Callback paths gated behind AIRFLOW_V_3_3_PLUS. Public renamed methods/attrs preserved as deprecation shims so subclassers continue to work.


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 4.7) for code drafting, sibling-PR review comparison, and end-to-end test orchestration

Generated-by: Claude Code (Opus 4.7) following the guidelines

@boring-cyborgboring-cyborgBot added area:providers provider:amazon AWS/Amazon - related issues labels Mar 15, 2026
@shivaam
shivaamforce-pushed the feature/ecs-executor-callback branch from f3156ea to 4728b66CompareMarch 15, 2026 14:50
@shivaam

Copy link
Copy Markdown
ContributorAuthor

@ferruzzi Draft PR

@shivaam
shivaamforce-pushed the feature/ecs-executor-callback branch 5 times, most recently from 38ddfea to 6ac2634CompareMarch 15, 2026 18:44
@shivaam
shivaam marked this pull request as ready for review March 15, 2026 21:26
@shivaam
shivaam requested a review from o-nikolas as a code ownerMarch 15, 2026 21:26
@potiukpotiuk added the ready for maintainer review Set after triaging when all criteria pass. label Mar 16, 2026
@ferruzziferruzzi mentioned this pull request Mar 16, 2026
2 tasks
@shivaam
shivaamforce-pushed the feature/ecs-executor-callback branch 2 times, most recently from 0372a19 to a93048eCompareApril 16, 2026 05:15

@ferruzziferruzzi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once #65392 merges (hopefully today) you'll need to rebase and make some small tweaks. That may also let you clean up some of those #ignore directives in _process_workloads, hopefully.

@shivaam
shivaamforce-pushed the feature/ecs-executor-callback branch 2 times, most recently from 6a1c191 to a543ec3CompareApril 19, 2026 01:35
@shivaam

shivaam commented Apr 19, 2026

Copy link
Copy Markdown
ContributorAuthor

@ferruzzi Thanks for the review! I have updated the Pr and addressed all your comments.

  1. Dropped the cast() wrappers and isinstance(tuple) guards now that Widen BaseExecutor method signatures to accept WorkloadKey #65392 is merged — BaseExecutor handles WorkloadKey natively and skips callback keys in log_task_event internally. Removed the unused cast import and one test whose coverage moved to airflow-core.

  2. Renamed the _process_workloads param to workload_items so the module import no longer needs as wl.

  3. Renamed the task-named methods and pending_tasks to their workload equivalents. Straight rename, no compat shim

@shivaam
shivaamforce-pushed the feature/ecs-executor-callback branch from ea660cf to 28b791fCompareApril 19, 2026 02:44
Enables the ECS executor to dispatch ExecuteCallback workloads (deadline
alerts) alongside regular ExecuteTask workloads. Builds on apache#65392 which
widened BaseExecutor signatures to accept WorkloadKey.
- supports_callbacks = True (gated on AIRFLOW_V_3_3_PLUS)
- Widen key types to WorkloadKey throughout EcsQueuedTask / EcsTaskCollection
- Branch _process_workloads on ExecuteTask vs ExecuteCallback
- Add AIRFLOW_V_3_3_PLUS to version_compat.py
- Unit tests for queueing, processing, serialization, sync, mixed keys
@shivaam
shivaamforce-pushed the feature/ecs-executor-callback branch 3 times, most recently from 08bb01a to 726fbd1CompareMay 3, 2026 00:24
Renames (mirrors the merged Lambda callback PR — straight rename,
no shim, executor-internal surface):
sync_running_tasks -> sync_running_workloads
attempt_task_runs -> attempt_workload_runs
pending_tasks (attr) -> pending_workloads
__update_running_task -> __update_running_workload
__handle_failed_task -> __handle_failed_workload
Fix CI on older Airflow compat tests:
- Restore queue_workload() override. Airflow 3.3+ BaseExecutor routes
ExecuteCallback natively, but pre-3.3 raises ValueError for anything
not ExecuteTask. Override works across versions.
- Import AIRFLOW_V_3_3_PLUS from tests_common (main bumped to 3.3).
check-airflow-v-imports-in-tests hook disallows provider-internal
version_compat imports from test files.
@shivaam
shivaamforce-pushed the feature/ecs-executor-callback branch from 726fbd1 to 233ec6bCompareMay 3, 2026 01:19
@eladkal
eladkal requested a review from vincbeckMay 4, 2026 16:56
shivaamand others added 3 commits May 11, 2026 21:38
Co-authored-by: D. Ferruzzi <ferruzzi@amazon.com>
Mirror the Lambda executor pattern: widen CommandType to
Sequence[str] | Sequence[ExecuteTask | ExecuteCallback] on Airflow 3.3+,
and pre-declare queue/key/command at the top of the _process_workloads
loop so mypy doesn't infer narrow types from the first if-branch.
Removes five # type: ignore comments that previously covered the
signature mismatch and cross-branch reassignment.
Comment threadproviders/amazon/src/airflow/providers/amazon/aws/executors/ecs/utils.py Outdated
@ferruzzi

Copy link
Copy Markdown
Contributor

hm. That's an odd CI failure....It looks like Sphinx is throwing a "duplicate object description" warning because oth the Lambda and ECS executors define _process_workloads with the same signature, and autoapi is indexing them as the same object.

I think the easiest fix is to add :sphinx-autoapi-skip: to the _process_workloads docstring. It's a private method that shouldn't be in the public API docs anyway:

def_process_workloads(self, workload_items: Sequence[workloads.All]) ->None:
""":sphinx-autoapi-skip:"""

devel-common/src/docs/utils/conf_constants.py has some code that skips methodfs with that instruction. The spellcheck failure should clear once the docs build passes.

Give that a try??

Two adjustments to silence Sphinx duplicate-object warnings between the
Lambda and ECS executors that block the docs build:
- Add `:sphinx-autoapi-skip:` docstring on ECS `_process_workloads` so
autoapi skips this private method.
- Split the inner union in ECS `CommandType` so its rendered TypeAlias
text differs from Lambda's, avoiding a duplicate. The split form is
also stricter (disallows mixed task+callback lists, which our code
never produces).
@shivaam

Copy link
Copy Markdown
ContributorAuthor

Done — added :sphinx-autoapi-skip: on _process_workloads. Also had to split the inner union of CommandType (Sequence[ExecuteTask] | Sequence[ExecuteCallback] instead of Sequence[ExecuteTask | ExecuteCallback]) because Sphinx was flagging a second duplicate-object warning on the TypeAlias text itself shared with Lambda. The split form is also stricter — it disallows mixed task+callback lists, which our code never produces.

@ferruzzi

Copy link
Copy Markdown
Contributor

That new failing static check is related, sorry about that. looks like you need to add a period since the first line of every docstring needs to be a "complete sentence" and end in a period. Try this: """:sphinx-autoapi-skip:.""" that's kind of awkward, but I think that's what it wants.

The provider one I don't think is related

@ferruzziferruzzi mentioned this pull request May 14, 2026
1 task

@ferruzziferruzzi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. That's an ugly docstring, and maybe it would look nicer as something like

 def _process_workloads(self, workload_items: Sequence[workloads.All]) -> None:
"""
Process queued workloads by dispatching to ECS.
:sphinx-autoapi-skip:
"""

but it was my suggestion, it works, and I'm not generally keen on docstrings that just rephrase the method name. Approving.

@shivaam

Copy link
Copy Markdown
ContributorAuthor

LGTM. That's an ugly docstring, and maybe it would look nicer as something like

 def _process_workloads(self, workload_items: Sequence[workloads.All]) -> None:
"""
Process queued workloads by dispatching to ECS.
:sphinx-autoapi-skip:
"""

but it was my suggestion, it works, and I'm not generally keen on docstrings that just rephrase the method name. Approving.

Let me know if there are any other comments or if this ready to be merged.

shivaam added 3 commits May 21, 2026 08:49
Switch from Mock(spec=tuple) / bare-string callback key to
Mock(spec=TaskInstanceKey) and a real CallbackKey(...) instance,
matching the pattern established in apache#67268 for the rest of the
ECS/Lambda/Batch executor tests.
CallbackKey became a frozen dataclass in apache#66973 and no longer
accepts bare strings; this test was added in this PR so it was
missed by the apache#67268 cleanup sweep.
@o-nikolas
o-nikolas merged commit f2af887 into apache:mainMay 21, 2026
94 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issuesready for maintainer reviewSet after triaging when all criteria pass.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@shivaam@ferruzzi@seanghaeli@o-nikolas@vincbeck@potiuk