From cc7970c20abb6534c38e99e2cb1460c03b683d3c Mon Sep 17 00:00:00 2001 From: Nitjsefnie Date: Fri, 17 Jul 2026 05:29:51 +0000 Subject: [PATCH] feat(providers): validate retriever settings keys to catch typos early Retriever providers accept a free-form `settings` dict, so a typo like `collectoin_name` was either silently swallowed by a provider's `**kwargs` (qdrant) or raised an opaque `TypeError` deep in a constructor (chroma) -- and only at runtime. Add a validation helper that, at provider-build time, warns on unknown setting keys and suggests the closest known key (`difflib`), then drops the key. This mirrors how unknown/misspelled metric names are surfaced in `config.loader` (warn + drop, not hard-fail). Providers opt in by declaring a `SETTINGS_KEYS` set; chroma and qdrant declare theirs, derived from their actual constructor parameters. Co-Authored-By: Claude Opus 4.8 --- openagent_eval/providers/factory.py | 12 +++ .../providers/retrievers/_validation.py | 60 ++++++++++++ openagent_eval/providers/retrievers/chroma.py | 7 ++ openagent_eval/providers/retrievers/qdrant.py | 7 ++ .../test_retriever_settings_validation.py | 97 +++++++++++++++++++ 5 files changed, 183 insertions(+) create mode 100644 openagent_eval/providers/retrievers/_validation.py create mode 100644 tests/unit/test_providers/test_retriever_settings_validation.py diff --git a/openagent_eval/providers/factory.py b/openagent_eval/providers/factory.py index 1bc19f6..02f48ca 100644 --- a/openagent_eval/providers/factory.py +++ b/openagent_eval/providers/factory.py @@ -22,6 +22,9 @@ from openagent_eval.providers.base.retriever import Retriever from openagent_eval.providers.embedders.base import Embedder from openagent_eval.providers.models import Document, LLMResponse, TokenUsage +from openagent_eval.providers.retrievers._validation import ( + validate_retriever_settings, +) # --------------------------------------------------------------------------- # # LLM provider registry # @@ -115,6 +118,15 @@ def get_retriever(config: RetrieverConfig) -> Retriever: retriever_cls = _resolve(_RETRIEVER_PROVIDERS[key]) settings = dict(config.settings or {}) + # Catch typos/unknown keys in the free-form settings dict early, before the + # values reach a provider constructor (which may silently swallow unknown + # kwargs). Providers opt in by declaring ``SETTINGS_KEYS``. + allowed_keys = getattr(retriever_cls, "SETTINGS_KEYS", None) + if allowed_keys is not None: + unknown = validate_retriever_settings(key, settings, allowed_keys) + for bad_key in unknown: + settings.pop(bad_key, None) + embedder = None if config.embedder is not None: embedder = get_embedder(config.embedder) diff --git a/openagent_eval/providers/retrievers/_validation.py b/openagent_eval/providers/retrievers/_validation.py new file mode 100644 index 0000000..0cb8d51 --- /dev/null +++ b/openagent_eval/providers/retrievers/_validation.py @@ -0,0 +1,60 @@ +"""Early validation for retriever ``settings`` keys. + +Retriever providers accept a free-form ``settings`` dict from user config +(``retriever.settings`` in the YAML). Without validation, a typo such as +``collectoin_name`` is either swallowed by a provider's ``**kwargs`` or blows +up with an opaque ``TypeError`` deep inside a constructor -- and only at +runtime. This module surfaces unknown keys early, at provider-build time, with +a "did you mean" suggestion. + +The behaviour mirrors how unknown/misspelled metric names are handled in +:mod:`openagent_eval.config.loader` (see the metrics block around +``loader.py:139``): the offending keys are dropped and a ``logging.warning`` is +emitted naming them, rather than hard-failing. This keeps a single stray key +from aborting an otherwise-valid evaluation run. +""" + +from __future__ import annotations + +import difflib +import logging +from typing import TYPE_CHECKING, Any + +if TYPE_CHECKING: + from collections.abc import Iterable, Mapping + +logger = logging.getLogger(__name__) + + +def validate_retriever_settings( + provider_name: str, + settings: Mapping[str, Any], + allowed_keys: Iterable[str], +) -> list[str]: + """Warn about unknown ``settings`` keys for a retriever provider. + + Args: + provider_name: Name of the retriever provider (for the log message). + settings: The user-supplied settings mapping to check. + allowed_keys: The setting keys the provider actually understands. + + Returns: + The list of unknown keys found (in the order they appear in + ``settings``). Empty when every key is recognised. + """ + allowed = set(allowed_keys) + unknown = [key for key in settings if key not in allowed] + + for key in unknown: + match = difflib.get_close_matches(key, allowed, n=1) + suggestion = f" Did you mean '{match[0]}'?" if match else "" + logger.warning( + "Unknown setting '%s' for retriever '%s'; it will be ignored.%s " + "Known settings: %s", + key, + provider_name, + suggestion, + ", ".join(sorted(allowed)), + ) + + return unknown diff --git a/openagent_eval/providers/retrievers/chroma.py b/openagent_eval/providers/retrievers/chroma.py index 46abce6..1294e83 100644 --- a/openagent_eval/providers/retrievers/chroma.py +++ b/openagent_eval/providers/retrievers/chroma.py @@ -67,6 +67,13 @@ class ChromaRetriever(Retriever): name: str = "chroma" description: str = "ChromaDB vector-based document retriever" + #: Setting keys accepted via ``retriever.settings``. Used by the provider + #: factory to catch typos/unknown keys early (mirrors the constructor's + #: parameters below). + SETTINGS_KEYS: frozenset[str] = frozenset( + {"collection_name", "persist_directory", "distance_fn"} + ) + def __init__( self, collection_name: str, diff --git a/openagent_eval/providers/retrievers/qdrant.py b/openagent_eval/providers/retrievers/qdrant.py index dcf1c9a..952ecbc 100644 --- a/openagent_eval/providers/retrievers/qdrant.py +++ b/openagent_eval/providers/retrievers/qdrant.py @@ -25,6 +25,13 @@ class QdrantRetriever(Retriever): name: str = "qdrant" description: str = "Qdrant vector database retriever" + #: Setting keys accepted via ``retriever.settings``. Used by the provider + #: factory to catch typos/unknown keys early. Note the constructor swallows + #: unknown kwargs via ``**_``, so without this a typo would pass silently. + SETTINGS_KEYS: frozenset[str] = frozenset( + {"collection_name", "embedder", "url", "api_key", "prefer_grpc", "distance"} + ) + def __init__( self, collection_name: str, diff --git a/tests/unit/test_providers/test_retriever_settings_validation.py b/tests/unit/test_providers/test_retriever_settings_validation.py new file mode 100644 index 0000000..9cf501a --- /dev/null +++ b/tests/unit/test_providers/test_retriever_settings_validation.py @@ -0,0 +1,97 @@ +"""Tests for early validation of retriever ``settings`` keys (issue #116).""" + +from __future__ import annotations + +import logging + +import pytest + +from openagent_eval.config.models import RetrieverConfig +from openagent_eval.exceptions.provider import ProviderConnectionError +from openagent_eval.providers.factory import get_retriever +from openagent_eval.providers.retrievers._validation import ( + validate_retriever_settings, +) + +CHROMA_KEYS = {"collection_name", "persist_directory", "distance_fn"} + + +class TestValidateRetrieverSettings: + """Unit tests for the validation helper in isolation.""" + + def test_unknown_key_is_reported_and_named(self, caplog): + with caplog.at_level(logging.WARNING): + unknown = validate_retriever_settings( + "chroma", {"collectoin_name": "docs"}, CHROMA_KEYS + ) + + assert unknown == ["collectoin_name"] + assert "collectoin_name" in caplog.text + # The suggestion points at the closest real key. + assert "Did you mean 'collection_name'?" in caplog.text + + def test_all_known_keys_pass(self, caplog): + with caplog.at_level(logging.WARNING): + unknown = validate_retriever_settings( + "chroma", + { + "collection_name": "docs", + "persist_directory": "./db", + "distance_fn": "cosine", + }, + CHROMA_KEYS, + ) + + assert unknown == [] + assert "Unknown setting" not in caplog.text + + def test_unknown_key_without_close_match_has_no_suggestion(self, caplog): + with caplog.at_level(logging.WARNING): + unknown = validate_retriever_settings("chroma", {"zzzzzz": 1}, CHROMA_KEYS) + + assert unknown == ["zzzzzz"] + assert "zzzzzz" in caplog.text + assert "Did you mean" not in caplog.text + + +class TestFactorySettingsValidation: + """The factory wires validation into ``get_retriever`` before build.""" + + def test_typo_in_qdrant_settings_warns_with_suggestion(self, caplog): + cfg = RetrieverConfig( + provider="qdrant", + settings={"collection_name": "docs", "collectoin_name": "docs"}, + ) + # qdrant still errors (no embedder); validation runs first. + with ( + caplog.at_level(logging.WARNING), + pytest.raises(ProviderConnectionError), + ): + get_retriever(cfg) + + assert "collectoin_name" in caplog.text + assert "Did you mean 'collection_name'?" in caplog.text + + def test_known_qdrant_settings_emit_no_unknown_warning(self, caplog): + cfg = RetrieverConfig( + provider="qdrant", + settings={ + "collection_name": "docs", + "url": "http://localhost:6333", + "prefer_grpc": True, + "distance": "Cosine", + }, + ) + with ( + caplog.at_level(logging.WARNING), + pytest.raises(ProviderConnectionError), + ): + get_retriever(cfg) + + assert "Unknown setting" not in caplog.text + + def test_chroma_declares_expected_settings_keys(self): + pytest.importorskip("chromadb") + from openagent_eval.providers.retrievers.chroma import ChromaRetriever + + assert set(ChromaRetriever.SETTINGS_KEYS) == CHROMA_KEYS