Use Task SDK BaseHook in Amazon executors#62090
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
SameerMesiah97
left a comment
There was a problem hiding this comment.
I think this needs some work. I’m not sure this change is fully backwards compatible, given as this introduces Task SDK APIs that only exist in Airflow 3.x.
The tests also seem incomplete. At a minimum, the construction of the command (both for Airflow ≥3.0 and Airflow <3.0) should be covered by new tests. I can also see existing tests, which are currently skipped for ECS and Batch executors (test_try_adopt_task_instances) that might be worth revisiting, especially since the regression occurred in that path.
| "airflow.sdk.execution_time.execute_workload", | ||
| "--json-string", | ||
| workload_json, | ||
| ] |
There was a problem hiding this comment.
Is this compatible with airflow 2.x? It seems like this will not work for versions before Airflow 3.0 as airflow.sdk was introduced then. I think you should have a look at PR #58207. There are issues with that but I think the implementation in that PR was more backwards compatible.
There was a problem hiding this comment.
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
| class AwsEcsExecutor(BaseExecutor): | ||
| """Executor that runs Airflow tasks on AWS ECS.""" | ||
|
|
||
| def _ti_to_execute_workload_cmd(self, ti): |
There was a problem hiding this comment.
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.
|
Closing this one since @vincbeck put in a fix already for this. |
Closes #58205
What
Update Amazon Batch and ECS executors to use the Task SDK BaseHook import path. (Update Amazon Batch and ECS executors to use the Task SDK
BaseHookimport path instead of the deprecatedairflow.hooks.base.BaseHook.)Why
Avoid deprecated BaseHook import usage and align these executors with the Airflow Task SDK hook location. The previous import path is deprecated. This change aligns the Amazon executors with the current Task SDK structure and removes usage of deprecated core hook imports.
Tests
Verified by running Amazon provider executor unit tests:
AIRFLOW__SECRETS__BACKEND="airflow.secrets.environment_variables.EnvironmentVariablesBackend"
AIRFLOW_CONN_SSH_DEFAULT='ssh://airflowtest:airflowtest@127.0.0.1:22?extra={"look_for_keys":false,"allow_agent":false,"allow_host_key_change":true}'
PYTHONPATH="$PWD/airflow-core/src:$PWD/task-sdk/src:$PWD/devel-common/src:$PWD/dev:$PWD:$PROVIDERS_SRC"
.venv-breeze/bin/pytest -q providers/amazon/tests -k "batch_executor or ecs_executor"
All relevant Batch and ECS executor tests pass.