From 467960b296614ec5726a0467ff139ab4123cd2aa Mon Sep 17 00:00:00 2001 From: Ed Oakes Date: Mon, 28 Feb 2022 14:11:03 -0600 Subject: [PATCH 01/10] fix --- dashboard/modules/serve/tests/test_schema.py | 7 +++++++ dashboard/modules/serve/tests/test_serve_head.py | 13 ++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/dashboard/modules/serve/tests/test_schema.py b/dashboard/modules/serve/tests/test_schema.py index ab1d6aec80ed..b72d35f3eb96 100644 --- a/dashboard/modules/serve/tests/test_schema.py +++ b/dashboard/modules/serve/tests/test_schema.py @@ -1,4 +1,7 @@ +import sys + from pydantic import ValidationError + import pytest import requests @@ -489,3 +492,7 @@ def f2(): assert len(deployment_names) == 0 serve.shutdown() + + +if __name__ == "__main__": + sys.exit(pytest.main(["-v", __file__])) diff --git a/dashboard/modules/serve/tests/test_serve_head.py b/dashboard/modules/serve/tests/test_serve_head.py index eed91ca4003b..74a635403605 100644 --- a/dashboard/modules/serve/tests/test_serve_head.py +++ b/dashboard/modules/serve/tests/test_serve_head.py @@ -1,9 +1,12 @@ -import pytest -import subprocess -import requests import json +import subprocess +import sys from typing import List, Dict +import pytest + +import requests + GET_OR_PUT_URL = "http://localhost:8265/api/serve/deployments/" STATUS_URL = "http://localhost:8265/api/serve/deployments/status" @@ -181,3 +184,7 @@ def test_get_status_info(serve_start_stop): assert len(expected_deployment_names) == 0 print(statuses) + + +if __name__ == "__main__": + sys.exit(pytest.main(["-v", __file__])) From 53f9a78bb16279609375e6f027af3516e0400030 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Mon, 28 Feb 2022 13:27:02 -0800 Subject: [PATCH 02/10] Use dashboard namespace --- dashboard/modules/serve/tests/test_serve_head.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dashboard/modules/serve/tests/test_serve_head.py b/dashboard/modules/serve/tests/test_serve_head.py index 74a635403605..caea8075a068 100644 --- a/dashboard/modules/serve/tests/test_serve_head.py +++ b/dashboard/modules/serve/tests/test_serve_head.py @@ -7,6 +7,8 @@ import requests +from ray.dashboard.optional_utils import RAY_INTERNAL_DASHBOARD_NAMESPACE + GET_OR_PUT_URL = "http://localhost:8265/api/serve/deployments/" STATUS_URL = "http://localhost:8265/api/serve/deployments/status" @@ -17,7 +19,7 @@ @pytest.fixture def serve_start_stop(): subprocess.check_output(["ray", "start", "--head"]) - subprocess.check_output(["serve", "start"]) + subprocess.check_output(["serve", "-n", RAY_INTERNAL_DASHBOARD_NAMESPACE, "start"]) yield subprocess.check_output(["ray", "stop", "--force"]) From a25fd8b08c98eeb4480a228c1a33dce0670b4c88 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Mon, 28 Feb 2022 14:50:53 -0800 Subject: [PATCH 03/10] Make test_serve_head a large test --- dashboard/BUILD | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/dashboard/BUILD b/dashboard/BUILD index 467dd091a682..87e76ca30fab 100644 --- a/dashboard/BUILD +++ b/dashboard/BUILD @@ -13,7 +13,7 @@ py_library( py_test_run_all_subdirectory( size = "medium", include = ["**/test*.py"], - exclude = ["modules/test/**", "modules/node/tests/test_node.py", "tests/test_dashboard.py"], + exclude = ["modules/test/**", "modules/node/tests/test_node.py", "tests/test_dashboard.py", "modules/serve/tests/test_serve_head.py"], extra_srcs = [], tags = ["exclusive", "team:serve"], ) @@ -31,3 +31,10 @@ py_test( srcs = ["tests/test_dashboard.py"], tags = ["exclusive", "team:serve"], ) + +py_test( + name = "test_serve_head", + size = "large", + srcs = ["modules/serve/tests/test_serve_head.py"], + tags = ["exclusive", "team:serve"], +) From e745433c7e55c8ce4771f8ab6af7d6708538b329 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Mon, 28 Feb 2022 14:51:44 -0800 Subject: [PATCH 04/10] Reduce iterations --- dashboard/modules/serve/tests/test_serve_head.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dashboard/modules/serve/tests/test_serve_head.py b/dashboard/modules/serve/tests/test_serve_head.py index caea8075a068..5b2f2eb6adca 100644 --- a/dashboard/modules/serve/tests/test_serve_head.py +++ b/dashboard/modules/serve/tests/test_serve_head.py @@ -70,7 +70,7 @@ def test_put_get_success(serve_start_stop): ) # Ensure the REST API is idempotent - for _ in range(3): + for _ in range(2): deployments = [shallow, deep, one] put_response = requests.put(GET_OR_PUT_URL, json={"deployments": deployments}) @@ -126,7 +126,7 @@ def test_delete_success(serve_start_stop): ) # Ensure the REST API is idempotent - for _ in range(5): + for _ in range(2): put_response = requests.put(GET_OR_PUT_URL, json={"deployments": [shallow]}) assert put_response.status_code == 200 assert ( From 90087ec7dbe1f578356a81dc719dd1a136ce60db Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Mon, 28 Feb 2022 14:56:33 -0800 Subject: [PATCH 05/10] Add timeouts to REST requests --- .../modules/serve/tests/test_serve_head.py | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/dashboard/modules/serve/tests/test_serve_head.py b/dashboard/modules/serve/tests/test_serve_head.py index 5b2f2eb6adca..5ff2bf710d7f 100644 --- a/dashboard/modules/serve/tests/test_serve_head.py +++ b/dashboard/modules/serve/tests/test_serve_head.py @@ -73,7 +73,9 @@ def test_put_get_success(serve_start_stop): for _ in range(2): deployments = [shallow, deep, one] - put_response = requests.put(GET_OR_PUT_URL, json={"deployments": deployments}) + put_response = requests.put( + GET_OR_PUT_URL, json={"deployments": deployments}, timeout=60 + ) assert put_response.status_code == 200 assert ( requests.get("http://localhost:8000/shallow").text == "Hello shallow world!" @@ -81,7 +83,7 @@ def test_put_get_success(serve_start_stop): assert requests.get("http://localhost:8000/deep").text == "Hello deep world!" assert requests.get("http://localhost:8000/one").text == "2" - get_response = requests.get(GET_OR_PUT_URL) + get_response = requests.get(GET_OR_PUT_URL, timeout=60) assert get_response.status_code == 200 with open("three_deployments_response.json", "r") as f: @@ -90,7 +92,9 @@ def test_put_get_success(serve_start_stop): assert deployments_match(response_deployments, expected_deployments) deployments = [shallow, one] - put_response = requests.put(GET_OR_PUT_URL, json={"deployments": deployments}) + put_response = requests.put( + GET_OR_PUT_URL, json={"deployments": deployments}, timeout=60 + ) assert put_response.status_code == 200 assert ( requests.get("http://localhost:8000/shallow").text == "Hello shallow world!" @@ -98,7 +102,7 @@ def test_put_get_success(serve_start_stop): assert requests.get("http://localhost:8000/deep").status_code == 404 assert requests.get("http://localhost:8000/one").text == "2" - get_response = requests.get(GET_OR_PUT_URL) + get_response = requests.get(GET_OR_PUT_URL, timeout=60) assert get_response.status_code == 200 with open("two_deployments_response.json", "r") as f: @@ -127,17 +131,19 @@ def test_delete_success(serve_start_stop): # Ensure the REST API is idempotent for _ in range(2): - put_response = requests.put(GET_OR_PUT_URL, json={"deployments": [shallow]}) + put_response = requests.put( + GET_OR_PUT_URL, json={"deployments": [shallow]}, timeout=60 + ) assert put_response.status_code == 200 assert ( requests.get("http://localhost:8000/shallow").text == "Hello shallow world!" ) - delete_response = requests.delete(GET_OR_PUT_URL) + delete_response = requests.delete(GET_OR_PUT_URL, timeout=60) assert delete_response.status_code == 200 # Make sure no deployments exist - get_response = requests.get(GET_OR_PUT_URL) + get_response = requests.get(GET_OR_PUT_URL, timeout=60) assert len(json.loads(get_response.json())["deployments"]) == 0 @@ -169,10 +175,12 @@ def test_get_status_info(serve_start_stop): deployments = [shallow, deep, one] - put_response = requests.put(GET_OR_PUT_URL, json={"deployments": deployments}) + put_response = requests.put( + GET_OR_PUT_URL, json={"deployments": deployments}, timeout=60 + ) assert put_response.status_code == 200 - status_response = requests.get(STATUS_URL) + status_response = requests.get(STATUS_URL, timeout=60) assert status_response.status_code == 200 statuses = json.loads(status_response.json())["statuses"] From 3ccee4bd04b82925ae4b7e4e122055b1bf78e99f Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Mon, 28 Feb 2022 14:59:46 -0800 Subject: [PATCH 06/10] Remove unncessary serve start --- dashboard/modules/serve/tests/test_serve_head.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/dashboard/modules/serve/tests/test_serve_head.py b/dashboard/modules/serve/tests/test_serve_head.py index 5ff2bf710d7f..08df626189df 100644 --- a/dashboard/modules/serve/tests/test_serve_head.py +++ b/dashboard/modules/serve/tests/test_serve_head.py @@ -7,8 +7,6 @@ import requests -from ray.dashboard.optional_utils import RAY_INTERNAL_DASHBOARD_NAMESPACE - GET_OR_PUT_URL = "http://localhost:8265/api/serve/deployments/" STATUS_URL = "http://localhost:8265/api/serve/deployments/status" @@ -17,9 +15,8 @@ @pytest.fixture -def serve_start_stop(): +def ray_start_stop(): subprocess.check_output(["ray", "start", "--head"]) - subprocess.check_output(["serve", "-n", RAY_INTERNAL_DASHBOARD_NAMESPACE, "start"]) yield subprocess.check_output(["ray", "stop", "--force"]) @@ -43,7 +40,7 @@ def deployments_match(list1: List[Dict], list2: List[Dict]) -> bool: return len(list2) == 0 -def test_put_get_success(serve_start_stop): +def test_put_get_success(ray_start_stop): ray_actor_options = {"runtime_env": {"py_modules": [test_env_uri, test_module_uri]}} shallow = dict( @@ -111,7 +108,7 @@ def test_put_get_success(serve_start_stop): assert deployments_match(response_deployments, expected_deployments) -def test_delete_success(serve_start_stop): +def test_delete_success(ray_start_stop): ray_actor_options = { "runtime_env": { "working_dir": ( @@ -147,7 +144,7 @@ def test_delete_success(serve_start_stop): assert len(json.loads(get_response.json())["deployments"]) == 0 -def test_get_status_info(serve_start_stop): +def test_get_status_info(ray_start_stop): ray_actor_options = {"runtime_env": {"py_modules": [test_env_uri, test_module_uri]}} shallow = dict( From eeb3586ed89b29c1dda4cda10e9ae0c52aba3d31 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Mon, 28 Feb 2022 16:50:59 -0800 Subject: [PATCH 07/10] Add more timeouts --- .../modules/serve/tests/test_serve_head.py | 38 +++++++++++-------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/dashboard/modules/serve/tests/test_serve_head.py b/dashboard/modules/serve/tests/test_serve_head.py index 08df626189df..2c2add29c189 100644 --- a/dashboard/modules/serve/tests/test_serve_head.py +++ b/dashboard/modules/serve/tests/test_serve_head.py @@ -71,16 +71,20 @@ def test_put_get_success(ray_start_stop): deployments = [shallow, deep, one] put_response = requests.put( - GET_OR_PUT_URL, json={"deployments": deployments}, timeout=60 + GET_OR_PUT_URL, json={"deployments": deployments}, timeout=30 ) assert put_response.status_code == 200 assert ( - requests.get("http://localhost:8000/shallow").text == "Hello shallow world!" + requests.get("http://localhost:8000/shallow", timeout=30).text + == "Hello shallow world!" ) - assert requests.get("http://localhost:8000/deep").text == "Hello deep world!" - assert requests.get("http://localhost:8000/one").text == "2" + assert ( + requests.get("http://localhost:8000/deep", timeout=30).text + == "Hello deep world!" + ) + assert requests.get("http://localhost:8000/one", timeout=30).text == "2" - get_response = requests.get(GET_OR_PUT_URL, timeout=60) + get_response = requests.get(GET_OR_PUT_URL, timeout=30) assert get_response.status_code == 200 with open("three_deployments_response.json", "r") as f: @@ -90,16 +94,17 @@ def test_put_get_success(ray_start_stop): deployments = [shallow, one] put_response = requests.put( - GET_OR_PUT_URL, json={"deployments": deployments}, timeout=60 + GET_OR_PUT_URL, json={"deployments": deployments}, timeout=30 ) assert put_response.status_code == 200 assert ( - requests.get("http://localhost:8000/shallow").text == "Hello shallow world!" + requests.get("http://localhost:8000/shallow", timeout=30).text + == "Hello shallow world!" ) - assert requests.get("http://localhost:8000/deep").status_code == 404 - assert requests.get("http://localhost:8000/one").text == "2" + assert requests.get("http://localhost:8000/deep", timeout=30).status_code == 404 + assert requests.get("http://localhost:8000/one", timeout=30).text == "2" - get_response = requests.get(GET_OR_PUT_URL, timeout=60) + get_response = requests.get(GET_OR_PUT_URL, timeout=30) assert get_response.status_code == 200 with open("two_deployments_response.json", "r") as f: @@ -129,18 +134,19 @@ def test_delete_success(ray_start_stop): # Ensure the REST API is idempotent for _ in range(2): put_response = requests.put( - GET_OR_PUT_URL, json={"deployments": [shallow]}, timeout=60 + GET_OR_PUT_URL, json={"deployments": [shallow]}, timeout=30 ) assert put_response.status_code == 200 assert ( - requests.get("http://localhost:8000/shallow").text == "Hello shallow world!" + requests.get("http://localhost:8000/shallow", timeout=30).text + == "Hello shallow world!" ) - delete_response = requests.delete(GET_OR_PUT_URL, timeout=60) + delete_response = requests.delete(GET_OR_PUT_URL, timeout=30) assert delete_response.status_code == 200 # Make sure no deployments exist - get_response = requests.get(GET_OR_PUT_URL, timeout=60) + get_response = requests.get(GET_OR_PUT_URL, timeout=30) assert len(json.loads(get_response.json())["deployments"]) == 0 @@ -173,11 +179,11 @@ def test_get_status_info(ray_start_stop): deployments = [shallow, deep, one] put_response = requests.put( - GET_OR_PUT_URL, json={"deployments": deployments}, timeout=60 + GET_OR_PUT_URL, json={"deployments": deployments}, timeout=30 ) assert put_response.status_code == 200 - status_response = requests.get(STATUS_URL, timeout=60) + status_response = requests.get(STATUS_URL, timeout=30) assert status_response.status_code == 200 statuses = json.loads(status_response.json())["statuses"] From 55e4c4fa34fef5d3f72c659fc502d65e8c3ddf0f Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Mon, 28 Feb 2022 18:18:44 -0800 Subject: [PATCH 08/10] Reduce num_cpus in get_put test --- .../modules/serve/tests/test_serve_head.py | 31 ++++++++++++++----- .../tests/three_deployments_response.json | 6 ++-- .../serve/tests/two_deployments_response.json | 4 +-- 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/dashboard/modules/serve/tests/test_serve_head.py b/dashboard/modules/serve/tests/test_serve_head.py index 2c2add29c189..3bc3b5144ebe 100644 --- a/dashboard/modules/serve/tests/test_serve_head.py +++ b/dashboard/modules/serve/tests/test_serve_head.py @@ -1,7 +1,7 @@ import json import subprocess import sys -from typing import List, Dict +from typing import List, Dict, Set import pytest @@ -21,8 +21,11 @@ def ray_start_stop(): subprocess.check_output(["ray", "stop", "--force"]) -def deployments_match(list1: List[Dict], list2: List[Dict]) -> bool: - # Helper that takes in 2 lists of deployment dictionaries and compares them +def deployments_match(list1: List[Dict], list2: List[Dict], properties: Set) -> bool: + """ + Helper that takes in 2 lists of deployment dictionaries and compares their + properties and ray_actor_options. + """ if len(list1) != len(list2): return False @@ -31,7 +34,10 @@ def deployments_match(list1: List[Dict], list2: List[Dict]) -> bool: matching_deployment = None for i in range(len(list2)): deployment2 = list2[i] - if deployment1 == deployment2: + for property in properties: + if deployment1[property] != deployment2[property]: + break + else: matching_deployment = i if matching_deployment is None: return False @@ -41,7 +47,10 @@ def deployments_match(list1: List[Dict], list2: List[Dict]) -> bool: def test_put_get_success(ray_start_stop): - ray_actor_options = {"runtime_env": {"py_modules": [test_env_uri, test_module_uri]}} + ray_actor_options = { + "runtime_env": {"py_modules": [test_env_uri, test_module_uri]}, + "num_cpus": 0.1, + } shallow = dict( name="shallow", @@ -90,7 +99,11 @@ def test_put_get_success(ray_start_stop): with open("three_deployments_response.json", "r") as f: response_deployments = json.loads(get_response.json())["deployments"] expected_deployments = json.load(f)["deployments"] - assert deployments_match(response_deployments, expected_deployments) + assert deployments_match( + response_deployments, + expected_deployments, + {"name", "import_path", "num_replicas", "route_prefix"}, + ) deployments = [shallow, one] put_response = requests.put( @@ -110,7 +123,11 @@ def test_put_get_success(ray_start_stop): with open("two_deployments_response.json", "r") as f: response_deployments = json.loads(get_response.json())["deployments"] expected_deployments = json.load(f)["deployments"] - assert deployments_match(response_deployments, expected_deployments) + assert deployments_match( + response_deployments, + expected_deployments, + {"name", "import_path", "num_replicas", "route_prefix"}, + ) def test_delete_success(ray_start_stop): diff --git a/dashboard/modules/serve/tests/three_deployments_response.json b/dashboard/modules/serve/tests/three_deployments_response.json index c44030df8720..cb3a3a185383 100644 --- a/dashboard/modules/serve/tests/three_deployments_response.json +++ b/dashboard/modules/serve/tests/three_deployments_response.json @@ -23,7 +23,7 @@ ], "working_dir": null }, - "num_cpus": 1.0, + "num_cpus": 0.1, "num_gpus": 0.0, "memory": 0.0, "object_store_memory": 0.0, @@ -54,7 +54,7 @@ ], "working_dir": null }, - "num_cpus": 1.0, + "num_cpus": 0.1, "num_gpus": 0.0, "memory": 0.0, "object_store_memory": 0.0, @@ -85,7 +85,7 @@ ], "working_dir": null }, - "num_cpus": 1.0, + "num_cpus": 0.1, "num_gpus": 0.0, "memory": 0.0, "object_store_memory": 0.0, diff --git a/dashboard/modules/serve/tests/two_deployments_response.json b/dashboard/modules/serve/tests/two_deployments_response.json index fb81945ec307..49701e79169c 100644 --- a/dashboard/modules/serve/tests/two_deployments_response.json +++ b/dashboard/modules/serve/tests/two_deployments_response.json @@ -23,7 +23,7 @@ ], "working_dir": null }, - "num_cpus": 1.0, + "num_cpus": 0.1, "num_gpus": 0.0, "memory": 0.0, "object_store_memory": 0.0, @@ -54,7 +54,7 @@ ], "working_dir": null }, - "num_cpus": 1.0, + "num_cpus": 0.1, "num_gpus": 0.0, "memory": 0.0, "object_store_memory": 0.0, From d885de9eb51b54b2d1973f8e9e43cbaa78b8959f Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Mon, 28 Feb 2022 21:27:49 -0800 Subject: [PATCH 09/10] Use absolute file paths --- dashboard/modules/serve/tests/test_serve_head.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/dashboard/modules/serve/tests/test_serve_head.py b/dashboard/modules/serve/tests/test_serve_head.py index 3bc3b5144ebe..6afcd599701a 100644 --- a/dashboard/modules/serve/tests/test_serve_head.py +++ b/dashboard/modules/serve/tests/test_serve_head.py @@ -1,6 +1,7 @@ import json import subprocess import sys +import os from typing import List, Dict, Set import pytest @@ -46,6 +47,9 @@ def deployments_match(list1: List[Dict], list2: List[Dict], properties: Set) -> return len(list2) == 0 +@pytest.mark.skipif( + sys.platform == "win32", reason="File paths incompatible with Windows." +) def test_put_get_success(ray_start_stop): ray_actor_options = { "runtime_env": {"py_modules": [test_env_uri, test_module_uri]}, @@ -75,6 +79,14 @@ def test_put_get_success(ray_start_stop): import_path="test_module.test.one", ) + three_deployments = os.path.join( + os.path.dirname(__file__), "three_deployments_response.json" + ) + + two_deployments = os.path.join( + os.path.dirname(__file__), "two_deployments_response.json" + ) + # Ensure the REST API is idempotent for _ in range(2): deployments = [shallow, deep, one] @@ -96,7 +108,7 @@ def test_put_get_success(ray_start_stop): get_response = requests.get(GET_OR_PUT_URL, timeout=30) assert get_response.status_code == 200 - with open("three_deployments_response.json", "r") as f: + with open(three_deployments, "r") as f: response_deployments = json.loads(get_response.json())["deployments"] expected_deployments = json.load(f)["deployments"] assert deployments_match( @@ -120,7 +132,7 @@ def test_put_get_success(ray_start_stop): get_response = requests.get(GET_OR_PUT_URL, timeout=30) assert get_response.status_code == 200 - with open("two_deployments_response.json", "r") as f: + with open(two_deployments, "r") as f: response_deployments = json.loads(get_response.json())["deployments"] expected_deployments = json.load(f)["deployments"] assert deployments_match( From 49da8b68a45687d135d6c71ad68a8fc73e906d18 Mon Sep 17 00:00:00 2001 From: Shreyas Krishnaswamy Date: Mon, 28 Feb 2022 21:32:22 -0800 Subject: [PATCH 10/10] Make test_serve_head a medium test again --- dashboard/BUILD | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/dashboard/BUILD b/dashboard/BUILD index 87e76ca30fab..467dd091a682 100644 --- a/dashboard/BUILD +++ b/dashboard/BUILD @@ -13,7 +13,7 @@ py_library( py_test_run_all_subdirectory( size = "medium", include = ["**/test*.py"], - exclude = ["modules/test/**", "modules/node/tests/test_node.py", "tests/test_dashboard.py", "modules/serve/tests/test_serve_head.py"], + exclude = ["modules/test/**", "modules/node/tests/test_node.py", "tests/test_dashboard.py"], extra_srcs = [], tags = ["exclusive", "team:serve"], ) @@ -31,10 +31,3 @@ py_test( srcs = ["tests/test_dashboard.py"], tags = ["exclusive", "team:serve"], ) - -py_test( - name = "test_serve_head", - size = "large", - srcs = ["modules/serve/tests/test_serve_head.py"], - tags = ["exclusive", "team:serve"], -)