Add support for context_size and include 'interaction_id' in SearchRe… #1385

austintlee · 2023-09-25T22:35:00Z

Description

[Describe what this change achieves]

Issues Resolved

Check List

[x ] New functionality includes testing.
- [x ] All tests pass
New functionality has been documented.
- New functionality has javadoc added
[x ] Commits are signed per the DCO using --signoff

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.

…sponse. [Issue opensearch-project#1372] Signed-off-by: Austin Lee <[email protected]>

austintlee · 2023-09-26T13:57:27Z

Tests with failures:
 - org.opensearch.ml.action.prediction.PredictionITTests.testPredictionWithSearchInput_LogisticRegression

Seems like a flaky test.

austintlee · 2023-09-26T13:58:41Z

@dhrubo-os @jngz-es @ylwu-amzn can you please take a look at this PR? Thanks.

jngz-es · 2023-09-28T17:18:34Z

.../org/opensearch/searchpipelines/questionanswering/generative/ext/GenerativeQAParameters.java

@@ -22,6 +22,8 @@
 import lombok.Getter;
 import lombok.NoArgsConstructor;
 import lombok.Setter;
+import org.opensearch.common.recycler.Recycler;


Didn't see where to use it.

Removed. I added "spotless" so I don't miss stuff like this going forward.

jngz-es · 2023-09-28T17:46:24Z

...ain/java/org/opensearch/searchpipelines/questionanswering/generative/llm/DefaultLlmImpl.java

+            /*
+             * error={message=This model's maximum context length is 4097 tokens. However, your messages resulted in 4456 tokens.
+             *                Please reduce the length of the messages.,
+             *        type=invalid_request_error, param=messages, code=context_length_exceeded}
+             */


jngz-es · 2023-09-28T18:06:49Z

...main/java/org/opensearch/searchpipelines/questionanswering/generative/prompt/PromptUtil.java

+        if (Strings.isNullOrEmpty(systemPrompt) && Strings.isNullOrEmpty(userInstructions)) {
+            systemPrompt = DEFAULT_SYSTEM_PROMPT;
+        }


If only userInstructions exists, we don't set default system promt for chat use case?

Yes, that's right. Actually, you can pass a system prompt as part of user instructions. We are getting some mixed results and keep the behavior this way so we can do more experiments.

codecov · 2023-09-29T21:18:16Z

Codecov Report

Merging #1385 (857f436) into 2.x (2c8cc02) will increase coverage by 0.14%.
Report is 9 commits behind head on 2.x.
The diff coverage is 88.88%.

@@             Coverage Diff              @@
##                2.x    #1385      +/-   ##
============================================
+ Coverage     78.35%   78.49%   +0.14%     
- Complexity     2275     2296      +21     
============================================
  Files           190      190              
  Lines          9286     9418     +132     
  Branches        910      934      +24     
============================================
+ Hits           7276     7393     +117     
- Misses         1599     1602       +3     
- Partials        411      423      +12

Flag	Coverage Δ
ml-commons	`78.49% <88.88%> (+0.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...ing/generative/GenerativeQAProcessorConstants.java	`75.00% <ø> (ø)`
...ering/generative/GenerativeQARequestProcessor.java	`100.00% <100.00%> (ø)`
.../generative/client/ConversationalMemoryClient.java	`97.22% <100.00%> (+0.34%)`	⬆️
...ng/generative/ext/GenerativeQAParamExtBuilder.java	`100.00% <ø> (ø)`
...nswering/generative/ext/GenerativeQAParamUtil.java	`92.85% <100.00%> (+3.96%)`	⬆️
...nanswering/generative/llm/ChatCompletionInput.java	`100.00% <ø> (ø)`
...estionanswering/generative/llm/DefaultLlmImpl.java	`100.00% <100.00%> (ø)`
...es/questionanswering/generative/llm/LlmIOUtil.java	`100.00% <100.00%> (ø)`
...questionanswering/generative/llm/ModelLocator.java	`100.00% <ø> (ø)`
...answering/generative/GenerativeSearchResponse.java	`94.73% <87.50%> (-5.27%)`	⬇️
... and 5 more

dhrubo-os · 2023-09-29T20:23:15Z

...g/opensearch/searchpipelines/questionanswering/generative/GenerativeQAResponseProcessor.java

@@ -58,11 +60,16 @@ public class GenerativeQAResponseProcessor extends AbstractProcessor implements

    private static final int DEFAULT_CHAT_HISTORY_WINDOW = 10;

+    private static final int MAX_PROCESSOR_TIME_IN_SECONDS = 60;


I feel like DEFAULT makes more sense here.

Hmm.. Maybe a "max" value doesn't make sense here, although I think 60 seconds is a long time. I will change this to a 30 second default time (timeout).

dhrubo-os · 2023-09-29T20:26:36Z

.../org/opensearch/searchpipelines/questionanswering/generative/ext/GenerativeQAParameters.java

+    private static final ParseField INTERACTION_SIZE = new ParseField("interaction_size");
+    private static final ParseField TIMEOUT = new ParseField("timeout");
+
+    public static final int SIZE_NULL_VALUE = -1;


Can we please add a comment for this variable as the variable name isn't quite self explanatory.

dhrubo-os · 2023-09-29T20:31:23Z

...g/opensearch/searchpipelines/questionanswering/generative/GenerativeQAResponseProcessor.java

+        }
+        log.info("Using interaction size of {}", interactionSize);
+        List<Interaction> chatHistory = (conversationId == null) ? Collections.emptyList() : memoryClient.getInteractions(conversationId, interactionSize);
+        log.info("Retrieved chat history. ({})", getDuration(start));


Just curious to know what's the goal of adding getDuration(start) in the log?

It logs the elapsed time for client calls. I can use debug for the log level.

dhrubo-os · 2023-09-29T21:09:09Z

...ensearch/searchpipelines/questionanswering/generative/client/ConversationalMemoryClient.java

@@ -37,6 +42,7 @@

 import java.util.ArrayList;
 import java.util.List;
+import java.util.function.BiFunction;


Did we use these newly imported libraries?

dhrubo-os · 2023-09-29T21:17:09Z

...g/opensearch/searchpipelines/questionanswering/generative/GenerativeQAResponseProcessor.java

@@ -93,45 +102,94 @@ public SearchResponse processResponse(SearchRequest request, SearchResponse resp
        }

        GenerativeQAParameters params = GenerativeQAParamUtil.getGenerativeQAParameters(request);
+
+        Integer timeout = params.getTimeout();


one liner?

Integer timeout = (params.getTimeout() != null && params.getTimeout() != GenerativeQAParameters.SIZE_NULL_VALUE) ? params.getTimeout() : MAX_PROCESSOR_TIME_IN_SECONDS;

Well, that would involve calling .getTimeout() up to three times. Does the Java compiler do something clever to avoid that?

That's a good point. We could do:

Integer timeoutValue = params.getTimeout(); Integer timeout = (timeoutValue != null && timeoutValue != GenerativeQAParameters.SIZE_NULL_VALUE) ? timeoutValue : MAX_PROCESSOR_TIME_IN_SECONDS;

But upto you, keep the code as it is if you want.

dhrubo-os · 2023-09-29T21:17:17Z

...g/opensearch/searchpipelines/questionanswering/generative/GenerativeQAResponseProcessor.java

+        List<Interaction> chatHistory = (conversationId == null) ? Collections.emptyList() : memoryClient.getInteractions(conversationId, interactionSize);
+        log.info("Retrieved chat history. ({})", getDuration(start));
+
+        Integer topN = params.getContextSize();


one liner?

Integer topN = params.getContextSize() != null ? params.getContextSize() : GenerativeQAParameters.SIZE_NULL_VALUE;

dhrubo-os · 2023-09-29T21:30:39Z

...va/org/opensearch/searchpipelines/questionanswering/generative/GenerativeSearchResponse.java

+            builder.field(GENERATIVE_QA_ERROR_FIELD_NAME, this.errorMessage);
+        } else {
+            /*     body of our stuff    */
+            builder.field(GENERATIVE_QA_ANSWER_FIELD_NAME, this.answer);


Apology if I missed it, don't we need corresponding parser?

There is a TODO at the top of the file to address your question.

dhrubo-os · 2023-09-29T21:31:20Z

...main/java/org/opensearch/searchpipelines/questionanswering/generative/prompt/PromptUtil.java

-        for (String result : contexts) {
-            messageArray.add(new Message(ChatRole.USER, "SEARCH RESULT: " + result).toJson());
+
+        /*


Are we keeping this?

Signed-off-by: Austin Lee <[email protected]>

austintlee · 2023-10-03T15:11:37Z

@dhrubo-os @jngz-es anything else?

HenryL27

lgtm

HenryL27 · 2023-10-03T18:22:46Z

...g/opensearch/searchpipelines/questionanswering/generative/GenerativeQAResponseProcessor.java

-    // TODO Add "interaction_count".  This is how far back in chat history we want to go back when calling LLM.
+    private static final int DEFAULT_PROCESSOR_TIME_IN_SECONDS = 30;
+
+    // TODO Add "interaction_count". This is how far back in chat history we want to go back when calling LLM.


would this be a hard add?

opensearch-project#1385) * Add support for context_size and include 'interaction_id' in SearchResponse. [Issue opensearch-project#1372] Signed-off-by: Austin Lee <[email protected]> * Added spotless, removed unused code, added more comments. Signed-off-by: Austin Lee <[email protected]> --------- Signed-off-by: Austin Lee <[email protected]> Signed-off-by: HenryL27 <[email protected]>

opensearch-project#1385) * Add support for context_size and include 'interaction_id' in SearchResponse. [Issue opensearch-project#1372] Signed-off-by: Austin Lee <[email protected]> * Added spotless, removed unused code, added more comments. Signed-off-by: Austin Lee <[email protected]> --------- Signed-off-by: Austin Lee <[email protected]> (cherry picked from commit ae6995a) Signed-off-by: HenryL27 <[email protected]>

#1385) (#1433) * Add support for context_size and include 'interaction_id' in SearchResponse. [Issue #1372] * Added spotless, removed unused code, added more comments. --------- (cherry picked from commit ae6995a) Signed-off-by: Austin Lee <[email protected]> Signed-off-by: HenryL27 <[email protected]> Co-authored-by: Austin Lee <[email protected]>

opensearch-project#1385) (opensearch-project#1433) * Add support for context_size and include 'interaction_id' in SearchResponse. [Issue opensearch-project#1372] * Added spotless, removed unused code, added more comments. --------- (cherry picked from commit ae6995a) Signed-off-by: Austin Lee <[email protected]> Signed-off-by: HenryL27 <[email protected]> Co-authored-by: Austin Lee <[email protected]> Signed-off-by: TrungBui59 <[email protected]>

Add support for context_size and include 'interaction_id' in SearchRe…

e499ec5

…sponse. [Issue opensearch-project#1372] Signed-off-by: Austin Lee <[email protected]>

austintlee had a problem deploying to ml-commons-cicd-env September 25, 2023 22:35 — with GitHub Actions Error

austintlee had a problem deploying to ml-commons-cicd-env September 25, 2023 22:35 — with GitHub Actions Failure

jngz-es reviewed Sep 28, 2023

View reviewed changes

austintlee had a problem deploying to ml-commons-cicd-env September 29, 2023 19:35 — with GitHub Actions Error

austintlee had a problem deploying to ml-commons-cicd-env September 29, 2023 19:35 — with GitHub Actions Failure

austintlee had a problem deploying to ml-commons-cicd-env September 29, 2023 20:58 — with GitHub Actions Error

austintlee temporarily deployed to ml-commons-cicd-env September 29, 2023 20:58 — with GitHub Actions Inactive

austintlee had a problem deploying to ml-commons-cicd-env September 29, 2023 20:58 — with GitHub Actions Failure

dhrubo-os reviewed Sep 29, 2023

View reviewed changes

Added spotless, removed unused code, added more comments.

857f436

Signed-off-by: Austin Lee <[email protected]>

austintlee temporarily deployed to ml-commons-cicd-env October 2, 2023 17:15 — with GitHub Actions Inactive

jngz-es approved these changes Oct 3, 2023

View reviewed changes

HenryL27 approved these changes Oct 3, 2023

View reviewed changes

dhrubo-os approved these changes Oct 3, 2023

View reviewed changes

dhrubo-os merged commit ae6995a into opensearch-project:2.x Oct 3, 2023
7 of 9 checks passed

HenryL27 mentioned this pull request Oct 4, 2023

[Backport] Add support for context_size and include 'interaction_id' in SearchRe… #1433

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for context_size and include 'interaction_id' in SearchRe… #1385

Add support for context_size and include 'interaction_id' in SearchRe… #1385

austintlee commented Sep 25, 2023

austintlee commented Sep 26, 2023

austintlee commented Sep 26, 2023

jngz-es Sep 28, 2023

austintlee Oct 2, 2023

jngz-es Sep 28, 2023

austintlee Oct 2, 2023

jngz-es Sep 28, 2023

austintlee Oct 2, 2023

codecov bot commented Sep 29, 2023 •

edited

Loading

dhrubo-os Sep 29, 2023

austintlee Oct 2, 2023

dhrubo-os Sep 29, 2023

austintlee Oct 2, 2023

dhrubo-os Sep 29, 2023

austintlee Oct 2, 2023

dhrubo-os Sep 29, 2023

austintlee Oct 2, 2023

dhrubo-os Sep 29, 2023

austintlee Oct 2, 2023

dhrubo-os Oct 2, 2023

dhrubo-os Sep 29, 2023

dhrubo-os Sep 29, 2023

austintlee Oct 2, 2023

dhrubo-os Sep 29, 2023

austintlee Oct 2, 2023

austintlee commented Oct 3, 2023

HenryL27 left a comment

HenryL27 Oct 3, 2023

		@@ -58,11 +60,16 @@ public class GenerativeQAResponseProcessor extends AbstractProcessor implements

		private static final int DEFAULT_CHAT_HISTORY_WINDOW = 10;

		private static final int MAX_PROCESSOR_TIME_IN_SECONDS = 60;

Add support for context_size and include 'interaction_id' in SearchRe… #1385

Add support for context_size and include 'interaction_id' in SearchRe… #1385

Conversation

austintlee commented Sep 25, 2023

Description

Issues Resolved

Check List

austintlee commented Sep 26, 2023

austintlee commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 29, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austintlee commented Oct 3, 2023

HenryL27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 29, 2023 •

edited

Loading