Rest API #3

giulio-zhou · 2016-11-07T16:58:24Z

parse uid, input (vector) and label from predict and update requests
attaches boost continuation onto future returned from call to qp.predict and qp.update, which then fulfills the http request
included server_http.hpp file directly
TODO: put this in an interface library

* parse uid, input (vector<double>) and label from predict and update requests * attaches boost continuation onto future returned from call to qp.predict and qp.update, which then fulfills the http request * included server_http.hpp file directly TODO: put this in an interface library

dcrankshaw

This looks good!

One key part of the REST API is the ability to configure new endpoints at runtime. For example, someone in devops should be able to deploy a Clipper cluster (potentially with no REST endpoints). A data scientist who wants to add a predictive recommendation application should then be able to add a new REST endpoint to the running Clipper instance without having to write any code or even restart the service.

That shouldn't all happen in this initial implementation, but let's make the implementation general enough to support this going forward. Here are the things that need to happen on a per-endpoint basis (ideally at runtime):

Listen at endpoint (http://clipper:1337/ads, http://clipper:1337/recs, etc).
Parse input
Specify selection policy
Generate query (get list of candidate models, set SLO.

TODO for this PR:

Figure out how to support multiple named endpoints with the same request handler without explicitly hardcoding each endpoint.
Create a generic add_application() function to create each endpoint that takes as arguments those 4 items above. For now this doesn't need to be called at runtime and can take lambdas or something to parse the input and specify the candidate models.

dcrankshaw · 2016-11-07T17:52:34Z

src/frontends/src/bench.cpp

@@ -4,7 +4,7 @@
 #include <utility>
 #include <vector>

-#define BOOST_THREAD_VERSION 3
+#define BOOST_THREAD_VERSION 4


I wonder if there's a canonical place to define this once, so that we don't have to rely on developers setting this every time they include boost/thread.

yes, when using Makefile projects I was just passing the -DBOOST_THREAD_VERSION=X preprocessing flag.

something like this :
https://cmake.org/cmake/help/v3.2/command/add_definitions.html
add_definitions( -DBOOST_THREAD_VERSION=4)

dcrankshaw · 2016-11-07T17:54:17Z

src/frontends/src/bench.cpp

@@ -4,7 +4,7 @@
 #include <utility>
 #include <vector>

-#define BOOST_THREAD_VERSION 3
+#define BOOST_THREAD_VERSION 4
 #include <boost/thread.hpp>
 #include <boost/thread/future.hpp>


I was reading about how to include boost thread this weekend, and it looks like we don't need both include lines. You can remove #include <boost/thread/future.hpp> because boost/thread.hpp includes it.

See http://www.boost.org/doc/libs/1_62_0/doc/html/thread.html

Yeah, turns out it's not necessary. Will remove on the next update to this PR.

dcrankshaw · 2016-11-07T17:58:00Z

src/frontends/src/server_http.hpp

@@ -0,0 +1,438 @@
+#ifndef SERVER_HTTP_HPP


Is this file from Simple-Web-Server? We should attribute the source and include its license in a comment at the top of the file.

We should also add it to src/libs and expose it as an INTERFACE library in cmake using add_library()

Yep, definitely. Will try to get that working on the next update.

dcrankshaw · 2016-11-07T18:06:17Z

src/frontends/src/rest.cpp

+#include "server_http.hpp"
+
+using namespace std;
+using namespace clipper;


Don't include all members of a namespace (see https://google.github.io/styleguide/cppguide.html#Namespaces). You can using ::clipper::QueryProcessor; instead.

In this case, I'd end up with each of these using statements.
using clipper::DoubleVector;
using clipper::FeedbackAck;
using clipper::Input;
using clipper::Output;
using clipper::QueryProcessor;
using clipper::Response;
which seems a bit excessive. Any thoughts?

dcrankshaw · 2016-11-07T18:11:14Z

src/frontends/src/rest.cpp

+using namespace std;
+using namespace clipper;
+using namespace boost::property_tree; 
+typedef SimpleWeb::Server<SimpleWeb::HTTP> HttpServer;


Use alias declarations not typedef. So this should be using HttpServer = SimpleWeb::Server<SimpleWeb::HTTP>;

See Item 9 in Effective Modern C++ for an explanation (I can send you a pdf of it).

Will fix. Also, that would be useful. Thanks!

dcrankshaw · 2016-11-07T18:12:19Z

src/frontends/src/rest.cpp

+}
+
+class RequestHandler {
+    HttpServer server;


Is this private? Let's have an explicit private section.

giulio-zhou · 2016-11-08T17:54:41Z

Updated the request handler with an add_endpoint function (currently defines the entire handling function). Later on, I'll break it up into the multiple functions corresponding to the steps you described.

dcrankshaw · 2016-11-10T17:22:10Z

@giulio-zhou Is this ready for a final review?

giulio-zhou · 2016-11-10T17:30:30Z

Not quite yet, but almost ready. Still need to add server_http.hpp as an INTERFACE library. Let me know if you have any other thoughts on the current state of the code. For now, it may be sufficient to just have the add_endpoint method take in a lambda that does everything.

dcrankshaw · 2016-11-10T17:42:46Z

Cool. Let me know when it's ready. @Corey-Zumar and I spent some time figuring out how to add new third-party header-only libraries as interface libraries the other day. Check out the example for ZeroMQ to get started.

giulio-zhou · 2016-11-10T20:45:03Z

Included server_http as a library, ready for review.

dcrankshaw · 2016-11-11T00:26:32Z

LGTM. Can you add a short README.md in src/frontends that shows an example predict query and we'll get this merged?

giulio-zhou · 2016-11-11T01:06:51Z

Just wanted to clarify something before proceeding. For the readme, are you thinking of, say, how to formulate a prediction query from a Python client? Or more like the steps I'd need to take to define a different endpoint?

dcrankshaw · 2016-11-11T01:18:05Z

How to issue a query with curl. Say what the endpoint is (http://localhost:1337/predict), that it should be a post message, and what the format of the body should be.

giulio-zhou · 2016-11-11T01:22:13Z

Ok, sounds good. I'll do that later tonight.

dcrankshaw · 2016-11-11T02:18:42Z

For some reason @atumanov's comments aren't making it into the thread. To echo them:
something like this :
https://cmake.org/cmake/help/v3.2/command/add_definitions.html
add_definitions( -DBOOST_THREAD_VERSION=4)

giulio-zhou · 2016-11-11T05:24:57Z

Added README and fixed up boost thread version includes.

dcrankshaw · 2016-11-11T06:17:54Z

Merged!

# This is the 1st commit message: started work on spark container # This is the commit message #2: rearranged files in maven packages # This is the commit message #3: training example works # This is the commit message #4: Copies jar to known location # This is the commit message #5: loading container class from jar not working yet # This is the commit message #6: got jar loading working

…o repl support (#133) * # This is a combination of 6 commits. # This is the 1st commit message: started work on spark container # This is the commit message #2: rearranged files in maven packages # This is the commit message #3: training example works # This is the commit message #4: Copies jar to known location # This is the commit message #5: loading container class from jar not working yet # This is the commit message #6: got jar loading working * initial work on incorporating REPL classes. some cleanup and re-org. untested basic example working loading classes defined in REPL still doesn't work started java reorg Reorganized code. Everything compiles Finished implementation. Now need docker container and lots of debugging. start work on docker container fixed bug in docker file Fixed some bugs Clipper local deploy seems to be working Fixed some bugs Deploy is working but there's something wrong with the container Remote deploy is working! Spark scala deploy is working both locally and remotely Formatting started to add tests * mostly done with rebase * rebase complete with existing tests passing * Format code * Fix typo * added unit tests * addressed review comments * add some notes on how to run Spark example * fixed typo in comment

giulio-zhou added 2 commits November 7, 2016 08:46

Updated BOOST_THREAD_VERSION to 4

af8ba4a

dcrankshaw requested changes Nov 7, 2016

View reviewed changes

add_endpoint function to add endpoints after initialization

8f7dbb0

Include server_http as INTERFACE library

3397e55

giulio-zhou changed the title ~~Rest API (first draft, DO NOT MERGE)~~ Rest API Nov 10, 2016

dcrankshaw approved these changes Nov 11, 2016

View reviewed changes

Cleaned up Boost version dependency and added REST README

40bad94

dcrankshaw merged commit 4c10726 into develop Nov 11, 2016

frank2wang87 mentioned this pull request Apr 10, 2017

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

Closed

dcrankshaw deleted the rest-api branch June 23, 2017 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rest API #3

Rest API #3

giulio-zhou commented Nov 7, 2016

dcrankshaw left a comment •

edited

Loading

dcrankshaw Nov 7, 2016

atumanov Nov 11, 2016

atumanov Nov 11, 2016

dcrankshaw Nov 7, 2016

giulio-zhou Nov 8, 2016

dcrankshaw Nov 7, 2016

giulio-zhou Nov 8, 2016

dcrankshaw Nov 7, 2016

giulio-zhou Nov 8, 2016

dcrankshaw Nov 7, 2016

giulio-zhou Nov 8, 2016

dcrankshaw Nov 7, 2016

giulio-zhou Nov 8, 2016

giulio-zhou commented Nov 8, 2016

dcrankshaw commented Nov 10, 2016

giulio-zhou commented Nov 10, 2016

dcrankshaw commented Nov 10, 2016

giulio-zhou commented Nov 10, 2016

dcrankshaw commented Nov 11, 2016

giulio-zhou commented Nov 11, 2016

dcrankshaw commented Nov 11, 2016

giulio-zhou commented Nov 11, 2016

dcrankshaw commented Nov 11, 2016

giulio-zhou commented Nov 11, 2016

dcrankshaw commented Nov 11, 2016

Rest API #3

Rest API #3

Conversation

giulio-zhou commented Nov 7, 2016

dcrankshaw left a comment • edited Loading

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

giulio-zhou commented Nov 8, 2016

dcrankshaw commented Nov 10, 2016

giulio-zhou commented Nov 10, 2016

dcrankshaw commented Nov 10, 2016

giulio-zhou commented Nov 10, 2016

dcrankshaw commented Nov 11, 2016

giulio-zhou commented Nov 11, 2016

dcrankshaw commented Nov 11, 2016

giulio-zhou commented Nov 11, 2016

dcrankshaw commented Nov 11, 2016

giulio-zhou commented Nov 11, 2016

dcrankshaw commented Nov 11, 2016

dcrankshaw left a comment •

edited

Loading