-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
fix(langchain): Fix structured parser error handling #7232
base: main
Are you sure you want to change the base?
fix(langchain): Fix structured parser error handling #7232
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
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.
thanks for doing this! I have two small comments, but other than that, lgtm!
Please re-request my review once you've implemented them
text | ||
); | ||
// eslint-disable-next-line no-instanceof/no-instanceof | ||
if (e instanceof SyntaxError && e.message.includes("JSON")) { |
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't use instanceof
as it's not available in all JS environments.
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.
It's not that it doesn't exist, it's just that it's not always reliable for packages since in some environments bundlers can duplicate class definitions across multiple chunks
(native classes I think are ok)
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.
Do you think it would be better to change this condition to something like if (e && (e as Error).name !== "ZodError")
?
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.
Unfortunately minifiers can also mess with that 😬
It looks like SyntaxError
is a native object though?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SyntaxError
So probably ok to do the instanceof check
return await this.schema.parseAsync(JSON.parse(text.trim())); | ||
} catch (e2) { | ||
throw new OutputParserException( | ||
`Failed to parse. Text: "${text}". Error: ${e2}`, |
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.
If e2
is an object, it will not be rendered properly in this message. Please add a check beforehand which stringifies the error if it's an object
This was an issue before, but we should fix now.
Previously, StructuredOutputParser would sometimes throw JSON.parse errors instead of the original Zod schema validation errors.
This PR modifies the fallback mechanism to only trigger for JSON parsing failures, while preserving Zod's schema validation errors.
Relevant links: