-
-
Notifications
You must be signed in to change notification settings - Fork 115
[Agent] Allow exposing tool exceptions to AI #478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4fdb1ea to
205131d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be part of the fixtures in agent component
| use Symfony\AI\Agent\Toolbox\ToolFactory\ChainFactory; | ||
| use Symfony\AI\Agent\Toolbox\ToolFactory\MemoryToolFactory; | ||
| use Symfony\AI\Agent\Toolbox\ToolFactory\ReflectionToolFactory; | ||
| use Symfony\AI\Fixtures\Tool\ToolCustomException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, same for the other fixtures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean all fixtures in Symfony\AI\Fixtures\Tool namespace? I think they were only recently moved to root fixtures 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check where they are used? If they are only used in agent then we should move them.
cc @chr-hertel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
70% of Symfony\AI\Fixtures\Tool usage is in Agent component and the rest 30% is in Platform
I'd probably keep them as they are now instead of having them in several locations. Or remove all these from the root (and move to the packages), and just duplicate the ones that are needed in several components. I think it is better DX-wise if a developer can add the new fixture where there already are existing ones, instead of needing to figure out which directory you should choose (in case we had both, root + in the package).
205131d to
a06e551
Compare
This comment was marked as outdated.
This comment was marked as outdated.
| } | ||
| } | ||
|
|
||
| #[AsTool('get_user_age', 'Get age by user id')] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if possible, but wouldn't it be better if you would be able to provide this info via callable in the attribute to not couple you code with the 3rd party exception?
| #[AsTool('get_user_age', 'Get age by user id')] | |
| #[AsTool('get_user_age', 'Get age by user id', exceptions: [EntityNotFoundException::class => static fn(THE TOOL CALL ARGUMENTS HERE)....])] |
Or is this not possible? Or is it a bad idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting idea,
but my reasoning for this feature is actually about throwing from an event listener of ToolCallArgumentsResolved – therefore I don't see this as a good place for it as you'd have to put it into every tool definition then.
Maybe this is just a bad example in the docs?
Another possibility to implement what I need could be an ability to define a custom error handler service for particular toolbox (or maybe even globally for all agents). How that could be implemented, f.e. instead of current fault_tolerant_toolbox: false/true you could set fault_tolerant_toolbox: 'app.my_abstract_fault_tolerant_toolbox' which would then use your service definition instead of builtin FaultTolerantToolbox.
I find the exception interface more convenient, however, it does have the downside of not being able to use/inject services to produce the result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with the interface, the error handler seems to complicated for such a "normal" use case, WDYT @chr-hertel ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that interface as well - valid use-case too me - and opt-in 👍
|
Thank you @valtzu. |
Make it possible to throw custom exceptions that expose the error to the LLM when using
FaultTolerantToolbox.Example use case: an event listener for
ToolCallArgumentsResolvedthat validates the tool call arguments and throws if any violations found. Then the LLM can auto-fix the input.