Personalized intelligent ranking for open search requests #138

kulket · 2023-05-18T18:12:14Z

Description:

This change uses search pipeline feature of open search to allow customers to create a response processor.
Response processor will rerank search results obtained from open search using Personalization solution before returning results to the user.

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

Signed-off-by: Ketan Kulkarni kektnr@amazon.com

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.

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

...ch/relevance/transformer/personalizeintelligentranking/client/PersonalizeClientSettings.java

msfroh · 2023-05-23T16:23:35Z

@kulket -- Can you please rebase from main and force-push to your fork?

If you check the list of commits associated with this PR, it includes the Jackson-core update, which isn't actually part of this PR.

msfroh · 2023-05-22T22:16:05Z

.../transformer/personalizeintelligentranking/client/PersonalizeCredentialsProviderFactory.java

+     * @param clientSettings Personalize client settings
+     * @return AWS credentials provider for accessing Personalize
+     */
+    public AWSCredentialsProvider getCredentialsProvider(PersonalizeClientSettings clientSettings) {


Does this one get called from anywhere else? Can it be private + static?

.../transformer/personalizeintelligentranking/client/PersonalizeCredentialsProviderFactory.java

codecov · 2023-05-23T16:27:29Z

Codecov Report

Merging #138 (e695728) into main (2e78139) will increase coverage by 0.99%.
The diff coverage is 84.27%.

@@             Coverage Diff              @@
##               main     #138      +/-   ##
============================================
+ Coverage     81.05%   82.05%   +0.99%     
- Complexity      218      293      +75     
============================================
  Files            29       41      +12     
  Lines           887     1131     +244     
  Branches        120      139      +19     
============================================
+ Hits            719      928     +209     
- Misses          106      127      +21     
- Partials         62       76      +14

Impacted Files	Coverage Δ
...search/search/relevance/SearchRelevancePlugin.java	`0.00% <0.00%> (ø)`
...izeintelligentranking/configuration/Constants.java	`0.00% <0.00%> (ø)`
...intelligentranking/requestparameter/Constants.java	`0.00% <0.00%> (ø)`
...zeintelligentranking/client/PersonalizeClient.java	`58.82% <58.82%> (ø)`
...uestparameter/PersonalizeRequestParameterUtil.java	`75.00% <75.00%> (ø)`
...ameter/PersonalizeRequestParametersExtBuilder.java	`78.26% <78.26%> (ø)`
...ng/reranker/impl/AmazonPersonalizedRankerImpl.java	`82.05% <82.05%> (ø)`
...ion/PersonalizeIntelligentRankerConfiguration.java	`85.71% <85.71%> (ø)`
...requestparameter/PersonalizeRequestParameters.java	`88.00% <88.00%> (ø)`
.../client/PersonalizeCredentialsProviderFactory.java	`95.83% <95.83%> (ø)`
... and 3 more

Description: ============ This change uses search pipeline feature of open search to allow customers to create a response processor. Response processor will rerank search results obtained from open search using Personalization solution before returning results to the user. Check List: ========== - [Y] New functionality includes testing. - [Y] All tests pass - [Y] New functionality has been documented. - [Y] New functionality has javadoc added - [Y] Commits are signed as per the DCO using --signoff Signed-off-by: Ketan Kulkarni <kektnr@amazon.com>

Description: ============ This change introduces ability ot pass user id as part of search request to be used by Personalize response processor. SearchExtSpec will be used to support getting user id as well as other personalize related request parameters to personalize search results. Check list: =========== - [x] New functionality includes testing. - [x] All tests pass - [x] New functionality has been documented. - [x] New functionality has javadoc added - [x] Commits are signed as per the DCO using --signoff Signed-off-by: Ketan Kulkarni <kektnr@amazon.com>

Description: ============ SearchPipelinePlugin interface now uses Processor.Factory<SearchResponseProcessor> instead of Processor.Factory. Also use getResponseProcessors method instead of getProcessors method. Check list: =========== - [x] New functionality includes testing. - [x] All tests pass - [x] New functionality has been documented. - [x] New functionality has javadoc added - [x] Commits are signed as per the DCO using --signoff Signed-off-by: Ketan Kulkarni <kektnr@amazon.com>

kulket · 2023-05-23T18:45:34Z

@kulket -- Can you please rebase from main and force-push to your fork?

If you check the list of commits associated with this PR, it includes the Jackson-core update, which isn't actually part of this PR.

Performed rebased as well as force push.
I am seeing 3 commits now.

Added unit tests for PersonalizeRankingResponseProcessor and PersonalizeRequestParameterUtil. Also, needed to update JaCoCo to 0.8.9 in order to get rid of warnings about invalid bytecode versions since we upgraded to JDK 20. Signed-off-by: Michael Froh <froh@amazon.com>

mingshl · 2023-05-24T00:01:45Z

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

+import java.util.concurrent.TimeUnit;
+import java.util.function.BiFunction;
+
+import static org.opensearch.search.relevance.transformer.personalizeintelligentranking.configuration.Constants.CAMPAIGN_ARN_CONFIG_NAME;


it seems that the class is importing individual configuration constants from the configuration package. While this approach can work, I believe it might be more maintainable and flexible to incorporate the configuration as an attribute of the plugins and access it using this.personalizeClientsetting. This way, the class can receive the configuration as a parameter, allowing for greater modularity and easier testing.

Check out this PR, which use this.kendraClientsetting

search-processor/src/main/java/org/opensearch/search/relevance/SearchRelevancePlugin.java

Line 95 in 01ca204

this.kendraClientSettings = KendraClientSettings.getClientSettings(environment.settings());

search-processor/src/main/java/org/opensearch/search/relevance/SearchRelevancePlugin.java

Line 119 in 01ca204

return Map.of(KendraRankingResponseProcessor.TYPE, new KendraRankingResponseProcessor.Factory(this.kendraClientSettings));

Hi mingshl,
Thank you for your review and comment.

I kept code for getting PersonalizeClientSettings instance to limited to the PersonalizeRankingResponseProcessor.Factory instance creation so that PersonalizeClientSettings are not created when they are not required. For e.g. Creating that instance outside factory creation may create PersonalizeClientSettings instance when using plugin for Kendra only use case.

I did thought about creating one common class for Kendra and Personalize related settings. But Kendra needs more configurations than just credentials and hence opted to keep separate class for Personalize.

Let me know your thoughts regarding above.

I guess another question is whether it makes sense to declare the Factory parameters in a separate Constants class just to use them here. They're only used to pass values to the processor Factory. As such, they can be kept in this class.

A "global" Constants class is only really useful for values used in multiple classes. Otherwise, it's just an extra hop to find values that are only used in one class.

Makes sense. Updated code to move search pipeline related configuration constants to Factory class instead of using earlier approach of global constants.

noCharger · 2023-05-24T15:45:02Z

...rch/search/relevance/transformer/personalizeintelligentranking/client/PersonalizeClient.java

+            result = AccessController.doPrivileged(
+                    (PrivilegedAction<GetPersonalizedRankingResult>) () -> personalizeRuntime.getPersonalizedRanking(request));
+        } catch (AmazonServiceException ex) {
+            throw ex;


Should this be a direct rethrow or an OpenSearchException?

Will be using follow up PR to update exception handling mentioned here.

noCharger · 2023-05-24T16:46:12Z

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

+            // TODO: Parse context from request parameters
+            String userId = requestParameters.getUserId();
+            logger.info("User ID from request parameters. User ID: {}", userId);
+            GetPersonalizedRankingRequest personalizeRequest = new GetPersonalizedRankingRequest()
+                    .withCampaignArn(rankerConfig.getPersonalizeCampaign())
+                    .withInputList(documentIdsToRank)
+                    .withUserId(userId);
+            GetPersonalizedRankingResult result = personalizeClient.getPersonalizedRanking(personalizeRequest);
+
+            //TODO: Combine Personalize and open search result. Change the result after transform logic is implemented


Seems to unfinished impl. If it's impl else where, please remove the TODOs

We will update unfinished implementation as a follow up PR.

noCharger · 2023-05-24T16:51:43Z

.../relevance/transformer/personalizeintelligentranking/reranker/PersonalizedRankerFactory.java

+/**
+ * Factory for creating Personalize ranker instance based on Personalize ranker configuration
+ */
+public class PersonalizedRankerFactory {


If there is only one impl expected, can we simplify the code structure without utilizing a factory?

We expect more than one implementation going forward so use factory approach here.

Description: ============ Configuration constants are only used by search response processor factory. Move related constants from global configuration constants file to factory class to ensure they are closer to the palce where they are used. Signed-off-by: Ketan Kulkarni <kektnr@amazon.com>

mingshl

LGTM

* Personalized intelligent ranking for open search requests Description: ============ This change uses search pipeline feature of open search to allow customers to create a response processor. Response processor will rerank search results obtained from open search using Personalization solution before returning results to the user. Check List: ========== - [Y] New functionality includes testing. - [Y] All tests pass - [Y] New functionality has been documented. - [Y] New functionality has javadoc added - [Y] Commits are signed as per the DCO using --signoff Signed-off-by: Ketan Kulkarni <kektnr@amazon.com> * Support passing user id for Personalization as part of search request Description: ============ This change introduces ability ot pass user id as part of search request to be used by Personalize response processor. SearchExtSpec will be used to support getting user id as well as other personalize related request parameters to personalize search results. Check list: =========== - [x] New functionality includes testing. - [x] All tests pass - [x] New functionality has been documented. - [x] New functionality has javadoc added - [x] Commits are signed as per the DCO using --signoff Signed-off-by: Ketan Kulkarni <kektnr@amazon.com> * Incorporate changes to SearchPipelinePlugin interface Description: ============ SearchPipelinePlugin interface now uses Processor.Factory<SearchResponseProcessor> instead of Processor.Factory. Also use getResponseProcessors method instead of getProcessors method. Check list: =========== - [x] New functionality includes testing. - [x] All tests pass - [x] New functionality has been documented. - [x] New functionality has javadoc added - [x] Commits are signed as per the DCO using --signoff Signed-off-by: Ketan Kulkarni <kektnr@amazon.com> * Improve test coverage Added unit tests for PersonalizeRankingResponseProcessor and PersonalizeRequestParameterUtil. Also, needed to update JaCoCo to 0.8.9 in order to get rid of warnings about invalid bytecode versions since we upgraded to JDK 20. Signed-off-by: Michael Froh <froh@amazon.com> * Move configruation constants to the search pipeline processor factory Description: ============ Configuration constants are only used by search response processor factory. Move related constants from global configuration constants file to factory class to ensure they are closer to the palce where they are used. Signed-off-by: Ketan Kulkarni <kektnr@amazon.com> --------- Signed-off-by: Ketan Kulkarni <kektnr@amazon.com> Signed-off-by: Michael Froh <froh@amazon.com> Co-authored-by: Michael Froh <froh@amazon.com> (cherry picked from commit 23b5226)

* Personalized intelligent ranking for open search requests Description: ============ This change uses search pipeline feature of open search to allow customers to create a response processor. Response processor will rerank search results obtained from open search using Personalization solution before returning results to the user. Signed-off-by: Ketan Kulkarni <kektnr@amazon.com> * Support passing user id for Personalization as part of search request Description: ============ This change introduces ability ot pass user id as part of search request to be used by Personalize response processor. SearchExtSpec will be used to support getting user id as well as other personalize related request parameters to personalize search results. Signed-off-by: Ketan Kulkarni <kektnr@amazon.com> * Incorporate changes to SearchPipelinePlugin interface Description: ============ SearchPipelinePlugin interface now uses Processor.Factory<SearchResponseProcessor> instead of Processor.Factory. Also use getResponseProcessors method instead of getProcessors method. Signed-off-by: Ketan Kulkarni <kektnr@amazon.com> * Improve test coverage Added unit tests for PersonalizeRankingResponseProcessor and PersonalizeRequestParameterUtil. Also, needed to update JaCoCo to 0.8.9 in order to get rid of warnings about invalid bytecode versions since we upgraded to JDK 20. Signed-off-by: Michael Froh <froh@amazon.com> * Move configruation constants to the search pipeline processor factory Description: ============ Configuration constants are only used by search response processor factory. Move related constants from global configuration constants file to factory class to ensure they are closer to the palce where they are used. Signed-off-by: Ketan Kulkarni <kektnr@amazon.com> --------- Signed-off-by: Ketan Kulkarni <kektnr@amazon.com> Signed-off-by: Michael Froh <froh@amazon.com> Co-authored-by: Michael Froh <froh@amazon.com> (cherry picked from commit 23b5226) Co-authored-by: kulket <130191298+kulket@users.noreply.github.com>

kulket force-pushed the feat-intelligent-ranking branch from 64bd77b to 4d8cde4 Compare May 18, 2023 19:46

msfroh reviewed May 19, 2023

View reviewed changes

...relevance/transformer/personalizeintelligentranking/PersonalizeRankingResponseProcessor.java Outdated Show resolved Hide resolved

msfroh reviewed May 19, 2023

View reviewed changes

...ch/relevance/transformer/personalizeintelligentranking/client/PersonalizeClientSettings.java Show resolved Hide resolved

msfroh reviewed May 23, 2023

View reviewed changes

msfroh marked this pull request as ready for review May 23, 2023 18:04

msfroh requested review from macohen, noCharger, mingshl and sejli as code owners May 23, 2023 18:04

kulket added 3 commits May 23, 2023 11:29

kulket force-pushed the feat-intelligent-ranking branch from f789ebd to 0472a12 Compare May 23, 2023 18:42

mingshl reviewed May 24, 2023

View reviewed changes

noCharger reviewed May 24, 2023

View reviewed changes

msfroh added the backport 2.x Backport to 2.x branch label May 24, 2023

mingshl approved these changes May 24, 2023

View reviewed changes

msfroh merged commit 23b5226 into opensearch-project:main May 24, 2023

opensearch-trigger-bot bot mentioned this pull request May 24, 2023

[Backport 2.x] Personalized intelligent ranking for open search requests #142

Merged

mingshl added the feature introduce a net new unit of functionality of a software system that satisfies a requirement label Jul 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Personalized intelligent ranking for open search requests #138

Personalized intelligent ranking for open search requests #138

kulket commented May 18, 2023 •

edited by msfroh

Loading

msfroh commented May 23, 2023

msfroh May 22, 2023

codecov bot commented May 23, 2023 •

edited

Loading

kulket commented May 23, 2023

mingshl May 24, 2023

kulket May 24, 2023

msfroh May 24, 2023

kulket May 24, 2023

noCharger May 24, 2023

kulket May 24, 2023

noCharger May 24, 2023 •

edited

Loading

kulket May 24, 2023

noCharger May 24, 2023

kulket May 24, 2023

mingshl left a comment

Personalized intelligent ranking for open search requests #138

Personalized intelligent ranking for open search requests #138

Conversation

kulket commented May 18, 2023 • edited by msfroh Loading

Description:

Check List:

msfroh commented May 23, 2023

Choose a reason for hiding this comment

codecov bot commented May 23, 2023 • edited Loading

Codecov Report

kulket commented May 23, 2023

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

noCharger May 24, 2023 • edited Loading

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

kulket commented May 18, 2023 •

edited by msfroh

Loading

codecov bot commented May 23, 2023 •

edited

Loading

noCharger May 24, 2023 •

edited

Loading