[CLIPPER-105] Refactor selection policy to support single-model applications with default output #89

dcrankshaw · 2017-03-18T20:35:39Z

This is a big PR, but it's not quite as big as it looks. I refactored the JSON utils to split it between a .hpp and a .cpp file and I fixed some formatting with the code formatter. The big changes are in the query processor and the selection policies.

I refactored the selection policy API to be an abstract base class that implementations inherit from. This makes adding a new policy much simpler than the templates-based API we had previously and cleans up the QueryProcessor implementation substantially. I also ripped out all the existing policies and added the DefaultOutputSelectionPolicy which returns the model prediction if it arrives in time, and otherwise returns a static default output that is configured on a per-application basis.

Currently this PR does not update clipper_manager.py or the tutorial to the new selection policy implementation, which means they are broken.

Do you think we should update them here and fix this all in one PR so that we go from working codebase to working codebase? Or should we separate them into to PRs because they are somewhat distinct changes?

AmplabJenkins · 2017-03-18T20:48:14Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/86/
Test PASSed.

AmplabJenkins · 2017-03-18T20:48:27Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/87/
Test FAILed.

AmplabJenkins · 2017-03-20T02:39:01Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/88/
Test PASSed.

Corey-Zumar · 2017-03-20T09:41:53Z

Regarding required changes to clipper_manager.py and the tutorial, a separate PR seems to make the most sense. I think it's best to hold off on merging this PR until a PR has been submitted and reviewed that makes the necessary changes to the manager and tutorial components.

Reviewing this now...

Corey-Zumar

Having trouble running the end-to-end benchmark with these policy changes. Can you take a look at this?

In general, the changes make sense - comments are mostly nits and documentation.

Corey-Zumar · 2017-03-20T09:53:35Z

src/libclipper/include/clipper/persistent_state.hpp

@@ -25,7 +25,7 @@ size_t state_key_hash(const StateKey& key);
 // Threadsafe, non-copyable state storage
 class StateDB {
 public:
-  StateDB();
+  explicit StateDB();


A zero-argument constructor doesn't benefit from the explicit keyword

Good catch. I added a single-argument constructor then deleted it and forgot to remove explicit.

Corey-Zumar · 2017-03-20T09:58:30Z

src/libclipper/include/clipper/selection_policies.hpp

-                               std::function<size_t(const VersionedModelId&)>>;
-
-class BanditPolicyState {
+class SelectionState {


We should have some documentation here explaining the role of selection state in enacting selection policies.

Corey-Zumar · 2017-03-20T09:59:27Z

src/libclipper/include/clipper/selection_policies.hpp

+  SelectionState(SelectionState&&) = default;
+  SelectionState& operator=(SelectionState&&) = default;
+  virtual ~SelectionState() = default;
+  virtual std::string get_debug_string() const = 0;


When would this be called? Is there an intended format for the debug string?

It is called from the admin frontend. There's no particular format any policy needs to conform to. Whatever the implementer thinks would be useful information to expose for debugging/runtime inspection.

Corey-Zumar · 2017-03-20T10:03:58Z

src/libclipper/include/clipper/selection_policies.hpp

+  DefaultOutputSelectionPolicy& operator=(DefaultOutputSelectionPolicy&&) =
+      default;
+  ~DefaultOutputSelectionPolicy() = default;
+  std::shared_ptr<SelectionState> init_state(Output default_output) const;


What does this method do? It seems like state initialization will be necessary for many selection policies; we should consider defining a virtual init_state method in SelectionPolicy and overriding it here.

The problem with defining a virtual init_state method is that it's likely the arguments to it will vary based on the selection policy. So I'm not sure what the function signature would be. For the 0.1 release, we only have one selection policy (the DefaultOutputSelectionPolicy). init_state is called each time a new application is created, which is the correct behavior for this selection policy. When we expand our functionality in 0.2 to support more sophisticated types of selection policies, we will need to revisit this. But I already knew we would need to refine our selection policy API anyway. CLIPPER-99 will track this effort.

Corey-Zumar · 2017-03-20T10:09:06Z

src/libclipper/src/selection_policies.cpp

-  // Turn y_hat into either 0 or 1
-  if (y_hat < 0.5) {
-    y_hat = 0.0;
+  int num_candidate_models = query.candidate_models_.size();


nit: this should be a size_t

Corey-Zumar · 2017-03-20T10:28:34Z

src/libclipper/test/redis_test.cpp

@@ -147,17 +147,19 @@ TEST_F(RedisTest, AddApplication) {
      std::make_pair("simple_svm", 2), std::make_pair("music_cnn", 4)};
  InputType input_type = InputType::Doubles;
  std::string policy = "exp3_policy";


This is no longer an accurate name.

Corey-Zumar · 2017-03-20T10:30:53Z

src/libclipper/test/selection_policies_test.cpp

-  auto bytes = Exp3Policy::serialize_state(state);
-  auto new_state = Exp3Policy::deserialize_state(bytes);
-  ASSERT_EQ(state.weight_sum_, new_state.weight_sum_);
+TEST_F(DefaultOutputSelectionPolicyTest, InitState) {}


This is an empty test

Oops. Removed it. There's not much to test there.

Corey-Zumar · 2017-03-20T10:34:13Z

src/libclipper/test/selection_policies_test.cpp

-  ASSERT_EQ(state.weight_sum_, new_state.weight_sum_);
+TEST_F(DefaultOutputSelectionPolicyTest, InitState) {}
+
+TEST_F(DefaultOutputSelectionPolicyTest, TestSelectPredictTasks) {


We should add some documentation or use a more descriptive test name.

Split the SelectPredictTask and CombinePredictions tests into individual test cases with more descriptive test names.

Corey-Zumar · 2017-03-20T10:58:05Z

src/frontends/src/query_frontend.hpp

+    // selection policies have a default output?
+    //
+    // Initialize selection state for this application
+    if (policy == "DefaultOutputSelectionPolicy") {


We should define a static get_name() function within each selection policy.

policy == DefaultOutputSelectionPolicy::get_name() is cleaner and less error prone.

Corey-Zumar · 2017-03-20T11:29:18Z

src/libclipper/src/query_processor.cpp

+  std::shared_ptr<SelectionPolicy> current_policy = current_policy_iter->second;
+
+  auto state_opt = state_db_->get(StateKey{query.label_, query.user_id_, 0});
+  if (!state_opt) {


After changing the specified selection policy in the end-to-end benchmark from "EXP3" to "DefaultOutputSelectionPolicy", the benchmark is still logging an error here:

[04:22:23.560][error] [QUERYPR...] No selection state found for query with label: test

AmplabJenkins · 2017-03-22T18:21:17Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/95/
Test FAILed.

dcrankshaw · 2017-03-23T18:36:13Z

I fixed the clipper_manager library, but this PR still breaks the tutorial. Unfortunately, we won't really be able to fix it until CLIPPER-111 is implemented so that we can update the version of a model associated with an application.

AmplabJenkins · 2017-03-23T18:52:40Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/98/
Test PASSed.

Corey-Zumar

Looks good to me. We shouldn't merge this until CLIPPER-111 is addressed and the tutorial is fixed.

AmplabJenkins · 2017-03-31T21:03:58Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/110/
Test PASSed.

dcrankshaw · 2017-04-07T03:20:53Z

Once #115 is merged, this can be rebased on develop and will be ready for merge.

…_utils

Corey-Zumar · 2017-04-12T23:36:11Z

Note: Tutorial calls to deploy_model() need to be refactored to specify a default output instead of a selection policy.

dcrankshaw · 2017-04-14T00:20:25Z

@Corey-Zumar I rebased on develop to include the model versioning PR #115 and fixed the tutorial to specify a default output instead of a selection policy. I believe this is ready to go. Definitely do a last pass over the code, and run the tutorial to make sure it works (I ran it as well without any problems). When you run the tutorial, note that you'll need to build the Clipper docker containers locally to test.

…n_policy

AmplabJenkins · 2017-04-14T00:31:09Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/145/
Test PASSed.

AmplabJenkins · 2017-04-14T00:43:30Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/146/
Test PASSed.

Corey-Zumar · 2017-04-14T04:04:56Z

Confirmed that the end_to_end_benchmark is still functional, the tutorial is working properly, and all tests pass. LGTM. Merging...

dcrankshaw added the status: needs review label Mar 18, 2017

dcrankshaw self-assigned this Mar 18, 2017

dcrankshaw requested a review from Corey-Zumar March 18, 2017 20:35

dcrankshaw force-pushed the refactor_selection_policy branch from a269c57 to 614dafd Compare March 18, 2017 20:42

dcrankshaw force-pushed the refactor_selection_policy branch from 614dafd to 33e8d84 Compare March 20, 2017 02:26

Corey-Zumar requested changes Mar 20, 2017

View reviewed changes

dcrankshaw removed the status: needs review label Mar 22, 2017

dcrankshaw force-pushed the refactor_selection_policy branch from e29ee8c to 055b914 Compare March 23, 2017 18:32

Corey-Zumar approved these changes Mar 24, 2017

View reviewed changes

dcrankshaw force-pushed the refactor_selection_policy branch from 055b914 to 8c66acf Compare March 31, 2017 20:53

This was referenced Apr 9, 2017

[CLIPPER-107][CLIPPER-109] Implement JSON-formatted responses for query frontend dcrankshaw/clipper#2

Closed

[CLIPPER-107][CLIPPER-109] Return JSON-formatted responses to prediction queries #116

Merged

This was referenced Apr 10, 2017

Clipper prediction doesn't seem to match the model prediction before deployment #118

Closed

Failed to test the tutorial in the clipper source code version #117

Closed

dcrankshaw added 7 commits April 12, 2017 14:48

draft of the selection policy rewrite

f77f94e

adapted query processor to use new selection policy interface

863c733

minor cleanup

061b035

added default output to application table

28150d5

things are compiling but there's a linker error from header-only json…

fed4f36

…_utils

unit tests passing

13d06c2

addressed review comments and fixed end to end bench

4a47ce2

dcrankshaw added 2 commits April 12, 2017 15:02

fixed unittests

eb666c9

tweaks

4b0dc25

dcrankshaw added 2 commits April 13, 2017 16:46

about to test tutorial

07de73c

tutorial works

5beb668

dcrankshaw force-pushed the refactor_selection_policy branch from 8c66acf to 5beb668 Compare April 14, 2017 00:16

Merge remote-tracking branch 'ucbrise/develop' into refactor_selectio…

e9f5d25

…n_policy

Corey-Zumar merged commit 11f5732 into ucbrise:develop Apr 14, 2017

dcrankshaw deleted the refactor_selection_policy branch May 17, 2017 06:18

[CLIPPER-105] Refactor selection policy to support single-model applications with default output #89

[CLIPPER-105] Refactor selection policy to support single-model applications with default output #89

Conversation

dcrankshaw commented Mar 18, 2017

AmplabJenkins commented Mar 18, 2017

AmplabJenkins commented Mar 18, 2017

AmplabJenkins commented Mar 20, 2017

Corey-Zumar commented Mar 20, 2017 • edited Loading

Corey-Zumar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Mar 22, 2017

dcrankshaw commented Mar 23, 2017

AmplabJenkins commented Mar 23, 2017

Corey-Zumar left a comment • edited Loading

Choose a reason for hiding this comment

AmplabJenkins commented Mar 31, 2017

dcrankshaw commented Apr 7, 2017

Corey-Zumar commented Apr 12, 2017

dcrankshaw commented Apr 14, 2017

AmplabJenkins commented Apr 14, 2017

AmplabJenkins commented Apr 14, 2017

Corey-Zumar commented Apr 14, 2017

Corey-Zumar commented Mar 20, 2017 •

edited

Loading

Corey-Zumar left a comment •

edited

Loading