-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix azure #2665
Fix azure #2665
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
"""add_deployment_name_to_llmprovider | ||
|
||
Revision ID: e4334d5b33ba | ||
Revises: 46b7a812670f | ||
Create Date: 2024-10-04 09:52:34.896867 | ||
|
||
""" | ||
from alembic import op | ||
import sqlalchemy as sa | ||
|
||
|
||
# revision identifiers, used by Alembic. | ||
revision = "e4334d5b33ba" | ||
down_revision = "46b7a812670f" | ||
branch_labels = None | ||
depends_on = None | ||
|
||
|
||
def upgrade() -> None: | ||
op.add_column( | ||
"llm_provider", sa.Column("deployment_name", sa.String(), nullable=True) | ||
) | ||
|
||
|
||
def downgrade() -> None: | ||
op.drop_column("llm_provider", "deployment_name") |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Add a comment explaining when this field is used |
||
|
||
|
||
OPENAI_PROVIDER_NAME = "openai" | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Azure provider not included in _PROVIDER_TO_MODELS_MAP, this may return an empty list |
||
deployment_name_required=True, | ||
single_model_supported=True, | ||
), | ||
WellKnownLLMProviderDescriptor( | ||
name=BEDROCK_PROVIDER_NAME, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,7 @@ def test_llm_configuration( | |
api_version=test_llm_request.api_version, | ||
custom_config=test_llm_request.custom_config, | ||
) | ||
|
||
functions_with_args: list[tuple[Callable, tuple]] = [(test_llm, (llm,))] | ||
|
||
if ( | ||
|
@@ -141,6 +142,20 @@ def put_llm_provider( | |
detail=f"LLM Provider with name {llm_provider.name} already exists", | ||
) | ||
|
||
# Ensure default_model_name and fast_default_model_name are in display_model_names | ||
# This is necessary for custom models and Bedrock/Azure models | ||
if llm_provider.display_model_names is None: | ||
llm_provider.display_model_names = [] | ||
|
||
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) | ||
Comment on lines
+150
to
+157
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better to clean upsert as clean and minimal as possible |
||
|
||
try: | ||
return upsert_llm_provider( | ||
llm_provider=llm_provider, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,11 @@ | ||
import json | ||
|
||
from sqlalchemy.orm import Session | ||
|
||
from danswer.configs.app_configs import AZURE_DALLE_API_KEY | ||
from danswer.db.connector import check_connectors_exist | ||
from danswer.db.document import check_docs_exist | ||
from danswer.db.models import LLMProvider | ||
from danswer.natural_language_processing.utils import BaseTokenizer | ||
from danswer.tools.tool import Tool | ||
|
||
|
@@ -26,3 +32,18 @@ def compute_tool_tokens(tool: Tool, llm_tokenizer: BaseTokenizer) -> int: | |
|
||
def compute_all_tool_tokens(tools: list[Tool], llm_tokenizer: BaseTokenizer) -> int: | ||
return sum(compute_tool_tokens(tool, llm_tokenizer) for tool in tools) | ||
|
||
|
||
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) | ||
Comment on lines
+37
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: This function assumes OpenAI always supports image generation. Consider checking for specific OpenAI models that support DALL-E. |
||
|
||
|
||
def is_document_search_available(db_session: Session) -> bool: | ||
docs_exist = check_docs_exist(db_session) | ||
connectors_exist = check_connectors_exist(db_session) | ||
return docs_exist or connectors_exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding a comment explaining the purpose of this field