Add validations for Personalize input and configurations #151

kulket · 2023-07-05T23:59:18Z

Description

This pull request adds following changes:

Change scoring logic for Personalized ranking to consider ranks instead of scores as well as refactor overall scoring logic implementation
Add validation for search request parameters as well as response processor configurations related to Personalization
Add E2E tests in the form of unit tests for Personalize ranking
Add user agent configuration to Personalize client to understand plugin adoption

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed as per the DCO using --signoff

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.

codecov · 2023-07-06T00:03:53Z

Codecov Report

Merging #151 (e0490ff) into main (174ae5f) will increase coverage by 0.02%.
The diff coverage is 92.75%.

@@             Coverage Diff              @@
##               main     #151      +/-   ##
============================================
+ Coverage     83.41%   83.44%   +0.02%     
- Complexity      324      331       +7     
============================================
  Files            41       42       +1     
  Lines          1242     1250       +8     
  Branches        152      153       +1     
============================================
+ Hits           1036     1043       +7     
  Misses          130      130              
- Partials         76       77       +1

Impacted Files	Coverage Δ
...nalizeintelligentranking/utils/ValidationUtil.java	`75.00% <75.00%> (ø)`
...ng/reranker/impl/AmazonPersonalizedRankerImpl.java	`96.10% <97.82%> (+2.84%)`	⬆️
...ntranking/PersonalizeRankingResponseProcessor.java	`100.00% <100.00%> (ø)`
...zeintelligentranking/client/PersonalizeClient.java	`65.00% <100.00%> (+6.17%)`	⬆️
...igentranking/client/PersonalizeClientSettings.java	`96.55% <100.00%> (ø)`

... and 2 files with indirect coverage changes

noCharger

Overall looks good, have some small comments

noCharger · 2023-07-06T21:13:53Z

...ce/transformer/personalizeintelligentranking/reranker/impl/AmazonPersonalizedRankerImpl.java

@@ -10,11 +10,13 @@
 import com.amazonaws.services.personalizeruntime.model.GetPersonalizedRankingRequest;
 import com.amazonaws.services.personalizeruntime.model.GetPersonalizedRankingResult;
 import com.amazonaws.services.personalizeruntime.model.PredictedItem;
+import org.apache.commons.lang3.StringUtils;


Using StringUtils requires adding the Apache Commons Lang library as a dependency. While this is not a significant issue, it does introduce an additional external dependency that needs to be managed and potentially increases the project's size. Also, calling utility methods for simple string operations (only isEmpty is used) can add some overhead compared to inline operations.

Sure. Will replace it with !=null and isEmpty() checks on the String object itself.

noCharger · 2023-07-06T21:15:20Z

...nsearch/search/relevance/transformer/personalizeintelligentranking/utils/ValidationUtil.java

+        }
+        // Validate Personalize recipe
+        if(!SUPPORTED_PERSONALIZE_RECIPES.contains(config.getRecipe())) {
+            throw ConfigurationUtils.newConfigurationException(processorType, processorTag, "recipe", "not supported recipe provided");


Please add test case on this exception.

Already have a test case supporting this: Refer here.

Check warning on line 48 in src/main/java/org/opensearch/search/relevance/transformer/personalizeintelligentranking/utils/ValidationUtil.java Codecov / codecov/patch src/main/java/org/opensearch/search/relevance/transformer/personalizeintelligentranking/utils/ValidationUtil.java#L48

noCharger · 2023-07-06T21:17:10Z

...nsearch/search/relevance/transformer/personalizeintelligentranking/utils/ValidationUtil.java

+    private static boolean isValidCampaignOrRoleArn(String arn, String expectedService) {
+        try {
+            Arn arnObj = Arn.fromString(arn);
+            String arnService = arnObj.getService();
+            if (!arnService.equals(expectedService)) {
+                return false;
+            }
+        } catch (IllegalArgumentException iae) {
+            return false;
+        }
+        return true;
+    }


Suggested change

private static boolean isValidCampaignOrRoleArn(String arn, String expectedService) {

try {

Arn arnObj = Arn.fromString(arn);

String arnService = arnObj.getService();

if (!arnService.equals(expectedService)) {

return false;

}

} catch (IllegalArgumentException iae) {

return false;

}

return true;

}

private static boolean isValidCampaignOrRoleArn(String arn, String expectedService) {

try {

Arn arnObj = Arn.fromString(arn);

return arnObj.getService().equals(expectedService);

} catch (IllegalArgumentException ignored) {

return false;

}

}

Thank you for suggestion. Will make this change.

Description: =========== This commit adds following changes: * Change scoring logic to consider ranks instead of scores * Add validation for Personalize inputs and configurations * Add E2E tests in the form of unit tests for Personalize ranking Signed-off-by: Ketan Kulkarni <[email protected]>

…tion Description: ============ This change adds following: * User agent configuration for Personalize client to understand plugin adoption * Add validation related unit tests Signed-off-by: Ketan Kulkarni <[email protected]>

Description: =========== This commit adds following changes: * Change scoring logic to consider ranks instead of scores * Add validation for Personalize inputs and configurations * Add E2E tests in the form of unit tests for Personalize ranking Signed-off-by: Ketan Kulkarni <[email protected]>

Description: =========== This commit includes following: * Change response processor name from personalize_ranking to personalized_search_ranking * Change client settings to use same naming convention * Address review comments Signed-off-by: Ketan Kulkarni <[email protected]>

Signed-off-by: Ketan Kulkarni <[email protected]>

mingshl

LGTM

noCharger

Overall looks good. Left some non-blocking comments. Can't wait to see it included in 2.9!

...relevance/transformer/personalizeintelligentranking/PersonalizeRankingResponseProcessor.java

noCharger · 2023-07-12T00:42:42Z

...nsearch/search/relevance/transformer/personalizeintelligentranking/utils/ValidationUtil.java

+        }
+        // Validate Personalize recipe
+        if(!SUPPORTED_PERSONALIZE_RECIPES.contains(config.getRecipe())) {
+            throw ConfigurationUtils.newConfigurationException(processorType, processorTag, "recipe", "not supported recipe provided");


Check warning on line 48 in src/main/java/org/opensearch/search/relevance/transformer/personalizeintelligentranking/utils/ValidationUtil.java Codecov / codecov/patch src/main/java/org/opensearch/search/relevance/transformer/personalizeintelligentranking/utils/ValidationUtil.java#L48

* Add validations for Personalize input and configurations Description: =========== This commit adds following changes: * Change scoring logic to consider ranks instead of scores * Add validation for Personalize inputs and configurations * Add E2E tests in the form of unit tests for Personalize ranking Signed-off-by: Ketan Kulkarni <[email protected]> * Add user agent prefix to Personalize client to understand plugin adoption Description: ============ This change adds following: * User agent configuration for Personalize client to understand plugin adoption * Add validation related unit tests Signed-off-by: Ketan Kulkarni <[email protected]> * Add validations for Personalize input and configurations Description: =========== This commit adds following changes: * Change scoring logic to consider ranks instead of scores * Add validation for Personalize inputs and configurations * Add E2E tests in the form of unit tests for Personalize ranking Signed-off-by: Ketan Kulkarni <[email protected]> * Change response processor config name Description: =========== This commit includes following: * Change response processor name from personalize_ranking to personalized_search_ranking * Change client settings to use same naming convention * Address review comments Signed-off-by: Ketan Kulkarni <[email protected]> * Fix typo in unit test Signed-off-by: Ketan Kulkarni <[email protected]> --------- Signed-off-by: Ketan Kulkarni <[email protected]> (cherry picked from commit e305c13)

* Add validations for Personalize input and configurations Description: =========== This commit adds following changes: * Change scoring logic to consider ranks instead of scores * Add validation for Personalize inputs and configurations * Add E2E tests in the form of unit tests for Personalize ranking Signed-off-by: Ketan Kulkarni <[email protected]> * Add user agent prefix to Personalize client to understand plugin adoption Description: ============ This change adds following: * User agent configuration for Personalize client to understand plugin adoption * Add validation related unit tests Signed-off-by: Ketan Kulkarni <[email protected]> * Add validations for Personalize input and configurations Description: =========== This commit adds following changes: * Change scoring logic to consider ranks instead of scores * Add validation for Personalize inputs and configurations * Add E2E tests in the form of unit tests for Personalize ranking Signed-off-by: Ketan Kulkarni <[email protected]> * Change response processor config name Description: =========== This commit includes following: * Change response processor name from personalize_ranking to personalized_search_ranking * Change client settings to use same naming convention * Address review comments Signed-off-by: Ketan Kulkarni <[email protected]> * Fix typo in unit test Signed-off-by: Ketan Kulkarni <[email protected]> --------- Signed-off-by: Ketan Kulkarni <[email protected]> (cherry picked from commit e305c13) Co-authored-by: kulket <[email protected]>

kulket marked this pull request as ready for review July 6, 2023 02:01

kulket requested review from msfroh, macohen, noCharger, mingshl and sejli as code owners July 6, 2023 02:01

mishprs approved these changes Jul 6, 2023

View reviewed changes

noCharger reviewed Jul 6, 2023

View reviewed changes

kulket added 5 commits July 11, 2023 17:00

Fix typo in unit test

e0490ff

Signed-off-by: Ketan Kulkarni <[email protected]>

kulket force-pushed the feat-validation-exception-handling branch from 9f3f660 to e0490ff Compare July 12, 2023 00:07

mingshl approved these changes Jul 12, 2023

View reviewed changes

noCharger reviewed Jul 12, 2023

View reviewed changes

mingshl merged commit e305c13 into opensearch-project:main Jul 12, 2023
16 checks passed

mingshl added the backport 2.x Backport to 2.x branch label Jul 12, 2023

opensearch-trigger-bot bot mentioned this pull request Jul 12, 2023

[Backport 2.x] Add validations for Personalize input and configurations #160

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validations for Personalize input and configurations #151

Add validations for Personalize input and configurations #151

kulket commented Jul 5, 2023 •

edited

Loading

codecov bot commented Jul 6, 2023 •

edited

Loading

noCharger left a comment

noCharger Jul 6, 2023

kulket Jul 8, 2023

noCharger Jul 6, 2023

kulket Jul 8, 2023

noCharger Jul 12, 2023

noCharger Jul 6, 2023

kulket Jul 8, 2023

mingshl left a comment

noCharger left a comment

noCharger Jul 12, 2023

Add validations for Personalize input and configurations #151

Add validations for Personalize input and configurations #151

Conversation

kulket commented Jul 5, 2023 • edited Loading

Description

Check List

codecov bot commented Jul 6, 2023 • edited Loading

Codecov Report

noCharger 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

mingshl left a comment

Choose a reason for hiding this comment

noCharger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kulket commented Jul 5, 2023 •

edited

Loading

codecov bot commented Jul 6, 2023 •

edited

Loading