guan404ming commented on code in PR #72157:
URL: https://github.com/apache/airflow/pull/72157#discussion_r4035311283
##########
providers/common/ai/tests/unit/common/ai/operators/test_llm.py:
##########
@@ -359,6 +363,26 @@ def
test_on_approval_timeout_without_prerequisites_raises(self, kwargs):
):
LLMOperator(task_id="t", prompt="p", llm_conn_id="c",
on_approval_timeout="approve", **kwargs)
+ @pytest.mark.skipif(not AIRFLOW_V_3_1_PLUS, reason="assigned_users needs
Airflow 3.1+")
+ @pytest.mark.parametrize(
+ "assigned_users",
+ [{"id": "u1", "name": "alice"}, [{"id": "u1", "name": "alice"}]],
+ ids=["single", "list"],
+ )
+ def test_approval_assigned_users_normalized_to_list(self, assigned_users):
+ op = LLMOperator(task_id="t", prompt="p", llm_conn_id="c",
approval_assigned_users=assigned_users)
+ assert op.approval_assigned_users == [{"id": "u1", "name": "alice"}]
+
+ @pytest.mark.skipif(AIRFLOW_V_3_1_PLUS, reason="guard only fires on cores
before 3.1")
Review Comment:
Replaced the skipif with
`@patch("airflow.providers.common.ai.operators.llm.AIRFLOW_V_3_1_PLUS",
False)`, matching `test_agent.py`, so it runs on every lane. Dropped the
redundant method-level skipif on the normalization test too.
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -166,6 +174,12 @@ def defer_for_approval(
},
}
+ # Only pass assigned_users when set: cores before 3.2 have no such
argument, and the
Review Comment:
Passing `assigned_users=self.approval_assigned_users` unconditionally now;
the conditional and its comment are gone. PR body corrected to 3.1.
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -174,6 +188,7 @@ def defer_for_approval(
defaults=timeout_defaults,
multiple=False,
params=hitl_params,
+ **assignee_kwargs,
Review Comment:
Added to `llm.rst`, the param doc and the mixin docstring: the list is
stored when the review is first created and a cleared task re-runs against the
existing row. The core-side refresh is #73235.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm.py:
##########
@@ -173,10 +179,17 @@ def __init__(
"a positive approval_timeout to fire. "
"Set both, or leave on_approval_timeout as 'fail'."
)
+ if approval_assigned_users and not AIRFLOW_V_3_1_PLUS:
+ raise
AirflowOptionalProviderFeatureException("approval_assigned_users needs Airflow
3.1+.")
self.require_approval = require_approval
self.approval_timeout = approval_timeout
self.on_approval_timeout = on_approval_timeout
self.allow_modifications = allow_modifications
+ self.approval_assigned_users = (
Review Comment:
Normalization runs first, every entry must be a `{'id': str, 'name': str}`
dict, and the version guard tests the normalized list, so `{}` and `"alice"`
fail at parse time. Six malformed shapes covered in
`test_rejects_malformed_approval_assigned_users`.
##########
providers/common/ai/docs/operators/llm.rst:
##########
@@ -254,6 +254,12 @@ Reject the primary button:
:start-after: [START howto_operator_llm_approval]
:end-before: [END howto_operator_llm_approval]
+By default any user with the permission can answer the review. Pass
+``approval_assigned_users=[{"id": "<user-id>", "name": "<user-name>"}]`` to
+restrict it to named reviewers, the way
+:class:`~airflow.providers.standard.operators.hitl.HITLOperator` does with
+``assigned_users``. This needs Airflow 3.1+.
Review Comment:
Documented that `id` is the auth manager's user id (FAB: numeric row id as a
string), that 3.1.0 through 3.1.5 compare `name` as well, and that 3.1.6+
matches on `id` only. Placeholder renamed.
##########
providers/common/ai/tests/unit/common/ai/mixins/test_approval.py:
##########
@@ -176,6 +178,23 @@ def test_array_schema_passes_list_param_value(
defer_kwargs = approval_op_with_modifications.defer.call_args[1]
assert defer_kwargs["kwargs"]["generated_output"] == '["task_a"]'
+ @patch(HITL_TRIGGER_PATH, autospec=True)
+ @patch(UPSERT_HITL_PATH)
+ def test_assigned_users_are_forwarded(self, mock_upsert, mock_trigger_cls,
context):
+ users = [{"id": "u1", "name": "alice"}]
+ op = FakeOperator(approval_assigned_users=users)
+
+ op.defer_for_approval(context, "output")
+
+ assert mock_upsert.call_args[1]["assigned_users"] == users
+
+ @patch(HITL_TRIGGER_PATH, autospec=True)
+ @patch(UPSERT_HITL_PATH)
+ def test_assigned_users_omitted_when_unset(self, mock_upsert,
mock_trigger_cls, approval_op, context):
+ approval_op.defer_for_approval(context, "output")
+
+ assert "assigned_users" not in mock_upsert.call_args[1]
Review Comment:
Now asserts `mock_upsert.call_args[1]["assigned_users"] == ()`, pinning that
the kwarg is always passed.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm_sql.py:
##########
@@ -85,7 +85,7 @@ class LLMSQLQueryOperator(LLMOperator):
Human-in-the-Loop approval parameters are inherited from
:class:`~airflow.providers.common.ai.operators.llm.LLMOperator`
(``require_approval``, ``approval_timeout``, ``on_approval_timeout``,
- ``allow_modifications``).
+ ``allow_modifications``, ``approval_assigned_users``).
Review Comment:
Added `approval_assigned_users` to the `llm_sql.rst` prose.
--
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]
