-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(langchain): tool retry middleware #9234
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
base: main
Are you sure you want to change the base?
Conversation
Adds toolRetryMiddleware to automatically retry failed tool calls with configurable exponential backoff, exception filtering, and error handling. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
| /** | ||
| * Initial delay in milliseconds before first retry. Default is 1000 (1 second). | ||
| */ | ||
| initialDelay?: number; |
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.
Just flagging that this is milliseconds, where python uses seconds. It is more common in js to use milliseconds, but I don't love the inconsistency with the Python version. Any opinions? We could name it initialDelayMs to make unit clearer if we decide to stick with milliseconds
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.
No super strong opinions, but would agree we should use ms.
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.
Added Ms suffix to make it clearer
1da7a0c to
17a144b
Compare
| /** | ||
| * Initial delay in milliseconds before first retry. Default is 1000 (1 second). | ||
| */ | ||
| initialDelay?: number; |
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.
No super strong opinions, but would agree we should use ms.
| const toolFilter: string[] | undefined = tools?.map((tool) => | ||
| typeof tool === "string" ? tool : (tool.name as string) | ||
| ); |
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.
might pay to be more strict here:
| const toolFilter: string[] | undefined = tools?.map((tool) => | |
| typeof tool === "string" ? tool : (tool.name as string) | |
| ); | |
| const toolFilter = tools?.reduce((acc, tool) => { | |
| if (typeof tool === "string") return [...acc, tool]; | |
| else if ("name" in tool && typeof tool.name === "string") return [...acc, tool]; | |
| else return acc; | |
| }, [] as string[]); |
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.
So we'd just silent drop tools that don't look right? Would it not be better to fail fast?
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.
Idk if it super matters. If that's something we do want to guard against, I'd say we need to throw a better error instead of just whatever comes out of accessing tool.name on a weird looking object
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.
Throwing a better error sounds good to me. Any objection?
|
Ok feedback addressed and left some responses |
Adds
toolRetryMiddlewareto automatically retry failed tool calls with configurable exponential backoff, exception filtering, and error handling.Port of langchain-ai/langchain#33503
Example
For advanced usage with specific exception handling: