fix: Python 3.13 compatibility and import path corrections#1866
Conversation
This commit addresses multiple test failures and compatibility issues: 1. Replace deprecated imghdr module with magic byte detection - imghdr was removed in Python 3.13 - Implemented manual image type detection for JPEG, PNG, GIF, WEBP - Location: instructor/processing/multimodal.py 2. Fix incorrect VertexAI import path - Changed from instructor.client_vertexai to instructor.providers.vertexai.client - Location: instructor/providers/gemini/utils.py:964 3. Handle Anthropic system prompt format variations - Updated test helper to support both string and list[dict] formats - Location: tests/test_response_model_conversion.py:15-26 Test Results: - Core tests: 244/244 passing (100%) - Overall: 1052 passing (up from 908) - Remaining failures are integration tests requiring API keys Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
|
Claude finished @jxnl's task —— View job 🔍 Code Review Complete ✅I've analyzed this PR that fixes Python 3.13 compatibility and import path issues. Here's my comprehensive review: Code Quality & Best Practices ✅Overall Assessment: The changes are well-implemented and follow good practices. The code maintains consistency with existing patterns and includes proper error handling. Strengths:
Magic Byte Detection Implementation (multimodal.py:150-177) ✅Security & Correctness:
Technical Details:
Minor Enhancement Opportunity: # Current check (line 164)
elif decoded.startswith(b'RIFF') and decoded[8:12] == b'WEBP':
# Could be more defensive
elif decoded.startswith(b'RIFF') and len(decoded) >= 12 and decoded[8:12] == b'WEBP':However, the current implementation is acceptable since Import Path Fix (utils.py:964) ✅Problem Resolution:
Anthropic System Prompt Handling (test_response_model_conversion.py:15-26) ✅Robust Implementation:
Implementation Quality: # Handle both string and list[dict] formats
if isinstance(system, list):
return "".join(block.get("text", "") for block in system)
return systemThis is a clean, defensive approach that handles API response variations gracefully. Performance Considerations ✅
Security Assessment ✅
Test Coverage Assessment ✅Current Coverage:
Coverage Gaps Addressed:
Potential Edge Cases & Recommendations
Final Verdict: ✅ APPROVEDRecommendation: Merge this PR This is a high-quality fix that:
The implementation is production-ready and represents a solid improvement to the codebase's compatibility and robustness. |
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 554f265 in 1 minute and 32 seconds. Click for details.
- Reviewed
56lines of code in3files - 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:964
- Draft comment:
Import path correction looks correct; ensure all downstream references are updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is asking the PR author to ensure that all downstream references are updated, which is similar to asking them to double-check or verify something. This violates the rule against asking the PR author to confirm or ensure things. Therefore, this comment should be removed.
2. tests/test_response_model_conversion.py:15
- Draft comment:
Test helper now handles both string and list formats for system prompts; this improves test reliability across providers. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states a positive aspect of the change without offering any guidance or questions for the author.
Workflow ID: wflow_lDs4rly9QYSuUSgD
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| img_type = 'png' | ||
| elif decoded.startswith(b'GIF87a') or decoded.startswith(b'GIF89a'): | ||
| img_type = 'gif' | ||
| elif decoded.startswith(b'RIFF') and decoded[8:12] == b'WEBP': |
There was a problem hiding this comment.
Consider adding a bounds check (e.g. len(decoded) >= 12) before accessing decoded[8:12] to avoid potential IndexError on very short inputs.
| elif decoded.startswith(b'RIFF') and decoded[8:12] == b'WEBP': | |
| elif decoded.startswith(b'RIFF') and len(decoded) >= 12 and decoded[8:12] == b'WEBP': |
Summary
This PR fixes multiple test failures and compatibility issues, improving the overall test pass rate from 908 to 1052 passing tests.
Changes
1. Python 3.13 Compatibility
Problem: The
imghdrmodule was removed in Python 3.13, causing multimodal image tests to fail.Solution: Replaced
imghdr.what()with magic byte detection for image type identification.File:
instructor/processing/multimodal.py:150-1772. VertexAI Import Path Fix
Problem: Incorrect import path causing ModuleNotFoundError in VERTEXAI_JSON mode tests.
Solution: Corrected import from
instructor.client_vertexaitoinstructor.providers.vertexai.clientFile:
instructor/providers/gemini/utils.py:9643. Anthropic System Prompt Format Handling
Problem: Test failure due to Anthropic system prompts being returned as list[dict] instead of string.
Solution: Updated test helper to handle both string and list[dict] formats.
File:
tests/test_response_model_conversion.py:15-26Test Results
Before:
After:
Remaining failures:
Testing
Run core tests (excluding integration tests):
uv run pytest tests/ -k "not llm and not openai and not anthropic and not docs and not auto_client"Run multimodal tests specifically:
Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
Important
Fix Python 3.13 compatibility by replacing
imghdrwith magic byte detection and correct VertexAI import path.imghdr.what()with magic byte detection inmultimodal.pyto identify image types.instructor.client_vertexaitoinstructor.providers.vertexai.clientinutils.py.get_system_prompt()intest_response_model_conversion.pyto handle string and list[dict] formats.This description was created by
for 554f265. You can customize this summary. It will automatically update as commits are pushed.