Add pre and post process functions for Bedrock Rerank API #3254 #3339

tkykenmt · 2025-01-07T04:00:13Z

Description

Amazon Bedrock introduced Rerank model support. OpenSearch can invoke Rerank models on Bedrock by writing custom pre and post processing function, but pre-built function is good for performance.

Related Issues

Resolves #3254

Check List

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

…project#3254 Signed-off-by: tkykenmt <[email protected]>

dhrubo-os · 2025-01-07T15:10:44Z

Apply Spotless

brianf-aws

Minor feedback on validation/converting scores.

brianf-aws · 2025-01-07T20:44:58Z

...g/opensearch/ml/common/connector/functions/postprocess/BedrockRerankPostProcessFunction.java

+        }
+        List<?> outerList = (List<?>) input;
+        if (!outerList.isEmpty()) {
+            if (!(outerList.get(0) instanceof Map)) {


can we make this iteratively? That way we check all the items for having a map and a valid index and relevance score key

Thanks, will be updated on the next commit

updated on 4af169a

brianf-aws · 2025-01-07T20:49:09Z

...g/opensearch/ml/common/connector/functions/postprocess/BedrockRerankPostProcessFunction.java

+                scores[index] = switch (relevanceScore) {
+                    case BigDecimal bd -> bd.doubleValue();
+                    case Double d -> d;
+                    case null -> throw new IllegalArgumentException("relevanceScore is null");
+                    default -> throw new IllegalArgumentException("Unexpected type for relevanceScore: " +
+                            relevanceScore.getClass().getName());
+                };
+            }


This is nice but I believe it will cause issues on backports without this feature what if we exctracted this into a method and do manual if-else checks?

Something like

private Double toDouble(Object score) { if (relevanceScore instanceof BigDecimal) { return ((BigDecimal) relevanceScore).doubleValue(); } else if (relevanceScore instanceof Double) { return (Double) relevanceScore; } else if (relevanceScore == null) { throw new IllegalArgumentException("relevanceScore is null"); } throw new IllegalArgumentException("Unexpected type for relevanceScore: " + relevanceScore.getClass().getName()); }

Thanks, will be updated on the next commit

updated on 4af169a

brianf-aws · 2025-01-07T21:42:04Z

...org/opensearch/ml/common/connector/functions/preprocess/BedrockRerankPreProcessFunction.java

+
+    @Override
+    public void validate(MLInput mlInput) {
+        if (!(mlInput.getInputDataset() instanceof TextSimilarityInputDataSet)) {


Maybe also check for null before getInputDataset()?

Thanks, will be updated on the next commit

I've checked null check is already implemented in apply method in superclass, ConnectorPreProcessFunction and ConnectorPostProcessFunction. apply method is wrapping validate method.

https://github.com/opensearch-project/ml-commons/blob/main/common/src/main/java/org/opensearch/ml/common/connector/functions/preprocess/ConnectorPreProcessFunction.java

https://github.com/opensearch-project/ml-commons/blob/main/common/src/main/java/org/opensearch/ml/common/connector/functions/postprocess/ConnectorPostProcessFunction.java

Thus, I think it's not necessary to implement nullcheck in validate method again.

I've just added following validation in validate method.

if (mlInput.getInputDataset() == null) { throw new IllegalArgumentException("Input dataset cannot be null."); }

updated on 4af169a

brianf-aws · 2025-01-07T21:47:36Z

...ensearch/ml/common/connector/functions/postprocess/BedrockRerankPostProcessFunctionTest.java

+        exceptionRule.expectMessage("The rerank result should contain index and relevance_score.");
+        function.apply(Arrays.asList(Map.of("test1", "value1")));
+    }
+


Maybe lets make a null test? just so we can understand?

null test can be implemented by referring superclass, but writing superclass's test case in test class for extended class can lead build failure when superclass is updated.

brianf-aws · 2025-01-07T21:49:49Z

...ensearch/ml/common/connector/functions/postprocess/BedrockRerankPostProcessFunctionTest.java

+                Map.of("index", 2, "relevanceScore", 0.7711548805236816),
+                Map.of("index", 0, "relevanceScore", 0.0025114635936915874),
+                Map.of("index", 1, "relevanceScore", 2.4876489987946115e-05),
+                Map.of("index", 3, "relevanceScore", 6.339210358419223e-06)


Currently this will pass when just the first is in correct format but does not check the rest. Like mentioned early if you can change the validation to check each entry is in the right format

Let me add a test case to check a list having incorrect map. will update on the next commit.

@Test public void process_WrongInput_NotCorrectMap() { exceptionRule.expect(IllegalArgumentException.class); exceptionRule.expectMessage("Rerank result should have both index and relevanceScore."); List<Map<String, Object>> rerankResults = List .of( Map.of("index", 2, "relevanceScore", 0.7711548805236816), Map.of("index", 0, "relevanceScore", 0.0025114635936915874), Map.of("index", 1, "relevanceScore", 2.4876489987946115e-05), Map.of("test1", "value1") ); function.apply(rerankResults); }

updated on 4af169a

Signed-off-by: tkykenmt <[email protected]>

brianf-aws · 2025-01-08T18:21:15Z

Looks like there is a build failure on a test I thought I fixed but I can see there is a different underlying problem within the IT involving encryption

VisualizationsToolIT > testVisualizationFound FAILED
    org.opensearch.client.ResponseException: method [POST], host [http://127.0.0.1:54529/], URI [/_plugins/_ml/agents/bLjaRJQB515KRnslfdUv/_execute], status line [HTTP/1.1 500 Internal Server Error]
    {"status":500,"error":{"type":"AEADBadTagException","reason":"System Error","details":"Tag mismatch"}}
        at app//org.opensearch.client.RestClient.convertResponse(RestClient.java:501)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:384)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:359)
        at app//org.opensearch.ml.utils.TestHelper.makeRequest(TestHelper.java:182)
        at app//org.opensearch.ml.utils.TestHelper.makeRequest(TestHelper.java:155)
        at app//org.opensearch.ml.utils.TestHelper.makeRequest(TestHelper.java:144)
        at app//org.opensearch.ml.tools.VisualizationsToolIT.testVisualizationFound(VisualizationsToolIT.java:74)

    java.lang.AssertionError: The response failed to meet condition after 5 attempts. Attempted to perform GET : /_plugins/_ml/models/arjaRJQB515KRnsleNWv
        at org.junit.Assert.fail(Assert.java:89)
        at org.opensearch.ml.tools.ToolIntegrationWithLLMTest.waitResponseMeetingCondition(ToolIntegrationWithLLMTest.java:103)
        at org.opensearch.ml.tools.ToolIntegrationWithLLMTest.checkForModelUndeployedStatus(ToolIntegrationWithLLMTest.java:89)
        at org.opensearch.ml.tools.ToolIntegrationWithLLMTest.deleteModel(ToolIntegrationWithLLMTest.java:74)
        at

... 

2> REPRODUCE WITH: gradlew ':opensearch-ml-plugin:integTest' --tests "org.opensearch.ml.tools.VisualizationsToolIT.testVisualizationFound" -Dtests.seed=AD7A0603B7C68274 -Dtests.security.manager=false -Dtests.locale=fr-GN -Dtests.timezone=America/Argentina/Buenos_Aires -Druntime.java=21
  2> org.opensearch.client.ResponseException: method [POST], host [http://127.0.0.1:54529/], URI [/_plugins/_ml/agents/bLjaRJQB515KRnslfdUv/_execute], status line [HTTP/1.1 500 Internal Server Error]
    {"status":500,"error":{"type":"AEADBadTagException","reason":"System Error","details":"Tag mismatch"}}
        at app//org.opensearch.client.RestClient.convertResponse(RestClient.java:501)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:384)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:359)
        at app//org.opensearch.ml.utils.TestHelper.makeRequest(TestHelper.java:182)
        at app//org.opensearch.ml.utils.TestHelper.makeRequest(TestHelper.java:155)
        at app//org.opensearch.ml.utils.TestHelper.makeRequest(TestHelper.java:144)
        at app//org.opensearch.ml.tools.VisualizationsToolIT.testVisualizationFound(VisualizationsToolIT.java:74)

    java.lang.AssertionError: The response failed to meet condition after 5 attempts. Attempted to perform GET : /_plugins/_ml/models/arjaRJQB515KRnsleNWv
        at org.junit.Assert.fail(Assert.java:89)
        at org.opensearch.ml.tools.ToolIntegrationWithLLMTest.waitResponseMeetingCondition(ToolIntegrationWithLLMTest.java:103)
        at org.opensearch.ml.tools.ToolIntegrationWithLLMTest.checkForModelUndeployedStatus(ToolIntegrationWithLLMTest.java:89)
        at org.opensearch.ml.tools.ToolIntegrationWithLLMTest.deleteModel(ToolIntegrationWithLLMTest.java:74)
        at

See here and here

brianf-aws

I agree with the code changes It covers all the possible scenarios I can think of.

brianf-aws · 2025-01-08T21:52:36Z

...ensearch/ml/common/connector/functions/postprocess/BedrockRerankPostProcessFunctionTest.java

+        exceptionRule.expectMessage("Rerank result is empty.");
+        function.apply(Arrays.asList(Map.of()));
+    }
+
    @Test
    public void process_WrongInput_NotCorrectMap() {


process_WrongInput_NotCorrectListOfMapsFormat(){
}

updated on the commit 8a4fdb2

dhrubo-os · 2025-01-08T22:05:54Z

Are you planning to update the corresponding blueprint in a separate PR?

…ect#3339 Signed-off-by: tkykenmt <[email protected]>

tkykenmt · 2025-01-09T00:55:38Z

@dhrubo-os
I'm willing to add blueprint under the directory.

https://github.com/opensearch-project/ml-commons/tree/main/docs/remote_inference_blueprints

tkykenmt · 2025-01-09T04:21:17Z

@dhrubo-os
I submitted another PR for a blueprint and new tutorial.
#3352

dhrubo-os · 2025-01-10T00:52:02Z

...g/opensearch/ml/common/connector/functions/postprocess/BedrockRerankPostProcessFunction.java

+
+        if (!rerankResults.isEmpty()) {
+            Double[] scores = new Double[rerankResults.size()];
+            for (Map<?, ?> rerankResult : rerankResults) {


why do we need to cast the elements as Map? We defined this as parameter: List<Map<String, Object>> rerankResults.

Thank you for your review. I agree with you. For the logic, we don't need to cast as Map but just specify data type for enhanced loop as follows.

for (Map rerankResult : rerankResults) {

fixed on ededf78

Signed-off-by: tkykenmt <[email protected]>

Add pre and post process functions for Bedrock Rerank API opensearch-…

3f7a00f

…project#3254 Signed-off-by: tkykenmt <[email protected]>

tkykenmt requested review from b4sjoo, dhrubo-os, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27 and xinyual as code owners January 7, 2025 04:00

tkykenmt had a problem deploying to ml-commons-cicd-env-require-approval January 7, 2025 04:00 — with GitHub Actions Failure

This was referenced Jan 7, 2025

[DOC] Add support for Bedrock Rerank API opensearch-project/documentation-website#9027

Closed

Add support for Bedrock Rerank API #9027 opensearch-project/documentation-website#9029

Merged

brianf-aws suggested changes Jan 7, 2025

View reviewed changes

modify format using spotlessApply

316de2c

Signed-off-by: tkykenmt <[email protected]>

tkykenmt had a problem deploying to ml-commons-cicd-env-require-approval January 7, 2025 23:14 — with GitHub Actions Failure

Fix on validation/converting scores opensearch-project#3339

4af169a

Signed-off-by: tkykenmt <[email protected]>

tkykenmt had a problem deploying to ml-commons-cicd-env-require-approval January 8, 2025 06:57 — with GitHub Actions Failure

tkykenmt temporarily deployed to ml-commons-cicd-env-require-approval January 8, 2025 06:57 — with GitHub Actions Inactive

tkykenmt had a problem deploying to ml-commons-cicd-env-require-approval January 8, 2025 08:04 — with GitHub Actions Failure

brianf-aws approved these changes Jan 8, 2025

View reviewed changes

Fix on method name of test case for list of maps data opensearch-proj…

8a4fdb2

…ect#3339 Signed-off-by: tkykenmt <[email protected]>

tkykenmt temporarily deployed to ml-commons-cicd-env-require-approval January 9, 2025 00:52 — with GitHub Actions Inactive

dhrubo-os reviewed Jan 10, 2025

View reviewed changes

tkykenmt requested a deployment to ml-commons-cicd-env-require-approval January 10, 2025 01:39 — with GitHub Actions Waiting

remove unnecessary cast opensearch-project#3339

ededf78

Signed-off-by: tkykenmt <[email protected]>

tkykenmt requested a deployment to ml-commons-cicd-env-require-approval January 10, 2025 05:41 — with GitHub Actions Waiting

tkykenmt requested a review from dhrubo-os January 14, 2025 06:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pre and post process functions for Bedrock Rerank API #3254 #3339

Add pre and post process functions for Bedrock Rerank API #3254 #3339

tkykenmt commented Jan 7, 2025

dhrubo-os commented Jan 7, 2025

brianf-aws left a comment

brianf-aws Jan 7, 2025

tkykenmt Jan 8, 2025

tkykenmt Jan 8, 2025

brianf-aws Jan 7, 2025

tkykenmt Jan 8, 2025

tkykenmt Jan 8, 2025

brianf-aws Jan 7, 2025

tkykenmt Jan 8, 2025

tkykenmt Jan 8, 2025

tkykenmt Jan 8, 2025

tkykenmt Jan 8, 2025

brianf-aws Jan 7, 2025

tkykenmt Jan 8, 2025

brianf-aws Jan 7, 2025

tkykenmt Jan 8, 2025 •

edited

Loading

tkykenmt Jan 8, 2025

brianf-aws commented Jan 8, 2025 •

edited

Loading

brianf-aws left a comment

brianf-aws Jan 8, 2025

tkykenmt Jan 9, 2025

dhrubo-os commented Jan 8, 2025

tkykenmt commented Jan 9, 2025

tkykenmt commented Jan 9, 2025

dhrubo-os Jan 10, 2025

tkykenmt Jan 10, 2025

tkykenmt Jan 10, 2025

Add pre and post process functions for Bedrock Rerank API #3254 #3339

Are you sure you want to change the base?

Add pre and post process functions for Bedrock Rerank API #3254 #3339

Conversation

tkykenmt commented Jan 7, 2025

Description

Related Issues

Check List

dhrubo-os commented Jan 7, 2025

brianf-aws left a 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

tkykenmt Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianf-aws commented Jan 8, 2025 • edited Loading

brianf-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhrubo-os commented Jan 8, 2025

tkykenmt commented Jan 9, 2025

tkykenmt commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkykenmt Jan 8, 2025 •

edited

Loading

brianf-aws commented Jan 8, 2025 •

edited

Loading