Selki/ff 3514 bandit parameters polling only when needed #131
Conversation
|
2 unit tests are failing due to changes in test json file that happened before I even forked the repo. I've fixed them already though! But in this PR so we can ignore those here. :)) |
There was a problem hiding this comment.
I beefed up the tests! But I've never done this kind of fetch mocking shenanigans before in js, so I'm not 100% confident in the approach I took: passing mock response generator function to fetchSpy initializer and the updateUFC flag which serves as a semi global variable - I hate using this but nothing better comes to my mind tbh.
There was a problem hiding this comment.
Thanks for iterating!
Approving, but before merging you'll want to do three things:
- Rebase on latest main and ensure
package.jsonis just a patch (last number) bump - Install this locally and put together a corresponding change and PR for the node SDK to make sure it works upstream as intended
- Get the sign off from @typotter who requested changes
src/configuration-requestor.spec.ts
Outdated
| expect(fetchSpy).toHaveBeenCalledTimes(3); // Once just for UFC, bandits should be skipped | ||
| }); | ||
|
|
||
| const warmStartBanditReference = { |
There was a problem hiding this comment.
these will be hoised to the top-level describe. Consider putting them in either in it's new nested describe() block along with the three tests that use it.
src/configuration-requestor.spec.ts
Outdated
| * 1. initial call - 1 fetch for ufc 1 for bandits | ||
| * 2. 2nd call - 1 fetch for ufc only; bandits unchanged | ||
| * 3. 3rd call - new bandit ref injected to UFC; 2 fetches, because new bandit appeared | ||
| * 4. 4th call - we remove a bandit from ufc; 1 fetch because there is no need to update. | ||
| * The bandit removed from UFC should still be in memory. |
There was a problem hiding this comment.
👍 agreed - super handy. It's easy to get lost when a test has to do multiple rounds of Arrange-Act-Assert to fully test functionality.
maya-the-mango
force-pushed
the
selki/FF-3514-bandit-parameters-polling-only-when-needed
branch
from
490336d to
31cde96
Compare
labels: mergeable
Fixes: #issue
Motivation and Context
//: # Requesting bandits only when needed (when modelVersions differ) saves bandwidth! :)
Description
//: # Before requesting new bandits I do a simple comparison of loaded model version vs the newest model version contained in UFC
How has this been tested?