C++ Model Container #249

Corey-Zumar · 2017-07-10T04:58:38Z

No description provided.

AmplabJenkins · 2017-07-10T07:20:01Z

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

dcrankshaw · 2017-07-10T15:00:13Z

Looks great! Let me know when it's ready for review.

AmplabJenkins · 2017-07-10T18:40:05Z

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

AmplabJenkins · 2017-07-10T20:37:31Z

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

…into rcontainer

Corey-Zumar · 2017-07-10T22:53:54Z

@dcrankshaw This is ready for review.

AmplabJenkins · 2017-07-20T04:26:06Z

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

…into rcontainer

AmplabJenkins · 2017-07-20T21:25: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/583/
Test FAILed.

Corey-Zumar

@dcrankshaw Addressed your comments

Corey-Zumar · 2017-07-20T20:42:29Z

containers/container_implementation.md

@@ -170,7 +170,7 @@ RPC requests sent from Clipper to model containers are divided into two categori
 ### Serializing Prediction Requests
 1. All requests begin with a 32-bit integer header, sent as a single ZeroMQ message. The value of this integer will be 0, indicating that the request is a prediction request.

-2. The second ZeroMQ message contains the size of the input header, in bytes, as a 64-bit long. This is the size of the content of the third ZeroMQ message.
+2. The second ZeroMQ message contains the size of the input header, in bytes, as a 32-bit integer. This is the size of the content of the third ZeroMQ message.


I didn't realize that a C++ long is equivalent to a uint32_t, so the correct input header input type has always been 32-bit integer and the docs need this correction.

Corey-Zumar · 2017-07-20T20:42:43Z

src/container/include/container/container_parsing.hpp

+  }
+
+  const std::vector<Input<D>> get_string_inputs(
+      const std::vector<int>& /*input_header*/, long input_content_size) {


Exactly. See the bottom of the "Serializing Prediction Requests" section of container_implementation.md

Corey-Zumar · 2017-07-20T20:44:38Z

src/libclipper/include/clipper/datatypes.hpp

@@ -17,6 +17,7 @@ using QueryId = long;
 using FeedbackAck = bool;

 enum class InputType {
+  Invalid = -1,


I added the Invalid type to for the purpose of defining the input_trait struct in container_rpc.hpp. This trait allows us to infer the InputType enum categorization for the based on the type of Input<D> that a user's model is parameterized with (ByteVector, DoubleVector).

Corey-Zumar · 2017-07-20T20:55:48Z

src/container/include/container/container_rpc.hpp

+        [this, clipper_address, &model, model_name, model_version]() {
+          serve_model(model, model_name, model_version, clipper_address);
+        });
+    serving_thread_.detach();


Corey-Zumar · 2017-07-20T20:58:28Z

src/container/include/container/container_rpc.hpp

+  template <typename D>
+  void serve_model(Model<Input<D>>& model, std::string model_name,
+                   int model_version, std::string clipper_address) {
+    zmq::context_t context(1);


The context is responsible for initializing and providing IO threads used by ZMQ socket s(See http://api.zeromq.org/2-1:zmq-cpp # Context).

The 1 indicates that our context has a single IO thread. I've added documentation to this effect.

Corey-Zumar · 2017-07-20T21:04:02Z

src/container/include/container/container_parsing.hpp

+    int num_splits = input_header[1] - 1;
+    std::vector<Input<D>> inputs;
+    int prev_split = 0;
+    // Iterate from the beginning of the input data content


Good catch. This should say "input header content" instead of "input data content". Fixed.

Corey-Zumar · 2017-07-20T21:05:02Z

src/container/include/container/container_parsing.hpp

+    int prev_split = 0;
+    // Iterate from the beginning of the input data content
+    // (the first two elements of the header are metadata)
+    for (int i = 2; i < num_splits + 2; i++) {


Corey-Zumar · 2017-07-20T21:13:57Z

src/container/include/container/container_rpc.hpp

+                // Do nothing for now
+                break;
+
+              default: break;


Added an exception

Corey-Zumar · 2017-07-20T21:14:04Z

src/container/include/container/container_rpc.hpp

+            log_error_formatted(
+                LOGGING_TAG_CONTAINER,
+                "Received erroneous new container message from Clipper!");
+          default: break;


Done - added an exception

Corey-Zumar · 2017-07-20T21:16:52Z

src/container/include/container/container_rpc.hpp

+                break;
+
+              case RequestType::FeedbackRequest:
+                // Do nothing for now


Added an exception

AmplabJenkins · 2017-07-20T21:35:15Z

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

AmplabJenkins · 2017-07-20T21:40:19Z

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

Corey-Zumar · 2017-07-20T21:47:17Z

@dcrankshaw Build is failing due to code format violations that the format_code script isn't catching on my machine. Can you try formatting when you get a chance?

AmplabJenkins · 2017-07-20T21:50:15Z

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

AmplabJenkins · 2017-07-21T05:03:45Z

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

dcrankshaw

This looks good to me. I formatted the code so as long as the tests pass I'll merge this.

dcrankshaw · 2017-07-21T05:04:44Z

jenkins test this please

AmplabJenkins · 2017-07-21T05:41:33Z

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

Corey-Zumar added 11 commits June 30, 2017 17:34

Initial commits

e2ad225

More impl

2e061a7

Continue RPC implementation

b0143d0

Completed container implementation - needs logging and testing

43ef8ac

Docs, revert datatypes change

c57d572

Event logging

002d5fd

Completed container. Ready for testing

39e835d

Fixes

9175b13

Fixes - now functioning correctly. Minimal tests.

f41280d

Add more tests, update test scripts

1ed595c

Format code

994b344

Corey-Zumar added status: in progress type: enhancement labels Jul 10, 2017

Fix rpc container test script

bdf56f8

Corey-Zumar added 4 commits July 10, 2017 12:59

Add performance logging

8db072f

format code

e453990

Update container_implementation.md

58c7c13

Add doc

ba4c753

Corey-Zumar added 2 commits July 10, 2017 15:52

fix test

2834f24

Merge branch 'rcontainer' of https://github.com/Corey-Zumar/clipper-1 …

b798477

…into rcontainer

Corey-Zumar added status: needs review and removed status: in progress labels Jul 10, 2017

Corey-Zumar requested review from dcrankshaw and nishadsingh1 July 10, 2017 22:53

Merge branch 'develop' into rcontainer

c270279

dcrankshaw removed the request for review from nishadsingh1 July 20, 2017 03:45

dcrankshaw added status: needs revision and removed status: needs review labels Jul 20, 2017

dcrankshaw assigned Corey-Zumar Jul 20, 2017

Merge branch 'develop' into rcontainer

c5554f1

Corey-Zumar added 3 commits July 20, 2017 14:21

Address review comments

a466ff4

Format code

5c0129e

Merge branch 'rcontainer' of https://github.com/Corey-Zumar/clipper-1 …

1cb14ff

…into rcontainer

Corey-Zumar added 2 commits July 20, 2017 14:28

Add output length verification

a236291

Format code

f39b47f

Corey-Zumar commented Jul 20, 2017

View reviewed changes

Format code again

961b552

Corey-Zumar added status: needs review and removed status: needs revision labels Jul 20, 2017

Corey-Zumar added 2 commits July 20, 2017 14:43

Handle all enum cases in rpc bench

338ed11

Format

27410be

format code

6b433a8

dcrankshaw approved these changes Jul 21, 2017

View reviewed changes

dcrankshaw added status: accepted and removed status: needs review labels Jul 21, 2017

dcrankshaw merged commit 9f396e0 into ucbrise:develop Jul 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C++ Model Container #249

C++ Model Container #249

Corey-Zumar commented Jul 10, 2017

AmplabJenkins commented Jul 10, 2017

dcrankshaw commented Jul 10, 2017

AmplabJenkins commented Jul 10, 2017

AmplabJenkins commented Jul 10, 2017

Corey-Zumar commented Jul 10, 2017

AmplabJenkins commented Jul 20, 2017

AmplabJenkins commented Jul 20, 2017

Corey-Zumar left a comment

Corey-Zumar Jul 20, 2017

Corey-Zumar Jul 20, 2017

Corey-Zumar Jul 20, 2017 •

edited

Loading

Corey-Zumar Jul 20, 2017

Corey-Zumar Jul 20, 2017

Corey-Zumar Jul 20, 2017

Corey-Zumar Jul 20, 2017

Corey-Zumar Jul 20, 2017

Corey-Zumar Jul 20, 2017

Corey-Zumar Jul 20, 2017

AmplabJenkins commented Jul 20, 2017

AmplabJenkins commented Jul 20, 2017

Corey-Zumar commented Jul 20, 2017

AmplabJenkins commented Jul 20, 2017

AmplabJenkins commented Jul 21, 2017

dcrankshaw left a comment

dcrankshaw commented Jul 21, 2017

AmplabJenkins commented Jul 21, 2017

C++ Model Container #249

C++ Model Container #249

Conversation

Corey-Zumar commented Jul 10, 2017

AmplabJenkins commented Jul 10, 2017

dcrankshaw commented Jul 10, 2017

AmplabJenkins commented Jul 10, 2017

AmplabJenkins commented Jul 10, 2017

Corey-Zumar commented Jul 10, 2017

AmplabJenkins commented Jul 20, 2017

AmplabJenkins commented Jul 20, 2017

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

Corey-Zumar Jul 20, 2017 • 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

AmplabJenkins commented Jul 20, 2017

AmplabJenkins commented Jul 20, 2017

Corey-Zumar commented Jul 20, 2017

AmplabJenkins commented Jul 20, 2017

AmplabJenkins commented Jul 21, 2017

dcrankshaw left a comment

Choose a reason for hiding this comment

dcrankshaw commented Jul 21, 2017

AmplabJenkins commented Jul 21, 2017

Corey-Zumar Jul 20, 2017 •

edited

Loading