feat: add enable_thinking support for OpenAI-compatible providers - #2181
Conversation
🤖 Open Code ReviewTarget: PR #2181 🔍 OpenCodeReview found 1 issue(s) in this PR. 1.
|
|
07ba039 to
59e98be
Compare
|
|
Closes MemTensor#2149 Adds enable_thinking configuration parameter for OpenAI-compatible providers (qwen, deepseek, minimax) to control whether the model produces <think> reasoning blocks before the actual response. Changes: - Added enable_thinking field (bool | None) to OpenAILLMConfig - None (default): preserve provider's default behavior - True: explicitly enable thinking mode - False: explicitly disable thinking mode (prevents JSON breaking) - OpenAILLM.generate / generate_stream now pass enable_thinking to API calls when configured, via enable_thinking body param - AzureLLM.generate / generate_stream also support the parameter (gated by getattr for backward compatibility with older configs) - Per-call override supported via kwargs (enable_thinking=True/False) - Added tests covering default, config-level, and kwarg-level enable_thinking behavior in test_enable_thinking.py This is a non-breaking change: when enable_thinking is unset, request bodies are identical to previous versions. Test: python3 -m py_compile src/memos/configs/llm.py Test: python3 -m py_compile src/memos/llms/openai.py Test: python3 -m py_compile tests/llms/test_enable_thinking.py
08760c2 to
333852d
Compare
✅ Automated Test Results: PASSEDAll tests passed (14/14 executed). memos_python_core/changed-repo-python: 14/14. Duration: 5s [advisory, non-gating] AI-generated tests on branch test/auto-gen-03457e832d3ea1fa-20260805133404: 87/129 passed, 42 failed — these do NOT affect the PR verdict; review the branch manually. Branch: |
|
Thanks for adding ProblemThe PR currently passes client.chat.completions.create(
...,
enable_thinking=False,
)However, I tested the following SDK versions:
The exact error is: Version 1.77.0 is especially relevant because it is the minimum version currently allowed by MemOS: openai = ">=1.77.0,<2.0.0"Therefore, using an older official SDK does not make the current implementation work. It might only work with a vendor-specific fork or wrapper that accepts arbitrary keyword arguments. The current tests mock Suggested implementationProvider-specific parameters should be merged into the SDK-supported def _merge_enable_thinking(extra_body, enable_thinking):
if enable_thinking is None:
return extra_body
return {
**(extra_body or {}),
"enable_thinking": enable_thinking,
}Then use: client.chat.completions.create(
...,
extra_body=_merge_enable_thinking(extra_body, enable_thinking),
)The SDK will merge {
"enable_thinking": false
}This approach also:
Azure configuration
I suggest adding: class AzureLLMConfig(BaseLLMConfig):
# Existing fields...
extra_body: Any = Field(
default=None,
description="Extra request body parameters",
)
enable_thinking: bool | None = Field(
default=None,
description=(
"Enable or disable thinking mode. "
"When None, preserve the provider's default behavior."
),
)Without this, Pydantic rejects an Azure configuration containing |
✅ Automated Test Results: PASSEDAll tests passed (19/19 executed). memos_python_core/changed-repo-python: 19/19. Duration: 6s Branch: |
Exercise the independent Azure streaming request-body path with a per-call enable_thinking override and an existing vendor extra_body option. Test: .venv/bin/python -m pytest tests/llms/test_enable_thinking.py tests/configs/test_llm.py -q\nTest: .venv/bin/python -m pytest tests/llms -q --ignore=tests/llms/test_hf.py\nTest: .venv/bin/python -m pytest tests/configs -q
|
Thanks @endxxxx for testing the official SDK versions and for pushing I kept that production fix unchanged and verified the compatibility boundary directly:
Verification:
CI update for run
I have kept the unrelated core/optional dependency issue out of this PR. |
✅ Automated Test Results: PASSEDAll tests passed (20/20 executed). memos_python_core/changed-repo-python: 20/20. Duration: 5s [advisory, non-gating] AI-generated tests on branch test/auto-gen-7f506e4e6f76a1a9-20260812102405: 35/35 passed — these do NOT affect the PR verdict; review the branch manually. Branch: |
✅ Automated Test Results: PASSEDAll tests passed (20/20 executed). memos_python_core/changed-repo-python: 20/20. Duration: 6s [advisory, non-gating] AI-generated tests on branch test/auto-gen-6974f526c003764f-20260813165549: 172/172 passed — these do NOT affect the PR verdict; review the branch manually. Branch: |
Description
Fixes #2149
Adds
enable_thinkingconfiguration parameter for OpenAI-compatible providers (Qwen, DeepSeek, MiniMax, etc.) to control whether the model produces<think>reasoning blocks before the actual response.Models like Qwen3 and DeepSeek-R1 support an
enable_thinkingparameter in the chat completion body. When thinking is enabled, output contains<think>...</think>tags before the actual response, which can break JSON parsing in structured-output tasks (capture summarization, L3 abstraction, skill crystallization).Changes
enable_thinkingfield toOpenAILLMConfig—bool | Nonedefaulting toNone:None(default): provider's default behavior preserved (backward compatible)True: explicitly passenable_thinking=trueFalse: explicitly passenable_thinking=false(protects JSON output tasks)OpenAILLM.generate()— extracted_build_request_body()helper; passesenable_thinkingto request body when configuredOpenAILLM.generate_stream()— sameenable_thinkinginjectionAzureLLM.generate() / generate_stream()— addedgetattr-gated support forenable_thinking(future-proof)kwargs["enable_thinking"]takes precedence over config-level settingtests/llms/test_enable_thinking.py: default, config-level, kwarg-level, False-value, param-preservationBefore (provider default)
After (configurable)
Type of change
How Has This Been Tested?
python3 -m py_compile src/memos/configs/llm.pypython3 -m py_compile src/memos/llms/openai.pypython3 -m py_compile tests/llms/test_enable_thinking.pyenable_thinking=None→ param omitted;True/False→ param present; kwarg override winsenable_thinkingis not set (backward compatible)Checklist