Update ECS executor to support Task SDK#48513
Conversation
|
Closing and re-opening to run test (they somehow did not run) |
vincbeck
left a comment
There was a problem hiding this comment.
Overall looks good! Good job with figuring out all these things. Two questions:
- Can we add unit tests? ECS executor changes are not tested
- It looks like it is but just double checking. With these changes, is ECS executor still compatible with AF2?
Sure, I can look at adding some. But most all of the actual behaviour of the executor itself hasn't changed. Just the interface to it (which should honestly be tested by the base executor once all this stuff moves into that class). But I'll look at adding a few.
Yupp it should be still working with AF2, I can do a manual test to verify 👍 |
|
Follow up on this:
Unit test added! It pretty comprehensively tests the task SDK path beginning to end. Let me know what you think!
Manually setup a repro with AF2 and the current executor, everything works as expected! |
amoghrajesh
left a comment
There was a problem hiding this comment.
@o-nikolas thanks, your changes mostly look good. Some comments
Follows the paradigm used in the k8s executor except the serialized workload is passed along as string instead of a file, since there is no (easy) way to plumb a file into the ECS container at run time. In the future using a secret to pass along the workload could be another possible approach, but this requires the use of another cloud service (i.e. Secret Manager or Parameter Store).
a8154dd to
9f65a6f
Compare
9f65a6f to
9ca3c45
Compare
* Update ECS executor to support Task SDK Follows the paradigm used in the k8s executor except the serialized workload is passed along as string instead of a file, since there is no (easy) way to plumb a file into the ECS container at run time. In the future using a secret to pass along the workload could be another possible approach, but this requires the use of another cloud service (i.e. Secret Manager or Parameter Store). * Add unit test to test task sdk path * Small fix in arg ordering * Only import within Airflow 3 scope for back compat tests * fixes from PR review
Summary
Follows the paradigm used in the k8s executor except the serialized workload is passed along as string instead of a file, since there is no (easy) way to plumb a file into the ECS container at run time.
In the future using a secret to pass along the workload could be another possible approach, but this requires the use of another cloud service (i.e. Secret Manager or Parameter Store).
Testing
I tested the ECS executor and execute_workload changes in combination and everything is passing green.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.