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.

Sign up for GitHub

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

Jump to bottom

Added Orchestration LLM Config Convenience #152

Merged
merged 23 commits into from
Nov 19, 2024
Merged

Added Orchestration LLM Config Convenience #152

merged 23 commits into from
Nov 19, 2024

Conversation

CharlesDuboisSAP
Copy link
Contributor

Context

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

As an orchestration user, I want to have a convenient way of configuring which LLM to use, and with which hyper parameters.

Feature scope:

  • Added OrchestrationAiModel extends LLMModuleConfig

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 added the please-review Request to review a pull-request label Nov 12, 2024
@CharlesDuboisSAP CharlesDuboisSAP self-assigned this Nov 12, 2024
@CharlesDuboisSAP CharlesDuboisSAP removed the please-review Request to review a pull-request label Nov 13, 2024
MatKuhr
MatKuhr reviewed Nov 14, 2024
View reviewed changes
Copy link
Member

@MatKuhr MatKuhr left a comment

Choose a reason for hiding this comment

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

Also, I think its a bit weird that we don't use our own, central interface for this. Any reason not to?

Jonas Israel added 4 commits November 15, 2024 13:09
Work in progress
f8fb1de
Merge branch 'main' into orchestration-llm-config
869e83a 
# Conflicts:
#	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
Work in progress
0b9a107
Work in progress
3fe07db
@Jonas-Isr
Copy link
Member

Also, I think its a bit weird that we don't use our own, central interface for this. Any reason not to?

The AiModel interface is only there to cache deployments and Orchestration does not use deployments, so it is unrelated.

@MatKuhr
Copy link
Member

MatKuhr commented Nov 18, 2024

The AiModel interface is only there to cache deployments and Orchestration does not use deployments, so it is unrelated.

But this is implemented by OpenAiModel, i.e. we use this for our other model constants, so how is it unrelated?

Jonas Israel added 2 commits November 18, 2024 15:22
Merge branch 'main' into orchestration-llm-config
6862111 
# Conflicts:
#	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
Merged main branch.
fd29be9
@Jonas-Isr Jonas-Isr added the please-review Request to review a pull-request label Nov 18, 2024
@CharlesDuboisSAP
Copy link
Contributor Author

CharlesDuboisSAP commented Nov 18, 2024 •
edited
Loading

The AiModel interface is only there to cache deployments and Orchestration does not use deployments, so it is unrelated.

But this is implemented by OpenAiModel, i.e. we use this for our other model constants, so how is it unrelated?

The cache is the only user of this AiModel interface, I don't need to cache GPT 35 Turbo with temperature 0.5 and version 2024-5-5 to use it in orchestration.

CharlesDuboisSAP
CharlesDuboisSAP commented Nov 19, 2024
View reviewed changes
Copy link
Contributor Author

@CharlesDuboisSAP CharlesDuboisSAP left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • Removed unavailable models (like embeddings)
  • OrchestrationAiModel mow implements AiModel
  • Changed withLlmConfig parameter to OrchestrationAiModel

MatKuhr
MatKuhr reviewed Nov 19, 2024
View reviewed changes
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationAiModel.java Outdated Show resolved Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationAiModel.java Outdated Show resolved Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationAiModel.java Outdated Show resolved Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationAiModel.java Outdated Show resolved Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationAiModel.java Outdated Show resolved Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationAiModel.java Outdated Show resolved Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationAiModel.java Outdated Show resolved Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationModuleConfig.java Outdated Show resolved Hide resolved
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationAiModel.java Show resolved Hide resolved
@CharlesDuboisSAP CharlesDuboisSAP mentioned this pull request Nov 19, 2024
Merged
8 tasks
@CharlesDuboisSAP CharlesDuboisSAP enabled auto-merge (squash) November 19, 2024 10:37
newtork
newtork reviewed Nov 19, 2024
View reviewed changes
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationAiModel.java
@Value
@With
@AllArgsConstructor
public class OrchestrationAiModel {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

newtork
newtork reviewed Nov 19, 2024
View reviewed changes
orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationAiModel.java
OrchestrationAiModel(@Nonnull final String modelName) {
setModelName(modelName);
setModelParams(Map.of());
this(modelName, Map.of(), "latest");
Copy link
Contributor

Choose a reason for hiding this comment

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

(Question)

Is latest always supported by orchestration?

Copy link
Member

Choose a reason for hiding this comment

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

from my testing and understanding, yes

@CharlesDuboisSAP CharlesDuboisSAP merged commit d2c65bb into main Nov 19, 2024
5 checks passed
@CharlesDuboisSAP CharlesDuboisSAP deleted the orchestration-llm-config branch November 19, 2024 12:05
newtork
newtork reviewed Nov 19, 2024
View reviewed changes
docs/guides/ORCHESTRATION_CHAT_COMPLETION.md
Comment on lines +220 to +228
OrchestrationAiModel customGPT4O =
OrchestrationAiModel.GPT_4O
.withModelParams(
Map.of(
"max_tokens", 50,
"temperature", 0.1,
"frequency_penalty", 0,
"presence_penalty", 0));
"presence_penalty", 0))
.withModelVersion("2024-05-13");
Copy link
Contributor

Choose a reason for hiding this comment

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

(Minor/Question)

Could/should we shorten the method names...?

  OrchestrationAiModel.GPT_4O
-   .withModelParams(...)
-   .withModelVersion("2024-05-13");
+   .withParams(...)
+   .withVersion("2024-05-13");

Copy link
Contributor

Choose a reason for hiding this comment

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

We can discuss here:
#170

@newtork newtork mentioned this pull request Nov 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jonas-Isr Jonas-Isr Jonas-Isr left review comments

@newtork newtork newtork left review comments

@MatKuhr MatKuhr MatKuhr approved these changes

Assignees

@CharlesDuboisSAP CharlesDuboisSAP

@Jonas-Isr Jonas-Isr

Labels
please-review Request to review a pull-request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@CharlesDuboisSAP @Jonas-Isr @MatKuhr @newtork @bot-sdk-js