kaxil commented on code in PR #72159:
URL: https://github.com/apache/airflow/pull/72159#discussion_r3972718540
##########
providers/common/ai/tests/unit/common/ai/mixins/test_approval.py:
##########
@@ -174,6 +178,31 @@ def test_array_schema_passes_list_param_value(
defer_kwargs = approval_op_with_modifications.defer.call_args[1]
assert defer_kwargs["kwargs"]["generated_output"] == '["task_a"]'
+ @patch(HITL_TRIGGER_PATH, autospec=True)
+ @patch(UPSERT_HITL_PATH)
+ def test_notifiers_fire_once_the_review_is_open(self, mock_upsert,
mock_trigger_cls, context):
Review Comment:
Both of these land inside `TestDeferForApproval`, whose class-level patch at
line 89 pins the version flag to `False`, so they only cover the `defer()`
fallback and never the `awaiting_input` branch. If the loop were moved below
the `raise TaskAwaitingInput` block, notifiers would go silently dead on that
branch and both tests would still pass. `TestAwaitInputForApproval` at line 459
looks like the natural home for one more case asserting the notifier fired
before `TaskAwaitingInput` is raised.
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -165,6 +175,9 @@ def defer_for_approval(
params=hitl_params,
)
+ for notifier in self.approval_notifiers:
Review Comment:
Raising here discards a completed (and paid for) LLM run, and the retry does
not start clean. The HITLDetail row survives it (`record_ti` copies it to
history without deleting, and `hitl_detail.ti_id` is `onupdate="CASCADE"`), and
the execution API upsert returns an existing response-less row unchanged, so
the second notification points at a review body still showing the first try's
output. `HITLOperator` raises too, but it has no expensive non-idempotent step
ahead of the notify, so would log-and-continue be the better default here?
##########
providers/common/ai/docs/operators/llm.rst:
##########
@@ -205,6 +205,13 @@ approving with ``allow_modifications=True``, and set a
deadline with
:start-after: [START howto_operator_llm_approval]
:end-before: [END howto_operator_llm_approval]
+A pending review is only visible on the Required Actions page. Pass
Review Comment:
This is not quite accurate. A pending review also shows up as the
`NeedsReviewBadge` on the DAGs list and DAG cards, in the DAG and Run headers,
in the dashboard stats and the task overview, and `awaiting_input` is a
first-class state in the grid and the state filter. Something like "is not
surfaced as a notification" would say what you mean without the overclaim.
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -165,6 +175,9 @@ def defer_for_approval(
params=hitl_params,
)
+ for notifier in self.approval_notifiers:
+ notifier(context)
Review Comment:
The notifier only gets `context`, and `subject` / `body` are locals here, so
a notification template has nothing to reference beyond the dag/task ids and
`{{ task.prompt }}`. That bites `LLMSchemaCompareOperator` most, since the body
it composes carries the compatibility verdict and the mismatch summary. Merging
`subject` and `body` into the context passed here (or setting them on `self`)
would give notifiers the equivalent of `HITLOperator`'s `{{ task.subject }}`.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm.py:
##########
@@ -88,6 +88,8 @@ class LLMOperator(BaseOperator, LLMApprovalMixin):
:param allow_modifications: If ``True``, the reviewer can edit the output
before approving. The modified value is returned as the task result.
Default ``False``.
+ :param approval_notifiers: Notifiers called once the review is open, so a
Review Comment:
Worth adding "Only takes effect with ``require_approval=True``", the way
`fail_on_reject` and `ignore_downstream_trigger_rules` do in `llm_branch.py`.
Notifiers are only read inside `defer_for_approval`, so passing them without
`require_approval=True` silently does nothing.
--
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]
