[bugfix] fix json_parse_to_dict #5996
Conversation
Summary of ChangesHello @Jintao-Huang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a bug in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug in json_parse_to_dict. Previously, when strict=False, an invalid JSON string would undergo a repair attempt, and if that still resulted in invalid JSON, a modified (repaired) string might be returned instead of the original. The new logic correctly attempts repair only when strict=True, ensuring the original string is returned when parsing fails in non-strict mode. I have one suggestion to improve the error logging for better debugging.
| f"the string before and after repair are '{old_value}' | '{value}'") | ||
| value = json.loads(value) | ||
| except Exception: | ||
| logger.error(f"Unable to parse json string: '{value}', and try to repair failed") |
There was a problem hiding this comment.
For better debugging, it's helpful to log the original malformed JSON string that caused the failure, instead of the potentially modified value. Using old_value here would make the log message more consistent and informative, as it would always point to the initial problematic input.
| logger.error(f"Unable to parse json string: '{value}', and try to repair failed") | |
| logger.error(f"Unable to parse json string: '{old_value}', and try to repair failed") |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several bug fixes across the codebase. The primary change in json_parse_to_dict correctly restricts JSON repair attempts to strict mode only, which clarifies its behavior. Other improvements include reordering argument initialization and saving in sft.py for correctness, and adjusting argument precedence during model conversion in convert.py. The documentation for rope_scaling has also been updated. Overall, these changes enhance the robustness and correctness of the code. I have one suggestion to improve the logging in json_parse_to_dict for better error diagnostics.
| f"the string before and after repair are '{old_value}' | '{value}'") | ||
| value = json.loads(value) | ||
| except Exception: | ||
| logger.error(f"Unable to parse json string: '{value}', and try to repair failed") |
There was a problem hiding this comment.
For better debugging, it would be more helpful to log the original malformed string (old_value) that failed to be repaired, instead of the repaired string (value). This will show exactly what input caused the repair to fail.
| logger.error(f"Unable to parse json string: '{value}', and try to repair failed") | |
| logger.error(f"Unable to parse json string: '{old_value}', and try to repair failed") |
No description provided.