Ability to refresh LiteLLM models list #3687
Conversation
|
dosubot
bot
added
size:XL
slytechnical
changed the title
|
Currently addressing a potential bug in this where if you have the LiteLLM provider set up but stop your local LiteLLM dev server it can cause calls to refresh other provider models to fail. Haven't identified yet if that was a regression or previously existing bug, but I will reopen this once the bug is addressed and will add a test case for this |
dosubot
bot
added
size:XXL
|
PR re-opened with a fix for the bug where model call failing for one provider could prevent model list refresh for other providers along with test cases for that bug |
| const routerNameFlush: RouterName = toRouterName(message.text) | ||
| await flushModels(routerNameFlush) | ||
| break | ||
| case "requestProviderModels": { |
There was a problem hiding this comment.
This is a nice change 👍
What do you think about starting to move some of the logic from these case statements into separate modules (especially the large ones)? This is a pretty good candidate for that.
There was a problem hiding this comment.
1000% would love to start breaking this apart/refactoring. That + a few other small things still in flight for this pr, will push soon
There was a problem hiding this comment.
In the interest of keeping this PR right-sized I think we should actually tackle that separately. I can look into some potential implementations of that as a refactor and make a github issue.
Probably better to step back and come up with the right long term path than break the existing pattern for just one case in this PR
There was a problem hiding this comment.
https://github.com/RooVetGit/Roo-Code/tree/webview-message-handler-refactor speculative branch of one approach
cte
left a comment
cte
left a comment
There was a problem hiding this comment.
Let a few minor comments, but otherwise LGTM.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
|
@cte ok this is ready for re-review
|
|
I encountered this same behavior on this branch #3658 except the error message doesn't flash display at all, you just hang on the loading spinner. I wasn't aware that I altered/touched chat view code at all so I'm looking into the issue Edit: update that I did see it once even on this branch, so it's probably some kind of race condition and I'm thinking this branch doesn't add a regression but I'm not 100% sure |
slytechnical
force-pushed
the
litellm-provider-config
branch
from
ff116d6 to
fb6b59f
Compare
slytechnical
force-pushed
the
litellm-provider-config
branch
from
1cfc116 to
4591b1b
Compare
| options = { provider: "unbound", apiKey: this.apiKey } | ||
| break | ||
| case "litellm": | ||
| options = { provider: "litellm", apiKey: this.apiKey, baseUrl: this.baseURL } |
There was a problem hiding this comment.
Typo: The property key baseUrl is used here, but the rest of the file uses baseURL. Consider changing it for consistency, if it's not intentional.
| if (modelName.includes("claude-3-7-sonnet")) { | ||
| // due to https://github.com/BerriAI/litellm/issues/8984 until proper extended thinking support is added | ||
| determinedMaxTokens = 64000 | ||
| } |
There was a problem hiding this comment.
Can we include the Anthropic header instead? https://docs.litellm.ai/docs/proxy/request_headers
|
|
||
| if (!modelName || !modelInfo || !litellmModelName) continue | ||
|
|
||
| let determinedMaxTokens = modelInfo.max_tokens || modelInfo.max_output_tokens || 8192 |
There was a problem hiding this comment.
The other thing is that 64k or 128k output tokens is too high of a number in most cases for Sonnet - it eats up a ton of the available context window. In other providers we have a slider to choose the max tokens value.
There was a problem hiding this comment.
Are the changes in this file intentional?
There was a problem hiding this comment.
I wondered also- I believe it was the husky pre-commit running prettier. I'm curious how that was in main without prettier formatting in the first place.
| const currentProvider = apiConfiguration.apiProvider | ||
| // Needs provider default if apiProvider is undefined, null, or an empty string. | ||
| const needsProviderDefault = !currentProvider | ||
| const isLiteLLMSelectedOrShouldBeDefault = currentProvider === "litellm" || needsProviderDefault |
There was a problem hiding this comment.
Do you mind explaining why this is necessary? It's not possible to just configure litellm on the welcome view otherwise? Kind of feels like this logic should live elsewhere (not in the view layer).
There was a problem hiding this comment.
Correct, with the current main branch it's not possible to set up liteLLM on the welcome screen. The changes to the base url and api key fields don't take effect until you hit the save button but the welcome screen doesn't have a save button. This is kind of a special case though and there could be a better approach
There was a problem hiding this comment.
How do the other providers handle it?
There was a problem hiding this comment.
I believe for them it doesn't matter if the values update while you're on the actual welcome screen since it's at the point of the handleSubmit
that theupsertApiConfiguration is actually posted
For liteLLM it does matter on the welcome screen itself pre-submit because these updated values are what's used to set the base url and api key for the refresh models call. liteLLM I think is unique in that you need to be able to update these values pre clicking the primary submit button of the welcome screen
Requesty has the refresh models call but it's always the same url to refresh the models. liteLLM the url and apiKey are dynamic since you can be running it locally and they're whatever you have them configured to be
There was a problem hiding this comment.
What about the OpenAI compatible option?
There was a problem hiding this comment.
Is this change needed for something?
There was a problem hiding this comment.
Without this WelcomeView.tsx ends up in an infinite render loop
|
closing in favor of #3852 |
github-project-automation
bot
moved this from PR [Pre Approval Review]
to Done
in Roo Code Roadmap
Related GitHub Issue
Closes: #3613
Description
Test Procedure
litellm.test.tsandvalidate.test.tsType of Change
srcor test files.Pre-Submission Checklist
npm run lint).console.log) has been removed.npm test).mainbranch.npm run changesetif this PR includes user-facing changes or dependency updates.Screenshots / Videos
Documentation Updates
Important
Adds LiteLLM model refresh functionality with UI feedback, specific provider fetching, and updates validation and tests.
ApiOptions.tsxandLiteLLM.tsx.LiteLLM.tsxandRequesty.tsx.webviewMessageHandler.ts.claude-3-7models inlitellm.ts.validateModelId()invalidate.tsto check model availability.validate.test.tsfor model validation logic.litellm.test.tsfor testing LiteLLM handler.webviewMessageHandler.test.tsfor testing message handling.This description was created by
for 4591b1b. You can customize this summary. It will automatically update as commits are pushed.