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

Better handling the case when a new user gets an error #100

Open
NP4567-dev opened this issue Dec 5, 2024 · 1 comment
Open

Better handling the case when a new user gets an error #100

NP4567-dev opened this issue Dec 5, 2024 · 1 comment
Labels
feature request Feature request

Comments

@NP4567-dev
Copy link
Collaborator

Description

Scenario:

  • A user wants to try ecologits
  • They copy and paste the getting started tutorial
  • Because of some unforeseeable virtual environment issue, the Ecologits.init() method fails on one specific provider with an obscure not caught exception.
  • Depending on their ability to identify the root cause of the problem, they might juste deem the package broken and give up on using it.

It just happened to a colleague of mine, I will see if I have enough info to create a bug issue too on it later.

Suggested possible way to handle this:

1- Throw a more explicit error
When one specific provider fails to start catch the exception and raise an error with the following informations:

  • there is an error with xxx provider
  • please try specifying providers other than xxx + example
  • if you need xxx provider, pleas retry on a new virtual environnment or update your dependencies first
  • if it still does not work, please open an issue with the error you just encountered and you virtual environement specifications
    This seem to me like a good way to handle it but I might have missed a point.

2- Catch the error , warn the user there was a problem loading it and leave it "for later".
Either the user does not need the incriminated provider and won't have any problem or they will get an error when they end up trying the given provider.
Not a big fan of this one as it might end up with the user just not having the impacts calculated on their results not knowing why.

3- Update the getting started tutorial
Needing more than one providers on a project is probably not happening often, so this would be closer to reality and less error prone I guess?
The bad part is that it entails choosing a "favorite" provider or using a placeholder. Also the user choosing a provider that rises an error in their environment would still be annoyed and might give up on ecologits.

@NP4567-dev NP4567-dev added the feature request Feature request label Dec 5, 2024
@samuelrince
Copy link
Member

Hi @NP4567-dev

Thanks for the feedback! I am very curious about the bug that one of your colleagues encountered.

I agree with you, we need to better detect initialization issues and especially if it is related to EcoLogits. I think I'd rather have the initialization fail (and throw an exception) instead of a simple warning or silent error. (If you intend it to work and it does not, it should fail in that case)

The more I think about this, the more I am convinced that we probably need to rework the initialization process. I agree that use cases where you have multiple providers inside one project can be rare. The issue with trying to initialize everything all at once could be more prone to errors.

We can get inspired by what other libraries do for monitoring. We can take as an example how it's currently done in Logfire, as we discussed last time. Something like:

from ecologits import EcoLogits

# Setup global config such as:
#   1. Endpoint and auth for monitoring (in the future)
#   2. Override default config globally
EcoLogits.configure()

# Initialize for a specific provider (also possible to override config for that provider)
EcoLogits.init_openai()

This way, if I have huggingface-hub installed, but I don't use the inference endpoints, it will never try to load it. The key difference here is that we drop the init() function (or we enforce to manually put providers in the parameters of the function).

Plus, if the initialization fails, in that case, it will be clear that it is when working with OpenAI and not some other provider being detected.

Furthermore, having the configure() method makes more sense if you only want to use the library to compute environmental impacts and not for tracing/monitoring. This could be great for an "ecologits-api" project, or even just the calculator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Feature request
Projects
None yet
Development

No branches or pull requests

2 participants