-
Notifications
You must be signed in to change notification settings - Fork 279
Batch predict #321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Batch predict #321
Conversation
Merging with develop branch after batch input testing
|
Can one of the admins verify this patch? |
|
jenkins ok to test |
|
Test FAILed. |
|
Test FAILed. |
|
Test PASSed. |
dcrankshaw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. There are a couple things that you should add:
- Add an integration test that uses the batch prediction interface for end-to-end queries. Just add another test like this one that sends batch predictions and checks to make sure the right number of predictions are returned.
- Add an example of querying Clipper using the batch prediction interface to the examples directory and add a subsection to the Clipper documentation here that explains the batch prediction interface.
| */ | ||
| * Returns the value corresponding to `key` in `config` as a long | ||
| */ | ||
| long get_long(const std::string &key, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert these changes (to keep the commit history clean).
| */ | ||
| * @return `true` if the received heartbeat is a request for container | ||
| * metadata. `false` otherwise. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert
src/frontends/src/query_frontend.hpp
Outdated
| final_content += content + "\n"; | ||
| } catch (const std::exception& e) { | ||
| // case: returned a response before all predictions in the | ||
| // batch were ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log this error
src/frontends/src/query_frontend.hpp
Outdated
| predictions | ||
| .then([response, | ||
| app_metrics](std::vector<folly::Try<Response>> tries) { | ||
| std::string final_content; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a std::stringstream to accumulate the full response.
src/frontends/src/query_frontend.hpp
Outdated
| app_metrics.throughput_->mark(1); | ||
|
|
||
| std::string content = get_prediction_response_content(r); | ||
| final_content += content + "\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will turn into a stringstream appends (e.g. final_content << content << "\n";)
src/frontends/src/query_frontend.hpp
Outdated
| auto prediction = query_processor_.predict( | ||
| Query{name, uid, input, latency_slo_micros, policy, models}); | ||
| predictions.push_back(std::move(prediction)); | ||
| } else { // d.HasMember("input_batch") instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to explicitly check for d.hasMember("input_batch") here, rather than leaving it as an else statement?
|
|
||
| /** | ||
| * Issues a command to Redis and checks return code. | ||
| * \return Returns true if the command was successful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert all these whitespace changes
| * \param `msg`: A vector of individual messages to send to this container. | ||
| * The messages will be sent as a single, multi-part ZeroMQ message so | ||
| * it is very efficient. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert
|
Test FAILed. |
merge develop branch
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
…atch_predict pulling new formatting
|
Test PASSed. |
|
Test FAILed. |
|
Test PASSed. |
|
Test FAILed. |
dcrankshaw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Test PASSed. |
Added support for input of type:
{ "input_batch" := [[double] | [int] | [byte] | [float] | string] }Source code changes:
query_frontend.hppjson_util.cpp/hppquery_frontend_tests.cppAdded testing for:
[[double] | [int] | [float] | string]Questions/Still to-do:
[[byte]]. There's no testing for type[byte]either, I think becauseEXPECT_EQ()doesn't take binary inputs