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] Configuration as JSON String #177

Merged
merged 8 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions docs/guides/ORCHESTRATION_CHAT_COMPLETION.md
Original file line number Diff line number Diff line change
Expand Up @@ -208,3 +208,21 @@ OrchestrationAiModel customGPT4O =
"presence_penalty", 0))
.withVersion("2024-05-13");
```

### Using a Configuration from AI Launchpad
MatKuhr marked this conversation as resolved.
Show resolved Hide resolved

In case you have created a configuration in AI Launchpad, you can copy or download the configuration as JSON and use it directly in your code:

```java
var configJson = """
... paste your configuration JSON in here ...
""";
// or load your config from a file, e.g.
// configJson = Files.readString(Paths.get("path/to/my/orchestration-config.json"));

var prompt = new OrchestrationPrompt(Map.of("your-input-parameter", "your-param-value"));

new OrchestrationClient().executeRequestFromJsonModuleConfig(prompt, configJson);
Copy link
Contributor

@newtork newtork Nov 22, 2024

Choose a reason for hiding this comment

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

Your suggested code change results in the following three methods on client:

OrchestrationClient client = new OrchestrationClient();

// execute completion request (exist)
CompletionPostRequest requestDto;
CompletionPostResponse responseDto = client
  .executeRequest(
    requestDto);

// execute prompt (exist)
OrchestrationPrompt promptConvenience;
OrchestrationModuleConfig configConvenience;
OrchestrationChatResponse responseConvenience = client
  .executeRequest(
    promptConvenience,
    configConvenience);

// execute prompt (new)
OrchestrationPrompt promptConvenience;
String configJson;
OrchestrationChatResponse responseConvenience = client
  .executeRequestFromJsonModuleConfig(
    promptConvenience,
    configJson);

(Question)

Is it not possible to derive reuse existing method and allow for OrchestrationModuleConfig to be created from String configJson? That would look a little cleaner, maybe(?)

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 also thought about this approach. But unfortunately not, because the JSON string is expected to contain things not yet supported by OrchestrationModuleConfig or it's individual config objects, i.e. throughout the entire JSON. That includes cases of oneOf, e.g. TemplateRef.

```

While this is not recommended for long term use, it can be useful for creating demos and PoCs.
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.PropertyAccessor;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
import com.google.common.annotations.Beta;
import com.sap.ai.sdk.core.AiCoreDeployment;
import com.sap.ai.sdk.core.AiCoreService;
import com.sap.ai.sdk.orchestration.client.model.CompletionPostRequest;
Expand All @@ -30,7 +33,6 @@
import org.apache.hc.client5.http.classic.methods.HttpPost;
import org.apache.hc.core5.http.ContentType;
import org.apache.hc.core5.http.io.entity.StringEntity;
import org.apache.hc.core5.http.message.BasicClassicHttpRequest;
import org.springframework.http.converter.json.Jackson2ObjectMapperBuilder;

/** Client to execute requests to the orchestration service. */
Expand Down Expand Up @@ -132,26 +134,75 @@ public OrchestrationChatResponse chatCompletion(
@Nonnull
public CompletionPostResponse executeRequest(@Nonnull final CompletionPostRequest request)
throws OrchestrationClientException {
final BasicClassicHttpRequest postRequest = new HttpPost("/completion");
final String jsonRequest;
try {
val json = JACKSON.writeValueAsString(request);
log.debug("Serialized request into JSON payload: {}", json);
postRequest.setEntity(new StringEntity(json, ContentType.APPLICATION_JSON));
jsonRequest = JACKSON.writeValueAsString(request);
log.debug("Serialized request into JSON payload: {}", jsonRequest);
} catch (final JsonProcessingException e) {
throw new OrchestrationClientException("Failed to serialize request parameters", e);
}

return executeRequest(postRequest);
return executeRequest(jsonRequest);
}

/**
* Perform a request to the orchestration service using a module configuration provided as JSON
* string. This can be useful when building a configuration in the AI Launchpad UI and exporting
* it as JSON. Furthermore, this allows for using features that are not yet supported natively by
* the API.
*
* <p><b>NOTE:</b> This method does not support streaming.
*
* @param prompt The input parameters and optionally message history to use for prompt execution.
* @param moduleConfig The module configuration in JSON format.
* @return The completion response.
* @throws OrchestrationClientException If the request fails.
*/
@Beta
Copy link
Contributor

Choose a reason for hiding this comment

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

👍
Let's keep this as beta before the alignment with e.g., Launchpad.

@Nonnull
public OrchestrationChatResponse executeRequestFromJsonModuleConfig(
MatKuhr marked this conversation as resolved.
Show resolved Hide resolved
@Nonnull final OrchestrationPrompt prompt, @Nonnull final String moduleConfig)
MatKuhr marked this conversation as resolved.
Show resolved Hide resolved
throws OrchestrationClientException {
if (!prompt.getMessages().isEmpty()) {
throw new IllegalArgumentException(
"Prompt must not contain any messages when using a JSON module configuration, as the template is already defined in the JSON.");
}
Comment on lines +166 to +169
Copy link
Contributor

Choose a reason for hiding this comment

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

An OrchestrationPrompt is not the right object then. Why not accept a map of input params?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because that is missing the (optional) messages history.

OrchestrationPrompt is not the right object

I'd argue it is. We will have further cases where passing messages is not allowed, e.g. when using TemplateRef. If we want we could create another Prompt object for that to make this distinction, but so far I figured it's not worth it.


final var request =
new CompletionPostRequest()
.messagesHistory(prompt.getMessagesHistory())
.inputParams(prompt.getTemplateParameters());

final ObjectNode requestJson = JACKSON.valueToTree(request);
final JsonNode moduleConfigJson;
try {
moduleConfigJson = JACKSON.readTree(moduleConfig);
} catch (JsonProcessingException e) {
throw new IllegalArgumentException(
"The provided module configuration is not valid JSON: " + moduleConfig, e);
}
requestJson.set("orchestration_config", moduleConfigJson);
Copy link
Contributor

@newtork newtork Nov 25, 2024

Choose a reason for hiding this comment

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

(Major/Question)

Not sure, but wouldn't it make more sense applying the module config JSON to client instead of per request?

Seems like the only place where orchestration_config is provided, it could also be an optional parameter that is passed along the convenience API(?)

(My hidden motivation is to challenge this method a little.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm not exactly sure what you mean.. Do you mean setting the config on the OrchestrationClient object instead? Or something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.
Boiling this down to the question: Is orchestration_config to be expected request specific or client specific?

Copy link
Member Author

@MatKuhr MatKuhr Nov 25, 2024

Choose a reason for hiding this comment

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

Essentially same as the OrchestrationConfig object: It depends how much, if any, config changes per request. But the tendency is it is tied to the client, i.e. for typical use cases the majority of config will stay the same across multiple requests.

However, we decided to not allow config per client to make the API simpler. So the same strategy is applied for this method too. If we want to, we can still change this in the future though.

Copy link
Contributor

@newtork newtork Nov 25, 2024

Choose a reason for hiding this comment

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

Understood.

To me, from a user perspective, it would feel weird to pass a String object along (over multiple controller classes potentially) just to pass to my request invocations.

But it's difficult to argue about client / request specific data handling, when I haven't yet seen a proper use-case "in the wild". Maybe we will discuss this again, after having users first interact with that.


final String body;
try {
body = JACKSON.writeValueAsString(requestJson);
} catch (JsonProcessingException e) {
throw new OrchestrationClientException("Failed to serialize request to JSON", e);
}
return new OrchestrationChatResponse(executeRequest(body));
}

@Nonnull
CompletionPostResponse executeRequest(@Nonnull final BasicClassicHttpRequest request) {
CompletionPostResponse executeRequest(@Nonnull final String request) {
val postRequest = new HttpPost("/completion");
postRequest.setEntity(new StringEntity(request, ContentType.APPLICATION_JSON));

try {
val destination = deployment.get().destination();
log.debug("Using destination {} to connect to orchestration service", destination);
val client = ApacheHttpClient5Accessor.getHttpClient(destination);
return client.execute(
request, new OrchestrationResponseHandler<>(CompletionPostResponse.class));
postRequest, new OrchestrationResponseHandler<>(CompletionPostResponse.class));
} catch (NoSuchElementException
| DestinationAccessException
| DestinationNotFoundException
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -380,4 +380,55 @@ void testErrorHandling() {

softly.assertAll();
}

@Test
void testExecuteRequestFromJson() {
stubFor(post(anyUrl()).willReturn(okJson("{}")));

prompt = new OrchestrationPrompt(Map.of());
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make the prompt as optional parameter.

Copy link
Member Author

@MatKuhr MatKuhr Nov 21, 2024

Choose a reason for hiding this comment

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

It's not optional, it has to be given. In this unit test I'm just skipping the effort to give params in the map

Copy link
Contributor

@jjtang1985 jjtang1985 Nov 21, 2024

Choose a reason for hiding this comment

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

It's not optional, it has to be given.

You are right, so we can discuss the API signature/design 😄

Please have a look at the following json, that I copied from the Launchpad.

{
    "module_configurations": {
        "llm_module_config": {
            "model_name": "mistralai--mistral-large-instruct",
            "model_params": {},
            "model_version": "2407"
        },
        "templating_module_config": {
            "template": [
                {
                    "role": "user",
                    "content": "How are you?"
                }
            ],
            "defaults": {}
        }
    }
}

I would assume, I've got everything from the JSON and should be able to run it with a function like

myFunction(json);

The second param (prompt in this case) is only needed (so it should be optional), when I want to pass a inputParam or provide message history like the following screenshot:

Screenshot 2024-11-21 at 20 55 47

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah true, Launchpad doesn't force you to use any variables. Okay, will update 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

After thinking about this again, I actually think the use case for this is not very strong. Because if there are no input params or messages history, prompt execution will always yield the same or very similar results (depending on the temperature setting).

The same is true when using template references. If we really want to support such usage, we could consider allowing new OrchestrationPrompt() for these cases. But making the prompt itself optional would mean we add 2-3 method overloads for non-productive cases..

final var configJson =
"""
{
"module_configurations": {
"llm_module_config": {
"model_name": "mistralai--mistral-large-instruct",
"model_params": {}
}
}
}
""";

final var expectedJson =
"""
{
"messages_history": [],
"input_params": {},
"orchestration_config": {
"module_configurations": {
"llm_module_config": {
"model_name": "mistralai--mistral-large-instruct",
"model_params": {}
}
}
}
}
""";

var result = client.executeRequestFromJsonModuleConfig(prompt, configJson);
assertThat(result).isNotNull();

verify(postRequestedFor(anyUrl()).withRequestBody(equalToJson(expectedJson)));
}

@Test
void testExecuteRequestFromJsonThrows() {
assertThatThrownBy(() -> client.executeRequestFromJsonModuleConfig(prompt, "{}"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("messages");

prompt = new OrchestrationPrompt(Map.of());
assertThatThrownBy(() -> client.executeRequestFromJsonModuleConfig(prompt, "{ foo"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("not valid JSON");
}
}