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

Fix custom prompt substitute with List issue #2871

Merged

Conversation

mingshl
Copy link
Collaborator

@mingshl mingshl commented Aug 31, 2024

Description

fix custom prompt substitute with List issue in ml inference search response processor

using toString() method in placeholder to convert list into string, for example, ${parameters.context.toString()} in the following processors config:

PUT /_search/pipeline/my_pipeline_request_review_llm
{
  "response_processors": [
    {
      "ml_inference": {
        "tag": "ml_inference",
        "description": "This processor is going to run llm",
        "model_id": "cf46K5EBoVpekzRp8x_3",
        "function_name": "REMOTE",
        "input_map": [
          {
            "context": "review"
          }
        ],
        "output_map": [
          {
            "llm_response": "response"
          }
        ],
        "model_config": {
          "prompt":"\n\nHuman: You are a professional data analysist. You will always answer question based on the given context first. If the answer is not directly shown in the context, you will analyze the data and find the answer. If you don't know the answer, just say I don't know. Context: ${parameters.context.toString()}. \n\n Human: please summarize the documents \n\n Assistant:"
        },
        "ignore_missing": false,
        "ignore_failure": false
      }
    }
  ]
}

reverting https:/opensearch-project/ml-commons/pull/2811/files as a more appropriate approach to fix the toString issue #2839

#2880

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Mingshi Liu <[email protected]>
* @param parameters A map containing key-value pairs where the values may contain toString() method calls.
* @return A new map with the processed values for the toString() method calls.
*/
public static Map<String, String> parseParameters(Map<String, String> parameters) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add prefix to method parameter list ? For connector, it will be parameters. For other usecase, it maybe different, like local model could be model_config

Suggested change
public static Map<String, String> parseParameters(Map<String, String> parameters) {
public static Map<String, String> parseParameters(String prefix, Map<String, String> parameters) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

all parameters will be prefix with parameters in here

StringSubstitutor substitutor = new StringSubstitutor(parameters, "${parameters.", "}");

I added the test to test when it's parameters.model_config.context, then it would be taking the prefix as model_config.context properly, please see this UT

public void testParseParametersListToStringModelConfig() {
Map<String, String> parameters = new HashMap<>();
parameters
.put(
"prompt",
"\\n\\nHuman: You are a professional data analyst. You will always answer question based on the given context first. If the answer is not directly shown in the context, you will analyze the data and find the answer. If you don't know the answer, just say I don't know. Context: ${parameters.model_config.context.toString()}. \\n\\n Human: please summarize the documents \\n\\n Assistant:"
);
ArrayList<String> listOfDocuments = new ArrayList<>();
listOfDocuments.add("document1");
parameters.put("model_config.context", toJson(listOfDocuments));
parseParameters(parameters);
System.out.println(parameters.get("context" + TO_STRING_FUNCTION_NAME));
System.out.println(parameters);
assertEquals(parameters.get("model_config.context" + TO_STRING_FUNCTION_NAME), "[\\\"document1\\\"]");
String requestBody = "{\"prompt\": \"${parameters.prompt}\"}";
StringSubstitutor substitutor = new StringSubstitutor(parameters, "${parameters.", "}");
requestBody = substitutor.replace(requestBody);
System.out.println(requestBody);
assertEquals(
requestBody,
"{\"prompt\": \"\\n\\nHuman: You are a professional data analyst. You will always answer question based on the given context first. If the answer is not directly shown in the context, you will analyze the data and find the answer. If you don't know the answer, just say I don't know. Context: [\\\"document1\\\"]. \\n\\n Human: please summarize the documents \\n\\n Assistant:\"}"
);
}

ylwu-amzn
ylwu-amzn previously approved these changes Aug 31, 2024
@mingshl mingshl temporarily deployed to ml-commons-cicd-env August 31, 2024 22:53 — with GitHub Actions Inactive
@mingshl mingshl temporarily deployed to ml-commons-cicd-env August 31, 2024 22:53 — with GitHub Actions Inactive
@mingshl mingshl merged commit 49d4a01 into opensearch-project:main Sep 2, 2024
8 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 2, 2024
mingshl added a commit that referenced this pull request Sep 3, 2024
…esponse processor (#2871) (#2874)

(cherry picked from commit 49d4a01)

Co-authored-by: Mingshi Liu <[email protected]>
@mingshl mingshl changed the title Fix custom prompt substitute with List issue in ml inference search response processor Fix custom prompt substitute with List issue Sep 5, 2024
@mingshl mingshl mentioned this pull request Sep 5, 2024
5 tasks
@dhrubo-os
Copy link
Collaborator

dhrubo-os commented Sep 6, 2024

Shouldn't we use model interface instead of adding toString to the parameter to identify if we need to convert that to string? cc @ylwu-amzn

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

Successfully merging this pull request may close these issues.

4 participants