feat: Bandits part 4: Evaluation #27
Conversation
src/Bandits/BanditEvaluator.php
Outdated
| if (empty($actionsWithContexts)) { | ||
| throw new InvalidArgumentException("No actions provided for bandit evaluation"); | ||
| } |
There was a problem hiding this comment.
We only want to do this if the bandit is still at play (e.g., this flag has a bandit).
This is to support the case of users removing bandits from the flag more in a friendly way, such as rolling out a winning variation. That way, if they just stop supplying actions but leave this in, it will still work as they expect.
You can see the node example here: https://github.com/Eppo-exp/js-client-sdk-common/blob/main/src/client/eppo-client.ts#L471
There was a problem hiding this comment.
This is actually handled up in the EppoClient. My reasoning for throwing an exception here is that if we're invoking the BanditEvaluator, we've already matched an assignment to a bandit, pulled the Bandit model and should not have done all that work if the action list is empty.
There was a problem hiding this comment.
Following the parallel discussion in Slack and the PR for the JS commons SDK, does this path eventually return the default variation? Or is the InvalidArgumentException handled differently? (Maybe it never even reaches this point if the action list is empty?)
There was a problem hiding this comment.
This will return but not log the default variation. It sounds like our latest plan is for us to wait to check for zero actions until a variation has been assigned?
src/Bandits/BanditEvaluator.php
Outdated
| if (empty($actionsWithContexts)) { | ||
| throw new InvalidArgumentException("No actions provided for bandit evaluation"); | ||
| } |
There was a problem hiding this comment.
Following the parallel discussion in Slack and the PR for the JS commons SDK, does this path eventually return the default variation? Or is the InvalidArgumentException handled differently? (Maybe it never even reaches this point if the action list is empty?)
src/Bandits/BanditEvaluator.php
Outdated
| $selectedActionContext = $actionsWithContexts[$selectedAction]; | ||
| $actionScore = $actionScores[$selectedAction]; | ||
| $actionWeight = $actionWeights[$selectedAction]; |
There was a problem hiding this comment.
nit: for consistency, should these be selectedActionScore and selectedActionWeight?
| $remainingWeight = max(0.0, 1.0 - array_sum($weights)); | ||
| $weights[$bestActionKey] = $remainingWeight; |
There was a problem hiding this comment.
Very clever! Took me a moment to grok; this is adding a new key to weights (bestActionKey was filtered out above), not overwriting it.
src/Bandits/BanditEvaluator.php
Outdated
| if (empty($actionsWithContexts)) { | ||
| throw new InvalidArgumentException("No actions provided for bandit evaluation"); | ||
| } |
There was a problem hiding this comment.
This will return but not log the default variation. It sounds like our latest plan is for us to wait to check for zero actions until a variation has been assigned?
src/Bandits/BanditEvaluator.php
Outdated
| if ($aValue == $bValue) { | ||
| return $a->action <=> $b->action; | ||
| } | ||
| return $aValue <=> $bValue; |
There was a problem hiding this comment.
🙌
Could this be simplified to:
return ($aValue <=> $bValue) ?: ($a->action <=> $b->action);| $this->assertEquals($expectedScore, $actualScore); | ||
| } | ||
|
|
||
| public function testScoreNumericIgnoringNonNumericAttributes(): void |
|
|
||
| public function testScoreNumericIgnoringNonNumericAttributes(): void | ||
| { | ||
| $numericAttributes = ['age' => 30, 'height' => 170, 'shouldBeANumber' => 'but_it_is_not']; |
|
Thanks for the review, folks. Integrating this and applying the recent no-actions action in the Client PR. |
Motivation and Context
Here is where the magic happens; where user data is matrix multiplied against bandit coefficient models to determine the best action for the user.
Description
How has this been tested