kaxil commented on code in PR #74447:
URL: https://github.com/apache/airflow/pull/74447#discussion_r4237590014
##########
providers/common/ai/src/airflow/providers/common/ai/toolsets/hook.py:
##########
@@ -95,10 +101,30 @@ def __init__(
hook: BaseHook,
*,
allowed_methods: list[str],
- tool_name_prefix: 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(
+ "Parameter `tool_name_prefix` is deprecated. Use the parameter
`tool_prefix` instead.",
Review Comment:
Every documented call so far passed `tool_name_prefix="s3_"`, so the most
likely response to this warning is renaming the kwarg to `tool_prefix="s3_"`.
That passes `isidentifier()`, and the tools silently become `s3__list_keys` /
`s3__read_key`, which breaks system prompts or approval rules that name the old
tools. Could the warning give the migrated value, e.g. `f"... Use
tool_prefix={tool_name_prefix.removesuffix('_')!r} instead; HookToolset adds
the '_' itself."`?
##########
providers/common/ai/src/airflow/providers/common/ai/tools/__init__.py:
##########
@@ -165,8 +165,7 @@ def collect_tools(sources: Iterable[ToolProvider |
AirflowTool]) -> list[Airflow
names = [tool.name for tool in tools]
if duplicates := sorted({name for name in names if names.count(name) > 1}):
raise ValueError(
- f"More than one tool is named {', '.join(duplicates)}. Give the
toolsets different tool "
- "name prefixes where they take one (HookToolset's
tool_name_prefix, the tool_prefix of "
- "SandboxToolset), so the model can call each of them."
+ f"More than one tool is named {', '.join(duplicates)}. "
+ "Give the toolsets different tool_prefix values so the model can
call each of them."
Review Comment:
My suggested wording dropped the "where they take one" qualifier, and that
qualifier mattered. `SQLToolset` and `DataFusionToolset` always emit
`list_tables` / `get_schema` / `query` and take no `tool_prefix`, so two SQL
toolsets passed to the ADK or Strands adapter hit this error, and following its
advice gives a `TypeError`. Maybe "Give the toolsets different tool_prefix
values, where they take one, so the model can call each of them."
--
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]
