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: created custom error when env var for providers does not exist or unknown provider, moderator is attempted to be loaded #69

Closed
wants to merge 8 commits into from

Conversation

lifeizhou-ap
Copy link
Collaborator

@lifeizhou-ap lifeizhou-ap commented Sep 30, 2024

Why

Currently it throws a runtime error and print the stack trace when

  • when the environment variable is not set for LLM apis,
  • when unknown provider or moderator is trying to be loaded

This make it hard for users to understand what is going.

What

  • Raise a MissingProviderEnvVariableError with attributes of provider, env_variable
  • Raise a LoadExchangeAttributeError for unknown provider or moderator

With the above custom erros we can construct user friendly messages in goose

How will it be used
The errors will be caught in goose. goose PR

Note
This PR is for early feedback, and it won't be merged into this repo as @baxen is going to move this repo to goose. I will move the PR change to goose accordingly afterwards

@lifeizhou-ap lifeizhou-ap marked this pull request as draft September 30, 2024 08:11
@lifeizhou-ap lifeizhou-ap marked this pull request as ready for review October 1, 2024 03:36
@lifeizhou-ap lifeizhou-ap changed the title created custom error when env var for providers does not exist fix: created custom error when env var for providers does not exist Oct 1, 2024
@lifeizhou-ap lifeizhou-ap changed the title fix: created custom error when env var for providers does not exist fix: created custom error when env var for providers does not exist or unknown provider, moderator is attempted to be loaded Oct 2, 2024
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

looks like progress

self.instructions = instructions
self.message = f"Missing environment variable: {env_variable} for provider {provider}"
if instructions:
self.message += f". {instructions}"
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe rename instructions to instructions_url?

Also, not sure style, but if ok with multi-line error we could \n this and then make the asserts multi-line

Suggested change
self.message += f". {instructions}"
self.message += f". See {instructions} for instructions."

Copy link
Collaborator Author

@lifeizhou-ap lifeizhou-ap Oct 2, 2024

Choose a reason for hiding this comment

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

Thanks @codefromthecrypt for your feedback!

Yes, we can rename to instructions_url so that we will have consistent message pattern. On the other hand, I am thinking whether we shall have some flexibility for the instructions. For example, people can put instructions without url. It is hard to know the future :).

I'll change as you suggested, in case we want to have more flexibility, it is fine because the function signature won't change anyway.

Also I've added the error message for unknown configuration values and put the screenshot in block/goose#103. I would like to get your feedback too! Thank you!

@lifeizhou-ap
Copy link
Collaborator Author

moved the change to block/goose#103

codefromthecrypt pushed a commit to codefromthecrypt/exchange that referenced this pull request Oct 13, 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.

2 participants