-
Notifications
You must be signed in to change notification settings - Fork 0
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 bandit support #25
Conversation
@@ -23,6 +23,7 @@ dependencies { | |||
testImplementation 'commons-io:commons-io:2.11.0' | |||
testImplementation "com.google.truth:truth:1.4.4" | |||
testImplementation 'org.mockito:mockito-core:4.11.0' | |||
testImplementation 'org.mockito:mockito-inline:4.11.0' |
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.
Needed for mocking static methods (such as evaluateBandit()
)
// The coefficient implementation knows how to score | ||
double attributeScore = attributeCoefficients.scoreForAttributeValue(contextValue); |
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.
This is a bit different than other SDKs; the logic for how to score a given attribute value is collocated with the type of attribute (i.e. BanditNumericAttributeCoefficients
and BanditCategoricalAttributeCoefficients
)
Set<String> neededModelVersions = configurationStore.banditModelVersions(); | ||
boolean needBanditParameters = !loadedBanditModelVersions.containsAll(neededModelVersions); |
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.
Taking advantage of the latest and greatest UFC feature: bandit model versions.
We will only request bandit parameters if we are missing model versions at play (as opposed to every time we poll if there are one or more bandits).
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.
Shooting for bleeding edge all at once eh? I like your style π€
} else { | ||
// TODO: atomic flags to prevent clobbering like android does | ||
// Record that flags were set from a response so we don't later clobber them with a | ||
// slow cache read | ||
flags = new ConcurrentHashMap<>(config.getFlags()); | ||
log.debug("Loaded " + flags.size() + " flags from configuration response"); | ||
banditReferences = new ConcurrentHashMap<>(config.getBanditReferences()); |
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.
banditReferences
is a top-level UFC property containing some information on any bandits at play (but not their model data, which is provided separately)
for (BanditFlagVariation banditFlagVariation : banditReference.getFlagVariations()) { | ||
if (banditFlagVariation.getFlagKey().equals(flagKey) |
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.
While we could use the shortcut of assuming the variation key and value and bandit key are all the same, since the UFC explicitly provides the information to be more specific, I'll take advantage of it. We can have other SDKs do this once needed.
private static final BanditLogger mockBanditLogger = mock(BanditLogger.class); | ||
private static final Date testStart = new Date(); | ||
|
||
// TODO: possibly consolidate code between this and the non-bandit test |
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.
Thinking of leaving this as a TODO for now until everything is said and done. I don't want to prematurely combine parts of tests only to have things diverge once non-bandit-only (for now) things like caching is layered on.
EppoClient.getInstance().setIsGracefulFailureMode(false); | ||
} | ||
|
||
@ParameterizedTest |
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.
Shared UFC Bandit Tests! πͺ
import static org.junit.Assert.assertThrows; | ||
import static org.junit.Assert.assertTrue; | ||
import static org.junit.Assert.fail; | ||
import static org.junit.jupiter.api.Assertions.assertEquals; |
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.
Consolidating on the jupiter flavors to match the imported @Test
@@ -201,26 +192,26 @@ private <T> void assertAssignment( | |||
|
|||
if (assignment instanceof Boolean) { | |||
assertEquals( | |||
failureMessage, expectedSubjectAssignment.getAssignment().booleanValue(), assignment); | |||
expectedSubjectAssignment.getAssignment().booleanValue(), assignment, failureMessage); |
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.
jupiter flavored asserts have the error message at the end (which I like more anyways π€·ββοΈ )
private final String flag; | ||
private final String defaultValue; | ||
private final List<SubjectBanditAssignment> subjects; | ||
private String fileName; |
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.
Set later (after deserialization) to make helpful test case "names" that are displayed in IDE/terminal
@@ -0,0 +1,101 @@ | |||
{ |
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.
For now, our deserializer tests use their own files (as opposed to the shared UFC test file). I think this is ok, as it makes the deserializer tests less fragile. The shared test data should ensure that the SDK correctly handles anything new thrown at it. Nevertheless, we could consider having the deserializer tests also use shared test data.
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.
just my 0.02 (or 0.0144USD after adjusting for ForEx)
For deserialization, I think using these files is the best way to go. Imo, it doesn't make a lot of sense to pull in dynamic data, parse it, and then assert on hard-coded values. Regardless of whether the underlying data is stable, the mismatch of dynamic vs coded feels awkward and hangs a lampshade on the fragility.
throw new RuntimeException(ex); | ||
} | ||
assertTrue(capturedAssignment.getTimestamp().after(testStart)); | ||
Date inTheNearFuture = new Date(System.currentTimeMillis() + 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.
Deflake the test for when things are particularly speedy
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.
Impeccable! Big THANK YOU for the detailed comments explaining what changed and why π
|
||
private final String flagKey; | ||
private final String subjectKey; | ||
private final DiscriminableAttributes subjectAttributes; |
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.
Fancy word! I like it
@@ -60,7 +60,7 @@ private List<SubjectAssignment> deserializeSubjectAssignments(JsonNode jsonNode) | |||
} | |||
|
|||
private TestCaseValue deserializeTestCaseValue(JsonNode jsonNode) { | |||
if (jsonNode != null && jsonNode.isObject()) { | |||
if (jsonNode != null && (jsonNode.isObject() || jsonNode.isArray())) { |
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.
Is this defensive or is there a specific test that needs it?
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.
We have a test case with JSON array now! https://github.com/Eppo-exp/sdk-test-data/blob/main/ufc/tests/test-json-array-config-flag.json#L4
Added four days ago (PR) when a customer caught it wasn't working for them π
We're still deciding as a company if we actually want to support this.
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.
I actually rolled back that test case earlier after talking with Leo. Some of the SDKs would need a fair amount of work to support the JSON Array of Objects as a variation value. More work, I think, than is agreeable while it's questionable whether support for this is desired.
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.
Rolling back the test case sounds good. Any harm in leaving support in SDKs that can handle it (this one, node, etc.?)
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.
no hard that I can think of - especially on the test parsing side.
// Note: In practice this double loop should be quite quick as the number of bandits and bandit | ||
// variations will be small. Should this ever change, we can optimize things. |
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.
Agreed - no need to prematurely optimize this.
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.
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.
what a monster of a pull πͺ
Requesting changes but only because of the math mismatch here
// Compute weight (probability) | ||
double unboundedProbability = | ||
1 / (actionScores.size() + (gamma * (highestScore - actionScore.getValue()))); | ||
double boundedProbability = Math.max(unboundedProbability, actionProbabilityFloor); |
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.
In the other SDKs, we divide actionProbabilityFloor by the number of actions to weigh;
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.
Yikes! GREAT catch π Dividing by actions is what we should do.
This logic was part of what was copied over from the original Java implementation, before we changed up our strategy, and I failed to update it.
What this does mean is that we don't have a test enforcing that logic. It may be hard to put one together, but I'll see if I can. π€
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.
The reason why this doesn't get teased out in a test is because we're using small min probability floors and typically a gamma of 1 with only a couple of actions. With some tweaking of scoring parameters, should be able to create a test case that pushes it over the edge
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.
PR for test cases that cover this (and more!) here: Eppo-exp/sdk-test-data#52
ShardUtils.getShard( | ||
flagKey + "-" + subjectKey + "-" + actionKey, | ||
BANDIT_ASSIGNMENT_SHARDS)) | ||
.thenComparing(actionKey -> actionKey)) |
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.
π
Set<String> neededModelVersions = configurationStore.banditModelVersions(); | ||
boolean needBanditParameters = !loadedBanditModelVersions.containsAll(neededModelVersions); |
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.
Shooting for bleeding edge all at once eh? I like your style π€
public void load() { | ||
log.debug("Fetching configuration"); | ||
Response response = client.get("/api/flag-config/v1/config"); | ||
String flagConfigurationJsonString = requestBody("/api/flag-config/v1/config"); | ||
configurationStore.setFlagsFromJsonString(flagConfigurationJsonString); |
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.
Do you need to worry about threading here and lock reads until the bandit models are set below?
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.
Good thinking! The underlying implementation is a thread-safe ConcurrentHashMap, which is what we are using in the Android SDK without (known) issues
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.
The ConcurrentHashMap underlying will keep reads and writes happening in an orderly fashion, but there's no lock around the setting of the flags and the subsequent setting of the bandit parameters below, so, there's a brief instant the config store could be accessed while it has incomplete configuration set. If a caller manages to evaluate a bandit between those two instructions, the SDK will have incomplete data and may not have the required bandit. The scenario is probably unlikely, contrived and probably nearly impossible to test but I'm curious how wrong things would go if this happened (user would get returned the bandit ket as the variation with action=null which the devs should already have handling for in their app).
String flagConfigurationJsonString = requestBody("/api/flag-config/v1/config"); | ||
configurationStore.setFlagsFromJsonString(flagConfigurationJsonString); | ||
|
||
Set<String> neededModelVersions = configurationStore.banditModelVersions(); |
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.
Is it possible for more than one bandit to have the same model version string?
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.
No, model versions are unique to each bandit.
Under the hood, this version ID--for now--is the auto incrementing primary key in a model_versions table that has versions for all bandits (as it includes the bandit id)
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.
Thanks for the clarification
// Note: In practice this double loop should be quite quick as the number of bandits and bandit | ||
// variations will be small. Should this ever change, we can optimize things. |
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.
} | ||
|
||
// Update result to reflect that we've been assigned an action | ||
result = new BanditResult(assignedVariation, banditResult.getActionKey()); |
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.
result should get set above the call to banditLogger.logBanditAssignment
. This ensures that if the user-provided bandit logger throws an exception, the throwIfNotGraceful
call below has an accurate result to return.
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.
Great point. That should be done and all logger calls should be wrapped in try-catches so they are non fatal
import java.util.Date; | ||
import java.util.Map; | ||
|
||
public class BanditAssignment { |
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.
Would you consider public final fields instead of getters? I'm not sure what the java wisdom would say 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.
My impression is that Java has a weird obsession with getters π
The way you access fields for Record
s seems to confirm this.
Avoiding directly referencing fields will also make interacting with this class consistent with all the others, which I like.
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.
Works for me. In the case of the SDKs, my preference is going to always be to "do it like the community does it" regardless of how neat and tidy I might find it.
import java.util.Set; | ||
import java.util.stream.Collectors; | ||
|
||
public class BanditActions extends HashMap<String, DiscriminableAttributes> implements Actions { |
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.
π§βπ³
sign publishing.publications.mavenJava | ||
if (System.env['CI']) { | ||
useInMemoryPgpKeys(System.env.GPG_PRIVATE_KEY, System.env.GPG_PASSPHRASE) | ||
if (!project.gradle.startParameter.taskNames.contains('publishToMavenLocal')) { |
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.
Don't sign the maven package if publishing to local maven repository (for multi-repository development)
flagKey, subjectKey, subjectAttributes, actions, banditParameters.getModelData()); | ||
|
||
// Update result to reflect that we've been assigned an action | ||
result = new BanditResult(assignedVariation, banditResult.getActionKey()); |
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.
@typotter we now set the action part of the result as soon as we get it, and surround bandit logging with a try
-catch
like we do assignment logging.
Non-fatal logging error tests were added as well.
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.
awesomesauce
@Test | ||
public void testAssignmentLogErrorNonFatal() { |
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.
Adding a test to ensure errors with logging assignments are non-fatal
} | ||
|
||
@Test | ||
public void testBanditLogErrorNonFatal() { |
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.
@typotter test case to ensure any issues with the user-provided bandit logging method do not interfere with returning the bandit result.
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.
setting the bar higher and higher. πͺ
// Compute weight (probability) | ||
double unboundedProbability = | ||
1 / (actionScores.size() + (gamma * (highestScore - actionScore.getValue()))); | ||
double minimumProbability = actionProbabilityFloor / actionScores.size(); |
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.
@typotter bandit probability floor now normalized
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.
π
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.
β΅
// Compute weight (probability) | ||
double unboundedProbability = | ||
1 / (actionScores.size() + (gamma * (highestScore - actionScore.getValue()))); | ||
double minimumProbability = actionProbabilityFloor / actionScores.size(); |
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.
π
String flagConfigurationJsonString = requestBody("/api/flag-config/v1/config"); | ||
configurationStore.setFlagsFromJsonString(flagConfigurationJsonString); | ||
|
||
Set<String> neededModelVersions = configurationStore.banditModelVersions(); |
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.
Thanks for the clarification
public void load() { | ||
log.debug("Fetching configuration"); | ||
Response response = client.get("/api/flag-config/v1/config"); | ||
String flagConfigurationJsonString = requestBody("/api/flag-config/v1/config"); | ||
configurationStore.setFlagsFromJsonString(flagConfigurationJsonString); |
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.
The ConcurrentHashMap underlying will keep reads and writes happening in an orderly fashion, but there's no lock around the setting of the flags and the subsequent setting of the bandit parameters below, so, there's a brief instant the config store could be accessed while it has incomplete configuration set. If a caller manages to evaluate a bandit between those two instructions, the SDK will have incomplete data and may not have the required bandit. The scenario is probably unlikely, contrived and probably nearly impossible to test but I'm curious how wrong things would go if this happened (user would get returned the bandit ket as the variation with action=null which the devs should already have handling for in their app).
flagKey, subjectKey, subjectAttributes, actions, banditParameters.getModelData()); | ||
|
||
// Update result to reflect that we've been assigned an action | ||
result = new BanditResult(assignedVariation, banditResult.getActionKey()); |
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.
awesomesauce
@@ -79,11 +55,11 @@ public String getSubject() { | |||
return subject; | |||
} | |||
|
|||
public String getTimestamp() { | |||
public Date getTimestamp() { |
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.
Since you're changing the data type of the date value in Assignment
, would this be considered a breaking change in the SDKs which consume this class? Devs will need to make a code change in order to manage the new data type. I am not sure what the best way to signal downstream that there's an underlying API change that'll require appropriate version bumps.
import java.util.Date; | ||
import java.util.Map; | ||
|
||
public class BanditAssignment { |
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.
Works for me. In the case of the SDKs, my preference is going to always be to "do it like the community does it" regardless of how neat and tidy I might find it.
} | ||
|
||
@Test | ||
public void testBanditLogErrorNonFatal() { |
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.
setting the bar higher and higher. πͺ
@@ -0,0 +1,101 @@ | |||
{ |
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.
just my 0.02 (or 0.0144USD after adjusting for ForEx)
For deserialization, I think using these files is the best way to go. Imo, it doesn't make a lot of sense to pull in dynamic data, parse it, and then assert on hard-coded values. Regardless of whether the underlying data is stable, the mismatch of dynamic vs coded feels awkward and hangs a lampshade on the fragility.
* Eppo Client with shared UFC tests passing (#23) * tests passing for rule evaluator, flag evaluator, and eppo value * work in progress * shared UFC tests passing * don't check in test data * changes from self-review of PR * apply spotless linter * working on tests * better test logging * use make test for tests * Add bandit support (#25) * bandit test harness ready * add bandit result * set up for dropping in bandit evaluation * bandit deserialization wired up * loading bandit parameters * bandit stuff happening * shared bandit tests passing * bandit logger classes * bandit log test passing * more tests for logger * bandit tests for graceful mode * apply spotless formatting autofix * changes from self-review of PR so far * more changes from self-review of PR * more changes from self-review * make test less fragile * bump version; don't sign local maven * bandit logging errors should be non-fatal * use normalized probability floor * update result before even attempting to log bandit * spotless π * do the rename (#26) * Remove singleton pattern (#28) * work in progress * remove singleton for base client * linter * expose bandit test harnesses * expose test uilities * changes from self-review of PR * make base client constructor protected
* Eppo Client with shared UFC tests passing (#23) * tests passing for rule evaluator, flag evaluator, and eppo value * work in progress * shared UFC tests passing * don't check in test data * changes from self-review of PR * apply spotless linter * working on tests * better test logging * use make test for tests * Add bandit support (#25) * bandit test harness ready * add bandit result * set up for dropping in bandit evaluation * bandit deserialization wired up * loading bandit parameters * bandit stuff happening * shared bandit tests passing * bandit logger classes * bandit log test passing * more tests for logger * bandit tests for graceful mode * apply spotless formatting autofix * changes from self-review of PR so far * more changes from self-review of PR * more changes from self-review * make test less fragile * bump version; don't sign local maven * bandit logging errors should be non-fatal * use normalized probability floor * update result before even attempting to log bandit * spotless π * do the rename (#26) * work in progress * remove singleton for base client * linter * expose bandit test harnesses * expose test uilities * changes from self-review of PR * make base client constructor protected * add simple logger for tests * basic profiling test * improvement not using bigint * faster getShard() * linter * more helpful failure message * increase CPU time allowance to account for slower machines * bump version
* Eppo Client with shared UFC tests passing (#23) * tests passing for rule evaluator, flag evaluator, and eppo value * work in progress * shared UFC tests passing * don't check in test data * changes from self-review of PR * apply spotless linter * working on tests * better test logging * use make test for tests * Add bandit support (#25) * bandit test harness ready * add bandit result * set up for dropping in bandit evaluation * bandit deserialization wired up * loading bandit parameters * bandit stuff happening * shared bandit tests passing * bandit logger classes * bandit log test passing * more tests for logger * bandit tests for graceful mode * apply spotless formatting autofix * changes from self-review of PR so far * more changes from self-review of PR * more changes from self-review * make test less fragile * bump version; don't sign local maven * bandit logging errors should be non-fatal * use normalized probability floor * update result before even attempting to log bandit * spotless π * do the rename (#26) * work in progress * remove singleton for base client * linter * expose bandit test harnesses * expose test uilities * changes from self-review of PR * make base client constructor protected * add simple logger for tests * basic profiling test * improvement not using bigint * faster getShard() * linter * more helpful failure message * increase CPU time allowance to account for slower machines * bump version * download test data for tests
Eppo Internal:
ποΈ Ticket: FF-2743 - Common Shared Bandits Tests Passing
This pull request adds support for contextual multi-armed bandits to the common SDK for future use by upstream Universal Flag Configuration (UFC)-powered Java and--perhaps in the future--Android SDKs.
Much of the classes, tests, and logic have been ported from the existing Java bandits implementation, updated to account for new decisions and edge-case handling as bandit development has taken place.
Bandit functionality is verified against our shared SDK test data bandit tests. Additional tests ensure bandits log and handle errors as expected.