-
Notifications
You must be signed in to change notification settings - Fork 8
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
Openapi generator orchestration #146
Conversation
- Introduce a generic fallback and LLMModuleResult specific deserializers. - Generator ignoring all interfaces - Adapt jackson annotations
# Conflicts: # orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationClient.java # orchestration/src/test/java/com/sap/ai/sdk/orchestration/OrchestrationUnitTest.java # sample-code/spring-app/src/main/java/com/sap/ai/sdk/app/controllers/OrchestrationController.java # sample-code/spring-app/src/test/java/com/sap/ai/sdk/app/controllers/OrchestrationTest.java
orchestration/src/main/java/com/sap/ai/sdk/orchestration/client/model/LLMModuleResult.java
Show resolved
Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationClient.java
Outdated
Show resolved
Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/LLMModuleResultDeserializer.java
Outdated
Show resolved
Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/client/model/FilterConfig.java
Outdated
Show resolved
Hide resolved
…SAP/ai-sdk-java into openapi-generator-orchestration # Conflicts: # orchestration/src/main/java/com/sap/ai/sdk/orchestration/client/model/AzureContentSafetyFilterConfig.java
- Make deserializer lenient to allowed values
orchestration/src/main/java/com/sap/ai/sdk/orchestration/LLMModuleResultDeserializer.java
Outdated
Show resolved
Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/NoTypeInfoMixin.java
Outdated
Show resolved
Hide resolved
orchestration/src/test/java/com/sap/ai/sdk/orchestration/LLMModuleResultDeserializerTest.java
Show resolved
Hide resolved
assertThat( | ||
((LLMModuleResultSynchronous) result.getOrchestrationResult()) | ||
.getChoices() | ||
.get(0) | ||
.getMessage() | ||
.getContent()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Question/Major)
If I understand correctly, the PR changes public API in a way, that requires users to cast a result object to a specific class, before being able to read the response content. Is that correct? Is that the expected result?
Please also update the /docs/guides/
orchestration document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this will be fixed by either AI/ai-sdk-java-backlog#110 or https://github.tools.sap/AI/llm-orchestration/pull/753
.../src/main/java/com/sap/ai/sdk/orchestration/client/model/SearchDocumentKeyValueListPair.java
Outdated
Show resolved
Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/ConfigToRequestTransformer.java
Outdated
Show resolved
Hide resolved
* Simplifications and cleanup * Remove parsing for streaming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful stuff, LGTM
Context
AI/ai-sdk-java-backlog#99.
Support oneOf from Orchestration spec using OpenAPI code generator.
Avoid template and spec modification.
Feature scope:
Definition of Done
Error handling created / updated & covered by the tests aboveAligned changes with the JavaScript SDKDocumentation updatedRelease notes updated