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

Add support for Bedrock Converse API (Anthropic Messages API, Claude 3.5 Sonnet) #2851

Merged
merged 11 commits into from
Sep 6, 2024

Conversation

austintlee
Copy link
Collaborator

Description

Adds support for latest Message API

Related Issues

Resolves #2826

Check List

  • [ x] New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • [ x] 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.

@austintlee
Copy link
Collaborator Author

I need a jpeg file and a pdf file for testing image and document contents and they are failing the utf-8 content checks.

I see that in the core, the ingest plugin has this:

 forbiddenPatterns {
   exclude '**/*.doc'
   exclude '**/*.docx'
   exclude '**/*.pdf'
   exclude '**/*.epub'
   exclude '**/*.vsdx'
 }

I can add that to plugin/build.gradle.

@Zhangxunmt
Copy link
Collaborator

@austintlee just a few comments left but LGTM overall.
Since this new API support will be used by the requester in #2826, do you mind adding a tutorial for using this API like this one https://github.com/opensearch-project/ml-commons/blob/main/docs/tutorials/conversational_search/conversational_search_with_Cohere_Command.md, and update the limitations specified in this tutorial? I am 100% sure that the requester will follow the tutorial to build solutions.

static class DummyStreamOutput extends StreamOutput {

List<String> list = new ArrayList<>();
List<Integer> intValues = new ArrayList<>();

@Override
public void writeString(String str) {
System.out.println("Adding string: " + str);
Copy link
Collaborator

Choose a reason for hiding this comment

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

a leftover? remove 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.

It's just for debugging. This is only in a test.

@@ -136,6 +136,19 @@ protected Map<String, String> getInputParameters(ChatCompletionInput chatComplet
chatCompletionInput.getContexts()
)
);
} else if (chatCompletionInput.getModelProvider() == ModelProvider.BEDROCK_CONVERSE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of keep adding new "else if" blocks here, why not just define an annotation for different methods and use reflection to process different models servers at runtime? In that way I think we could reduce the code complexity 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.

Given the time constraint, I would do the refactoring at a later time. I have been thinking about a good way to avoid this style of handling different cases of LLM vendors and APIs, but I was waiting for some general patterns to emerge which I think is this Message API, but again there are still small differences between LLM providers.

@austintlee
Copy link
Collaborator Author

@austintlee just a few comments left but LGTM overall. Since this new API support will be used by the requester in #2826, do you mind adding a tutorial for using this API like this one https://github.com/opensearch-project/ml-commons/blob/main/docs/tutorials/conversational_search/conversational_search_with_Cohere_Command.md, and update the limitations specified in this tutorial? I am 100% sure that the requester will follow the tutorial to build solutions.

Yes, I'll produce some documentation, but I don't have time this week. You can look at the IT tests to get a sense of the syntax for now if that helps with reviewing and making sense of the code.

Zhangxunmt
Zhangxunmt previously approved these changes Aug 30, 2024
}

JsonArray messageArray = new JsonArray();
MessageArrayBuilder bldr = new MessageArrayBuilder(provider);
Copy link
Collaborator

Choose a reason for hiding this comment

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

bldr is hard to understand , use more meaningful variable name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure.

@austintlee
Copy link
Collaborator Author

  {"error":{"root_cause":[{"type":"status_exception","reason":"Error from remote service: {\"message\":\"The security token included in the request is invalid.\"}"}],"type":"status_exception","reason":"Error from remote service: {\"message\":\"The security token included in the request is invalid.\"}"},"status":403}

@austintlee
Copy link
Collaborator Author

Is this an issue with GH setup?

Copy link
Collaborator

@mingshl mingshl Sep 5, 2024

Choose a reason for hiding this comment

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

Do we need license for the picture or pdf in this PR? @kolchfa-aws

Copy link
Collaborator

Choose a reason for hiding this comment

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

@austintlee is this image generated from openai image generation?

Copy link
Contributor

Choose a reason for hiding this comment

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

@austintlee Could you tell me the original source of this image please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI, that image is from OpenAI's API documentation - https://platform.openai.com/docs/api-reference/chat/create. It's in the example they give for including images in chat completion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The actual source is Wikipedia.

@Zhangxunmt
Copy link
Collaborator

@austintlee any objection to merge the PR if the issue is only in the test itself? Let’s use a separate PR to track the IT issue.

@austintlee
Copy link
Collaborator Author

RestBedRockInferenceIT > test_bedrock_multimodal_model FAILED
    org.opensearch.client.ResponseException: method [POST], host [http://[::1]:38749], URI [/_plugins/_ml/models/null/_deploy], status line [HTTP/1.1 404 Not Found]
    {"error":{"root_cause":[{"type":"status_exception","reason":"Failed to find model"}],"type":"status_exception","reason":"Failed to find model"},"status":404}

This is not my test. Is this a known issue? Some (internal) model got removed? Am I the only one seeing this issue?

@austintlee
Copy link
Collaborator Author

Also, all these time-outs we were seeing - I don't see this happening on Windows or Mac. Should I try to repro it on linux? It's really frustrating that I can't repro it locally.

@Zhangxunmt
Copy link
Collaborator

Also, all these time-outs we were seeing - I don't see this happening on Windows or Mac. Should I try to repro it on linux? It's really frustrating that I can't repro it locally.

That test flaky is because of the congestion in the ITs running environment. We should remove some outdated or duplicate ITs to unchoke. That multi modal IT should update to use auto deploy so it won’t fail due to model not found because it will be auto deployed in that case.

@austintlee
Copy link
Collaborator Author

Can we merge this?

@ylwu-amzn ylwu-amzn merged commit 17e81ae into opensearch-project:main Sep 6, 2024
9 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2851-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 17e81ae618bebc72b8d3cb76d57f1556b1d8c8e1
# Push it to GitHub
git push --set-upstream origin backport/backport-2851-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2851-to-2.x.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.17 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.17 2.17
# Navigate to the new working tree
cd .worktrees/backport-2.17
# Create a new branch
git switch --create backport/backport-2851-to-2.17
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 17e81ae618bebc72b8d3cb76d57f1556b1d8c8e1
# Push it to GitHub
git push --set-upstream origin backport/backport-2851-to-2.17
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.17

Then, create a pull request where the base branch is 2.17 and the compare/head branch is backport/backport-2851-to-2.17.

austintlee added a commit to austintlee/ml-commons that referenced this pull request Sep 6, 2024
…3.5 Sonnet) (opensearch-project#2851)

* Add support for Anthropic Message API (Issue 2826)

Signed-off-by: Austin Lee <austin@aryn.ai>

* Fix a bug.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Add unit tests, improve coverage, clean up code.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Allow pdf and jpg files for IT tests for multimodel conversation API testing.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Fix spotless check issues.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Update IT to work with session tokens.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Fix MLRAGSearchProcessorIT not to extend RestMLRemoteInferenceIT.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Use suite specific model group name.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Disable tests that require futher investigation.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Skip two additional tests with time-outs.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Restore a change from RestMLRemoteInferenceIT.

Signed-off-by: Austin Lee <austin@aryn.ai>

---------

Signed-off-by: Austin Lee <austin@aryn.ai>
(cherry picked from commit 17e81ae)
austintlee added a commit to austintlee/ml-commons that referenced this pull request Sep 6, 2024
…3.5 Sonnet) (opensearch-project#2851)

* Add support for Anthropic Message API (Issue 2826)

Signed-off-by: Austin Lee <austin@aryn.ai>

* Fix a bug.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Add unit tests, improve coverage, clean up code.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Allow pdf and jpg files for IT tests for multimodel conversation API testing.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Fix spotless check issues.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Update IT to work with session tokens.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Fix MLRAGSearchProcessorIT not to extend RestMLRemoteInferenceIT.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Use suite specific model group name.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Disable tests that require futher investigation.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Skip two additional tests with time-outs.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Restore a change from RestMLRemoteInferenceIT.

Signed-off-by: Austin Lee <austin@aryn.ai>

---------

Signed-off-by: Austin Lee <austin@aryn.ai>
(cherry picked from commit 17e81ae)
@austintlee
Copy link
Collaborator Author

I opened #2914 to bring back the rest of the tests.

@austintlee
Copy link
Collaborator Author

Doc - opensearch-project/documentation-website#8195. I don't have a PR, yet. Do I need to prepare a PR or is it someone from the docs team?

jngz-es pushed a commit that referenced this pull request Sep 9, 2024
…3.5 Sonnet) (#2851) (#2912)

* Add support for Anthropic Message API (Issue 2826)

Signed-off-by: Austin Lee <austin@aryn.ai>

* Fix a bug.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Add unit tests, improve coverage, clean up code.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Allow pdf and jpg files for IT tests for multimodel conversation API testing.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Fix spotless check issues.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Update IT to work with session tokens.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Fix MLRAGSearchProcessorIT not to extend RestMLRemoteInferenceIT.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Use suite specific model group name.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Disable tests that require futher investigation.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Skip two additional tests with time-outs.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Restore a change from RestMLRemoteInferenceIT.

Signed-off-by: Austin Lee <austin@aryn.ai>

---------

Signed-off-by: Austin Lee <austin@aryn.ai>
(cherry picked from commit 17e81ae)
jngz-es pushed a commit that referenced this pull request Sep 9, 2024
…3.5 Sonnet) (#2851) (#2913)

* Add support for Anthropic Message API (Issue 2826)

Signed-off-by: Austin Lee <austin@aryn.ai>

* Fix a bug.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Add unit tests, improve coverage, clean up code.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Allow pdf and jpg files for IT tests for multimodel conversation API testing.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Fix spotless check issues.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Update IT to work with session tokens.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Fix MLRAGSearchProcessorIT not to extend RestMLRemoteInferenceIT.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Use suite specific model group name.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Disable tests that require futher investigation.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Skip two additional tests with time-outs.

Signed-off-by: Austin Lee <austin@aryn.ai>

* Restore a change from RestMLRemoteInferenceIT.

Signed-off-by: Austin Lee <austin@aryn.ai>

---------

Signed-off-by: Austin Lee <austin@aryn.ai>
(cherry picked from commit 17e81ae)
@kolchfa-aws
Copy link
Contributor

@austintlee Our workflow is that the feature developer puts up a documentation PR in the doc repo and then the doc team reviews. Please feel free to ask any questions. Thank you!

Zhangxunmt added a commit to Zhangxunmt/ml-commons that referenced this pull request Sep 10, 2024
Zhangxunmt added a commit that referenced this pull request Sep 10, 2024
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.

Conversation search support for Message APIs
5 participants