mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
fix(models): tolerate null template keys in the legacy thinking disable path (#6838)
* fix(models): tolerate null template keys in the legacy thinking disable path
`ModelConfig.when_thinking_enabled` is a plain `dict | None` with no
per-key validation, so a template key left blank in config.yaml
(`extra_body:`, `thinking:`) loads as an explicit None. The legacy
disable path read those keys with `effective_wte.get(key, {})`, whose
default only applies to an absent key — so it called `.get` on None and
every create_chat_model for the profile crashed with
AttributeError: 'NoneType' object has no attribute 'get', on the
default thinking_enabled=False path (title generation, summarization,
subagent and lead-agent builds).
Guard the three reads with the same `or {}` idiom the sibling accesses
in this module already use (`_infer_dialect`, the contract path), so a
null value falls through the dialect heuristics like an absent one and
the legacy path matches the contract path, which already tolerates the
same profile shape.
* fix(models): tolerate null nested thinking templates
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
This commit is contained in:
parent
eadc1e230e
commit
a4be043aec
@ -343,6 +343,8 @@ streams simulate chunks from a non-streaming response, while no-tool streams sta
|
||||
|
||||
To route OpenAI models through `/v1/responses`, keep using `langchain_openai:ChatOpenAI` and set `use_responses_api: true` with `output_version: responses/v1`.
|
||||
|
||||
When thinking is disabled on a legacy profile, blank `extra_body`, `thinking`, and nested `extra_body.thinking` keys in a `when_thinking_enabled` template are treated as absent during dialect inference. When thinking is enabled, template values are forwarded as configured.
|
||||
|
||||
Models whose provider contract differs from DeerFlow's generic thinking/effort assumptions can declare a per-model mapping-valued `reasoning:` block (thinking `unsupported`/`optional`/`required`, the accepted effort values with aliases and a default, the payload dialect, and the reasoning-history requirement). The setup wizard's Z.AI GLM-5.3-Flash profile uses it: thinking stays on for every foreground and background call, and the effort selector offers the model's own `low`/`high`/`max` levels. Ollama's existing boolean `reasoning: true` remains a native provider setting and is forwarded to ChatOllama. When migrating a profile to a custom effort `path`, remove any old `reasoning_effort` setting from the profile and thinking templates; configuration validation rejects the leftover key. The chat UI drops a remembered provider-specific effort when switching to a legacy model that does not advertise it. Profiles without the block keep their existing provider behavior. See `config.example.yaml` for the shape and the equivalent manual configuration.
|
||||
|
||||
For vLLM 0.19.0, use `deerflow.models.vllm_provider:VllmChatModel`. For Qwen-style reasoning models, DeerFlow toggles reasoning with `extra_body.chat_template_kwargs.enable_thinking` and preserves vLLM's non-standard `reasoning` field across multi-turn tool-call conversations. Legacy `thinking` configs are normalized automatically for backward compatibility, without modifying model defaults or caller-owned `extra_body` dictionaries; a reused dictionary can change the legacy switch between requests. If the endpoint reports a cumulative usage snapshot on every streaming chunk, set `cumulative_stream_usage: true` so DeerFlow converts those snapshots into per-chunk deltas; the option is disabled by default and leaves usage unchanged when a stable completion id is unavailable. Reasoning models may also require the server to be started with `--reasoning-parser ...`. If your local vLLM deployment accepts any non-empty API key, you can still set `VLLM_API_KEY` to a placeholder value.
|
||||
|
||||
@ -33,6 +33,7 @@ fast-path permit check from queue insertion, or an older caller can be overtaken
|
||||
while handing off to the wait queue.
|
||||
|
||||
- `create_chat_model(name, thinking_enabled)` instantiates LLM from config via reflection
|
||||
- Legacy disable-path heuristics treat null `extra_body`, `thinking`, and nested `extra_body.thinking` template values as absent, allowing native/vLLM fallbacks. The enable path still forwards configured values. Regression cases live in `tests/test_model_factory.py`.
|
||||
- Supports `thinking_enabled` flag with per-model `when_thinking_enabled` / `when_thinking_disabled` overrides; on both the legacy and the contract path every template and every synthesized `extra_body` payload goes through `_merge_thinking_payload` (the native-Anthropic disable assigns the top-level `thinking` mapping outright — a tagged union keyed by `type`): keys are never removed (a template can only add or override; nested mappings inherit the profile's other keys), template wins on conflicts, so a template's `extra_body` never clobbers sibling keys such as `extra_body.tool_stream`. Corollary: a disable template cannot clear a key the profile sets, so enable-only keys such as `extra_body.thinking.budget_tokens` belong in `when_thinking_enabled`, never in the base `extra_body`. Templates are deep-copied as they are merged, so constructor kwargs never alias the cached `ModelConfig`. The vLLM switch has two spellings (`_VLLM_THINKING_SWITCHES`: legacy `thinking` alias, `enable_thinking`); when a profile spells it differently from the payload, the payload's value is mirrored onto the profile's spelling rather than deleting or canonicalizing it — `VllmChatModel` collapses the pair to `enable_thinking`, other OpenAI-compatible classes send both, and either way the server reads the template's intent. Do not canonicalize `thinking` → `enable_thinking` in the factory: chat templates differ on which key they honor, and only `VllmChatModel` opts into that remap
|
||||
- Supports vLLM-style thinking toggles via `when_thinking_enabled.extra_body.chat_template_kwargs.enable_thinking` for Qwen reasoning models, while normalizing legacy `thinking` configs for backward compatibility
|
||||
- A per-request `reasoning_effort` kwarg (the regular, non-bootstrap lead-agent build passes it even when `None`) is popped from `kwargs` and layered like `model_overrides`: a non-`None` value replaces the profile's, and the thinking transforms applied afterwards (`when_thinking_enabled`, `when_thinking_disabled`, the `extra_body.thinking` disable path) still decide the final value. Never let a key reach the constructor through both `kwargs` and the profile settings — Python raises `got multiple values for keyword argument` and the lead agent cannot be built for that model. Codex checks the requested level itself. Pinned by `tests/test_model_factory.py` and `tests/test_lead_agent_model_resolution.py`
|
||||
|
||||
@ -229,14 +229,14 @@ def _apply_legacy_thinking_settings(
|
||||
if model_config.when_thinking_disabled is not None:
|
||||
# User-provided disable settings win on conflicts; profile keys survive.
|
||||
_merge_thinking_payload(settings, model_config.when_thinking_disabled)
|
||||
elif has_thinking_settings and effective_wte.get("extra_body", {}).get("thinking", {}).get("type"):
|
||||
elif has_thinking_settings and ((effective_wte.get("extra_body") or {}).get("thinking") or {}).get("type"):
|
||||
# OpenAI-compatible gateway: thinking is nested under extra_body
|
||||
_merge_thinking_payload(settings, {"extra_body": {"thinking": {"type": "disabled"}}})
|
||||
settings["reasoning_effort"] = "minimal"
|
||||
elif has_thinking_settings and (disable_chat_template_kwargs := _vllm_disable_chat_template_kwargs(effective_wte.get("extra_body", {}).get("chat_template_kwargs") or {})):
|
||||
elif has_thinking_settings and (disable_chat_template_kwargs := _vllm_disable_chat_template_kwargs((effective_wte.get("extra_body") or {}).get("chat_template_kwargs") or {})):
|
||||
# vLLM uses chat template kwargs to switch thinking on/off.
|
||||
_merge_thinking_payload(settings, {"extra_body": {"chat_template_kwargs": disable_chat_template_kwargs}})
|
||||
elif has_thinking_settings and effective_wte.get("thinking", {}).get("type"):
|
||||
elif has_thinking_settings and (effective_wte.get("thinking") or {}).get("type"):
|
||||
# Native langchain_anthropic: thinking is a direct constructor parameter
|
||||
settings["thinking"] = {"type": "disabled"}
|
||||
|
||||
|
||||
@ -435,6 +435,46 @@ def test_legacy_non_mapping_template_values_are_forwarded_not_rejected(monkeypat
|
||||
assert captured["extra_body"] == expected_extra_body
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("thinking_enabled", "when_thinking_enabled", "expected"),
|
||||
[
|
||||
(False, {"extra_body": None, "thinking": {"type": "enabled"}}, {"thinking": {"type": "disabled"}}),
|
||||
(True, {"extra_body": None, "thinking": {"type": "enabled"}}, {"extra_body": None, "thinking": {"type": "enabled"}}),
|
||||
(False, {"extra_body": None}, {}),
|
||||
(False, {"thinking": None}, {}),
|
||||
(False, {"extra_body": {"thinking": None}}, {}),
|
||||
(True, {"extra_body": {"thinking": None}}, {"extra_body": {"thinking": None}}),
|
||||
(False, {"extra_body": {"thinking": None}, "thinking": {"type": "enabled"}}, {"thinking": {"type": "disabled"}}),
|
||||
(
|
||||
False,
|
||||
{"extra_body": {"thinking": None, "chat_template_kwargs": {"enable_thinking": True}}},
|
||||
{"extra_body": {"chat_template_kwargs": {"enable_thinking": False}}},
|
||||
),
|
||||
],
|
||||
ids=["thinking-off", "thinking-on", "extra-body-null", "thinking-null", "nested-null-off", "nested-null-on", "nested-null-native-fallback", "nested-null-vllm-fallback"],
|
||||
)
|
||||
def test_legacy_null_template_values_build_on_both_thinking_paths(monkeypatch, thinking_enabled, when_thinking_enabled, expected):
|
||||
"""A template key left blank in ``config.yaml`` (``extra_body:``, ``thinking:``) loads as an
|
||||
explicit ``None`` — and ``dict.get(key, {})`` only defaults an *absent* key. The legacy
|
||||
disable path therefore called ``.get`` on ``None`` and every ``create_chat_model`` for the
|
||||
profile crashed with ``AttributeError`` — on the default ``thinking_enabled=False`` path,
|
||||
i.e. title generation, summarization, subagent and lead-agent builds. A null template
|
||||
value must fall through the disable-path dialect heuristics like an absent one, as the
|
||||
guarded sibling accesses (``_infer_dialect``, the contract path) already do, while the
|
||||
enable path keeps forwarding the template unchanged."""
|
||||
model = _make_model("legacy-null", supports_thinking=True, when_thinking_enabled=when_thinking_enabled)
|
||||
captured: dict = {}
|
||||
_patch_factory(monkeypatch, _make_app_config([model]), model_class=_capturing_class(FakeChatModel, captured))
|
||||
|
||||
factory_module.create_chat_model(name="legacy-null", thinking_enabled=thinking_enabled)
|
||||
|
||||
for key in ("extra_body", "thinking"):
|
||||
if key in expected:
|
||||
assert captured[key] == expected[key]
|
||||
else:
|
||||
assert key not in captured
|
||||
|
||||
|
||||
@pytest.mark.parametrize("thinking_enabled", [True, False], ids=["thinking-on", "thinking-off"])
|
||||
def test_legacy_when_thinking_enabled_deep_merges_into_the_base_extra_body(monkeypatch, thinking_enabled):
|
||||
"""The legacy enable path used ``dict.update``, so ``when_thinking_enabled.extra_body``
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user