Merged
Conversation
Co-authored-by: jason <jason@jxnl.co>
Co-authored-by: jason <jason@jxnl.co>
|
Cursor Agent can help with this pull request. Just |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
instructor | eb5f85a | Commit Preview URL Branch Preview URL |
Jan 16 2026, 05:28 PM |
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to eb5f85a in 1 minute and 14 seconds. Click for details.
- Reviewed
221lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5draft 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:332
- Draft comment:
Good change: the extraction of thinking_config now supports both dicts and objects. Consider abstracting this extraction (and similar extraction for labels and cached_content) into a helper to reduce duplication. - Reason this comment was not posted:
Confidence changes required:33%<= threshold85%None
2. instructor/providers/gemini/utils.py:355
- Draft comment:
The merging logic for 'automatic_function_calling', 'labels', and 'cached_content' now properly handles both dict and object styles. Consider DRYing up the repeated extraction by using a helper function. - Reason this comment was not posted:
Confidence changes required:33%<= threshold85%None
3. instructor/providers/gemini/utils.py:890
- Draft comment:
In handle_genai_structured_outputs, extraction of thinking_config and cached_content now handles dict inputs correctly. The updated comments are clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
4. instructor/providers/gemini/utils.py:970
- Draft comment:
Similarly, handle_genai_tools now correctly extracts config fields from dict or object. Consider refactoring this common logic into a shared helper to keep the code DRY. - Reason this comment was not posted:
Confidence changes required:33%<= threshold85%None
5. tests/test_genai_config_merging.py:402
- Draft comment:
Excellent and comprehensive tests covering both object and dict configs, including labels, cached_content, and thinking_config. They clearly validate the merged configuration behavior (issue #1759). - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
Workflow ID: wflow_SGnig1h19DTyKTZY
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.
fix(genai): preserve config labels from dict config
Describe your changes
This PR resolves an issue where GCP billing/monitoring
labelsandcached_contentwere discarded or incorrectly handled when theconfigparameter was provided as a dictionary in Google GenAI provider functions. This was blocking enterprise usage due to tracking issues.The changes update
update_genai_kwargs,handle_genai_structured_outputs, andhandle_genai_toolsto correctly mergelabels,cached_content,automatic_function_calling, andthinking_configfrom both dictionary-style and object-styleconfigparameters. This ensures all relevant configuration fields are preserved and applied as intended, andcached_contentcorrectly suppressessystem_instructionand tool definitions when active.New regression tests have been added to verify label preservation and cached content behavior for dictionary-style configs.
Issue ticket number and link
Linear: 567-269
GitHub: #1759
Checklist before requesting a review
Linear Issue: 567-269
Important
Fixes GenAI config handling to preserve
labelsandcached_contentwhen provided as a dictionary, ensuring correct merging of configuration fields.labelsandcached_contentwere discarded whenconfigwas a dictionary inupdate_genai_kwargs,handle_genai_structured_outputs, andhandle_genai_tools.labels,cached_content,automatic_function_calling, andthinking_configare merged from both dictionary and object-styleconfig.cached_contentsuppressessystem_instructionand tool definitions when active.test_genai_config_merging.pyto verify label preservation and cached content behavior for dictionary-style configs.thinking_config,automatic_function_calling, andlabelsare correctly extracted and do not override existing base config values.This description was created by
for eb5f85a. You can customize this summary. It will automatically update as commits are pushed.