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

changed ValueError to ImportError #5103

Merged
merged 2 commits into from
May 22, 2023
Merged

changed ValueError to ImportError #5103

merged 2 commits into from
May 22, 2023

Conversation

leo-gan
Copy link
Collaborator

@leo-gan leo-gan commented May 22, 2023

changed ValueError to ImportError

Code cleaning.
Fixed inconsistencies in ImportError handling. Sometimes it raises ImportError and sometime ValueError.
I've changed all cases to the raise ImportError
Also:

  • added installation instruction in the error message, where it missed;
  • fixed several installation instructions in the error message;
  • fixed several error handling in regards to the ImportError

Who can review?

@hwchase17
@dev2049

@leo-gan leo-gan marked this pull request as ready for review May 22, 2023 21:25

tokens = []
Copy link
Contributor

Choose a reason for hiding this comment

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

bit hard to tell exactly what's going on here but guessing you just took all the downstream code out of the try/except without any functional changes?

Copy link
Collaborator Author

@leo-gan leo-gan May 22, 2023

Choose a reason for hiding this comment

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

yes, exactly.
BTW I've found several places where import is inside class/function but without try: except ImportError brackets. Is it OK, or would we add these brackets to all internal imports?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think some classes perform the check at instantiation, so in those cases it's fine not to check within an instance method (since you would've had to successfully instantiate the object to get there)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But then why this import is inside the code not at the file beginning? I'm still not comfortable with this idea. But the worst scenario we still get the same ImportError but without a clear explanation of how the missed package should be installed ("... use pip install <package-name>"). It is not a big deal.

@leo-gan leo-gan requested a review from dev2049 May 22, 2023 22:11
@dev2049 dev2049 merged commit c28cc0f into langchain-ai:master May 22, 2023
@danielchalef danielchalef mentioned this pull request Jun 5, 2023
This was referenced Jun 25, 2023
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