kaxil commented on code in PR #71051:
URL: https://github.com/apache/airflow/pull/71051#discussion_r3740526728
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm_schema_compare.py:
##########
@@ -313,8 +324,23 @@ def execute(self, context: Context) -> dict[str, Any]:
self.log.info("Running LLM schema comparison...")
result = agent.run_sync(self.prompt, usage_limits=self.usage_limits)
log_run_summary(self.log, result)
+ output = result.output
+
+ if self.require_approval:
+ severity_counts = Counter(mismatch.severity for mismatch in
output.mismatches)
+ summary = ", ".join(
+ f"{severity_counts[severity]} {severity}"
+ for severity in ("critical", "warning", "info")
+ if severity_counts[severity]
+ )
+ body = (
+ f"Compatible: {output.compatible}"
+ + (f" (mismatches: {summary})" if summary else "")
+ + f"\n\n```\nPrompt:
{self.prompt}\n\n{output.model_dump_json(indent=2)}\n```"
+ )
+ self.defer_for_approval(context, output, body=body) # type:
ignore[misc]
Review Comment:
`defer_for_approval` always raises, so on the approval path the `Schema
comparison result` log two lines below never runs, and `execute_complete`
doesn't log it either. The result then only exists in the HITL body, never in
the task log. `LLMSQLOperator` logs the generated SQL before it defers --
moving the log above this block would keep the two paths consistent.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm_schema_compare.py:
##########
@@ -126,9 +134,9 @@ def __init__(
system_prompt: str = DEFAULT_SYSTEM_PROMPT,
**kwargs: Any,
) -> None:
- kwargs.pop("output_type", None)
- if kwargs.get("require_approval"):
- raise ValueError("require_approval=True is not supported by
LLMSchemaCompareOperator.")
+ kwargs["output_type"] = SchemaCompareResult
+ # execute() always returns a dict, so the approval resume path must too
+ kwargs["serialize_output"] = True
Review Comment:
With `allow_modifications=True` this contract doesn't quite hold.
`LLMOperator.execute_complete` goes through `rehydrate_pydantic_output`, which
returns the raw string unchanged when validation fails, so a reviewer edit that
isn't valid `SchemaCompareResult` JSON lands in XCom as a `str`. I ran the
cases: `{"compatible": true, "mismatches": []}` (missing `summary`) and `looks
fine to me` both come back as strings, while the unmodified approve path
correctly gives a dict. The conditional example in the docs does
`comparison_result["compatible"]`, which would raise `TypeError: string indices
must be integers` on that value.
`LLMSQLOperator.execute_complete` overrides and re-validates whenever
`output != generated_output`; the same guard here (fail the task when the
edited JSON doesn't parse into `SchemaCompareResult`) would keep the dict
promise.
##########
providers/common/ai/docs/operators/llm_schema_compare.rst:
##########
@@ -114,6 +114,23 @@ The callable may also return a non-empty
``Sequence[UserContent]`` for
multimodal inputs -- see
:ref:`@task.agent multimodal prompts <howto/operator:agent-multimodal>`.
+Human-in-the-Loop Approval
+--------------------------
+
+Set ``require_approval=True`` to pause the task after the comparison and wait
+for a human reviewer to approve the result before it is returned. The review
+body shows the compatibility verdict, a mismatch severity summary, and the
+full result JSON. Rejecting the review, or letting ``approval_timeout``
+expire, fails the task:
+
+.. exampleinclude::
/../../ai/src/airflow/providers/common/ai/example_dags/example_llm_schema_compare.py
+ :language: python
+ :start-after: [START howto_operator_llm_schema_compare_approval]
+ :end-before: [END howto_operator_llm_schema_compare_approval]
+
+``approval_timeout``, ``allow_modifications``, and the rest of the approval
Review Comment:
`llm_branch.rst` spells out that `require_approval=True` needs a string
prompt, and that a callable returning `Sequence[UserContent]` raises
`TypeError` before the LLM call. The same constraint applies here
(`test_execute_rejects_sequence_prompt_with_require_approval` covers it), and
the paragraph directly above this new section tells readers the callable may
return a `Sequence[UserContent]`. Worth carrying the same note over.
--
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]
