From 7963e775ca5e57a236925fad98c8055245e183be Mon Sep 17 00:00:00 2001 From: Colin Versteeg Date: Sat, 12 Oct 2019 11:07:49 -0700 Subject: [PATCH] Cherry pick 2 OnnxRuntime Server fixes --- onnxruntime/server/http/core/session.cc | 5 --- .../server/http/predict_request_handler.cc | 5 ++- onnxruntime/server/main.cc | 9 +++- .../integration_tests/function_tests.py | 44 +++++++++++++++++++ .../server/unit_tests/http_routes_tests.cc | 34 +++++++++----- 5 files changed, 78 insertions(+), 19 deletions(-) diff --git a/onnxruntime/server/http/core/session.cc b/onnxruntime/server/http/core/session.cc index 1d415a9f90..f197f4cf0b 100644 --- a/onnxruntime/server/http/core/session.cc +++ b/onnxruntime/server/http/core/session.cc @@ -121,11 +121,6 @@ http::status HttpSession::ExecuteUserFunction(HttpContext& context) { context.client_request_id = context.request[util::MS_CLIENT_REQUEST_ID_HEADER].to_string(); } - if (path == "/score") { - // This is a shortcut since we have only one model instance currently. - // This code path will be removed once we start supporting multiple models or multiple versions of one model. - path = "/v1/models/default/versions/1:predict"; - } auto status = routes_.ParseUrl(context.request.method(), path, model_name, model_version, action, func); diff --git a/onnxruntime/server/http/predict_request_handler.cc b/onnxruntime/server/http/predict_request_handler.cc index 10c5b6280e..e6d59616c5 100644 --- a/onnxruntime/server/http/predict_request_handler.cc +++ b/onnxruntime/server/http/predict_request_handler.cc @@ -39,6 +39,9 @@ void Predict(const std::string& name, auto logger = env->GetLogger(context.request_id); logger->info("Model Name: {}, Version: {}, Action: {}", name, version, action); + auto effective_name = name.empty() ? "default" : name; + auto effective_version = version.empty() ? "1" : version; + if (!context.client_request_id.empty()) { logger->info("{}: [{}]", util::MS_CLIENT_REQUEST_ID_HEADER, context.client_request_id); } @@ -64,7 +67,7 @@ void Predict(const std::string& name, // Run Prediction Executor executor(env.get(), context.request_id); PredictResponse predict_response{}; - auto status = executor.Predict(name, version, predict_request, predict_response); + auto status = executor.Predict(effective_name, effective_version, predict_request, predict_response); if (!status.ok()) { GenerateErrorResponse(logger, GetHttpStatusCode((status)), status.error_message(), context); return; diff --git a/onnxruntime/server/main.cc b/onnxruntime/server/main.cc index 724b563689..27709e0d15 100644 --- a/onnxruntime/server/main.cc +++ b/onnxruntime/server/main.cc @@ -104,11 +104,18 @@ int main(int argc, char* argv[]) { }); app.RegisterPost( - R"(/v1/models/([^/:]+)(?:/versions/(\d+))?:(classify|regress|predict))", + R"(/(?:v1/models/([^/:]+)(?:/versions/(\d+))?:(classify|regress|predict))|(?:score()()()))", [&env](const auto& name, const auto& version, const auto& action, auto& context) -> void { server::Predict(name, version, action, context, env); }); + app.RegisterPost( + R"(/score()()())", + [&env](const auto& name, const auto& version, const auto& action, auto& context) -> void { + server::Predict(name, version, action, context, env); + } + ); + app.Bind(boost_address, config.http_port) .NumThreads(config.num_http_threads) .Run(); diff --git a/onnxruntime/test/server/integration_tests/function_tests.py b/onnxruntime/test/server/integration_tests/function_tests.py index e91a0a26df..c8595dc300 100644 --- a/onnxruntime/test/server/integration_tests/function_tests.py +++ b/onnxruntime/test/server/integration_tests/function_tests.py @@ -192,6 +192,50 @@ class HttpJsonPayloadTests(unittest.TestCase): for i in range(0, 10): self.assertTrue(test_util.compare_floats(actual_data[i], expected_data[i])) + def test_single_version_shortcut(self): + input_data_file = os.path.join(self.test_data_path, 'mnist_test_data_set_0_input.json') + output_data_file = os.path.join(self.test_data_path, 'mnist_test_data_set_0_output.json') + + with open(input_data_file, 'r') as f: + request_payload = f.read() + + with open(output_data_file, 'r') as f: + expected_response_json = f.read() + expected_response = json.loads(expected_response_json) + + request_headers = { + 'Content-Type': 'application/json', + 'Accept': 'application/json', + 'x-ms-client-request-id': 'This~is~my~id' + } + + url = "http://{0}:{1}/v1/models/{2}:predict".format(self.server_ip, self.server_port, 'default') + test_util.test_log(url) + r = requests.post(url, headers=request_headers, data=request_payload) + self.assertEqual(r.status_code, 200) + self.assertEqual(r.headers.get('Content-Type'), 'application/json') + self.assertTrue(r.headers.get('x-ms-request-id')) + self.assertEqual(r.headers.get('x-ms-client-request-id'), 'This~is~my~id') + + actual_response = json.loads(r.content.decode('utf-8')) + + # Note: + # The 'dims' field is defined as "repeated int64" in protobuf. + # When it is serialized to JSON, all int64/fixed64/uint64 numbers are converted to string + # Reference: https://developers.google.com/protocol-buffers/docs/proto3#json + + self.assertTrue(actual_response['outputs']) + self.assertTrue(actual_response['outputs']['Plus214_Output_0']) + self.assertTrue(actual_response['outputs']['Plus214_Output_0']['dims']) + self.assertEqual(actual_response['outputs']['Plus214_Output_0']['dims'], ['1', '10']) + self.assertTrue(actual_response['outputs']['Plus214_Output_0']['dataType']) + self.assertEqual(actual_response['outputs']['Plus214_Output_0']['dataType'], 1) + self.assertTrue(actual_response['outputs']['Plus214_Output_0']['rawData']) + actual_data = test_util.decode_base64_string(actual_response['outputs']['Plus214_Output_0']['rawData'], '10f') + expected_data = test_util.decode_base64_string(expected_response['outputs']['Plus214_Output_0']['rawData'], '10f') + + for i in range(0, 10): + self.assertTrue(test_util.compare_floats(actual_data[i], expected_data[i])) class HttpProtobufPayloadTests(unittest.TestCase): server_ip = '127.0.0.1' diff --git a/onnxruntime/test/server/unit_tests/http_routes_tests.cc b/onnxruntime/test/server/unit_tests/http_routes_tests.cc index dbb9550501..842eabb071 100644 --- a/onnxruntime/test/server/unit_tests/http_routes_tests.cc +++ b/onnxruntime/test/server/unit_tests/http_routes_tests.cc @@ -10,6 +10,7 @@ namespace onnxruntime { namespace server { namespace test { +static const std::string predict_regex = R"(/(?:v1/models/([^/:]+)(?:/versions/(\d+))?:(classify|regress|predict)))"; using test_data = std::tuple; void do_something(const std::string& name, const std::string& version, @@ -20,7 +21,6 @@ void do_something(const std::string& name, const std::string& version, void run_route(const std::string& pattern, http::verb method, const std::vector& data, bool does_validate_data); TEST(HttpRouteTests, RegisterTest) { - auto predict_regex = R"(/v1/models/([^/:]+)(?:/versions/(\d+))?:(classify|regress|predict))"; Routes routes; EXPECT_TRUE(routes.RegisterController(http::verb::post, predict_regex, do_something)); @@ -29,20 +29,22 @@ TEST(HttpRouteTests, RegisterTest) { } TEST(HttpRouteTests, PostRouteTest) { - auto predict_regex = R"(/v1/models/([^/:]+)(?:/versions/(\d+))?:(classify|regress|predict))"; std::vector actions{ std::make_tuple(http::verb::post, "/v1/models/abc/versions/23:predict", "abc", "23", "predict", http::status::ok), std::make_tuple(http::verb::post, "/v1/models/abc:predict", "abc", "", "predict", http::status::ok), std::make_tuple(http::verb::post, "/v1/models/models/versions/45:predict", "models", "45", "predict", http::status::ok), std::make_tuple(http::verb::post, "/v1/models/??$$%%@@$^^/versions/45:predict", "??$$%%@@$^^", "45", "predict", http::status::ok), - std::make_tuple(http::verb::post, "/v1/models/versions/versions/45:predict", "versions", "45", "predict", http::status::ok)}; + std::make_tuple(http::verb::post, "/v1/models/versions/versions/45:predict", "versions", "45", "predict", http::status::ok), + std::make_tuple(http::verb::post, "/v1/models/versions:predict", "versions", "", "predict", http::status::ok), + std::make_tuple(http::verb::post, "/v1/models/default:predict", "default", "", "predict", http::status::ok) + }; + run_route(predict_regex, http::verb::post, actions, true); } TEST(HttpRouteTests, PostRouteInvalidURLTest) { - auto predict_regex = R"(/v1/models/([^/:]+)(?:/versions/(\d+))?:(classify|regress|predict))"; std::vector actions{ std::make_tuple(http::verb::post, "", "", "", "", http::status::not_found), @@ -56,9 +58,9 @@ TEST(HttpRouteTests, PostRouteInvalidURLTest) { std::make_tuple(http::verb::post, "/models/abc/versions/2:predict", "", "", "", http::status::not_found), std::make_tuple(http::verb::post, "/v1/models/versions/2:predict", "", "", "", http::status::not_found), std::make_tuple(http::verb::post, "/v1/models/foo/versions/:predict", "", "", "", http::status::not_found), - std::make_tuple(http::verb::post, "/v1/models/foo/versions:predict", "", "", "", http::status::not_found), std::make_tuple(http::verb::post, "v1/models/foo/versions/12:predict", "", "", "", http::status::not_found), - std::make_tuple(http::verb::post, "/v1/models/abc/versions/23:foo", "", "", "", http::status::not_found)}; + std::make_tuple(http::verb::post, "/v1/models/abc/versions/23:foo", "", "", "", http::status::not_found) + }; run_route(predict_regex, http::verb::post, actions, false); } @@ -66,17 +68,25 @@ TEST(HttpRouteTests, PostRouteInvalidURLTest) { // These tests are because we currently only support POST and GET // Some HTTP methods should be removed from test data if we support more (e.g. PUT) TEST(HttpRouteTests, PostRouteInvalidMethodTest) { - auto predict_regex = R"(/v1/models/([^/:]+)(?:/versions/(\d+))?:(classify|regress|predict))"; std::vector actions{ std::make_tuple(http::verb::get, "/v1/models/abc/versions/23:predict", "abc", "23", "predict", http::status::method_not_allowed), std::make_tuple(http::verb::put, "/v1/models", "", "", "", http::status::method_not_allowed), std::make_tuple(http::verb::delete_, "/v1/models", "", "", "", http::status::method_not_allowed), - std::make_tuple(http::verb::head, "/v1/models", "", "", "", http::status::method_not_allowed)}; + std::make_tuple(http::verb::head, "/v1/models", "", "", "", http::status::method_not_allowed) + }; run_route(predict_regex, http::verb::post, actions, false); } +TEST(HttpRouteTests, PostRouteSpecialMethodTest){ + std::vector actions{ + std::make_tuple(http::verb::post, "/score", "", "", "", http::status::ok) + }; + + run_route(R"(/score()()())", http::verb::post, actions, true); +} + void run_route(const std::string& pattern, http::verb method, const std::vector& data, bool does_validate_data) { Routes routes; EXPECT_TRUE(routes.RegisterController(method, pattern, do_something)); @@ -95,11 +105,11 @@ void run_route(const std::string& pattern, http::verb method, const std::vector< http::status expected_status; std::tie(test_method, url_string, expected_name, expected_version, expected_action, expected_status) = i; - EXPECT_EQ(expected_status, routes.ParseUrl(test_method, url_string, name, version, action, fn)); + EXPECT_EQ(expected_status, routes.ParseUrl(test_method, url_string, name, version, action, fn)) << "On route " << url_string; if (does_validate_data) { - EXPECT_EQ(name, expected_name); - EXPECT_EQ(version, expected_version); - EXPECT_EQ(action, expected_action); + EXPECT_EQ(name, expected_name) << "On route " << url_string; + EXPECT_EQ(version, expected_version) << "On route " << url_string; + EXPECT_EQ(action, expected_action) << "On route " << url_string; } } }