pankajkoti commented on code in PR #74473:
URL: https://github.com/apache/airflow/pull/74473#discussion_r4251354398
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -195,10 +195,10 @@ def defer_for_approval(
raw_output = output
if isinstance(output, BaseModel):
- output = output.model_dump_json()
+ output = output.model_dump_json(by_alias=True)
Review Comment:
Good catch, thanks. You are right that dumping by alias only fixed
`Field(alias=...)`. Fixed in d869d0e4ef. Both serializers now go through one
helper, `dump_output_to_json`, which dumps by field name with
`round_trip=True`. `rehydrate_pydantic_output` validates with `by_alias=True,
by_name=True`, so an edit written with either spelling is accepted. Computed
fields are left out of the dump so a model with `extra="forbid"` still
validates back. `test_an_unedited_review_string_validates_back` now does the
dump-then-validate round trip through `_to_string` for a validation alias, a
serialization-only alias, `serialize_by_alias`, a list of aliased models and a
computed-field model with `extra="forbid"`. The approval gate side shares the
helper, and `test_approval.py` pins its dump for the same alias styles.
##########
providers/common/ai/src/airflow/providers/common/ai/utils/output_type.py:
##########
@@ -30,25 +35,50 @@ def rehydrate_pydantic_output(
serialize_output: bool,
) -> Any:
"""
- Turn a JSON string back into a value of ``output_type``.
+ Turn the reviewed string (JSON, or bare text for str-valued types) back
into ``output_type``.
Used by the HITL/approval paths in ``LLMOperator`` and ``AgentOperator``
that round-trip the output through a string when deferring to a human
reviewer. ``str`` outputs pass through unchanged; any other ``output_type``
(``BaseModel`` subclass, ``int``, ``list[str]``, ...) is validated with a
- pydantic ``TypeAdapter``. When validation fails (reviewer edited the string
- into something the type rejects), returns ``raw`` unchanged.
+ pydantic ``TypeAdapter``, first as JSON and then as bare text.
+ An ``output_type`` pydantic has no schema for, or an output function,
returns ``raw`` unchanged.
When ``serialize_output`` is ``True``, returns the model dumped to a
``dict`` -- matches the operator's ``serialize_output=True`` opt-in for
consumers that want the dict shape.
+
+ :raises ReviewedOutputValidationError: If ``output_type`` is not ``str``
and ``raw``
+ (typically a reviewer's edit) is neither valid JSON for it nor valid
as plain text.
"""
if output_type is str:
return raw
try:
- rehydrated = TypeAdapter(output_type).validate_json(raw)
- except (ValidationError, ValueError, TypeError):
+ adapter: TypeAdapter[Any] = TypeAdapter(output_type)
+ except PydanticSchemaGenerationError:
+ # Nothing to validate against, so the reviewed text is all there is.
return raw
+ if adapter.core_schema["type"] == "call":
Review Comment:
Reproduced and fixed in d869d0e4ef. The check now unwraps a `definitions`
schema before looking for a `call`, and a NamedTuple (whose schema is also a
`call`) is still validated.
`test_returns_raw_for_output_function_without_calling_it` now takes your
`route(order: Order)` shape with the shared `Address` as a second case, and
asserts the function is not called.
##########
providers/common/ai/src/airflow/providers/common/ai/utils/output_type.py:
##########
@@ -30,25 +35,50 @@ def rehydrate_pydantic_output(
serialize_output: bool,
) -> Any:
"""
- Turn a JSON string back into a value of ``output_type``.
+ Turn the reviewed string (JSON, or bare text for str-valued types) back
into ``output_type``.
Used by the HITL/approval paths in ``LLMOperator`` and ``AgentOperator``
that round-trip the output through a string when deferring to a human
reviewer. ``str`` outputs pass through unchanged; any other ``output_type``
(``BaseModel`` subclass, ``int``, ``list[str]``, ...) is validated with a
- pydantic ``TypeAdapter``. When validation fails (reviewer edited the string
- into something the type rejects), returns ``raw`` unchanged.
+ pydantic ``TypeAdapter``, first as JSON and then as bare text.
+ An ``output_type`` pydantic has no schema for, or an output function,
returns ``raw`` unchanged.
When ``serialize_output`` is ``True``, returns the model dumped to a
``dict`` -- matches the operator's ``serialize_output=True`` opt-in for
consumers that want the dict shape.
+
+ :raises ReviewedOutputValidationError: If ``output_type`` is not ``str``
and ``raw``
+ (typically a reviewer's edit) is neither valid JSON for it nor valid
as plain text.
"""
if output_type is str:
return raw
try:
- rehydrated = TypeAdapter(output_type).validate_json(raw)
- except (ValidationError, ValueError, TypeError):
+ adapter: TypeAdapter[Any] = TypeAdapter(output_type)
+ except PydanticSchemaGenerationError:
Review Comment:
Agreed. Added in d869d0e4ef.
`test_returns_raw_for_type_pydantic_cannot_build_a_schema_for` covers a plain
class as `output_type`, and `TestToString` takes a plain-class instance and an
unresolved forward reference, so the `str(output)` fallback is pinned. While in
there I made the guard catch `PydanticUserError`, the parent of
`PydanticSchemaGenerationError`, because an unresolved forward reference raises
a different subclass. The fallback in `_to_string` now logs a warning, and a
test asserts it.
--
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]
