kaxil commented on code in PR #74695: URL: https://github.com/apache/airflow/pull/74695#discussion_r4504710594
########## providers/common/ai/docs/changelog.rst: ########## @@ -34,17 +34,16 @@ Changelog The worker cannot register a model class for XCom deserialization when ``output_type`` is a list of output types such as ``[Summary, int]``, an output marker such as ``ToolOutput(Summary)``, an output function, a ``TypeAliasType`` or a ``NewType``. For these - output types, ``AgentOperator`` and ``@task.agent`` now push each model in the output as a - JSON-compatible dict (``model_dump(mode="json")``), including models inside a list, tuple or - dict. On Airflow < 3.3, the worker registers no model class at all, so a model inside a list, - tuple or dict is now pushed as a JSON-compatible dict for every ``output_type``. A top-level - model is still dumped with plain ``model_dump()`` when ``serialize_output=True`` is set or on - Airflow < 3.3. + output types, ``LLMOperator``, ``@task.llm``, ``LLMFileAnalysisOperator``, ``AgentOperator`` and Review Comment: 1.1.0 shipped on Nov 1, so these edits now rewrite a released section, and it ends up describing things 1.1.0 didn't do (the LLM operators dumping `[MyModel, int]`, the agent pushing `list[MyModel]` as instances before 3.3). Could the 1.1.0 text go back to what the tag has, with this PR's changes in a new note above it, and the same split for the 1.0.0-to-1.1.0 upgrade bullets in `structured_output.rst`? That note also needs the agent's own `[M, int]` changes from 1.1.0 (`fullName` becomes `full_name` for a `serialize_by_alias` model, a tuple becomes a list, a `date` next to the model becomes a string) and the MCP `tool_prefix` default change, which has no line now. ########## providers/common/ai/src/airflow/providers/common/ai/utils/output_type.py: ########## @@ -38,6 +46,59 @@ _CONVERSION_ERRORS = (ValidationError, ValueError, TypeError, AttributeError, KeyError, IndexError) +def is_model_dump_required(output: Any, output_type: Any) -> bool: + """ + Tell whether an output holding ``BaseModel`` instances must be pushed to XCom dumped. + + The instances are looked up in the output itself, as ``_iter_models_in_output`` walks it: the elements + of a list, tuple (a NamedTuple included), set or frozenset, and the keys and values of a dict. A downstream task can load a model from XCom only if the worker + registered the model's class, and the worker registers the classes ``iter_pydantic_models`` finds in + ``output_type``. It finds none in a list of output types, an output marker, an output function, a + ``TypeAliasType`` or a ``NewType``, so an instance of a class it did not find is dumped. Without + ``iter_pydantic_models`` there is no evidence that a class is unregistered, so nothing is dumped. + """ + if iter_pydantic_models is None: Review Comment: 1.1.0 went out on Nov 1 with `AgentOperator` dumping a model inside a list, tuple or dict before Airflow 3.3 (`_dump_unregistered_models(output, set())` at the tag), and its `structured_output.rst` says so. This early return puts the agent's `list[Summary]` back to instances, so on 3.1 and 3.2 the downstream task fails with "not found in allow list" unless the class is allow-listed. No class is registered at all without the walker, so could a missing `iter_pydantic_models` count as an empty registered set instead? That keeps the one helper and also makes the LLM operators' `list[M]` loadable before 3.3, which the changelog would then need to say. -- 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]
