kaxil commented on code in PR #72155:
URL: https://github.com/apache/airflow/pull/72155#discussion_r4004604077
##########
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:
"has no effect" is not quite right now that the same commit documents a
second effect. With `require_approval=True` and no `approval_timeout`,
`defer_for_approval` still passes `defaults=timeout_defaults` to the upsert
(`mixins/approval.py:174`), so the review form pre-highlights the chosen option
exactly as the new docs sentence describes; only the deadline never fires.
Rejecting that combination is a defensible call, since the parameter is named
for a timeout, but the message tells the user the setting does nothing when it
would in fact change what the reviewer sees. Naming the missing piece instead,
along the lines of "needs a positive `approval_timeout` to fire", matches what
the check requires.
##########
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:
Correcting my own round-3 claim first: I said the pre-3.3 `timedelta(0)`
behaviour pre-dated the PR, and it did not. I read the branch after the
`timeout=` removal had already landed rather than `main`.
At the merge base `self.defer()` still carried
`timeout=self.approval_timeout`, and nothing on that path tests the value for
truthiness. Checked against the released `3.2.2` tag, which is the core this
PR's own compat job runs: `TaskDeferred` stores the value as passed,
`task-sdk/.../task_runner.py:1211` forwards it as `trigger_timeout`, and the
execution API gates on `is not None`
(`execution_api/routes/task_instances.py:549-550`). So `timedelta(0)` landed
`TI.trigger_timeout = utcnow()`, and `check_trigger_timeouts`
(`scheduler_job_runner.py:2915-2916`) flipped the TI to `__fail__` on the next
tick.
Dropping that kwarg, which I asked for, leaves this line as the only
authority, and `bool(timedelta(0))` is `False`. `approval_timeout=timedelta(0)`
now yields `timeout_datetime=None`, so `HITLTrigger.run()` pokes every 5s
forever where it used to fail. 3.3+ is unaffected, since `TaskAwaitingInput`
reaches the same `is not None` test (`3.3.0` tag: the awaiting-input branch at
`task_instances.py:709` applies it at `:739`).
The new `__init__` guard does not cover this, because it only fires when
`on_approval_timeout != "fail"` -- so the configuration that hangs is the
default one. `if self.approval_timeout is not None else None` restores the old
outcome and makes the two wait paths agree: a deadline of `utcnow()` sends the
first `run()` iteration into `_handle_timeout`, which with `defaults=None`
yields the timeout failure event.
--
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]
