Lee-W commented on code in PR #73261:
URL: https://github.com/apache/airflow/pull/73261#discussion_r4033559925
##########
providers/common/ai/docs/operators/llm.rst:
##########
@@ -271,7 +277,7 @@ Parameters
Fails the task when token / request / tool-call budgets are exceeded, or
when a
templated dict value cannot be coerced. Default ``None``.
- ``require_approval``: If ``True``, the task defers after generating output
and waits
- for human review. Default ``False``.
+ for human review. Default ``False``. Needs Airflow 3.1+.
Review Comment:
```suggestion
for human review. Default ``False``. Needs Airflow 3.1+.
```
##########
providers/common/ai/docs/operators/llm.rst:
##########
@@ -240,6 +240,12 @@ HITL interface. Optionally allow the reviewer to edit the
output before
approving with ``allow_modifications=True``, and set a deadline with
``approval_timeout``.
+Human-in-the-loop review needs Airflow 3.1+. On an older core the operator
raises
Review Comment:
```suggestion
Human-in-the-loop review needs Airflow 3.1+. On an older core the operator
raises
```
##########
providers/common/ai/tests/unit/common/ai/operators/test_llm.py:
##########
@@ -312,6 +312,58 @@ def _make_context(ti_id=None):
return MagicMock(**{"__getitem__": lambda self, key: {"task_instance":
ti}[key]})
+class TestLLMOperatorApprovalVersionGate:
+ """__init__ rejects require_approval on cores without human-in-the-loop
support.
+
+ Deliberately carries no class-level 3.1 skipif. These tests simulate an
old core by
+ patching the flag, so they must not inherit the sibling class's skip --
and on a
+ genuine pre-3.1 core, such as the 3.0.6 providers-compatibility job, they
are the
+ only tests that exercise the gate natively.
+ """
+
+ @patch("airflow.providers.common.ai.operators.llm.AIRFLOW_V_3_1_PLUS",
False)
+ def test_require_approval_rejected_on_old_core(self):
Review Comment:
Non-blocking, but I probably would merge these 3 `with pytest.raises` into 1
parameterized test
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm.py:
##########
@@ -165,6 +166,15 @@ def __init__(
raise ValueError(
f"on_approval_timeout must be 'fail', 'approve', or 'reject',
got {on_approval_timeout!r}."
)
+ # Checked ahead of the combination rule below. On a core older than
3.1 the core
+ # version is the real blocker -- defer_for_approval imports
HITLTrigger, which
+ # raises on import there, and airflow.sdk.execution_time.hitl, which
does not
+ # exist there -- and reporting a combination error first would send
the user to
+ # drop an argument that was never the problem. Without this the Dag
imports,
+ # execute() pays for the model call, and only then does the import
fail.
+ if require_approval and not AIRFLOW_V_3_1_PLUS:
Review Comment:
Should we check it at the beginning of `__init__`?
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm.py:
##########
@@ -98,7 +99,7 @@ class LLMOperator(BaseOperator, LLMApprovalMixin):
caveats.
:param require_approval: If ``True``, the task defers after generating
output and waits for a human reviewer to approve or reject via the
- HITL interface. Default ``False``.
+ HITL interface. Default ``False``. Needs Airflow 3.1+.
Review Comment:
```suggestion
HITL interface. Default ``False``. Needs Airflow 3.1+.
```
--
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]
