Skip to content
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 support for contextual bandits #64

Merged
merged 6 commits into from
Jul 8, 2024
Merged

Conversation

aarsilv
Copy link
Contributor

@aarsilv aarsilv commented Jul 2, 2024

Eppo Internal:
🎟️ Ticket: FF-2021 - Add bandits to Node SDK
🗺️ Design Review: Bandits Engineering Design

Motivation and Context

This change brings contextual multi-armed bandit support to our Node SDK, as described in the high-level documentation.

Description

Updates the Node JS SDK to use the updated shared common JavaScript client which has bandit support. This includes its new public function getBanditAction().

How has this been tested?

Added top-level tests which exercise getBanditActin() using the shared common test data.

image

@@ -29,7 +29,7 @@
},
"homepage": "https://github.com/Eppo-exp/node-server-sdk#readme",
"dependencies": {
"@eppo/js-client-sdk-common": "3.0.6",
"@eppo/js-client-sdk-common": "3.5.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📈

import * as td from 'testdouble';

import apiServer, { TEST_SERVER_PORT } from '../test/mockApiServer';
import apiServer, { TEST_BANDIT_API_KEY, TEST_SERVER_PORT } from '../test/mockApiServer';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test actually spins up a server vs mocking fetch responses. Since different tests need different UFC files served, I'm switching off the provided SDK key.

Any client initialized with this key will be served the UFC file with bandit flags.

throw new Error(`Unknown variation type: ${variationType}`);
}
describe('Shared UFC General Test Cases', () => {
const testCases = testCasesByFileName<IAssignmentTestCase>(ASSIGNMENT_TEST_DATA_DIR);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes for nice, unique test case names / output
image

@@ -236,9 +302,9 @@ describe('EppoClient E2E test', () => {
};

it('retries initial configuration request before resolving', async () => {
td.replace(HttpClient.prototype, 'get');
td.replace(HttpClient.prototype, 'getUniversalFlagConfiguration');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our http client interface now has two methods, one for the UFC and one for bandit model parameters

clientInstance = new EppoClient(configurationStore, requestConfiguration);
clientInstance.setLogger(config.assignmentLogger);
const flagConfigurationStore = new MemoryOnlyConfigurationStore<Flag>();
const banditVariationConfigurationStore = new MemoryOnlyConfigurationStore<BanditVariation[]>();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This store is needed to easily know if a flag has any bandits, which we want for handling an edge case of empty actions being passed to getBanditAction()

clientInstance.setLogger(config.assignmentLogger);
const flagConfigurationStore = new MemoryOnlyConfigurationStore<Flag>();
const banditVariationConfigurationStore = new MemoryOnlyConfigurationStore<BanditVariation[]>();
const banditModelConfigurationStore = new MemoryOnlyConfigurationStore<BanditParameters>();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This store holds the nitty gritty model parameters (coefficients, gamma, etc.)

@@ -227,6 +254,66 @@ describe('EppoClient E2E test', () => {
});
});

describe('Shared Bandit Test Cases', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bandits in the Node SDK!

Comment on lines +18 to +20
const ufcFile = req.url.includes(TEST_BANDIT_API_KEY)
? MOCK_FLAGS_WITH_BANDITS_RESPONSE_FILE
: MOCK_UFC_RESPONSE_FILE;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests use one file, some use the other; use the API key to decide which to serve

@aarsilv aarsilv marked this pull request as ready for review July 3, 2024 20:33
Copy link
Collaborator

@giorgiomartini0 giorgiomartini0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't quite follow the comments about the mock stores in index.spec.ts, or evaluate your choice of spinning up a server vs mocking. What I did understand looks great!

Comment on lines +299 to +306
// Do this check in addition to assertions to provide helpful information on exactly which
// evaluation failed to produce an expected result
if (
banditAssignment.variation !== subject.assignment.variation ||
banditAssignment.action !== subject.assignment.action
) {
console.error(`Unexpected result for flag ${flagKey} and subject ${subject.subjectKey}`);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@felipecsl felipecsl self-requested a review July 8, 2024 16:21
@aarsilv aarsilv merged commit c397ca4 into main Jul 8, 2024
2 checks passed
@aarsilv aarsilv deleted the aaron/ff-2021/add-bandits branch July 8, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants