From 1c2668a8b388ddce2a4ddefb350ca5ccc921a459 Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Wed, 26 Mar 2025 06:40:27 +0000 Subject: [PATCH 01/17] Update --labels argument and add --labels-from-file Signed-off-by: Ryan O'Leary --- python/ray/_private/utils.py | 61 ++++++++++++++++++++- python/ray/scripts/scripts.py | 10 +++- python/ray/tests/test_utils.py | 97 +++++++++++++++++++++++++++++++++- 3 files changed, 165 insertions(+), 3 deletions(-) diff --git a/python/ray/_private/utils.py b/python/ray/_private/utils.py index 199ac568ad33..4baa7ff5a467 100644 --- a/python/ray/_private/utils.py +++ b/python/ray/_private/utils.py @@ -82,6 +82,14 @@ # Match the standard alphabet used for UUIDs. RANDOM_STRING_ALPHABET = string.ascii_lowercase + string.digits +# Regex patterns used to validate that labels conform to Kubernetes label syntax rules. +# Regex for optional prefix (DNS subdomain) +LABEL_PREFIX_REGEX = ( + r"^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*)$" +) +# Regex for mandatory name (label key without prefix) or value +LABEL_REGEX = r"^[a-zA-Z0-9]([a-zA-Z0-9_.-]*[a-zA-Z0-9])?$" + def get_random_alphanumeric_string(length: int): """Generates random string of length consisting exclusively of @@ -1803,8 +1811,35 @@ def update_envs(env_vars: Dict[str, str]): os.environ[key] = result +def parse_node_labels_string( + labels_str: str, cli_logger, cf, command_arg="--labels" +) -> Dict[str, str]: + try: + labels = {} + # Labels argument should consist of a string of key=value pairs + # separated by commas. Labels follow Kubernetes label syntax. + label_pairs = labels_str.split(",") + for pair in label_pairs: + # Split each pair by `=` + key_value = pair.split("=") + if len(key_value) != 2: + raise ValueError("Label value is not a key-value pair.") + key = key_value[0].strip() + value = key_value[1].strip() + labels[key] = value + except Exception as e: + cli_logger.abort( + "`{}` is not a valid string of key-value pairs, detail error:{}" + "Valid values look like this: `{}`", + cf.bold(f"{command_arg}={labels_str}"), + str(e), + cf.bold(f'{command_arg}="key1=val1,key2=val2"'), + ) + return labels + + def parse_node_labels_json( - labels_json: str, cli_logger, cf, command_arg="--labels" + labels_json: str, cli_logger, cf, command_arg="--labels-from-file" ) -> Dict[str, str]: try: labels = json.loads(labels_json) @@ -1838,6 +1873,30 @@ def validate_node_labels(labels: Dict[str, str]): f"`{ray_constants.RAY_DEFAULT_LABEL_KEYS_PREFIX}`. " f"This is reserved for Ray defined labels." ) + if "/" in key: + prefix, name = key.rsplit("/") + if len(prefix) > 253 or not re.match(LABEL_PREFIX_REGEX, prefix): + raise ValueError( + f"Invalid label key prefix `{prefix}`. Prefix must be a series of DNS labels " + f"separated by dots (.),not longer than 253 characters in total." + ) + else: + name = key + if len(name) > 63 or not re.match(LABEL_REGEX, name): + raise ValueError( + f"Invalid label key name `{name}`. Name must be 63 chars or less beginning and ending " + f"with an alphanumeric character ([a-z0-9A-Z]) with dashes (-), underscores (_)," + f"dots (.), and alphanumerics between." + ) + value = labels.get(key) + if value is None or value == "": + return + if len(value) > 63 or not re.match(LABEL_REGEX, value): + raise ValueError( + f"Invalid label key value `{value}`. Value must be 63 chars or less beginning and ending " + f"with an alphanumeric character ([a-z0-9A-Z]) with dashes (-), underscores (_)," + f"dots (.), and alphanumerics between." + ) def parse_pg_formatted_resources_to_original( diff --git a/python/ray/scripts/scripts.py b/python/ray/scripts/scripts.py index eef52687d574..1fff3b66905e 100644 --- a/python/ray/scripts/scripts.py +++ b/python/ray/scripts/scripts.py @@ -629,9 +629,17 @@ def debug(address: str, verbose: bool): "--labels", required=False, hidden=True, + default="", + type=str, + help="a string list of key-value pairs mapping label name to label value.", +) +@click.option( + "--labels-from-file", + required=False, + hidden=True, default="{}", type=str, - help="a JSON serialized dictionary mapping label name to label value.", + help="a JSON serialized dictionary mapping label name to label value sourced from a file.", ) @click.option( "--include-log-monitor", diff --git a/python/ray/tests/test_utils.py b/python/ray/tests/test_utils.py index b5bb5d34f86a..fb83b56c7e99 100644 --- a/python/ray/tests/test_utils.py +++ b/python/ray/tests/test_utils.py @@ -1,17 +1,22 @@ # coding: utf-8 """ -Unit/Integration Testing for python/_private/utils.py +Unit/Integration Testing for python/ray/_private/utils.py This currently expects to work for minimal installs. """ +import click import pytest import logging from ray._private.utils import ( parse_pg_formatted_resources_to_original, try_import_each_module, get_current_node_cpu_model_name, + parse_node_labels_string, + parse_node_labels_json, + validate_node_labels, ) +from ray.autoscaler._private.cli_logger import cf, cli_logger from unittest.mock import patch, mock_open import sys @@ -71,6 +76,96 @@ def test_parse_pg_formatted_resources(): assert out == {"CPU": 1, "memory": 100} +def test_parse_node_labels_from_string(): + # Empty/invalid label argument passed + labels_string = "" + with pytest.raises(click.exceptions.ClickException) as e: + parse_node_labels_string(labels_string, cli_logger, cf) + assert "is not a valid string of key-value pairs" in str(e) + + # Valid label key with empty value + labels_string = "region=" + labels_dict = parse_node_labels_string(labels_string, cli_logger, cf) + assert labels_dict == {"region": ""} + + # Multiple valid label keys and values + labels_string = "ray.io/accelerator-type=A100,region=us-west4" + labels_dict = parse_node_labels_string(labels_string, cli_logger, cf) + assert labels_dict == {"ray.io/accelerator-type": "A100", "region": "us-west4"} + + # Invalid label + labels_string = "ray.io/accelerator-type=type=A100" + with pytest.raises(click.exceptions.ClickException) as e: + parse_node_labels_string(labels_string, cli_logger, cf) + assert "is not a valid string of key-value pairs" in str(e) + + +def test_parse_node_labels_from_json(): + # Empty/invalid json + labels_json = "" + with pytest.raises(click.exceptions.ClickException) as e: + parse_node_labels_json(labels_json, cli_logger, cf) + assert "is not a valid JSON string" in str(e) + + # Valid label key with empty value + labels_json = '{"ray.io/accelerator-type": ""}' + labels_dict = parse_node_labels_json(labels_json, cli_logger, cf) + assert labels_dict == {"ray.io/accelerator-type": ""} + + # Multiple valid label keys and values + labels_json = ( + '{"ray.io/accelerator-type": "A100", "region": "us", "market-type": "spot"}' + ) + labels_dict = parse_node_labels_json(labels_json, cli_logger, cf) + assert labels_dict == { + "ray.io/accelerator-type": "A100", + "region": "us", + "market-type": "spot", + } + + # Non-string label key + labels_json = '{100: "A100"}' + with pytest.raises(click.exceptions.ClickException) as e: + parse_node_labels_json(labels_json, cli_logger, cf) + assert "is not a valid JSON string" in str(e) + + # Non-string label value + labels_json = '{"ray.io/accelerator-type": 5}' + with pytest.raises(click.exceptions.ClickException) as e: + parse_node_labels_json(labels_json, cli_logger, cf) + assert "is not a valid JSON string" in str(e) + + +def test_validate_node_labels(): + # Custom label starts with ray.io prefix + labels_dict = {"ray.io/accelerator-type": "A100"} + with pytest.raises(ValueError) as e: + validate_node_labels(labels_dict) + assert "This is reserved for Ray defined labels." in str(e) + + # Invalid key prefix syntax + labels_dict = {"invalidPrefix/accelerator-type": "A100"} + with pytest.raises(ValueError) as e: + validate_node_labels(labels_dict) + assert "Invalid label key prefix" in str(e) + + # Invalid key name syntax + labels_dict = {"!!accelerator-type?": "A100"} + with pytest.raises(ValueError) as e: + validate_node_labels(labels_dict) + assert "Invalid label key name" in str(e) + + # Invalid key value syntax + labels_dict = {"accelerator-type": "??"} + with pytest.raises(ValueError) as e: + validate_node_labels(labels_dict) + assert "Invalid label key value" in str(e) + + # Valid node label + labels_dict = {"accelerator-type": "A100"} + validate_node_labels(labels_dict) + + @pytest.mark.skipif( not sys.platform.startswith("linux"), reason="Doesn't support non-linux" ) From b3ced81ed887e9c930056860b44654c216ca8ef6 Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Wed, 26 Mar 2025 07:00:29 +0000 Subject: [PATCH 02/17] Fix scripts.py label parsing Signed-off-by: Ryan O'Leary --- python/ray/scripts/scripts.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/python/ray/scripts/scripts.py b/python/ray/scripts/scripts.py index 1fff3b66905e..3ac753bbc2e4 100644 --- a/python/ray/scripts/scripts.py +++ b/python/ray/scripts/scripts.py @@ -30,6 +30,7 @@ load_class, parse_resources_json, parse_node_labels_json, + parse_node_labels_string, ) from ray._private.internal_api import memory_summary from ray._private.usage import usage_lib @@ -696,6 +697,7 @@ def start( ray_debugger_external, disable_usage_stats, labels, + labels_from_file, include_log_monitor, ): """Start Ray processes manually on the local machine.""" @@ -716,7 +718,14 @@ def start( node_ip_address = services.resolve_ip_for_localhost(node_ip_address) resources = parse_resources_json(resources, cli_logger, cf) - labels_dict = parse_node_labels_json(labels, cli_logger, cf) + + # Compose labels passed in with `--labels` and `--labels-from-file`. + # The label value from `--labels` will overrwite the value of any duplicate keys. + labels_from_file_dict = parse_node_labels_json(labels_from_file, cli_logger, cf) + labels_dict = { + **labels_from_file_dict, + **parse_node_labels_string(labels, cli_logger, cf), + } if plasma_store_socket_name is not None: warnings.warn( From 8734a47ea89077aede074ee17fc25044911c3026 Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Wed, 26 Mar 2025 08:44:15 +0000 Subject: [PATCH 03/17] Handle empty string case Signed-off-by: Ryan O'Leary --- python/ray/_private/utils.py | 2 ++ python/ray/tests/test_utils.py | 12 +++++------- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/python/ray/_private/utils.py b/python/ray/_private/utils.py index 4baa7ff5a467..bb9ca541059a 100644 --- a/python/ray/_private/utils.py +++ b/python/ray/_private/utils.py @@ -1816,6 +1816,8 @@ def parse_node_labels_string( ) -> Dict[str, str]: try: labels = {} + if labels_str == "": + return labels # Labels argument should consist of a string of key=value pairs # separated by commas. Labels follow Kubernetes label syntax. label_pairs = labels_str.split(",") diff --git a/python/ray/tests/test_utils.py b/python/ray/tests/test_utils.py index fb83b56c7e99..a371be336a8f 100644 --- a/python/ray/tests/test_utils.py +++ b/python/ray/tests/test_utils.py @@ -77,11 +77,10 @@ def test_parse_pg_formatted_resources(): def test_parse_node_labels_from_string(): - # Empty/invalid label argument passed + # Empty label argument passed labels_string = "" - with pytest.raises(click.exceptions.ClickException) as e: - parse_node_labels_string(labels_string, cli_logger, cf) - assert "is not a valid string of key-value pairs" in str(e) + labels_dict = parse_node_labels_string(labels_string, cli_logger, cf) + assert labels_dict == {} # Valid label key with empty value labels_string = "region=" @@ -103,9 +102,8 @@ def test_parse_node_labels_from_string(): def test_parse_node_labels_from_json(): # Empty/invalid json labels_json = "" - with pytest.raises(click.exceptions.ClickException) as e: - parse_node_labels_json(labels_json, cli_logger, cf) - assert "is not a valid JSON string" in str(e) + labels_dict = parse_node_labels_json(labels_json, cli_logger, cf) + assert labels_dict == {} # Valid label key with empty value labels_json = '{"ray.io/accelerator-type": ""}' From 35649453626a0a023c6e8c83b6bd13ea22f8851f Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Wed, 26 Mar 2025 10:45:52 +0000 Subject: [PATCH 04/17] Add implicit None check Signed-off-by: Ryan O'Leary --- python/ray/scripts/scripts.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/python/ray/scripts/scripts.py b/python/ray/scripts/scripts.py index 3ac753bbc2e4..3dc972e24647 100644 --- a/python/ray/scripts/scripts.py +++ b/python/ray/scripts/scripts.py @@ -722,10 +722,12 @@ def start( # Compose labels passed in with `--labels` and `--labels-from-file`. # The label value from `--labels` will overrwite the value of any duplicate keys. labels_from_file_dict = parse_node_labels_json(labels_from_file, cli_logger, cf) - labels_dict = { - **labels_from_file_dict, - **parse_node_labels_string(labels, cli_logger, cf), - } + labels_from_string = parse_node_labels_string(labels, cli_logger, cf) + labels_dict = ( + {**labels_from_file_dict, **labels_from_string} + if labels_from_file_dict + else labels_from_string + ) if plasma_store_socket_name is not None: warnings.warn( From 5567fb732156f260f746eb05c3d2044da060549b Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Wed, 26 Mar 2025 10:49:38 +0000 Subject: [PATCH 05/17] Fix test Signed-off-by: Ryan O'Leary --- python/ray/tests/test_utils.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/python/ray/tests/test_utils.py b/python/ray/tests/test_utils.py index a371be336a8f..37e8cc57560d 100644 --- a/python/ray/tests/test_utils.py +++ b/python/ray/tests/test_utils.py @@ -102,8 +102,9 @@ def test_parse_node_labels_from_string(): def test_parse_node_labels_from_json(): # Empty/invalid json labels_json = "" - labels_dict = parse_node_labels_json(labels_json, cli_logger, cf) - assert labels_dict == {} + with pytest.raises(click.exceptions.ClickException) as e: + parse_node_labels_json(labels_json, cli_logger, cf) + assert "is not a valid JSON string" in str(e) # Valid label key with empty value labels_json = '{"ray.io/accelerator-type": ""}' From b6e99ea7dc1a3dff42a59247dbf223bfad631b91 Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Sat, 29 Mar 2025 03:24:59 +0000 Subject: [PATCH 06/17] Add read from file and fix tests Signed-off-by: Ryan O'Leary --- python/ray/_private/utils.py | 33 +++++++++-------- python/ray/scripts/scripts.py | 14 ++++---- python/ray/tests/test_utils.py | 65 ++++++++++++++++++++-------------- 3 files changed, 64 insertions(+), 48 deletions(-) diff --git a/python/ray/_private/utils.py b/python/ray/_private/utils.py index bb9ca541059a..37fb9c6251b8 100644 --- a/python/ray/_private/utils.py +++ b/python/ray/_private/utils.py @@ -20,6 +20,7 @@ import threading import time import warnings +import yaml from inspect import signature from pathlib import Path from subprocess import list2cmdline @@ -1840,27 +1841,29 @@ def parse_node_labels_string( return labels -def parse_node_labels_json( - labels_json: str, cli_logger, cf, command_arg="--labels-from-file" +def parse_node_labels_from_yaml_file( + path: str, cli_logger, cf, command_arg="--labels-file" ) -> Dict[str, str]: try: - labels = json.loads(labels_json) - if not isinstance(labels, dict): - raise ValueError( - "The format after deserialization is not a key-value pair map" - ) - for key, value in labels.items(): - if not isinstance(key, str): - raise ValueError("The key is not string type.") - if not isinstance(value, str): - raise ValueError(f'The value of the "{key}" is not string type') + with open(path, "r") as file: + # Expects valid YAML content + labels = yaml.safe_load(file) + if not isinstance(labels, dict): + raise ValueError( + "The format after deserialization is not a key-value pair map" + ) + for key, value in labels.items(): + if not isinstance(key, str): + raise ValueError("The key is not string type.") + if not isinstance(value, str): + raise ValueError(f'The value of the "{key}" is not string type') except Exception as e: cli_logger.abort( - "`{}` is not a valid JSON string, detail error:{}" + "The file at `{}` is not a valid YAML file, detail error:{}" "Valid values look like this: `{}`", - cf.bold(f"{command_arg}={labels_json}"), + cf.bold(f"{command_arg}={path}"), str(e), - cf.bold(f'{command_arg}=\'{{"gpu_type": "A100", "region": "us"}}\''), + cf.bold(f"{command_arg}='gpu_type: A100\nregion: us'"), ) return labels diff --git a/python/ray/scripts/scripts.py b/python/ray/scripts/scripts.py index 3dc972e24647..9eeb029fd904 100644 --- a/python/ray/scripts/scripts.py +++ b/python/ray/scripts/scripts.py @@ -29,7 +29,7 @@ check_ray_client_dependencies_installed, load_class, parse_resources_json, - parse_node_labels_json, + parse_node_labels_from_yaml_file, parse_node_labels_string, ) from ray._private.internal_api import memory_summary @@ -635,12 +635,12 @@ def debug(address: str, verbose: bool): help="a string list of key-value pairs mapping label name to label value.", ) @click.option( - "--labels-from-file", + "--labels-file", required=False, hidden=True, - default="{}", + default="", type=str, - help="a JSON serialized dictionary mapping label name to label value sourced from a file.", + help="a path to a YAML file containing a serialized dictionary mapping of labels.", ) @click.option( "--include-log-monitor", @@ -697,7 +697,7 @@ def start( ray_debugger_external, disable_usage_stats, labels, - labels_from_file, + labels_file, include_log_monitor, ): """Start Ray processes manually on the local machine.""" @@ -721,7 +721,9 @@ def start( # Compose labels passed in with `--labels` and `--labels-from-file`. # The label value from `--labels` will overrwite the value of any duplicate keys. - labels_from_file_dict = parse_node_labels_json(labels_from_file, cli_logger, cf) + labels_from_file_dict = parse_node_labels_from_yaml_file( + labels_file, cli_logger, cf + ) labels_from_string = parse_node_labels_string(labels, cli_logger, cf) labels_dict = ( {**labels_from_file_dict, **labels_from_string} diff --git a/python/ray/tests/test_utils.py b/python/ray/tests/test_utils.py index 37e8cc57560d..d79437104074 100644 --- a/python/ray/tests/test_utils.py +++ b/python/ray/tests/test_utils.py @@ -13,12 +13,13 @@ try_import_each_module, get_current_node_cpu_model_name, parse_node_labels_string, - parse_node_labels_json, + parse_node_labels_from_yaml_file, validate_node_labels, ) from ray.autoscaler._private.cli_logger import cf, cli_logger from unittest.mock import patch, mock_open import sys +import tempfile logger = logging.getLogger(__name__) @@ -99,40 +100,50 @@ def test_parse_node_labels_from_string(): assert "is not a valid string of key-value pairs" in str(e) -def test_parse_node_labels_from_json(): - # Empty/invalid json - labels_json = "" - with pytest.raises(click.exceptions.ClickException) as e: - parse_node_labels_json(labels_json, cli_logger, cf) - assert "is not a valid JSON string" in str(e) +def test_parse_node_labels_from_yaml_file(): + # Empty/invalid yaml + with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: + test_file.write("") + test_file.flush() # Ensure data is written + with pytest.raises(click.exceptions.ClickException) as e: + parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) + assert "is not a valid YAML string" in str(e) # Valid label key with empty value - labels_json = '{"ray.io/accelerator-type": ""}' - labels_dict = parse_node_labels_json(labels_json, cli_logger, cf) - assert labels_dict == {"ray.io/accelerator-type": ""} + with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: + test_file.write('"ray.io/accelerator-type": ""') + test_file.flush() # Ensure data is written + labels_dict = parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) + assert labels_dict == {"ray.io/accelerator-type": ""} # Multiple valid label keys and values - labels_json = ( - '{"ray.io/accelerator-type": "A100", "region": "us", "market-type": "spot"}' - ) - labels_dict = parse_node_labels_json(labels_json, cli_logger, cf) - assert labels_dict == { - "ray.io/accelerator-type": "A100", - "region": "us", - "market-type": "spot", - } + with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: + test_file.write( + '"ray.io/accelerator-type": "A100"\n"region": "us"\n"market-type": "spot"' + ) + test_file.flush() # Ensure data is written + labels_dict = parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) + assert labels_dict == { + "ray.io/accelerator-type": "A100", + "region": "us", + "market-type": "spot", + } # Non-string label key - labels_json = '{100: "A100"}' - with pytest.raises(click.exceptions.ClickException) as e: - parse_node_labels_json(labels_json, cli_logger, cf) - assert "is not a valid JSON string" in str(e) + with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: + test_file.write('{100: "A100"}') + test_file.flush() # Ensure data is written + with pytest.raises(click.exceptions.ClickException) as e: + parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) + assert "is not a valid YAML string" in str(e) # Non-string label value - labels_json = '{"ray.io/accelerator-type": 5}' - with pytest.raises(click.exceptions.ClickException) as e: - parse_node_labels_json(labels_json, cli_logger, cf) - assert "is not a valid JSON string" in str(e) + with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: + test_file.write('{100: "A100"}') + test_file.flush() # Ensure data is written + with pytest.raises(click.exceptions.ClickException) as e: + parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) + assert "is not a valid YAML string" in str(e) def test_validate_node_labels(): From 04d8eb697dc44f5d2b49201a78972eb124b187b6 Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Mon, 31 Mar 2025 20:49:59 +0000 Subject: [PATCH 07/17] Handle empty path Signed-off-by: Ryan O'Leary --- python/ray/_private/utils.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/python/ray/_private/utils.py b/python/ray/_private/utils.py index 37fb9c6251b8..e3fbadff96d3 100644 --- a/python/ray/_private/utils.py +++ b/python/ray/_private/utils.py @@ -1845,6 +1845,8 @@ def parse_node_labels_from_yaml_file( path: str, cli_logger, cf, command_arg="--labels-file" ) -> Dict[str, str]: try: + if path == "": + return {} with open(path, "r") as file: # Expects valid YAML content labels = yaml.safe_load(file) From 5a50d2801f86b2727bb15afb3a0a8fc9c5852346 Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Wed, 2 Apr 2025 19:16:55 +0000 Subject: [PATCH 08/17] Move label utils to separate files Signed-off-by: Ryan O'Leary --- python/ray/_private/label_utils.py | 107 ++++++++++++++++++++++++ python/ray/_private/parameter.py | 6 +- python/ray/_private/utils.py | 103 ----------------------- python/ray/scripts/scripts.py | 2 +- python/ray/tests/test_label_utils.py | 119 +++++++++++++++++++++++++++ python/ray/tests/test_utils.py | 105 ----------------------- 6 files changed, 229 insertions(+), 213 deletions(-) create mode 100644 python/ray/_private/label_utils.py create mode 100644 python/ray/tests/test_label_utils.py diff --git a/python/ray/_private/label_utils.py b/python/ray/_private/label_utils.py new file mode 100644 index 000000000000..6727bc556b42 --- /dev/null +++ b/python/ray/_private/label_utils.py @@ -0,0 +1,107 @@ +import re +import yaml +from typing import Dict + +import ray._private.ray_constants as ray_constants + +# Regex patterns used to validate that labels conform to Kubernetes label syntax rules. +# Regex for optional prefix (DNS subdomain) +LABEL_PREFIX_REGEX = ( + r"^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*)$" +) +# Regex for mandatory name (label key without prefix) or value +LABEL_REGEX = r"^[a-zA-Z0-9]([a-zA-Z0-9_.-]*[a-zA-Z0-9])?$" + + +def parse_node_labels_string( + labels_str: str, cli_logger, cf, command_arg="--labels" +) -> Dict[str, str]: + try: + labels = {} + if labels_str == "": + return labels + # Labels argument should consist of a string of key=value pairs + # separated by commas. Labels follow Kubernetes label syntax. + label_pairs = labels_str.split(",") + for pair in label_pairs: + # Split each pair by `=` + key_value = pair.split("=") + if len(key_value) != 2: + raise ValueError("Label value is not a key-value pair.") + key = key_value[0].strip() + value = key_value[1].strip() + labels[key] = value + except Exception as e: + cli_logger.abort( + "`{}` is not a valid string of key-value pairs, detail error:{}" + "Valid values look like this: `{}`", + cf.bold(f"{command_arg}={labels_str}"), + str(e), + cf.bold(f'{command_arg}="key1=val1,key2=val2"'), + ) + return labels + + +def parse_node_labels_from_yaml_file( + path: str, cli_logger, cf, command_arg="--labels-file" +) -> Dict[str, str]: + try: + if path == "": + return {} + with open(path, "r") as file: + # Expects valid YAML content + labels = yaml.safe_load(file) + if not isinstance(labels, dict): + raise ValueError( + "The format after deserialization is not a key-value pair map" + ) + for key, value in labels.items(): + if not isinstance(key, str): + raise ValueError("The key is not string type.") + if not isinstance(value, str): + raise ValueError(f'The value of the "{key}" is not string type') + except Exception as e: + cli_logger.abort( + "The file at `{}` is not a valid YAML file, detail error:{}" + "Valid values look like this: `{}`", + cf.bold(f"{command_arg}={path}"), + str(e), + cf.bold(f"{command_arg}='gpu_type: A100\nregion: us'"), + ) + return labels + + +def validate_node_labels(labels: Dict[str, str]): + if labels is None: + return + for key in labels.keys(): + if key.startswith(ray_constants.RAY_DEFAULT_LABEL_KEYS_PREFIX): + raise ValueError( + f"Custom label keys `{key}` cannot start with the prefix " + f"`{ray_constants.RAY_DEFAULT_LABEL_KEYS_PREFIX}`. " + f"This is reserved for Ray defined labels." + ) + if "/" in key: + prefix, name = key.rsplit("/") + if len(prefix) > 253 or not re.match(LABEL_PREFIX_REGEX, prefix): + raise ValueError( + f"Invalid label key prefix `{prefix}`. Prefix must be a series of DNS labels " + f"separated by dots (.),not longer than 253 characters in total." + ) + else: + name = key + if len(name) > 63 or not re.match(LABEL_REGEX, name): + raise ValueError( + f"Invalid label key name `{name}`. Name must be 63 chars or less beginning and ending " + f"with an alphanumeric character ([a-z0-9A-Z]) with dashes (-), underscores (_)," + f"dots (.), and alphanumerics between." + ) + value = labels.get(key) + if value is None or value == "": + return + if len(value) > 63 or not re.match(LABEL_REGEX, value): + raise ValueError( + f"Invalid label key value `{value}`. Value must be 63 chars or less beginning and ending " + f"with an alphanumeric character ([a-z0-9A-Z]) with dashes (-), underscores (_)," + f"dots (.), and alphanumerics between." + ) diff --git a/python/ray/_private/parameter.py b/python/ray/_private/parameter.py index 9e999ad42fd4..ef391eaf606b 100644 --- a/python/ray/_private/parameter.py +++ b/python/ray/_private/parameter.py @@ -3,10 +3,8 @@ from typing import Dict, List, Optional import ray._private.ray_constants as ray_constants -from ray._private.utils import ( - validate_node_labels, - check_ray_client_dependencies_installed, -) +from ray._private.label_utils import validate_node_labels +from ray._private.utils import check_ray_client_dependencies_installed logger = logging.getLogger(__name__) diff --git a/python/ray/_private/utils.py b/python/ray/_private/utils.py index 9500a5a6caab..169d44baa4db 100644 --- a/python/ray/_private/utils.py +++ b/python/ray/_private/utils.py @@ -20,7 +20,6 @@ import threading import time import warnings -import yaml from inspect import signature from pathlib import Path from subprocess import list2cmdline @@ -83,14 +82,6 @@ # Match the standard alphabet used for UUIDs. RANDOM_STRING_ALPHABET = string.ascii_lowercase + string.digits -# Regex patterns used to validate that labels conform to Kubernetes label syntax rules. -# Regex for optional prefix (DNS subdomain) -LABEL_PREFIX_REGEX = ( - r"^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*)$" -) -# Regex for mandatory name (label key without prefix) or value -LABEL_REGEX = r"^[a-zA-Z0-9]([a-zA-Z0-9_.-]*[a-zA-Z0-9])?$" - def get_random_alphanumeric_string(length: int): """Generates random string of length consisting exclusively of @@ -1812,100 +1803,6 @@ def update_envs(env_vars: Dict[str, str]): os.environ[key] = result -def parse_node_labels_string( - labels_str: str, cli_logger, cf, command_arg="--labels" -) -> Dict[str, str]: - try: - labels = {} - if labels_str == "": - return labels - # Labels argument should consist of a string of key=value pairs - # separated by commas. Labels follow Kubernetes label syntax. - label_pairs = labels_str.split(",") - for pair in label_pairs: - # Split each pair by `=` - key_value = pair.split("=") - if len(key_value) != 2: - raise ValueError("Label value is not a key-value pair.") - key = key_value[0].strip() - value = key_value[1].strip() - labels[key] = value - except Exception as e: - cli_logger.abort( - "`{}` is not a valid string of key-value pairs, detail error:{}" - "Valid values look like this: `{}`", - cf.bold(f"{command_arg}={labels_str}"), - str(e), - cf.bold(f'{command_arg}="key1=val1,key2=val2"'), - ) - return labels - - -def parse_node_labels_from_yaml_file( - path: str, cli_logger, cf, command_arg="--labels-file" -) -> Dict[str, str]: - try: - if path == "": - return {} - with open(path, "r") as file: - # Expects valid YAML content - labels = yaml.safe_load(file) - if not isinstance(labels, dict): - raise ValueError( - "The format after deserialization is not a key-value pair map" - ) - for key, value in labels.items(): - if not isinstance(key, str): - raise ValueError("The key is not string type.") - if not isinstance(value, str): - raise ValueError(f'The value of the "{key}" is not string type') - except Exception as e: - cli_logger.abort( - "The file at `{}` is not a valid YAML file, detail error:{}" - "Valid values look like this: `{}`", - cf.bold(f"{command_arg}={path}"), - str(e), - cf.bold(f"{command_arg}='gpu_type: A100\nregion: us'"), - ) - return labels - - -def validate_node_labels(labels: Dict[str, str]): - if labels is None: - return - for key in labels.keys(): - if key.startswith(ray_constants.RAY_DEFAULT_LABEL_KEYS_PREFIX): - raise ValueError( - f"Custom label keys `{key}` cannot start with the prefix " - f"`{ray_constants.RAY_DEFAULT_LABEL_KEYS_PREFIX}`. " - f"This is reserved for Ray defined labels." - ) - if "/" in key: - prefix, name = key.rsplit("/") - if len(prefix) > 253 or not re.match(LABEL_PREFIX_REGEX, prefix): - raise ValueError( - f"Invalid label key prefix `{prefix}`. Prefix must be a series of DNS labels " - f"separated by dots (.),not longer than 253 characters in total." - ) - else: - name = key - if len(name) > 63 or not re.match(LABEL_REGEX, name): - raise ValueError( - f"Invalid label key name `{name}`. Name must be 63 chars or less beginning and ending " - f"with an alphanumeric character ([a-z0-9A-Z]) with dashes (-), underscores (_)," - f"dots (.), and alphanumerics between." - ) - value = labels.get(key) - if value is None or value == "": - return - if len(value) > 63 or not re.match(LABEL_REGEX, value): - raise ValueError( - f"Invalid label key value `{value}`. Value must be 63 chars or less beginning and ending " - f"with an alphanumeric character ([a-z0-9A-Z]) with dashes (-), underscores (_)," - f"dots (.), and alphanumerics between." - ) - - def parse_pg_formatted_resources_to_original( pg_formatted_resources: Dict[str, float] ) -> Dict[str, float]: diff --git a/python/ray/scripts/scripts.py b/python/ray/scripts/scripts.py index 888549a733c5..0c6a96817345 100644 --- a/python/ray/scripts/scripts.py +++ b/python/ray/scripts/scripts.py @@ -726,7 +726,7 @@ def start( resources = parse_resources_json(resources, cli_logger, cf) - # Compose labels passed in with `--labels` and `--labels-from-file`. + # Compose labels passed in with `--labels` and `--labels-file`. # The label value from `--labels` will overrwite the value of any duplicate keys. labels_from_file_dict = parse_node_labels_from_yaml_file( labels_file, cli_logger, cf diff --git a/python/ray/tests/test_label_utils.py b/python/ray/tests/test_label_utils.py new file mode 100644 index 000000000000..412c472ef771 --- /dev/null +++ b/python/ray/tests/test_label_utils.py @@ -0,0 +1,119 @@ +import pytest +import click +from ray._private.label_utils import ( + parse_node_labels_string, + parse_node_labels_from_yaml_file, + validate_node_labels, +) +from ray.autoscaler._private.cli_logger import cf, cli_logger +import sys +import tempfile + + +def test_parse_node_labels_from_string(): + # Empty label argument passed + labels_string = "" + labels_dict = parse_node_labels_string(labels_string, cli_logger, cf) + assert labels_dict == {} + + # Valid label key with empty value + labels_string = "region=" + labels_dict = parse_node_labels_string(labels_string, cli_logger, cf) + assert labels_dict == {"region": ""} + + # Multiple valid label keys and values + labels_string = "ray.io/accelerator-type=A100,region=us-west4" + labels_dict = parse_node_labels_string(labels_string, cli_logger, cf) + assert labels_dict == {"ray.io/accelerator-type": "A100", "region": "us-west4"} + + # Invalid label + labels_string = "ray.io/accelerator-type=type=A100" + with pytest.raises(click.exceptions.ClickException) as e: + parse_node_labels_string(labels_string, cli_logger, cf) + assert "is not a valid string of key-value pairs" in str(e) + + +def test_parse_node_labels_from_yaml_file(): + # Empty/invalid yaml + with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: + test_file.write("") + test_file.flush() # Ensure data is written + with pytest.raises(click.exceptions.ClickException) as e: + parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) + assert "is not a valid YAML string" in str(e) + + # Valid label key with empty value + with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: + test_file.write('"ray.io/accelerator-type": ""') + test_file.flush() # Ensure data is written + labels_dict = parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) + assert labels_dict == {"ray.io/accelerator-type": ""} + + # Multiple valid label keys and values + with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: + test_file.write( + '"ray.io/accelerator-type": "A100"\n"region": "us"\n"market-type": "spot"' + ) + test_file.flush() # Ensure data is written + labels_dict = parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) + assert labels_dict == { + "ray.io/accelerator-type": "A100", + "region": "us", + "market-type": "spot", + } + + # Non-string label key + with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: + test_file.write('{100: "A100"}') + test_file.flush() # Ensure data is written + with pytest.raises(click.exceptions.ClickException) as e: + parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) + assert "is not a valid YAML string" in str(e) + + # Non-string label value + with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: + test_file.write('{100: "A100"}') + test_file.flush() # Ensure data is written + with pytest.raises(click.exceptions.ClickException) as e: + parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) + assert "is not a valid YAML string" in str(e) + + +def test_validate_node_labels(): + # Custom label starts with ray.io prefix + labels_dict = {"ray.io/accelerator-type": "A100"} + with pytest.raises(ValueError) as e: + validate_node_labels(labels_dict) + assert "This is reserved for Ray defined labels." in str(e) + + # Invalid key prefix syntax + labels_dict = {"invalidPrefix/accelerator-type": "A100"} + with pytest.raises(ValueError) as e: + validate_node_labels(labels_dict) + assert "Invalid label key prefix" in str(e) + + # Invalid key name syntax + labels_dict = {"!!accelerator-type?": "A100"} + with pytest.raises(ValueError) as e: + validate_node_labels(labels_dict) + assert "Invalid label key name" in str(e) + + # Invalid key value syntax + labels_dict = {"accelerator-type": "??"} + with pytest.raises(ValueError) as e: + validate_node_labels(labels_dict) + assert "Invalid label key value" in str(e) + + # Valid node label + labels_dict = {"accelerator-type": "A100"} + validate_node_labels(labels_dict) + + +if __name__ == "__main__": + import os + + # Skip test_basic_2_client_mode for now- the test suite is breaking. + if os.environ.get("PARALLEL_CI"): + sys.exit(pytest.main(["-n", "auto", "--boxed", "-vs", __file__])) + else: + sys.exit(pytest.main(["-sv", __file__])) diff --git a/python/ray/tests/test_utils.py b/python/ray/tests/test_utils.py index d79437104074..fe81b3ac7ed3 100644 --- a/python/ray/tests/test_utils.py +++ b/python/ray/tests/test_utils.py @@ -5,21 +5,15 @@ This currently expects to work for minimal installs. """ -import click import pytest import logging from ray._private.utils import ( parse_pg_formatted_resources_to_original, try_import_each_module, get_current_node_cpu_model_name, - parse_node_labels_string, - parse_node_labels_from_yaml_file, - validate_node_labels, ) -from ray.autoscaler._private.cli_logger import cf, cli_logger from unittest.mock import patch, mock_open import sys -import tempfile logger = logging.getLogger(__name__) @@ -77,105 +71,6 @@ def test_parse_pg_formatted_resources(): assert out == {"CPU": 1, "memory": 100} -def test_parse_node_labels_from_string(): - # Empty label argument passed - labels_string = "" - labels_dict = parse_node_labels_string(labels_string, cli_logger, cf) - assert labels_dict == {} - - # Valid label key with empty value - labels_string = "region=" - labels_dict = parse_node_labels_string(labels_string, cli_logger, cf) - assert labels_dict == {"region": ""} - - # Multiple valid label keys and values - labels_string = "ray.io/accelerator-type=A100,region=us-west4" - labels_dict = parse_node_labels_string(labels_string, cli_logger, cf) - assert labels_dict == {"ray.io/accelerator-type": "A100", "region": "us-west4"} - - # Invalid label - labels_string = "ray.io/accelerator-type=type=A100" - with pytest.raises(click.exceptions.ClickException) as e: - parse_node_labels_string(labels_string, cli_logger, cf) - assert "is not a valid string of key-value pairs" in str(e) - - -def test_parse_node_labels_from_yaml_file(): - # Empty/invalid yaml - with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: - test_file.write("") - test_file.flush() # Ensure data is written - with pytest.raises(click.exceptions.ClickException) as e: - parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) - assert "is not a valid YAML string" in str(e) - - # Valid label key with empty value - with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: - test_file.write('"ray.io/accelerator-type": ""') - test_file.flush() # Ensure data is written - labels_dict = parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) - assert labels_dict == {"ray.io/accelerator-type": ""} - - # Multiple valid label keys and values - with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: - test_file.write( - '"ray.io/accelerator-type": "A100"\n"region": "us"\n"market-type": "spot"' - ) - test_file.flush() # Ensure data is written - labels_dict = parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) - assert labels_dict == { - "ray.io/accelerator-type": "A100", - "region": "us", - "market-type": "spot", - } - - # Non-string label key - with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: - test_file.write('{100: "A100"}') - test_file.flush() # Ensure data is written - with pytest.raises(click.exceptions.ClickException) as e: - parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) - assert "is not a valid YAML string" in str(e) - - # Non-string label value - with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: - test_file.write('{100: "A100"}') - test_file.flush() # Ensure data is written - with pytest.raises(click.exceptions.ClickException) as e: - parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) - assert "is not a valid YAML string" in str(e) - - -def test_validate_node_labels(): - # Custom label starts with ray.io prefix - labels_dict = {"ray.io/accelerator-type": "A100"} - with pytest.raises(ValueError) as e: - validate_node_labels(labels_dict) - assert "This is reserved for Ray defined labels." in str(e) - - # Invalid key prefix syntax - labels_dict = {"invalidPrefix/accelerator-type": "A100"} - with pytest.raises(ValueError) as e: - validate_node_labels(labels_dict) - assert "Invalid label key prefix" in str(e) - - # Invalid key name syntax - labels_dict = {"!!accelerator-type?": "A100"} - with pytest.raises(ValueError) as e: - validate_node_labels(labels_dict) - assert "Invalid label key name" in str(e) - - # Invalid key value syntax - labels_dict = {"accelerator-type": "??"} - with pytest.raises(ValueError) as e: - validate_node_labels(labels_dict) - assert "Invalid label key value" in str(e) - - # Valid node label - labels_dict = {"accelerator-type": "A100"} - validate_node_labels(labels_dict) - - @pytest.mark.skipif( not sys.platform.startswith("linux"), reason="Doesn't support non-linux" ) From 67dbc021b4118f5f3d5055a5b6c7a48eec9ed989 Mon Sep 17 00:00:00 2001 From: ryanaoleary <113500783+ryanaoleary@users.noreply.github.com> Date: Wed, 2 Apr 2025 13:03:32 -0700 Subject: [PATCH 09/17] Update python/ray/scripts/scripts.py Co-authored-by: Edward Oakes Signed-off-by: ryanaoleary <113500783+ryanaoleary@users.noreply.github.com> --- python/ray/scripts/scripts.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ray/scripts/scripts.py b/python/ray/scripts/scripts.py index 0c6a96817345..d4898baf4925 100644 --- a/python/ray/scripts/scripts.py +++ b/python/ray/scripts/scripts.py @@ -646,7 +646,7 @@ def debug(address: str, verbose: bool): hidden=True, default="", type=str, - help="a path to a YAML file containing a serialized dictionary mapping of labels.", + help="a path to a YAML file containing a dictionary mapping of label keys to values.", ) @click.option( "--include-log-monitor", From e04ab0f91bc7fd4df67de35b5e3f62bfa961642a Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Wed, 2 Apr 2025 20:08:42 +0000 Subject: [PATCH 10/17] Move cli_logger logic to CLI Signed-off-by: Ryan O'Leary --- python/ray/_private/label_utils.py | 84 +++++++++++----------------- python/ray/scripts/scripts.py | 30 ++++++++-- python/ray/tests/test_label_utils.py | 54 +++++++++--------- 3 files changed, 84 insertions(+), 84 deletions(-) diff --git a/python/ray/_private/label_utils.py b/python/ray/_private/label_utils.py index 6727bc556b42..eea5d529ce96 100644 --- a/python/ray/_private/label_utils.py +++ b/python/ray/_private/label_utils.py @@ -13,61 +13,41 @@ LABEL_REGEX = r"^[a-zA-Z0-9]([a-zA-Z0-9_.-]*[a-zA-Z0-9])?$" -def parse_node_labels_string( - labels_str: str, cli_logger, cf, command_arg="--labels" -) -> Dict[str, str]: - try: - labels = {} - if labels_str == "": - return labels - # Labels argument should consist of a string of key=value pairs - # separated by commas. Labels follow Kubernetes label syntax. - label_pairs = labels_str.split(",") - for pair in label_pairs: - # Split each pair by `=` - key_value = pair.split("=") - if len(key_value) != 2: - raise ValueError("Label value is not a key-value pair.") - key = key_value[0].strip() - value = key_value[1].strip() - labels[key] = value - except Exception as e: - cli_logger.abort( - "`{}` is not a valid string of key-value pairs, detail error:{}" - "Valid values look like this: `{}`", - cf.bold(f"{command_arg}={labels_str}"), - str(e), - cf.bold(f'{command_arg}="key1=val1,key2=val2"'), - ) +def parse_node_labels_string(labels_str: str) -> Dict[str, str]: + labels = {} + if labels_str == "": + return labels + # Labels argument should consist of a string of key=value pairs + # separated by commas. Labels follow Kubernetes label syntax. + label_pairs = labels_str.split(",") + for pair in label_pairs: + # Split each pair by `=` + key_value = pair.split("=") + if len(key_value) != 2: + raise ValueError("Label value is not a key-value pair.") + key = key_value[0].strip() + value = key_value[1].strip() + labels[key] = value + return labels -def parse_node_labels_from_yaml_file( - path: str, cli_logger, cf, command_arg="--labels-file" -) -> Dict[str, str]: - try: - if path == "": - return {} - with open(path, "r") as file: - # Expects valid YAML content - labels = yaml.safe_load(file) - if not isinstance(labels, dict): - raise ValueError( - "The format after deserialization is not a key-value pair map" - ) - for key, value in labels.items(): - if not isinstance(key, str): - raise ValueError("The key is not string type.") - if not isinstance(value, str): - raise ValueError(f'The value of the "{key}" is not string type') - except Exception as e: - cli_logger.abort( - "The file at `{}` is not a valid YAML file, detail error:{}" - "Valid values look like this: `{}`", - cf.bold(f"{command_arg}={path}"), - str(e), - cf.bold(f"{command_arg}='gpu_type: A100\nregion: us'"), - ) +def parse_node_labels_from_yaml_file(path: str) -> Dict[str, str]: + if path == "": + return {} + with open(path, "r") as file: + # Expects valid YAML content + labels = yaml.safe_load(file) + if not isinstance(labels, dict): + raise ValueError( + "The format after deserialization is not a key-value pair map." + ) + for key, value in labels.items(): + if not isinstance(key, str): + raise ValueError("The key is not string type.") + if not isinstance(value, str): + raise ValueError(f'The value of "{key}" is not string type.') + return labels diff --git a/python/ray/scripts/scripts.py b/python/ray/scripts/scripts.py index d4898baf4925..d65b5437a1b3 100644 --- a/python/ray/scripts/scripts.py +++ b/python/ray/scripts/scripts.py @@ -25,12 +25,14 @@ import ray import ray._private.ray_constants as ray_constants import ray._private.services as services +from ray._private.label_utils import ( + parse_node_labels_from_yaml_file, + parse_node_labels_string, +) from ray._private.utils import ( check_ray_client_dependencies_installed, load_class, parse_resources_json, - parse_node_labels_from_yaml_file, - parse_node_labels_string, ) from ray._private.internal_api import memory_summary from ray._private.usage import usage_lib @@ -728,10 +730,26 @@ def start( # Compose labels passed in with `--labels` and `--labels-file`. # The label value from `--labels` will overrwite the value of any duplicate keys. - labels_from_file_dict = parse_node_labels_from_yaml_file( - labels_file, cli_logger, cf - ) - labels_from_string = parse_node_labels_string(labels, cli_logger, cf) + try: + labels_from_file_dict = parse_node_labels_from_yaml_file(labels_file) + except Exception as e: + cli_logger.abort( + "The file at `{}` is not a valid YAML file, detailed error:{}" + "Valid values look like this: `{}`", + cf.bold(f"--labels-file={labels_file}"), + str(e), + cf.bold("--labels-file='gpu_type: A100\nregion: us'"), + ) + try: + labels_from_string = parse_node_labels_string(labels) + except Exception as e: + cli_logger.abort( + "`{}` is not a valid string of key-value pairs, detail error:{}" + "Valid values look like this: `{}`", + cf.bold(f"--labels={labels}"), + str(e), + cf.bold('--labels="key1=val1,key2=val2"'), + ) labels_dict = ( {**labels_from_file_dict, **labels_from_string} if labels_from_file_dict diff --git a/python/ray/tests/test_label_utils.py b/python/ray/tests/test_label_utils.py index 412c472ef771..bf5e5aa7b711 100644 --- a/python/ray/tests/test_label_utils.py +++ b/python/ray/tests/test_label_utils.py @@ -1,11 +1,9 @@ import pytest -import click from ray._private.label_utils import ( parse_node_labels_string, parse_node_labels_from_yaml_file, validate_node_labels, ) -from ray.autoscaler._private.cli_logger import cf, cli_logger import sys import tempfile @@ -13,24 +11,24 @@ def test_parse_node_labels_from_string(): # Empty label argument passed labels_string = "" - labels_dict = parse_node_labels_string(labels_string, cli_logger, cf) + labels_dict = parse_node_labels_string(labels_string) assert labels_dict == {} # Valid label key with empty value labels_string = "region=" - labels_dict = parse_node_labels_string(labels_string, cli_logger, cf) + labels_dict = parse_node_labels_string(labels_string) assert labels_dict == {"region": ""} # Multiple valid label keys and values labels_string = "ray.io/accelerator-type=A100,region=us-west4" - labels_dict = parse_node_labels_string(labels_string, cli_logger, cf) + labels_dict = parse_node_labels_string(labels_string) assert labels_dict == {"ray.io/accelerator-type": "A100", "region": "us-west4"} # Invalid label labels_string = "ray.io/accelerator-type=type=A100" - with pytest.raises(click.exceptions.ClickException) as e: - parse_node_labels_string(labels_string, cli_logger, cf) - assert "is not a valid string of key-value pairs" in str(e) + with pytest.raises(ValueError) as e: + parse_node_labels_string(labels_string) + assert "Label value is not a key-value pair" in str(e) def test_parse_node_labels_from_yaml_file(): @@ -38,16 +36,20 @@ def test_parse_node_labels_from_yaml_file(): with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: test_file.write("") test_file.flush() # Ensure data is written - with pytest.raises(click.exceptions.ClickException) as e: - parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) - assert "is not a valid YAML string" in str(e) + with pytest.raises(ValueError) as e: + parse_node_labels_from_yaml_file(test_file.name) + assert "The format after deserialization is not a key-value pair map" in str(e) + + # With non-existent yaml file + with pytest.raises(FileNotFoundError): + parse_node_labels_from_yaml_file("missing-file.yaml") # Valid label key with empty value with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: test_file.write('"ray.io/accelerator-type": ""') test_file.flush() # Ensure data is written - labels_dict = parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) - assert labels_dict == {"ray.io/accelerator-type": ""} + labels_dict = parse_node_labels_from_yaml_file(test_file.name) + assert labels_dict == {"ray.io/accelerator-type": ""} # Multiple valid label keys and values with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: @@ -55,28 +57,28 @@ def test_parse_node_labels_from_yaml_file(): '"ray.io/accelerator-type": "A100"\n"region": "us"\n"market-type": "spot"' ) test_file.flush() # Ensure data is written - labels_dict = parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) - assert labels_dict == { - "ray.io/accelerator-type": "A100", - "region": "us", - "market-type": "spot", - } + labels_dict = parse_node_labels_from_yaml_file(test_file.name) + assert labels_dict == { + "ray.io/accelerator-type": "A100", + "region": "us", + "market-type": "spot", + } # Non-string label key with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: test_file.write('{100: "A100"}') test_file.flush() # Ensure data is written - with pytest.raises(click.exceptions.ClickException) as e: - parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) - assert "is not a valid YAML string" in str(e) + with pytest.raises(ValueError) as e: + parse_node_labels_from_yaml_file(test_file.name) + assert "The key is not string type." in str(e) # Non-string label value with tempfile.NamedTemporaryFile(mode="w+", delete=True) as test_file: - test_file.write('{100: "A100"}') + test_file.write('{"gpu": 100}') test_file.flush() # Ensure data is written - with pytest.raises(click.exceptions.ClickException) as e: - parse_node_labels_from_yaml_file(test_file.name, cli_logger, cf) - assert "is not a valid YAML string" in str(e) + with pytest.raises(ValueError) as e: + parse_node_labels_from_yaml_file(test_file.name) + assert 'The value of "gpu" is not string type' in str(e) def test_validate_node_labels(): From 32f0d9ce84b40228c7f49528cee520ec5ca2c722 Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Wed, 2 Apr 2025 20:14:34 +0000 Subject: [PATCH 11/17] Update --labels help comment Signed-off-by: Ryan O'Leary --- python/ray/scripts/scripts.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/python/ray/scripts/scripts.py b/python/ray/scripts/scripts.py index d65b5437a1b3..e37b4f527d93 100644 --- a/python/ray/scripts/scripts.py +++ b/python/ray/scripts/scripts.py @@ -640,7 +640,9 @@ def debug(address: str, verbose: bool): hidden=True, default="", type=str, - help="a string list of key-value pairs mapping label name to label value.", + help="a string list of key-value pairs mapping label name to label value." + "These values take precedence over conflicting keys passed in from --labels-file." + 'Ex: --labels "key1=val1,key2=val2"', ) @click.option( "--labels-file", From 0c6d430167b31e8eff8d5a9c2bce47f95c3fc54d Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Wed, 2 Apr 2025 20:18:16 +0000 Subject: [PATCH 12/17] Add new test to BUILD file Signed-off-by: Ryan O'Leary --- python/ray/tests/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/python/ray/tests/BUILD b/python/ray/tests/BUILD index fa0982bb7bd1..6ee9838eff72 100644 --- a/python/ray/tests/BUILD +++ b/python/ray/tests/BUILD @@ -349,6 +349,7 @@ py_test_module_list( py_test_module_list( size = "medium", files = [ + "test_label_utils.py", "test_minimal_install.py", "test_runtime_env_ray_minimal.py", "test_utils.py", From 05fb2a20b861414d97fa123c7dc381704137682d Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Thu, 3 Apr 2025 03:11:47 +0000 Subject: [PATCH 13/17] Fix --labels commands in tests Signed-off-by: Ryan O'Leary --- python/ray/_private/label_utils.py | 17 ++++++++++++----- .../test_node_label_scheduling_strategy.py | 2 +- python/ray/tests/test_node_labels.py | 16 +++++++--------- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/python/ray/_private/label_utils.py b/python/ray/_private/label_utils.py index eea5d529ce96..612d99f9072c 100644 --- a/python/ray/_private/label_utils.py +++ b/python/ray/_private/label_utils.py @@ -5,12 +5,19 @@ import ray._private.ray_constants as ray_constants # Regex patterns used to validate that labels conform to Kubernetes label syntax rules. +# https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set + +# Regex for mandatory name (DNS label) or value +# Examples: +# Valid matches: "a", "label-name", "a-._b", "123", "this_is_a_valid_label" +# Invalid matches: "-abc", "abc-", "my@label", "a" * 64 +LABEL_REGEX = re.compile(r"[a-zA-Z0-9]([a-zA-Z0-9_.-]*[a-zA-Z0-9]){0,62}") + # Regex for optional prefix (DNS subdomain) -LABEL_PREFIX_REGEX = ( - r"^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*)$" -) -# Regex for mandatory name (label key without prefix) or value -LABEL_REGEX = r"^[a-zA-Z0-9]([a-zA-Z0-9_.-]*[a-zA-Z0-9])?$" +# Examples: +# Valid matches: "abc", "sub.domain.example", "my-label", "123.456.789" +# Invalid matches: "-abc", "prefix_", "sub..domain", sub.$$.example +LABEL_PREFIX_REGEX = rf"^({LABEL_REGEX.pattern}?(\.{LABEL_REGEX.pattern}?)*)$" def parse_node_labels_string(labels_str: str) -> Dict[str, str]: diff --git a/python/ray/tests/test_node_label_scheduling_strategy.py b/python/ray/tests/test_node_label_scheduling_strategy.py index 83195c6922e9..851153626cc7 100644 --- a/python/ray/tests/test_node_label_scheduling_strategy.py +++ b/python/ray/tests/test_node_label_scheduling_strategy.py @@ -31,7 +31,7 @@ def get_node_id(): @pytest.mark.parametrize( "call_ray_start", - ['ray start --head --labels={"gpu_type":"A100","region":"us"}'], + ['ray start --head --labels "gpu_type=A100, region=us"'], indirect=True, ) def test_node_label_scheduling_basic(call_ray_start): diff --git a/python/ray/tests/test_node_labels.py b/python/ray/tests/test_node_labels.py index 0854286d3bda..5f2da94ab15e 100644 --- a/python/ray/tests/test_node_labels.py +++ b/python/ray/tests/test_node_labels.py @@ -19,7 +19,7 @@ def add_default_labels(node_info, labels): @pytest.mark.parametrize( "call_ray_start", - ['ray start --head --labels={"gpu_type":"A100","region":"us"}'], + ['ray start --head --labels= "gpu_type=A100,region=us"'], indirect=True, ) def test_ray_start_set_node_labels(call_ray_start): @@ -80,19 +80,17 @@ def test_ray_init_set_node_labels_value_error(ray_start_cluster): def test_ray_start_set_node_labels_value_error(): - out = check_cmd_stderr(["ray", "start", "--head", "--labels=xxx"]) - assert "is not a valid JSON string, detail error" in out + out = check_cmd_stderr(["ray", "start", "--head", "--labels", "xxx"]) + assert "is not a valid string of key-value pairs, detail error" in out - out = check_cmd_stderr(["ray", "start", "--head", '--labels={"gpu_type":1}']) - assert 'The value of the "gpu_type" is not string type' in out + out = check_cmd_stderr(["ray", "start", "--head", "--labels", "!gpu_type=1"]) + assert "Invalid label key name `!gpu_type`" in out - out = check_cmd_stderr( - ["ray", "start", "--head", '--labels={"ray.io/node_id":"111"}'] - ) + out = check_cmd_stderr(["ray", "start", "--head", "--labels", "ray.io/node_id=111"]) assert "cannot start with the prefix `ray.io/`" in out out = check_cmd_stderr( - ["ray", "start", "--head", '--labels={"ray.io/other_key":"111"}'] + ["ray", "start", "--head", "--labels", "ray.io/other_key=111"] ) assert "cannot start with the prefix `ray.io/`" in out From 7cd9849371fffa985fa74ae97bbd6f8580f5dcb6 Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Thu, 3 Apr 2025 12:20:09 +0000 Subject: [PATCH 14/17] Fix label utils test Signed-off-by: Ryan O'Leary --- python/ray/tests/test_label_utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ray/tests/test_label_utils.py b/python/ray/tests/test_label_utils.py index bf5e5aa7b711..328a2594602b 100644 --- a/python/ray/tests/test_label_utils.py +++ b/python/ray/tests/test_label_utils.py @@ -89,7 +89,7 @@ def test_validate_node_labels(): assert "This is reserved for Ray defined labels." in str(e) # Invalid key prefix syntax - labels_dict = {"invalidPrefix/accelerator-type": "A100"} + labels_dict = {"!invalidPrefix/accelerator-type": "A100"} with pytest.raises(ValueError) as e: validate_node_labels(labels_dict) assert "Invalid label key prefix" in str(e) From 0e19c3d52b16f9ee96c14f100704dc10ddf4bfff Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Thu, 3 Apr 2025 12:38:11 +0000 Subject: [PATCH 15/17] Fix java test Signed-off-by: Ryan O'Leary --- .../test/src/main/java/io/ray/test/NodeLabelSchedulingTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/test/src/main/java/io/ray/test/NodeLabelSchedulingTest.java b/java/test/src/main/java/io/ray/test/NodeLabelSchedulingTest.java index b25457e87d6e..e321ecd9984f 100644 --- a/java/test/src/main/java/io/ray/test/NodeLabelSchedulingTest.java +++ b/java/test/src/main/java/io/ray/test/NodeLabelSchedulingTest.java @@ -24,7 +24,7 @@ public void testEmptyNodeLabels() { } public void testSetNodeLabels() { - System.setProperty("ray.head-args.0", "--labels={\"gpu_type\":\"A100\",\"azone\":\"azone-1\"}"); + System.setProperty("ray.head-args.0", "--labels=\"gpu_type=A100,azone=azone-1\""); try { Ray.init(); List nodeInfos = Ray.getRuntimeContext().getAllNodeInfo(); From abe5c555ce304f8fada80d709c0b55f31049e696 Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Thu, 3 Apr 2025 19:14:17 +0000 Subject: [PATCH 16/17] Fix --labels commands in tests Signed-off-by: Ryan O'Leary --- .../test/src/main/java/io/ray/test/NodeLabelSchedulingTest.java | 2 +- python/ray/tests/test_node_label_scheduling_strategy.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/java/test/src/main/java/io/ray/test/NodeLabelSchedulingTest.java b/java/test/src/main/java/io/ray/test/NodeLabelSchedulingTest.java index e321ecd9984f..3585a9597fb9 100644 --- a/java/test/src/main/java/io/ray/test/NodeLabelSchedulingTest.java +++ b/java/test/src/main/java/io/ray/test/NodeLabelSchedulingTest.java @@ -24,7 +24,7 @@ public void testEmptyNodeLabels() { } public void testSetNodeLabels() { - System.setProperty("ray.head-args.0", "--labels=\"gpu_type=A100,azone=azone-1\""); + System.setProperty("ray.head-args.0", "--labels gpu_type=A100,azone=azone-1"); try { Ray.init(); List nodeInfos = Ray.getRuntimeContext().getAllNodeInfo(); diff --git a/python/ray/tests/test_node_label_scheduling_strategy.py b/python/ray/tests/test_node_label_scheduling_strategy.py index 851153626cc7..8ed19dfba377 100644 --- a/python/ray/tests/test_node_label_scheduling_strategy.py +++ b/python/ray/tests/test_node_label_scheduling_strategy.py @@ -31,7 +31,7 @@ def get_node_id(): @pytest.mark.parametrize( "call_ray_start", - ['ray start --head --labels "gpu_type=A100, region=us"'], + ["ray start --head --labels gpu_type=A100,region=us"], indirect=True, ) def test_node_label_scheduling_basic(call_ray_start): From 549f78d251f62139dff248567cc07412c9dd75b5 Mon Sep 17 00:00:00 2001 From: Ryan O'Leary Date: Fri, 4 Apr 2025 02:35:43 +0000 Subject: [PATCH 17/17] Parse out leading/trailing quotes Signed-off-by: Ryan O'Leary --- .../src/main/java/io/ray/test/NodeLabelSchedulingTest.java | 2 +- python/ray/_private/label_utils.py | 6 ++++++ python/ray/tests/test_node_labels.py | 4 ++-- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/java/test/src/main/java/io/ray/test/NodeLabelSchedulingTest.java b/java/test/src/main/java/io/ray/test/NodeLabelSchedulingTest.java index 3585a9597fb9..e321ecd9984f 100644 --- a/java/test/src/main/java/io/ray/test/NodeLabelSchedulingTest.java +++ b/java/test/src/main/java/io/ray/test/NodeLabelSchedulingTest.java @@ -24,7 +24,7 @@ public void testEmptyNodeLabels() { } public void testSetNodeLabels() { - System.setProperty("ray.head-args.0", "--labels gpu_type=A100,azone=azone-1"); + System.setProperty("ray.head-args.0", "--labels=\"gpu_type=A100,azone=azone-1\""); try { Ray.init(); List nodeInfos = Ray.getRuntimeContext().getAllNodeInfo(); diff --git a/python/ray/_private/label_utils.py b/python/ray/_private/label_utils.py index 612d99f9072c..b4ac0d96fa3a 100644 --- a/python/ray/_private/label_utils.py +++ b/python/ray/_private/label_utils.py @@ -22,8 +22,14 @@ def parse_node_labels_string(labels_str: str) -> Dict[str, str]: labels = {} + + # Remove surrounding quotes if they exist + if len(labels_str) > 1 and labels_str.startswith('"') and labels_str.endswith('"'): + labels_str = labels_str[1:-1] + if labels_str == "": return labels + # Labels argument should consist of a string of key=value pairs # separated by commas. Labels follow Kubernetes label syntax. label_pairs = labels_str.split(",") diff --git a/python/ray/tests/test_node_labels.py b/python/ray/tests/test_node_labels.py index 5f2da94ab15e..850263d3b532 100644 --- a/python/ray/tests/test_node_labels.py +++ b/python/ray/tests/test_node_labels.py @@ -19,7 +19,7 @@ def add_default_labels(node_info, labels): @pytest.mark.parametrize( "call_ray_start", - ['ray start --head --labels= "gpu_type=A100,region=us"'], + ['ray start --head --labels "gpu_type=A100,region=us"'], indirect=True, ) def test_ray_start_set_node_labels(call_ray_start): @@ -33,7 +33,7 @@ def test_ray_start_set_node_labels(call_ray_start): @pytest.mark.parametrize( "call_ray_start", [ - "ray start --head --labels={}", + 'ray start --head --labels ""', ], indirect=True, )