You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ColtenOuO (via GitHub)" <gi...@apache.org> on 2026/08/17 20:39:27 UTC
[PR] Avoid unnecessary LLM calls when require_approval prompt is invalid [airflow]
ColtenOuO opened a new pull request, #71734:
URL: https://github.com/apache/airflow/pull/71734
### Sumarry
`LLMFileAnalysisOperator.execute()` was the only `require_approval`-capable operator in `providers/common/ai` that didn't validate the prompt before doing any real work. Its four siblings (`LLMOperator`, `LLMBranchOperator`, `LLMSqlOperator`, `LLMSchemaCompareOperator`) all call `validate_approval_prompt()` as the first line of `execute()`, so a non-string prompt (e.g. from a native-templating render that resolves to a `Sequence[UserContent]`) is rejected immediately.
`LLMFileAnalysisOperator` only discovered the same error later, inside `defer_for_approval()` -- by which point it had already read the target file(s) from storage and made a real (possibly multimodal) LLM call, only to then reject the output because `require_approval=True` doesn't support a non-string prompt.
### What
Add the same `validate_approval_prompt()` preflight to `LLMFileAnalysisOperator.execute()`, mirroring the other four operators,
plus a regression test (`TestLLMFileAnalysisOperatorMultimodalPromptGuard`) asserting the failure happens before `build_file_analysis_request`/`agent.run_sync` are ever called.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject non-string prompts in LLMFileAnalysisOperator before reading files [airflow]
Posted by "ColtenOuO (via GitHub)" <gi...@apache.org>.
ColtenOuO commented on code in PR #71734:
URL: https://github.com/apache/airflow/pull/71734#discussion_r3989043713
##########
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
+
+
+@pytest.mark.skipif(
Review Comment:
Thanks, removed the skip and renamed the class. It's now parametrized over `require_approval` so both paths are covered.
The same `AIRFLOW_V_3_1_PLUS` skip is on the prompt-guard tests in `test_llm.py`, `test_llm_sql.py`, and `test_agent.py`, and those also raise before any HITL code runs. Should I fix them in a separate follow-up PR?
(because the orgional test implementation is base on these files, so I notice this point ...or am I misunderstanding something?)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject non-string prompts in LLMFileAnalysisOperator before reading files [airflow]
Posted by "ColtenOuO (via GitHub)" <gi...@apache.org>.
ColtenOuO commented on PR #71734:
URL: https://github.com/apache/airflow/pull/71734#issuecomment-5790078581
Thanks for mreging, I've updated the PR's description for AI disclosure.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject non-string prompts in LLMFileAnalysisOperator before reading files [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #71734:
URL: https://github.com/apache/airflow/pull/71734
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject non-string prompts in LLMFileAnalysisOperator before reading files [airflow]
Posted by "ColtenOuO (via GitHub)" <gi...@apache.org>.
ColtenOuO commented on PR #71734:
URL: https://github.com/apache/airflow/pull/71734#issuecomment-5645773770
CI looks good :D ready for the next round review!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid unnecessary LLM calls when require_approval prompt is invalid [airflow]
Posted by "fat-catTW (via GitHub)" <gi...@apache.org>.
fat-catTW commented on PR #71734:
URL: https://github.com/apache/airflow/pull/71734#issuecomment-5323526760
Thanks for contribution!
LGTM
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject non-string prompts in LLMFileAnalysisOperator before reading files [airflow]
Posted by "ColtenOuO (via GitHub)" <gi...@apache.org>.
ColtenOuO commented on code in PR #71734:
URL: https://github.com/apache/airflow/pull/71734#discussion_r3989043713
##########
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
+
+
+@pytest.mark.skipif(
Review Comment:
Thanks, removed the skip and renamed the class. It's now parametrized over `require_approval` so both paths are covered.
The same `AIRFLOW_V_3_1_PLUS` skip is on the prompt-guard tests in `test_llm.py`, `test_llm_sql.py`, and `test_agent.py`, and those also raise before any HITL code runs. Should I fix them here, or in a separate follow-up PR?
(because the orgional test implementation is base on these files, so I notice this point ...or am I misunderstanding something?)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject non-string prompts in LLMFileAnalysisOperator before reading files [airflow]
Posted by "ColtenOuO (via GitHub)" <gi...@apache.org>.
ColtenOuO commented on PR #71734:
URL: https://github.com/apache/airflow/pull/71734#issuecomment-5757254761
fixed the conflicts, thanks for reviewing!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid unnecessary LLM calls when require_approval prompt is invalid [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
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
+
+
+@pytest.mark.skipif(
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject non-string prompts in LLMFileAnalysisOperator before reading files [airflow]
Posted by "ColtenOuO (via GitHub)" <gi...@apache.org>.
ColtenOuO commented on code in PR #71734:
URL: https://github.com/apache/airflow/pull/71734#discussion_r3989007451
##########
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:
Yeah... I had a slight misunderstanding of the codebase earlier. My apologies, I should have run it locally to double-check first.
I have updated the PR title and description. Also, making the check unconditional rather than tied to `require_approval` makes total sense to me, handling it directly inside with a clearer error message provides a much better experience for users.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org