kaxil commented on code in PR #74695:
URL: https://github.com/apache/airflow/pull/74695#discussion_r4271166388
##########
providers/common/ai/docs/changelog.rst:
##########
@@ -44,6 +44,14 @@ Changelog
OpenSandbox server configuration, or pass
``SandboxSpec(block_network=False)`` for a sandbox
that may reach the network. See :ref:`the OpenSandbox backend
<sandbox-backend-opensandbox>`.
+.. note::
+ ``HookToolset`` no longer accepts the deprecated ``tool_name_prefix``
parameter. Pass
+ ``tool_prefix`` instead: ``tool_name_prefix="s3_"`` becomes
``tool_prefix="s3"``, since
+ ``HookToolset`` adds the underscore itself. ``MCPToolset``'s ``tool_prefix``
now defaults to
+ ``""``, like every other toolset's and like ``MCPHook``'s.
Review Comment:
`MCPHook`'s default changed in this PR too (`str | None = None` to `str =
""`), and several toolsets take no `tool_prefix` at all. Maybe: "`MCPToolset`'s
and `MCPHook`'s `tool_prefix` now default to `""` instead of `None`, like the
other toolsets that take a prefix."
##########
providers/common/ai/src/airflow/providers/common/ai/operators/agent.py:
##########
@@ -491,7 +494,7 @@ def __init__(
model_id: str | None = None,
fallback_conn_ids: list[str] | None = None,
system_prompt: str = "",
- output_type: type = str,
+ output_type: type | UnionType = str,
Review Comment:
`_agent_output_type` has a branch for a list `output_type` (`[str, int]`),
and `test_a_list_output_type_is_extended_not_nested` passes one, but this
annotation and the docstring only cover a single type or a union. Since this
signature is the one being settled, worth adding the list form here and on
`LLMOperator`, e.g. `type | UnionType | Sequence[type]`?
##########
providers/common/ai/src/airflow/providers/common/ai/toolsets/sql.py:
##########
@@ -369,7 +379,11 @@ def _is_ref_allowed(self, catalog: str | None, schema: str
| None, table: str) -
@property
def id(self) -> str:
- return f"sql-{self._db_conn_id}"
+ suffix = f"-{self._tool_prefix}" if self._tool_prefix else ""
+ return f"sql-{self._db_conn_id}{suffix}"
+
+ def _tool_name(self, base: str) -> str:
Review Comment:
The hints now follow `tool_prefix`, but the error users hit first steers
them the other way: two `SQLToolset`s on one agent fail with pydantic-ai's
default `tool_name_conflict_hint`, "wrap the toolset in a `PrefixedToolset`".
`.prefixed("pg")` leaves `_tool_prefix` empty, so the model sees
`pg_get_schema` while the retry text and the `get_schema` description still
name `list_tables` and `get_schema`, which belong to the other database. Could
`SQLToolset` and `DataFusionToolset` override `tool_name_conflict_hint` to say
"pass `tool_prefix=...`" instead?
--
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]
