guan404ming commented on code in PR #72155:
URL: https://github.com/apache/airflow/pull/72155#discussion_r4023634391
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -179,16 +190,22 @@ def defer_for_approval(
trigger=HITLTrigger(
ti_id=ti_id,
options=[LLMApprovalMixin.APPROVE, LLMApprovalMixin.REJECT],
- defaults=None,
+ defaults=timeout_defaults,
params=hitl_params,
multiple=False,
timeout_datetime=utcnow() + self.approval_timeout if
self.approval_timeout else None,
Review Comment:
Fixed in b90c74c9f6: switched to `is not None`, so `timedelta(0)` yields a
deadline of `utcnow()` and fails on the first `run()` iteration as before.
Added `test_zero_timeout_sets_timeout_datetime_to_now` with the default
`on_approval_timeout`.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm.py:
##########
@@ -135,8 +143,21 @@ 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 is not None and
approval_timeout > timedelta(0)
+ ):
+ raise ValueError(
+ f"on_approval_timeout={on_approval_timeout!r} has no effect
without "
Review Comment:
Reworded to `needs require_approval=True and a positive approval_timeout to
fire`, naming the missing piece instead of claiming no effect.
--
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]
