guan404ming commented on code in PR #72155:
URL: https://github.com/apache/airflow/pull/72155#discussion_r3965654227
##########
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:
Good catch, dropped the `timeout=` kwarg from `defer()` so the trigger's
`timeout_datetime` is the single authority on pre-3.3 cores, matching
`HITLOperator`. The scheduler sweep no longer races the triggerer poke, so the
configured approve/reject default is always applied. Updated
`test_timeout_sets_timeout_datetime` and `test_no_timeout_passes_none` to
assert the kwarg is absent.
##########
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:
Thanks, `execute_complete` now ignores `params_input` whenever
`event["timedout"]` is true, since there is no reviewer on that path and the
value can only be stale `hitl_detail.params` from an earlier attempt. Added
`test_timed_out_approval_ignores_stale_params_input` covering the
auto-approve-after-reject scenario.
##########
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:
Fixed, the guard is now `on_approval_timeout != "fail" and not
(require_approval and approval_timeout)`, so setting a timeout default without
`require_approval=True` also raises. Widened the error message, the docstring,
and the "Requires ..." lines in the three parameter tables, and parametrized
the test over both missing prerequisites.
--
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]
