From 4f137aa14d783c9b23d7af1e923081ba404cae58 Mon Sep 17 00:00:00 2001 From: "Sungjun.Kim" Date: Fri, 29 Mar 2019 00:48:35 +0900 Subject: [PATCH] Implement delete_model_links API (#654) * Implement delete_model_links API * related issue : https://github.com/ucbrise/clipper/issues/603 * Delete the link between a model and an app. * Implemented `unlink_model_from_app` Clipper API in clipper_admin. > Description > ----------- > unlink_model_from_app(app_name, model_name) > Parameters > ---------- > app_name : str > The name of the application > model_name : str > The name of the model to link to the application > Raises > ------ > :py:exc:`clipper.UnconnectedException` > versions. All replicas for each version of each model will be stopped. * Implemented `delete_model_links` Management-Frontend API. > API path > -------- > POST /admin/delete_model_links > Request schema > -------------- > const std::string DELETE_MODEL_LINKS_JSON_SCHEMA = R"( > { > "app_name" := string, > "model_names" := [string] > } > )"; * Fix build error in redis_test.cpp * Decrease test samples for test_stop_models case --- clipper_admin/clipper_admin/clipper_admin.py | 43 ++++++++++- integration-tests/clipper_admin_tests.py | 19 +++++ src/frontends/src/query_frontend.hpp | 7 ++ src/libclipper/include/clipper/redis.hpp | 8 ++ src/libclipper/src/redis.cpp | 16 ++++ src/libclipper/test/redis_test.cpp | 31 ++++++++ src/management/src/management_frontend.hpp | 81 ++++++++++++++++++++ 7 files changed, 204 insertions(+), 1 deletion(-) diff --git a/clipper_admin/clipper_admin/clipper_admin.py b/clipper_admin/clipper_admin/clipper_admin.py index 3a4291778..7fa98a011 100644 --- a/clipper_admin/clipper_admin/clipper_admin.py +++ b/clipper_admin/clipper_admin/clipper_admin.py @@ -281,6 +281,47 @@ def link_model_to_app(self, app_name, model_name): "Model {model} is now linked to application {app}".format( model=model_name, app=app_name)) + def unlink_model_from_app(self, app_name, model_name): + """ + Prevents the model with `model_name` from being used by the app with `app_name`. + The model and app should both be registered with Clipper and a link should + already exist between them. + + Parameters + ---------- + app_name : str + The name of the application + model_name : str + The name of the model to link to the application + + Raises + ------ + :py:exc:`clipper.UnconnectedException` + :py:exc:`clipper.ClipperException` + """ + + if not self.connected: + raise UnconnectedException() + + url = "http://{host}/admin/delete_model_links".format( + host=self.cm.get_admin_addr()) + req_json = json.dumps({ + "app_name": app_name, + "model_names": [model_name] + }) + headers = {'Content-type': 'application/json'} + r = requests.post(url, headers=headers, data=req_json) + logger.debug(r.text) + if r.status_code != requests.codes.ok: + msg = "Received error status code: {code} and message: {msg}".format( + code=r.status_code, msg=r.text) + logger.error(msg) + raise ClipperException(msg) + else: + logger.info( + "Model {model} is now removed to application {app}".format( + model=model_name, app=app_name)) + def build_and_deploy_model(self, name, version, @@ -859,7 +900,7 @@ def get_linked_models(self, app_name): ------- list Returns a list of the names of models linked to the app. - If no models are linked to the specified app, None is returned. + If no models are linked to the specified app, empty list is returned. Raises ------ diff --git a/integration-tests/clipper_admin_tests.py b/integration-tests/clipper_admin_tests.py index 91333ba15..797e8b06b 100644 --- a/integration-tests/clipper_admin_tests.py +++ b/integration-tests/clipper_admin_tests.py @@ -130,6 +130,25 @@ def test_link_registered_model_to_app_succeeds(self): result = self.clipper_conn.get_linked_models(app_name) self.assertEqual([model_name], result) + def test_unlink_registered_model_from_app_succeeds(self): + # Register app + app_name = "testapp" + input_type = "doubles" + default_output = "DEFAULT" + slo_micros = 30000 + self.clipper_conn.register_application(app_name, input_type, + default_output, slo_micros) + + # Register model + model_name = "m" + self.clipper_conn.register_model(model_name, "v1", input_type) + + self.clipper_conn.link_model_to_app(app_name, model_name) + self.clipper_conn.unlink_model_from_app(app_name, model_name) + + result = self.clipper_conn.get_linked_models(app_name) + self.assertEqual([], result) + def get_app_info_for_registered_app_returns_info_dictionary(self): # Register app app_name = "testapp" diff --git a/src/frontends/src/query_frontend.hpp b/src/frontends/src/query_frontend.hpp index 8341efb03..f6257a453 100644 --- a/src/frontends/src/query_frontend.hpp +++ b/src/frontends/src/query_frontend.hpp @@ -204,6 +204,13 @@ class RequestHandler { auto linked_model_names = clipper::redis::get_linked_models(redis_connection_, app_name); set_linked_models_for_app(app_name, linked_model_names); + + } else if (event_type == "srem") { + clipper::log_info_formatted(LOGGING_TAG_QUERY_FRONTEND, + "Model link removal detected for app: {}", app_name); + auto linked_model_names = + clipper::redis::get_linked_models(redis_connection_, app_name); + set_linked_models_for_app(app_name, linked_model_names); } }); diff --git a/src/libclipper/include/clipper/redis.hpp b/src/libclipper/include/clipper/redis.hpp index 4768c35a2..45e94ee04 100644 --- a/src/libclipper/include/clipper/redis.hpp +++ b/src/libclipper/include/clipper/redis.hpp @@ -281,6 +281,14 @@ bool add_application(redox::Redox& redis, const std::string& app_name, bool add_model_links(redox::Redox& redis, const std::string& app_name, const std::vector& model_names); + /** + * Deletes links between the specified app and models. + * + * \return Returns true if the removal was successful. + */ + bool delete_model_links(redox::Redox& redis, const std::string& app_name, + const std::vector& model_names); + /** * Deletes a container from the container table if it exists. * diff --git a/src/libclipper/src/redis.cpp b/src/libclipper/src/redis.cpp index 8b1c8eab3..a60a10662 100644 --- a/src/libclipper/src/redis.cpp +++ b/src/libclipper/src/redis.cpp @@ -489,6 +489,22 @@ bool add_model_links(redox::Redox& redis, const std::string& appname, } } +bool delete_model_links(redox::Redox& redis, const std::string& appname, + const std::vector& model_names) { + if (send_cmd_no_reply( + redis, {"SELECT", std::to_string(REDIS_APP_MODEL_LINKS_DB_NUM)})) { + for (auto model_name : model_names) { + if (!send_cmd_no_reply( + redis, vector{"SREM", appname, model_name})) { + return false; + } + } + return true; + } else { + return false; + } +} + bool delete_application(redox::Redox& redis, const std::string& appname) { if (send_cmd_no_reply( redis, {"SELECT", std::to_string(REDIS_APPLICATION_DB_NUM)})) { diff --git a/src/libclipper/test/redis_test.cpp b/src/libclipper/test/redis_test.cpp index 7d04c2553..cf292fd56 100644 --- a/src/libclipper/test/redis_test.cpp +++ b/src/libclipper/test/redis_test.cpp @@ -134,6 +134,37 @@ TEST_F(RedisTest, AddModelLinks) { ASSERT_EQ(model_names, linked_models); } +TEST_F(RedisTest, DeleteModelLinks) { + std::string app_name = "my_app_name"; + InputType input_type = InputType::Doubles; + std::string policy = DefaultOutputSelectionPolicy::get_name(); + std::string default_output = "1.0"; + int latency_slo_micros = 10000; + ASSERT_TRUE(add_application(*redis_, app_name, input_type, policy, + default_output, latency_slo_micros)); + + std::vector labels{"ads", "images", "experimental", "other", + "labels"}; + std::string model_name_1 = "model_1"; + std::string model_name_2 = "model_2"; + VersionedModelId model_1 = VersionedModelId(model_name_1, "1"); + VersionedModelId model_2 = VersionedModelId(model_name_2, "1"); + std::string container_name = "clipper/test_container"; + std::string model_path = "/tmp/models/m/1"; + ASSERT_TRUE(add_model(*redis_, model_1, input_type, labels, container_name, + model_path, DEFAULT_BATCH_SIZE)); + ASSERT_TRUE(add_model(*redis_, model_2, input_type, labels, container_name, + model_path, DEFAULT_BATCH_SIZE)); + + std::vector model_names = + std::vector{model_name_1, model_name_2}; + ASSERT_TRUE(add_model_links(*redis_, app_name, model_names)); + ASSERT_TRUE(delete_model_links(*redis_, app_name, model_names)); + + auto linked_models = get_linked_models(*redis_, app_name); + ASSERT_EQ(linked_models.size(), (size_t)0); +} + TEST_F(RedisTest, SetCurrentModelVersion) { std::string model_name = "mymodel"; ASSERT_TRUE(set_current_model_version(*redis_, model_name, "2")); diff --git a/src/management/src/management_frontend.hpp b/src/management/src/management_frontend.hpp index 2376a5ffb..8ed6fb63a 100644 --- a/src/management/src/management_frontend.hpp +++ b/src/management/src/management_frontend.hpp @@ -65,6 +65,7 @@ const std::string GET_ALL_MODELS = ADMIN_PATH + "/get_all_models$"; const std::string GET_MODEL = ADMIN_PATH + "/get_model$"; const std::string GET_ALL_CONTAINERS = ADMIN_PATH + "/get_all_containers$"; const std::string GET_CONTAINER = ADMIN_PATH + "/get_container$"; +const std::string DELETE_MODEL_LINKS = ADMIN_PATH + "/delete_model_links$"; const std::string PING = ADMIN_PATH + "/ping$"; @@ -84,6 +85,13 @@ const std::string ADD_MODEL_LINKS_JSON_SCHEMA = R"( } )"; +const std::string DELETE_MODEL_LINKS_JSON_SCHEMA = R"( + { + "app_name" := string, + "model_names" := [string] + } +)"; + const std::string GET_LINKED_MODELS_REQUESTS_SCHEMA = R"( { "app_name" := string @@ -241,6 +249,27 @@ class RequestHandler { respond_http(e.what(), "400 Bad Request", response); } }); + server_.add_endpoint( + DELETE_MODEL_LINKS, "POST", + [this](std::shared_ptr response, + std::shared_ptr request) { + try { + clipper::log_info(LOGGING_TAG_MANAGEMENT_FRONTEND, + "Remove application links POST request"); + std::string result = delete_model_links(request->content.string()); + respond_http(result, "200 OK", response); + } catch (const json_parse_error& e) { + std::string err_msg = + json_error_msg(e.what(), DELETE_MODEL_LINKS_JSON_SCHEMA); + respond_http(err_msg, "400 Bad Request", response); + } catch (const json_semantic_error& e) { + std::string err_msg = + json_error_msg(e.what(), DELETE_MODEL_LINKS_JSON_SCHEMA); + respond_http(err_msg, "400 Bad Request", response); + } catch (const std::invalid_argument& e) { + respond_http(e.what(), "400 Bad Request", response); + } + }); server_.add_endpoint( ADD_MODEL, "POST", [this](std::shared_ptr response, @@ -620,6 +649,58 @@ class RequestHandler { } } + /** + * Creates an endpoint that listens for requests to remove links between + * apps and models + * + * JSON format: + * { + * "app_name" := string, + * "model_names" := [string] + * } + */ + std::string delete_model_links(const std::string& json) { + rapidjson::Document d; + parse_json(json, d); + + std::string app_name = get_string(d, "app_name"); + std::vector model_names = get_string_array(d, "model_names"); + + // Confirm that the app exists + auto app_info = + clipper::redis::get_application(redis_connection_, app_name); + if (app_info.size() == 0) { + std::stringstream ss; + ss << "No app with name " << app_name << " exists."; + throw std::invalid_argument(ss.str()); + } + + // Confirm that the model names supplied are of linked models + auto existing_linked_models = + clipper::redis::get_linked_models(redis_connection_, app_name); + + for (auto const& model_name : model_names) { + if (std::find(existing_linked_models.begin(), + existing_linked_models.end(), + model_name) == existing_linked_models.end()) { + std::stringstream ss; + ss << "Cannot remove nonexistent link between app " << app_name + << " and model " << model_name; + throw std::invalid_argument(ss.str()); + } + } + + if (clipper::redis::delete_model_links(redis_connection_, app_name, + model_names)) { + return "Success!"; + } else { + std::stringstream ss; + ss << "Error deleting linked models from " << app_name << " in Redis"; + throw std::invalid_argument(ss.str()); + } + } + + /** * Processes a request to add a new application to Clipper *