Add bandit assignment cache #120

aarsilv · 2024-08-26T23:57:58Z

Eppo Internal:
🎟️ Ticket: FF-3112 - JS Common SDK - Assignment cache should work at the bandit action level
🗨️ Slack: "...are subsequent calls of the SDK assignment function still sending those events to the logger..."

Motivation and Context

Customers may optionally want to exclude repeat bandit assignments from executing the logger callback to reduce the number of entries sent to their data warehouse.

Description

This PR mirrors the flag assignment cache functionality for bandit actions. The difference is that the unique and cached value is a function of the bandit key and action key instead of the allocation key and variation key. The use of the flag key and subject key remains the same.

Bandit action caching can be independently toggled from flag assignment caching. This could be useful when one wants action assignment caching but not bandit action caching, such as when using bandits to display ads and knowing when every impression was useful.

Caching happens at the subject and flag key levels for both--i.e. if a flag results in one action being assigned and then another, the first assignment is evicted from the cache to record the second. This allows for treating changed assignments as new, even if previously assigned.

How has this been tested?

Newly added Assignment logging deduplication test case to eppo-client-with-bandits.spec.ts

aarsilv · 2024-08-27T00:01:39Z

src/client/eppo-client-with-bandits.spec.ts

+      let actionToReturn: string | null;
+
+      // Convenience method for repeatedly making the exact same assignment call
+      function requestClientBanditAction(): Omit<IAssignmentDetails<string>, 'evaluationDetails'> {


I find function more descriptive than const, and since I don't need lexical this nor does this being hoisted matter, I'm thus opting to go for it.

aarsilv · 2024-08-27T00:02:18Z

src/client/eppo-client-with-bandits.spec.ts

+              subjectKey,
+              subjectAttributes,
+              allocationKey: 'mock-allocation',
+              variation: { key: variationToReturn, value: variationToReturn },


Where variationToReturn is injected as the assigned variation for mocking Evaluator.evaluateFlag()

aarsilv · 2024-08-27T00:02:38Z

src/client/eppo-client-with-bandits.spec.ts

+              flagKey,
+              subjectKey,
+              subjectAttributes: { numericAttributes: {}, categoricalAttributes: {} },
+              actionKey: actionToReturn,


Where actionToReturn is injected as the assigned action for mocking BanditEvaluator.evaluateBandit()

rasendubi

A couple of nitpicks but overall looks good!

rasendubi · 2024-08-27T14:13:10Z

src/client/eppo-client.ts

+    // What our bandit assignment cache cares about for avoiding logging duplicate bandit assignments,
+    // if one is active. Like the flag assignment cache, entries are only stored for a given flag
+    // and subject. However, Bandit and action keys are also used for determining assignment uniqueness.
+    // This means that if a flag's bandit assigns one action, and then later a new action, the first
+    // one will be evicted from the cache. If later assigned again, it will be treated as new.


nit: this comment seems misplaced. The best place for it is calculation of cache key and value (as these are controlling this behavior). If cache key or value change, this comment could easily become outdated.

Yeah I was just trying to explain what banditAssignmentCacheProperties is and how it will be used, but agree the comment could get stale. I'll experiment with better spatially colocating it.

I would move cache entry construction inside the caching module. Otherwise, it looks like a leaking implementation detail — the caller needs to know what keys are used in caching and copy them into banditAssignmentCacheProperties. It would be simpler if the cache was accepting the event

I found a good home for this in abstract-assignment-cache.ts after all! 🎉

rasendubi · 2024-08-27T14:14:53Z

src/client/eppo-client.ts

+      // Ignore repeat assignment
+      return;
+    }
+
    if (!this.banditLogger) {
      // No bandit logger set; enqueue the event in case a logger is later set
      if (this.queuedBanditEvents.length < MAX_EVENT_QUEUE_SIZE) {
        this.queuedBanditEvents.push(banditEvent);


Q: shall we deduplicate events in the queuedBanditEvents?

I don't think so as these are basically all sent to a logger if one is added, so we want deduping to happen beforehand like we're doing

We're not deduplicating them beforehand here because return on the next line prevents us from setting the entry in the cache.

If we log the same event twice before the user sets the logger, the event will be sent to user's logger twice

Ah I see what you are saying now, good catch! Will address

rasendubi · 2024-08-27T14:22:51Z

src/cache/abstract-assignment-cache.ts

+export function assignmentCacheValueToString(cacheValue: AssignmentCacheValue): string {
+  const fieldsToHash: string[] = [];
+
+  if ('allocationKey' in cacheValue && 'variationKey' in cacheValue) {
+    fieldsToHash.push(cacheValue.allocationKey, cacheValue.variationKey);
+  }
+
+  if ('banditKey' in cacheValue && 'actionKey' in cacheValue) {
+    fieldsToHash.push(cacheValue.banditKey, cacheValue.actionKey);
+  }
+
+  return getMD5Hash(fieldsToHash.join(';'));


nit: this code looks like it's trying to cram two functions into one which makes it convoluted to follow (e.g., is it possible that both ifs match? none?)

If we add new event types (or decide to add allocation/variation key to bandit action), this is going to get messy.

A cleaner solution would be to make cache type generic over what it caches and/or accept key/value mapping functions as parameters.

+1 to this comment

I don't think we could do generic because we only want to hash a specific subset of the provided properties. The code is indeed just handling two specific possible code paths

Update: since the other fields are part of the cache key and we know they will be unique, we can just simply hash the entire value, making this generic 💪

aarsilv · 2024-08-27T21:15:50Z

src/cache/abstract-assignment-cache.ts

+/**
+ * Assignment cache keys are only on the subject and flag level, while the entire value is used
+ * for uniqueness checking. This way that if an assigned variation or bandit action changes for a
+ * flag, it evicts the old one. Then, if an older assignment is later reassigned, it will be treated
+ * as new.
+ */


@rasendubi moved comment here

aarsilv · 2024-08-27T21:15:59Z

src/cache/abstract-assignment-cache.ts

 export type AssignmentCacheKey = {
  subjectKey: string;
  flagKey: string;
 };

+export type AssignmentCacheValue = Record<string, string>;


@rasendubi & @felipecsl cache is now generic

We will lose type-safety by using generic Record. Would using generics be possible?

export type CacheKeyPair<T extends string, U extends string> = { [K in T]: string; } & { [K in U]: string; }; export type AssignmentCacheValue = CacheKeyPair<'allocationKey', 'variationKey'>; export type BanditCacheValue = CacheKeyPair<'banditKey', 'actionKey'>; // Usage example const assignmentCache: AssignmentCacheValue = { allocationKey: 'someAllocationKey', variationKey: 'someVariationKey' }; const banditCache: BanditCacheValue = { banditKey: 'someBanditKey', actionKey: 'someActionKey' };

Oh this is an interesting thought! I think this would be possible, but I don't know if we need to be not generic. If they send in something other than bandit key and action key, we'd still hash it and use that to define uniqueness for the entry, which I think is ok. Keeps it simpler and makes it more extensible if we need to cache other stuff going forward.

Paired on this and got 80% the way there; will merge then possibly go for the other 20% later.

aarsilv · 2024-08-27T21:16:18Z

src/client/eppo-client-with-bandits.spec.ts

@@ -220,6 +223,20 @@ describe('EppoClient Bandits E2E test', () => {
      expect(mockLogAssignment).not.toHaveBeenCalled();
      expect(mockLogBanditAction).not.toHaveBeenCalled();

+      const repeatAssignment = client.getBanditAction(


@rasendubi adjusted test that flushes events to also ensure they are deduped as well (verified later with the .toHaveBeenCalledTimes(1) check)

aarsilv · 2024-08-27T21:18:11Z

src/client/eppo-client.ts

+        this.queuedBanditEvents.push(banditEvent);
+      }
+      // Record in the assignment cache, if active, to deduplicate subsequent repeat assignments
+      this.banditAssignmentCache?.set(banditAssignmentCacheProperties);


@rasendubi no early return before setting the cache entry--instead if-else if is used to handle the flow control for calling banditLogger

package.json

leoromanovsky · 2024-08-27T22:58:16Z

src/cache/abstract-assignment-cache.ts

 export type AssignmentCacheKey = {
  subjectKey: string;
  flagKey: string;
 };

+export type AssignmentCacheValue = Record<string, string>;


We will lose type-safety by using generic Record. Would using generics be possible?

export type CacheKeyPair<T extends string, U extends string> = { [K in T]: string; } & { [K in U]: string; }; export type AssignmentCacheValue = CacheKeyPair<'allocationKey', 'variationKey'>; export type BanditCacheValue = CacheKeyPair<'banditKey', 'actionKey'>; // Usage example const assignmentCache: AssignmentCacheValue = { allocationKey: 'someAllocationKey', variationKey: 'someVariationKey' }; const banditCache: BanditCacheValue = { banditKey: 'someBanditKey', actionKey: 'someActionKey' };

Co-authored-by: Leo Romanovsky <[email protected]>

aarsilv added 2 commits August 23, 2024 16:32

test in place

6b8e782

use an assignment cache to deduplicate bandit action assignments

f263b50

aarsilv commented Aug 27, 2024

View reviewed changes

aarsilv added 2 commits August 26, 2024 18:10

feedback from self-review of PR

d4371b9

fix type and lint errors

651cb66

aarsilv marked this pull request as ready for review August 27, 2024 00:20

aarsilv requested review from typotter, rasendubi and leoromanovsky August 27, 2024 00:20

aarsilv assigned rasendubi Aug 27, 2024

rasendubi approved these changes Aug 27, 2024

View reviewed changes

bump minor version

899967f

aarsilv assigned aarsilv and unassigned rasendubi Aug 27, 2024

aarsilv added 3 commits August 27, 2024 14:53

just hash whole cache value

74bcc0f

feedback from PR

8bc8abc

dedupe when queuing assignments as well

78b2932

aarsilv commented Aug 27, 2024

View reviewed changes

leoromanovsky reviewed Aug 27, 2024

View reviewed changes

Bump package.json version correctly

b0c347d

Co-authored-by: Leo Romanovsky <[email protected]>

leoromanovsky approved these changes Aug 29, 2024

View reviewed changes

feedback from PR

7703143

aarsilv merged commit d7945ff into main Aug 29, 2024
2 checks passed

aarsilv deleted the aaron/ff-3112/bandit-assignment-cache branch August 29, 2024 18:51

aarsilv mentioned this pull request Aug 29, 2024

Bump version of common js client to take advantage of bandit assignment caching Eppo-exp/node-server-sdk#70

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bandit assignment cache #120

Add bandit assignment cache #120

aarsilv commented Aug 26, 2024

aarsilv Aug 27, 2024

aarsilv Aug 27, 2024

aarsilv Aug 27, 2024

rasendubi left a comment

rasendubi Aug 27, 2024

aarsilv Aug 27, 2024

rasendubi Aug 27, 2024

aarsilv Aug 27, 2024

rasendubi Aug 27, 2024

aarsilv Aug 27, 2024

rasendubi Aug 27, 2024

aarsilv Aug 27, 2024

rasendubi Aug 27, 2024

felipecsl Aug 27, 2024

aarsilv Aug 27, 2024

aarsilv Aug 27, 2024

aarsilv Aug 27, 2024

aarsilv Aug 27, 2024

leoromanovsky Aug 27, 2024

aarsilv Aug 29, 2024

aarsilv Aug 29, 2024

aarsilv Aug 27, 2024

aarsilv Aug 27, 2024

leoromanovsky Aug 27, 2024

Add bandit assignment cache #120

Add bandit assignment cache #120

Conversation

aarsilv commented Aug 26, 2024

Motivation and Context

Description

How has this been tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rasendubi 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

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