-
Notifications
You must be signed in to change notification settings - Fork 24
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 validations for Personalize input and configurations #151
Add validations for Personalize input and configurations #151
Conversation
Codecov Report
@@ 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
|
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.
Overall looks good, have some small comments
@@ -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; |
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.
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.
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.
Sure. Will replace it with !=null
and isEmpty()
checks on the String
object itself.
} | ||
// Validate Personalize recipe | ||
if(!SUPPORTED_PERSONALIZE_RECIPES.contains(config.getRecipe())) { | ||
throw ConfigurationUtils.newConfigurationException(processorType, processorTag, "recipe", "not supported recipe provided"); |
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.
Please add test case on this exception.
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.
Already have a test case supporting this: Refer here.
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.
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
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; | ||
} |
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.
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; | |
} | |
} |
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.
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 <kektnr@amazon.com>
…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 <kektnr@amazon.com>
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 <kektnr@amazon.com>
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 <kektnr@amazon.com>
Signed-off-by: Ketan Kulkarni <kektnr@amazon.com>
9f3f660
to
e0490ff
Compare
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.
LGTM
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.
Overall looks good. Left some non-blocking comments. Can't wait to see it included in 2.9!
...relevance/transformer/personalizeintelligentranking/PersonalizeRankingResponseProcessor.java
Show resolved
Hide resolved
} | ||
// Validate Personalize recipe | ||
if(!SUPPORTED_PERSONALIZE_RECIPES.contains(config.getRecipe())) { | ||
throw ConfigurationUtils.newConfigurationException(processorType, processorTag, "recipe", "not supported recipe provided"); |
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.
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 <kektnr@amazon.com> * 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 <kektnr@amazon.com> * 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 <kektnr@amazon.com> * 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 <kektnr@amazon.com> * Fix typo in unit test Signed-off-by: Ketan Kulkarni <kektnr@amazon.com> --------- Signed-off-by: Ketan Kulkarni <kektnr@amazon.com> (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 <kektnr@amazon.com> * 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 <kektnr@amazon.com> * 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 <kektnr@amazon.com> * 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 <kektnr@amazon.com> * Fix typo in unit test Signed-off-by: Ketan Kulkarni <kektnr@amazon.com> --------- Signed-off-by: Ketan Kulkarni <kektnr@amazon.com> (cherry picked from commit e305c13) Co-authored-by: kulket <130191298+kulket@users.noreply.github.com>
Description
This pull request adds following changes:
Check List
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.