feat(openai): Implement OpenAI o1 thinking mode with budget configuration (#98) #170
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR implements OpenAI's o1 and o1-mini thinking mode feature, allowing models to perform extended reasoning before generating responses. The implementation adds thinking budget configuration, handles mutual exclusion with streaming and tool calling, and uses reflection to extract thinking content from API responses.
Key Changes:
- Added
enableThinkingconfiguration parameter and thinking budget support toOpenAIChatModel - Implemented thinking parameter application in
OpenAIToolsHelperwith proper JSON structure - Added reflection-based thinking content extraction in
OpenAIResponseParserfor SDK compatibility
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
OpenAIThinkingModeTest.java |
Comprehensive test suite with 15 unit tests covering model creation, configuration, parameter application, and content extraction |
OpenAIChatModel.java |
Core model changes including thinking enablement, tool/streaming compatibility checks, and parameter application ordering |
OpenAIToolsHelper.java |
Helper methods for applying thinking configuration and filtering null values in additional parameters |
OpenAIResponseParser.java |
Reflection-based thinking content extraction with fallback handling for SDK version compatibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| // For other types (Optional, wrapper objects, etc.), try toString | ||
| // and verify it's not a default Object.toString() | ||
| String strValue = thinkingValue.toString(); | ||
| if (strValue != null | ||
| && !strValue.isEmpty() | ||
| && !isDefaultObjectToString(strValue)) { | ||
| return strValue; | ||
| } |
There was a problem hiding this comment.
The handling of Optional types is incomplete. If thinkingValue is an Optional<String>, calling toString() will return "Optional[value]" or "Optional.empty" rather than the actual content. This should explicitly check for Optional and extract its value.
Consider adding:
else if (thinkingValue instanceof java.util.Optional<?> opt) {
if (opt.isPresent()) {
Object value = opt.get();
if (value instanceof String strValue && !strValue.isEmpty()) {
return strValue;
}
}
} else {
// For other types (wrapper objects, etc.), try toString
// and verify it's not a default Object.toString()
String strValue = thinkingValue.toString();
if (strValue != null
&& !strValue.isEmpty()
&& !isDefaultObjectToString(strValue)) {
return strValue;
}
}| @Test | ||
| @DisplayName("Should apply thinking with options budget") | ||
| void testApplyThinkingWithOptions() { | ||
| assertDoesNotThrow( | ||
| () -> { | ||
| ChatCompletionCreateParams.Builder paramsBuilder = | ||
| ChatCompletionCreateParams.builder() | ||
| .model(com.openai.models.ChatModel.GPT_4); | ||
| GenerateOptions options = | ||
| GenerateOptions.builder().thinkingBudget(5000).build(); | ||
| toolsHelper.applyThinking(paramsBuilder, options, null); | ||
| assertNotNull(paramsBuilder, "Params builder should be modified"); | ||
| }); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Should apply thinking with default options budget") | ||
| void testApplyThinkingWithDefaultOptions() { | ||
| assertDoesNotThrow( | ||
| () -> { | ||
| ChatCompletionCreateParams.Builder paramsBuilder = | ||
| ChatCompletionCreateParams.builder() | ||
| .model(com.openai.models.ChatModel.GPT_4); | ||
| GenerateOptions defaultOptions = | ||
| GenerateOptions.builder().thinkingBudget(8000).build(); | ||
| toolsHelper.applyThinking(paramsBuilder, null, defaultOptions); | ||
| assertNotNull(paramsBuilder, "Params builder should be modified"); | ||
| }); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Should prefer options budget over default budget") | ||
| void testThinkingBudgetPriority() { | ||
| assertDoesNotThrow( | ||
| () -> { | ||
| ChatCompletionCreateParams.Builder paramsBuilder = | ||
| ChatCompletionCreateParams.builder() | ||
| .model(com.openai.models.ChatModel.GPT_4); | ||
| GenerateOptions options = | ||
| GenerateOptions.builder().thinkingBudget(5000).build(); | ||
| GenerateOptions defaultOptions = | ||
| GenerateOptions.builder().thinkingBudget(8000).build(); | ||
| toolsHelper.applyThinking(paramsBuilder, options, defaultOptions); | ||
| assertNotNull(paramsBuilder, "Params builder should be modified"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
These tests only verify that no exceptions are thrown, but don't verify the actual behavior. They should assert that the thinking configuration is properly applied to the params builder. For example, by building the params and verifying the "thinking" property exists in the additional body params.
Consider verifying the actual applied values:
ChatCompletionCreateParams params = paramsBuilder.build();
assertNotNull(params, "Should successfully build params with thinking");
// Verify thinking was actually applied (if accessible via the params object)| */ | ||
| protected String extractThinking(ChatCompletionMessage message) { | ||
| try { | ||
| // First, try to use a public getter if it exists |
There was a problem hiding this comment.
This comment is misleading. The actual code comment says "First, try to use a public getter if it exists" but the code immediately uses reflection on a field (not a getter method). The comment should be updated to accurately reflect that this is using reflection to access the private field directly.
Suggested fix:
// Try to access the thinking field using reflection
// The OpenAI SDK may not yet expose this field in the public API| // First, try to use a public getter if it exists | |
| // Try to access the thinking field using reflection | |
| // The OpenAI SDK may not yet expose this field in the public API |
| GenerateOptions options = | ||
| GenerateOptions.builder().thinkingBudget(10000).build(); | ||
| assertNotNull( | ||
| options.getThinkingBudget(), | ||
| "Thinking budget should be set in options"); | ||
| assertTrue( | ||
| options.getThinkingBudget() > 0, "Thinking budget should be positive"); | ||
| }); |
There was a problem hiding this comment.
This test only verifies that the thinking budget is set and is positive, but doesn't verify that the exact value (10000) was correctly stored. The assertion should verify the specific value to ensure proper data handling.
Consider:
assertEquals(10000, options.getThinkingBudget(), "Thinking budget should match the configured value");| for (Map.Entry<String, String> entry : headers.entrySet()) { | ||
| // Skip null values to prevent API errors | ||
| if (entry.getValue() == null) { | ||
| log.debug("Skipped null value for header '{}'", entry.getKey()); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The code checks for null values but doesn't check for null keys. If a map entry has a null key, this will cause a NullPointerException when passing it to the setter. While this is unlikely in practice, defensive programming suggests checking both key and value.
Consider:
if (entry.getKey() == null || entry.getValue() == null) {
log.debug("Skipped entry with null key or value");
continue;
}| // Skip null parameter values to prevent OpenAI SDK errors | ||
| if (entry.getValue() == null) { | ||
| log.debug( | ||
| "Skipped null value for parameter '{}' in tool '{}'", | ||
| entry.getKey(), | ||
| toolSchema.getName()); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Similar to the previous checks, null keys should also be validated here to prevent potential issues when passing to the logger or setter.
Consider:
if (entry.getKey() == null) {
log.debug("Skipped entry with null key in tool parameters");
continue;
}
if (entry.getValue() == null) {
log.debug("Skipped null value for parameter '{}' in tool '{}'",
entry.getKey(), toolSchema.getName());
continue;
}| // Get thinking budget from options with fallback to defaultOptions | ||
| // Note: defaultOptions is guaranteed to be non-null (set in constructor) | ||
| Integer budget = | ||
| options != null ? options.getThinkingBudget() : defaultOptions.getThinkingBudget(); |
There was a problem hiding this comment.
The priority logic here differs from the logic in applyThinking(). Here, if options is provided but has null thinkingBudget, it falls back to defaultOptions.getThinkingBudget(). However, in applyThinking() (lines 392-396), if options is non-null but has null thinkingBudget, it doesn't check defaultOptions.
This inconsistency could lead to unexpected behavior where isThinkingEnabled() returns true based on defaultOptions budget, but then applyThinking() doesn't actually apply anything because options is non-null with null budget.
Consider ensuring consistent priority logic:
Integer budget =
(options != null && options.getThinkingBudget() != null)
? options.getThinkingBudget()
: (defaultOptions != null ? defaultOptions.getThinkingBudget() : null);| options != null ? options.getThinkingBudget() : defaultOptions.getThinkingBudget(); | |
| (options != null && options.getThinkingBudget() != null) | |
| ? options.getThinkingBudget() | |
| : (defaultOptions != null ? defaultOptions.getThinkingBudget() : null); |
| // First, try to use a public getter if it exists | ||
| try { | ||
| Field thinkingField = ChatCompletionMessage.class.getDeclaredField("thinking"); | ||
| thinkingField.setAccessible(true); | ||
| Object thinkingValue = thinkingField.get(message); | ||
|
|
||
| if (thinkingValue != null) { | ||
| // If it's a String, return directly | ||
| if (thinkingValue instanceof String) { | ||
| String strValue = (String) thinkingValue; | ||
| if (!strValue.isEmpty()) { | ||
| return strValue; | ||
| } | ||
| } else { | ||
| // For other types (Optional, wrapper objects, etc.), try toString | ||
| // and verify it's not a default Object.toString() | ||
| String strValue = thinkingValue.toString(); | ||
| if (strValue != null | ||
| && !strValue.isEmpty() | ||
| && !isDefaultObjectToString(strValue)) { | ||
| return strValue; | ||
| } | ||
| } | ||
| } | ||
| } catch (NoSuchFieldException e) { | ||
| // Thinking field may not exist in this SDK version | ||
| log.trace( | ||
| "Thinking field not found in ChatCompletionMessage (SDK may not support it" | ||
| + " yet)"); | ||
| } catch (IllegalAccessException | IllegalArgumentException e) { | ||
| // Field access issues - SDK version compatibility or security restrictions | ||
| log.debug("Unable to access thinking field via reflection: {}", e.getMessage()); | ||
| } | ||
|
|
||
| // If reflection didn't work, return null (thinking not supported or not present) | ||
| return null; | ||
| } catch (Exception e) { | ||
| log.debug("Exception while extracting thinking content: {}", e.getMessage()); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This nested try-catch structure with identical exception handling in both the inner and outer catch blocks is redundant. The outer catch block will never be reached for the caught exceptions (NoSuchFieldException, IllegalAccessException, IllegalArgumentException) because they're already handled by the inner catch. The outer catch should only handle unexpected exceptions not covered by the inner catch.
Consider simplifying to:
protected String extractThinking(ChatCompletionMessage message) {
try {
Field thinkingField = ChatCompletionMessage.class.getDeclaredField("thinking");
thinkingField.setAccessible(true);
Object thinkingValue = thinkingField.get(message);
// ... rest of logic ...
} catch (NoSuchFieldException e) {
log.trace("Thinking field not found in ChatCompletionMessage (SDK may not support it yet)");
} catch (IllegalAccessException | IllegalArgumentException e) {
log.debug("Unable to access thinking field via reflection: {}", e.getMessage());
} catch (Exception e) {
log.debug("Unexpected exception while extracting thinking content: {}", e.getMessage());
}
return null;
}| // First, try to use a public getter if it exists | |
| try { | |
| Field thinkingField = ChatCompletionMessage.class.getDeclaredField("thinking"); | |
| thinkingField.setAccessible(true); | |
| Object thinkingValue = thinkingField.get(message); | |
| if (thinkingValue != null) { | |
| // If it's a String, return directly | |
| if (thinkingValue instanceof String) { | |
| String strValue = (String) thinkingValue; | |
| if (!strValue.isEmpty()) { | |
| return strValue; | |
| } | |
| } else { | |
| // For other types (Optional, wrapper objects, etc.), try toString | |
| // and verify it's not a default Object.toString() | |
| String strValue = thinkingValue.toString(); | |
| if (strValue != null | |
| && !strValue.isEmpty() | |
| && !isDefaultObjectToString(strValue)) { | |
| return strValue; | |
| } | |
| } | |
| } | |
| } catch (NoSuchFieldException e) { | |
| // Thinking field may not exist in this SDK version | |
| log.trace( | |
| "Thinking field not found in ChatCompletionMessage (SDK may not support it" | |
| + " yet)"); | |
| } catch (IllegalAccessException | IllegalArgumentException e) { | |
| // Field access issues - SDK version compatibility or security restrictions | |
| log.debug("Unable to access thinking field via reflection: {}", e.getMessage()); | |
| } | |
| // If reflection didn't work, return null (thinking not supported or not present) | |
| return null; | |
| } catch (Exception e) { | |
| log.debug("Exception while extracting thinking content: {}", e.getMessage()); | |
| return null; | |
| } | |
| Field thinkingField = ChatCompletionMessage.class.getDeclaredField("thinking"); | |
| thinkingField.setAccessible(true); | |
| Object thinkingValue = thinkingField.get(message); | |
| if (thinkingValue != null) { | |
| // If it's a String, return directly | |
| if (thinkingValue instanceof String) { | |
| String strValue = (String) thinkingValue; | |
| if (!strValue.isEmpty()) { | |
| return strValue; | |
| } | |
| } else { | |
| // For other types (Optional, wrapper objects, etc.), try toString | |
| // and verify it's not a default Object.toString() | |
| String strValue = thinkingValue.toString(); | |
| if (strValue != null | |
| && !strValue.isEmpty() | |
| && !isDefaultObjectToString(strValue)) { | |
| return strValue; | |
| } | |
| } | |
| } | |
| } catch (NoSuchFieldException e) { | |
| // Thinking field may not exist in this SDK version | |
| log.trace( | |
| "Thinking field not found in ChatCompletionMessage (SDK may not support it" | |
| + " yet)"); | |
| } catch (IllegalAccessException | IllegalArgumentException e) { | |
| // Field access issues - SDK version compatibility or security restrictions | |
| log.debug("Unable to access thinking field via reflection: {}", e.getMessage()); | |
| } catch (Exception e) { | |
| log.debug("Unexpected exception while extracting thinking content: {}", e.getMessage()); | |
| } | |
| // If reflection didn't work, return null (thinking not supported or not present) | |
| return null; |
| // First, try to use a public getter if it exists | ||
| try { | ||
| Field thinkingField = ChatCompletionMessage.class.getDeclaredField("thinking"); | ||
| thinkingField.setAccessible(true); |
There was a problem hiding this comment.
Using setAccessible(true) on a private field may fail in environments with strict security managers or in Java modules with restricted access. Consider catching SecurityException explicitly and providing a more informative log message about security restrictions preventing reflection access.
Example:
try {
thinkingField.setAccessible(true);
} catch (SecurityException e) {
log.debug("Security restrictions prevent reflection access to thinking field: {}", e.getMessage());
return null;
}| thinkingField.setAccessible(true); | |
| try { | |
| thinkingField.setAccessible(true); | |
| } catch (SecurityException se) { | |
| log.debug("Security restrictions prevent reflection access to thinking field: {}", se.getMessage()); | |
| return null; | |
| } |
| for (Map.Entry<String, String> entry : params.entrySet()) { | ||
| // Skip null values to prevent API errors | ||
| if (entry.getValue() == null) { | ||
| log.debug("Skipped null value for query param '{}'", entry.getKey()); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The code checks for null values but doesn't check for null keys. If a map entry has a null key, this will cause a NullPointerException when passing it to the setter. Consider checking both key and value for robustness.
Consider:
if (entry.getKey() == null || entry.getValue() == null) {
log.debug("Skipped entry with null key or value");
continue;
}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| try { | ||
| // First, try to use a public getter if it exists | ||
| try { | ||
| Field thinkingField = ChatCompletionMessage.class.getDeclaredField("thinking"); |
There was a problem hiding this comment.
Does the OpenAI Java SDK currently support this field?
I checked the latest source code in ChatCompletionMessage.kt but couldn't locate it: https://github.com/openai/openai-java/blob/main/openai-java-core/src/main/kotlin/com/openai/models/chat/completions/ChatCompletionMessage.kt
There was a problem hiding this comment.
Does the OpenAI Java SDK currently support this field?
I checked the latest source code in
ChatCompletionMessage.ktbut couldn't locate it: https://github.com/openai/openai-java/blob/main/openai-java-core/src/main/kotlin/com/openai/models/chat/completions/ChatCompletionMessage.kt
是的 我又去看了sdk。
Chat Completions API 不支持 O 系列推理模型。O1/O3 等推理模型必须使用 Responses API（而非 Chat Completions API），通过 ResponseCreateParams.builder().reasoning(Reasoning.builder().effort(...).budget(...).build()) 传入推理配置。
所以说我前面的修改不对，我重新修改。
There was a problem hiding this comment.
可以看一下目前其他模型通过 OpenAI 转发以后怎么带的 Thinking block，比如 openrouter、dashscope、deepseek 等
There was a problem hiding this comment.
可以看一下目前其他模型通过 OpenAI 转发以后怎么带的 Thinking block，比如 openrouter、dashscope、deepseek 等
如果使用Chat Completions API 是不太可行的，我看官方推荐的也是Responses API。我如果使用OpenAI 用 Responses API → 返回 ThinkingBlock 你觉得这样可以吗？
There was a problem hiding this comment.
Responses API 这个得单独开一个实现了，默认的话还是最好走 chat，这个是兼容性最好的模式
There was a problem hiding this comment.
Responses API 这个得单独开一个实现了，默认的话还是最好走 chat，这个是兼容性最好的模式
OpenAIChatModel（默认）- Chat API 零改动，完全兼容，用户无感知
OpenAIReasoningModel（可选）- Responses API 完全独立，仅供需要推理功能的用户使用
用两个独立的 Model class，
这样才能最大化兼容性（默认用户零改动）和可用性（需要推理的用户能用上） 我是这么认为的。
…o1-thinking-mode-with-budget-configuration-(agentscope-ai#98)
2 participants
AgentScope-Java Version
1.0.2-SNAPSHOT
Description
Background & Purpose
This PR implements support for OpenAI's o1 and o1-mini models with advanced reasoning capabilities, specifically the thinking mode feature. The thinking mode allows models to perform extended reasoning before generating responses, significantly improving accuracy on complex tasks.
Issue: #98 - "Support OpenAI o1 thinking mode with budget configuration"
Changes Made
Core Implementation
OpenAIChatModel (
src/main/java/io/agentscope/core/model/OpenAIChatModel.java)enableThinkingconfiguration parameter (Boolean, supports three states: true/false/null)toolsHelpermember variable for centralized parameter managementapplyThinkingIfAvailable()method to apply thinking configurationisThinkingEnabled()method to check if thinking mode is enableddoStream()to:enableThinking()method to Builder classOpenAIToolsHelper (
src/main/java/io/agentscope/core/formatter/openai/OpenAIToolsHelper.java)applyThinking()method to:{"type": "enabled", "budget_tokens": <number>}applyAdditionalBodyParams(): Filter "thinking" key to prevent user overrideapplyAdditionalQueryParams(): Filter null valuesapplyAdditionalHeaders(): Filter null valuesapplyTools(): Filter null parameter valuesOpenAIResponseParser (
src/main/java/io/agentscope/core/formatter/openai/OpenAIResponseParser.java)extractThinking()method to:isDefaultObjectToString()helper for robust pattern matchingTest Suite (
src/test/java/io/agentscope/core/model/OpenAIThinkingModeTest.java)Key Features
options > defaultOptionsBug Fixes
isThinkingEnabled()- now reuses cached computed valueapplyThinking()to include cases where thinking mode is not appliedTesting & Validation
Test Results:
Test Coverage:
Validation Checklist:
mvn spotless:applyHow to Test
1. Build and Test