kaxil commented on code in PR #71073:
URL: https://github.com/apache/airflow/pull/71073#discussion_r3762038784
##########
providers/common/ai/docs/operators/llm_branch.rst:
##########
@@ -92,9 +92,11 @@ against the downstream task IDs before branching:
:start-after: [START howto_operator_llm_branch_approval]
:end-before: [END howto_operator_llm_branch_approval]
-Rejecting the review, or letting ``approval_timeout`` expire, **fails** the
-task (``HITLRejectException`` / ``HITLTimeoutError``), so downstream tasks
-end up ``upstream_failed`` rather than skipped.
+Rejecting the review **skips the direct downstream tasks except teardowns**,
+matching
+:class:`~airflow.providers.standard.operators.hitl.ApprovalOperator`. Set
+``fail_on_reject=True`` to fail the task instead (generally discouraged).
+Letting ``approval_timeout`` expire fails the task (``HITLTimeoutError``).
Review Comment:
Worth scoping this sentence to the reject path. The approve path goes
through `skip_all_except`, which has no `is_teardown` filter, so on the same
`branch >> [task_a, task_b, cleanup.as_teardown()]` topology from the thread
above, approving skips `['cleanup', 'task_b']` and the teardown is discarded.
That's inherited branch behaviour rather than anything this PR changes, but as
written it reads like a general guarantee, and the surprising half is that
rejecting preserves `cleanup` while approving drops it.
Separately, line 105 says `approval_timeout` and the rest of the approval
behaviour are inherited from `LLMOperator`, which is no longer true of
`fail_on_reject`.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm_branch.py:
##########
@@ -69,11 +77,13 @@ def __init__(
self,
*,
allow_multiple_branches: bool = False,
+ fail_on_reject: bool = False,
Review Comment:
Non-blocking: why does `fail_on_reject` land only on `LLMBranchOperator`?
The reject is raised in the shared `LLMApprovalMixin.execute_complete`
(`mixins/approval.py:222`), so `LLMOperator`, `LLMSQLQueryOperator`,
`LLMSchemaCompareOperator` and `LLMFileAnalysisOperator` still fail the task on
reject with no opt-out, and two operators carrying the same
`require_approval=True` flag now do opposite things. Putting the flag on the
mixin would keep the family consistent; happy for that to be a follow-up if
you'd rather keep this PR tight.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
