-
Notifications
You must be signed in to change notification settings - Fork 17.4k
Use Task SDK BaseHook in Amazon executors #62090
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,6 +69,19 @@ | |
|
|
||
|
|
||
| class AwsEcsExecutor(BaseExecutor): | ||
| """Executor that runs Airflow tasks on AWS ECS.""" | ||
|
|
||
| def _ti_to_execute_workload_cmd(self, ti): | ||
| from airflow.executors.workloads import ExecuteTask | ||
|
|
||
| workload_json = ExecuteTask.make(ti).model_dump_json() | ||
| return [ | ||
| "python3", | ||
| "-m", | ||
| "airflow.sdk.execution_time.execute_workload", | ||
| "--json-string", | ||
| workload_json, | ||
| ] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this compatible with airflow 2.x? It seems like this will not work for versions before Airflow 3.0 as
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for calling this out. As written, using airflow.sdk.execution_time.execute_workload is not compatible with Airflow 2.x since airflow.sdk is only available in Airflow 3.x. I will update the executor command construction to branch on Airflow version, using the SDK entrypoint for Airflow 3.x and the legacy task runner entrypoint for Airflow 2.x. I will add unit tests that assert the exact command built for both Airflow >= 3.0 and Airflow < 3.0, including the entrypoint and arguments like --json-string. Since the regression occurred there, I will take another look at test_try_adopt_task_instances for ECS and Batch. If unskipping is not feasible due to environment assumptions, I will add a targeted test to cover the same behavior with mocking. On factoring this into executor/utils, that sounds reasonable. I can do it in this PR |
||
| """ | ||
| Executes the provided Airflow command on an ECS instance. | ||
|
|
||
|
|
@@ -159,7 +172,7 @@ def _process_workloads(self, workloads: Sequence[workloads.All]) -> None: | |
| def start(self): | ||
| """Call this when the Executor is run for the first time by the scheduler.""" | ||
| check_health = self.conf.getboolean( | ||
| CONFIG_GROUP_NAME, AllEcsConfigKeys.CHECK_HEALTH_ON_STARTUP, fallback=False | ||
| CONFIG_GROUP_NAME, AllEcsConfigKeys.CHECK_HEALTH_ON_STARTUP, fallback=True | ||
| ) | ||
|
|
||
| if not check_health: | ||
|
|
@@ -590,7 +603,7 @@ def try_adopt_task_instances(self, tis: Sequence[TaskInstance]) -> Sequence[Task | |
| task, | ||
| ti.key, | ||
| ti.queue, | ||
| ti.command_as_list(), | ||
| self._ti_to_execute_workload_cmd(ti), | ||
| ti.executor_config, | ||
| ti.try_number, | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this functionality can be included in the executor/utils folder to avoid duplication.. I do not think this is a massive issue as it is only duplicated once but I think it should be considered. I would wait for the codeowner to weigh in here.