feat: [OpenAI] Stable Convenience API for OpenAI #305
Conversation
# Conflicts: # foundation-models/openai/src/main/java/com/sap/ai/sdk/foundationmodels/openai/OpenAiClient.java
…-openai # Conflicts: # foundation-models/openai/src/main/java/com/sap/ai/sdk/foundationmodels/openai/OpenAiChatCompletionDelta.java # foundation-models/openai/src/test/java/com/sap/ai/sdk/foundationmodels/openai/OpenAiClientTest.java
# Conflicts: # foundation-models/openai/src/test/java/com/sap/ai/sdk/foundationmodels/openai/OpenAiClientTest.java
- createDTO need to be more limited in access
…onse only - Reasoning: to remove casting and include future changes Note: Dependent on prev manual change of generated model class
…able-conv-generated-openai
- No tooling/function api yet - No user-image/user-text message api
…able-conv-generated-openai # Conflicts: # foundation-models/openai/pom.xml
rpanackal
changed the title
- No args constructor is invalid as empty list of messages in payload is invalid
…ai' into chore/stable-conv-generated-openai
- java docs and unit tests - message convenience missing multipart
| * @throws OpenAiClientException | ||
| */ | ||
| @Nonnull | ||
| public OpenAiChatCompletionOutput chatCompletion(@Nonnull final OpenAiChatCompletionPrompt prompt) |
There was a problem hiding this comment.
(Major)
I like the convenient options you offer to the user!
But I think we should discuss API design, your branch assumes four methods for regular chat completion and four additional methods for streamed chat completion. That feels a bit much. In orchestration it's three/two - which is also not great.
Currently I'm considering to pitch a two layer-approach similar to what we have in SAP Cloud SDK for OData v2/v4 -> Generic OData Client. Unfortunately we don't have it nicely-documented. But the gist is: You can use the high-level API (convenience API) on the regular client, but if you know what you're doing and want to use the low-level API (generated model classes), you would access a second API level via new OpenAiClient(...).custom() that only allows model classes. Though I'm not sure about method naming here.
The API quality was improved significantly that way.
I'm open to have this discussion in a separate PR / ticket.
There was a problem hiding this comment.
I felt the same way that there are too many chatCompletion methods and agree that we need to find a way to minimise this.
| @Beta | ||
| @Value | ||
| @AllArgsConstructor(onConstructor = @__({@JsonCreator}), access = AccessLevel.PROTECTED) | ||
| public class OpenAiError implements ClientError { |
There was a problem hiding this comment.
(Comment/Opinion)
I'm unhappy about the interface name "ClientError", since "Error" is reserved for non-recoverable exceptions leading to application shutdown.
No action required.
...dation-models/openai/src/main/java/com/sap/ai/sdk/foundationmodels/openai/OpenAiMessage.java
Show resolved
Hide resolved
…pt code minimisation
…ai' into chore/stable-conv-generated-openai
…able-conv-generated-openai # Conflicts: # sample-code/spring-app/src/main/java/com/sap/ai/sdk/app/controllers/OpenAiController.java
| /** Factory class for creating OpenAI Embedding Requests. */ | ||
| @Accessors(fluent = true) | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| public class OpenAiEmbeddingsRequestFactory { |
There was a problem hiding this comment.
(Minor)
I don't think we'll support this public API in that shape:
- I think it should be
@Beta - "Factory" name is discouraged
- (Instead) rather have convenience class for embedding-create-request, that can be instantiated via String(s). Then we wouldn't need this class.
There was a problem hiding this comment.
Apart from the naming, can you expand a bit on why this is not a good approach?
My initial approach was what you suggested but later moved to the current state for the following reasons:
- Avoid duplication of model class
- User can call setters for generated model fields on top
- Avoid adding an additional method in client (getting bloated) for accepting high level api object for embedding
- In case, embedding become a bigger topic, we can add further static factory methods that returns a new convenience objects.
I might be missing some issue(s) that you are seeing.
...ain/java/com/sap/ai/sdk/foundationmodels/openai/model2/ChatCompletionFunctionCallOption.java
Show resolved
Hide resolved
…ai' into chore/stable-conv-generated-openai
- Stop maintaining defaults for request parameters - remove extra serialised data from test
- keep completion and embedding calls with old api in client - Keep tests - improve test for reusability
…ChatCompletionResponse access
…able-conv-generated-openai
rpanackal
changed the title
rpanackal
force-pushed
the
chore/openai-gen-code
branch
from
ec7e58d to
18b795b
Compare
|
This PR has been split into two new ones
This conversation is now closed, but continued on the new PRs. |
Context
AI/ai-sdk-java-backlog#172.
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
orchestrationscenario, the ai model itself is selected on a per request level. While, infoundation-modelscenario, we have a deployment specific to a model, while parameters alone (eg: temperature) are altered per request.=> parameters scoping under a model, like in
OrchestrationAiModelclass, is not sensible in OpenAI convenience API.Feature scope:
OpenAiChatCompletionDeltaDefinition of Done
Aligned changes with the JavaScript SDK