-
Notifications
You must be signed in to change notification settings - Fork 9
feat: [Orchestration] Enable local prompt templates #423
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
Conversation
# Conflicts: # orchestration/src/main/java/com/sap/ai/sdk/orchestration/ConfigToRequestTransformer.java
# Conflicts: # sample-code/spring-app/src/main/resources/static/index.html
final Object obj; | ||
try { | ||
final ObjectMapper yamlReader = new ObjectMapper(new YAMLFactory()); | ||
obj = yamlReader.readValue(inputYaml, Object.class); | ||
} catch (JsonProcessingException ex) { | ||
throw new IOException("Failed to parse the YAML input: " + ex.getMessage(), ex); | ||
} | ||
try { | ||
final ObjectMapper jsonWriter = new ObjectMapper(); | ||
return fromJson(jsonWriter.writeValueAsString(obj)); | ||
} catch (JsonProcessingException ex) { | ||
throw new IOException("Failed to deserialize the input: " + ex.getMessage(), ex); | ||
} |
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 am not sure if these are the correct/best Exception types. I am open to suggestions :)
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.
Instead of using two objectmappers, I was about to pitch something like...
private static class TemplateYaml {
@JsonProperty("spec")
private OrchestrationTemplate spec;
}
But that would've required additional customization to propagate the modules/rules along for Yaml Factory.
private OrchestrationTemplate fromJson(@Nonnull final String inputString) throws IOException { | ||
final ObjectMapper objectMapper = | ||
OrchestrationJacksonConfiguration.getOrchestrationObjectMapper(); | ||
final JsonNode rootNode = objectMapper.readTree(inputString); | ||
return objectMapper.treeToValue(rootNode.get("spec"), OrchestrationTemplate.class); | ||
} |
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.
This method could principally be made public as an additional feature. I left it private because it is not part of the ticket, not tested on its own, and not aligned with JS SDK (which only has the yaml feature afaik).
* @since 1.7.0 | ||
*/ | ||
@Nullable | ||
public OrchestrationTemplate fromYaml(@Nonnull final String inputYaml) throws IOException { |
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 to reviewer)
Should we maybe annotate this with @Beta
?
Pro: We might not be able to continue to keep this method/feature alive without additional work if the specs of prompt-registry and orchestration were to diverge at some point (see "Note 1" in the PR description).
Con: Additional beta annotation for convenience code.
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.
as discussed, since this API has no alternative to users, I would keep it non-Beta
Context
AI/ai-sdk-java-backlog#215
This PR enables users to validate and use prompt templates from their local environment, without the use of prompt registry. For this, the user can provide a String containing a YAML representation of the prompt template and we will deserialise this into our (high level)
OrchestrationTemplate
class, which then can be used to configure an Orchestration call. If the provided prompt template does not match theprompt-registry
spec, an exception is thrown.Note 1: We have to assume that the
prompt-registry
spec keeps exactly equal to theorchestration
spec for the relevant fields. If the two diverge, we can no longer deserialise the prompt template (which followsprompt-registry
spec) into our Java classes (which follow mostly theorchestration
prompt). Since we deserialise into high-level classes, we might have some wiggle room, but that would probably become quite messy.Note 2: The whole process on our side (i.e. validation and deserialisation) ignores the
additionalFields
of the prompt templates. This is aligned with PO and JS SDK. (The reason is that we cannot know what is inside these fields, and it usually is information we cannot handle in our template classes.)Feature scope:
Definition of Done