diff --git a/backend/onyx/indexing/indexing_pipeline.py b/backend/onyx/indexing/indexing_pipeline.py index e80ea94798..2e46b078ae 100644 --- a/backend/onyx/indexing/indexing_pipeline.py +++ b/backend/onyx/indexing/indexing_pipeline.py @@ -840,9 +840,9 @@ def _get_image_summarization_llm( llm = get_default_llm_with_vision() if llm is None: logger.warning( - "Image analysis is enabled but no vision-capable LLM is " - "available — images will not be summarized. Configure a " - "vision model in the admin LLM settings." + "Image analysis is enabled but no usable captioning model is " + "available — images will not be summarized. Check the captioning " + "model under Index Settings." ) return llm diff --git a/backend/onyx/llm/factory.py b/backend/onyx/llm/factory.py index a5e2300b74..dd15f83069 100644 --- a/backend/onyx/llm/factory.py +++ b/backend/onyx/llm/factory.py @@ -6,14 +6,13 @@ from sqlalchemy.orm import Session from onyx.auth.permissions import has_global_permission from onyx.configs.model_configs import GEN_AI_TEMPERATURE from onyx.db.engine.sql_engine import get_session_with_current_tenant -from onyx.db.enums import LLMModelFlowType, Permission +from onyx.db.enums import Permission from onyx.db.llm import ( can_user_access_llm_provider, fetch_default_contextual_rag_model, fetch_default_llm_model, fetch_default_vision_model, fetch_existing_llm_provider, - fetch_existing_models, fetch_model_configuration_by_id, fetch_user_group_ids, ) @@ -245,105 +244,47 @@ def get_default_llm_with_vision( temperature: float | None = None, additional_headers: dict[str, str] | None = None, ) -> LLM | None: - """Get an LLM that supports image input, with the following priority: - 1. Use the designated default vision provider if it exists and supports image input - 2. Fall back to the first LLM provider that supports image input + """The designated default vision model, or None. - Returns None if no providers exist or if no provider supports images. + There is deliberately no fallback. With no default set, image captioning + is off: picking an arbitrary image-capable model would spend money on a + model nobody chose. """ + with get_session_with_current_tenant() as db_session: + default_model = fetch_default_vision_model(db_session) + if default_model is None: + logger.warning( + "No default vision model is set — image summarization will be " + "disabled. Pick a captioning model under Index Settings." + ) + return None - def create_vision_llm(provider: LLMProviderView, model: str) -> LLM: - """Helper to create an LLM if the provider supports image input.""" + if not model_supports_image_input( + default_model.name, + default_model.llm_provider.provider, + default_model.llm_provider.deployment_name, + ): + logger.warning( + "Default vision model %s (provider=%s) does not support image " + "input — image summarization will be disabled", + default_model.name, + default_model.llm_provider.provider, + ) + return None + + logger.info( + "Using default vision model: %s (provider=%s)", + default_model.name, + default_model.llm_provider.provider, + ) return llm_from_provider( - model_name=model, - llm_provider=provider, + model_name=default_model.name, + llm_provider=LLMProviderView.from_model(default_model.llm_provider), timeout=timeout, temperature=temperature, additional_headers=additional_headers, ) - provider_map = {} - with get_session_with_current_tenant() as db_session: - # Try the default vision provider first - default_model = fetch_default_vision_model(db_session) - if default_model: - if model_supports_image_input( - default_model.name, - default_model.llm_provider.provider, - default_model.llm_provider.deployment_name, - ): - logger.info( - "Using default vision model: %s (provider=%s)", - default_model.name, - default_model.llm_provider.provider, - ) - return create_vision_llm( - LLMProviderView.from_model(default_model.llm_provider), - default_model.name, - ) - else: - logger.warning( - "Default vision model %s (provider=%s) does not support " - "image input — falling back to searching all providers", - default_model.name, - default_model.llm_provider.provider, - ) - - # Fall back to searching all providers - models = fetch_existing_models( - db_session=db_session, - flow_types=[LLMModelFlowType.VISION, LLMModelFlowType.CHAT], - ) - - if not models: - logger.warning( - "No LLM models with VISION or CHAT flow type found — " - "image summarization will be disabled" - ) - return None - - for model in models: - if model.llm_provider_id not in provider_map: - provider_map[model.llm_provider_id] = LLMProviderView.from_model( - model.llm_provider - ) - - # Search for viable vision model followed by chat models - # Sort models from VISION to CHAT priority - sorted_models = sorted( - models, - key=lambda x: ( - LLMModelFlowType.VISION in x.llm_model_flow_types, - LLMModelFlowType.CHAT in x.llm_model_flow_types, - ), - reverse=True, - ) - - for model in sorted_models: - if model_supports_image_input( - model.name, model.llm_provider.provider, model.llm_provider.deployment_name - ): - logger.info( - "Using fallback vision model: %s (provider=%s)", - model.name, - model.llm_provider.provider, - ) - return create_vision_llm( - provider_map[model.llm_provider_id], - model.name, - ) - - checked_models = [ - f"{m.name} (provider={m.llm_provider.provider})" for m in sorted_models - ] - logger.warning( - "No vision-capable model found among %d candidates: %s — " - "image summarization will be disabled", - len(sorted_models), - ", ".join(checked_models), - ) - return None - def llm_from_provider( model_name: str, diff --git a/backend/tests/integration/common_utils/managers/llm_provider.py b/backend/tests/integration/common_utils/managers/llm_provider.py index 1e2e2f1e30..ea218486e9 100644 --- a/backend/tests/integration/common_utils/managers/llm_provider.py +++ b/backend/tests/integration/common_utils/managers/llm_provider.py @@ -159,6 +159,24 @@ class LLMProviderManager: if not verify_deleted: raise ValueError(f"LLM Provider {llm_provider.id} not found") + @staticmethod + def set_default_vision( + provider_id: int, + user_performing_action: DATestUser, + model_name: str, + ) -> None: + """Point image captioning at this provider's model. Without a default + vision model, indexing skips captions entirely.""" + response = client.post( + f"{API_SERVER_URL}/admin/llm/default-vision", + json={ + "provider_id": provider_id, + "model_name": model_name, + }, + headers=user_performing_action.headers, + ) + response.raise_for_status() + @staticmethod def get_default_model( user_performing_action: DATestUser | None = None, diff --git a/backend/tests/integration/tests/image_indexing/test_indexing_images.py b/backend/tests/integration/tests/image_indexing/test_indexing_images.py index 55cfda8d07..8648b34250 100644 --- a/backend/tests/integration/tests/image_indexing/test_indexing_images.py +++ b/backend/tests/integration/tests/image_indexing/test_indexing_images.py @@ -35,10 +35,16 @@ def test_image_indexing( file_path=test_file_path, file_name=FILE_NAME, user_performing_action=admin_user ) - LLMProviderManager.create( + llm_provider = LLMProviderManager.create( name="test_llm", user_performing_action=admin_user, ) + assert llm_provider.default_model_name is not None + LLMProviderManager.set_default_vision( + llm_provider.id, + user_performing_action=admin_user, + model_name=llm_provider.default_model_name, + ) SettingsManager.update_settings( DATestSettings( @@ -133,10 +139,16 @@ def test_docx_image_indexing( user_performing_action=admin_user, ) - LLMProviderManager.create( + llm_provider = LLMProviderManager.create( name="test_llm_docx", user_performing_action=admin_user, ) + assert llm_provider.default_model_name is not None + LLMProviderManager.set_default_vision( + llm_provider.id, + user_performing_action=admin_user, + model_name=llm_provider.default_model_name, + ) SettingsManager.update_settings( DATestSettings( diff --git a/backend/tests/integration/tests/llm_provider/test_llm_provider.py b/backend/tests/integration/tests/llm_provider/test_llm_provider.py index 2c2818a56c..20f1b9abdc 100644 --- a/backend/tests/integration/tests/llm_provider/test_llm_provider.py +++ b/backend/tests/integration/tests/llm_provider/test_llm_provider.py @@ -20,6 +20,7 @@ from onyx.llm.well_known_providers.llm_provider_options import ( from onyx.server.manage.llm.models import ModelConfigurationUpsertRequest from tests.integration.common_utils.constants import API_SERVER_URL from tests.integration.common_utils.http_client import client +from tests.integration.common_utils.managers.llm_provider import LLMProviderManager from tests.integration.common_utils.managers.user import UserManager from tests.integration.common_utils.test_models import DATestUser @@ -630,7 +631,7 @@ def test_delete_default_vision_provider_clears_vision_default( ) assert vision_response.status_code == 200 vision_provider = vision_response.json() - _set_default_vision_provider(admin_user, vision_provider["id"], "gpt-4o") + LLMProviderManager.set_default_vision(vision_provider["id"], admin_user, "gpt-4o") # Verify vision default is set data = _get_providers_admin(admin_user) @@ -1375,21 +1376,6 @@ def _set_default_provider( assert response.status_code == 200 -def _set_default_vision_provider( - admin_user: DATestUser, provider_id: int, vision_model: str | None = None -) -> None: - """Utility function to set a provider as the default vision provider.""" - response = client.post( - f"{API_SERVER_URL}/admin/llm/default-vision", - json={ - "provider_id": provider_id, - "model_name": vision_model, - }, - headers=admin_user.headers, - ) - assert response.status_code == 200 - - def test_multiple_providers_default_switching( reset: None, # noqa: ARG001 ) -> None: # noqa: ARG001 @@ -1859,8 +1845,8 @@ def test_default_provider_and_vision_provider_selection( _set_default_provider(admin_user, provider_1["id"], provider_1_non_vision_model) # Step 4: Set provider 2 with a specific vision model as the default vision provider - _set_default_vision_provider( - admin_user, provider_2["id"], provider_2_vision_model_1 + LLMProviderManager.set_default_vision( + provider_2["id"], admin_user, provider_2_vision_model_1 ) # Step 5: Verify via admin endpoint @@ -2211,8 +2197,8 @@ def test_all_three_provider_types_no_mixup(reset: None) -> None: # noqa: ARG001 vision_provider = create_vision_response.json() # Set as default vision provider - _set_default_vision_provider( - admin_user, vision_provider["id"], "gpt-4-vision-preview" + LLMProviderManager.set_default_vision( + vision_provider["id"], admin_user, "gpt-4-vision-preview" ) # Step 3: Create image generation config using clone mode from regular provider diff --git a/backend/tests/unit/onyx/llm/test_vision_model_selection_logging.py b/backend/tests/unit/onyx/llm/test_vision_model_selection_logging.py index 7c8d44cc14..54ebe7f459 100644 --- a/backend/tests/unit/onyx/llm/test_vision_model_selection_logging.py +++ b/backend/tests/unit/onyx/llm/test_vision_model_selection_logging.py @@ -1,10 +1,8 @@ """ -Unit tests for vision model selection logging in get_default_llm_with_vision. +Unit tests for get_default_llm_with_vision. -Verifies that operators get clear feedback about: -1. Which vision model was selected and why -2. When the default vision model doesn't support image input -3. When no vision-capable model exists at all +Only the designated default vision model is ever used. With none set, or with +one that cannot take images, captioning is off. There is no fallback scan. """ from unittest.mock import MagicMock, patch @@ -19,13 +17,11 @@ def _make_mock_model( name: str = "gpt-4o", provider: str = "openai", provider_id: int = 1, - flow_types: list[str] | None = None, ) -> MagicMock: model = MagicMock() model.name = name model.llm_provider_id = provider_id model.llm_provider.provider = provider - model.llm_model_flow_types = flow_types or [] return model @@ -35,18 +31,19 @@ def _make_mock_model( @patch(f"{_FACTORY}.llm_from_provider") @patch(f"{_FACTORY}.LLMProviderView") @patch(f"{_FACTORY}.logger") -def test_logs_when_using_default_vision_model( +def test_uses_the_default_vision_model( mock_logger: MagicMock, mock_provider_view: MagicMock, # noqa: ARG001 - mock_llm_from: MagicMock, # noqa: ARG001 + mock_llm_from: MagicMock, mock_supports: MagicMock, # noqa: ARG001 mock_fetch_default: MagicMock, mock_session: MagicMock, # noqa: ARG001 ) -> None: mock_fetch_default.return_value = _make_mock_model(name="gpt-4o", provider="azure") - get_default_llm_with_vision() + result = get_default_llm_with_vision() + assert result is mock_llm_from.return_value mock_logger.info.assert_called_once() log_msg = mock_logger.info.call_args[0][0] assert "default vision model" in log_msg.lower() @@ -55,11 +52,11 @@ def test_logs_when_using_default_vision_model( @patch(f"{_FACTORY}.get_session_with_current_tenant") @patch(f"{_FACTORY}.fetch_default_vision_model") @patch(f"{_FACTORY}.model_supports_image_input", return_value=False) -@patch(f"{_FACTORY}.fetch_existing_models", return_value=[]) +@patch(f"{_FACTORY}.llm_from_provider") @patch(f"{_FACTORY}.logger") -def test_warns_when_default_model_lacks_vision( +def test_returns_none_when_default_model_lacks_vision( mock_logger: MagicMock, - mock_fetch_models: MagicMock, # noqa: ARG001 + mock_llm_from: MagicMock, mock_supports: MagicMock, # noqa: ARG001 mock_fetch_default: MagicMock, mock_session: MagicMock, # noqa: ARG001 @@ -71,58 +68,30 @@ def test_warns_when_default_model_lacks_vision( result = get_default_llm_with_vision() assert result is None - # Should have warned about the default model not supporting vision + mock_llm_from.assert_not_called() warning_calls = [ call for call in mock_logger.warning.call_args_list if "does not support" in str(call) ] - assert len(warning_calls) >= 1 + assert len(warning_calls) == 1 @patch(f"{_FACTORY}.get_session_with_current_tenant") @patch(f"{_FACTORY}.fetch_default_vision_model", return_value=None) -@patch(f"{_FACTORY}.fetch_existing_models", return_value=[]) +@patch(f"{_FACTORY}.llm_from_provider") @patch(f"{_FACTORY}.logger") -def test_warns_when_no_models_exist( +def test_returns_none_when_no_default_is_set( mock_logger: MagicMock, - mock_fetch_models: MagicMock, # noqa: ARG001 + mock_llm_from: MagicMock, mock_fetch_default: MagicMock, # noqa: ARG001 mock_session: MagicMock, # noqa: ARG001 ) -> None: + """No default means no captioning. Nothing else is tried.""" result = get_default_llm_with_vision() assert result is None + mock_llm_from.assert_not_called() mock_logger.warning.assert_called_once() log_msg = mock_logger.warning.call_args[0][0] - assert "no llm models" in log_msg.lower() - - -@patch(f"{_FACTORY}.get_session_with_current_tenant") -@patch(f"{_FACTORY}.fetch_default_vision_model", return_value=None) -@patch(f"{_FACTORY}.fetch_existing_models") -@patch(f"{_FACTORY}.model_supports_image_input", return_value=False) -@patch(f"{_FACTORY}.LLMProviderView") -@patch(f"{_FACTORY}.logger") -def test_warns_when_no_model_supports_vision( - mock_logger: MagicMock, - mock_provider_view: MagicMock, # noqa: ARG001 - mock_supports: MagicMock, # noqa: ARG001 - mock_fetch_models: MagicMock, - mock_fetch_default: MagicMock, # noqa: ARG001 - mock_session: MagicMock, # noqa: ARG001 -) -> None: - mock_fetch_models.return_value = [ - _make_mock_model(name="text-model-1", provider="openai"), - _make_mock_model(name="text-model-2", provider="azure", provider_id=2), - ] - - result = get_default_llm_with_vision() - - assert result is None - warning_calls = [ - call - for call in mock_logger.warning.call_args_list - if "no vision-capable model" in str(call).lower() - ] - assert len(warning_calls) == 1 + assert "no default vision model" in log_msg.lower()