Fix: Gracefully handle error when generative_qa_parameters is not pro…

…vided (#3100) * fix: gracefully handle error when generative_qa_parameters is not provided Signed-off-by: Pavan Yekbote <[email protected]> * fix: spotless apply Signed-off-by: Pavan Yekbote <[email protected]> * docs: adding documentation link to error message Signed-off-by: Pavan Yekbote <[email protected]> * tests: adding UT to test null params Signed-off-by: Pavan Yekbote <[email protected]> --------- Signed-off-by: Pavan Yekbote <[email protected]> (cherry picked from commit 0f7481e)
opensearch-project · Oct 15, 2024 · b6736dc · b6736dc
1 parent 5e2863f
commit b6736dc
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 0 deletions.
diff --git a/...ensearch/searchpipelines/questionanswering/generative/GenerativeQAProcessorConstants.java b/...ensearch/searchpipelines/questionanswering/generative/GenerativeQAProcessorConstants.java
@@ -43,4 +43,8 @@ public class GenerativeQAProcessorConstants {
         .boolSetting("plugins.ml_commons.rag_pipeline_feature_enabled", true, Setting.Property.NodeScope, Setting.Property.Dynamic);
 
     public static final String FEATURE_NOT_ENABLED_ERROR_MSG = RAG_PIPELINE_FEATURE_ENABLED.getKey() + " is not enabled.";
+
+    public static final String RAG_NULL_GEN_QA_PARAMS_ERROR_MSG = "generative_qa_parameters not found."
+        + " Please provide ext.generative_qa_parameters to proceed."
+        + " For more info, refer: https://opensearch.org/docs/latest/search-plugins/conversational-search/#step-6-use-the-pipeline-for-rag";
 }
diff --git a/...pensearch/searchpipelines/questionanswering/generative/GenerativeQAResponseProcessor.java b/...pensearch/searchpipelines/questionanswering/generative/GenerativeQAResponseProcessor.java
@@ -18,6 +18,7 @@
 package org.opensearch.searchpipelines.questionanswering.generative;
 
 import static org.opensearch.ingest.ConfigurationUtils.newConfigurationException;
+import static org.opensearch.searchpipelines.questionanswering.generative.GenerativeQAProcessorConstants.RAG_NULL_GEN_QA_PARAMS_ERROR_MSG;
 
 import java.time.Duration;
 import java.time.Instant;
@@ -126,6 +127,9 @@ public void processResponseAsync(
         }
 
         GenerativeQAParameters params = GenerativeQAParamUtil.getGenerativeQAParameters(request);
+        if (params == null) {
+            throw new IllegalArgumentException(RAG_NULL_GEN_QA_PARAMS_ERROR_MSG);
+        }
 
         Integer t = params.getTimeout();
         if (t == null || t == GenerativeQAParameters.SIZE_NULL_VALUE) {

diff --git a/...arch/searchpipelines/questionanswering/generative/GenerativeQAResponseProcessorTests.java b/...arch/searchpipelines/questionanswering/generative/GenerativeQAResponseProcessorTests.java
@@ -24,6 +24,7 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
+import static org.opensearch.searchpipelines.questionanswering.generative.GenerativeQAProcessorConstants.RAG_NULL_GEN_QA_PARAMS_ERROR_MSG;
 
 import java.time.Instant;
 import java.util.Collections;
@@ -646,6 +647,77 @@ public void testProcessResponseNullValueInteractions() throws Exception {
         }));
     }
 
+    public void testProcessResponseIllegalArgumentForNullParams() throws Exception {
+        exceptionRule.expect(IllegalArgumentException.class);
+        exceptionRule.expectMessage(RAG_NULL_GEN_QA_PARAMS_ERROR_MSG);
+
+        Client client = mock(Client.class);
+        Map<String, Object> config = new HashMap<>();
+        config.put(GenerativeQAProcessorConstants.CONFIG_NAME_MODEL_ID, "dummy-model");
+        config.put(GenerativeQAProcessorConstants.CONFIG_NAME_CONTEXT_FIELD_LIST, List.of("text"));
+
+        GenerativeQAResponseProcessor processor = (GenerativeQAResponseProcessor) new GenerativeQAResponseProcessor.Factory(
+            client,
+            alwaysOn
+        ).create(null, "tag", "desc", true, config, null);
+
+        ConversationalMemoryClient memoryClient = mock(ConversationalMemoryClient.class);
+        List<Interaction> chatHistory = List
+            .of(
+                new Interaction(
+                    "0",
+                    Instant.now(),
+                    "1",
+                    "question",
+                    "",
+                    "answer",
+                    "foo",
+                    Collections.singletonMap("meta data", "some meta")
+                )
+            );
+        doAnswer(invocation -> {
+            ((ActionListener<List<Interaction>>) invocation.getArguments()[2]).onResponse(chatHistory);
+            return null;
+        }).when(memoryClient).getInteractions(any(), anyInt(), any());
+        processor.setMemoryClient(memoryClient);
+
+        SearchRequest request = new SearchRequest();
+        SearchSourceBuilder sourceBuilder = new SearchSourceBuilder();
+        GenerativeQAParamExtBuilder extBuilder = new GenerativeQAParamExtBuilder();
+        extBuilder.setParams(null);
+        request.source(sourceBuilder);
+        sourceBuilder.ext(List.of(extBuilder));
+
+        int numHits = 10;
+        SearchHit[] hitsArray = new SearchHit[numHits];
+        for (int i = 0; i < numHits; i++) {
+            XContentBuilder sourceContent = JsonXContent
+                .contentBuilder()
+                .startObject()
+                .field("_id", String.valueOf(i))
+                .field("text", "passage" + i)
+                .field("title", "This is the title for document " + i)
+                .endObject();
+            hitsArray[i] = new SearchHit(i, "doc" + i, Map.of(), Map.of());
+            hitsArray[i].sourceRef(BytesReference.bytes(sourceContent));
+        }
+
+        SearchHits searchHits = new SearchHits(hitsArray, null, 1.0f);
+        SearchResponseSections internal = new SearchResponseSections(searchHits, null, null, false, false, null, 0);
+        SearchResponse response = new SearchResponse(internal, null, 1, 1, 0, 1, null, null, null);
+
+        Llm llm = mock(Llm.class);
+        processor.setLlm(llm);
+
+        processor
+            .processResponseAsync(
+                request,
+                response,
+                null,
+                ActionListener.wrap(r -> { assertTrue(r instanceof GenerativeSearchResponse); }, e -> {})
+            );
+    }
+
     public void testProcessResponseIllegalArgument() throws Exception {
         exceptionRule.expect(IllegalArgumentException.class);
         exceptionRule.expectMessage("llm_model cannot be null.");