Simpler azure embedding #2751
Simpler azure embedding #2751
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 introduces support for Azure OpenAI embedding models and simplifies the overall embedding configuration process across the Danswer application. Here are the key changes:
- Added 'api_version' and 'deployment_name' fields to support Azure OpenAI embeddings in backend models and database schema.
- Introduced Azure as a new EmbeddingProvider option in shared configurations.
- Updated the EmbeddingModel class to handle Azure-specific parameters in the natural language processing module.
- Modified the CloudEmbedding class in the model server to support Azure embedding integration.
- Refactored frontend components to accommodate Azure embedding configuration, including new form fields and validation.
- Simplified the UI for different provider types, particularly in the ChangeCredentialsModal and ProviderCreationModal components.
18 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings
| Revision ID: 5d12a446f5c0 | ||
| Revises: 5d12a446f5c0 |
There was a problem hiding this comment.
syntax: Revision ID is duplicated in 'Revises' field
| `} | ||
| value={modelName} | ||
| onChange={(e: any) => setModelName(e.target.value)} | ||
| placeholder="Paste your API URL here" |
There was a problem hiding this comment.
syntax: Placeholder text for model name input is incorrect. It should be 'Enter model name' instead of 'Paste your API URL here'
| {deletionError && ( | ||
| <Callout title="Error" color="red" className="mt-4"> | ||
| {deletionError} | ||
| </Callout> | ||
| )} |
There was a problem hiding this comment.
logic: Duplicate error display for deletionError. This block should be removed as it's already handled earlier
| <Subtitle className="mt-4 font-bold text-lg mb-2"> | ||
| You can delete your configuration. | ||
| </Subtitle> | ||
| <Text className="mb-2"> | ||
| This is only possible if you have already switched to a different | ||
| embedding type! | ||
| </Text> |
There was a problem hiding this comment.
logic: The deletion section is now outside the conditional rendering block, making it available for all provider types including Azure. Verify if this is the intended behavior
| const [azureProvider, setAzureProvider] = useState< | ||
| EmbeddingDetails | undefined | ||
| >(undefined); |
There was a problem hiding this comment.
style: Consider initializing azureProvider with null instead of undefined for consistency
| }, [embeddingProviderDetails]); | ||
|
|
||
| const isAzureConfigured = !!azureProvider; |
There was a problem hiding this comment.
style: Use explicit comparison (isAzureConfigured = azureProvider !== undefined) for clarity
| // currentValues={ | ||
| // currentModel.provider_type === 'litetllm' ? currentModel : null | ||
| // } |
There was a problem hiding this comment.
style: Remove commented out code before merging
| @@ -25,7 +27,7 @@ export function LiteLLMModelForm({ | |||
| normalize: false, | |||
| query_prefix: "", | |||
| passage_prefix: "", | |||
| provider_type: "LiteLLM", | |||
| provider_type: embeddingType === "azure" ? "Azure" : "LiteLLM", | |||
There was a problem hiding this comment.
logic: The provider_type is now dynamically set based on the embeddingType prop. Verify that this change doesn't break any existing functionality that relies on the provider_type value.
Description
[Provide a brief description of the changes in this PR]
How Has This Been Tested?
[Describe the tests you ran to verify your changes]
Accepted Risk
[Any know risks or failure modes to point out to reviewers]
Related Issue(s)
[If applicable, link to the issue(s) this PR addresses]
Checklist: