kaxil commented on code in PR #74695:
URL: https://github.com/apache/airflow/pull/74695#discussion_r4314032144
##########
providers/common/ai/src/airflow/providers/common/ai/toolsets/hook.py:
##########
@@ -152,31 +146,14 @@ def __init__(
*,
allowed_methods: list[str],
tool_prefix: str = "",
- tool_name_prefix: str | None = None,
pinned_arguments: dict[str, Any] | None = None,
max_retries: int | None = None,
) -> None:
if tool_prefix and not tool_prefix.isidentifier():
# The prefixed names are rendered as Python function signatures
under
# code mode, so a name that is not an identifier breaks there.
raise ValueError(f"tool_prefix must be a valid Python identifier,
got {tool_prefix!r}.")
- if tool_name_prefix is not None:
- if tool_prefix and f"{tool_prefix}_" != tool_name_prefix:
- raise ValueError(
- f"tool_prefix={tool_prefix!r} and
tool_name_prefix={tool_name_prefix!r} conflict. "
- f"Pass only tool_prefix; tool_name_prefix is its
deprecated alias and must equal "
- f"{tool_prefix + '_'!r} when both are given."
- )
- warnings.warn(
- f"Parameter `tool_name_prefix` is deprecated. Use "
- f"`tool_prefix={tool_name_prefix.removesuffix('_')!r}`
instead; "
- f"HookToolset adds the '_' itself.",
- category=AirflowProviderDeprecationWarning,
- stacklevel=2,
- )
- name_prefix = tool_name_prefix
- else:
- name_prefix = f"{tool_prefix}_" if tool_prefix else ""
+ name_prefix = f"{tool_prefix}_" if tool_prefix else ""
Review Comment:
common.ai 1.0.0 went to PyPI today with `tool_name_prefix` still accepted
(deprecated, with its warning), and its `stability.rst` lists `HookToolset` as
stable: it "keeps its public parameters, their defaults and the behaviour
described below until the next major release". Dropping the alias now turns
`HookToolset(..., tool_name_prefix="s3_")` into a `TypeError` at parse on a
minor upgrade, and the changelog note for it lands in the already-released
1.0.0 section. Could the alias and its warning stay until 2.0, with the note
moved out of 1.0.0? The new `tool_prefix` on `SQLToolset` and
`DataFusionToolset` and the conflict hints are fine for 1.1.
##########
providers/common/ai/src/airflow/providers/common/ai/utils/output_type.py:
##########
@@ -38,6 +45,24 @@
_CONVERSION_ERRORS = (ValidationError, ValueError, TypeError, AttributeError,
KeyError, IndexError)
+def is_model_dump_required(output: Any, output_type: Any, *, serialize_output:
bool) -> bool:
+ """
+ Tell whether a ``BaseModel`` output must be pushed to XCom as 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 those shapes are dumped.
+ Without ``iter_pydantic_models`` there is no evidence either way, so the
caller's ``serialize_output`` decides.
+ """
+ if not isinstance(output, BaseModel):
+ return False
+ if serialize_output:
+ return True
+ if iter_pydantic_models is None:
+ return False
+ return type(output) not in iter_pydantic_models(output_type)
Review Comment:
The callers dump this with a python-mode `model_dump()`, so an `HttpUrl`,
`Path` or plain `Enum` field stays an object serde can't encode, and
`output_type=[Page, str]` with `url: HttpUrl` now fails at the XCom push. 1.0.0
pushed the instance there and serde's pydantic serializer handled it with
`mode="json"`. Separately, `[list[Item], str]` never gets here since the output
isn't a `BaseModel`, so those models still go out unregistered. Could this dump
with `pydantic_core.to_jsonable_python(output)` in one shared helper, with a
test that feeds the result to `serde.serialize`?
--
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]
