Skip to content
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

feat: Orchestration client #114

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

feat: Orchestration client #114

wants to merge 9 commits into from

Conversation

MatKuhr
Copy link
Member

@MatKuhr MatKuhr commented Oct 22, 2024

Context

This PR introduces a completely new API for orchestration.

Feature scope:

  • Dedicated client class
    • Ready for Streaming
  • Streamlined config API via delegation
    • Ready for Spring or Langchain integration
  • Convenience API for llm config
    • Integrated with core class AiModel
  • Convenience API for messages
    • Convenience for message history raised a feature request to enable this here
  • Convenience API for templating
  • Convenience API for filtering
  • Convenience API for masking

TODO

  • Javadoc
  • Logging
  • Mark generated models as @Beta and (maybe?) rename model package to dto
  • Follow-up PR: Reduce code duplication with OpenAI client
  • (maybe?) More tests asserting equals & hash code
  • ...

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

@@ -0,0 +1,3 @@
package com.sap.ai.sdk.orchestration;

public sealed interface ContentFilter permits AzureContentFilter {}
Copy link
Member Author

Choose a reason for hiding this comment

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

Sealed is quite useful here, as this is a marker interface, and users can't supply their own implementations, because we wouldn't have the implementation to translate those to DTOs.

With a higher Java version, we could even switch over implementations of the interface. That has the nice benefit that the compiler can force us to handle all implementations. But only with Java 21 🤷🏻

}

@Nonnull
com.sap.ai.sdk.orchestration.client.model.FilterConfig toFilterConfigDTO() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to consistently label the DTOs as such in the code, so that it's always obvious when we are (preparing) (de-) serialization

public interface OrchestrationConfig<T extends OrchestrationConfig<T>> {

@Nonnull
Option<AiModel> getLlmConfig();
Copy link
Member Author

Choose a reason for hiding this comment

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

Debatable, if we want to keep VAVR in our public API. Done here for now because this makes our own code (arguably) much more beautiful 😄

Alternatives would be Java Optional or just @Nullable.

import lombok.RequiredArgsConstructor;
import lombok.val;

public record OrchestrationResponse(
Copy link
Member Author

Choose a reason for hiding this comment

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

Debatable if this should be a record or not. The AllArgs constructor is a bit awkward. Also, we might offer more convenience later on..


public record OrchestrationResponse(
@Nonnull ChatMessage assistantMessage,
@Nonnull List<ChatMessage> allMessages,
Copy link
Member Author

Choose a reason for hiding this comment

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

allMessages is a notable convenience for end users, this is not natively included in the orchestration response, but needed for chat scenarios or in general for an easy display of prompt + response.

Map.of(
"orgCV",
"""
Patrick Morgan
Copy link
Member Author

Choose a reason for hiding this comment

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

While this is a nice example in principal, I felt that it is too large to properly see the relationship between masking config, prompt and result.

Also, I'm not sure if applying all masking profiles is a good idea.

@MatKuhr MatKuhr marked this pull request as ready for review October 22, 2024 21:03

@RequiredArgsConstructor
public enum Sensitivity {
HIGH(0),
Copy link
Member Author

@MatKuhr MatKuhr Oct 22, 2024

Choose a reason for hiding this comment

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

Naming aligned w/ Orchestration team, to be aligned w/ JS and Launchpad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants