From 3e7baba39a9597e480dd82bbc18aee806679a2bf Mon Sep 17 00:00:00 2001 From: Daoyuan Li <94409450+DaoyuanLi2816@users.noreply.github.com> Date: Mon, 13 Jul 2026 06:28:44 -0700 Subject: [PATCH] fix(models): apply stream_chunk_timeout default to all BaseChatOpenAI subclasses (#4102) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(models): apply stream_chunk_timeout default to all BaseChatOpenAI subclasses The 240s stream_chunk_timeout default (issue #3189, PR #3195) was scoped to a class-path allowlist of only ChatOpenAI and PatchedChatOpenAI. Every other OpenAI-compatible provider that subclasses BaseChatOpenAI — VllmChatModel, MindIEChatModel, PatchedChatDeepSeek, PatchedChatMiMo, PatchedChatStepFun and PatchedChatMiniMax — was excluded, so they kept langchain-openai's aggressive 120s built-in chunk-gap timeout and, worse, silently discarded a user's explicit stream_chunk_timeout override from config.yaml. Issue #3189 was itself reported on mimo-v2.5 (PatchedChatMiMo), the exact class the original fix left out. Gate the injection on issubclass(model_class, BaseChatOpenAI) instead of the string allowlist, so any OpenAI-compatible subclass inherits the default and honors an explicit override. Genuinely non-OpenAI clients (e.g. ChatAnthropic) stay excluded and still have the kwarg dropped before it reaches a constructor that would divert it into model_kwargs and fail at request time. * fix(models): address review nits on stream_chunk_timeout default Correct the module-level comment above _DEFAULT_STREAM_CHUNK_TIMEOUT_SECONDS: langchain-openai's built-in stream_chunk_timeout default is 120s, not 60s (BaseChatOpenAI.stream_chunk_timeout's default_factory reads LANGCHAIN_OPENAI_STREAM_CHUNK_TIMEOUT_S with a 120.0 fallback). Simplify the BaseChatOpenAI gate in _apply_stream_chunk_timeout_default from `isinstance(model_class, type) and issubclass(model_class, BaseChatOpenAI)` to just `issubclass(...)`. The sole caller passes model_class from resolve_class(), which already raises before returning anything that isn't a type, so the isinstance half can never be False there. Also soften the docstring's non-OpenAI-client bullet: ChatAnthropic declares extra="ignore" and silently drops an unrecognized kwarg rather than diverting it into model_kwargs and failing at request time (that failure mode is specific to other OpenAI-style clients). --- .../harness/deerflow/models/factory.py | 37 ++-- backend/tests/test_model_factory.py | 163 ++++++++++++++---- 2 files changed, 153 insertions(+), 47 deletions(-) diff --git a/backend/packages/harness/deerflow/models/factory.py b/backend/packages/harness/deerflow/models/factory.py index 9eeb7386c..aa9bcd1de 100644 --- a/backend/packages/harness/deerflow/models/factory.py +++ b/backend/packages/harness/deerflow/models/factory.py @@ -1,6 +1,7 @@ import logging from langchain.chat_models import BaseChatModel +from langchain_openai.chat_models.base import BaseChatOpenAI from deerflow.config import get_app_config from deerflow.config.app_config import AppConfig @@ -133,7 +134,7 @@ def _warn_unknown_model_settings(model_use_path: str, model_class, model_name: s # Default chunk-gap budget for OpenAI-compatible streaming responses. # # langchain-openai raises ``StreamChunkTimeoutError`` after this many seconds -# without receiving a chunk. Its own default is 60s, which is too aggressive for +# without receiving a chunk. Its own default is 120s, which is too aggressive for # reasoning models (DeepSeek-R1, Doubao-thinking, GPT-5) whose first chunk can # legitimately take 90~150s. We default to 240s so the streaming layer rarely # trips on long thinking pauses; the LLMErrorHandlingMiddleware still retries @@ -141,20 +142,36 @@ def _warn_unknown_model_settings(model_use_path: str, model_class, model_name: s _DEFAULT_STREAM_CHUNK_TIMEOUT_SECONDS: float = 240.0 -def _apply_stream_chunk_timeout_default(model_use_path: str, model_settings_from_config: dict) -> None: +def _apply_stream_chunk_timeout_default(model_class: type, model_settings_from_config: dict) -> None: """Inject a generous ``stream_chunk_timeout`` for OpenAI-compatible clients. - The ``stream_chunk_timeout`` kwarg is specific to ``langchain_openai:ChatOpenAI`` - and is rejected by other providers' constructors as an unexpected keyword - argument. Behaviour: + ``stream_chunk_timeout`` is a field of langchain-openai's ``BaseChatOpenAI``, so + it is accepted by ``ChatOpenAI`` and by every DeerFlow provider that subclasses + it: ``PatchedChatOpenAI`` plus the self-hosted / reasoning adapters + ``VllmChatModel``, ``MindIEChatModel``, ``PatchedChatDeepSeek``, + ``PatchedChatMiMo``, ``PatchedChatStepFun`` and ``PatchedChatMiniMax``. We gate on + ``issubclass(model_class, BaseChatOpenAI)`` rather than an explicit class-path + allowlist so any OpenAI-compatible subclass inherits the default (and honors an + explicit override) automatically. Issue #3189 was reported against ``mimo-v2.5`` + (``PatchedChatMiMo``); the original fix (#3195) matched only ``ChatOpenAI`` / + ``PatchedChatOpenAI``, so those subclasses kept langchain-openai's aggressive + built-in chunk-gap timeout and — worse — silently discarded a user's explicit + ``stream_chunk_timeout``. - * OpenAI-compatible path: an explicit value in ``config.yaml`` is preserved. + Behaviour: + + * ``BaseChatOpenAI`` subclass: an explicit value in ``config.yaml`` is preserved. An explicit ``null`` is dropped upstream by ``model_dump(exclude_none=True)`` and therefore treated as "unset", so the default is injected. - * Non-OpenAI path: drop the key so it is never forwarded to an incompatible - constructor (which would raise ``TypeError: unexpected keyword argument``). + * Any other client (e.g. ``ChatAnthropic``): drop the key so it is never + forwarded to a constructor that does not declare it. The kwarg is not a + declared field of these clients: depending on the client it is either + silently dropped (``ChatAnthropic`` declares ``extra="ignore"``) or, for + other OpenAI-style clients, diverted into ``model_kwargs`` and rejected + at request time. Either way the user's intent is lost, so we drop it + proactively instead. """ - if model_use_path not in _OPENAI_COMPAT_USE_PATHS: + if not issubclass(model_class, BaseChatOpenAI): model_settings_from_config.pop("stream_chunk_timeout", None) return if "stream_chunk_timeout" in model_settings_from_config: @@ -250,7 +267,7 @@ def create_chat_model(name: str | None = None, thinking_enabled: bool = False, * # 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) + _apply_stream_chunk_timeout_default(model_class, model_settings_from_config) # For Codex Responses API models: map thinking mode to reasoning_effort from deerflow.models.openai_codex_provider import CodexChatModel diff --git a/backend/tests/test_model_factory.py b/backend/tests/test_model_factory.py index 2efd780f2..7415465f1 100644 --- a/backend/tests/test_model_factory.py +++ b/backend/tests/test_model_factory.py @@ -10,6 +10,7 @@ from deerflow.config.model_config import ModelConfig from deerflow.config.sandbox_config import SandboxConfig from deerflow.models import factory as factory_module from deerflow.models import openai_codex_provider as codex_provider_module +from deerflow.reflection import resolve_class # --------------------------------------------------------------------------- # Helpers @@ -78,6 +79,26 @@ def _patch_factory(monkeypatch, app_config: AppConfig, model_class=FakeChatModel monkeypatch.setattr(factory_module, "build_tracing_callbacks", lambda: []) +def _capturing_class(base_cls: type, captured: dict) -> type: + """Build a kwargs-capturing subclass of a REAL provider class. + + ``_apply_stream_chunk_timeout_default`` gates on ``issubclass(model_class, + BaseChatOpenAI)``, so the resolved class must genuinely subclass the real + provider for the test to exercise that gate. ``__init__`` only records the + constructor kwargs and deliberately skips the provider's real ``__init__`` (so no + api_key / network / event loop is required); the factory never reads the returned + instance's fields when tracing is patched to ``[]``, so a bare instance is safe + for these config-level assertions. + """ + + class _Capturing(base_cls): # type: ignore[valid-type,misc] + def __init__(self, **kwargs): + captured.clear() + captured.update(kwargs) + + return _Capturing + + # --------------------------------------------------------------------------- # Model selection # --------------------------------------------------------------------------- @@ -1094,21 +1115,17 @@ def test_no_duplicate_kwarg_when_reasoning_effort_in_config_and_thinking_disable def test_stream_chunk_timeout_defaults_to_240_for_openai_compatible_model(monkeypatch): - """OpenAI-compatible clients must receive a generous 240s chunk-gap budget by + """A bare ChatOpenAI client must receive a generous 240s chunk-gap budget by default, so reasoning models with long thinking pauses don't trip - langchain-openai's aggressive 60s built-in default. + langchain-openai's aggressive built-in default. """ + from langchain_openai import ChatOpenAI + model = _make_model(use="langchain_openai:ChatOpenAI") cfg = _make_app_config([model]) captured: dict = {} - - class CapturingModel(FakeChatModel): - def __init__(self, **kwargs): - captured.update(kwargs) - BaseChatModel.__init__(self, **kwargs) - - _patch_factory(monkeypatch, cfg, model_class=CapturingModel) + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(ChatOpenAI, captured)) factory_module.create_chat_model(name="test-model") assert captured.get("stream_chunk_timeout") == 240.0 @@ -1119,6 +1136,8 @@ def test_stream_chunk_timeout_user_value_not_overridden(monkeypatch): factory must not overwrite it with the default — even if the value is smaller (60s) or larger (600s) than the default. """ + from langchain_openai import ChatOpenAI + model = ModelConfig( name="custom-timeout-model", display_name="Custom Timeout", @@ -1130,33 +1149,24 @@ def test_stream_chunk_timeout_user_value_not_overridden(monkeypatch): cfg = _make_app_config([model]) captured: dict = {} - - class CapturingModel(FakeChatModel): - def __init__(self, **kwargs): - captured.update(kwargs) - BaseChatModel.__init__(self, **kwargs) - - _patch_factory(monkeypatch, cfg, model_class=CapturingModel) + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(ChatOpenAI, captured)) factory_module.create_chat_model(name="custom-timeout-model") assert captured.get("stream_chunk_timeout") == 60.0 def test_stream_chunk_timeout_not_injected_for_non_openai_provider(monkeypatch): - """Only langchain_openai:ChatOpenAI receives the default. Anthropic / Vertex / - other clients that don't understand this kwarg must not be polluted with it. + """Only BaseChatOpenAI subclasses receive the default. A genuinely non-OpenAI + client (ChatAnthropic) that does not declare this kwarg must not be polluted + with it. """ + from langchain_anthropic import ChatAnthropic + model = _make_model(use="langchain_anthropic:ChatAnthropic") cfg = _make_app_config([model]) captured: dict = {} - - class CapturingModel(FakeChatModel): - def __init__(self, **kwargs): - captured.update(kwargs) - BaseChatModel.__init__(self, **kwargs) - - _patch_factory(monkeypatch, cfg, model_class=CapturingModel) + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(ChatAnthropic, captured)) factory_module.create_chat_model(name="test-model") assert "stream_chunk_timeout" not in captured @@ -1172,12 +1182,13 @@ def test_stream_chunk_timeout_default_constant_is_documented(): def test_stream_chunk_timeout_popped_for_non_openai_provider_when_user_set_it(monkeypatch): """Regression for CR feedback on issue #3189: if a user accidentally sets - ``stream_chunk_timeout`` on a non-OpenAI provider, the factory must drop - the kwarg before forwarding it to the model constructor. Otherwise the - third-party client raises ``TypeError: unexpected keyword argument - 'stream_chunk_timeout'`` because the parameter is specific to - ``langchain_openai:ChatOpenAI``. + ``stream_chunk_timeout`` on a non-OpenAI provider, the factory must drop the + kwarg before forwarding it to the model constructor. ChatAnthropic does not + declare the field, so it would otherwise divert the value into ``model_kwargs`` + and fail at request time. """ + from langchain_anthropic import ChatAnthropic + model = ModelConfig( name="anthropic-with-stray-timeout", display_name="Anthropic With Stray Timeout", @@ -1189,18 +1200,96 @@ def test_stream_chunk_timeout_popped_for_non_openai_provider_when_user_set_it(mo cfg = _make_app_config([model]) captured: dict = {} - - class CapturingModel(FakeChatModel): - def __init__(self, **kwargs): - captured.update(kwargs) - BaseChatModel.__init__(self, **kwargs) - - _patch_factory(monkeypatch, cfg, model_class=CapturingModel) + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(ChatAnthropic, captured)) factory_module.create_chat_model(name="anthropic-with-stray-timeout") assert "stream_chunk_timeout" not in captured +# --------------------------------------------------------------------------- +# stream_chunk_timeout applies to ALL BaseChatOpenAI subclasses, not just the +# ChatOpenAI/PatchedChatOpenAI class-path allowlist (issue #3189 was reported on +# mimo-v2.5 → PatchedChatMiMo, which the original #3195 allowlist excluded). +# --------------------------------------------------------------------------- + +# Every in-repo provider that subclasses BaseChatOpenAI (and therefore inherits the +# stream_chunk_timeout mechanism) but was NOT in the original ChatOpenAI / +# PatchedChatOpenAI allowlist. +_STREAM_TIMEOUT_OPENAI_SUBCLASS_USE_PATHS = [ + "deerflow.models.vllm_provider:VllmChatModel", + "deerflow.models.mindie_provider:MindIEChatModel", + "deerflow.models.patched_deepseek:PatchedChatDeepSeek", + "deerflow.models.patched_mimo:PatchedChatMiMo", + "deerflow.models.patched_stepfun:PatchedChatStepFun", + "deerflow.models.patched_minimax:PatchedChatMiniMax", +] + + +@pytest.mark.parametrize("use_path", _STREAM_TIMEOUT_OPENAI_SUBCLASS_USE_PATHS) +def test_stream_chunk_timeout_defaults_to_240_for_all_openai_subclasses(monkeypatch, use_path): + """Every BaseChatOpenAI subclass provider — not just ChatOpenAI — must receive + the 240s default when the user leaves stream_chunk_timeout unset. These classes + were silently excluded by the original class-path allowlist and fell back to + langchain-openai's aggressive built-in gap timeout. + """ + real_cls = resolve_class(use_path, BaseChatModel) + model = _make_model(use=use_path) + cfg = _make_app_config([model]) + + captured: dict = {} + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(real_cls, captured)) + factory_module.create_chat_model(name="test-model") + + assert captured.get("stream_chunk_timeout") == 240.0 + + +@pytest.mark.parametrize("use_path", _STREAM_TIMEOUT_OPENAI_SUBCLASS_USE_PATHS) +def test_stream_chunk_timeout_user_override_honored_for_all_openai_subclasses(monkeypatch, use_path): + """A user's explicit stream_chunk_timeout must survive for every BaseChatOpenAI + subclass provider. The original allowlist popped it unconditionally for these + classes, silently discarding a config.yaml override with no warning. + """ + real_cls = resolve_class(use_path, BaseChatModel) + model = ModelConfig( + name="override-model", + display_name="Override", + description=None, + use=use_path, + model="reasoning-model", + stream_chunk_timeout=300.0, # explicit user override + ) + cfg = _make_app_config([model]) + + captured: dict = {} + _patch_factory(monkeypatch, cfg, model_class=_capturing_class(real_cls, captured)) + factory_module.create_chat_model(name="override-model") + + assert captured.get("stream_chunk_timeout") == 300.0 + + +def test_stream_chunk_timeout_240_reaches_real_mimo_constructor(monkeypatch): + """End-to-end anchor for issue #3189 (reported on mimo-v2.5): the 240s default + must be accepted as a genuine ``stream_chunk_timeout`` field by the real + ``PatchedChatMiMo`` constructor — not diverted into ``model_kwargs`` — so the + streaming layer actually honors it. Builds the real class (no network / dummy + key) instead of a capturing stub. + """ + model = _make_model_with_extras( + "mimo", + use="deerflow.models.patched_mimo:PatchedChatMiMo", + api_key="sk-dummy", + base_url="http://localhost:8000/v1", + ) + cfg = _make_app_config([model]) + # Do NOT patch resolve_class — construct the real PatchedChatMiMo 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="mimo") + + assert instance.stream_chunk_timeout == 240.0 + + # --------------------------------------------------------------------------- # OpenAI base_url normalization + unknown-key warning # (regression: api_base copied onto a ChatOpenAI model crashed at request time)