-
Notifications
You must be signed in to change notification settings - Fork 16
feat: [Orchestration] Support for Orchestration config persistence #697
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
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # docs/release_notes.md
orchestration/src/main/java/com/sap/ai/sdk/orchestration/ConfigToRequestTransformer.java
Outdated
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/OrchestrationConfigReference.java
Show resolved
Hide resolved
.../prompt-registry/src/main/java/com/sap/ai/sdk/prompt/registry/OrchestrationConfigClient.java
Show resolved
Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationConfigReference.java
Outdated
Show resolved
Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationConfigReference.java
Show resolved
Hide resolved
CharlesDuboisSAP
left a comment
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.
Looking good 😎
sample-code/spring-app/src/main/java/com/sap/ai/sdk/app/services/OrchestrationService.java
Outdated
Show resolved
Hide resolved
sample-code/spring-app/src/main/java/com/sap/ai/sdk/app/services/OrchestrationService.java
Outdated
Show resolved
Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/ConfigToRequestTransformer.java
Outdated
Show resolved
Hide resolved
sample-code/spring-app/src/main/java/com/sap/ai/sdk/app/services/OrchestrationService.java
Outdated
Show resolved
Hide resolved
# Conflicts: # docs/release_notes.md # sample-code/spring-app/src/main/java/com/sap/ai/sdk/app/controllers/PromptRegistryController.java # sample-code/spring-app/src/test/java/com/sap/ai/sdk/app/controllers/PromptRegistryTest.java
| * parameters. | ||
| */ | ||
| @Nonnull | ||
| OrchestrationPrompt templateParameters(@Nonnull final Map<String, String> templateParameters) { |
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.
I need this to be able to set the template parameters in OrchestrationConfigReference.templateParameters()
| @AllArgsConstructor(access = AccessLevel.PRIVATE) | ||
| @Getter(AccessLevel.PACKAGE) | ||
| @Beta | ||
| public class OrchestrationConfigReference { |
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.
I put the prompt needed to call a config by reference into this class now to make it more convenient for the user.
CharlesDuboisSAP
left a comment
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.
LGTM
orchestration/src/main/java/com/sap/ai/sdk/orchestration/ConfigToRequestTransformer.java
Show resolved
Hide resolved
| @Beta | ||
| @Nonnull | ||
| public OrchestrationChatResponse chatCompletionUsingReference( | ||
| @Nonnull final OrchestrationConfigReference reference) { |
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)
I'm confused. Wouldn't we want (optional) prompt arguments?
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.
I guess we expect the usage via prompt-parameters
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.
All the necessary (and possible) options are now stored in the OrchestrationConfigReference object.
| public OrchestrationConfigReference templateParameters( | ||
| @Nonnull final Map<String, String> templateParameters) { | ||
| prompt.templateParameters(templateParameters); | ||
| return this; |
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.
(Minor)
You are using shallow-immutability then? Can be acceptable, just a little surprising to me.
| .templateParameters(params); | ||
| final var response = client.chatCompletionUsingReference(reference); | ||
|
|
||
| final String expectedRequest = fileLoaderStr.apply("orchConfigByRequestHistoryParams.json"); |
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.
(Comment)
Looks good!
| .addMixIn(OutputFilterConfig.class, JacksonMixin.OutputFilter.class) | ||
| .addMixIn(InputFilterConfig.class, JacksonMixin.InputFilter.class); | ||
| Iterables.filter(rt.getMessageConverters(), MappingJackson2YamlHttpMessageConverter.class) | ||
| .forEach(converter -> converter.setObjectMapper(yamlMapper)); |
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)
Why do we need a MappingJackson2YamlHttpMessageConverter?
| .template( | ||
| UserChatMessage.create() | ||
| .content( | ||
| new UserChatMessageContent.InnerString("message")) |
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.
(Minor)
Please fix format.
Can we avoid this helper method? We should not attempt hiding complexity in our demo app.
Context
The PR enables the management of Orchestration Configs in prompt registry (create, delete, list via generated code and new client) as well as convenience to use these Orchestration Configs in the
OrchestrationClient.Sample of new convenience:
Feature scope:
OchestrationConfigClientin prompt registryOrchestrationClientDefinition of Done
Aligned changes with the JavaScript SDK