From f6a910dec91153af424bec1d713a14d35ccb20dd Mon Sep 17 00:00:00 2001 From: Prax Lannister Date: Sun, 5 Jul 2026 07:47:25 +0530 Subject: [PATCH] fix(models): normalize api_base->base_url for ChatOpenAI + warn on unknown config keys (#3790) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ModelConfig is `extra="allow"`, so a config key like `api_base` (which config.example.yaml uses for other model classes, e.g. PatchedChatDeepSeek and Moonshot) gets copied onto a `langchain_openai:ChatOpenAI` model by users. LangChain's OpenAI client does not reject the unknown kwarg — it transfers it into `model_kwargs` (with a UserWarning), which is then spread into every `Completions.create()` call and rejected by the OpenAI SDK at REQUEST time with an opaque `unexpected keyword argument 'api_base'` error. The endpoint override is also silently dropped, so the model targets the wrong base URL. Changes in factory.py, mirroring the existing OpenAI-compatible helpers: - `_normalize_openai_base_url`: renames `api_base` -> `base_url` for the OpenAI-compatible family (ChatOpenAI + PatchedChatOpenAI); when an endpoint key is already present, drops the alias with a warning. Runs before the stream_usage/stream_chunk_timeout heuristics so they see the canonical key. - `_warn_unknown_model_settings`: scoped to the same OpenAI-compatible family (where the model_kwargs divert-and-crash actually happens and the field/alias set is accurate), logs an actionable warning for unrecognized config keys. - The three OpenAI-compatible helpers now share `_OPENAI_COMPAT_USE_PATHS` instead of disagreeing on the literal class string. Adds a note to docs/CONFIGURATION.md and 9 tests covering normalization (ChatOpenAI + PatchedChatOpenAI, both-set precedence for base_url and openai_api_base, non-OpenAI class untouched, no-op when unset) and the unknown-key warning (fires on a typo, silent on a clean config and on non-OpenAI providers like ChatAnthropic). --- backend/docs/CONFIGURATION.md | 2 + .../harness/deerflow/models/factory.py | 92 ++++++++++- backend/tests/test_model_factory.py | 147 ++++++++++++++++++ 3 files changed, 239 insertions(+), 2 deletions(-) diff --git a/backend/docs/CONFIGURATION.md b/backend/docs/CONFIGURATION.md index b7d4a5c5e..5c4c875b3 100644 --- a/backend/docs/CONFIGURATION.md +++ b/backend/docs/CONFIGURATION.md @@ -81,6 +81,8 @@ models: For OpenAI-compatible gateways (for example Novita or OpenRouter), keep using `langchain_openai:ChatOpenAI` and set `base_url`: +> **Note:** for `langchain_openai:ChatOpenAI` the endpoint override key is `base_url` (not `api_base`). If you write `api_base` it is automatically normalized to `base_url`, and unrecognized keys are logged with a warning at model-build time. Some other model classes (e.g. `PatchedChatDeepSeek`) do use `api_base` — match the key to the class you configured. + ```yaml models: - name: novita-deepseek-v3.2 diff --git a/backend/packages/harness/deerflow/models/factory.py b/backend/packages/harness/deerflow/models/factory.py index dce3ca1ba..9eeb7386c 100644 --- a/backend/packages/harness/deerflow/models/factory.py +++ b/backend/packages/harness/deerflow/models/factory.py @@ -31,6 +31,14 @@ def _vllm_disable_chat_template_kwargs(chat_template_kwargs: dict) -> dict: return disable_kwargs +# OpenAI-compatible model classes whose constructor takes ``base_url`` (not ``api_base``) +# and to which the OpenAI-specific defaults below apply. +_OPENAI_COMPAT_USE_PATHS = ( + "langchain_openai:ChatOpenAI", + "deerflow.models.patched_openai:PatchedChatOpenAI", +) + + def _enable_stream_usage_by_default(model_use_path: str, model_settings_from_config: dict) -> None: """Enable stream usage for OpenAI-compatible models unless explicitly configured. @@ -39,7 +47,7 @@ def _enable_stream_usage_by_default(model_use_path: str, model_settings_from_con gateways, so token usage tracking would otherwise stay empty and the TokenUsageMiddleware would have nothing to log. """ - if model_use_path != "langchain_openai:ChatOpenAI": + if model_use_path not in _OPENAI_COMPAT_USE_PATHS: return if "stream_usage" in model_settings_from_config: return @@ -47,6 +55,81 @@ def _enable_stream_usage_by_default(model_use_path: str, model_settings_from_con model_settings_from_config["stream_usage"] = True +def _normalize_openai_base_url(model_use_path: str, model_settings_from_config: dict) -> None: + """Map the common ``api_base`` alias to ``base_url`` for OpenAI-compatible clients. + + ``langchain_openai:ChatOpenAI`` (and the ``PatchedChatOpenAI`` subclass) accept the OpenAI + endpoint override as ``base_url`` (with ``openai_api_base`` as a legacy alias). Several + providers in ``config.example.yaml`` use ``api_base`` for *other* model classes, so users + frequently copy ``api_base`` onto a ChatOpenAI model by mistake. Because ``ModelConfig`` is + ``extra="allow"``, the bad key is not caught at config-load time — it is forwarded to the + constructor, which does not reject it but transfers it into ``model_kwargs``; that is then + spread into every ``Completions.create()`` call and rejected by the OpenAI SDK at *request* + time with an opaque ``unexpected keyword argument 'api_base'`` error (and the endpoint override + is silently dropped). Rename it here so the model works as the user intended. + """ + if model_use_path not in _OPENAI_COMPAT_USE_PATHS: + return + if "api_base" not in model_settings_from_config: + return + if "base_url" in model_settings_from_config or "openai_api_base" in model_settings_from_config: + # Canonical key already present; drop the alias to avoid a duplicate-intent kwarg. + model_settings_from_config.pop("api_base", None) + logger.warning("Model config sets both an endpoint key (base_url/openai_api_base) and 'api_base'; using the former and ignoring 'api_base'.") + return + model_settings_from_config["base_url"] = model_settings_from_config.pop("api_base") + logger.debug("Normalized model config key 'api_base' -> 'base_url' for OpenAI-compatible client.") + + +def _warn_unknown_model_settings(model_use_path: str, model_class, model_name: str, model_settings_from_config: dict) -> None: + """Warn about config keys the OpenAI client will silently divert into ``model_kwargs``. + + ``ModelConfig`` is ``extra="allow"``, so a typo'd key (e.g. ``maxx_tokens``) is not caught at + config-load time. LangChain's OpenAI client does not reject an unknown constructor kwarg — it + emits a ``UserWarning`` and transfers the key into ``model_kwargs``, which is then spread into + every ``Completions.create()`` call and rejected by the OpenAI SDK at *request* time with an + opaque ``unexpected keyword argument`` error that is very hard to trace back to a config typo. + + This turns that latent failure into an explicit, actionable log line at model-build time. It is + **scoped to the OpenAI-compatible family** (``_OPENAI_COMPAT_USE_PATHS``) — that is where the + ``model_kwargs`` divert-and-crash behavior occurs and where the known field/alias set is + accurate. Other providers (e.g. ``ChatAnthropic``) route extra kwargs differently and would + false-positive against this allow-list, so they are intentionally left alone. Best-effort and + non-fatal: it only fires when the class exposes a pydantic ``model_fields`` schema, treats both + field names and their aliases as valid, and allow-lists the standard passthrough kwargs the + factory injects and the OpenAI client accepts. + """ + if model_use_path not in _OPENAI_COMPAT_USE_PATHS: + return + known = getattr(model_class, "model_fields", None) + if not known: + return + valid_names = set(known.keys()) + for field in known.values(): + alias = getattr(field, "alias", None) + if alias: + valid_names.add(alias) + # Standard kwargs the factory injects or the OpenAI client accepts beyond declared fields. + valid_names |= { + "model", + "model_kwargs", + "extra_body", + "default_headers", + "default_query", + "stream_usage", + "stream_chunk_timeout", + "reasoning_effort", + } + unknown = sorted(k for k in model_settings_from_config if k not in valid_names) + if unknown: + logger.warning( + "Model '%s' (%s): config key(s) %s are not recognized parameters of the model class and will be forwarded as-is; this may raise at request time. Check for typos (e.g. 'maxx_tokens' -> 'max_tokens').", + model_name, + getattr(model_class, "__name__", "?"), + unknown, + ) + + # Default chunk-gap budget for OpenAI-compatible streaming responses. # # langchain-openai raises ``StreamChunkTimeoutError`` after this many seconds @@ -71,7 +154,7 @@ def _apply_stream_chunk_timeout_default(model_use_path: str, model_settings_from * Non-OpenAI path: drop the key so it is never forwarded to an incompatible constructor (which would raise ``TypeError: unexpected keyword argument``). """ - if model_use_path != "langchain_openai:ChatOpenAI": + if model_use_path not in _OPENAI_COMPAT_USE_PATHS: model_settings_from_config.pop("stream_chunk_timeout", None) return if "stream_chunk_timeout" in model_settings_from_config: @@ -163,6 +246,9 @@ def create_chat_model(name: str | None = None, thinking_enabled: bool = False, * kwargs.pop("reasoning_effort", None) model_settings_from_config.pop("reasoning_effort", None) + # Normalize the api_base -> base_url alias FIRST, so the downstream OpenAI-compatible + # heuristics (stream_usage / stream_chunk_timeout) see the canonical endpoint key. + _normalize_openai_base_url(model_config.use, model_settings_from_config) _enable_stream_usage_by_default(model_config.use, model_settings_from_config) _apply_stream_chunk_timeout_default(model_config.use, model_settings_from_config) @@ -197,6 +283,8 @@ def create_chat_model(name: str | None = None, thinking_enabled: bool = False, * if "stream_usage" in getattr(model_class, "model_fields", {}): model_settings_from_config["stream_usage"] = True + _warn_unknown_model_settings(model_config.use, model_class, name, model_settings_from_config) + model_instance = model_class(**kwargs, **model_settings_from_config) if attach_tracing: diff --git a/backend/tests/test_model_factory.py b/backend/tests/test_model_factory.py index 706749341..2efd780f2 100644 --- a/backend/tests/test_model_factory.py +++ b/backend/tests/test_model_factory.py @@ -1199,3 +1199,150 @@ def test_stream_chunk_timeout_popped_for_non_openai_provider_when_user_set_it(mo factory_module.create_chat_model(name="anthropic-with-stray-timeout") assert "stream_chunk_timeout" not in captured + + +# --------------------------------------------------------------------------- +# OpenAI base_url normalization + unknown-key warning +# (regression: api_base copied onto a ChatOpenAI model crashed at request time) +# --------------------------------------------------------------------------- + + +def _make_model_with_extras(name="extra-model", *, use="langchain_openai:ChatOpenAI", **extras): + """Build a ModelConfig with arbitrary extra keys (ModelConfig is extra='allow').""" + return ModelConfig( + name=name, + display_name=name, + description=None, + use=use, + model=name, + supports_thinking=False, + supports_reasoning_effort=False, + supports_vision=False, + **extras, + ) + + +def test_api_base_normalized_to_base_url_for_chatopenai(monkeypatch): + """A config that sets api_base on a ChatOpenAI model should reach the constructor as base_url.""" + cfg = _make_app_config([_make_model_with_extras("oai", api_base="http://localhost:4001/v1")]) + _patch_factory(monkeypatch, cfg) + + FakeChatModel.captured_kwargs = {} + factory_module.create_chat_model(name="oai") + + assert FakeChatModel.captured_kwargs.get("base_url") == "http://localhost:4001/v1" + assert "api_base" not in FakeChatModel.captured_kwargs + + +def test_base_url_takes_precedence_when_both_set(monkeypatch): + """When both base_url and api_base are present, base_url wins and api_base is dropped.""" + cfg = _make_app_config([_make_model_with_extras("oai", base_url="http://canonical/v1", api_base="http://alias/v1")]) + _patch_factory(monkeypatch, cfg) + + FakeChatModel.captured_kwargs = {} + factory_module.create_chat_model(name="oai") + + assert FakeChatModel.captured_kwargs.get("base_url") == "http://canonical/v1" + assert "api_base" not in FakeChatModel.captured_kwargs + + +def test_api_base_not_normalized_for_non_openai_class(monkeypatch): + """api_base must be left untouched for model classes that are not the OpenAI-compatible family.""" + cfg = _make_app_config([_make_model_with_extras("ds", use="deerflow.models.patched_deepseek:PatchedChatDeepSeek", api_base="http://ds/v3")]) + _patch_factory(monkeypatch, cfg) + + FakeChatModel.captured_kwargs = {} + factory_module.create_chat_model(name="ds") + + # PatchedChatDeepSeek legitimately takes api_base — it must pass through unchanged. + assert FakeChatModel.captured_kwargs.get("api_base") == "http://ds/v3" + assert "base_url" not in FakeChatModel.captured_kwargs + + +def test_no_op_when_neither_base_url_nor_api_base(monkeypatch): + """Normalization is a no-op when the model declares no endpoint override.""" + cfg = _make_app_config([_make_model("plain")]) + _patch_factory(monkeypatch, cfg) + + FakeChatModel.captured_kwargs = {} + factory_module.create_chat_model(name="plain") + + assert "base_url" not in FakeChatModel.captured_kwargs + assert "api_base" not in FakeChatModel.captured_kwargs + + +def test_unknown_config_key_emits_warning(monkeypatch, caplog): + """A typo'd config key should produce a heads-up warning naming the offending key. + + Uses the real ChatOpenAI class (not the stub) so the field/alias schema is realistic — the + warning's whole value is that it matches what LangChain will actually divert to model_kwargs. + """ + import logging + + from langchain_openai import ChatOpenAI + + cfg = _make_app_config([_make_model_with_extras("typo", api_key="sk-test", definitely_not_a_real_kwarg=True)]) + _patch_factory(monkeypatch, cfg, model_class=ChatOpenAI) + + with caplog.at_level(logging.WARNING, logger=factory_module.__name__): + factory_module.create_chat_model(name="typo") + + assert any("definitely_not_a_real_kwarg" in rec.message for rec in caplog.records) + + +def test_known_config_keys_emit_no_warning(monkeypatch, caplog): + """Recognized keys (model, base_url alias, max_tokens, factory-injected kwargs) must not warn.""" + import logging + + from langchain_openai import ChatOpenAI + + cfg = _make_app_config([_make_model_with_extras("clean", api_key="sk-test", base_url="http://ok/v1", max_tokens=100)]) + _patch_factory(monkeypatch, cfg, model_class=ChatOpenAI) + + with caplog.at_level(logging.WARNING, logger=factory_module.__name__): + factory_module.create_chat_model(name="clean") + + assert not any("not recognized parameters" in rec.message for rec in caplog.records) + + +def test_api_base_normalized_for_patched_chatopenai(monkeypatch): + """The PatchedChatOpenAI subclass is in the OpenAI-compatible family and must normalize too.""" + cfg = _make_app_config([_make_model_with_extras("patched", use="deerflow.models.patched_openai:PatchedChatOpenAI", api_base="http://localhost:4001/v1")]) + _patch_factory(monkeypatch, cfg) + + FakeChatModel.captured_kwargs = {} + factory_module.create_chat_model(name="patched") + + assert FakeChatModel.captured_kwargs.get("base_url") == "http://localhost:4001/v1" + assert "api_base" not in FakeChatModel.captured_kwargs + + +def test_api_base_dropped_when_openai_api_base_field_name_set(monkeypatch): + """If the field-name openai_api_base is set alongside api_base, the alias is dropped (no dup).""" + cfg = _make_app_config([_make_model_with_extras("oai", openai_api_base="http://canonical/v1", api_base="http://alias/v1")]) + _patch_factory(monkeypatch, cfg) + + FakeChatModel.captured_kwargs = {} + factory_module.create_chat_model(name="oai") + + assert FakeChatModel.captured_kwargs.get("openai_api_base") == "http://canonical/v1" + assert "api_base" not in FakeChatModel.captured_kwargs + assert "base_url" not in FakeChatModel.captured_kwargs + + +def test_no_unknown_key_warning_for_non_openai_class(monkeypatch, caplog): + """The unknown-key warning is scoped to the OpenAI family; other providers must not false-positive. + + Regression: a ChatAnthropic model with a legit kwarg like frequency_penalty (which LangChain + routes into model_kwargs for that provider) previously tripped the 'not recognized' warning. + """ + import logging + + cfg = _make_app_config([_make_model_with_extras("anthropic", use="langchain_anthropic:ChatAnthropic", frequency_penalty=0.5)]) + _patch_factory(monkeypatch, cfg) + + FakeChatModel.captured_kwargs = {} + with caplog.at_level(logging.WARNING, logger=factory_module.__name__): + factory_module.create_chat_model(name="anthropic") + + assert not any("not recognized parameters" in rec.message for rec in caplog.records)