Have bandit logger separate out context #5

aarsilv · 2024-02-13T21:13:05Z

Eppo Internal:
🎟️ Ticket: FF-1426 - Adjust bandit logger to separate context by subject and action, and numeric and categorical attributes
🗺️ Design Review: Bandits Engineering Design (Relevant Section: 1/31 Data Sync)
🗨️ Slack Thread: ...are we ok with these property names?...

Description

To simplify using context to train the bandit, we want to have the SDKs separate out context by numeric (numbers) and categorical (everything else) attributes. We also want these separated by subject and action in the off chance two things are named the same. Therefore, BanditLogData has been adjusted to have four fields for use by the customer's logger:

Map<String, Double> subjectNumericAttributes,
Map<String, String> subjectCategoricalAttributes,
Map<String, Double> actionNumericAttributes,
Map<String, String> actionCategoricalAttributes

The intention is for the customer to then log these in four different columns in their bandit_assignments table.

Updated the tests in EppoClientTest to ensure things were sorted out correctly for both getStringAssignment() and logNonBanditAction().

aarsilv · 2024-02-13T21:13:42Z

src/main/java/com/eppo/sdk/EppoClient.java

@@ -45,7 +45,7 @@ private Optional<EppoValue> getAssignmentValue(
            String subjectKey,
            String flagKey,
            EppoAttributes subjectAttributes,
-            Map<String, EppoAttributes> assignmentOptions
+            Map<String, EppoAttributes> actionsWithAttributes


after coming back to it, I decided to rename this variable to be more bandit-centric and clear on what it is.

aarsilv · 2024-02-13T21:14:44Z

src/main/java/com/eppo/sdk/EppoClient.java

+        return eppoAttributes.entrySet().stream().filter(e -> !e.getValue().isNumeric() && !e.getValue().isNull()
+        ).collect(Collectors.toMap(
+          Map.Entry::getKey,
+          e -> e.getValue().toString())


toString() is used over stringValue() because we want the string representation of non-string-typed EppoValues (e.g., Booleans)

aarsilv · 2024-02-13T21:16:57Z

src/main/java/com/eppo/sdk/dto/IBanditLogger.java

@@ -4,5 +4,5 @@
 * Assignment Logger Interface
 */
 public interface IBanditLogger {
-    public void logBanditAction(BanditLogData logData);


public is implied for interfaces 🪓

giorgiomartini0

Looking great! Only significant issue is parallel to the one in the previous PR, on how to handle categorical attributes that could be numeric (but aren't).

giorgiomartini0 · 2024-02-13T23:29:23Z

src/main/java/com/eppo/sdk/EppoClient.java

+        if (eppoAttributes == null) {
+            return Map.of();
+        }
+        return eppoAttributes.entrySet().stream().filter(e -> !e.getValue().isNumeric() && !e.getValue().isNull()


Same considerations as in the previous PR. If my understanding is correct, we should remove the !e.getValue().isNumeric() condition.

I think we need to know if the value was initialized with a truly numeric value and use that.

giorgiomartini0 · 2024-02-13T23:31:59Z

src/main/java/com/eppo/sdk/dto/BanditLogData.java

+    String variation,
+    String subject,
+    String action,
+    Float actionProbability,


Maybe this should be Double, since computeActionWeights returns probabilities/weights as doubles.

Oh yes great catch!

giorgiomartini0 · 2024-02-13T23:33:09Z

src/test/java/com/eppo/sdk/EppoClientTest.java

    assertEquals(0.3333, capturedBanditLog.actionProbability, 0.0002);
    assertEquals("cold start", capturedBanditLog.modelVersion);
  }

  @Test
  public void testBanditModelActionLogging() {
+    // Note: some of the passed in attributes are not used for scoring, but we do still want to make sure they are logged


Good test! We definitely want to log "new" attributes, so that they can be used for the next model retrain.

giorgiomartini0 · 2024-02-13T23:36:33Z

src/test/java/com/eppo/sdk/EppoClientTest.java

-      "days_since_signup", EppoValue.valueOf(130) // note: unused for scoring (which looks for account_age)
+    "gender_identity", EppoValue.valueOf("female"),
+    "days_since_signup", EppoValue.valueOf(130), // unused for scoring (which looks for account_age)
+    "is_premium", EppoValue.valueOf(false), // unused for scoring


Once we settle the question of categorical value that could cast to numeric (but are not), we should add a test to crystalize that behavior. Could be "account_tier", EppoValue.valueOf("2"), for example.

sameerank · 2024-02-13T23:56:24Z

src/test/java/com/eppo/sdk/EppoClientTest.java

+    assertEquals(Map.of("account_age", 90.0), capturedBanditLog.subjectNumericAttributes);
+    assertEquals(Map.of("loyalty_tier", "gold", "is_account_admin", "false"), capturedBanditLog.subjectCategoricalAttributes);
+    assertEquals(Map.of("num_past_purchases", 0.0), capturedBanditLog.actionNumericAttributes);
+    assertEquals(Map.of("brand", "skechers", "has_promo_code", "true"), capturedBanditLog.actionCategoricalAttributes);


sameerank · 2024-02-13T23:59:42Z

src/test/java/com/eppo/sdk/EppoClientTest.java

@@ -441,11 +462,17 @@ public void testBanditControlAction() {

    Set<String> banditActions = Set.of("option1", "option2", "option3");

+    EppoAttributes subjectAttributes = new EppoAttributes(Map.of(
+      "account_age", EppoValue.valueOf(90),


Not super familiar with Java; TIL integer types are converted to double automatically

sameerank · 2024-02-14T00:07:43Z

src/main/java/com/eppo/sdk/EppoClient.java

              null,
-              null
+              null,


public String action; public Float actionProbability; --> null public String modelVersion; --> null

What's an example of a "NonBandit" action string?

Any action not selected by the bandit, such as something selected by a recommendation algorithm.

…exp#32) * [EN-17858] FetchConfigurationsTask is more fault tolerant * revert change to pom.xml, thereby having a single, clean commit that can be merged back to the Eppo main repo

* add graceful mode by default that does not throw exceptions (FF-949) (Eppo-exp#29) * add graceful mode by default that does not throw exceptions (FF-949) * test graceful mode on and off * test all functions * remove from javadoc * Bump com.github.tomakehurst:wiremock-jre8 from 2.33.2 to 2.35.1 (Eppo-exp#28) Bumps [com.github.tomakehurst:wiremock-jre8](https://github.com/wiremock/wiremock) from 2.33.2 to 2.35.1. - [Release notes](https://github.com/wiremock/wiremock/releases) - [Commits](wiremock/wiremock@2.33.2...2.35.1) --- updated-dependencies: - dependency-name: com.github.tomakehurst:wiremock-jre8 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * add compatibility for java 8 (Eppo-exp#35) * java8 compatibility using apache httpclient * cleanup * added timeouts * add maven-gpg-plugin * test across java 8, 11, 17 * pom * remove List.of * fix test * eppo value * fix plugin * profile --------- Co-authored-by: Gaurav Arora <gauravarora@Gauravs-MacBook-Pro.local> * Bump org.apache.httpcomponents:httpclient from 4.5.10 to 4.5.13 (Eppo-exp#36) Bumps org.apache.httpcomponents:httpclient from 4.5.10 to 4.5.13. --- updated-dependencies: - dependency-name: org.apache.httpcomponents:httpclient dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * [EN-17858] FetchConfigurationsTask is more fault tolerant (#5) (Eppo-exp#32) * [EN-17858] FetchConfigurationsTask is more fault tolerant * revert change to pom.xml, thereby having a single, clean commit that can be merged back to the Eppo main repo * bump to version 2.2.0 (Eppo-exp#38) * add support for semver rule evaluation (FF-1569) (Eppo-exp#39) * add support for semver rule evaluation (FF-1569) * azul builds * adjust unit tests for semver comparison * numeric first * add back in isXXX checks * helper function for JSON string serialization * rename variation to banditKey for bandit logging * remove accidentally added junit --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Leo Romanovsky <leoromanovsky@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Gaurav Arora <gauravarora@Gauravs-MacBook-Pro.local> Co-authored-by: Simon Dale <33907262+simondale00@users.noreply.github.com>

aarsilv added 16 commits February 6, 2024 16:14

work in progress

10a599d

math in place

60b7250

working on tests still

5598d54

tests passing

79bb76c

new test cases

70c3a74

wip manually verifying math

ce56f20

tests passing

1ea315b

remove logging

47dd568

clean up score computation

2571b7c

changes from self-reivew of PR

3361228

consolidate test bandit rac setup code

e0d4a08

clean up test file

c8ff978

update comments

be2d0e4

more comment improvements

289af2b

failing tests in place

cb75b14

break out attributes by type

a133c0e

aarsilv commented Feb 13, 2024

View reviewed changes

changes from self-review of PR

c509e59

aarsilv marked this pull request as ready for review February 13, 2024 21:19

aarsilv requested review from giorgiomartini0, sameerank, leoromanovsky and agamkrbit February 13, 2024 21:24

aarsilv assigned giorgiomartini0 Feb 13, 2024

giorgiomartini0 approved these changes Feb 13, 2024

View reviewed changes

aarsilv assigned aarsilv and unassigned giorgiomartini0 Feb 13, 2024

sameerank approved these changes Feb 14, 2024

View reviewed changes

aarsilv added 2 commits February 15, 2024 15:06

return type as provided

7b94140

make EppoValue constructor private

ff49a3e

Base automatically changed from aaron/ff-1535/use-bandit-parameters to main February 15, 2024 22:11

aarsilv added 2 commits February 15, 2024 15:16

feedback from PR

aa269e7

Merge branch 'main' into aaron/ff-1546/separate-logger-context

de7cf58

aarsilv merged commit 7fcf0a5 into main Feb 15, 2024
1 check passed

aarsilv deleted the aaron/ff-1546/separate-logger-context branch February 15, 2024 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have bandit logger separate out context #5

Have bandit logger separate out context #5

aarsilv commented Feb 13, 2024 •

edited

Loading

aarsilv Feb 13, 2024

aarsilv Feb 13, 2024

aarsilv Feb 13, 2024

giorgiomartini0 left a comment

giorgiomartini0 Feb 13, 2024

aarsilv Feb 13, 2024

giorgiomartini0 Feb 13, 2024

aarsilv Feb 13, 2024

giorgiomartini0 Feb 13, 2024

giorgiomartini0 Feb 13, 2024

sameerank Feb 13, 2024

sameerank Feb 13, 2024

sameerank Feb 14, 2024

aarsilv Feb 15, 2024

Have bandit logger separate out context #5

Have bandit logger separate out context #5

Conversation

aarsilv commented Feb 13, 2024 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giorgiomartini0 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

aarsilv commented Feb 13, 2024 •

edited

Loading