kaxil commented on code in PR #74447:
URL: https://github.com/apache/airflow/pull/74447#discussion_r4218074614
##########
providers/common/ai/src/airflow/providers/common/ai/toolsets/hook.py:
##########
@@ -95,10 +99,23 @@ def __init__(
hook: BaseHook,
*,
allowed_methods: list[str],
- tool_name_prefix: str = "",
+ tool_prefix: str = "",
Review Comment:
The name now matches the other toolsets, but the behaviour doesn't.
`SandboxToolset` and `ObjectStorageToolset` add the `_` themselves
(`f"{self._tool_prefix}_{base}"`) and reject a prefix that isn't an identifier,
and `MCPToolset` goes through pydantic-ai's `prefixed()`, which also joins with
`_`. Here the prefix is concatenated raw, so `HookToolset(...,
tool_prefix="s3")` silently names the tool `s3list_keys`, and the `"s3_"` from
these docs copied onto a `SandboxToolset` gives `s3__run_command`.
Since nothing uses `tool_prefix` on `HookToolset` yet, could it join with
`_` and run the same `isidentifier()` check, keeping the raw join only for the
deprecated `tool_name_prefix`? The both-passed check would then compare
`tool_name_prefix` against `f"{tool_prefix}_"`, and the docs and example Dags
would switch to `"s3"` / `"http"`. `stability.rst` lists the HookToolset tool
naming as stable, where changing it needs a major release, and already
documents MCP names as `<tool_prefix>_<name>`, so now is the cheap time to line
them up.
##########
providers/common/ai/src/airflow/providers/common/ai/tools/__init__.py:
##########
@@ -166,7 +166,7 @@ def collect_tools(sources: Iterable[ToolProvider |
AirflowTool]) -> list[Airflow
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 "
+ "name prefixes where they take one (HookToolset's tool_prefix, the
tool_prefix of "
Review Comment:
After the rename this reads "HookToolset's tool_prefix, the tool_prefix of
SandboxToolset", and `MCPToolset` and `ObjectStorageToolset` take `tool_prefix`
too and also feed `collect_tools`. Maybe just "Give the toolsets different
tool_prefix values 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]
