diff --git a/ci/raydepsets/cli.py b/ci/raydepsets/cli.py index 7136a757cdce..f99bc886ca57 100644 --- a/ci/raydepsets/cli.py +++ b/ci/raydepsets/cli.py @@ -177,7 +177,9 @@ def _build(self): for depset_name in depset.depsets: self.build_graph.add_edge(depset_name, depset.name) else: - raise ValueError(f"Invalid operation: {depset.operation}") + raise ValueError( + f"Invalid operation: {depset.operation} for depset {depset.name} in config {depset.config_name}" + ) if depset.pre_hooks: for ind, hook in enumerate(depset.pre_hooks): hook_name = f"{depset.name}_pre_hook_{ind+1}" @@ -232,24 +234,27 @@ def exec_uv_cmd( self, cmd: str, args: List[str], stdin: Optional[bytes] = None ) -> str: cmd = [self._uv_binary, "pip", cmd, *args] - click.echo(f"Executing command: {cmd}") - status = subprocess.run(cmd, cwd=self.workspace.dir, input=stdin) + click.echo(f"Executing command: {' '.join(cmd)}") + status = subprocess.run( + cmd, cwd=self.workspace.dir, input=stdin, capture_output=True + ) if status.returncode != 0: - raise RuntimeError(f"Failed to execute command: {cmd}") - return status.stdout + raise RuntimeError( + f"Failed to execute command: {' '.join(cmd)} with error: {status.stderr.decode('utf-8')}" + ) + return status.stdout.decode("utf-8") def execute_pre_hook(self, pre_hook: str): status = subprocess.run( shlex.split(pre_hook), cwd=self.workspace.dir, capture_output=True, - text=True, ) if status.returncode != 0: raise RuntimeError( - f"Failed to execute pre_hook {pre_hook} with error: {status.stderr}", + f"Failed to execute pre_hook {pre_hook} with error: {status.stderr.decode('utf-8')}", ) - click.echo(f"{status.stdout}") + click.echo(f"{status.stdout.decode('utf-8')}") click.echo(f"Executed pre_hook {pre_hook} successfully") def execute_depset(self, depset: Depset): @@ -379,7 +384,7 @@ def check_subset_exists(self, source_depset: Depset, requirements: List[str]): for req in requirements: if req not in source_depset.requirements: raise RuntimeError( - f"Requirement {req} is not a subset of {source_depset.name}" + f"Requirement {req} is not a subset of {source_depset.name} in config {source_depset.config_name}" ) def get_expanded_depset_requirements( diff --git a/ci/raydepsets/tests/test_cli.py b/ci/raydepsets/tests/test_cli.py index 74c7537bbc21..ae349d6bba42 100644 --- a/ci/raydepsets/tests/test_cli.py +++ b/ci/raydepsets/tests/test_cli.py @@ -27,6 +27,7 @@ replace_in_file, save_file_as, save_packages_to_file, + write_to_config_file, ) from ci.raydepsets.workspace import ( Depset, @@ -67,20 +68,6 @@ def _invoke_build(tmpdir: str, config_path: str, name: Optional[str] = None): ) -def _write_to_config_file(tmpdir: str, depset: Depset, config_name: str): - with open(Path(tmpdir) / config_name, "w") as f: - f.write( - f""" -depsets: - - name: {depset.name} - operation: {depset.operation} - {f"constraints: {depset.constraints}" if depset.constraints else ""} - {f"requirements: {depset.requirements}" if depset.requirements else ""} - output: {depset.output} - """ - ) - - class TestCli(unittest.TestCase): def test_cli_load_fail_no_config(self): with tempfile.TemporaryDirectory() as tmpdir: @@ -99,13 +86,6 @@ def test_dependency_set_manager_init(self): assert len(manager.config.depsets) > 0 assert len(manager.build_graph.nodes) > 0 - def test_get_depset(self): - with tempfile.TemporaryDirectory() as tmpdir: - copy_data_to_tmpdir(tmpdir) - manager = _create_test_manager(tmpdir) - with self.assertRaises(KeyError): - _get_depset(manager.config.depsets, "fake_depset") - def test_uv_binary_exists(self): assert _uv_binary() is not None @@ -248,7 +228,7 @@ def test_subset_does_not_exist(self): output="requirements_compiled_general.txt", ) - with self.assertRaises(RuntimeError): + with self.assertRaises(RuntimeError) as e: manager.subset( source_depset="general_depset__py311_cpu", requirements=["requirements_compiled_test.txt"], @@ -256,6 +236,10 @@ def test_subset_does_not_exist(self): name="subset_general_depset__py311_cpu", output="requirements_compiled_subset_general.txt", ) + assert ( + "Requirement requirements_compiled_test.txt is not a subset of general_depset__py311_cpu in config test.depsets.yaml" + in str(e.exception) + ) def test_check_if_subset_exists(self): with tempfile.TemporaryDirectory() as tmpdir: @@ -271,23 +255,28 @@ def test_check_if_subset_exists(self): override_flags=[], config_name="test.depsets.yaml", ) - with self.assertRaises(RuntimeError): + with self.assertRaises(RuntimeError) as e: manager.check_subset_exists( source_depset=source_depset, requirements=["requirements_3.txt"], ) + assert ( + "Requirement requirements_3.txt is not a subset of general_depset__py311_cpu in config test.depsets.yaml" + in str(e.exception) + ) def test_compile_bad_requirements(self): with tempfile.TemporaryDirectory() as tmpdir: copy_data_to_tmpdir(tmpdir) manager = _create_test_manager(tmpdir) - with self.assertRaises(RuntimeError): + with self.assertRaises(RuntimeError) as e: manager.compile( constraints=[], requirements=["requirements_test_bad.txt"], name="general_depset", output="requirements_compiled_general.txt", ) + assert "File not found: `requirements_test_bad.txt" in str(e.exception) def test_get_path(self): with tempfile.TemporaryDirectory() as tmpdir: @@ -436,9 +425,13 @@ def test_build_graph_bad_operation(self): output="requirements_compiled_invalid_op.txt", config_name="test.depsets.yaml", ) - _write_to_config_file(tmpdir, depset, "test.depsets.yaml") - with self.assertRaises(ValueError): + write_to_config_file(tmpdir, depset, "test.depsets.yaml") + with self.assertRaises(ValueError) as e: _create_test_manager(tmpdir) + assert ( + "Invalid operation: invalid_op for depset invalid_op_depset in config test.depsets.yaml" + in str(e.exception) + ) def test_execute(self): with tempfile.TemporaryDirectory() as tmpdir: @@ -459,8 +452,9 @@ def test_execute_single_depset_that_does_not_exist(self): with tempfile.TemporaryDirectory() as tmpdir: copy_data_to_tmpdir(tmpdir) manager = _create_test_manager(tmpdir) - with self.assertRaises(KeyError): + with self.assertRaises(KeyError) as e: manager.execute(single_depset_name="fake_depset") + assert "Dependency set fake_depset not found" in str(e.exception) def test_expand(self): with tempfile.TemporaryDirectory() as tmpdir: @@ -565,16 +559,6 @@ def test_get_depset_without_build_arg_set(self): depset = _get_depset(manager.config.depsets, "ray_base_test_depset") assert depset.name == "ray_base_test_depset" - def test_get_depset_with_build_arg_set_and_no_build_arg_set_provided(self): - with tempfile.TemporaryDirectory() as tmpdir: - copy_data_to_tmpdir(tmpdir) - manager = DependencySetManager( - config_path="test.depsets.yaml", - workspace_dir=tmpdir, - ) - with self.assertRaises(KeyError): - _get_depset(manager.config.depsets, "build_args_test_depset_py311") - def test_execute_single_pre_hook(self): with tempfile.TemporaryDirectory() as tmpdir: copy_data_to_tmpdir(tmpdir) @@ -608,7 +592,7 @@ def test_copy_lock_files_to_temp_dir(self): output="requirements_compiled_test.txt", config_name="test.depsets.yaml", ) - _write_to_config_file(tmpdir, depset, "test.depsets.yaml") + write_to_config_file(tmpdir, depset, "test.depsets.yaml") save_file_as( Path(tmpdir) / "requirements_compiled_test.txt", Path(tmpdir) / "requirements_compiled.txt", @@ -637,7 +621,7 @@ def test_diff_lock_files_out_of_date(self): output="requirements_compiled_test.txt", config_name="test.depsets.yaml", ) - _write_to_config_file(tmpdir, depset, "test.depsets.yaml") + write_to_config_file(tmpdir, depset, "test.depsets.yaml") manager = _create_test_manager(tmpdir, check=True) manager.compile( constraints=["requirement_constraints_test.txt"], @@ -672,7 +656,7 @@ def test_diff_lock_files_up_to_date(self): output="requirements_compiled_test.txt", config_name="test.depsets.yaml", ) - _write_to_config_file(tmpdir, depset, "test.depsets.yaml") + write_to_config_file(tmpdir, depset, "test.depsets.yaml") manager = _create_test_manager(tmpdir, check=True) manager.compile( constraints=["requirement_constraints_test.txt"], diff --git a/ci/raydepsets/tests/test_workspace.py b/ci/raydepsets/tests/test_workspace.py index 069d308eddff..b1102604f9de 100644 --- a/ci/raydepsets/tests/test_workspace.py +++ b/ci/raydepsets/tests/test_workspace.py @@ -1,11 +1,21 @@ import sys import tempfile +import unittest from pathlib import Path import pytest -from ci.raydepsets.tests.utils import copy_data_to_tmpdir, get_depset_by_name -from ci.raydepsets.workspace import BuildArgSet, Workspace, _substitute_build_args +from ci.raydepsets.tests.utils import ( + copy_data_to_tmpdir, + get_depset_by_name, + write_to_config_file, +) +from ci.raydepsets.workspace import ( + BuildArgSet, + Depset, + Workspace, + _substitute_build_args, +) def test_workspace_init(): @@ -134,5 +144,31 @@ def test_get_configs_dir(): assert f"{tmpdir}/test2.depsets.yaml" in configs_dir +def test_invalid_build_arg_set_in_config(): + with tempfile.TemporaryDirectory() as tmpdir: + copy_data_to_tmpdir(tmpdir) + depset = Depset( + name="invalid_build_arg_set", + operation="compile", + requirements=["requirements_test.txt"], + output="requirements_compiled_invalid_build_arg_set.txt", + config_name="test.depsets.yaml", + ) + write_to_config_file( + tmpdir, + depset, + "test.depsets.yaml", + build_arg_sets=["invalid_build_arg_set"], + ) + workspace = Workspace(dir=tmpdir) + with unittest.TestCase().assertRaises(KeyError) as e: + workspace.load_config(config_path=Path(tmpdir) / "test.depsets.yaml") + print(str(e.exception)) + assert ( + "Build arg set invalid_build_arg_set not found in config test.depsets.yaml" + in str(e.exception) + ) + + if __name__ == "__main__": sys.exit(pytest.main(["-v", __file__])) diff --git a/ci/raydepsets/tests/utils.py b/ci/raydepsets/tests/utils.py index 3179b68ddb8f..caba8a210047 100644 --- a/ci/raydepsets/tests/utils.py +++ b/ci/raydepsets/tests/utils.py @@ -1,10 +1,13 @@ """Shared test utilities for raydepsets tests.""" import shutil -from typing import Optional +from pathlib import Path +from typing import List, Optional import runfiles +from ci.raydepsets.workspace import Depset + _REPO_NAME = "io_ray" _runfiles = runfiles.Create() @@ -51,3 +54,27 @@ def get_depset_by_name(depsets, name): for depset in depsets: if depset.name == name: return depset + + +def write_to_config_file( + tmpdir: str, depset: Depset, config_name: str, build_arg_sets: List[str] = None +): + with open(Path(tmpdir) / config_name, "w") as f: + f.write( + f""" +depsets: + - name: {depset.name} + operation: {depset.operation} + {f"constraints: {depset.constraints}" if depset.constraints else ""} + {f"requirements: {depset.requirements}" if depset.requirements else ""} + output: {depset.output} + {f"pre_hooks: {depset.pre_hooks}" if depset.pre_hooks else ""} + {f"depsets: {depset.depsets}" if depset.depsets else ""} + {f"source_depset: {depset.source_depset}" if depset.source_depset else ""} + {f"config_name: {depset.config_name}" if depset.config_name else ""} + {f"append_flags: {depset.append_flags}" if depset.append_flags else ""} + {f"override_flags: {depset.override_flags}" if depset.override_flags else ""} + {f"packages: {depset.packages}" if depset.packages else ""} + {f"build_arg_sets: {build_arg_sets}" if build_arg_sets else ""} + """ + ) diff --git a/ci/raydepsets/workspace.py b/ci/raydepsets/workspace.py index 8794a2939350..c156bdb36a40 100644 --- a/ci/raydepsets/workspace.py +++ b/ci/raydepsets/workspace.py @@ -72,9 +72,12 @@ def from_dict(cls, data: dict, config_name: str) -> "Config": if build_arg_set_keys: # Expand the depset for each build arg set for build_arg_set_key in build_arg_set_keys: - build_arg_set = build_arg_sets[build_arg_set_key] - if build_arg_set is None: - raise KeyError(f"Build arg set {build_arg_set_key} not found") + try: + build_arg_set = build_arg_sets[build_arg_set_key] + except KeyError: + raise KeyError( + f"Build arg set {build_arg_set_key} not found in config {config_name}" + ) depset_yaml = _substitute_build_args(depset, build_arg_set) depsets.append(_dict_to_depset(depset_yaml, config_name)) else: