OpenHands icon indicating copy to clipboard operation
OpenHands copied to clipboard

[Bug]: Error when API key is not entered is not clear

Open neubig opened this issue 1 year ago • 4 comments

Is there an existing issue for the same bug?

  • [X] I have checked the troubleshooting document at https://docs.all-hands.dev/modules/usage/troubleshooting
  • [X] I have checked the existing issues.

Describe the bug

In the frontend, if no API key has been entered, we should have a comprehensible error. However, if not API key is entered, we get an opaque error that just says "An error occurred while running the agent"

image (2) image (1)

This should be made more clear. @amanape , could you take a look at this?

Current OpenHands version

0.9

Installation and Configuration

docker?

Model and Agent

No response

Operating System

No response

Reproduction Steps

No response

Logs, Errors, Screenshots, and Additional Context

See conversation on slack: https://openhands-ai.slack.com/archives/C06U8UTKSAD/p1728265897191799

neubig avatar Oct 07 '24 05:10 neubig

This seems a duplicate (particular case) of https://github.com/All-Hands-AI/OpenHands/issues/1451, and https://github.com/All-Hands-AI/OpenHands/issues/3739.

enyst avatar Oct 07 '24 05:10 enyst

Yeah, possibly. But I also think that maybe we shouldn't even be able to start entering text if the user hasn't entered an API key and selected a model.

neubig avatar Oct 07 '24 05:10 neubig

That makes sense, although I'd note that, personally, I actually tried to not enforce a non-empty API key in the backend, kept it theoretically optional just as the API key is optional in liteLLM, because there are at least a few providers and set ups that don't need one. I seem to recall it supports at least this scenario, where Azure does not use an API key, but a vault.

In theory, we support such use cases too, although I am not aware of any success report yet and I don't have access to Azure myself to check.

We used to open the Settings window in the UI at the first run. That was intended to help the user set their credentials, doesn't that happen anymore?

On the other hand, I suppose it's not a big problem if we enforce even harsher that the user must enter a dummy key at least... I can see why it makes sense from a usability for most uses cases point of view.

enyst avatar Oct 07 '24 06:10 enyst

I think it would be helpful to add some sort of indicator to let the user know there is a missing API key. The way the UI is currently set up makes it look like it is optional, and even if it were the case for some, most providers do require an API key anyways. Having the user change the setting mid session after realizing the error results in resetting the session and waiting to start a new one, which can be annoying.

We could introduce some sort of (!) or (?) symbol next to the settings icon if there is any missing setting that is important or requires the users attention.

We can also go all the way and disable the ability to start a new session until the API key is set IF liteLLM provides the information on which LLMs require it, and which do not

amanape avatar Oct 07 '24 06:10 amanape

Having the user change the setting mid session after realizing the error results in resetting the session and waiting to start a new one, which can be annoying.

Well, if the key didn't work, it's the beginning... except, yes, for the time taken by the runtime. I am not sure we really have to restart it all... By the way, the new UI looks so slick!

We can also go all the way and disable the ability to start a new session until the API key is set IF liteLLM provides the information on which LLMs require it, and which do not

Not that I know of 🤔

A ! or ? symbol sounds good to me FWIW.

enyst avatar Oct 13 '24 18:10 enyst

IMO you should not be able to close the settings modal without an API key entered. Maybe if you enter "advanced" you can then close it (since maybe some private backends don't require a key)

I'd like to also put in some logic to check the key before letting you close the modal, e.g. making a test call to the API

rbren avatar Oct 14 '24 14:10 rbren

  • Keeping a user trapped in a modal isn't ideal - they could've just "clicked around" to see what options exist.
  • A test call should be optional via a button, not a requirement (do not cause cost without user consent)
  • Most helpful way imo would be, as @enyst mentioned, a UI hint for the missing key (if not a local llm) for this.

tobitege avatar Oct 14 '24 15:10 tobitege

Keeping a user trapped in a modal isn't ideal - they could've just "clicked around" to see what options exist.

An alternative would be a follow-up pop-up "Are you sure? You are about to proceed without an API key which [...]". Coupled with better error messages in the chat interface due to invalid API keys, we can't say we didn't warn them 🤷‍♂️

amanape avatar Oct 14 '24 15:10 amanape

@amanape What if we could add an error toast or popup when the user exits the settings modal like an alert displaying "Are you sure? You are about to proceed without an API key you might not be able to use the agent" and so when the user confirms he can exit the modal even though he has not entered an api key.

We can also add a hanging fixed toast saying the API Keys is absent. If you have any other ideas please. I think i would do it, assign it to me.

Yashwanth-Chandrakumar avatar Oct 16 '24 05:10 Yashwanth-Chandrakumar

@Yashwanth-Chandrakumar After talking with the designer, the flow should be something like this:

  1. User sees open settings modal (whether its as first time startup or they opened it from the sidebar)
  2. User presses close without setting an API key
  3. Another modal pops up saying "Are you sure [...]"
  4. If the user presses "YES", both modals now close.
  5. If the user presses "Cancel", go back to the settings modal and repeat from step 2 until they enter a key

This is for the settings modal, but we should probably improve display error messages from the backend (in a separate PR) displayed to the user from the chat interface. Currently we only say "an error occurred", which isn't the best indicator.

Feel free to take on the assignment, we have a couple of modals and components to build modals you can find around the app for reference.

amanape avatar Oct 16 '24 05:10 amanape