Skip to content

feat: [Orchestration] reasoning content #939

Open
n-o-u-r-h-a-n wants to merge 10 commits into
mainfrom
reasoning-content
Open

feat: [Orchestration] reasoning content #939
n-o-u-r-h-a-n wants to merge 10 commits into
mainfrom
reasoning-content

Conversation

@n-o-u-r-h-a-n

@n-o-u-r-h-a-n n-o-u-r-h-a-n commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Context

AI/ai-sdk-java-backlog#412.

As a user, I would like to see the thinking processes when using the reasoning models, in addition to the final outcome. This is relevant for synchronous and streaming use-cases.

The harmonized API of orchestration supports this feature (see link above), so the SDK should also follow.

Feature scope:

  • supported reasoning content using model classes code
  • added sample code as well as updated controller and index page
  • added e2e and unit tests

Definition of Done

  • Functionality scope stated & covered
  • Tests cover the scope above
  • Error handling created / updated & covered by the tests above
  • Aligned changes with the JavaScript SDK
  • Documentation updated
  • Release notes updated

Comment thread orchestration/src/main/java/com/sap/ai/sdk/orchestration/AssistantMessage.java Outdated
@Nonnull final OrchestrationModuleConfig config,
@Nonnull final Consumer<String> onAnswerChunk,
@Nonnull final Consumer<String> onReasoningChunk,
@Nonnull final OrchestrationModuleConfig... fallbackConfigs)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how I feel about those 2 new public methods that only add a forEach on an existing public stream method.
Reasoning is a very specific feature and if we add public API for every single feature we would have way to many methods.
Also the customers could add the forEach themselves and it would allow more configurability.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

This would be the alternative solution which users will do if we decided to remove the public API.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this code snippet from the service class?

While I see your point that this is a lot of code for users to write, I would also agree with Charles that putting this into a convenience method is maybe too much. The reason for this is that I think the level of complexity the users have to understand (5 different inputs in the method, why 2 different consumers, asynchronicity,...) compared to the fact that we do not know how useful this actually will be for users (maybe they don't want to be streaming reasoning in this way or want to do something with it before parsing it to the consumer) does not match up. Usability is probably something we could argue over but the current complexity of using this convenience method I think is not "convenient" enough.

Plus, we need to publish a lot of new API for this (whole new public class ReasoningAccumulator that we otherwise don't need as far as I see). Since we would then forever need to kepp mantaining this I think it is not worth it.

@ZhongpinWang ZhongpinWang left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CharlesDuboisSAP Regarding the signature, it does not need a convenience to get it out. It only needs to be as part of the message history and be sent back together in the next LLM call for verification reasons. So IMO providing a convenience to access reasoning content text would be enough.

Comment thread orchestration/src/main/java/com/sap/ai/sdk/orchestration/AssistantMessage.java Outdated
Comment thread orchestration/src/main/java/com/sap/ai/sdk/orchestration/AssistantMessage.java Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants