Skip to content
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

Add support for json_mode unsupported models #1239

Merged
merged 3 commits into from
Feb 4, 2025
Merged

Conversation

whoisarpit
Copy link
Contributor

PR Checklist

  • The commit message follows our guidelines: Code of conduct
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Does this PR introduce a breaking change?
  • Include PR in release notes?

PR Type

  • Bugfix
  • Feature
  • Refactoring
  • Build /CI
  • Documentation
  • Others

What is the current behavior?

For models like gemini-2.0-flash-thinking-exp, there's a 400 error with no response.

What is the new behavior?

Adds support for models that don't support json mode, which is needed to support gemini-2.0-flash-thinking-exp

Other information

@whoisarpit whoisarpit requested a review from CTY-git February 4, 2025 05:09
@whoisarpit whoisarpit requested a review from CTY-git February 4, 2025 05:21
@patched-admin
Copy link
Contributor

The pull request review identifies several issues in the code. Firstly, there's a logical/functional bug in the __retry_unit method where the extraction of JSON from the text without validation could cause problems if the response lacks a JSON structure. It advises adding a validation check before proceeding with the extracted JSON. Secondly, there's a security vulnerability concern with the __extract_json_from_text method, as it risks injection attacks when handling untrusted data unless the extracted JSON is properly sanitized and validated. Thirdly, regarding coding standards, the review suggests reconsidering the accessibility of the __extract_json_from_text method, potentially changing it from private to protected or public for better extensibility or testing, and recommends adding a check in the run method to ensure the prompts list is populated before accessing its elements. Additionally, the pyproject.toml file changes only involve a version bump from 0.0.94 to 0.0.95, which is not associated with new bugs or security issues and adheres to coding standards if semantic versioning is practiced. Overall, the changes are deemed suitable provided additional context or related code modifications are not present.


  • File changed: patchwork/steps/SimplifiedLLM/SimplifiedLLM.py
    1. Logical/Functional Bug:
    • In the __retry_unit method, when checking for self.is_json_mode_unsupported, it modifies the response by extracting JSON from the text. This could be problematic if the model's response is not guaranteed to be a JSON or contain a JSON structure. An additional check could be added to validate that the extracted portion is indeed a valid JSON before proceeding.
  1. Security Vulnerability:

    • Using the __extract_json_from_text method, especially when working with untrusted data, could potentially expose the system to injection attacks if the extracted JSON is processed without proper validation. Ensure any extracted JSON is sanitized and validated before use.
  2. Coding Standards:

    • The SimplifiedLLM class has a static method __extract_json_from_text which is private. The double underscore suggests it's intended to be private to the class, but Python doesn't have true private methods. Consider whether this method should be protected (single underscore) or public for better future extensibility or testing purposes.
    • In the run method, the prompts[-1]["content"] is accessed directly and modified without checking if prompts list is populated. There should be a check to ensure that prompts contains at least one entry before attempting to access the last element.
  • File changed: pyproject.toml
    The diff in the pyproject.toml file only includes a version bump from 0.0.94 to 0.0.95.
  1. Potential Bugs: This diff doesn't introduce any potential bugs as it only alters the version number.
  2. Security Vulnerabilities: There are no security implications in this specific change, given that it's a version number update only.
  3. Adherence to Coding Standards: The code adheres to typical versioning standards, assuming the project follows semantic versioning. The change appears consistent with maintaining package version integrity.

Overall, the change seems appropriate barring any undisclosed context or associated code changes outside this diff.

@CTY-git CTY-git merged commit a4cfca0 into main Feb 4, 2025
6 of 9 checks passed
@CTY-git CTY-git deleted the feature/gemini-flash branch February 4, 2025 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants