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

feat: [OpenAI] Integrate Messaging Convenience #326

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

rpanackal
Copy link
Member

@rpanackal rpanackal commented Feb 10, 2025

Context

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

We have newly introduced a messaging API for orchestration. We would like to adapt the convenience to OpenAI module as well.

Feature scope:

  • Minimum - include convenience for user, assistant and system messages matching orchestration
  • Add tool type messages

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

@rpanackal rpanackal changed the base branch from main to feat/stable-conv-openai February 10, 2025 16:21
@rpanackal rpanackal force-pushed the feat/stable-conv-openai branch from bb53787 to 2b4b12b Compare February 11, 2025 10:45
@rpanackal rpanackal self-assigned this Feb 11, 2025
@rpanackal rpanackal force-pushed the feat/openai-conv-api-msg branch from 60f9d72 to c99cbb8 Compare February 11, 2025 10:54
Roshin Rajan Panackal added 21 commits February 17, 2025 15:14
- controller linked to new openai service only
- Move Jackson object initialization to field declaration
- update `streamChatCompletionDeltas` in `NewOpenAiService` to use basic string message
- `@deprecated` tag on deprecated `chatCompletion` doc
- `@Deprecated` annotation on embedding api in client
- `OpenAiController` streamChatCompletionDeltas emits usage
- make jackson mixin package private
- Use Old OpenAiService
- Adapt tests
@rpanackal rpanackal force-pushed the feat/stable-conv-openai branch from b3ead80 to f33bfad Compare February 17, 2025 14:14
Roshin Rajan Panackal added 7 commits February 18, 2025 14:15
- Enums over string values in test
- missed getContent assertion
- replicate history test in old api test
- Remove `@Value` from OpenAiChatCompletionDelta
…r visibility

- follow createX naming format over toX.
- move openai object mapper construction logic to utility method
- make OpenAiChatCompletionDelta constructor access package private
- remove docs for overriding methods
Base automatically changed from feat/stable-conv-openai to main February 19, 2025 09:43
Roshin Rajan Panackal added 8 commits February 19, 2025 17:12
- Add user, assistant and system conv on par with orchestration
- Adapt sample app service class for image input (new api)
- Adapt toDTO to account for OpenAI gen model architecture
- Throw on unsupported content item
- improve code quality user message to DTO mapper
- Add unit tests for messaging convenience api
@rpanackal rpanackal force-pushed the feat/openai-conv-api-msg branch from 538545c to 11c93c1 Compare February 19, 2025 17:03
Roshin Rajan Panackal added 2 commits February 20, 2025 10:31
…nai-conv-api-msg

# Conflicts:
#	foundation-models/openai/src/main/java/com/sap/ai/sdk/foundationmodels/openai/OpenAiAssistantMessage.java
#	foundation-models/openai/src/main/java/com/sap/ai/sdk/foundationmodels/openai/OpenAiMessage.java
#	foundation-models/openai/src/main/java/com/sap/ai/sdk/foundationmodels/openai/OpenAiSystemMessage.java
#	foundation-models/openai/src/main/java/com/sap/ai/sdk/foundationmodels/openai/OpenAiUserMessage.java
#	sample-code/spring-app/src/test/java/com/sap/ai/sdk/app/controllers/NewOpenAiTest.java
#	sample-code/spring-app/src/test/java/com/sap/ai/sdk/app/services/NewOpenAiService.java
@rpanackal rpanackal added the please-review Request to review a pull-request label Feb 20, 2025
@@ -3,34 +3,23 @@
import static com.sap.ai.sdk.foundationmodels.openai.OpenAiModel.GPT_35_TURBO;
Copy link
Contributor

Choose a reason for hiding this comment

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

This service class is in the test directory. Also I thought we wanted to remove NewXXX as a name

Copy link
Member Author

@rpanackal rpanackal Feb 21, 2025

Choose a reason for hiding this comment

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

The service class was intentionally moved to test dir, so we keep the e2e testing of the new api but not yet present it in the sample app.

I am open to any naming suggestions. eg:
OpenAiServiceV2, OpenAiV2Test
OpenAiServiceBeta, OpenAiBetaTest
OpenAiServiceGenerated, OpenAiGeneratedTest

Copy link
Member Author

@rpanackal rpanackal Feb 21, 2025

Choose a reason for hiding this comment

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

I have opted of OpenAiServiceV2 and OpenAiV2Test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please-review Request to review a pull-request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants