YAshhh29 commented on PR #69812:
URL: https://github.com/apache/airflow/pull/69812#issuecomment-4965462311
Thanks @kaxil — this is a genuinely great review. Every one of the five
catches
would have been a real footgun for users:
- The `model_dump()` enum bug would only surface at XCom push, after the API
call
had already been paid for — the exact kind of silent-until-production bug
that
makes structured-output code frustrating to trust.
- The `ValidationError` gap was worse: I'd been reasoning about
`output_parsed=None`
as "the" failure mode, but truncation raises inside `parse()` before
assignment,
so the operator's whole failure-handling story never fired. That's the
sort of
claim that needs to hold in the docs and didn't, before this review.
- The `previous_response_id` / model-requirement contradictions, the
`ParsedResponse[Any]` throwing away the SDK's own generic, the pydantic
import
down inside a test function — each fair on its own, and together they add
up
to code that reads like "someone shipped the first working version." Which
is
exactly what happened. Appreciate the read.
All five landed in `beb3d9e`. Recap:
1. **`model_dump(mode="json")`** — matches what
`airflow.serialization.serde.serializers.pydantic` already does. New
regression
test `test_openai_response_operator_structured_output_dumps_enum_as_json`
uses
a plain `str`-mixin-free `enum.Enum` field and asserts the dumped value is
`"high"` (str), not a `Priority` instance. Reverting `mode="json"` fails
it.
2. **`ValidationError` -> `ValueError`** — `parse_response` is now wrapped in
`try/except ValidationError`, re-raising as `ValueError` mentioning the
`text_format` class name. New test
`test_openai_response_operator_structured_output_validation_error_raises`
feeds a real `ValidationError` (built via
`_StructuredPerson.model_validate({})`)
through `parse_response.side_effect` and asserts the conversion.
3. **Richer refusal message** — the `output_parsed=None` `ValueError` now
includes `status`, `error`, and `incomplete_details` from the API
response.
The existing refusal test asserts all three snippets are present in the
message, so a regression back to "just say status" would fail.
4. **Docstring / example DAG / rst** — dropped the "requires
`gpt-4o-2024-08-06` or later" claim from the operator docstring, the hook
docstring, and the how-to guide. Dropped the `model="gpt-4o-2024-08-06"`
override + explanatory comment from the example DAG. Removed
`previous_response_id` from the "use hook directly" list in the docstring.
5. **Generic typing + hook-methods list** — `parse_response` is now
`_TextFormatT = TypeVar("_TextFormatT", bound="BaseModel")` returning
`ParsedResponse[_TextFormatT]`. Callers get `output_parsed` typed as
`_TextFormatT | None`, matching the SDK's own signature. Also added
`parse_response` to the Responses bullet in `docs/operators/openai.rst`.
6. **Test import** — `from pydantic import BaseModel` moved to the module top
of `tests/unit/openai/hooks/test_openai.py`.
Bonus: the failing `Build documentation (--spellcheck-only)` job was tripping
on "parseable" — rephrased across the docstring, the `ValueError` message,
and
the how-to guide to standard English so spellcheck passes.
### Author review
I read every fix line-by-line before pushing and validated the two behavioral
claims that mattered most against the installed `openai` and `pydantic`
packages:
that `Responses.parse` raises `ValidationError` inside its own body (so a
wrap
at the operator's call site is what catches it), and that `model_dump()`
defaults
to `mode="python"` while `mode="json"` renders `enum.Enum` fields as their
JSON
values. Both matched what your review described, so the fixes hold on the
same
grounds. The design calls the fixes preserve — one operator (not a new
class),
`ValueError` (not a new exception class), `model_dump(mode="json")` (not a
custom serializer) — are still mine.
---
Drafted-by: GitHub Copilot (Claude Opus 4.6); reviewed by @YAshhh29 before
posting
--
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]
