-
-
Notifications
You must be signed in to change notification settings - Fork 752
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
Jinja Templating and Documentation #1394
base: add-supp-genai-sdk
Are you sure you want to change the base?
Conversation
9a874bf
to
eb902e9
Compare
Deploying instructor-py with
|
Latest commit: |
1ca2d23
|
Status: | ✅ Deploy successful! |
Preview URL: | https://6ff198e2.instructor-py.pages.dev |
Branch Preview URL: | https://add-genai-jinja-support.instructor-py.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 1ca2d23 in 3 minutes and 5 seconds
More details
- Looked at
651
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
10
drafted comments based on config settings.
1. instructor/patch.py:159
- Draft comment:
BUG: In the async variant, the call to handle_templating is missing the required 'mode' parameter. It should be updated to pass mode=mode (e.g., handle_templating(new_kwargs, mode=mode, context=context)). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. tests/llm/test_genai/test_format.py:57
- Draft comment:
Duplicate Test: The function test_system_kwarg is defined twice. Consider renaming or merging to avoid duplicate definitions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. instructor/templating.py:22
- Draft comment:
Potential Type Issue: process_message accesses message.role and message.parts assuming an object with attributes. If messages are provided as dicts (as in some tests), this could lead to AttributeErrors. Consider normalizing message input (e.g., using message.get('role')) or adding type checks. - Reason this comment was not posted:
Confidence changes required:70%
<= threshold85%
None
4. docs/integrations/genai.md:17
- Draft comment:
Typo: "recomendation" should be "recommendation". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 80% vs. threshold = 85%
While spelling errors should be fixed, the review rules don't explicitly mention spelling/grammar. However, since this is a new documentation file being added, having correct spelling is important for readability and professionalism. The error is clear and the fix is straightforward.
Documentation quality might not be as critical as code quality. Also, this could be caught by a spell checker rather than needing manual review.
This is user-facing documentation that represents the project, so professional quality is important. The fix is simple and clearly correct.
Keep the comment. The spelling error should be fixed in this new documentation file to maintain professional quality.
5. docs/integrations/index.md:22
- Draft comment:
Ensure the new link for Google.GenAI is consistently labeled; verify it matches naming in other docs. - Reason this comment was not posted:
Confidence changes required:80%
<= threshold85%
None
6. instructor/patch.py:159
- Draft comment:
Inconsistency: The sync variant passes mode to handle_templating, but the async variant does not. Consider updating the async branch for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. instructor/templating.py:21
- Draft comment:
Potential type issue: In the GENAI_TOOLS branch, process_message accesses message.role and message.parts. If messages are plain dicts, these attribute accesses will fail. Consider using dictionary access (e.g., message['role']) or adding a type check. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 85%
The code was just changed to add the GENAI_TOOLS branch. The comment correctly identifies a potential type mismatch between the function signature and implementation. However, looking at the rest of the file, I see that attribute access is used consistently throughout other branches (VertexAI support also uses message.role and message.parts). The file starts with # type: ignore[all] suggesting type checking is intentionally disabled. This appears to be an intentional pattern in the codebase.
The comment raises a valid technical point about type safety. The code could be more type-safe by using dict access or proper typing.
However, this appears to be an intentional pattern throughout the codebase, with type checking explicitly disabled. The new code follows the existing conventions.
The comment should be deleted as it suggests changing an intentional pattern in the codebase. The new code follows existing conventions.
8. tests/llm/test_genai/test_format.py:57
- Draft comment:
Duplicate test name: Two functions are named 'test_system_kwarg'. Rename one to avoid override and ensure distinct test coverage. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. docs/integrations/genai.md:17
- Draft comment:
Typo found: 'recomendation' should be corrected to 'recommendation'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 80% vs. threshold = 85%
While spelling errors should be fixed, the review rules don't explicitly mention spelling/typos. However, since this is documentation that will be user-facing, accuracy is important. The typo is clear and the fix is straightforward. The file is new, so this is definitely about a change.
The rules don't explicitly mention handling spelling/typo corrections. Maybe this is too minor of an issue to flag?
Since this is user-facing documentation, even small errors like typos should be fixed to maintain professionalism and clarity. The fix is clear and actionable.
Keep the comment. Documentation quality is important, and this is a clear, actionable fix for a real error in a new file.
10. docs/integrations/index.md:19
- Draft comment:
The Google integration labels are inconsistent. One is labeled 'Google.GenerativeAI' (line 19) and another 'Google.GenAI' (line 22). Please verify if they should be the same for consistency or if they refer to different things. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 85%
The comment assumes these should be consistent, but they actually point to different documentation files and appear to be different integrations. The naming difference seems intentional to distinguish between two separate Google services. Looking at line 76, we can see a reference to "Gemini" which is likely related to one of these integrations.
I might be wrong about these being different services - without seeing the actual documentation files, I can't be 100% certain they're not duplicates.
However, the fact that they point to different documentation files (google.md vs genai.md) strongly suggests these are intentionally different integrations.
The comment should be deleted because it appears to be questioning something that is intentionally different, and the naming distinction seems purposeful to differentiate two separate Google services.
Workflow ID: wflow_6lfHe0D3CQhw2OyM
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -73,6 +75,31 @@ def test_system_kwarg(client, model, mode): | |||
assert response.users[0].age == 28 | |||
|
|||
|
|||
@pytest.mark.parametrize("model", models) | |||
@pytest.mark.parametrize("mode", modes) | |||
def test_system_kwarg(client, model, mode): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test duplicates the existing test_system_kwarg
function. The only difference is the message format (genai.types.Content
vs dict
), which doesn't justify a separate test case since it tests the same functionality.
- function
test_system_kwarg
(test_format.py)
Add documentation and Jinja templating support for GenAI SDK in instructor
Important
Adds Jinja templating support and documentation for GenAI SDK in Instructor, including updates to templating processes and new tests.
genai.md
with a comprehensive guide on using Instructor with Google's Generative AI (Gemini) for structured outputs.index.md
to include links to the new GenAI documentation.handle_templating()
intemplating.py
to supportMode.GENAI_TOOLS
.process_message()
to handlegenai.types.Content
forMode.GENAI_TOOLS
.new_create_sync()
inpatch.py
to passmode
tohandle_templating()
.test_format.py
for different message formats and templating scenarios.test_formatting.py
to test templating with various contexts and formats.This description was created by
for 1ca2d23. It will automatically update as commits are pushed.