Uh oh!
There was an error while loading. Please reload this page.
Add deferrable support to AzureBatchOperator - #59798
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds opt-in deferrable support to AzureBatchOperator, allowing the operator to release worker resources while waiting for Azure Batch jobs to complete instead of blocking with sleep-based polling.
- Introduces a
deferrableflag (defaultFalse) to maintain backward compatibility - Adds
AzureBatchJobTriggerfor asynchronous polling of Azure Batch job completion - Implements cleanup logic in
post_execute()to handle resource deletion in both sync and async modes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
providers/microsoft/azure/src/airflow/providers/microsoft/azure/triggers/batch.py | New trigger implementation for async polling of Azure Batch job completion status |
providers/microsoft/azure/src/airflow/providers/microsoft/azure/operators/batch.py | Updated operator to support deferrable mode with conditional execution paths and centralized cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def run(self) -> AsyncIterator[TriggerEvent]: | ||
| hook = AzureBatchHook(self.azure_batch_conn_id) | ||
| timeout_time = timezone.utcnow() + timedelta(minutes=self.timeout) |
There was a problem hiding this comment.
Using timezone.utcnow() is deprecated. Use timezone.utcnow() consistently if it's the project standard, but consider that datetime.now(timezone.utc) is the recommended approach in modern Python for timezone-aware UTC timestamps.
| self.clean_up(self.batch_pool_id) | ||
| self._cleanup_done = True | ||
| def execute_complete(self, context: Context, event: dict[str, Any] | None = None) -> str: |
There was a problem hiding this comment.
The execute_complete method lacks a docstring. Add documentation explaining the trigger callback, expected event structure (status, fail_task_ids, message), return value, and possible exceptions.
| defexecute_complete(self, context: Context, event: dict[str, Any] |None=None) ->str: | |
| defexecute_complete(self, context: Context, event: dict[str, Any] |None=None) ->str: | |
| """ | |
| CallbackexecutedwhenthedeferrableAzureBatchjobtriggerfires. | |
| Thismethodisinvokedafter :meth:`defer`whenrunningindeferrablemode. | |
| Itprocessestheeventemittedby :class:`~airflow.providers.microsoft.azure.triggers.batch.AzureBatchJobTrigger` | |
| andconvertsitintothefinaltaskoutcome. | |
| Theeventisexpectedtobeamappingwith (atleast) thefollowingkeys: | |
| *``status``: Astringindicatingthejoboutcome. Recognizedvaluesare: | |
| *``"success"``: alltaskscompletedsuccessfully. | |
| *``"failure"``: oneormoretasksfailed. | |
| *``"timeout"``: thejobdidnotcompletewithintheconfiguredtimeout. | |
| *``"error"``: aninternalerroroccurredwhilewaitingfortaskcompletion. | |
| *``fail_task_ids``: OptionaliterableoftaskIDsthatfailed. Defaultstoanemptylist | |
| whennofailuresarereported. | |
| *``message``: Optionalhuman-readabledescriptionprovidingadditionalcontextfor | |
| ``"timeout"``or``"error"``statuses. | |
| :paramcontext: Airflowtaskcontextprovidedbythescheduleronresumption. | |
| :paramevent: Eventdictionarysentfrom :class:`AzureBatchJobTrigger`describingthe | |
| terminalstateofthemonitoredAzureBatchjob. | |
| :returns: TheAzureBatchjobIDassociatedwiththisoperator, onsuccessfulcompletion. | |
| :raisesAirflowException: Ifnoeventisreceived, ifthestatusis``"error"``or | |
| ``"failure"``, ifanyfailedtaskIDsarereported, orifanunexpectedstatusis | |
| encountered. | |
| :raisesTimeoutError: Iftheeventindicatesthattheoperationtimedout. | |
| """ |
| fail_task_ids = event.get("fail_task_ids", []) | ||
| if status == "timeout": | ||
| raise TimeoutError(event.get("message", "Timed out waiting for tasks to complete")) |
There was a problem hiding this comment.
Using built-in TimeoutError is inconsistent with Airflow patterns. Consider using AirflowException with a timeout-specific message to maintain consistency with other exception handling in this operator.
| raiseTimeoutError(event.get("message", "Timed out waiting for tasks to complete")) | |
| raiseAirflowException(event.get("message", "Timed out waiting for tasks to complete")) |
| if status == "failure" or fail_task_ids: | ||
| raise AirflowException(f"Job failed. Failed tasks: {fail_task_ids}") | ||
| if status != "success": | ||
| raise AirflowException(f"Unexpected event status: {event}") |
There was a problem hiding this comment.
The error message could be more actionable. Consider clarifying what statuses are expected and provide the actual status received, e.g., f\"Unexpected event status '{status}'. Expected 'success', 'failure', 'timeout', or 'error'.\"
| raiseAirflowException(f"Unexpected event status: {event}") | |
| raiseAirflowException( | |
| f"Unexpected event status '{status}'. Expected 'success', 'failure', 'timeout', or 'error'. " | |
| f"Full event: {event}" | |
| ) |
Ankurdeewan
commented
Dec 26, 2025
I’ve pushed a set of updates to the PR to align the implementation closely with the patterns you mentioned on the issue. In particular: -deferrable default is now config-driven -polling is fully trigger-based with an operator-level poll_interval -cleanup semantics are centralized and preserved -added comprehensive unit tests for deferrable and non-deferrable paths This should now be consistent with AWSBatchOperator and existing Azure deferrable operators. @potiuk tagging for visibility after the updates. |
Ankurdeewan
commented
Jan 4, 2026
| from airflow.triggers.base import TriggerEvent | ||
| def test_azure_batch_job_trigger_serialize(): |
There was a problem hiding this comment.
Maybe put all tests under a class named TestAzureBatchJobTrigger.
| # Verify pool and nodes are in terminal state before deferral | ||
| pool = self.hook.connection.pool.get(self.batch_pool_id) | ||
| nodes = list(self.hook.connection.compute_node.list(self.batch_pool_id)) | ||
| if pool.resize_errors: |
There was a problem hiding this comment.
This check could be done before computing the node list no?
There was a problem hiding this comment.
Yess, that makes sense! I’ve reordered it so we check resize_errors first and only list compute nodes if the pool looks healthy.
| def post_execute(self, context: Context, result: Any | None = None) -> None: # type: ignore[override] | ||
| """Perform cleanup after task completion in both deferrable and non-deferrable modes.""" | ||
| if getattr(self, "_cleanup_done", False): |
There was a problem hiding this comment.
How come we have to use getattr here for _cleanup_done?
There was a problem hiding this comment.
Yeah, this is mainly because _cleanup_done might not exist in some paths; especially after deferral or when the task gets deserialized on resume. Using getattr(..., False) just keeps it safe and still guarantees cleanup only runs once.
Ankurdeewan
commented
Jan 17, 2026
@dabla I’ve pushed updates that address all the points ; tests are grouped, the pool check now short-circuits before listing nodes, and I added a comment explaining the getattr guard for cleanup. Would appreciate another look when you get a chance. |
dabla
commented
Jan 22, 2026
You'll need to add trigger entry in provider.yaml as well. |
Ankurdeewan
commented
Feb 1, 2026
@dabla please take a look when you can! thanks :) |
…e/triggers/batch.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
4454d6f to
32424b6Compare@Ankurdeewan 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. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
potiuk
commented
Apr 2, 2026
This pull request has been converted to draft due to quality issues more than a week ago and there has been no response from the author. @Ankurdeewan, you are welcome to reopen this PR when you are ready to continue working on it. Thank you for your contribution! |
This PR adds an opt-in deferrable mode to
AzureBatchOperator.What this changes
deferrableflag (default:False).deferrable=True, the operator:What stays the same
deferrable=False, the operator still runs exactly as before:deferrable=Trueis explicitly enabled.Cleanup behavior
Cleanup semantics are preserved:
execute_complete().Why this is needed
Previously, the operator blocked a worker while polling Azure Batch using sleep-based loops.
This change aligns the Azure Batch operator with existing deferrable patterns and reduces unnecessary worker usage.
Related issue: #59779