-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix: catch pydantic ValidationError in VectorStoreQueryOutputParser #20450
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?
fix: catch pydantic ValidationError in VectorStoreQueryOutputParser #20450
Conversation
Wrap pydantic ValidationError as OutputParserException to ensure consistent exception handling in VectorIndexAutoRetriever. Fixes run-llama#19410 Signed-off-by: majiayu000 <[email protected]>
| except ValidationError as e: | ||
| raise OutputParserException( | ||
| f"Failed to validate query spec. Error: {e}. Got JSON dict: {json_dict}" | ||
| ) from e |
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.
Did you notice a better UX here by throwing the OutputParserException instead of the ValidationError that would be normally thrown without this extra step?
Signed-off-by: majiayu000 <[email protected]>
Signed-off-by: majiayu000 <[email protected]>
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.
Looks like these changes are unrelated
|
@AstraBert Thanks for the suggestion. The parser now wraps the ValidationError and raises OutputParserException with context (see llama-index-core/llama_index/core/indices/vector_store/retrievers/auto_retriever/output_parser.py). @logan-markewich The schema changes are part of the MediaResource hash fix; happy to split if you prefer. |
|
@majiayu000 I see you have another PR open for the MediaResource hashing changes, we can follow up there with those and I suggest you remove them from here |
|
Thanks @AstraBert! You are right. I have updated this PR and removed the This PR is now focused solely on catching the |
Description
Wrap pydantic
ValidationErrorasOutputParserExceptioninVectorStoreQueryOutputParser.parse()to ensure consistent exception handling inVectorIndexAutoRetriever._parse_generated_spec.Previously, when the LLM returned malformed JSON that passed JSON parsing but failed Pydantic validation, an uncaught
ValidationErrorwould be raised instead of the expectedOutputParserException.Fixes #19410
New Package?
Version Bump?
Type of Change
How Has This Been Tested?
Added two test cases:
test_output_parser_invalid_schema_raises_output_parser_exception: Tests that missing required fields raiseOutputParserExceptiontest_output_parser_invalid_type_raises_output_parser_exception: Tests that invalid types raiseOutputParserExceptionSuggested Checklist:
uv run make format; uv run make lintto appease the lint gods