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

Allow llmQuestion to be optional when llmMessages is used. (Issue #3… #3072

Merged
merged 2 commits into from
Oct 9, 2024
Merged

Allow llmQuestion to be optional when llmMessages is used. (Issue #3… #3072

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
138def6
Allow llmQuestion to be optional when llmMessages is used. (Issue #3…
austintlee Oct 8, 2024
1492608
Remove unused lines.
austintlee Oct 9, 2024
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
24 changes: 18 additions & 6 deletions plugin/src/test/java/org/opensearch/ml/rest/RestMLRAGSearchProcessorIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ public class RestMLRAGSearchProcessorIT extends MLCommonsRestTestCase {
+ " \"ext\": {\n"
+ " \"generative_qa_parameters\": {\n"
+ " \"llm_model\": \"%s\",\n"
+ " \"llm_question\": \"%s\",\n"
// + " \"llm_question\": \"%s\",\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not removing this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do.

+ " \"system_prompt\": \"%s\",\n"
+ " \"user_instructions\": \"%s\",\n"
+ " \"context_size\": %d,\n"
Expand All @@ -378,7 +378,7 @@ public class RestMLRAGSearchProcessorIT extends MLCommonsRestTestCase {
+ " \"ext\": {\n"
+ " \"generative_qa_parameters\": {\n"
+ " \"llm_model\": \"%s\",\n"
+ " \"llm_question\": \"%s\",\n"
// + " \"llm_question\": \"%s\",\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will remove it.

// + " \"system_prompt\": \"%s\",\n"
+ " \"user_instructions\": \"%s\",\n"
+ " \"context_size\": %d,\n"
Expand Down Expand Up @@ -723,8 +723,12 @@ public void testBM25WithBedrock() throws Exception {
public void testBM25WithBedrockConverse() throws Exception {
// Skip test if key is null
if (AWS_ACCESS_KEY_ID == null) {
System.out.println("Skipping testBM25WithBedrockConverse because AWS_ACCESS_KEY_ID is null");
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor: can we use log ?

return;
}

System.out.println("Running testBM25WithBedrockConverse");

Response response = createConnector(BEDROCK_CONVERSE_CONNECTOR_BLUEPRINT);
Map responseMap = parseResponseToMap(response);
String connectorId = (String) responseMap.get("connector_id");
Expand Down Expand Up @@ -775,8 +779,11 @@ public void testBM25WithBedrockConverse() throws Exception {
public void testBM25WithBedrockConverseUsingLlmMessages() throws Exception {
// Skip test if key is null
if (AWS_ACCESS_KEY_ID == null) {
System.out.println("Skipping testBM25WithBedrockConverseUsingLlmMessages because AWS_ACCESS_KEY_ID is null");
return;
}
System.out.println("Running testBM25WithBedrockConverseUsingLlmMessages");

Response response = createConnector(BEDROCK_CONVERSE_CONNECTOR_BLUEPRINT2);
Map responseMap = parseResponseToMap(response);
String connectorId = (String) responseMap.get("connector_id");
Expand Down Expand Up @@ -835,8 +842,11 @@ public void testBM25WithBedrockConverseUsingLlmMessages() throws Exception {
public void testBM25WithBedrockConverseUsingLlmMessagesForDocumentChat() throws Exception {
// Skip test if key is null
if (AWS_ACCESS_KEY_ID == null) {
System.out.println("Skipping testBM25WithBedrockConverseUsingLlmMessagesForDocumentChat because AWS_ACCESS_KEY_ID is null");
return;
}

System.out.println("Running testBM25WithBedrockConverseUsingLlmMessagesForDocumentChat");
Response response = createConnector(BEDROCK_DOCUMENT_CONVERSE_CONNECTOR_BLUEPRINT2);
Map responseMap = parseResponseToMap(response);
String connectorId = (String) responseMap.get("connector_id");
Expand Down Expand Up @@ -894,8 +904,11 @@ public void testBM25WithBedrockConverseUsingLlmMessagesForDocumentChat() throws
public void testBM25WithOpenAIWithConversation() throws Exception {
// Skip test if key is null
if (OPENAI_KEY == null) {
System.out.println("Skipping testBM25WithOpenAIWithConversation because OPENAI_KEY is null");
return;
}
System.out.println("Running testBM25WithOpenAIWithConversation");

Response response = createConnector(OPENAI_CONNECTOR_BLUEPRINT);
Map responseMap = parseResponseToMap(response);
String connectorId = (String) responseMap.get("connector_id");
Expand Down Expand Up @@ -951,8 +964,11 @@ public void testBM25WithOpenAIWithConversation() throws Exception {
public void testBM25WithOpenAIWithConversationAndImage() throws Exception {
// Skip test if key is null
if (OPENAI_KEY == null) {
System.out.println("Skipping testBM25WithOpenAIWithConversationAndImage because OPENAI_KEY is null");
return;
}
System.out.println("Running testBM25WithOpenAIWithConversationAndImage");

Response response = createConnector(OPENAI_4o_CONNECTOR_BLUEPRINT);
Map responseMap = parseResponseToMap(response);
String connectorId = (String) responseMap.get("connector_id");
Expand Down Expand Up @@ -1245,7 +1261,6 @@ private Response performSearch(String indexName, String pipeline, int size, Sear
requestParameters.source,
requestParameters.match,
requestParameters.llmModel,
requestParameters.llmQuestion,
requestParameters.systemPrompt,
requestParameters.userInstructions,
requestParameters.contextSize,
Expand All @@ -1268,8 +1283,6 @@ private Response performSearch(String indexName, String pipeline, int size, Sear
requestParameters.source,
requestParameters.match,
requestParameters.llmModel,
requestParameters.llmQuestion,
// requestParameters.systemPrompt,
requestParameters.userInstructions,
requestParameters.contextSize,
requestParameters.interactionSize,
Expand Down Expand Up @@ -1309,7 +1322,6 @@ private Response performSearch(String indexName, String pipeline, int size, Sear
requestParameters.source,
requestParameters.match,
requestParameters.llmModel,
requestParameters.llmQuestion,
requestParameters.systemPrompt,
requestParameters.userInstructions,
requestParameters.contextSize,
Expand Down
14 changes: 7 additions & 7 deletions ...g/opensearch/searchpipelines/questionanswering/generative/ext/GenerativeQAParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,9 +167,11 @@ public GenerativeQAParameters(
this.conversationId = conversationId;
this.llmModel = llmModel;

// TODO: keep this requirement until we can extract the question from the query or from the request processor parameters
// for question rewriting.
Preconditions.checkArgument(!Strings.isNullOrEmpty(llmQuestion), LLM_QUESTION + " must be provided.");
Preconditions
.checkArgument(
!(Strings.isNullOrEmpty(llmQuestion) && (llmMessages == null || llmMessages.isEmpty())),
"At least one of " + LLM_QUESTION + " or " + LLM_MESSAGES_FIELD + " must be provided."
);
this.llmQuestion = llmQuestion;
this.systemPrompt = systemPrompt;
this.userInstructions = userInstructions;
Expand All @@ -185,7 +187,7 @@ public GenerativeQAParameters(
public GenerativeQAParameters(StreamInput input) throws IOException {
this.conversationId = input.readOptionalString();
this.llmModel = input.readOptionalString();
this.llmQuestion = input.readString();
this.llmQuestion = input.readOptionalString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @austintlee this does not quite make sense to me, why you only make llmQuestion into optional while keeping llmMessage as mandatory field, if you are trying to make user to choose one of them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@b4sjoo I do make them both optional in the first constructor:

public GenerativeQAParameters(
        String conversationId,
        String llmModel,
        String llmQuestion,
        String systemPrompt,
        String userInstructions,
        Integer contextSize,
        Integer interactionSize,
        Integer timeout,
        String llmResponseField,
        List<MessageBlock> llmMessages
    ) {
        this.conversationId = conversationId;
        this.llmModel = llmModel;

        Preconditions
            .checkArgument(
                !(Strings.isNullOrEmpty(llmQuestion) && (llmMessages == null || llmMessages.isEmpty())),
                "At least one of " + LLM_QUESTION + " or " + LLM_MESSAGES_FIELD + " must be provided."
            );
        this.llmQuestion = llmQuestion;
        this.systemPrompt = systemPrompt;
        this.userInstructions = userInstructions;
        this.contextSize = (contextSize == null) ? SIZE_NULL_VALUE : contextSize;
        this.interactionSize = (interactionSize == null) ? SIZE_NULL_VALUE : interactionSize;
        this.timeout = (timeout == null) ? SIZE_NULL_VALUE : timeout;
        this.llmResponseField = llmResponseField;
        if (llmMessages != null) {
            this.llmMessages.addAll(llmMessages);
        }
    }

But internally, llmMessages is never null and by default is an empty array.

So, when we write out to StreamOut, we don't need to do a null check:

public void writeTo(StreamOutput out) throws IOException {
        out.writeOptionalString(conversationId);
        out.writeOptionalString(llmModel);
        out.writeOptionalString(llmQuestion);
        out.writeOptionalString(systemPrompt);
        out.writeOptionalString(userInstructions);
        out.writeInt(contextSize);
        out.writeInt(interactionSize);
        out.writeInt(timeout);
        out.writeOptionalString(llmResponseField);
        out.writeList(llmMessages);
    }

Which is why I always expect it to be present (at least as an empty list) when I read it back:

public GenerativeQAParameters(StreamInput input) throws IOException {
        this.conversationId = input.readOptionalString();
        this.llmModel = input.readOptionalString();
        this.llmQuestion = input.readOptionalString();
        this.systemPrompt = input.readOptionalString();
        this.userInstructions = input.readOptionalString();
        this.contextSize = input.readInt();
        this.interactionSize = input.readInt();
        this.timeout = input.readInt();
        this.llmResponseField = input.readOptionalString();
        this.llmMessages.addAll(input.readList(MessageBlock::new));
    }

Is this an incorrect assumption? Does the StreamInput constructor need to consider llmMessages not being present in input?

You can also take a look at stream roundtrip test cases I have in GenerativeQAParamExtBuilderTests.

Copy link
Collaborator

@b4sjoo b4sjoo Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's just because I saw a null check above and then you make here mandatory makes me confused. I think your answer makes sense to me, that llmMessage should never be null due to an empty list created

Copy link
Collaborator

@b4sjoo b4sjoo Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

BTW, changing a readString() into readOptionalString() could potentially introduce a bwc issue when we have a mixed cluster

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Going from required to optional should be OK, but not the other way around. How do we test it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pyek-bot is currently testing it, he should have a result by tomorrow. Basically we create a lower version cluster (e.g. 2.16) with dedicated master node, then we upgrade the data node to the current version to test. After this we perform the test again, but we upgrade master this time. Does this make sense?

Copy link
Contributor

@pyek-bot pyek-bot Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have tested this scenario. It seems to work fine when all nodes are eventually upgraded to 2.17.

  • When only the data node is upgraded, the NPE [https://github.com/[BUG] RAG processor throws null pointer exception #2983] comes into play since the master cannot serialize the data and send it to the data node.
  • When only the master node is upgraded, the data node cannot de-serialize due to new format and throws unexpected byte error.

However, when both are upgraded it works as expected with both llmQuestion and llmMessages.

this.systemPrompt = input.readOptionalString();
this.userInstructions = input.readOptionalString();
this.contextSize = input.readInt();
Expand Down Expand Up @@ -246,9 +248,7 @@ public XContentBuilder toXContent(XContentBuilder xContentBuilder, Params params
public void writeTo(StreamOutput out) throws IOException {
out.writeOptionalString(conversationId);
out.writeOptionalString(llmModel);

Preconditions.checkNotNull(llmQuestion, "llm_question must not be null.");
out.writeString(llmQuestion);
out.writeOptionalString(llmQuestion);
out.writeOptionalString(systemPrompt);
out.writeOptionalString(userInstructions);
out.writeInt(contextSize);
Expand Down
3 changes: 1 addition & 2 deletions ...ch/searchpipelines/questionanswering/generative/ext/GenerativeQAParamExtBuilderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,7 @@ public void testMiscMethods() throws IOException {

StreamOutput so = mock(StreamOutput.class);
builder1.writeTo(so);
verify(so, times(5)).writeOptionalString(any());
verify(so, times(1)).writeString(any());
verify(so, times(6)).writeOptionalString(any());
}

public void testParse() throws IOException {
Expand Down
Loading