From e2816eaa9784e96e2921e05e7144239ba5d56cd5 Mon Sep 17 00:00:00 2001 From: Aari Date: Tue, 14 Jul 2026 09:32:53 +0800 Subject: [PATCH] fix(models): scope the OpenAI-compat rules to BaseChatOpenAI, not a class-path allowlist (#4146) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(models): scope the OpenAI-compat rules to BaseChatOpenAI, not a class-path allowlist * address review: drop redundant stream_usage helper, close test matrix - Remove _enable_stream_usage_by_default and its now-unused _OPENAI_COMPAT_USE_PATHS tuple. The class-field stream_usage fallback already sets stream_usage=True for every BaseChatOpenAI subclass (they all declare the field), so the helper's use-path allowlist gated nothing real — verified a no-op in prod, and the two stream_usage tests stay green on main with the helper present. Those tests used a BaseChatModel stub that does not declare the field; point them at a real ChatOpenAI capturing class so they exercise the fallback they now depend on. - Give the non-OpenAI normalization-skip test an actual api_base value so it exercises the skip path (api_base passed through verbatim, never rewritten to base_url). - Add an Unreleased CHANGELOG entry for the api_base behavior change on the five affected subclasses. --------- Co-authored-by: Willem Jiang --- CHANGELOG.md | 13 ++ .../harness/deerflow/models/factory.py | 79 +++---- backend/tests/test_model_factory.py | 199 +++++++++++++----- 3 files changed, 198 insertions(+), 93 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f14e0c107..dcab9ec13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,18 @@ All notable changes to DeerFlow are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed + +- **models:** Honor `api_base` on every `BaseChatOpenAI` subclass (`VllmChatModel`, + `MindIEChatModel`, `PatchedChatMiMo`, `PatchedChatStepFun`, `PatchedChatMiniMax`), + not just `ChatOpenAI` / `PatchedChatOpenAI`. Those five previously dropped the + configured endpoint silently and then failed every request with an opaque + `unexpected keyword argument 'api_base'`; the unknown-config-key warning was + disabled for them as well. Both now gate on `issubclass(BaseChatOpenAI)`. ([#4146]) + + ## [2.0.0] — 2026-06-15 DeerFlow 2.0 is a ground-up rewrite around a "super agent" harness with @@ -518,3 +530,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#3654]: https://github.com/bytedance/deer-flow/pull/3654 [#3657]: https://github.com/bytedance/deer-flow/pull/3657 [#3658]: https://github.com/bytedance/deer-flow/pull/3658 +[#4146]: https://github.com/bytedance/deer-flow/pull/4146 diff --git a/backend/packages/harness/deerflow/models/factory.py b/backend/packages/harness/deerflow/models/factory.py index aa9bcd1de..d06eed687 100644 --- a/backend/packages/harness/deerflow/models/factory.py +++ b/backend/packages/harness/deerflow/models/factory.py @@ -32,44 +32,35 @@ 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 _declares_api_base(model_class: type) -> bool: + """Whether *model_class* declares ``api_base`` as its own constructor field. - -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. - - LangChain only auto-enables ``stream_usage`` for OpenAI models when no custom - base URL or client is configured. DeerFlow frequently uses OpenAI-compatible - gateways, so token usage tracking would otherwise stay empty and the - TokenUsageMiddleware would have nothing to log. + ``langchain_deepseek:ChatDeepSeek`` (and therefore ``PatchedChatDeepSeek``) does, so for it + ``api_base`` is the canonical endpoint key and must be passed through untouched. Every other + ``BaseChatOpenAI`` subclass inherits only ``openai_api_base`` (alias ``base_url``). """ - if model_use_path not in _OPENAI_COMPAT_USE_PATHS: - return - if "stream_usage" in model_settings_from_config: - return - if "base_url" in model_settings_from_config or "openai_api_base" in model_settings_from_config: - model_settings_from_config["stream_usage"] = True + return "api_base" in getattr(model_class, "model_fields", {}) -def _normalize_openai_base_url(model_use_path: str, model_settings_from_config: dict) -> None: +def _normalize_openai_base_url(model_class: type, 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. + ``BaseChatOpenAI`` subclasses 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 such a 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. + + Gated on ``issubclass(model_class, BaseChatOpenAI)`` rather than a class-path allowlist, so any + OpenAI-compatible subclass is covered automatically — the divert-and-crash behaviour is a + property of the base class, not of the two paths that used to be listed. Classes that declare + ``api_base`` themselves are skipped: there the key is canonical, not a typo. """ - if model_use_path not in _OPENAI_COMPAT_USE_PATHS: + if not issubclass(model_class, BaseChatOpenAI) or _declares_api_base(model_class): return if "api_base" not in model_settings_from_config: return @@ -82,7 +73,7 @@ def _normalize_openai_base_url(model_use_path: str, model_settings_from_config: 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: +def _warn_unknown_model_settings(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 @@ -92,15 +83,16 @@ def _warn_unknown_model_settings(model_use_path: str, model_class, model_name: s 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. + **scoped to the OpenAI-compatible family** — that is where the ``model_kwargs`` + divert-and-crash behavior occurs and where the known field/alias set is accurate. The family is + ``issubclass(model_class, BaseChatOpenAI)``: the divert is implemented in that base class, so + every subclass inherits it. 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: + if not issubclass(model_class, BaseChatOpenAI): return known = getattr(model_class, "model_fields", None) if not known: @@ -264,9 +256,8 @@ def create_chat_model(name: str | None = None, thinking_enabled: bool = False, * 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) + # heuristics (stream_usage default below / stream_chunk_timeout) see the canonical endpoint key. + _normalize_openai_base_url(model_class, model_settings_from_config) _apply_stream_chunk_timeout_default(model_class, model_settings_from_config) # For Codex Responses API models: map thinking mode to reasoning_effort @@ -300,7 +291,7 @@ 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) + _warn_unknown_model_settings(model_class, name, model_settings_from_config) model_instance = model_class(**kwargs, **model_settings_from_config) diff --git a/backend/tests/test_model_factory.py b/backend/tests/test_model_factory.py index 7415465f1..ca2adc738 100644 --- a/backend/tests/test_model_factory.py +++ b/backend/tests/test_model_factory.py @@ -616,17 +616,13 @@ def test_openai_compatible_provider_passes_base_url(monkeypatch): supports_vision=True, supports_thinking=False, ) + from langchain_openai import ChatOpenAI + cfg = _make_app_config([model]) - _patch_factory(monkeypatch, cfg) - captured: dict = {} - - class CapturingModel(FakeChatModel): - def __init__(self, **kwargs): - captured.update(kwargs) - BaseChatModel.__init__(self, **kwargs) - - monkeypatch.setattr(factory_module, "resolve_class", lambda path, base: CapturingModel) + # Real ChatOpenAI: it declares the stream_usage field, so the factory's + # class-field default path (not a use-path allowlist) enables it. + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(ChatOpenAI, captured)) factory_module.create_chat_model(name="minimax-m3") @@ -682,17 +678,11 @@ def test_openai_compatible_provider_enables_stream_usage_for_openai_api_base(mon supports_vision=False, supports_thinking=False, ) + from langchain_openai import ChatOpenAI + cfg = _make_app_config([model]) - _patch_factory(monkeypatch, cfg) - captured: dict = {} - - class CapturingModel(FakeChatModel): - def __init__(self, **kwargs): - captured.update(kwargs) - BaseChatModel.__init__(self, **kwargs) - - monkeypatch.setattr(factory_module, "resolve_class", lambda path, base: CapturingModel) + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(ChatOpenAI, captured)) factory_module.create_chat_model(name="openai-compatible") @@ -1313,39 +1303,50 @@ def _make_model_with_extras(name="extra-model", *, use="langchain_openai:ChatOpe 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) + from langchain_openai import ChatOpenAI + + cfg = _make_app_config([_make_model_with_extras("oai", api_base="http://localhost:4001/v1")]) + captured: dict = {} + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(ChatOpenAI, captured)) - 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 + assert captured.get("base_url") == "http://localhost:4001/v1" + assert "api_base" not in captured 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) + from langchain_openai import ChatOpenAI + + cfg = _make_app_config([_make_model_with_extras("oai", base_url="http://canonical/v1", api_base="http://alias/v1")]) + captured: dict = {} + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(ChatOpenAI, captured)) - 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 + assert captured.get("base_url") == "http://canonical/v1" + assert "api_base" not in captured -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.""" +def test_api_base_preserved_for_provider_that_declares_it(monkeypatch): + """PatchedChatDeepSeek declares ``api_base`` as its own field, so the key is canonical there. + + This is the guard against over-widening the normalization. ``PatchedChatDeepSeek`` *is* a + ``BaseChatOpenAI`` subclass, so a naive ``issubclass`` gate would rewrite its ``api_base`` into + ``base_url`` and break every Doubao / Kimi config in ``config.example.yaml``, which document + ``api_base`` for exactly this class. + """ + from deerflow.models.patched_deepseek import PatchedChatDeepSeek + cfg = _make_app_config([_make_model_with_extras("ds", use="deerflow.models.patched_deepseek:PatchedChatDeepSeek", api_base="http://ds/v3")]) - _patch_factory(monkeypatch, cfg) + captured: dict = {} + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(PatchedChatDeepSeek, captured)) - 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 + assert captured.get("api_base") == "http://ds/v3" + assert "base_url" not in captured def test_no_op_when_neither_base_url_nor_api_base(monkeypatch): @@ -1396,27 +1397,31 @@ def test_known_config_keys_emit_no_warning(monkeypatch, caplog): 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) + from deerflow.models.patched_openai import PatchedChatOpenAI + + cfg = _make_app_config([_make_model_with_extras("patched", use="deerflow.models.patched_openai:PatchedChatOpenAI", api_base="http://localhost:4001/v1")]) + captured: dict = {} + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(PatchedChatOpenAI, captured)) - 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 + assert captured.get("base_url") == "http://localhost:4001/v1" + assert "api_base" not in captured 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) + from langchain_openai import ChatOpenAI + + cfg = _make_app_config([_make_model_with_extras("oai", openai_api_base="http://canonical/v1", api_base="http://alias/v1")]) + captured: dict = {} + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(ChatOpenAI, captured)) - 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 + assert captured.get("openai_api_base") == "http://canonical/v1" + assert "api_base" not in captured + assert "base_url" not in captured def test_no_unknown_key_warning_for_non_openai_class(monkeypatch, caplog): @@ -1427,11 +1432,107 @@ def test_no_unknown_key_warning_for_non_openai_class(monkeypatch, caplog): """ import logging - cfg = _make_app_config([_make_model_with_extras("anthropic", use="langchain_anthropic:ChatAnthropic", frequency_penalty=0.5)]) - _patch_factory(monkeypatch, cfg) + from langchain_anthropic import ChatAnthropic + + cfg = _make_app_config([_make_model_with_extras("anthropic", use="langchain_anthropic:ChatAnthropic", frequency_penalty=0.5, api_base="http://x/v1")]) + captured: dict = {} + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(ChatAnthropic, captured)) - 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) + # api_base normalization is likewise scoped to the OpenAI family: a non-BaseChatOpenAI + # provider must never have its keys rewritten. The config sets api_base, so this + # actually exercises the normalization-skip path (not just its absence): the alias + # is passed through verbatim and never rewritten to base_url. + assert captured.get("api_base") == "http://x/v1" + assert "base_url" not in captured + + +# --------------------------------------------------------------------------- +# The OpenAI-compatible family is issubclass(BaseChatOpenAI), not a class-path allowlist +# (regression: six in-repo BaseChatOpenAI subclasses were excluded from api_base +# normalization and from the unknown-key warning) +# --------------------------------------------------------------------------- + +# Every in-repo BaseChatOpenAI subclass that inherits only `openai_api_base` (alias `base_url`) +# and was NOT in the original ChatOpenAI / PatchedChatOpenAI allowlist. PatchedChatDeepSeek is +# deliberately absent: it declares `api_base` itself and is covered by the preservation test above. +_OPENAI_SUBCLASS_USE_PATHS_WITHOUT_API_BASE = [ + "deerflow.models.vllm_provider:VllmChatModel", + "deerflow.models.mindie_provider:MindIEChatModel", + "deerflow.models.patched_mimo:PatchedChatMiMo", + "deerflow.models.patched_stepfun:PatchedChatStepFun", + "deerflow.models.patched_minimax:PatchedChatMiniMax", +] + + +@pytest.mark.parametrize("use_path", _OPENAI_SUBCLASS_USE_PATHS_WITHOUT_API_BASE) +def test_api_base_normalized_for_all_openai_subclasses(monkeypatch, use_path): + """`api_base` must become `base_url` for every BaseChatOpenAI subclass, not just the two + stock OpenAI paths. + + These classes inherit the endpoint field as `openai_api_base` (alias `base_url`) and do not + declare `api_base`. Excluded by the old class-path allowlist, a user's `api_base` was diverted + into `model_kwargs` — so the endpoint override was silently dropped (the client fell back to + the default OpenAI endpoint) and the stray key was spread into every `Completions.create()` + call, failing at request time with an opaque `unexpected keyword argument 'api_base'`. + """ + real_cls = resolve_class(use_path, BaseChatModel) + cfg = _make_app_config([_make_model_with_extras("m", use=use_path, api_base="http://gw.example/v1")]) + captured: dict = {} + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(real_cls, captured)) + + factory_module.create_chat_model(name="m") + + assert captured.get("base_url") == "http://gw.example/v1" + assert "api_base" not in captured + + +@pytest.mark.parametrize("use_path", _OPENAI_SUBCLASS_USE_PATHS_WITHOUT_API_BASE) +def test_unknown_config_key_warns_for_all_openai_subclasses(monkeypatch, use_path, caplog): + """The unknown-key warning must fire for every BaseChatOpenAI subclass. + + The `model_kwargs` divert-and-crash behaviour is implemented in `BaseChatOpenAI`, so every + subclass inherits it. Scoping the warning to the two stock paths meant the diagnostic that + exists to surface this failure was disabled for exactly the classes that suffer it. + """ + import logging + + real_cls = resolve_class(use_path, BaseChatModel) + cfg = _make_app_config([_make_model_with_extras("m", use=use_path, definitely_not_a_real_kwarg=True)]) + captured: dict = {} + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(real_cls, captured)) + + with caplog.at_level(logging.WARNING, logger=factory_module.__name__): + factory_module.create_chat_model(name="m") + + assert any("definitely_not_a_real_kwarg" in rec.message for rec in caplog.records) + + +def test_api_base_reaches_real_minimax_constructor_as_base_url(monkeypatch): + """End-to-end anchor on a real provider class, nothing stubbed. + + Builds the genuine `PatchedChatMiniMax` (dummy key, no network) from a config that sets + `api_base`, and asserts the endpoint actually lands on the client's `openai_api_base` field + instead of being diverted into `model_kwargs`. + """ + cfg = _make_app_config( + [ + _make_model_with_extras( + "minimax", + use="deerflow.models.patched_minimax:PatchedChatMiniMax", + api_key="sk-dummy", + api_base="https://api.minimax.io/v1", + ) + ] + ) + # Do NOT patch resolve_class — construct the real PatchedChatMiniMax class. + monkeypatch.setattr(factory_module, "get_app_config", lambda: cfg) + monkeypatch.setattr(factory_module, "build_tracing_callbacks", lambda: []) + + instance = factory_module.create_chat_model(name="minimax") + + assert instance.openai_api_base == "https://api.minimax.io/v1" + assert "api_base" not in (instance.model_kwargs or {})