-
Notifications
You must be signed in to change notification settings - Fork 280
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
[CLIPPER-107][CLIPPER-109] Return JSON-formatted responses to prediction queries #116
Conversation
Test FAILed. |
Test PASSed. |
Test PASSed. |
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.
const char* PREDICTION_RESPONSE_KEY_OUTPUT = "output"; | ||
const char* PREDICTION_RESPONSE_KEY_USED_DEFAULT = "default"; | ||
const char* PREDICTION_ERROR_RESPONSE_KEY_ERROR = "error"; | ||
const char* PREDICTION_ERROR_RESPONSE_KEY_CAUSE = "cause"; |
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.
Why are these char*
instead of std::string
? (I'm guessing there's a good reason)
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.
Our utility methods for adding key-value pairs to JSON objects expect char*
keys. There is no implicit cast from std::string
to char*
, so I've defined the constants this way. Alternatively, I could define these with the std::string
type and call .data()
to get char* pointers, but this seems less readable.
@@ -6,6 +6,7 @@ | |||
#include <tuple> | |||
#include <utility> |
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.
Add #include <stdexcept>
(it's better to explicitly include all the headers needed)
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.
Included stdexept
in exceptions.hpp
const long query_id_; | ||
const std::string msg_; | ||
}; | ||
|
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.
Let's define this in a separate exceptions.hpp
file.
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.
Done
ss << "Explanation: " << msg_ << std::endl; | ||
return ss.str().data(); | ||
} | ||
|
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.
Move to exceptions.cpp
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.
Done
@@ -52,7 +52,7 @@ const std::string APPLICATION_JSON_SCHEMA = R"( | |||
{"model_name" := string, "model_version" := int} | |||
], | |||
"input_type" := "integers" | "bytes" | "floats" | "doubles" | "strings", | |||
"selection_policy" := string, | |||
"default_output" := number, |
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.
s/number/float
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.
Done
Test FAILed. |
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.
@dcrankshaw Addressed your comments - moved PredictError
to separate exceptions.hpp
and exceptions.cpp
files.
@@ -52,7 +52,7 @@ const std::string APPLICATION_JSON_SCHEMA = R"( | |||
{"model_name" := string, "model_version" := int} | |||
], | |||
"input_type" := "integers" | "bytes" | "floats" | "doubles" | "strings", | |||
"selection_policy" := string, | |||
"default_output" := number, |
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.
Done
Test PASSed. |
Test PASSed. |
UNREACHABLE(); | ||
} | ||
std::pair<Output, bool> final_output = | ||
current_policy->combine_predictions(selection_state, query, outputs); | ||
std::chrono::time_point<std::chrono::high_resolution_clock> end = |
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.
@Corey-Zumar Can you add a ratio counter here that tracks the fraction of predictions that use the default output?
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.
Done
605f063
to
4546912
Compare
Test PASSed. |
Test PASSed. |
@dcrankshaw Rebased on |
Test FAILed. |
Test FAILed. |
Test PASSed. |
Test PASSed. |
LGTM. Confirming that the tutorial works for me as well. |
Modifies query frontend and dependent infrastructure to return JSON-formatted responses to prediction queries.
From CLIPPER-107 and the functional spec:
Note: This PR is currently based on the branch corresponding to #89. As a result, many changes from #89 are redundantly included here.