Fix test / display models #4657
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR refactors the LLM provider system to improve model configuration management and display. Here's a summary of the key changes:
- Replaced
llm_nameswith structuredmodel_configurationsinWellKnownLLMProviderDescriptorto support visibility flags and image input capabilities - Updated
LLMProviderUpdateFormto handle model visibility and configuration throughselected_model_namesstate - Modified
ConfiguredLLMProviderDisplayto properly compare model configurations when determining provider types - Fixed model name references in tests to match current naming scheme (e.g., 'GPT 4o Mini')
- Aligned LLM descriptor structure in hooks to use
providerfield consistently across the application
The changes improve type safety and maintainability while fixing issues with model display and provider identification.
6 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
The logic prior to change just had the default and fast-default models visible, correct?
There was a problem hiding this comment.
@raunakab yep that's correct!
| (modelConfiguration) => ({ | ||
| // don't clean up names here to give admins descriptive names / handle duplicates | ||
| // like us.anthropic.claude-3-7-sonnet-20250219-v1:0 and anthropic.claude-3-7-sonnet-20250219-v1:0 | ||
| name: modelConfiguration.name, | ||
| value: modelConfiguration.name, | ||
| }) | ||
| )} |
There was a problem hiding this comment.
This logic seems to be duplicated many times. Maybe we could pull it out into its own helper function?
There was a problem hiding this comment.
imo this is simple enough / some of the usages are slightly different that it doesn't warrant a helper
There was a problem hiding this comment.
e.g. some have name and value where as others have value and label.
Putting into a helper makes it a little less explicit as well
| options={llmProviderDescriptor.model_configurations.map( | ||
| (modelConfiguration) => ({ | ||
| value: modelConfiguration.name, | ||
| // don't clean up names here to give admins descriptive names / handle duplicates | ||
| // like us.anthropic.claude-3-7-sonnet-20250219-v1:0 and anthropic.claude-3-7-sonnet-20250219-v1:0 | ||
| label: name, | ||
| label: modelConfiguration.name, |
| options={llmProviderDescriptor.model_configurations.map( | ||
| (modelConfiguration) => ({ | ||
| // don't clean up names here to give admins descriptive names / handle duplicates | ||
| // like us.anthropic.claude-3-7-sonnet-20250219-v1:0 and anthropic.claude-3-7-sonnet-20250219-v1:0 | ||
| name: modelConfiguration.name, | ||
| value: modelConfiguration.name, | ||
| }) | ||
| )} |
| ({ | ||
| name: modelConfiguration.name, | ||
| is_visible: visibleModels.includes(modelConfiguration.name), | ||
| max_input_tokens: null, | ||
| }) as ModelConfiguration |
There was a problem hiding this comment.
Does the ModelConfiguration type that we have defined not also require supports_image_input to be set?
There was a problem hiding this comment.
Addressed! Really should be ModelConfigurationUpsertRequest
* Fix test / display models * Address greptile comments * Increase wait time * Increase overall timeout * Move stuff to utils file * Updates
Description
Fixes https://linear.app/danswer/issue/DAN-1939/fix-playwright-testsmodel-iconsdefault-display-icons
How Has This Been Tested?
Tested with OpenAI/anthropic locally.
Backporting (check the box to trigger backport action)
Note: You have to check that the action passes, otherwise resolve the conflicts manually and tag the patches.