Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions providers/common/ai/docs/operators/llm_branch.rst
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,10 @@ 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 every downstream task**, 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``).

``require_approval=True`` requires a string prompt: a decorated callable
returning a ``Sequence[UserContent]`` raises ``TypeError`` before the LLM
Expand Down Expand Up @@ -133,6 +134,8 @@ Parameters
means wait indefinitely. Default ``None``.
- ``allow_modifications``: If ``True``, the reviewer can change the chosen
branch(es) before approving. Default ``False``.
- ``fail_on_reject``: If ``True``, a rejected review fails the task instead of
skipping every downstream task. Generally discouraged. Default ``False``.

Logging
-------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

from airflow.providers.common.ai.operators.llm import LLMOperator
from airflow.providers.common.ai.utils.logging import log_run_summary
from airflow.providers.standard.exceptions import HITLRejectException
from airflow.providers.standard.operators.branch import BranchMixIn

if TYPE_CHECKING:
Expand All @@ -46,14 +47,21 @@ class LLMBranchOperator(LLMOperator, BranchMixIn):
:param system_prompt: System-level instructions for the LLM agent.
:param allow_multiple_branches: When ``False`` (default) the LLM returns a
single task ID. When ``True`` the LLM may return one or more task IDs.
:param fail_on_reject: If ``True``, a rejected review fails the task
instead of skipping every downstream task. Generally discouraged,
as for :class:`~airflow.providers.standard.operators.hitl.ApprovalOperator`.
Default ``False``.
:param agent_params: Additional keyword arguments passed to the pydantic-ai
``Agent`` constructor (e.g. ``retries``, ``model_settings``, ``tools``).

Human-in-the-Loop approval parameters are inherited from
:class:`~airflow.providers.common.ai.operators.llm.LLMOperator`
(``require_approval``, ``approval_timeout``, ``allow_modifications``).
The task pauses after the LLM chooses the branch(es) and only skips the
unselected downstream tasks once a reviewer approves. The review form
unselected downstream tasks once a reviewer approves. Rejecting the
review skips every downstream task, matching
:class:`~airflow.providers.standard.operators.hitl.ApprovalOperator`;
set ``fail_on_reject=True`` to fail the task instead. The review form
lists the valid downstream task IDs; with ``allow_modifications=True``
the editable choice is rendered as a dropdown of those IDs (single-branch
mode) or a multi-select of them (``allow_multiple_branches=True``), and
Expand All @@ -69,11 +77,13 @@ def __init__(
self,
*,
allow_multiple_branches: bool = False,
fail_on_reject: bool = False,
**kwargs: Any,
) -> None:
kwargs.pop("output_type", None)
super().__init__(**kwargs)
self.allow_multiple_branches = allow_multiple_branches
self.fail_on_reject = fail_on_reject

def execute(self, context: Context) -> str | Iterable[str] | None:
if self.require_approval:
Expand Down Expand Up @@ -133,7 +143,13 @@ def execute(self, context: Context) -> str | Iterable[str] | None:

def execute_complete(self, context: Context, generated_output: str, event: dict[str, Any]) -> Any:
"""Resume after human review, validating the reviewed choice before branching."""
output = super().execute_complete(context, generated_output, event)
try:
output = super().execute_complete(context, generated_output, event)
except HITLRejectException:
if self.fail_on_reject:
raise
self.log.info("Rejected. Skipping all downstream tasks...")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception being swallowed here carries the reviewer name (Output was rejected by the reviewer <user>.), and the task log was the only place that surfaced. event["responded_by_user"] is right here, so self.log.info("Rejected by %s. Skipping all downstream tasks.", event.get("responded_by_user")) would keep the attribution. Right now the log for a rejected gate no longer says who rejected it.

return self.do_branch(context, None)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do_branch(context, None) skips every direct downstream task, teardowns included, but ApprovalOperator's reject path filters them out (yield from (t for t in tasks if not t.is_teardown)). I checked this on a real DAG with branch >> [task_a, task_b, cleanup.as_teardown()]: this path skips ['cleanup', 'task_a', 'task_b'] where ApprovalOperator skips ['task_a', 'task_b']. Before this change a reject failed the task and the teardown still ran on its all_done-style rule, so cleanup now silently stops running after a rejection.

Either filter teardowns here, or drop the "matching ApprovalOperator" wording from the docstring and the rst.

branches = self._parse_reviewed_branches(output)
selected = {branches} if isinstance(branches, str) else set(branches)
invalid = selected - self.downstream_task_ids
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
from airflow.providers.common.ai.operators.llm import LLMOperator
from airflow.providers.common.ai.operators.llm_branch import LLMBranchOperator
from airflow.providers.common.compat.sdk import Param, ParamValidationError, TaskDeferred
from airflow.providers.standard.exceptions import HITLRejectException

from tests_common.test_utils.version_compat import AIRFLOW_V_3_1_PLUS, AIRFLOW_V_3_3_PLUS

Expand Down Expand Up @@ -397,6 +398,30 @@ def test_execute_complete_approved_multiple_branches(self, mock_do_branch):
assert result == ["task_a", "task_c"]
mock_do_branch.assert_called_once_with(ctx, ["task_a", "task_c"])

@patch.object(LLMBranchOperator, "do_branch")
def test_execute_complete_reject_skips_all_downstream(self, mock_do_branch):
mock_do_branch.return_value = None
op = LLMBranchOperator(task_id="t", prompt="p", llm_conn_id="c")
op.downstream_task_ids = {"task_a", "task_b"}
event = {"chosen_options": ["Reject"], "responded_by_user": "admin"}
ctx = _make_context()

result = op.execute_complete(ctx, generated_output="task_a", event=event)

assert result is None
mock_do_branch.assert_called_once_with(ctx, None)

@patch.object(LLMBranchOperator, "do_branch")
def test_execute_complete_reject_fails_with_fail_on_reject(self, mock_do_branch):
op = LLMBranchOperator(task_id="t", prompt="p", llm_conn_id="c", fail_on_reject=True)
op.downstream_task_ids = {"task_a", "task_b"}
event = {"chosen_options": ["Reject"], "responded_by_user": "admin"}

with pytest.raises(HITLRejectException, match="rejected"):
op.execute_complete(_make_context(), generated_output="task_a", event=event)

mock_do_branch.assert_not_called()

@patch.object(LLMBranchOperator, "do_branch")
def test_execute_complete_with_modified_branch(self, mock_do_branch):
"""A reviewer-modified branch is used when it is a valid downstream task."""
Expand Down