Skip to content

Conversation

@rpanackal
Copy link
Member

@rpanackal rpanackal commented Feb 6, 2025

Context

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

PR split out of #305

We would like start using generated model classes for OpenAI module to keep up with spec changes easier.

Previously, thanks to Charles, we had a beautiful convenience API+model classes in place. But, we are migrating away from current convenience API to achieve parity with orchestration module. This equates to breaking changes.

100% convenience parity seems difficult due how both modules should treat ai model parameter configuration.

Model Parameter Configuration

Under orchestration scenario, the ai model itself is selected on a per request level. While, in foundation-model scenario, we have a deployment specific to a model, while parameters alone (eg: temperature) are altered per request.

=> parameters scoping under a model, like in OrchestrationAiModel class, is not sensible in OpenAI convenience API.

Feature scope:

  • Add convenient request, response and error classes
  • Add convenient message classes (to match orchestration)
  • Add new chatCompletion and streamChatCompletion methods in client
  • Request class made immutable for config reuse and no getters (@lombok.Value type)
  • Response class made immutable and no setters
  • toCreateChatCompletionRequestMessage, ie. toDTO equivalent, is non-public and moved to new utility class
  • Code reduction in OpenAiChatCompletionDelta
  • Reduce num of mixins
  • Adapt Tests
    • Replicate unit and e2e for both new and old api
    • New service class introduced in test
      • Old service class still in demo
    • Move common stubbing logic into a new parent test (consistent stubbing)
    • Add test for tool call
    • increase coverage for all api

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 self-assigned this Feb 6, 2025
.tools(null)
.parallelToolCalls(null);

return OpenAiClient.forModel(GPT_4O).chatCompletion(request);
Copy link
Contributor

Choose a reason for hiding this comment

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

(Comment)

I understand that we're waiting for orchestration multi-modal chat-message PR to be merged. Once available, we can update the openai convenience API. then this code here (based on generated classes) will hopefully not be necessary anymore.

@rpanackal rpanackal force-pushed the feat/stable-conv-openai branch from bb53787 to 2b4b12b Compare February 11, 2025 10:45
Base automatically changed from chore/openai-gen-code to main February 14, 2025 10:56
- 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
@rpanackal rpanackal force-pushed the feat/stable-conv-openai branch from b3ead80 to f33bfad Compare February 17, 2025 14:14
@rpanackal rpanackal added the please-review Request to review a pull-request label Feb 17, 2025
@rpanackal rpanackal force-pushed the feat/stable-conv-openai branch from 587c307 to d4a03e5 Compare February 17, 2025 17:17
- 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
null,
null,
null,
null);
Copy link
Contributor

@newtork newtork Feb 18, 2025

Choose a reason for hiding this comment

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

(Comment)

I was investigating whether @NoArgsConstructor would work. And yes it does, but unfortunately we would then need to (1) make messages a mutable list and (2) write a wither for it manually, which would invoke all-args constructor again. Therefore no code would be saved.

On a second note: In the future, we could add DEFAULT_XYZ here instead of null. They would be default values for the model fields - currently all of them null / undefined anyway. But it could be useful in the future, and for documentation purpose. We may consider this in future PRs, not part of your branch.

Copy link
Member Author

@rpanackal rpanackal Feb 18, 2025

Choose a reason for hiding this comment

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

I considered including the default but

  1. adding default values means the withXYZ will not be generated for @Value -> final fields.
  2. also, keeping track of defaults and as it may change for the api
    • could load from the generated model object itself - but maybe not so neat
  3. Say, we set default value for a nullable property -> It would now be part of all serialised json values and does not scale well for every property and message frequency.

I don't believe any of them are dealbreakers but still hesitant.

@rpanackal rpanackal merged commit 285960b into main Feb 19, 2025
6 checks passed
@rpanackal rpanackal deleted the feat/stable-conv-openai branch February 19, 2025 09:43
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