-
Notifications
You must be signed in to change notification settings - Fork 410
fix(streaming): rationalize thinking retrieval vs model budget #1853
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e3cea71
2dc25c0
87ab47e
b497c7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ import com.embabel.agent.spi.support.springai.toSpringAiMessage | |
| import com.embabel.agent.spi.support.springai.toSpringToolCallbacks | ||
| import com.embabel.chat.Message | ||
| import com.embabel.common.ai.converters.streaming.StreamingJacksonOutputConverter | ||
| import com.embabel.common.ai.model.Thinking | ||
| import com.embabel.common.core.streaming.StreamingEvent | ||
| import org.slf4j.LoggerFactory | ||
| import org.springframework.ai.chat.messages.SystemMessage | ||
|
|
@@ -234,6 +235,8 @@ internal class StreamingChatClientOperations( | |
| ): Flux<O> { | ||
| return doTransformObjectStreamInternal( | ||
| messages = messages, | ||
| // Object-only stream: leave Interaction thinking as-is. Format instructions follow | ||
| // Thinking.extractThinking (application-level), not provider tokenBudget (Thinking.enabled). | ||
| interaction = interaction, | ||
| outputClass = outputClass, | ||
| llmRequestEvent = llmRequestEvent, | ||
|
|
@@ -295,14 +298,36 @@ internal class StreamingChatClientOperations( | |
| ): Flux<StreamingEvent<O>> { | ||
| return doTransformObjectStreamInternal( | ||
| messages = messages, | ||
| interaction = interaction, | ||
| // *WithThinking*: ensure Interaction carries application-level Thinking | ||
| // (extractThinking). Format instructions follow that flag — no separate SPI param. | ||
| interaction = withApplicationLevelThinkingIfNecessary(interaction), | ||
| outputClass = outputClass, | ||
| llmRequestEvent = llmRequestEvent, | ||
| agentProcess = agentProcess, | ||
| action = action, | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Ensure [Thinking.extractThinking] is set on the interaction for application-level | ||
| * (prompt-instructed) thinking streams. Preserves any existing provider budget | ||
| * ([Thinking.enabled] / [Thinking.tokenBudget]) via [Thinking.applyExtraction]. | ||
| * | ||
| * This is *not* LLM-native reasoning (provider thinking channels — see #1716). | ||
| */ | ||
| private fun withApplicationLevelThinkingIfNecessary(interaction: LlmInteraction): LlmInteraction { | ||
| val existing = interaction.llm.thinking | ||
| val thinking = when (existing) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can dup code be avoided? |
||
| null, Thinking.NONE -> Thinking.withExtraction() | ||
| else -> if (existing.extractThinking) existing else existing.applyExtraction() | ||
| } | ||
| return if (thinking === existing) { | ||
| interaction | ||
| } else { | ||
| interaction.copy(llm = interaction.llm.withThinking(thinking)) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Internal unified streaming implementation - workhorse -that handles the complete transformation pipeline. | ||
| * | ||
|
|
@@ -330,6 +355,9 @@ internal class StreamingChatClientOperations( | |
| * **Performance Characteristics:** | ||
| * - Streaming-friendly: no blocking operations | ||
| * | ||
| * Prompt thinking format follows [Thinking.extractThinking] on [interaction] (application-level). | ||
| * Provider model budget remains [Thinking.enabled] / [Thinking.tokenBudget] and is independent. | ||
| * | ||
| * @return Unified Flux<StreamingEvent<O>> that public methods can filter as needed | ||
| */ | ||
| private fun <O> doTransformObjectStreamInternal( | ||
|
|
@@ -348,6 +376,9 @@ internal class StreamingChatClientOperations( | |
| // Chat Options, additional potential option "streaming" | ||
| val chatOptions = requireSpringAiLlm(llm).convertOptions(interaction.llm) | ||
|
|
||
| // Application-level thinking format: Thinking.extractThinking (not provider tokenBudget / enabled). | ||
| val includeApplicationLevelThinking = interaction.llm.thinking?.extractThinking == true | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you need this "val" , or just interaction.llm.thinking?.extractThinking == true use as is |
||
|
|
||
| // Spring AI 2.0's StreamingJacksonOutputConverter requires T : Any; | ||
| // erase O via Class<Any> for the construction, cast result back at use sites. | ||
| @Suppress("UNCHECKED_CAST") | ||
|
|
@@ -357,7 +388,7 @@ internal class StreamingChatClientOperations( | |
| clazz = outputClassAny, | ||
| objectMapper = chatClientLlmOperations.objectMapper, | ||
| fieldFilter = interaction.fieldFilter, | ||
| thinkingEnabled = interaction.llm.thinking?.enabled ?: false, | ||
| thinkingEnabled = includeApplicationLevelThinking, | ||
| ) as StreamingJacksonOutputConverter<O> // signature compatibility for downstream Flux<O>/StreamingEvent<O> uses | ||
|
|
||
| // Build prompt using helper methods, including streaming format instructions | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ import com.embabel.agent.spi.support.guardrails.validateUserInput | |
| import com.embabel.chat.Message | ||
| import com.embabel.chat.UserMessage | ||
| import com.embabel.common.ai.converters.streaming.StreamingJacksonOutputConverter | ||
| import com.embabel.common.ai.model.Thinking | ||
| import com.embabel.common.core.streaming.StreamingEvent | ||
| import tools.jackson.databind.ObjectMapper | ||
| import org.slf4j.LoggerFactory | ||
|
|
@@ -152,6 +153,7 @@ internal class StreamingLlmOperationsImpl( | |
| ): Flux<O> { | ||
| return doTransformObjectStreamInternal( | ||
| messages = messages, | ||
| // Object-only: format instructions follow Thinking.extractThinking on Interaction. | ||
| interaction = interaction, | ||
| outputClass = outputClass, | ||
| llmRequestEvent = llmRequestEvent, | ||
|
|
@@ -172,14 +174,32 @@ internal class StreamingLlmOperationsImpl( | |
| ): Flux<StreamingEvent<O>> { | ||
| return doTransformObjectStreamInternal( | ||
| messages = messages, | ||
| interaction = interaction, | ||
| // *WithThinking*: ensure Interaction has application-level Thinking.extractThinking. | ||
| interaction = withApplicationLevelThinkingIfNecessary(interaction), | ||
| outputClass = outputClass, | ||
| llmRequestEvent = llmRequestEvent, | ||
| agentProcess = agentProcess, | ||
| action = action, | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Ensure [Thinking.extractThinking] is set for application-level (prompt-instructed) thinking. | ||
| * Preserves provider budget via [Thinking.applyExtraction]. Not LLM-native reasoning (#1716). | ||
| */ | ||
| private fun withApplicationLevelThinkingIfNecessary(interaction: LlmInteraction): LlmInteraction { | ||
| val existing = interaction.llm.thinking | ||
| val thinking = when (existing) { | ||
| null, Thinking.NONE -> Thinking.withExtraction() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 3rd occurrence of dup code |
||
| else -> if (existing.extractThinking) existing else existing.applyExtraction() | ||
| } | ||
| return if (thinking === existing) { | ||
| interaction | ||
| } else { | ||
| interaction.copy(llm = interaction.llm.withThinking(thinking)) | ||
| } | ||
| } | ||
|
|
||
| // ======================================== | ||
| // Internal implementation | ||
| // ======================================== | ||
|
|
@@ -191,6 +211,9 @@ internal class StreamingLlmOperationsImpl( | |
| * 1. Raw LLM chunks from [LlmMessageStreamer] | ||
| * 2. Line buffering via [rawChunksToLines] | ||
| * 3. Event generation via [StreamingJacksonOutputConverter] | ||
| * | ||
| * Prompt thinking format follows [Thinking.extractThinking] on [interaction]. | ||
| * Provider model budget remains [Thinking.enabled] / [Thinking.tokenBudget]. | ||
| */ | ||
| private fun <O> doTransformObjectStreamInternal( | ||
| messages: List<Message>, | ||
|
|
@@ -204,14 +227,15 @@ internal class StreamingLlmOperationsImpl( | |
| // Create converter for JSONL parsing. | ||
| // Spring AI 2.0's StreamingJacksonOutputConverter requires T : Any; | ||
| // erase O via Class<Any> for the construction, cast back for downstream Flux<O>/StreamingEvent<O>. | ||
| val includeApplicationLevelThinking = interaction.llm.thinking?.extractThinking == true | ||
| @Suppress("UNCHECKED_CAST") | ||
| val outputClassAny = outputClass as Class<Any> | ||
| @Suppress("UNCHECKED_CAST") | ||
| val streamingConverter = StreamingJacksonOutputConverter<Any>( | ||
| clazz = outputClassAny, | ||
| objectMapper = objectMapper, | ||
| fieldFilter = interaction.fieldFilter, | ||
| thinkingEnabled = interaction.llm.thinking?.enabled ?: false, | ||
| thinkingEnabled = includeApplicationLevelThinking, | ||
| ) as StreamingJacksonOutputConverter<O> | ||
|
|
||
| // Build prompt contributions with streaming format instructions | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain the logic and how it actually behaves