kaxil commented on code in PR #71734:
URL: https://github.com/apache/airflow/pull/71734#discussion_r3973660078
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm_file_analysis.py:
##########
@@ -105,6 +105,9 @@ def __init__(
self.sample_rows = sample_rows
def execute(self, context: Context) -> Any:
+ if self.require_approval:
Review Comment:
I ran this against main and the LLM call was never reached. A list prompt
already dies inside `build_file_analysis_request`, because
`_build_text_preamble` drops the prompt straight into `"\n".join(lines)`
([file_analysis.py:654](https://github.com/apache/airflow/blob/main/providers/common/ai/src/airflow/providers/common/ai/utils/file_analysis.py#L654)),
so `agent.run_sync` never runs and `defer_for_approval` never gets a chance to
complain. What the guard does buy is skipping the storage reads and a much
clearer message, so the title and description overstate it a bit, and the title
is what lands in the provider changelog.
Given that, should this be unconditional instead of gated on
`require_approval`? `_build_text_preamble` needs a `str` either way, and the
`@task.llm_file_analysis` decorator already makes that point in its comment and
rejects non-strings with no approval check. With `require_approval=False` the
same prompt still reads every file and then fails with `TypeError: sequence
item 1: expected str instance, list found`.
##########
providers/common/ai/tests/unit/common/ai/operators/test_llm_file_analysis.py:
##########
@@ -346,3 +346,35 @@ def test_execute_with_approval_timeout(
op.execute(context=_make_context())
assert exc_info.value.timeout == timeout
+
+
[email protected](
Review Comment:
I don't think the skip is needed here. `validate_approval_prompt` has no
version gate (the mixin only branches on `AIRFLOW_V_3_3_PLUS`, for the pause
mechanism) and this test raises before any HITL code runs, so the new guard
ends up untested on the 3.0 core the provider still supports and CI still runs
(Compat 3.0.6). Minor: the class name says multimodal, but the prompt under
test is just a list.
--
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]
