Conversation
a0917ea to
27cc3a6
Compare
cbdffd1 to
62a765b
Compare
5997d33 to
bbd9550
Compare
bbd9550 to
739d310
Compare
739d310 to
a08c2ec
Compare
| } | ||
|
|
||
| ParseResult<Configuration> parseConfiguration(const std::string& flagConfigJson, | ||
| const std::string& banditModelsJson) { |
There was a problem hiding this comment.
minor: we have a configuration wire format that combines both flags and bandits configuration in the same json. I wonder if we should reserve parseConfiguration for that.
(From public API perspective, I think we only want to expose this configuration wire parsing as this is our intended format for passing configuration to/from SDKs.)
There was a problem hiding this comment.
I think this is fine, since we're likely not going to need to pull in configuration wire functionality in this SDK
src/configuration.hpp
Outdated
| * @param banditModelsJson JSON string containing bandit models (optional - pass empty string to | ||
| * skip) |
There was a problem hiding this comment.
nit: we might provide another parseConfiguration override that only takes a single argument, so this "pass empty string" is not exposed to user
There was a problem hiding this comment.
I think that comment wasn't necessary to begin with, since it's an optional parameter. Though adding the override is probably a bit more clear, so I'll update.
| run-bandits: examples | ||
| @echo "Running bandits example..." | ||
| @$(BUILD_DIR)/bandits | ||
| @cd examples && ../$(BUILD_DIR)/bandits |
There was a problem hiding this comment.
nit: do we expect examples to run from ./examples directory? Not opposed to that but that seems inconsistent with tests, that run from repository root
There was a problem hiding this comment.
I think this makes more sense than updating the relative paths in the examples, yeah. These are meant to be self-contained examples that the user could copy if they wanted.
Summary
Introduces a new
parseConfiguration()helper function that simplifies configuration setup for SDK users.Example: