Skip to content

Conversation

@CharlesDuboisSAP
Copy link
Contributor

@CharlesDuboisSAP CharlesDuboisSAP commented Jan 14, 2025

Context

AI/ai-sdk-java-backlog#8.

It's possible to use Spring AI and "under the hood" the AI SDK functionality is called

Feature scope:

  • Added OrchestrationChatModel, a client
  • Added OrchestrationChatOptions, that fits in the Spring AI Prompt
  • Added OrchestrationSpringChatResponse
  • Added a new SpringAiOrchestrationController end-to-end test

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

@CharlesDuboisSAP CharlesDuboisSAP self-assigned this Jan 14, 2025
@CharlesDuboisSAP CharlesDuboisSAP changed the title feat: |Orchestration] Spring AI integration feat: [Orchestration] Spring AI integration Jan 14, 2025
@CharlesDuboisSAP CharlesDuboisSAP added the please-review Request to review a pull-request label Jan 14, 2025
Comment on lines 208 to 210
private <T> T getLlmConfigParam(@Nonnull final String param) {
if (getLlmConfigNonNull().getModelParams() instanceof LinkedHashMap) {
return ((LinkedHashMap<String, T>) getLlmConfigNonNull().getModelParams()).get(param);
Copy link
Contributor Author

@CharlesDuboisSAP CharlesDuboisSAP Jan 14, 2025

Choose a reason for hiding this comment

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

Optional: T could be Parameter<ValueT> (from the class OrchestrationAiModel)
But I didn't find a way to do this...

Copy link
Contributor

@newtork newtork Jan 15, 2025

Choose a reason for hiding this comment

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

I tested that:

private <T> Option<T> getLlmConfigParam(@Nonnull final String param) {
  return Option.of(getLlmConfigNonNull().getModelParams())
    .filter(p -> p instanceof Map<?,?>)
    .map(p -> (T) ((Map<?,?>) p).get(param))
    .filter(Objects::nonNull);
}

Unfortunately upon usage, the impliciit T is not recognized:

  public Double getPresencePenalty() {
-   return getLlmConfigParam(PRESENCE_PENALTY.getName());
+   return getLlmConfigParam(PRESENCE_PENALTY.getName()).getOrNull();
  }

However this would work, but it looks worse:

  public Double getPresencePenalty() {
-   return getLlmConfigParam(PRESENCE_PENALTY.getName());
+   return this.<Double>getLlmConfigParam(PRESENCE_PENALTY.getName()).getOrNull();
  }

Comment on lines +42 to +43
throw new IllegalArgumentException(
"Please add OrchestrationChatOptions to the Prompt: new Prompt(\"message\", new OrchestrationChatOptions(config))");
Copy link
Contributor

@newtork newtork Jan 15, 2025

Choose a reason for hiding this comment

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

(Question)

So we're planning to throw an exception, when the provided prompt argument does not contain an OrchestrationChatOptions object. This is implicit behavior. Users may not catch that limitation on design-time.

Would it be possible to make it explicit, e.g. by enforcing a constructor argument for OrchestrationChatOptions OrchestrationModuleConfig on OrchestrationChatModel? Thus passing it on for every call as fallback?

Copy link
Contributor Author

@CharlesDuboisSAP CharlesDuboisSAP Jan 15, 2025

Choose a reason for hiding this comment

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

  • call is part if the interface, cannot be modified.
  • If we add a constructor param then there are 2 different places to add a config. We have to merge them which is implicit behaviour.

This is not solveable

Copy link
Contributor

Choose a reason for hiding this comment

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

If not constructor - offering an optional setDefaultChatOptions(..) is probably not helping either, right? Because then you would need to thin about merging options ._.

```

### Using a Custom Resource Group
## Using a Custom Resource Group
Copy link
Contributor

Choose a reason for hiding this comment

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

(Comment)

Why the drive-by changes 😅

Copy link
Contributor Author

@CharlesDuboisSAP CharlesDuboisSAP Jan 15, 2025

Choose a reason for hiding this comment

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

this ## adds underlines which makes it more readable because the file is super long, it also aligns with the other files.

underline

public final class ConfigToRequestTransformer {
@Nonnull
static CompletionPostRequest toCompletionPostRequest(
@Nonnull final OrchestrationPrompt prompt, @Nonnull final OrchestrationModuleConfig config) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(Major)

(I expect static code checks to flag warnings for) incomplete JavaDoc on public API.

Copy link
Member

Choose a reason for hiding this comment

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

I can see that we only need this for copy. Did you check if we really need to support copy? If not, we could think about throwing a NotImplementedException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OrchestrationModuleConfig is immutable. I will turn the deep copy into a normal copy since config cannot be modified anyway.

Copy link
Member

Choose a reason for hiding this comment

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

But the config inside of OrchestrationModuleConfig can be modified, e.g. LlmModuleConfig. Shallow copy is only okay if we are sure it won't be modified.

@CharlesDuboisSAP CharlesDuboisSAP enabled auto-merge (squash) January 17, 2025 08:58
Copy link
Contributor

@newtork newtork left a comment

Choose a reason for hiding this comment

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

looks lgtm to me

@CharlesDuboisSAP CharlesDuboisSAP merged commit 98facc1 into main Jan 17, 2025
6 checks passed
@CharlesDuboisSAP CharlesDuboisSAP deleted the bean branch January 17, 2025 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

please-review Request to review a pull-request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants