Skip to content

Restore multimodal import compatibility#1797

Merged
jxnl merged 5 commits intomainfrom
cursor/restore-multimodal-import-compatibility-f132
Sep 2, 2025
Merged

Restore multimodal import compatibility#1797
jxnl merged 5 commits intomainfrom
cursor/restore-multimodal-import-compatibility-f132

Conversation

@jxnl
Copy link
Copy Markdown
Collaborator

@jxnl jxnl commented Sep 1, 2025

fix: restore backward compatibility for multimodal imports

Describe your changes

This PR addresses the critical issue (#1796) where instructor.multimodal imports were removed in v1.11.1 without deprecation, causing production systems to break due to a violation of semantic versioning.

To restore backward compatibility and provide a proper migration path:

  • A shim module instructor/multimodal.py has been reintroduced. This module re-exports all previously available classes and functions from instructor.processing.multimodal.
  • A DeprecationWarning is now issued when instructor.multimodal is imported, guiding users to update their imports to instructor.processing.multimodal and indicating that the shim will be removed in v2.0.0.
  • The docs/examples/audio_extraction.md has been updated to use the new instructor.processing.multimodal import path.

This change unblocks affected production systems, restores semantic versioning, and provides a clear, documented upgrade path for users.

Issue ticket number and link

#1796 #1796

Checklist before requesting a review

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests.
  • If it is a core feature, I have added documentation.

Slack Thread

Open in Cursor Open in Web

Important

Restores backward compatibility for instructor.multimodal imports with a shim module and deprecation warnings, updates documentation, and bumps version to 1.11.3.

  • Backward Compatibility:
    • Reintroduces instructor/multimodal.py as a shim module to re-export classes and functions from instructor.processing.multimodal.
    • Issues DeprecationWarning for instructor.multimodal imports, advising users to switch to instructor.processing.multimodal.
    • Similar backward compatibility shims added for instructor.client, instructor.hooks, instructor.patch, instructor.process_response, and instructor.validators.
  • Documentation:
    • Updates docs/examples/audio_extraction.md to use instructor.processing.multimodal import path.
  • Versioning:
    • Bumps version in pyproject.toml from 1.11.2 to 1.11.3.

This description was created by Ellipsis for 638d03a. You can customize this summary. It will automatically update as commits are pushed.

@cursor
Copy link
Copy Markdown

cursor Bot commented Sep 1, 2025

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
Learn more about Cursor Agents

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Sep 1, 2025

Claude finished @jxnl's task —— View job


PR Review: Restore multimodal import compatibility

I've completed my review of this PR. This is a well-executed solution to restore backward compatibility for the multimodal import path. Here's my comprehensive feedback:

Strengths

1. Code Quality & Best Practices

  • Clean, well-documented shim implementation following Python conventions
  • Proper use of __all__ to control public API exports
  • Clear docstring explaining purpose and migration path
  • Follows repository's import order standards (stdlib → third-party → local)
  • Uses appropriate stacklevel=2 for warning attribution

2. Backward Compatibility Approach

  • Excellent solution that immediately unblocks affected production systems
  • Proper semantic versioning restoration (no breaking changes in minor versions)
  • Clear migration path with deprecation warning
  • All original exports preserved (PDF, Image, Audio, helper functions, constants)

3. Documentation Update

  • Updated audio_extraction.md to use new import path: /instructor/processing/multimodal.py:17
  • Shows users the correct way forward while maintaining runnable examples

⚠️ Areas for Improvement

1. Missing Test Coverage

  • Critical: No tests for the deprecation warning behavior
  • Important: No tests verifying that all imports work correctly from the shim
  • Recommendation: Add tests similar to /instructor/tests/llm/test_vertexai/test_deprecated_async.py:20-23

2. Suggested Test Implementation

def test_multimodal_import_deprecation_warning():
    """Test that importing from instructor.multimodal issues deprecation warning."""
    with pytest.warns(
        DeprecationWarning, 
        match="Importing from 'instructor.multimodal' is deprecated"
    ):
        from instructor.multimodal import PDF, Image, Audio

def test_multimodal_shim_exports():
    """Test that all expected exports are available from the shim."""
    from instructor.multimodal import PDF, Image, Audio, autodetect_media
    # Verify they're the same objects as the real imports
    from instructor.processing.multimodal import PDF as RealPDF
    assert PDF is RealPDF

🔍 Technical Analysis

Performance Considerations: ✅ Good

  • Minimal import overhead (direct re-exports)
  • No runtime performance impact
  • Warning only issued once per import

Security Considerations: ✅ No concerns

  • Simple re-export mechanism
  • No new attack surface introduced
  • Maintains same security posture as original module

Potential Issues: ⚠️ Minor

  • Warning will appear in every process that imports the old path
  • No version in warning message (though v2.0.0 is mentioned in docstring)

📋 Recommendations

  1. Add comprehensive tests - This is the most critical missing piece
  2. Consider updating the warning message to include current version:
    "Importing from 'instructor.multimodal' is deprecated as of v1.11.2 and will be removed in v2.0.0..."
  3. Add a migration guide to the main documentation referencing this change
  4. Consider adding the shim to init.py imports if not already present for completeness

📊 Overall Assessment

This PR effectively addresses the critical issue while following good software engineering practices. The approach is sound, the implementation is clean, and it provides exactly what's needed for backward compatibility. The main gap is test coverage, which should be addressed before merging.

Recommendation: ✅ Approve with minor changes - Add the missing tests and consider the warning message enhancement.


Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Examine the new multimodal.py shim file
  • Review the documentation update in audio_extraction.md
  • Analyze backward compatibility approach
  • Check for potential bugs or issues
  • Assess performance and security considerations
  • Evaluate test coverage needs
  • Provide comprehensive review feedback

@github-actions github-actions Bot added bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request python Pull requests that update python code size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 1, 2025
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Sep 1, 2025

Deploying with  Cloudflare Workers  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 638d03a Commit Preview URL

Branch Preview URL
Sep 02 2025, 07:23 PM

@KennyVaneetvelde
Copy link
Copy Markdown

Hi @jxnl - I don't think it was just the multimodal imports, though. There were others, but I don't know which ones exactly off the top of my head.

@jxnl jxnl marked this pull request as ready for review September 2, 2025 23:46
@jxnl jxnl merged commit 369d928 into main Sep 2, 2025
11 of 13 checks passed
@jxnl jxnl deleted the cursor/restore-multimodal-import-compatibility-f132 branch September 2, 2025 23:46
Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed everything up to 638d03a in 1 minute and 6 seconds. Click for details.
  • Reviewed 249 lines of code in 9 files
  • Skipped 0 files when reviewing.
  • Skipped posting 10 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/examples/audio_extraction.md:55
  • Draft comment:
    Updated import statement now references 'instructor.processing.multimodal'. Ensure all examples remain consistent with this change.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 85% None
2. instructor/__init__.py:32
  • Draft comment:
    Backward compatibility re-exports (handle_response_model and handle_parallel_model) have been added. Verify that their deprecation timeline is documented in the release notes.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 85% None
3. instructor/client.py:9
  • Draft comment:
    The lazy getattr deprecation for 'instructor.client' is implemented correctly with an informative warning.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 85% None
4. instructor/hooks.py:9
  • Draft comment:
    The deprecation mechanism for 'instructor.hooks' using getattr is consistent and clear.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 85% None
5. instructor/multimodal.py:9
  • Draft comment:
    The deprecation warning in 'instructor.multimodal' properly informs users to switch to 'instructor.processing.multimodal'.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 85% None
6. instructor/patch.py:9
  • Draft comment:
    The lazy getattr implementation in 'instructor/patch.py' is correctly set up with a clear deprecation message.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 85% None
7. instructor/process_response.py:9
  • Draft comment:
    The deprecation warning in 'instructor.process_response' is correctly implemented and directs users to 'instructor.processing.response'.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 85% None
8. instructor/validators.py:9
  • Draft comment:
    The fallback mechanism in the getattr for 'instructor.validators' (checking 'validation' then 'processing.validators') works as intended.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 85% None
9. pyproject.toml:20
  • Draft comment:
    Version bumped to 1.11.3. Ensure that the release notes and upgrade documentation reflect the backward compatibility changes and deprecation notices.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 85% None
10. instructor/client.py:1
  • Draft comment:
    Note: Similar lazy getattr deprecation wrappers are repeated across multiple modules (client, hooks, multimodal, patch, process_response, validators). Consider refactoring these into a common helper function to reduce duplication and ease future maintenance.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 85% None

Workflow ID: wflow_lWmHVNUkC9rZtsdg

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request python Pull requests that update python code size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants