kaxil commented on code in PR #72155:
URL: https://github.com/apache/airflow/pull/72155#discussion_r3963588705
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -219,6 +230,8 @@ def execute_complete(self, context: Context,
generated_output: str, event: dict[
responded_by_user = event.get("responded_by_user")
chosen = event["chosen_options"]
if self.APPROVE not in chosen:
+ if event.get("timedout"):
+ raise HITLRejectException("Output was rejected by the approval
timeout default.")
Review Comment:
There is no reviewer on this path, but the modification branch below still
treats it as one. On 3.3+, `check_awaiting_input_timeouts` fills `params_input`
from `hitl_detail.params`, so with `allow_modifications=True` and
`on_approval_timeout="approve"` line 277 can return that value while line 276
logs `modified by the reviewer=None`.
That is a no-op while `params` still matches the current output. The catch
is that the execution API upsert only clears the response columns on a
re-attempt, while `hitl_detail` follows the TI across the new `uuid7()` id that
`prepare_db_for_next_try` assigns, via `onupdate=CASCADE`. So after a rejected
attempt 1 the row still holds the `params` from attempt 1, and attempt 2
auto-approves the output the reviewer rejected. Ignoring `params_input` when
`event.get("timedout")` is true would close that, and is correct by
construction anyway. The legacy path needs nothing here, since
`HITLTrigger._handle_timeout` dumps the params it was built with.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm.py:
##########
@@ -135,8 +141,18 @@ 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 approval_timeout is None:
Review Comment:
`require_approval=False` leaves both settings just as inert, and this guard
misses that case: `LLMOperator(..., approval_timeout=timedelta(hours=1),
on_approval_timeout="approve")` with the default `require_approval=False`
passes both checks and does nothing, which is the misconfiguration the error
exists to catch. `if on_approval_timeout != "fail" and not (require_approval
and approval_timeout)` would cover it, with the message and the "Requires
``approval_timeout``" lines in the parameter tables widened to match.
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -179,7 +189,7 @@ def defer_for_approval(
trigger=HITLTrigger(
ti_id=ti_id,
options=[LLMApprovalMixin.APPROVE, LLMApprovalMixin.REJECT],
- defaults=None,
+ defaults=timeout_defaults,
Review Comment:
On cores below 3.3 this default has to win a race it can lose. The
`self.defer()` below still passes `timeout=self.approval_timeout` (line 199),
so the execution API stores `TI.trigger_timeout`, and the scheduler
`check_trigger_timeouts` sweep (`trigger_timeout_check_interval`, 15s default)
flips the TI to `next_method=__fail__` with `TRIGGER_TIMEOUT` whenever its tick
lands before the next 5s triggerer poke reaches `_handle_timeout`.
`resume_execution` then raises `TaskDeferralTimeout`, `execute_complete` never
runs, and the configured approve/reject is dropped: the task just fails.
`HITLOperator` avoids this by passing no `timeout=` to its `defer()` at all
(`standard/operators/hitl.py:244` sends only the trigger, `method_name` and
`kwargs`), which leaves `timeout_datetime` on the trigger as the single
authority. Dropping the kwarg here would do the same, and
`test_timeout_sets_timeout_datetime` would lose its `defer_kwargs["timeout"]`
assertion. The 3.3+ path above is fine, since its timeout is handled by
`check_awaiting_input_timeouts`, which applies the defaults itself.
--
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]
