enhancement: support multiple Tool string outputs #10279
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Pull Request Test Coverage Report for Build 20440323754Details
💛 - Coveralls |
|
Thanks @tstadel , taking a look today! |
vblagoje
left a comment
vblagoje
left a comment
There was a problem hiding this comment.
@tstadel If you could please add a bit of pydocs for this use case in tool.py that would be amazing icing on the cake for this enhancement. In reno note just another sentence or two with example (before - how it was not possible, and now how it is, where is the setting etc etc) will be super helpful to users and and as a history log for us as well.
haystack/tools/tool.py
Outdated
| The function that will be invoked when the Tool is called. | ||
| Must be a synchronous function; async functions are not supported. | ||
| :param outputs_to_string: | ||
| Optional dictionary defining how a tool outputs should be converted into a string. |
There was a problem hiding this comment.
Can we add example here for this use case @tstadel ?
|
@vblagoje if you want to have a final look over the added docs, I've updated them. |
|
@tstadel it looks good to me. However, as this is a crucial component, unless it is urgent I'd ask @anakin87 and perhaps @sjrl to have one last look. We need to think about the impact this change will have on agent - state - tool interactions. Have you played with this while prepping this PR? All good? |
Sounds good. Additionally to the tests in this PR (which of course ensure backward compatibility as this is just an extension) we'll infer the required state schema on platform side. For open source you need to set the state as before. |
| :param outputs_to_string: | ||
| Optional dictionary defining how a tool outputs should be converted into a string. | ||
| If the source is provided only the specified output key is sent to the handler. | ||
| If the source is omitted the whole tool result is sent to the handler. | ||
| Example: | ||
| ```python | ||
| { | ||
| "source": "docs", "handler": format_documents | ||
| } | ||
| ``` | ||
| Optional dictionary defining how tool outputs should be converted into string(s). | ||
| Supports two formats: |
There was a problem hiding this comment.
Please update the docstrings in all our tools, so also ComponentTool and PipelineTool
| function=search_func, # Returns {"documents": [Document(...)], "metadata": {"count": 5}, "debug_info": {...}} | ||
| outputs_to_string={ | ||
| "formatted_docs": {"source": "documents", "handler": format_documents}, |
There was a problem hiding this comment.
I think it would be better if we provided code examples for these two functions, even if they are dummy functions. Two functions in question are format_documents and search_func
| function=search_func, # Returns {"documents": [Document(...)], "metadata": {"count": 5}, "debug_info": {...}} | ||
| outputs_to_string={ | ||
| "formatted_docs": {"source": "documents", "handler": format_documents}, | ||
| "summary": {"source": "metadata", "handler": lambda m: f"Found {m['count']} results"} |
There was a problem hiding this comment.
I'm not sold on providing an example that uses a lambda function since this isn't encouraged by us since it will fail at serialization. So calling tool.to_dict() with this example will fail.
Could we also create a dummy handler function here as well?
| else: | ||
| for key, config in self.outputs_to_string.items(): |
There was a problem hiding this comment.
Could we add a dev comment here?
| else: | |
| for key, config in self.outputs_to_string.items(): | |
| else: | |
| # Multiple outputs configuration | |
| for key, config in self.outputs_to_string.items(): |
| tool_result_str = self._default_output_to_string_handler(tool_result) | ||
| return ChatMessage.from_tool(tool_result=tool_result_str, origin=tool_call) | ||
| except Exception as e: | ||
| error = StringConversionError(tool_call.tool_name, output_to_string_handler.__name__, e) |
There was a problem hiding this comment.
The new structure of the code could lead to a state where the output_to_string_handler is not defined when this exception is triggered.
I'm wondering if we should restructure this method a little bit. The point of raising StringConversionError was to catch errors specifically coming from calling tool_result_str = output_to_string_handler(value) and I think that is a little muddied with your version.
I wonder if a two-tiered try-except blocks would be better. So one specifically around calls to output_to_string_handler(value) and then another around the whole code block if you think that's still necessary. What do you think?
| # If no source key is provided, we use the output key itself | ||
| source_key = config.get("source", output_key) |
There was a problem hiding this comment.
This is kind of a sneaky feature that isn't mentioned anywhere in the docs. Is this really necessary to add or could we follow how the logic works for the single output format.
| for output_key, config in outputs_config.items(): | ||
| # If no source key is provided, we use the output key itself | ||
| source_key = config.get("source", output_key) | ||
| value = result[source_key] |
There was a problem hiding this comment.
This could cause a KeyError since there is no guarantee that the source_key actually exists in the result dict. That's why we used result.get(source_key) above. Was this intentional?
Related Issues
Currenlty it's not possible to make multiple selections for LLM (e.g. as in the sketch above) which increases UI complexity heavily.
Proposed Changes:
outputs_to_stringsimilar tooutputs_to_stateHow did you test it?
Notes for the reviewer
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.