Skip to content
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

Add explicit check for jsonref dep in vertexai client import guard #797

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

lemontheme
Copy link
Contributor

@lemontheme lemontheme commented Jul 1, 2024

b8ceaf6 introduced a dependency on jsonref in client_vertexai.py. This dependency is not present in a standard vertexai installation. The top-level __init__.py tries to export client_vertex.ai regardless of whether jsonref is installed, triggering an import error (#772).

This can be quite puzzling for users who may have vertexai installed in their environment for different reasons.

I ran into this issue as well because I'm using the AnthropicVertex client from Anthropic's SDK. The Anthropic docs say to pip install both anthropic[vertex] and google-cloud-aiplatform. The second package adds vertexai to my environment. As a result, even though I don't want to use Instructor's Vertex AI client, Instructor will try to import it.

The proposed solution is a tiny tweak to the import guard so that it explicitly checks whether jsonref is installed. When instructor is installed with the 'vertexai' extra, this check is guaranteed to pass. In other cases, unless a different depedency has happened to install jsonref, it will fail.


🚀 This description was created by Ellipsis for commit 933cc85

Summary:

Add explicit check for jsonref dependency in vertexai import guard in instructor/__init__.py.

Key points:

  • Update instructor/__init__.py to check for both vertexai and jsonref before importing from_vertexai.
  • Prevents import errors when jsonref is not installed but vertexai is present.
  • Ensures from_vertexai is only added to __all__ if both dependencies are available.

Generated with ❤️ by ellipsis.dev

Copy link
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.

👍 Looks good to me! Reviewed everything up to 933cc85 in 27 seconds

More details
  • Looked at 14 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. instructor/__init__.py:78
  • Draft comment:
    The change to add a check for jsonref along with vertexai in the import guard is appropriate and necessary to prevent import errors for users who do not have the jsonref package installed. This ensures that the client_vertexai is only imported when all its dependencies are satisfied, aligning with the intent described in the PR.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The PR description explains that the dependency on jsonref was introduced in a previous commit and is not part of a standard vertexai installation. This change in the import guard to check for both vertexai and jsonref before importing client_vertexai is a direct response to prevent import errors when jsonref is not installed. This is a logical and necessary change to ensure that the module only attempts to import client_vertexai when all dependencies are met, thus preventing runtime errors for users who do not have jsonref installed but have vertexai. The change uses Python's all() function with a generator expression to check for both packages, which is an efficient way to ensure all required packages are available before proceeding with the import.

Workflow ID: wflow_xrd9yy4ZdF2NOd5x


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@jxnl jxnl merged commit 710b747 into jxnl:main Jul 3, 2024
8 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants