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

feat(frontend): Improve models input UI/UX in settings #3530

Merged
merged 22 commits into from
Aug 23, 2024

Conversation

amanape
Copy link
Collaborator

@amanape amanape commented Aug 22, 2024

What is the problem that this fixes or functionality that this introduces? Does it fix any open issues?
The models input dumps all the raw LiteLLM IDs that cause slight performance issues and overwhelm the user with options.


Give a summary of what the PR does, explaining any non-trivial design decisions

Default behaviour

This PR breaks down the input into a Provider input and Models input. Given the provider, a filtered models list will be available for the user to choose.

image
  • Changed default LLM_MODEL to openai/gpt-4o from gpt-4o

The user can see the "Verified" providers and models that we recommend and know work well with OpenHands.
image
image

utils/verified-models.ts

export const VERIFIED_PROVIDERS = ["openai", "azure", "anthropic"];
export const VERIFIED_MODELS = ["gpt-4o", "claude-3-5-sonnet-20240620-v1:0"];

If preferred, the user can set a custom LLM model.
image

  • New settings were added, CUSTOM_LLM_MODEL and USING_CUSTOM_LLM. That way, the socket class sends the appropriate LLM with the LLM_MODEL key if settings.USING_CUSTOM_LLM is true.

Tests

  • ~85 - 90%+ test coverage

Notes

  • Need to confirm which are the verified providers and models we want to set and display to the user.
  • Need to confirm that the new way of setting custom LLM models is OK
  • Since LiteLLM does not maintain consistency between supported models (some are not returned at all and just say they support it by append some-provider/), most are dumped in an other provider. Need to confirm if it is OK, which ones do we want to "pull out", and if there are alternative ways to handle this (TBH considering going to the LiteLLM repo and making the changes for consistency myself)

TODO

  • Add a switch to allow the user to switch between LiteLLM models and a custom model
  • Investigate providers that offer custom models
  • Input validation
  • Set verified models and providers
  • Bug fixes

Accidentally removed #3514 after re-forking

@amanape amanape self-assigned this Aug 22, 2024
@amanape amanape marked this pull request as ready for review August 22, 2024 15:13
@amanape
Copy link
Collaborator Author

amanape commented Aug 22, 2024

@neubig Could confirm the verified providers and models we want to display?

export const VERIFIED_PROVIDERS = ["openai", "azure", "anthropic"];
export const VERIFIED_MODELS = ["gpt-4o", "claude-3-5-sonnet-20240620-v1:0"];

@tobitege
Copy link
Collaborator

tobitege commented Aug 22, 2024

Is OpenRouter still selectable as provider?
For reference:
https://openrouter.ai/models

@amanape
Copy link
Collaborator Author

amanape commented Aug 22, 2024

@tobitege Yes, looks like we do:
image

The thing I can't be certain is if all the models OpenRouter offers through LiteLLM are under this provider. It could be the case that there are additional models LiteLLM does NOT list/return, or they do but without the provider (e.g., some-model instead of openrouter/some-model). In those cases, I will need to do a little hardcoding stuff as I did for OpenAI

@tobitege
Copy link
Collaborator

@tobitege Yes, looks like we do:

Thanks! Yes, OpenRouter could add new models any day, but usually litellm is usually pretty fast in adding support for new ones.
I'd not restrict any selection a user may do with OpenRouter, it'll either work or not, to be blunt. 😬

@amanape
Copy link
Collaborator Author

amanape commented Aug 22, 2024

I'd not restrict any selection a user may do with OpenRouter, it'll either work or not, to be blunt.

Yes I'm trying not to restrict anything here. Actually the thing that this PR does remove is the ability to input a custom model given a provider (e.g., openrouter/some-random-model-maybe-not-in-docs). Do you think this is OK? I can't see the reasons why the user would want to do such a thing in the first place

@tobitege
Copy link
Collaborator

I'd not restrict any selection a user may do with OpenRouter, it'll either work or not, to be blunt.

Yes I'm trying not to restrict anything here. Actually the thing that this PR does remove is the ability to input a custom model given a provider (e.g., openrouter/some-random-model-maybe-not-in-docs). Do you think this is OK? I can't see the reasons why the user would want to do such a thing in the first place

I think it'd be ok. The typing in the box always felt wonky anyways.

@tobitege
Copy link
Collaborator

So users could still enter anything they want via the Custom option, right? Like for Ollama etc.?

Copy link
Collaborator

@tobitege tobitege left a comment

Choose a reason for hiding this comment

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

LGTM!
Just tried it out, works great, thanks!

@tobitege tobitege merged commit 07e750f into All-Hands-AI:main Aug 23, 2024
RajWorking pushed a commit to RajWorking/OpenHands that referenced this pull request Aug 26, 2024
…3530)

* Create helper functions

* Add map according to litellm docs

* Create ModelSelector

* Extend model selector

* use autocomplete from nextui

* Improve keys without providers

* Handle models without a provider

* Add verified section and some empty handling

* Add support for default or previously set models

* Update tests

* Lint

* Remove modifier

* Fix typescript error

* Functionality for switching to custom model

* Add verified models

* Respond to resetting to default

* Comment
@enyst
Copy link
Collaborator

enyst commented Sep 10, 2024

I'd not restrict any selection a user may do with OpenRouter, it'll either work or not, to be blunt.

Yes I'm trying not to restrict anything here. Actually the thing that this PR does remove is the ability to input a custom model given a provider (e.g., openrouter/some-random-model-maybe-not-in-docs). Do you think this is OK? I can't see the reasons why the user would want to do such a thing in the first place

@amanape There is this tricky thing where liteLLM interprets the model names like "openai/something" as a hint to route the completion call to an openai-compatible endpoint, which they build according to base_url or provider-specific url, and ending with /v1, in other words they build some API URL which they know accepts messages in OpenAI format, because the user added "openai" prefix. (In that case it also makes sure that the parameters are in the right format, I think)

I don't know if that has relevance for openrouter. I know our users needed it in some cases, like CommandR and/or Cohere where the prefix worked, its missing did not.

My point is that typing in the box was necessary to add /openai to some prefixes. Maybe it's fine if it's still possible via custom model, though.

Sorry for typing in a closed issue, just a quick thought and I'll take it these days elsewhere for further thinking/testing.

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.

3 participants