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

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

Closed
2 tasks done
neubig opened this issue Oct 7, 2024 · 10 comments · Fixed by #4429
Closed
2 tasks done

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

neubig opened this issue Oct 7, 2024 · 10 comments · Fixed by #4429
Assignees
Labels
bug Something isn't working frontend Related to frontend severity:medium Affecting multiple users

Comments

@neubig
Copy link
Contributor

neubig commented Oct 7, 2024

Is there an existing issue for the same bug?

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 neubig added bug Something isn't working frontend Related to frontend labels Oct 7, 2024
@enyst
Copy link
Collaborator

enyst commented Oct 7, 2024

This seems a duplicate (particular case) of #1451, and #3739.

@neubig
Copy link
Contributor Author

neubig commented Oct 7, 2024

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.

@enyst
Copy link
Collaborator

enyst commented Oct 7, 2024

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.

@amanape
Copy link
Member

amanape commented Oct 7, 2024

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

@mamoodi mamoodi added the severity:medium Affecting multiple users label Oct 7, 2024
@enyst
Copy link
Collaborator

enyst commented Oct 13, 2024

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.

@rbren
Copy link
Collaborator

rbren commented Oct 14, 2024

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

@tobitege
Copy link
Collaborator

  • 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.

@amanape
Copy link
Member

amanape commented Oct 14, 2024

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 🤷‍♂️

@Yashwanth-Chandrakumar
Copy link
Contributor

@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.

@amanape
Copy link
Member

amanape commented Oct 16, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working frontend Related to frontend severity:medium Affecting multiple users
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants