mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-21 02:05:45 +00:00
fix(models): apply stream_chunk_timeout default to all BaseChatOpenAI subclasses (#4102)
* 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).
This commit is contained in:
Daoyuan Li
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user