kaxil commented on code in PR #74388: URL: https://github.com/apache/airflow/pull/74388#discussion_r4320054447
########## providers/common/ai/tests/unit/common/ai/utils/test_prompt.py: ########## @@ -0,0 +1,68 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import pytest +from pydantic_ai.messages import ( + BinaryContent, + CachePoint, + DocumentUrl, + ImageUrl, + TextContent, + UploadedFile, +) + +from airflow.providers.common.ai.utils.prompt import describe_prompt + + [email protected]( + ("prompt", "expected"), + [ + pytest.param("plain text", "plain text", id="str"), + pytest.param(["first", "second"], "first\nsecond", id="str-parts"), + pytest.param([TextContent(content="typed text")], "typed text", id="text-content"), + pytest.param( + [BinaryContent(data=b"\x89PNG", media_type="image/png")], "[image/png, 4 bytes]", id="binary" + ), + pytest.param( + [ImageUrl(url="https://example.com/a.png";)], "[image-url: https://example.com/a.png]";, id="url" Review Comment: Only `ImageUrl` hits the URL branch here, so narrowing the `isinstance` tuple on `prompt.py:53` to `(ImageUrl,)` still passes. Could you parametrize this over the four URL classes? If one of them is a YouTube `VideoUrl`, you'll see it render as `https://www.youtube.com/watch` with the `v=` dropped, so maybe keep that parameter when `part.is_youtube` is true. ########## providers/common/ai/tests/unit/common/ai/mixins/test_approval.py: ########## @@ -321,10 +322,17 @@ def test_no_modifications_params_empty(self, mock_upsert, mock_trigger_cls, appr call_kwargs = mock_upsert.call_args[1] assert call_kwargs["params"] == {} - def test_raises_on_non_string_prompt(self, context): - op = FakeOperator(prompt=["Describe this:", object()]) # type: ignore[arg-type] - with pytest.raises(TypeError, match="non-string prompt"): - op.defer_for_approval(context, "output") + @patch(HITL_TRIGGER_PATH, autospec=True) + @patch(UPSERT_HITL_PATH) + def test_body_renders_a_sequence_prompt(self, mock_upsert, mock_trigger_cls, context): + op = FakeOperator(prompt=["Describe this:", BinaryContent(data=b"\x89PNG", media_type="image/png")]) # type: ignore[arg-type] Review Comment: Nit: the protocol now takes `str | Sequence[UserContent]`, so widening `FakeOperator.prompt` (line 70) to match would let this `# type: ignore[arg-type]` go. ########## providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py: ########## @@ -167,25 +168,13 @@ class LLMApprovalMixin: - ``on_approval_timeout`` (``Literal["fail", "approve", "reject"]``) - ``approval_notifiers`` (``Sequence[BaseNotifier]``) - ``approval_assigned_users`` (``list[HITLUser]``) - - ``prompt`` (``str``) + - ``prompt`` (``str`` or ``Sequence[UserContent]``) """ APPROVE = "Approve" REJECT = "Reject" TIMEOUT_DEFAULTS: ClassVar[dict[str, list[str]]] = {"approve": [APPROVE], "reject": [REJECT]} - def validate_approval_prompt(self: DeferForApprovalProtocol) -> None: Review Comment: `validate_approval_prompt` is public on `LLMApprovalMixin` in the 1.0.0 release, and the class docstring invites custom operators to use the mixin. After this, a subclass that copied the in-tree `if self._may_review: self.validate_approval_prompt()` call gets `AttributeError`, and one that overrode it to add checks stops being called, since `defer_for_approval` no longer invokes it. Could we keep it as a no-op that `defer_for_approval` still calls, or, if dropping it is intended, add a breaking-change note to the provider changelog? -- 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]
![https://example.com/a.png"](https://example.com/a.png"){kind=link}
![https://example.com/a.png]"](https://example.com/a.png]"){kind=link}