feat: add prefix to component tool names - #1432
Conversation
|
@akihikokuroda, we should create a full proposal for this. I don't know if we want to prefix component tools with an index. I think we need to ensure that performance isn't impacted. We also need to make sure that we can clearly link tools to any given component, which might require more testing / planning. |
|
Small note on just an initial pass, I think this will have conflicts will #1430 adding |
|
@jakelorocco Here is a proposal. Proposed Solution: Object ID-Based PrefixingDesignUse Python object identity as the stable component identifier: component_id = hex(id(component))[-8:] # e.g., "a1b2c3d4"
prefixed_name = f"component_{component_id}.{original_tool_name}"Properties
Example: Multi-Turn Scenario with ID-Based PrefixingAdvantages Over Index-Based1. Multi-Turn Stability
2. Prevents Parallel Tool Collisions
3. Component Context Independence
4. Better Observability
5. Integrates with PR #1430 (Tool Tracing)
Performance ImpactAnalysis
Example with 10 tools:
Token CostName length comparison:
Example with 10 tools in JSON schema:
Conclusion: Performance impact is negligible. No performance concerns.
About concern:
Better Solution: Metadata on TemplateRepresentationInstead of reverse mapping, store component metadata on TemplateRepresentation: @dataclass
class TemplateRepresentation:
obj: Any
args: dict[...]
tools: dict[str, AbstractMelleaTool] | None = None
tool_name_mapping: dict[str, str] | None = None
# NEW: Component identity metadata
component_id: str | None = None # e.g., "a1b2c3d4"
component_type: str | None = None # e.g., "SearchAgent"
component_description: str | None = None # Optional Why This is Better
Usage Example# At tool registration time:
if isinstance(action, Component):
tr = action.format_for_llm()
if isinstance(tr, TemplateRepresentation):
tr.component_id = hex(id(action))[-8:]
tr.component_type = type(action).__name__
# tool_name_mapping already set
# All metadata in one place ✅
# At trace time:
span.set_attribute("mellea.component.id", tr.component_id)
span.set_attribute("mellea.component.type", tr.component_type)
# Metadata available without reverse mapping ✅
# In error messages:
error = f"Tool {tool_name} failed (component: {tr.component_type})"
# Clear attribution without reverse mapping ✅
# For debugging:
# User reads error message, sees component type, can correlate
# No need to reverse-map tool name back to component ✅ |
|
@akihikokuroda, I think this is still a good idea but we probably need some evaluations to indicate that the model can properly track the difference between components. Especially since we don't print component ids when outputting them currently. |
|
@jakelorocco I added examples. docs/examples/components/pattern2_context_and_tools.py shows the component tools renaming, executing and telemetry output. |
planetf1
left a comment
There was a problem hiding this comment.
Thanks for tackling the tool-name-collision problem (#95) — the underlying idea (auto-prefix component tools so they don't clobber each other) is sound. A few things worth resolving before this merges, most importantly a correctness bug that would break real OpenAI/Azure usage but is currently invisible to CI because the only exercising examples are Ollama-gated.
Two non-code notes:
- The PR description says tools are prefixed with
component{N}.(sequential index, e.g.component0.search), but the shipped code usescomponent_{hex(id(action))[-8:]}.(object-identity hash). Worth updating the description so reviewers/future readers aren't misled. - Small thing, but since this will likely be squash-merged and the PR title becomes the commit message: "prfefix" → "prefix" in the title itself would be good to fix before merging.
| from mellea.core.base import AbstractMelleaTool | ||
| from mellea.formatters import TemplateFormatter | ||
| from mellea.stdlib.context import ChatContext | ||
| from mellea.stdlib.functional import _call_tools |
There was a problem hiding this comment.
This imports the private mellea.stdlib.functional._call_tools directly, and it's used nowhere else outside functional.py itself (only transform() calls it internally, e.g. functional.py:469). Docs/examples are what people copy from, so this is teaching users to reach into an implementation detail. If the goal is just "call the LLM and run any tools it invokes," this example can go through m.transform(...) / act(..., tool_calls=True), which already does this internally. If there's a genuine need to trigger tool execution manually outside of transform/act, that's a gap worth closing with a public helper rather than exporting from examples via a private import.
There was a problem hiding this comment.
It is not true that the act() is calling _call_tools.
| Function | File | Line | Calls _call_tools? | Executes Tools? |
|---|---|---|---|---|
session.act() |
session.py | 482 | ❌ No | ❌ No |
mfuncs.act() |
functional.py | 89 | ❌ No | ❌ No |
aact() |
functional.py | 576 | ❌ No | ❌ No |
backend.generate_from_context() |
backends/*.py | - | ❌ No | ❌ No |
transform() |
functional.py | 426 | ✅ Yes (line 469) | ✅ Yes |
query() |
functional.py | - | ✅ Yes | ✅ Yes |
| React framework | frameworks/react.py | 123 | ✅ Yes | ✅ Yes |
There was a problem hiding this comment.
The two private example files have now gone, but the README still retains their sections and run commands. It also recommends calling _call_tools() directly, including in the general guidance, even though call_tools() is public and the two remaining examples use it. Please remove the stale private-example sections and replace the remaining _call_tools() recommendations with call_tools().
| from mellea.core.base import AbstractMelleaTool | ||
| from mellea.formatters import TemplateFormatter | ||
| from mellea.stdlib.context import ChatContext | ||
| from mellea.stdlib.functional import _call_tools |
There was a problem hiding this comment.
Same private-import concern as in duplicate_tool_names.py:33 — see that comment.
It's more an issue of us ensuring this approach works long term and is resilient. If we go this route, we need data / benchmarks to prove that this approach works. We also have no way to correlate the components that are being output as messages with the prefixed tools still. |
|
@jakelorocco This should be a bug fix. Can we leave this bug as is?
I don't have idea what data / benchmarks needs. Please advise. The component doesn't have name. How is it identified?
I wonder what this correlation is used for? Thanks! |
I think these things should be addressed before merging. I think potential issues related to the prefixes are large enough that we ought to ensure the approach works across a wide range of situations before committing to it. Tools that come from components usually relate to that component. So the fact that we don't have a way to match a tool prefix to its serialized component message seems problematic. The fact that we don't know if this holds up for more complicated examples where many components are in scope / being added. For instance, if we ask a model to transpose all charts, is it smart enough to make that same tool call multiple times with the different prefixes. Do we need to add language to a system prompt to explain how to use the component-prefixed tools? I'm asking these questions because this likely isn't the only way to implement a feature like this. For example, instead, we could force tool calls to require an object identifier as a parameter instead of appending multiple of the "same" tools. An approach like that might actually work better by potentially reducing the context required to encode all the tools. |
|
I'll hold off further PR review until we address the design questions Jake raised, feel free to loop me in if you have a call to discuss things |
|
Prototyped the "we could force tool calls to require an object identifier as a parameter" It hits an issue and here is the analysis. The issue is that only ONE tool is in the tools dict (['query']), so the LLM doesn't know there are two different "query" tools from different components. It just sees one tool and calls it. This reveals a fundamental problem with the parameter approach when tools have duplicate names:
With multiple components having the same tool name, we need a different strategy:
The parameter approach I implemented assumes one-to-one tool names. To truly support multiple components with identical tool names, we'd need either:
Here is the list-based implementation analysis: Tool Structure: Where It's Defined and UsedCurrent Structure1. DefinitionFile: class TemplateRepresentation:
tools: dict[str, AbstractMelleaTool] | None = NoneType:
2. Tool ExtractionFile: def add_tools_from_context_actions(
tools_dict: dict[str, AbstractMelleaTool],
ctx_actions: list[Component | CBlock | ModelOutputThunk] | None,
):
for action in ctx_actions:
tr = action.format_for_llm()
for tool_name, func in tr.tools.items():
wrapped_tool = wrap_tool_with_component_id(func, component_id)
tools_dict[tool_name] = wrapped_tool # ← OVERWRITES duplicates!3. Tool UsageFile: def convert_tools_to_json(tools: dict[str, AbstractMelleaTool]) -> list[dict]:
return [t.as_json_tool for t in tools.values()]Converts dict values to JSON array for LLM. 4. Backend Tool PassingExample: chat_response = self._async_client.chat(
model=self._model_id,
messages=conversation,
tools=[t.as_json_tool for t in tools.values()], # ← Dict values to list
...
)The Problem with Dict StructureWhen multiple components have the same tool name:
SolutionsOption A: Keep Dict, Add Prefixes (Current PR #1432 approach)tools_dict["component_abc123__query"] = tool_1
tools_dict["component_def456__query"] = tool_2Pros: Works, but tool names become verbose Option B: Change to List-Based Structure@dataclass
class TemplateRepresentation:
tools: list[tuple[str, AbstractMelleaTool]] | None = NonePros: Preserves all tools, natural names + parameters work Option C: Dict with Composite Keystools_dict[(component_id, "query")] = toolPros: No overwrites, clean structure Option D: Dict of Liststools_dict["query"] = [tool_1, tool_2, ...]Pros: Simple, preserves all tools Files That Need ChangesIf changing to list-based (Option B):
Current Parameter Approach Impact:
Recommended FixTo make parameter approach work with multiple tools: |
|
Makes sense; I was just trying to describe that there are multiple ways to do something like this. And this is a big enough change that we should test out multiple options and ensure the chosen approach works in many different scenarios. We don't quite have a process for that right now; but moving this to a discussion might help. We can also probably figure out what performance metrics we are looking for with a change like this. |
|
Discussion (#1455) is created. |
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
0e9469c to
672c94e
Compare
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
|
Mark this as ready for review. discussions are in #1455 |
ajbozarth
left a comment
There was a problem hiding this comment.
Some feedback from Claude (posted by Alex). Core mechanism looks correct, comments inline. One item that can't be line-anchored: the PR description still describes a tool_name_mapping field and component0.search naming, neither of which exists in the code (the fields added are component_id/component_type/component_description, and the prefix is component_{hexid}__name). Please update the description to match.
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
ajbozarth
left a comment
There was a problem hiding this comment.
This LGTM with one docstring nit. I'll leave @planetf1 and @jakelorocco to re-review for final merge since they previously pushed back on the original implementation.
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
|
The PR description still needs updating before approval. The current implementation uses |
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Pull Request
Issue
Fixes #95
Description
Auto-prefixing of component tools (
mellea/backends/tools.py):component_{COMPONENT_ID}__to prevent naming collisionsCOMPONENT_IDis derived from Python object identity:hex(id(component))[-8:]JSON schema updates (
mellea/backends/tools.py):convert_tools_to_json()to ensure function names in JSON schemas match the prefixed keysTemplateRepresentation enhancements (
mellea/core/base.py):tool_name_mappingfield to track original → prefixed name mappingsTest coverage:
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.