kaxil commented on code in PR #72155:
URL: https://github.com/apache/airflow/pull/72155#discussion_r3997968733
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm.py:
##########
@@ -135,8 +141,19 @@ def __init__(
self._serialize_model_output = serialize_output or not _CORE_WALKER
self.agent_params = agent_params or {}
self.usage_limits = usage_limits
+ if on_approval_timeout not in ("fail",
*LLMApprovalMixin.TIMEOUT_DEFAULTS):
+ raise ValueError(
+ f"on_approval_timeout must be 'fail', 'approve', or 'reject',
got {on_approval_timeout!r}."
+ )
+ if on_approval_timeout != "fail" and not (require_approval and
approval_timeout):
Review Comment:
The reworded message promises a positive value but the check is still
truthiness, so `timedelta(0)` is rejected while `timedelta(hours=-1)` passes.
The deadline is then already in the past: pre-3.3 the first `run()` iteration
goes straight to `_handle_timeout` Case 3, and on 3.3+ `trigger_timeout` lands
behind `utcnow()` (`execution_api/routes/task_instances.py:772`, no positivity
check there either) so the next sweep tick applies the default. Either way the
output is released with no human opportunity at all, and nothing logs why.
Before this PR the same value failed the task, since there was no default to
apply. `approval_timeout > timedelta(0)` would match what the message already
claims, and the three `Requires ...` bullets (`llm.rst:233-235`,
`llm_branch.rst:145-147`, `llm_schema_compare.rst:195-197`) could pick up the
same word.
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -160,7 +170,7 @@ def defer_for_approval(
options=[LLMApprovalMixin.APPROVE, LLMApprovalMixin.REJECT],
subject=subject,
body=body,
- defaults=None,
+ defaults=timeout_defaults,
Review Comment:
This column outlives the task instance that writes it, and on 3.3+ it is
what the timeout sweep reads rather than the operator's attribute, so the two
wait paths take a config change at different times. `upsert_hitl_detail` writes
the request columns only on the insert branch
(`execution_api/routes/hitl.py:72-82`); the existing-row branch clears the four
response columns and nothing else (`:84-90`), and the row follows the TI across
the `uuid7()` rotation in `prepare_db_for_next_try`
(`models/taskinstance.py:1076`) because the FK is `onupdate=CASCADE`
(`models/hitl.py:177`).
The sequence that gets hit is the natural one for adopting this feature: run
with the default `"fail"`, which persists `defaults = NULL`, watch the review
time out and fail, set `on_approval_timeout="approve"`, then clear the task. On
3.1/3.2 `HITLTrigger` is rebuilt from the operator and approves; on 3.3+
`check_awaiting_input_timeouts` still reads `NULL` off the stale row
(`scheduler_job_runner.py:3589`) and keeps failing. The reverse edit is the
same mechanism pointing the other way: `"approve"` to `"fail"` plus a clear
leaves 3.3+ auto-approving output the DAG now says should never be released
unreviewed.
This was unreachable before this PR, since `defaults` was always `None`
here, and the fix belongs in the core upsert rather than in this file. Flagging
it because the setting this PR adds is the first one common.ai routes through
that column.
##########
providers/common/ai/docs/operators/llm.rst:
##########
@@ -223,6 +230,9 @@ Parameters
for human review. Default ``False``.
- ``approval_timeout``: Maximum time to wait for a review (``timedelta``).
``None``
means wait indefinitely. Default ``None``.
+- ``on_approval_timeout``: Outcome when ``approval_timeout`` expires without a
+ review: ``"fail"`` (default), ``"approve"``, or ``"reject"``. Requires
+ ``require_approval=True`` and ``approval_timeout``.
Review Comment:
Worth a sentence here that `on_approval_timeout` also changes what the
reviewer sees, since it writes the shared `HITLDetail.defaults` column.
`isHighlightOption`
(`airflow-core/src/airflow/ui/src/pages/HITLTaskInstances/HITLResponseForm.tsx:44-60`)
falls back to `defaults` while no response has arrived, and drives
`colorPalette` (`:139`) and `variant` (`:144`): with `defaults` null both
buttons render brand/solid, `["Approve"]` greys out Reject, and `["Reject"]`
greys out Approve. So `on_approval_timeout="reject"` makes Reject the primary
button on an approval gate.
`HITLOperator` documents its own `defaults` as both roles
(`standard/operators/hitl.py:65`); the wrapper here describes only the timeout
half, on all five pages and in the mixin docstring.
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -62,16 +63,23 @@ class LLMApprovalMixin:
before approving. The (possibly modified) output is then returned as the
task result.
+ ``on_approval_timeout`` decides what happens when ``approval_timeout``
+ expires without a response: ``"fail"`` raises ``HITLTimeoutError``, while
+ ``"approve"`` and ``"reject"`` answer the review with that option so the
+ task resumes as if a reviewer had chosen it.
+
Operators that use this mixin must set the following attributes:
- ``require_approval`` (``bool``)
- ``allow_modifications`` (``bool``)
- ``approval_timeout`` (``timedelta | None``)
+ - ``on_approval_timeout`` (``str``)
Review Comment:
The Protocol 31 lines up is now `Literal["fail", "approve", "reject"]`, so
this bullet is the last place saying `str`. Worth matching, since this is the
contract a non-`LLMOperator` implementor reads, and `defer_for_approval` looks
the value up with `TIMEOUT_DEFAULTS.get(...)`, which returns `None` rather than
raising for anything outside the set.
--
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]
