Expanding llm_parse_json and removing extract_score assumptions#1082
Expanding llm_parse_json and removing extract_score assumptions#1082jamesbraza merged 4 commits intomainfrom
llm_parse_json and removing extract_score assumptions#1082Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves the robustness of JSON extraction and removes hardcoded fallback scores from the extract_score function. The changes focus on making JSON parsing more fault-tolerant and ensuring proper error handling when score extraction fails.
Key changes:
- Enhanced
llm_parse_jsonfunction with better comma handling and error reporting - Modified
extract_scoreto raise exceptions instead of returning fallback scores - Added comprehensive test coverage for missing comma scenarios in JSON parsing
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/paperqa/core.py | Enhanced JSON parsing with missing comma detection and improved error handling |
| src/paperqa/utils.py | Removed fallback score assumptions, now raises ValueError on extraction failure |
| tests/test_paperqa.py | Updated tests to expect exceptions and added new test case for missing commas |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| # Handling float, str values for relevance_score | ||
| if "relevance_score" in data: | ||
| if "relevance_score" in data and not isinstance(data, int): |
There was a problem hiding this comment.
The condition not isinstance(data, int) is checking the wrong variable. It should check data['relevance_score'] instead of data itself, since data is a dictionary.
| if "relevance_score" in data and not isinstance(data, int): | |
| if "relevance_score" in data and not isinstance(data["relevance_score"], int): |
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, please have a team admin upgrade your team to Bugbot Pro by visiting the Cursor dashboard. Your first 14 days will be free!
Comment @cursor review or bugbot run to trigger another review on this PR
whitead
left a comment
There was a problem hiding this comment.
Nice work - unclear if test failures are real though.
Also - this code will now blow-up on parsing failures? This is a big change right?
Also - can you review the prompts and make sure that an empty response or "not relevant" is not specified as the correct behavior for irrelevant contexts? I seem to remember that some of this code assumed an empty response or something like it was preferred over JSON for irrelevant contexts.
8d35d46 to
943cadc
Compare
99d318a to
6f18abe
Compare
They were real, there was an issue that Cursorbot caught, and I've since fixed that. Now tests pass.
Thanks to #1083, we now can retry getting the context if there's a JSON parse or a score extraction failure. And if the LLM fails twice, we safely abandon the context. So now, if the LLM fails to provide a valid score twice, the context will be abandoned.
I think this is what you're getting at, so we should be good here. |
This PR makes our JSON extraction more robust and with less assumptions
llm_parse_jsonwas commonly failing on missing commas in the JSONllm_parse_jsonhad extra logicextract_scorewould inject two fake scores