Skip to content
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

Add Conversation AI to Java SDK #1235

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

siri-varma
Copy link

@siri-varma siri-varma commented Mar 6, 2025

PR Overview: Conversation AI SDK Integration

This PR introduces the Conversation AI SDK with the Converse API. The implementation is structured into the following categories:

  • Core Logic
  • Models
  • Unit Tests

Core Logic

The core logic is implemented in a key class, as detailed below:

Class Description Details
DaprConversationClient Manages the Conversation API. Handles validations and async calls. - Calls the Conversation API asynchronously.
- Validates inputs and manages job execution.

API Contracts

The following is the method signature for the Dapr Converse API:

Dapr Converse API

This method interacts with the Dapr Converse API.

Mono<DaprConversationResponse> converse(
    String conversationComponentName,
    List<DaprConversationInput> daprConversationInputs,
    String contextId,
    boolean scrubPii,
    double temperature
);
Parameter Description
conversationComponentName Name of the conversation component.
daprConversationInputs List of prompts that are part of the conversation.
contextId Identifier for an existing chat session (e.g., similar to ChatGPT).
scrubPii Whether to scrub personally identifiable information (PII) from responses.
temperature Controls response randomness (lower = more predictable, higher = more creative).
Returns DaprConversationResponse

Models

The SDK follows the builder pattern for constructing models, ensuring cleaner and more maintainable object creation.


Tests

The testing strategy includes:

  • Unit Tests: Validate individual components and business logic.

Issue Reference

We ensure that all PRs are linked to a relevant issue where the problem or feature has been discussed before implementation.

This PR closes the following issue: #1101


Checklist ✅

Please confirm the following before merging:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@siri-varma siri-varma requested review from a team as code owners March 6, 2025 16:51
@siri-varma siri-varma force-pushed the users/svegiraju/conversation-api-2 branch from 68d30ee to 9a8926e Compare March 6, 2025 16:52
@siri-varma
Copy link
Author

siri-varma commented Mar 6, 2025

@salaboy, @artur-ciocanu please let me know what you think. Working on integration and unit tests in the meanwhile

@yaron2
Copy link
Member

yaron2 commented Mar 6, 2025

Great contribution, thanks @siri-varma

@cicoyle cicoyle added this to the v1.15 milestone Mar 6, 2025
@siri-varma siri-varma force-pushed the users/svegiraju/conversation-api-2 branch from 9a8926e to 4cb6031 Compare March 6, 2025 19:10
Signed-off-by: Siri Varma Vegiraju <[email protected]>
Signed-off-by: sirivarma <[email protected]>
@siri-varma siri-varma force-pushed the users/svegiraju/conversation-api-2 branch from 4cb6031 to 75f487f Compare March 6, 2025 19:11
@salaboy
Copy link
Contributor

salaboy commented Mar 6, 2025

this is great @siri-varma ! I will start reviewing soon!

Signed-off-by: sirivarma <[email protected]>
@siri-varma siri-varma force-pushed the users/svegiraju/conversation-api-2 branch from bf70fd4 to 88fe015 Compare March 7, 2025 04:27
@siri-varma
Copy link
Author

siri-varma commented Mar 7, 2025

For integration tests, we must spin up a mock for third party LLM endpoint. Do we have similar patterns in the current setup? (where dapr is talking to a third party api and receiving responses)

@siri-varma siri-varma changed the title WIP: Add Conversation AI to Java SDK Add Conversation AI to Java SDK Mar 8, 2025
@salaboy salaboy mentioned this pull request Mar 10, 2025
import java.util.Map;
import java.util.function.Consumer;

public class DaprConversationClient implements AutoCloseable, DaprAiClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

@siri-varma I assume that you know that we had a previewClient, why is this introducing a new separated client for the conversation API? I would assume that we want to include this in PreviewClient first. @yaron2 we might need your help here.

Copy link
Contributor

Choose a reason for hiding this comment

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

and sorry for the confusion if the PreviewClient is an old thing that we are not supposed to use anymore. As the workflows now have their own separate client, this might be the way to go.

Copy link
Author

@siri-varma siri-varma Mar 13, 2025

Choose a reason for hiding this comment

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

Just to clarify, we are fine with current approach right ?

sdk-ai/pom.xml Outdated
<artifactId>dapr-sdk-ai</artifactId>
<packaging>jar</packaging>
<version>1.15.0-SNAPSHOT</version>
<name>dapr-sdk-ai</name>
Copy link
Contributor

Choose a reason for hiding this comment

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

@siri-varma should we call it conversation? instead of ai? as ai is too generic

Copy link
Author

Choose a reason for hiding this comment

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

Changed to conversation

sdk-ai/pom.xml Outdated
</executions>
</plugin>
<plugin>
<groupId>org.jacoco</groupId>
Copy link
Contributor

@salaboy salaboy Mar 13, 2025

Choose a reason for hiding this comment

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

@siri-varma please include the nexus plugin as shown in here: https://github.com/dapr/java-sdk/blob/master/dapr-spring/pom.xml#L95

Copy link
Author

Choose a reason for hiding this comment

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

added the nexus plugin

@salaboy
Copy link
Contributor

salaboy commented Mar 13, 2025

@siri-varma this is looking good, I left some comments that basically highlight my doubts about some JDK general topics. But this is looking really good, are you planning to add integration tests inside the sdk-tests too? We have some Spring Boot tests using Testcontainers, I was thinking that you might want to test the APIs against a local model running in a docker container to see that everything works as expected.

@siri-varma
Copy link
Author

siri-varma commented Mar 14, 2025

@salaboy I have tested the code by running it locally and using my OpenAI API Key.

Regarding the integration test, I think we will need an LLM model for daprd to communicate with, which unfortunately are huge in size. That is why I did not add the integration tests

Will get back to you on the integ tests

@salaboy
Copy link
Contributor

salaboy commented Mar 14, 2025

Yeah check this https://java.testcontainers.org/modules/ollama/

@siri-varma
Copy link
Author

@salaboy thank you for providing the testcontainer urls.

Like how we have url overrides for Scheduler, I was looking one for conversation too because we will have to point dapr to the local llm model. But could not find any

I tried looking in the below places and could not find anything. https://github.com/dapr/dapr/blob/2e7c61e933099b9d40fcdefbd57cc3e81069915a/pkg/injector/annotations/annotations.go#L73
https://github.com/dapr/dapr/blob/2e7c61e933099b9d40fcdefbd57cc3e81069915a/pkg/injector/consts/consts.go#L51
https://docs.dapr.io/reference/arguments-annotations-overview/

@salaboy or @cicoyle Would you folks know about this ?

@artur-ciocanu
Copy link
Contributor

@siri-varma thanks a lot for your contribution. I have reviewed your PR and I have a few comments similar to #1255:

  • Let's make sure the Conversation API is part of the DaprPreviewClient
  • Let's make sure that we use the Alpha1 suffix for the Conversation API
  • Let's make sure we keep everything under sdk module and we don't create a separate module for Conversation API
  • For the Conversation API converse let's make sure we add private methods to validate all the input parameters, before invoking the GRPC API
  • For Conversation API converse method let's ensure we keep number of parameters to a minimum, having to pass 5 parameters is too much. I think a better option is to have a builder for ConversationRequest which should be part of the model classes for DaprClient. This way we can have sane defaults, but allow users to override some of the values if desired.

CC: @salaboy @cicoyle

Copy link
Contributor

@artur-ciocanu artur-ciocanu left a comment

Choose a reason for hiding this comment

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

@siri-varma thanks a lot for your contribution, I have left a longer comment with some of my thoughts.

Please take a look and let me know what you think.

Signed-off-by: sirivarma <[email protected]>
@siri-varma siri-varma force-pushed the users/svegiraju/conversation-api-2 branch from 115b5e7 to 842b0e7 Compare March 18, 2025 15:10
Signed-off-by: sirivarma <[email protected]>
Signed-off-by: siri-varma <[email protected]>
@siri-varma
Copy link
Author

@siri-varma thanks a lot for your contribution. I have reviewed your PR and I have a few comments similar to #1255:

  • Let's make sure the Conversation API is part of the DaprPreviewClient
  • Let's make sure that we use the Alpha1 suffix for the Conversation API
  • Let's make sure we keep everything under sdk module and we don't create a separate module for Conversation API
  • For the Conversation API converse let's make sure we add private methods to validate all the input parameters, before invoking the GRPC API
  • For Conversation API converse method let's ensure we keep number of parameters to a minimum, having to pass 5 parameters is too much. I think a better option is to have a builder for ConversationRequest which should be part of the model classes for DaprClient. This way we can have sane defaults, but allow users to override some of the values if desired.

CC: @salaboy @cicoyle

@artur-ciocanu Addressed all the comments here as well.
@salaboy Added Integration tests too.

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.

5 participants