kaxil commented on code in PR #72159:
URL: https://github.com/apache/airflow/pull/72159#discussion_r3997970890
##########
providers/common/ai/tests/unit/common/ai/mixins/test_approval.py:
##########
@@ -174,6 +178,42 @@ 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):
+ notifier = MagicMock(spec=BaseNotifier)
+ order = MagicMock()
+ order.attach_mock(mock_upsert, "open_review")
+ order.attach_mock(notifier, "notify")
+ op = FakeOperator(approval_notifiers=[notifier])
+
+ op.defer_for_approval(context, "output")
+
+ notifier.assert_called_once_with(
Review Comment:
`MagicMock(spec=BaseNotifier)` never executes `BaseNotifier.__call__`, so
these cases prove a dict is handed to the notifier, not that a notifier can
read anything back out of it, which is how the `subject` shadowing above
survives a green suite. A small real `BaseNotifier` subclass that declares
`subject` in its own `template_fields` and asserts on the rendered value would
have caught it. Separately, every case here passes a single-element list, so
moving the `try` outside the `for` loop keeps the whole file green; the one
thing the per-iteration `try` buys, a dead notifier not silencing the one
beside it, has no coverage. One case built from `[failing, healthy]` would pin
that.
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -165,6 +177,12 @@ def defer_for_approval(
params=hitl_params,
)
+ for notifier in self.approval_notifiers:
+ try:
+ notifier({**context, "subject": subject, "body": body})
Review Comment:
`BaseNotifier.__call__` runs `_update_context()` before
`render_template_fields()`, and `_update_context` does `context.update((f,
getattr(self, f)) for f in self.template_fields)`, so merely declaring
`subject` or `body` in a notifier's own `template_fields` overwrites the review
value before anything renders. `SmtpNotifier` declares `subject` and
`AppriseNotifier` declares `body`, which are the two names the new docs line
tells people to use. The most natural recipe is the one that fails silently:
running a notifier that carries `SmtpNotifier`'s `template_fields` with
`html_content="<b>{{ subject }}</b><pre>{{ body }}</pre>"` and no `subject` of
its own renders `<b>None</b>`, because `getattr(self, "subject")` merges the
unset field's `None` over the review headline; set `subject="{{ subject }}"`
explicitly and it comes back as the literal `{{ subject }}` instead. A
Slack-shaped notifier (`text="{{ subject }}"`) renders correctly, so the
promise holds everywhere except the notif
iers whose own field names collide. `HITLOperator` never hits this because it
keeps them as operator attributes, which is why `example_hitl_operator.py`
templates `{{ task.subject }}` / `{{ task.body }}` rather than the bare names;
assigning `self.subject` / `self.body` before the loop, or renaming the
injected keys to something that cannot collide, would put them out of
`_update_context`'s reach.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm.py:
##########
@@ -138,6 +142,9 @@ def __init__(
self.require_approval = require_approval
self.approval_timeout = approval_timeout
self.allow_modifications = allow_modifications
+ self.approval_notifiers = (
+ [approval_notifiers] if isinstance(approval_notifiers,
BaseNotifier) else approval_notifiers or []
Review Comment:
The `else` branch passes any truthy non-`BaseNotifier` straight through, and
the `for` statement that consumes it sits outside the `try` in
`defer_for_approval`, so a bare function or `partial` escapes as `TypeError:
... is not iterable` after `agent.run_sync()` has been billed and
`upsert_hitl_detail` has already written the review row, while a plain string
iterates per character, turns each one into a caught `TypeError` in the worker
log, sends nothing, and parks the task as if all was well. The core submit
handler gates its resume on `AWAITING_INPUT`/`DEFERRED`, so in the first case
the reviewer can still answer that orphaned review and resume nothing.
`HITLOperator` uses a byte-identical expression, so the shape is defensible as
parity -- what makes it worth tightening here is the same asymmetry the failure
path already landed on: its loop runs before anything expensive or
irreversible, this one runs after both. Would wrapping the `else` in
`list(...)` and rejecting a non-`Bas
eNotifier` element here be worth it, so the mistake surfaces at parse time
instead?
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm_sql.py:
##########
@@ -83,7 +83,8 @@ class LLMSQLQueryOperator(LLMOperator):
Human-in-the-Loop approval parameters are inherited from
:class:`~airflow.providers.common.ai.operators.llm.LLMOperator`
- (``require_approval``, ``approval_timeout``, ``allow_modifications``).
+ (``require_approval``, ``approval_timeout``, ``allow_modifications``,
+ ``approval_notifiers``).
Review Comment:
`llm_sql.rst` is the one operator page the doc sweep missed. Its
Human-in-the-Loop Approval section still names only `require_approval` and
`allow_modifications` inline, while `llm.rst`, `llm_branch.rst`,
`llm_file_analysis.rst`, and `llm_schema_compare.rst` all picked up
`approval_notifiers` in this PR. One sentence pointing at :doc:`llm` would even
it up.
--
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]
