guan404ming commented on code in PR #72155:
URL: https://github.com/apache/airflow/pull/72155#discussion_r3985839318
##########
providers/standard/src/airflow/providers/standard/triggers/hitl.py:
##########
@@ -121,14 +121,18 @@ async def _handle_timeout(self) -> TriggerEvent:
# Case 1: Response arrived just before timeout
if resp.response_received and resp.chosen_options:
if TYPE_CHECKING:
- assert resp.responded_by_user is not None
assert resp.responded_at is not None
chosen_options_list = list(resp.chosen_options or [])
+ responded_by_user = (
+ HITLUser(id=resp.responded_by_user.id,
name=resp.responded_by_user.name)
+ if resp.responded_by_user
+ else None
+ )
self.log.info(
"[HITL] responded_by=%s (id=%s) options=%s at %s (timeout
fallback skipped)",
- resp.responded_by_user.name,
- resp.responded_by_user.id,
+ responded_by_user["name"] if responded_by_user else None,
Review Comment:
Applied the branch plus a shared `_get_responder` helper for both call
sites; assert dropped.
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -179,14 +189,13 @@ def defer_for_approval(
trigger=HITLTrigger(
ti_id=ti_id,
options=[LLMApprovalMixin.APPROVE, LLMApprovalMixin.REJECT],
- defaults=None,
+ defaults=timeout_defaults,
Review Comment:
Marked the floor `# use next version`; CI rejects hand-bumped `>=`, release
tooling resolves it.
##########
providers/standard/tests/unit/standard/triggers/test_hitl.py:
##########
@@ -206,6 +206,50 @@ async def
test_run_should_check_response_in_timeout_handler(
action_datetime,
)
+ @pytest.mark.db_test
+ @pytest.mark.asyncio
+ @mock.patch.object(HITLTrigger, "log")
+ @mock.patch("airflow.sdk.execution_time.hitl.update_hitl_detail_response")
Review Comment:
Patched the trigger module's global; `assert_not_called` now guards the
no-second-write invariant.
##########
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 not (require_approval and
approval_timeout):
Review Comment:
Kept the truthiness check, reworded to ask for a positive
`approval_timeout`, added a `timedelta(0)` test.
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -219,10 +229,16 @@ 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 automatically: approval_timeout
expired with "
+ "on_approval_timeout='reject'."
+ )
raise HITLRejectException(f"Output was rejected by the reviewer
{responded_by_user}.")
+ log.info("Output approved by %s.", responded_by_user or "the approval
timeout default")
Review Comment:
Added `_describe_responder` formatting the name, reused in `llm_branch.py`;
tests now pass `HITLUser` dicts.
--
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]
