Fix azure #2665
Fix azure #2665
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 adds support for Azure OpenAI deployments, focusing on the integration of the 'deployment_name' field across various components of the Danswer application.
- Added 'deployment_name' field to LLMProvider model in database and related interfaces
- Updated LLM configuration and factory to include Azure-specific deployment parameters
- Modified UI components to support Azure OpenAI deployments in assistant and LLM provider forms
- Adjusted PostgreSQL port from 5432 to 5433 in Docker Compose files to avoid potential conflicts
- Enhanced image generation tool handling, including support for Azure's DALL-E service
23 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings
| @@ -16,10 +16,12 @@ class WellKnownLLMProviderDescriptor(BaseModel): | |||
| api_base_required: bool | |||
| api_version_required: bool | |||
| custom_config_keys: list[CustomConfigKey] | None = None | |||
| single_model_supported: bool = False | |||
There was a problem hiding this comment.
style: Consider adding a comment explaining the purpose of this field
|
|
||
| llm_names: list[str] | ||
| default_model: str | None = None | ||
| default_fast_model: str | None = None | ||
| deployment_name_required: bool = False |
There was a problem hiding this comment.
style: Add a comment explaining when this field is used
| @@ -108,6 +110,8 @@ def fetch_available_well_known_llms() -> list[WellKnownLLMProviderDescriptor]: | |||
| api_version_required=True, | |||
| custom_config_keys=[], | |||
| llm_names=fetch_models_for_provider(AZURE_PROVIDER_NAME), | |||
There was a problem hiding this comment.
logic: Azure provider not included in _PROVIDER_TO_MODELS_MAP, this may return an empty list
| if llm_provider.default_model_name not in llm_provider.display_model_names: | ||
| llm_provider.display_model_names.append(llm_provider.default_model_name) | ||
|
|
||
| if ( | ||
| llm_provider.fast_default_model_name | ||
| and llm_provider.fast_default_model_name not in llm_provider.display_model_names | ||
| ): | ||
| llm_provider.display_model_names.append(llm_provider.fast_default_model_name) |
There was a problem hiding this comment.
style: This logic might be better placed in the LLMProviderUpsertRequest model's validation or in a separate function for cleaner code organization.
There was a problem hiding this comment.
Better to clean upsert as clean and minimal as possible
| def is_image_generation_available(db_session: Session) -> bool: | ||
| providers = db_session.query(LLMProvider).all() | ||
| for provider in providers: | ||
| if provider.name == "OpenAI": | ||
| return True | ||
|
|
||
| return bool(AZURE_DALLE_API_KEY) |
There was a problem hiding this comment.
logic: This function assumes OpenAI always supports image generation. Consider checking for specific OpenAI models that support DALL-E.
Description
Fixes https://linear.app/danswer/issue/DAN-733/add-proper-support-for-azure
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: