fix(genai): extract thinking_config from user-provided config object#1972
Merged
jxnl merged 1 commit into567-labs:mainfrom Dec 30, 2025
Merged
Conversation
When users pass thinking_config inside a GenerateContentConfig object (e.g., config=GenerateContentConfig(thinking_config=...)), the thinking_config was being ignored. This fix extracts thinking_config from the user-provided config object and includes it in the final configuration. The fix: - Checks for config parameter in handle_genai_structured_outputs and handle_genai_tools functions - Extracts thinking_config from user's config if present - Prioritizes separate kwarg thinking_config over config.thinking_config Closes 567-labs#1966
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 09f42bc in 1 minute and 40 seconds. Click for details.
- Reviewed
142lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. instructor/providers/gemini/utils.py:837
- Draft comment:
Good fix for extracting thinking_config. Note that the extraction block appears nearly identical in both handle_genai_structured_outputs and handle_genai_tools. Consider refactoring this into a helper to adhere to DRY principles. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 85% vs. threshold = 85% This is a code quality refactoring suggestion that is actionable and clear. The comment correctly identifies duplicated code that was added in this diff (both blocks are new additions). The suggestion to extract this into a helper function is reasonable and follows the DRY principle, which is explicitly mentioned in the additional rules. The comment is about changes made in the diff (both blocks are new code), so it's relevant. This is exactly the type of comment that should be kept according to the rules: "Comments that suggest code quality refactors are good! But only if they are actionable and clear." Could this be considered too minor or obvious? The duplication is only 11 lines and appears in just 2 places. Perhaps the author intentionally duplicated it for simplicity or readability. Also, I should verify that both blocks are truly identical and that extracting them would actually improve the code. While the duplication is relatively small, it's still meaningful - 11 lines duplicated across 2 functions. The blocks are completely identical, including comments, which strongly suggests they should be extracted. The DRY principle is explicitly listed in the additional rules, and this is a clear, actionable suggestion. Even if the author intentionally duplicated it, the comment provides value by pointing out the opportunity for improvement. This comment should be kept. It identifies clear code duplication that violates the DRY principle (explicitly listed in the rules), provides an actionable suggestion (extract to helper function), and is about code that was changed in this diff. This is exactly the type of code quality refactoring comment that the rules say should be kept.
2. tests/llm/test_genai/test_utils.py:206
- Draft comment:
Comprehensive tests for thinking_config extraction and kwarg priority have been added. This effectively verifies the fix for issue #1966. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
Workflow ID: wflow_ltfY2TVWigLCTimn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
thinking_configinside aGenerateContentConfigobject, it was being ignoredthinking_configfrom the user-provided config objectthinking_configoverconfig.thinking_configChanges
handle_genai_structured_outputs()to extract thinking_config from user confighandle_genai_tools()with the same fixTest plan
Closes #1966
Important
Fixes issue #1966 by extracting
thinking_configfrom user config inhandle_genai_structured_outputs()andhandle_genai_tools(), prioritizing kwarg over config.thinking_configinsideconfigparameter is ignored in GENAI_STRUCTURED_OUTPUTS mode #1966 by extractingthinking_configfromconfiginhandle_genai_structured_outputs()andhandle_genai_tools().thinking_configkwarg overconfig.thinking_config.test_handle_genai_structured_outputs_thinking_config_in_config()andtest_handle_genai_tools_thinking_config_in_config()to verify extraction.test_handle_genai_structured_outputs_thinking_config_kwarg_priority()to verify kwarg priority.This description was created by
for 09f42bc. You can customize this summary. It will automatically update as commits are pushed.