-
Notifications
You must be signed in to change notification settings - Fork 6.9k
[deps] raydepsets import all config (2/3) #57584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9095f31
0d24b1d
400c47f
b6e837c
c8dc2a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
build_arg_sets: | ||
py311_cpu: | ||
CUDA_VERSION: cpu | ||
PYTHON_VERSION: py311 | ||
|
||
depsets: | ||
- name: other_config_depset | ||
operation: expand | ||
depsets: | ||
- expand_general_depset__${PYTHON_VERSION}_${CUDA_VERSION} | ||
- expanded_depset__${PYTHON_VERSION}_${CUDA_VERSION} | ||
output: requirements_compiled_other_config.txt | ||
build_arg_sets: | ||
- py311_cpu | ||
- name: pre_hook_test_depset | ||
operation: compile | ||
requirements: | ||
- requirements_test.txt | ||
output: requirements_compiled_pre_hook.txt | ||
pre_hooks: | ||
- pre-hook-test.sh | ||
- name: pre_hook_invalid_test_depset | ||
operation: compile | ||
requirements: | ||
- requirements_test.txt | ||
output: requirements_compiled_pre_hook_invalid.txt | ||
pre_hooks: | ||
- pre-hook-error-test.sh |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -18,15 +18,14 @@ def test_parse_build_arg_sets(): | |||||||||||||
with tempfile.TemporaryDirectory() as tmpdir: | ||||||||||||||
copy_data_to_tmpdir(tmpdir) | ||||||||||||||
workspace = Workspace(dir=tmpdir) | ||||||||||||||
config = workspace.load_config(path=Path(tmpdir) / "test.depsets.yaml") | ||||||||||||||
assert config.build_arg_sets["py311_cpu"].build_args == { | ||||||||||||||
"CUDA_VERSION": "cpu", | ||||||||||||||
"PYTHON_VERSION": "py311", | ||||||||||||||
} | ||||||||||||||
assert config.build_arg_sets["py311_cuda128"].build_args == { | ||||||||||||||
"CUDA_VERSION": 128, | ||||||||||||||
"PYTHON_VERSION": "py311", | ||||||||||||||
} | ||||||||||||||
config = workspace.load_config(config_path=Path(tmpdir) / "test.depsets.yaml") | ||||||||||||||
assert "general_depset__py311_cpu" in [depset.name for depset in config.depsets] | ||||||||||||||
assert "build_args_test_depset__py311_cpu" in [ | ||||||||||||||
depset.name for depset in config.depsets | ||||||||||||||
] | ||||||||||||||
assert "expanded_depset__py311_cpu" in [ | ||||||||||||||
depset.name for depset in config.depsets | ||||||||||||||
] | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def test_substitute_build_args(): | ||||||||||||||
|
@@ -65,17 +64,78 @@ def test_invalid_build_arg_set(): | |||||||||||||
) | ||||||||||||||
with pytest.raises(KeyError): | ||||||||||||||
workspace = Workspace(dir=tmpdir) | ||||||||||||||
workspace.load_config(path=Path(tmpdir) / "test.depsets.yaml") | ||||||||||||||
workspace.load_config(config_path=Path(tmpdir) / "test.depsets.yaml") | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def test_parse_pre_hooks(): | ||||||||||||||
with tempfile.TemporaryDirectory() as tmpdir: | ||||||||||||||
copy_data_to_tmpdir(tmpdir) | ||||||||||||||
workspace = Workspace(dir=tmpdir) | ||||||||||||||
config = workspace.load_config(path=Path(tmpdir) / "test.depsets.yaml") | ||||||||||||||
config = workspace.load_config(config_path=Path(tmpdir) / "test2.depsets.yaml") | ||||||||||||||
pre_hook_depset = get_depset_by_name(config.depsets, "pre_hook_test_depset") | ||||||||||||||
assert pre_hook_depset.pre_hooks == ["pre-hook-test.sh"] | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def test_load_first_config(): | ||||||||||||||
with tempfile.TemporaryDirectory() as tmpdir: | ||||||||||||||
copy_data_to_tmpdir(tmpdir) | ||||||||||||||
workspace = Workspace(dir=tmpdir) | ||||||||||||||
config = workspace.load_config(config_path=Path(tmpdir) / "test.depsets.yaml") | ||||||||||||||
assert config.depsets is not None | ||||||||||||||
assert len(config.depsets) == 8 | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def test_load_second_config(): | ||||||||||||||
with tempfile.TemporaryDirectory() as tmpdir: | ||||||||||||||
copy_data_to_tmpdir(tmpdir) | ||||||||||||||
workspace = Workspace(dir=tmpdir) | ||||||||||||||
config = workspace.load_config(config_path=Path(tmpdir) / "test2.depsets.yaml") | ||||||||||||||
assert config.depsets is not None | ||||||||||||||
assert len(config.depsets) == 3 | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
# load all configs should always load all depsets | ||||||||||||||
def test_load_all_configs_first_config(): | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests Example: import pytest
@pytest.mark.parametrize("config_file", ["test.depsets.yaml", "test2.depsets.yaml"])
def test_load_all_configs(config_file):
with tempfile.TemporaryDirectory() as tmpdir:
copy_data_to_tmpdir(tmpdir)
workspace = Workspace(dir=tmpdir)
config = workspace.load_configs(config_path=Path(tmpdir) / config_file)
assert config.depsets is not None
assert len(config.depsets) == 11 |
||||||||||||||
with tempfile.TemporaryDirectory() as tmpdir: | ||||||||||||||
copy_data_to_tmpdir(tmpdir) | ||||||||||||||
workspace = Workspace(dir=tmpdir) | ||||||||||||||
config = workspace.load_configs(config_path=Path(tmpdir) / "test.depsets.yaml") | ||||||||||||||
assert config.depsets is not None | ||||||||||||||
assert len(config.depsets) == 11 | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
# load all configs should always load all depsets | ||||||||||||||
def test_load_all_configs_second_config(): | ||||||||||||||
with tempfile.TemporaryDirectory() as tmpdir: | ||||||||||||||
copy_data_to_tmpdir(tmpdir) | ||||||||||||||
workspace = Workspace(dir=tmpdir) | ||||||||||||||
config = workspace.load_configs(config_path=Path(tmpdir) / "test2.depsets.yaml") | ||||||||||||||
assert config.depsets is not None | ||||||||||||||
assert len(config.depsets) == 11 | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def test_merge_configs(): | ||||||||||||||
with tempfile.TemporaryDirectory() as tmpdir: | ||||||||||||||
copy_data_to_tmpdir(tmpdir) | ||||||||||||||
workspace = Workspace(dir=tmpdir) | ||||||||||||||
config = workspace.load_config(config_path=Path(tmpdir) / "test.depsets.yaml") | ||||||||||||||
config2 = workspace.load_config(config_path=Path(tmpdir) / "test2.depsets.yaml") | ||||||||||||||
merged_config = workspace.merge_configs([config, config2]) | ||||||||||||||
assert merged_config.depsets is not None | ||||||||||||||
assert len(merged_config.depsets) == 11 | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def test_get_configs_dir(): | ||||||||||||||
with tempfile.TemporaryDirectory() as tmpdir: | ||||||||||||||
copy_data_to_tmpdir(tmpdir) | ||||||||||||||
workspace = Workspace(dir=tmpdir) | ||||||||||||||
configs_dir = workspace.get_configs_dir( | ||||||||||||||
configs_path=Path(tmpdir) / "test.depsets.yaml" | ||||||||||||||
) | ||||||||||||||
assert len(configs_dir) == 2 | ||||||||||||||
assert f"{tmpdir}/test.depsets.yaml" in configs_dir | ||||||||||||||
assert f"{tmpdir}/test2.depsets.yaml" in configs_dir | ||||||||||||||
Comment on lines
+136
to
+137
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a couple of issues with these assertions:
Suggested change
|
||||||||||||||
|
||||||||||||||
|
||||||||||||||
if __name__ == "__main__": | ||||||||||||||
sys.exit(pytest.main(["-v", __file__])) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ class Depset: | |
name: str | ||
operation: str | ||
output: str | ||
config_name: str | ||
constraints: Optional[List[str]] = None | ||
override_flags: Optional[List[str]] = None | ||
append_flags: Optional[List[str]] = None | ||
|
@@ -40,7 +41,7 @@ def _substitute_build_args(obj: Any, build_arg_set: BuildArgSet): | |
return obj | ||
|
||
|
||
def _dict_to_depset(depset: dict) -> Depset: | ||
def _dict_to_depset(depset: dict, config_name: str) -> Depset: | ||
return Depset( | ||
name=depset.get("name"), | ||
requirements=depset.get("requirements", []), | ||
|
@@ -53,16 +54,16 @@ def _dict_to_depset(depset: dict) -> Depset: | |
append_flags=depset.get("append_flags", []), | ||
pre_hooks=depset.get("pre_hooks", []), | ||
packages=depset.get("packages", []), | ||
config_name=config_name, | ||
) | ||
|
||
|
||
@dataclass | ||
class Config: | ||
depsets: List[Depset] = field(default_factory=list) | ||
build_arg_sets: Dict[str, BuildArgSet] = field(default_factory=dict) | ||
|
||
@classmethod | ||
def from_dict(cls, data: dict) -> "Config": | ||
def from_dict(cls, data: dict, config_name: str) -> "Config": | ||
build_arg_sets = cls.parse_build_arg_sets(data.get("build_arg_sets", {})) | ||
raw_depsets = data.get("depsets", []) | ||
depsets = [] | ||
|
@@ -75,10 +76,10 @@ def from_dict(cls, data: dict) -> "Config": | |
if build_arg_set is None: | ||
raise KeyError(f"Build arg set {build_arg_set_key} not found") | ||
depset_yaml = _substitute_build_args(depset, build_arg_set) | ||
depsets.append(_dict_to_depset(depset_yaml)) | ||
depsets.append(_dict_to_depset(depset_yaml, config_name)) | ||
else: | ||
depsets.append(_dict_to_depset(depset)) | ||
return Config(depsets=depsets, build_arg_sets=build_arg_sets) | ||
depsets.append(_dict_to_depset(depset, config_name)) | ||
return Config(depsets=depsets) | ||
|
||
@staticmethod | ||
def parse_build_arg_sets(build_arg_sets: Dict[str, dict]) -> Dict[str, BuildArgSet]: | ||
|
@@ -98,7 +99,29 @@ def __init__(self, dir: str = None): | |
if self.dir is None: | ||
raise RuntimeError("BUILD_WORKSPACE_DIRECTORY is not set") | ||
|
||
def load_config(self, path: str) -> Config: | ||
with open(os.path.join(self.dir, path), "r") as f: | ||
def load_configs(self, config_path: str = None) -> Config: | ||
merged_configs = self.merge_configs(self.get_all_configs(config_path)) | ||
return merged_configs | ||
|
||
def get_all_configs(self, config_path: str = None) -> List[Config]: | ||
return [self.load_config(path) for path in self.get_configs_dir(config_path)] | ||
|
||
def get_configs_dir(self, configs_path: str) -> List[str]: | ||
configs_dir = os.path.dirname(os.path.join(self.dir, configs_path)) | ||
return [ | ||
os.path.join(self.dir, configs_dir, path) | ||
for path in os.listdir(os.path.join(self.dir, configs_dir)) | ||
if path.endswith(".depsets.yaml") | ||
] | ||
Comment on lines
+109
to
+115
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The implementation of A clearer approach would be to resolve the directory path once and then use it to find and construct the full paths of the config files. This would also simplify the def get_configs_dir(self, configs_path: str) -> List[str]:
if not os.path.isabs(configs_path):
configs_path = os.path.join(self.dir, configs_path)
configs_dir = os.path.dirname(configs_path)
return [
os.path.join(configs_dir, path)
for path in os.listdir(configs_dir)
if path.endswith(".depsets.yaml")
] |
||
|
||
def load_config(self, config_path: str) -> Config: | ||
with open(os.path.join(self.dir, config_path), "r") as f: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since with open(config_path, "r") as f: |
||
data = yaml.safe_load(f) | ||
return Config.from_dict(data) | ||
config_name = os.path.basename(config_path) | ||
config = Config.from_dict(data, config_name) | ||
return config | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Path Construction Error Causes File Load FailureThe |
||
|
||
def merge_configs(self, configs: List[Config]) -> Config: | ||
return Config( | ||
depsets=[depset for config in configs for depset in config.depsets] | ||
) | ||
Comment on lines
+124
to
+127
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current implementation of To prevent this, you should check for and disallow duplicate depset names across all configuration files. This will make the behavior deterministic and prevent subtle bugs. def merge_configs(self, configs: List[Config]) -> Config:
all_depsets = [depset for config in configs for depset in config.depsets]
depset_names = [depset.name for depset in all_depsets]
if len(depset_names) != len(set(depset_names)):
from collections import Counter
duplicates = [
name for name, count in Counter(depset_names).items() if count > 1
]
raise ValueError(f"Duplicate depset names found: {duplicates}")
return Config(depsets=all_depsets) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test implementation can be made more efficient and readable.
[depset.name for depset in config.depsets]
is created three times. It's better to compute it once.set
for membership testing (in
) is more efficient than alist
.Also, the test name
test_parse_build_arg_sets
is now misleading since the test's focus has changed from testingbuild_arg_sets
parsing to verifying the outcome ofload_config
. Consider renaming it to something liketest_load_config_with_build_args
for clarity.