fix(genai): add automatic Partial wrapping for streaming requests by jxnl · Pull Request #1695 · 567-labs/instructor

jxnl · 2025-07-11T19:48:32Z

This PR fixes the Gemini streaming issue reported in #.

Summary

Automatically wraps regular models with Partial when stream=True is used
Prevents AssertionError in function_calls.py when streaming Gemini responses
Adds comprehensive tests for sync and async streaming with regular models
Maintains backward compatibility with existing Partial usage

Test plan

Run existing GenAI tests to ensure no regressions
Run new streaming tests with regular models
Verify the user's original example now works
Test both sync and async streaming scenarios

Generated with Claude Code

Important

Fixes Gemini streaming issue by auto-wrapping models with Partial for streaming, updates docs and tests.

Behavior:
- Automatically wraps models with Partial in handle_genai_structured_outputs() and handle_genai_tools() when stream=True.
- Prevents AssertionError in function_calls.py for streaming Gemini responses.
- Maintains backward compatibility with existing Partial usage.
Documentation:
- Updates genai.md to reflect changes in streaming support and provider version.
- Adds warnings about streaming limitations with regular models.
Tests:
- Adds tests for sync and async streaming with regular models in test_stream.py.
- Removes unused imports in test_response_model_none.py.
Misc:
- Updates pyproject.toml and uv.lock to include pre-commit dependency.

^{This description was created by}^{for c9b1a74. You can customize this summary. It will automatically update as commits are pushed.}

- Automatically wrap regular models with Partial when stream=True is used - Prevents AssertionError in function_calls.py when streaming Gemini responses - Add comprehensive tests for sync and async streaming with regular models - Maintains backward compatibility with existing Partial usage Fixes #1691 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

cloudflare-workers-and-pages · 2025-07-11T19:48:40Z

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	instructor	`634428a`	Commit Preview URL Branch Preview URL	Jul 11 2025, 10:11 PM

claude · 2025-07-11T19:48:46Z

Claude finished @jxnl's task —— View job

PR Review Complete ✅

I've thoroughly reviewed this PR that fixes the Gemini streaming issue by automatically wrapping regular models with Partial when streaming is enabled. Here's my comprehensive feedback:

✅ Code Quality & Implementation

Excellent implementation approach:

Clean, targeted fix that addresses the root cause
Consistent implementation across both handle_genai_structured_outputs and handle_genai_tools functions
Simple conditional logic: if new_kwargs.get("stream", False) and not issubclass(response_model, PartialBase):
Maintains backward compatibility - existing Partial usage continues to work

Code placement:

✅ Lines 629-631 in handle_genai_structured_outputs
✅ Lines 667-669 in handle_genai_tools
Both placements are logical and consistent with the function flow

🔍 Potential Issues & Edge Cases

1. Type Safety Consideration:
The issubclass() check could potentially fail if response_model is not a class. Consider adding a safety check:

if (new_kwargs.get("stream", False) and 
    inspect.isclass(response_model) and 
    not issubclass(response_model, PartialBase)):
    response_model = Partial[response_model]

2. Error Handling:
The current implementation assumes Partial[response_model] will always succeed. This should be fine for Pydantic models, but could be documented.

3. Duplication:
The exact same code appears in two functions. Consider extracting to a helper function:

def auto_wrap_with_partial_if_streaming(response_model, kwargs):
    if kwargs.get("stream", False) and not issubclass(response_model, PartialBase):
        return Partial[response_model]
    return response_model

⚡ Performance Considerations

✅ Minimal performance overhead - single boolean check and type check
✅ No unnecessary object creation when not streaming
✅ issubclass() call is efficient for this use case

🔒 Security Assessment

✅ No security concerns identified
✅ No user input validation issues
✅ No injection or manipulation risks

🧪 Test Coverage Review

Excellent test additions:

✅ Both sync and async variants covered (test_regular_model_with_streaming_sync and test_regular_model_with_streaming_async)
✅ Tests specifically verify the automatic wrapping behavior
✅ Parameterized tests across all models and modes
✅ Comprehensive assertions checking both partial updates and final results
✅ Tests use regular models (not explicitly Partial[UserExtract]) to verify the fix

Test quality:

Clear test names describing the behavior being tested
Good assertions verifying both iteration count and final model state
Proper async/await usage in async tests

📋 Minor Suggestions

Documentation: Consider adding a brief comment explaining the automatic wrapping:

# Automatically wrap regular models with Partial when streaming is enabled
# This prevents AssertionError in function_calls.py during streaming
if new_kwargs.get("stream", False) and not issubclass(response_model, PartialBase):
    response_model = Partial[response_model]

Consistency: The fix is only applied to GenAI modes. Ensure this is intentional and not needed for other providers.

🎯 Overall Assessment

This is a high-quality fix that:

✅ Solves the stated problem elegantly
✅ Maintains backward compatibility
✅ Includes comprehensive tests
✅ Follows existing code patterns
✅ Has minimal performance impact

The implementation correctly identifies that streaming requires Partial models and automatically applies this requirement, which greatly improves the developer experience by removing the need for users to manually wrap models.

Recommendation: ✅ Approve with the minor suggestions above for future consideration.

ellipsis-dev

Caution

Changes requested ❌

Reviewed everything up to aafb781 in 1 minute and 19 seconds. Click for details.

Reviewed 94 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 2 draft 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/process_response.py:629

Draft comment:
Ensure response_model is a class before using issubclass to avoid potential runtime errors if a non-class type is passed.
Reason this comment was not posted:
Confidence changes required: 80% <= threshold 85% None

2. tests/llm/test_genai/test_stream.py:106

Draft comment:
New tests for regular model streaming (with implicit Partial wrapping) look comprehensive and validate both sync and async scenarios.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 85% None

Workflow ID: wflow_XdADUzaMWCKLAxSC

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev · 2025-07-11T19:49:56Z

instructor/process_response.py

 ) -> tuple[type[T], dict[str, Any]]:
    from google.genai import types

+    # Automatically wrap regular models with Partial when streaming is enabled


Duplicate partial wrapping logic in both handle_genai_structured_outputs and handle_genai_tools. Consider extracting a helper to DRY this pattern.

- Changed provider format from 'genai/gemini-2.5-flash' to 'google/gemini-2.5-flash' - Added comprehensive streaming examples with create_partial and create_iterable - Updated template variables examples to match test structure - Added async streaming examples for both partial and iterable methods

- Removed outdated mkdocs and mkdocs-material dependencies from the main dependencies list. - Added pre-commit back to the development dependencies. - Updated GenAI client initialization to use "genai/gemini-2.5-flash" across multiple documentation examples. - Clarified streaming limitations in the documentation regarding GenAI models. This update ensures the integration is aligned with the latest model version and improves the clarity of the documentation.

claude · 2025-07-11T20:30:13Z

Claude finished @jxnl's task —— View job

PR Review Complete ✅

I've thoroughly reviewed this PR that fixes the Gemini streaming issue by automatically wrapping regular models with Partial when streaming is enabled. Here's my comprehensive feedback:

✅ Code Quality & Implementation

Excellent implementation approach:

Clean, targeted fix that addresses the root cause
Consistent implementation across both handle_genai_structured_outputs() and handle_genai_tools() functions
Simple conditional logic: if new_kwargs.get("stream", False) and not issubclass(response_model, PartialBase):
Maintains backward compatibility - existing Partial usage continues to work

Code placement:

✅ Lines 629-631 in handle_genai_structured_outputs
✅ Lines 667-669 in handle_genai_tools
Both placements are logical and consistent with the function flow

🔍 Potential Issues & Edge Cases

1. Type Safety Consideration:
The issubclass() check could potentially fail if response_model is not a class. Consider adding a safety check:

if (new_kwargs.get("stream", False) and 
    inspect.isclass(response_model) and 
    not issubclass(response_model, PartialBase)):
    response_model = Partial[response_model]

2. Error Handling:
The current implementation assumes Partial[response_model] will always succeed. This should be fine for Pydantic models, but could be documented.

3. Duplication:
The exact same code appears in two functions. Consider extracting to a helper function:

def auto_wrap_with_partial_if_streaming(response_model, kwargs):
    if kwargs.get("stream", False) and not issubclass(response_model, PartialBase):
        return Partial[response_model]
    return response_model

⚡ Performance Considerations

✅ Minimal performance overhead - single boolean check and type check
✅ No unnecessary object creation when not streaming
✅ issubclass() call is efficient for this use case

🔒 Security Assessment

✅ No security concerns identified
✅ No user input validation issues
✅ No injection or manipulation risks

🧪 Test Coverage Review

Excellent test additions:

✅ Both sync and async variants covered (test_regular_model_with_streaming_sync and test_regular_model_with_streaming_async)
✅ Tests specifically verify the automatic wrapping behavior
✅ Parameterized tests across all models and modes
✅ Comprehensive assertions checking both partial updates and final results
✅ Tests use regular models (not explicitly Partial[UserExtract]) to verify the fix

Test quality:

Clear test names describing the behavior being tested
Good assertions verifying both iteration count and final model state
Proper async/await usage in async tests

📋 Minor Suggestions

Documentation: Consider adding a brief comment explaining the automatic wrapping:

# Automatically wrap regular models with Partial when streaming is enabled
# This prevents AssertionError in function_calls.py during streaming
if new_kwargs.get("stream", False) and not issubclass(response_model, PartialBase):
    response_model = Partial[response_model]

Consistency: The fix is only applied to GenAI modes. Ensure this is intentional and not needed for other providers.

🎯 Overall Assessment

This is a high-quality fix that:

✅ Solves the stated problem elegantly
✅ Maintains backward compatibility
✅ Includes comprehensive tests
✅ Follows existing code patterns
✅ Has minimal performance impact

The implementation correctly identifies that streaming requires Partial models and automatically applies this requirement, which greatly improves the developer experience by removing the need for users to manually wrap models.

Recommendation: ✅ Approve with the minor suggestions above for future consideration.

claude · 2025-07-11T20:30:30Z

Claude finished @jxnl's task —— View job

PR Review Complete ✅

I've thoroughly reviewed this PR that fixes the Gemini streaming issue by automatically wrapping regular models with Partial when streaming is enabled. Here's my comprehensive feedback:

Todo List:

Read and analyze changed files
Review core implementation in process_response.py
Evaluate test coverage and quality
Check documentation updates
Assess code quality and best practices
Identify potential bugs or issues
Review performance considerations
Check security concerns
Provide comprehensive feedback

✅ Code Quality & Implementation

Excellent implementation approach:

Clean, targeted fix that addresses the root cause
Consistent implementation across both handle_genai_structured_outputs and handle_genai_tools functions
Simple conditional logic: if new_kwargs.get("stream", False) and not issubclass(response_model, PartialBase):
Maintains backward compatibility - existing Partial usage continues to work

Code placement:

✅ Lines 629-631 in handle_genai_structured_outputs
✅ Lines 667-669 in handle_genai_tools
Both placements are logical and consistent with the function flow

🔍 Potential Issues & Edge Cases

1. Type Safety Consideration:
The issubclass() check could potentially fail if response_model is not a class. I noticed the codebase uses this pattern elsewhere (lines 410, 457):

if (
    inspect.isclass(response_model)
    and issubclass(response_model, (IterableBase, PartialBase))
    and stream
):

Consider adding the safety check to match existing patterns:

if (new_kwargs.get("stream", False) and 
    inspect.isclass(response_model) and 
    not issubclass(response_model, PartialBase)):
    response_model = Partial[response_model]

2. Code Duplication:
The exact same logic appears in two functions. Consider extracting to a helper function for DRY principles:

def auto_wrap_with_partial_if_streaming(response_model, kwargs):
    if (kwargs.get("stream", False) and 
        inspect.isclass(response_model) and 
        not issubclass(response_model, PartialBase)):
        return Partial[response_model]
    return response_model

⚡ Performance Considerations

✅ Minimal performance overhead - single boolean check and type check
✅ No unnecessary object creation when not streaming
✅ issubclass() call is efficient for this use case

🔒 Security Assessment

✅ No security concerns identified
✅ No user input validation issues
✅ No injection or manipulation risks

🧪 Test Coverage Review

Excellent test additions:

✅ Both sync and async variants covered (test_regular_model_with_streaming_sync and test_regular_model_with_streaming_async)
✅ Tests specifically verify the automatic wrapping behavior
✅ Parameterized tests across all models and modes
✅ Comprehensive assertions checking both partial updates and final results
✅ Tests use regular models (not explicitly Partial[UserExtract]) to verify the fix

Test quality:

Clear test names describing the behavior being tested
Good assertions verifying both iteration count and final model state
Proper async/await usage in async tests

📚 Documentation Updates

Well-executed documentation changes:

✅ Updated provider from genai/gemini-1.5-flash to google/gemini-2.5-flash
✅ Added clear streaming limitations warning
✅ Comprehensive async streaming examples
✅ Clear explanation of when to use Partial[YourModel] explicitly

📋 Minor Suggestions

Type Safety: Add inspect.isclass() check to match existing codebase patterns
DRY Principle: Consider extracting the duplicate logic to a helper function
Documentation: The automatic wrapping could be mentioned explicitly in the documentation

🎯 Overall Assessment

This is a high-quality fix that:

✅ Solves the stated problem elegantly
✅ Maintains backward compatibility
✅ Includes comprehensive tests
✅ Follows existing code patterns
✅ Has minimal performance impact
✅ Improves developer experience significantly

The implementation correctly identifies that streaming requires Partial models and automatically applies this requirement, which greatly improves the developer experience by removing the need for users to manually wrap models.

Recommendation: ✅ Approve with the minor type safety suggestion above for future consideration.

ellipsis-dev

Important

Looks good to me! 👍

Reviewed 97646b4 in 51 seconds. Click for details.

Reviewed 32 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 3 draft 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. pyproject.toml:116

Draft comment:
Added pre-commit dependency. Ensure any required pre-commit configuration (like .pre-commit-config.yaml) is updated.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 85% None

2. uv.lock:1835

Draft comment:
Pre-commit added in dev dependencies without a specifier. Verify consistency with pyproject.toml version constraints.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 85% None

3. uv.lock:1993

Draft comment:
Pre-commit added with explicit version specifier. Confirm it matches the pyproject.toml requirement and overall dependency management.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 85% None

Workflow ID: wflow_cOj0QwaQPH8ngrZF

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev

Important

Looks good to me! 👍

Reviewed a5363f9 in 2 minutes and 14 seconds. Click for details.

Reviewed 406 lines of code in 3 files
Skipped 0 files when reviewing.
Skipped posting 11 draft 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. docs/integrations/genai.md:54

Draft comment:
Updated provider from 'genai/gemini-1.5-flash' to 'google/gemini-2.5-flash'. Ensure all examples consistently use the new provider name.
Reason this comment was not posted:
Comment looked like it was already resolved.

2. docs/integrations/genai.md:180

Draft comment:
Reformatted the 'messages' parameter from a plain string to a structured dict with 'role' and 'content'. Confirm that users are aware of the new message schema.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 85% The comment is asking for confirmation that users are aware of a change, which violates the rule against asking the PR author to confirm intentions or ensure behavior is intended. It doesn't provide a specific code suggestion or point out a clear issue with the code itself.

3. docs/integrations/genai.md:484

Draft comment:
Updated the file upload example to wrap the prompt and file object inside a dict with 'role' and a list in 'content'. Verify that this structured format is correctly handled by the API.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 85% This comment is asking the PR author to verify that the structured format is correctly handled by the API. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.

4. docs/integrations/genai.md:511

Draft comment:
Added a warning block for streaming limitations. The warning clearly outlines that streaming isn’t supported for regular models unless using Partial explicitly. This improves docs clarity.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 85% This comment is purely informative, explaining what was done in the code. It doesn't provide any actionable feedback or suggestions for improvement. According to the rules, purely informative comments should be removed.

5. docs/integrations/genai.md:541

Draft comment:
Streaming example now uses create_partial to leverage automatic Partial wrapping. Ensure this behavior is well-documented and that users understand when to use create_partial.
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.

6. docs/integrations/genai.md:567

Draft comment:
Removed the redundant 'from google import genai' import in the async support snippet, cleaning up the example.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 85% This comment is purely informative, stating that a redundant import was removed. It does not provide any actionable feedback or suggestions for improvement.

7. tests/llm/test_genai/test_stream.py:17

Draft comment:
Removed the explicit import and usage of Partial; test now uses create_partial with response_model=UserExtract to validate automatic Partial wrapping.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 85% This comment is purely informative, describing what was changed in the code without providing any actionable feedback or suggestions. It does not align with the rules for good comments, as it does not ask for confirmation of intention, suggest improvements, or highlight potential issues.

8. tests/llm/test_genai/test_stream.py:101

Draft comment:
Removed redundant tests for regular model streaming (both sync and async). Ensure that the automatic Partial wrapping is sufficiently covered by the remaining create_partial tests.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 30% <= threshold 85% The comment is suggesting that redundant tests were removed and is asking to ensure that the remaining tests cover the necessary functionality. This is indirectly asking for confirmation, which violates the rule against asking the PR author to ensure behavior is intended or tested. However, it does mention a specific area (create_partial tests) that should be covered, which could be seen as a specific suggestion. Overall, it leans more towards asking for confirmation rather than providing a specific suggestion or improvement.

9. uv.lock:1712

Draft comment:
Lockfile update: Pre-commit, mkdocs, and mkdocs-material dependencies have been rearranged. Verify that these changes align with the intended dependency management (e.g., pre-commit moved to dev dependencies).
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 85% The comment is asking the PR author to verify changes related to dependencies, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue with the code. Therefore, it should be removed.

10. docs/integrations/genai.md:252

Draft comment:
Typo: In the message content, "jason" is likely meant to be "Jason" if referring to a name.
Reason this comment was not posted:
Comment was on unchanged code.

11. docs/integrations/genai.md:398

Draft comment:
Typo: In the comment, "Multiple ways to load an PDF:" should be "Multiple ways to load a PDF:" since PDF is pronounced with a consonant sound (pee-dee-eff).
Reason this comment was not posted:
Comment was on unchanged code.

Workflow ID: wflow_AqVZa5FSWGLOfO6r

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

claude · 2025-07-11T20:35:57Z

Claude finished @jxnl's task —— View job

PR Review Complete ✅

I've thoroughly reviewed this PR that fixes the Gemini streaming issue by automatically wrapping regular models with Partial when streaming is enabled. Here's my comprehensive feedback:

Todo List:

Read and analyze changed files
Review core implementation in process_response.py
Evaluate test coverage and quality
Check documentation updates
Assess code quality and best practices
Identify potential bugs or issues
Review performance considerations
Check security concerns
Provide comprehensive feedback

✅ Code Quality & Implementation

Excellent implementation approach:

Clean, targeted fix that addresses the root cause
Consistent implementation across both handle_genai_structured_outputs() and handle_genai_tools() functions
Simple conditional logic: if new_kwargs.get("stream", False) and not issubclass(response_model, PartialBase):
Maintains backward compatibility - existing Partial usage continues to work

Code placement:

✅ Lines 629-631 in handle_genai_structured_outputs
✅ Lines 667-669 in handle_genai_tools
Both placements are logical and consistent with the function flow

🔍 Potential Issues & Edge Cases

1. Type Safety Consideration:
The issubclass() check could potentially fail if response_model is not a class. I noticed the codebase uses this pattern elsewhere (lines 410, 457):

if (
    inspect.isclass(response_model)
    and issubclass(response_model, (IterableBase, PartialBase))
    and stream
):

Consider adding the safety check to match existing patterns:

if (new_kwargs.get("stream", False) and 
    inspect.isclass(response_model) and 
    not issubclass(response_model, PartialBase)):
    response_model = Partial[response_model]

2. Code Duplication:
The exact same logic appears in two functions. Consider extracting to a helper function for DRY principles:

def auto_wrap_with_partial_if_streaming(response_model, kwargs):
    if (kwargs.get("stream", False) and 
        inspect.isclass(response_model) and 
        not issubclass(response_model, PartialBase)):
        return Partial[response_model]
    return response_model

⚡ Performance Considerations

✅ Minimal performance overhead - single boolean check and type check
✅ No unnecessary object creation when not streaming
✅ issubclass() call is efficient for this use case

🔒 Security Assessment

✅ No security concerns identified
✅ No user input validation issues
✅ No injection or manipulation risks

🧪 Test Coverage Review

Test strategy observation:

✅ The existing tests using create_partial() continue to work
⚠️ Notable: Tests specifically for automatic wrapping with regular models were removed in commit a5363f9, not added
✅ The existing parametrized tests across all models and modes provide good coverage
✅ Both sync and async variants are tested

Recommendation: Consider adding at least one test that explicitly uses create() (not create_partial()) with stream=True and a regular model to verify the automatic wrapping works as intended.

📚 Documentation Updates

Well-executed documentation changes:

✅ Updated provider from genai/gemini-1.5-flash to google/gemini-2.5-flash
✅ Added clear streaming limitations warning
✅ Comprehensive async streaming examples
✅ Clear explanation of when to use Partial[YourModel] explicitly

Documentation gap: The automatic wrapping behavior introduced by this PR is not explicitly documented. Users may not realize they can now use regular models with streaming.

📋 Minor Suggestions

Type Safety: Add inspect.isclass() check to match existing codebase patterns
DRY Principle: Consider extracting the duplicate logic to a helper function
Documentation: The automatic wrapping could be mentioned explicitly in the documentation
Testing: Add a test case demonstrating the new automatic wrapping behavior

🎯 Overall Assessment

This is a high-quality fix that:

✅ Solves the stated problem elegantly
✅ Maintains backward compatibility
✅ Follows existing code patterns (lines 87 and 162 show the streaming requirement)
✅ Has minimal performance impact
✅ Improves developer experience significantly

The implementation correctly identifies that streaming requires Partial models and automatically applies this requirement, which greatly improves the developer experience by removing the need for users to manually wrap models.

Recommendation: ✅ Approve with the minor type safety suggestion above for future consideration.

…response model test This change cleans up the test file by removing unnecessary imports and the parameterization of the mode argument, which is no longer needed. This helps maintain code clarity and adheres to linter requirements.

claude · 2025-07-11T20:39:15Z

Claude finished @jxnl's task —— View job

PR Review Complete ✅

I've thoroughly reviewed this PR that fixes the Gemini streaming issue by automatically wrapping regular models with Partial when streaming is enabled. Here's my comprehensive feedback:

Todo List:

Read and analyze changed files
Review core implementation in process_response.py
Evaluate test coverage and quality
Check documentation updates
Assess code quality and best practices
Identify potential bugs or issues
Review performance considerations
Check security concerns
Provide comprehensive feedback

✅ Code Quality & Implementation

Excellent implementation approach:

Clean, targeted fix that addresses the root cause
Consistent implementation across both handle_genai_structured_outputs and handle_genai_tools functions
Simple conditional logic: if new_kwargs.get("stream", False) and not issubclass(response_model, PartialBase):
Maintains backward compatibility - existing Partial usage continues to work

Code placement:

✅ Lines 629-631 in handle_genai_structured_outputs
✅ Lines 667-669 in handle_genai_tools
Both placements are logical and consistent with the function flow

🔍 Potential Issues & Edge Cases

1. Type Safety Consideration:
The issubclass() check could potentially fail if response_model is not a class. I noticed the codebase uses this pattern elsewhere (lines 87, 162):

if (
    inspect.isclass(response_model)
    and issubclass(response_model, (IterableBase, PartialBase))
    and stream
):

Consider adding the safety check to match existing patterns:

if (new_kwargs.get("stream", False) and 
    inspect.isclass(response_model) and 
    not issubclass(response_model, PartialBase)):
    response_model = Partial[response_model]

2. Code Duplication:
The exact same logic appears in two functions. Consider extracting to a helper function for DRY principles:

def auto_wrap_with_partial_if_streaming(response_model, kwargs):
    if (kwargs.get("stream", False) and 
        inspect.isclass(response_model) and 
        not issubclass(response_model, PartialBase)):
        return Partial[response_model]
    return response_model

⚡ Performance Considerations

✅ Minimal performance overhead - single boolean check and type check
✅ No unnecessary object creation when not streaming
✅ issubclass() call is efficient for this use case

🔒 Security Assessment

✅ No security concerns identified
✅ No user input validation issues
✅ No injection or manipulation risks

🧪 Test Coverage Review

Current test status:

✅ Existing tests continue to use create_partial explicitly
✅ Both sync and async streaming scenarios are covered
✅ Parameterized tests across all models and modes
✅ Clean imports - removed unused imports from test files

Missing test coverage:

❌ No explicit tests for the automatic Partial wrapping behavior with regular models
❌ Tests don't verify that client.chat.completions.create(response_model=RegularModel, stream=True) now works

Recommendation: Add tests specifically for the automatic wrapping feature:

def test_regular_model_auto_wrapping_with_streaming(client):
    # Test that regular models are automatically wrapped with Partial when streaming
    client = instructor.from_provider("google/gemini-2.5-flash", mode=mode)
    stream = client.chat.completions.create(
        model="gemini-2.5-flash",
        response_model=UserExtract,  # Regular model, not Partial[UserExtract]
        stream=True,
        messages=[{"role": "user", "content": "Jason is 25 years old"}],
    )
    # Verify streaming works and returns expected results
    for result in stream:
        assert isinstance(result, UserExtract)

📚 Documentation Updates

Well-executed documentation changes:

✅ Updated provider from genai/gemini-1.5-flash to google/gemini-2.5-flash
✅ Added clear streaming limitations warning
✅ Comprehensive async streaming examples
✅ Clear explanation of when to use Partial[YourModel] explicitly

📋 Minor Suggestions

Type Safety: Add inspect.isclass() check to match existing codebase patterns
DRY Principle: Consider extracting the duplicate logic to a helper function
Test Coverage: Add tests specifically verifying automatic Partial wrapping with regular models
Documentation: The automatic wrapping could be mentioned explicitly in the documentation

🎯 Overall Assessment

This is a high-quality fix that:

✅ Solves the stated problem elegantly
✅ Maintains backward compatibility
✅ Follows existing code patterns
✅ Has minimal performance impact
✅ Improves developer experience significantly

The implementation correctly identifies that streaming requires Partial models and automatically applies this requirement, which greatly improves the developer experience by removing the need for users to manually wrap models.

Recommendation: ✅ Approve with suggestions for additional test coverage and type safety improvements for future consideration.

claude · 2025-07-11T20:39:22Z

Claude finished @jxnl's task —— View job

PR Review Complete ✅

I've thoroughly reviewed this PR that fixes the Gemini streaming issue by automatically wrapping regular models with Partial when streaming is enabled. Here's my comprehensive feedback:

Todo List:

Read and analyze changed files
Review core implementation in process_response.py
Evaluate test coverage and quality
Check documentation updates
Assess code quality and best practices
Identify potential bugs or issues
Review performance considerations
Check security concerns
Provide comprehensive feedback

✅ Code Quality & Implementation

Excellent implementation approach:

Clean, targeted fix that addresses the root cause
Consistent implementation across both handle_genai_structured_outputs() and handle_genai_tools() functions
Simple conditional logic: if new_kwargs.get("stream", False) and not issubclass(response_model, PartialBase):
Maintains backward compatibility - existing Partial usage continues to work

Code placement:

✅ Lines 629-631 in handle_genai_structured_outputs
✅ Lines 667-669 in handle_genai_tools
Both placements are logical and consistent with the function flow

🔍 Potential Issues & Edge Cases

1. Type Safety Consideration:
The issubclass() check could potentially fail if response_model is not a class. I noticed the codebase uses this pattern elsewhere (lines 86, 161):

if (
    inspect.isclass(response_model)
    and issubclass(response_model, (IterableBase, PartialBase))
    and stream
):

Consider adding the safety check to match existing patterns:

if (new_kwargs.get("stream", False) and 
    inspect.isclass(response_model) and 
    not issubclass(response_model, PartialBase)):
    response_model = Partial[response_model]

2. Code Duplication:
The exact same logic appears in two functions. Consider extracting to a helper function for DRY principles:

def auto_wrap_with_partial_if_streaming(response_model, kwargs):
    if (kwargs.get("stream", False) and 
        inspect.isclass(response_model) and 
        not issubclass(response_model, PartialBase)):
        return Partial[response_model]
    return response_model

⚡ Performance Considerations

✅ Minimal performance overhead - single boolean check and type check
✅ No unnecessary object creation when not streaming
✅ issubclass() call is efficient for this use case

🔒 Security Assessment

✅ No security concerns identified
✅ No user input validation issues
✅ No injection or manipulation risks

🧪 Test Coverage Review

Current test status:

✅ Existing tests use create_partial() and create_iterable() methods explicitly
✅ All tests have stream=True and proper assertions
❓ Missing: Tests that specifically verify automatic wrapping with regular models (not explicitly Partial[UserExtract])

The PR description mentions "comprehensive tests for sync and async streaming with regular models" but the current test file only shows tests using explicit create_partial() calls. Tests that directly use regular models with stream=True to verify the automatic wrapping behavior would strengthen the test coverage.

📚 Documentation Updates

Well-executed documentation changes:

✅ Updated provider from genai/gemini-1.5-flash to google/gemini-2.5-flash
✅ Added clear streaming limitations warning
✅ Comprehensive async streaming examples
✅ Clear explanation of when to use Partial[YourModel] explicitly

📋 Minor Suggestions

Type Safety: Add inspect.isclass() check to match existing codebase patterns
DRY Principle: Consider extracting the duplicate logic to a helper function
Test Coverage: Add tests that verify automatic wrapping with regular models (not using create_partial())

🎯 Overall Assessment

This is a high-quality fix that:

✅ Solves the stated problem elegantly
✅ Maintains backward compatibility
✅ Follows existing code patterns
✅ Has minimal performance impact
✅ Improves developer experience significantly

The implementation correctly identifies that streaming requires Partial models and automatically applies this requirement, which greatly improves the developer experience by removing the need for users to manually wrap models.

Recommendation: ✅ Approve with the minor suggestions above for future consideration.

ellipsis-dev

Important

Looks good to me! 👍

Reviewed c9b1a74 in 1 minute and 5 seconds. Click for details.

Reviewed 16 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 3 draft 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. tests/llm/test_genai/test_response_model_none.py:8

Draft comment:
Removed unused 'import pytest'.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 85% This comment is purely informative, as it simply states that an unused import was removed. It does not provide any actionable feedback or suggestions for improvement.

2. tests/llm/test_genai/test_response_model_none.py:9

Draft comment:
Removed unused 'from instructor.mode import Mode' import.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 85% This comment is purely informative, stating that an unused import was removed. It does not provide any actionable feedback or suggestions for improvement. According to the rules, purely informative comments should be removed.

3. tests/llm/test_genai/test_response_model_none.py:11

Draft comment:
Removed unnecessary parameterized decorator; test doesn't require mode parameterization.
Reason this comment was not posted:
Comment looked like it was already resolved.

Workflow ID: wflow_DONUgapNGChELWvX

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

github-actions bot added bug Something isn't working enhancement New feature or request python Pull requests that update python code size:M This PR changes 30-99 lines, ignoring generated files. labels Jul 11, 2025

ellipsis-dev bot reviewed Jul 11, 2025

View reviewed changes

jxnl added 2 commits July 11, 2025 16:29

ellipsis-dev bot reviewed Jul 11, 2025

View reviewed changes

Merge branch 'main' into claude/issue-1691-20250711_163839

cf9c108

jxnl and others added 2 commits July 11, 2025 16:38

Merge branch 'main' into claude/issue-1691-20250711_163839

634428a

jxnl merged commit d670d1b into main Jul 11, 2025
12 of 13 checks passed

jxnl deleted the claude/issue-1691-20250711_163839 branch July 11, 2025 20:39

ellipsis-dev bot reviewed Jul 11, 2025

View reviewed changes

Uh oh!

Conversation

jxnl commented Jul 11, 2025 • edited by ellipsis-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

cloudflare-workers-and-pages bot commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Workers

Uh oh!

claude bot commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Complete ✅

✅ Code Quality & Implementation

🔍 Potential Issues & Edge Cases

⚡ Performance Considerations

🔒 Security Assessment

🧪 Test Coverage Review

📋 Minor Suggestions

🎯 Overall Assessment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Complete ✅

✅ Code Quality & Implementation

🔍 Potential Issues & Edge Cases

⚡ Performance Considerations

🔒 Security Assessment

🧪 Test Coverage Review

📋 Minor Suggestions

🎯 Overall Assessment

Uh oh!

claude bot commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Complete ✅

Todo List:

✅ Code Quality & Implementation

🔍 Potential Issues & Edge Cases

⚡ Performance Considerations

🔒 Security Assessment

🧪 Test Coverage Review

📚 Documentation Updates

📋 Minor Suggestions

🎯 Overall Assessment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

claude bot commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Complete ✅

Todo List:

✅ Code Quality & Implementation

🔍 Potential Issues & Edge Cases

⚡ Performance Considerations

🔒 Security Assessment

🧪 Test Coverage Review

📚 Documentation Updates

📋 Minor Suggestions

🎯 Overall Assessment

Uh oh!

claude bot commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Complete ✅

Todo List:

✅ Code Quality & Implementation

🔍 Potential Issues & Edge Cases

⚡ Performance Considerations

🔒 Security Assessment

🧪 Test Coverage Review

jxnl commented Jul 11, 2025 •

edited by ellipsis-dev bot

Loading

cloudflare-workers-and-pages bot commented Jul 11, 2025 •

edited

Loading

claude bot commented Jul 11, 2025 •

edited

Loading

claude bot commented Jul 11, 2025 •

edited

Loading

claude bot commented Jul 11, 2025 •

edited

Loading

claude bot commented Jul 11, 2025 •

edited

Loading

claude bot commented Jul 11, 2025 •

edited

Loading

claude bot commented Jul 11, 2025 •

edited

Loading