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

fix: update imports to allow global dspy.Google #419

Merged
merged 3 commits into from
Feb 23, 2024

Conversation

KCaverly
Copy link
Collaborator

Currently, the new Google LM class, does not have consistent imports with the remainder of the LMs.

ie. dspy.Cohere works, but dspy.Google does not.

This PR makes small import changes to ensure consistency.

@okhat
Copy link
Collaborator

okhat commented Feb 21, 2024

LGTM, running checks

@insop
Copy link
Contributor

insop commented Feb 22, 2024

One minor question, and looks good to me.
Thank you,

@@ -8,7 +8,8 @@
import google.generativeai as genai
except ImportError:
google_api_error = Exception
print("Not loading Google because it is not installed.")
# print("Not loading Google because it is not installed.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why commenting this out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If it stays, we print this every time anybody imports dspy, regardless of whether they are importing Google specifically. It's similar to how imports are managed on the Cohere side in: dsp/modules/cohere.py.

I imagine there may be a cleaner way throughout the project to manage optional dependencies.

@okhat
Copy link
Collaborator

okhat commented Feb 23, 2024

Running some basic checks then merging

@okhat
Copy link
Collaborator

okhat commented Feb 23, 2024

I wish there was an automatic way to tell github "please merge but only if the checks pass" instead of just waiting for the checks. Maybe there is.

@okhat okhat merged commit 9d8a40c into stanfordnlp:main Feb 23, 2024
1 check passed
@nbqu nbqu mentioned this pull request Feb 26, 2024
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.

3 participants