-
Notifications
You must be signed in to change notification settings - Fork 183
Fix #4168: Support Claude V3 response format in DefaultLLMImpl #4275
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
base: main
Are you sure you want to change the base?
Fix #4168: Support Claude V3 response format in DefaultLLMImpl #4275
Conversation
aa64626
to
540a52e
Compare
Hi @mingshl @rithin-pullela-aws , Could you please review my PR and suggest changes. Thanks :) |
oh great, this is going to use the same interface string, and it will auto detect these two formats and parse accordingly. Can you also add in ITs? this is the best way we can monitor and keep track when the model interface failed. I ran your CIs and also I added the resolved issues and link the issue in the PR descriptions. |
Hi @mingshl @akolarkunnu @rithin-pullela-aws , Please review the code changes and run the test CIs. I also covered the test coverage in last PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for raising the PR!!
Added few comments
plugin/src/test/java/org/opensearch/ml/rest/RestBedRockInferenceIT.java
Outdated
Show resolved
Hide resolved
...ain/java/org/opensearch/searchpipelines/questionanswering/generative/llm/DefaultLlmImpl.java
Outdated
Show resolved
Hide resolved
...ain/java/org/opensearch/searchpipelines/questionanswering/generative/llm/DefaultLlmImpl.java
Outdated
Show resolved
Hide resolved
...ain/java/org/opensearch/searchpipelines/questionanswering/generative/llm/DefaultLlmImpl.java
Outdated
Show resolved
Hide resolved
...ain/java/org/opensearch/searchpipelines/questionanswering/generative/llm/DefaultLlmImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Anuj Soni <[email protected]>
Signed-off-by: Anuj Soni <[email protected]>
Signed-off-by: Anuj Soni <[email protected]>
…ntent, text) Signed-off-by: Anuj Soni <[email protected]>
Signed-off-by: Anuj Soni <[email protected]>
Signed-off-by: Anuj Soni <[email protected]>
a6199dc
to
cbe84d8
Compare
Hi @rithin-pullela-aws , sorry for bothering you again and again. But please if you have time to review them once and suggest me the changes, it'll keep me occupied by working on it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small Comment about the IT.
It should simple to have the IT fixed instead of writing a new one.
We can remove the BM25_SEARCH_REQUEST_WITH_CONVO_WITH_LLM_RESPONSE_TEMPLATE
from plugin/src/test/java/org/opensearch/ml/rest/RestMLRAGSearchProcessorIT.java
to have the IT hit the code change
private Map<String, Object> invokeBedrockInference(Map<String, Object> mockResponse) throws Exception { | ||
// Create DefaultLlmImpl and mock ML client | ||
DefaultLlmImpl connector = new DefaultLlmImpl("model_id", null); // Use getClient() from MLCommonsRestTestCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks more like a unit test than an integration test.
Generally in Integration tests, we make an actual call to the LLM and verify the response is as expected.
It should be something like:
GET /<index_name>/_search?search_pipeline=rag_pipeline
{
"query": {
"match": {
"text": "Abraham Lincoln"
}
},
"ext": {
"generative_qa_parameters": {
"llm_model": "bedrock/anthropic-claude",
"llm_question": "who is lincoln",
"system_prompt": "null",
"user_instructions": "null",
"context_size": 5,
"message_size": 5,
"timeout": 60
}
}
}
and we need to verify this is ITs.
This was being tested previously in the ITs, but we added a llm_response_field to get it working.
In this PR: dc8403f#diff-413a5184ffbe5b2e3f86084003df503a3b1fb86ec76ecf81ecb28ba213d67bca a new llm_response_field
was added to solve the issue. We can just undo the changes in this PR to check if the ITs pass after this change.
Now that code handles it we do not need this template: https://github.com/opensearch-project/ml-commons/blob/main/plugin/src/test/java/org/opensearch/ml/rest/RestMLRAGSearchProcessorIT.java#L473C33-L492
I believe instead of these changes we can try to update the other ITs
This PR updates the DefaultLlmImpl class to properly parse responses from the Claude V3 family of Bedrock models. Previously, only the V2 response format (completion field) was supported. With this change, the parser now supports the V3 format (content -> text) while maintaining backward compatibility with V2.
Changes Made
Detects "content" field for Claude V3 responses.
Extracts text from the first element of the content list.
Adds extracted text to answers.
Added a new unit test in DefaultLlmImplTests.java:
Mocks a V3-style Bedrock response and validates output parsing.
Reformatted test files to satisfy Spotless formatting rules.
Why this change is needed
Claude V2 models are deprecated; V3 is the current standard.
Without this fix, requests to "bedrock/anthropic-claude" with V3 models fail to extract answers, causing RAG pipelines to break.
Ensures users don’t need to pass llmResponseField manually for default Bedrock Claude models.
How to test
Run all existing unit tests:
./gradlew clean build
resolves #4168 (comment)