Add deferrable mode to AzureContainerInstancesOperator#62772
Add deferrable mode to AzureContainerInstancesOperator#62772cruseakshay wants to merge 7 commits intoapache:mainfrom
Conversation
...iders/microsoft/azure/src/airflow/providers/microsoft/azure/operators/container_instances.py
Outdated
Show resolved
Hide resolved
SameerMesiah97
left a comment
There was a problem hiding this comment.
The overall approach is solid, though the style is a bit different from the deferrable mode implementation for the ADF counterpart. I have left a few comments for you to address. On a slightly tangential point, the PR is quite large so, for future reference, I would advise splitting a PR like this into the following:
- PR 1: Async Hook
- PR 2: Deferrable mode implementation (Operator + Trigger)
- PR 3: Bug fix
Combining all 3 in one PR makes it harder to review. You need not do this now but please keep this in mind the next time you submit a PR.
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/container_instance.py
Outdated
Show resolved
Hide resolved
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/container_instance.py
Outdated
Show resolved
Hide resolved
...iders/microsoft/azure/src/airflow/providers/microsoft/azure/operators/container_instances.py
Show resolved
Hide resolved
providers/microsoft/azure/src/airflow/providers/microsoft/azure/triggers/container_instance.py
Outdated
Show resolved
Hide resolved
...iders/microsoft/azure/src/airflow/providers/microsoft/azure/operators/container_instances.py
Outdated
Show resolved
Hide resolved
...iders/microsoft/azure/src/airflow/providers/microsoft/azure/operators/container_instances.py
Show resolved
Hide resolved
providers/microsoft/azure/tests/unit/microsoft/azure/hooks/test_container_instance.py
Outdated
Show resolved
Hide resolved
providers/microsoft/azure/tests/unit/microsoft/azure/hooks/test_container_instance.py
Show resolved
Hide resolved
|
@cruseakshay This PR has been converted to draft because it does not yet meet our Pull Request quality criteria. Issues found:
What to do next:
Converting a PR to draft is not a rejection — it is an invitation to bring the PR up to the project's standards so that maintainer review time is spent productively. If you have questions, feel free to ask on the Airflow Slack. |
b2bd7cb to
fa44757
Compare
7f532ba to
4192abf
Compare
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/container_instance.py
Outdated
Show resolved
Hide resolved
...iders/microsoft/azure/src/airflow/providers/microsoft/azure/operators/container_instances.py
Outdated
Show resolved
Hide resolved
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/container_instance.py
Outdated
Show resolved
Hide resolved
| super().__init__(azure_conn_id=azure_conn_id) | ||
|
|
||
| async def __aenter__(self) -> AzureContainerInstanceAsyncHook: | ||
| return self |
There was a problem hiding this comment.
Don't really like the contextmanager on hook level, I would prefer to see this on the get_async_conn method instead, as the the connection we want to make sure is closed correctly in case of exception, not the hook, as the hook is just a way to get the connection, but it isn't the resource itself, the connection is.
So more something like this:
@asynccontextmanager
async def get_async_conn(self):
credential = ...
client = AsyncContainerInstanceManagementClient(...)
try:
yield client
finally:
await client.close()
if hasattr(credential, "close"):
await credential.close()
So later on it can be used like this in the trigger:
async with hook.get_async_conn() as client:
await client.container_groups.get(...)
But I really like the fact that you implemented a contextmanager, because that's a clean way to cope with connections.
There was a problem hiding this comment.
Thanks @dabla! I will work on asynccontextmanager as you mentioned.
There was a problem hiding this comment.
@dabla I have added asynccontextmanager, PTAL.
c37a19a to
6792fb7
Compare
- async credential: store and close _async_credential in close() - Remove per-poll state logging in trigger - Change default polling_interval from 5s to 30s - Simplify finally cleanup block in execute() - Add get_async_conn() caching test
6792fb7 to
68983da
Compare
Add
deferrable=Truesupport toAzureContainerInstancesOperatorsotasks release their worker slot while waiting for long-running containers,
offloading polling to the lightweight Triggerer process.
Changes
New
AzureContainerInstanceAsyncHook— async counterpart to theexisting sync hook, using
azure.mgmt.containerinstance.aiofornon-blocking state/log/delete calls.
New
AzureContainerInstanceTrigger— polls ACI at a configurableinterval and yields a
TriggerEventwhen the container reaches aterminal state.
AzureContainerInstancesOperator— three new parameters:deferrable(default fromconf) — enables deferrable modepolling_interval(default5.0s) — trigger poll frequencyremove_on_success(defaultTrue) — controls cleanup on successfinallycleanup block would delete astill-running container group immediately after
self.defer()raisedTaskDeferred.provider.yaml+get_provider_info.pyupdated to register thenew trigger.
closes: #62433
Was generative AI tooling used to co-author this PR?