Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions src/cloudai/models/scenario.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,14 @@ class TestRunModel(BaseModel):

id: str = Field(min_length=1)
test_name: Optional[str] = None
path: Optional[str] = Field(
default=None,
min_length=1,
description=(
"Path to a test TOML file, resolved relative to this scenario file's own directory. "
"Alternative to 'test_name': references a test by file location instead of by name."
),
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
num_nodes: int | list[int] | None = None
nodes: list[str] = Field(default_factory=list)
exclude_nodes: list[str] = Field(
Expand Down Expand Up @@ -126,14 +134,17 @@ def tdef_model_dump(self, by_alias: bool) -> dict:

@model_validator(mode="after")
def check_test_name_or_type_is_set(self):
has_base = self.test_name is not None
if self.test_name is not None and self.path is not None:
raise ValueError("'test_name' and 'path' must not both be set; use only one to reference a test.")

has_base = self.test_name is not None or self.path is not None
if not has_base and (self.test_template_name is None or self.name is None or self.description is None):
raise ValueError(
"When 'test_name' is not set, the following fields must be set: "
"When neither 'test_name' nor 'path' is set, the following fields must be set: "
"'test_template_name', 'name', 'description'."
)

if not self.test_name:
if not has_base:
if not self.test_template_name:
raise ValueError("'test_template_name' must be set if 'test_name' is not set.")

Expand All @@ -144,8 +155,8 @@ def check_test_name_or_type_is_set(self):
f"Possible values are: {', '.join(registry.test_definitions_map.keys())}"
)
else:
if self.test_template_name:
raise ValueError("'test_template_name' must not be set if 'test_name' is set.")
if self.test_template_name is not None:
raise ValueError("'test_template_name' must not be set if 'test_name' or 'path' is set.")

return self

Expand Down
17 changes: 15 additions & 2 deletions src/cloudai/test_scenario_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
)
from .models.scenario import TestRunModel, TestScenarioModel
from .models.workload import TestDefinition
from .test_parser import TestParser
from .test_parser import TestParser, load_test_toml_file
from .toml_utils import format_toml_decode_error


Expand Down Expand Up @@ -228,12 +228,25 @@ def _prepare_tdef(self, test_info: TestRunModel) -> TestDefinition:
tc_defined = test_info.tdef_model_dump(by_alias=True)
merged_data = deep_merge(test_defined, tc_defined)
test = tp.load_test_definition(merged_data)
elif test_info.path: # test referenced by file path, relative to this scenario file's directory
resolved_path = (self.file_path.parent / test_info.path).resolve()
if not resolved_path.is_file():
raise TestScenarioParsingError(
f"Test case '{test_info.id}' references path '{test_info.path}', "
f"which resolves to '{resolved_path}', but that file does not exist."
)
tp.current_file = resolved_path
with resolved_path.open() as fh:
test_defined = load_test_toml_file(fh, resolved_path)
tc_defined = test_info.tdef_model_dump(by_alias=True)
merged_data = deep_merge(test_defined, tc_defined)
test = tp.load_test_definition(merged_data)
elif test_info.test_template_name: # test fully defined in the scenario
test = tp._parse_data(test_info.tdef_model_dump(by_alias=True))
else:
# this should never happen, because we check for this in the modelvalidator
raise ValueError(
f"Cannot configure test case '{test_info.id}' with both 'test_name' and 'test_template_name'."
f"Test case '{test_info.id}' has none of 'test_name', 'path', or 'test_template_name' set."
)

return test
Expand Down
119 changes: 116 additions & 3 deletions tests/test_test_scenario.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
TestRun,
TestScenario,
TestScenarioParser,
TestScenarioParsingError,
)
from cloudai.models.scenario import TestRunModel, TestScenarioModel
from cloudai.report_generator.training import TrainingReportGenerationStrategy
Expand Down Expand Up @@ -322,7 +323,8 @@ def test_without_base(self, missing_arg: str):
with pytest.raises(ValueError) as exc_info:
TestRunModel.model_validate(spec)
assert exc_info.match(
"When 'test_name' is not set, the following fields must be set: 'test_template_name', 'name', 'description'"
"When neither 'test_name' nor 'path' is set, the following fields must be set: "
"'test_template_name', 'name', 'description'"
)

def test_name_is_not_in_mapping(self, test_scenario_parser: TestScenarioParser):
Expand All @@ -349,7 +351,7 @@ def test_cant_override_template_name(self):
spec = {"id": "1", "test_name": "nccl", "test_template_name": "NcclTest"}
with pytest.raises(ValueError) as exc_info:
TestRunModel.model_validate(spec)
assert exc_info.match("'test_template_name' must not be set if 'test_name' is set.")
assert exc_info.match("'test_template_name' must not be set if 'test_name' or 'path' is set.")

def test_spec_with_unknown_test_type(self):
with pytest.raises(ValueError) as exc_info:
Expand All @@ -359,7 +361,7 @@ def test_spec_with_unknown_test_type(self):
def test_type_is_not_allowed_when_name_is_set(self):
with pytest.raises(ValueError) as exc_info:
TestRunModel(id="1", test_name="nccl", test_template_name="NcclTest")
assert exc_info.match("'test_template_name' must not be set if 'test_name' is set.")
assert exc_info.match("'test_template_name' must not be set if 'test_name' or 'path' is set.")

def test_spec_without_base(self, test_scenario_parser: TestScenarioParser):
model = TestScenarioModel.model_validate(
Expand Down Expand Up @@ -922,3 +924,114 @@ def test_nsys_disable_override(self, test_scenario_parser: TestScenarioParser, s
assert tdef.nsys is not None
assert tdef.nsys.enable is False
assert tdef.nsys.output == "/base/output"


class TestPathReference:
def test_path_and_test_name_together_is_rejected(self):
with pytest.raises(ValueError) as exc_info:
TestRunModel(id="1", test_name="nccl", path="nccl.toml")
assert exc_info.match("'test_name' and 'path' must not both be set")

def test_path_and_test_template_name_together_is_rejected(self):
with pytest.raises(ValueError) as exc_info:
TestRunModel(id="1", path="nccl.toml", test_template_name="NcclTest")
assert exc_info.match("'test_template_name' must not be set if 'test_name' or 'path' is set.")

def test_path_and_empty_test_template_name_together_is_rejected(self):
"""An empty string is a truthy-looking but still explicitly-set value - it must not be
treated the same as unset, or 'test_template_name' could silently smuggle a value
through when combined with 'path'."""
with pytest.raises(ValueError, match="'test_template_name' must not be set if 'test_name' or 'path' is set"):
TestRunModel(id="1", path="nccl.toml", test_template_name="")

def test_path_alone_satisfies_the_base_requirement(self):
model = TestRunModel(id="1", path="nccl.toml")
assert model.path == "nccl.toml"

def test_empty_path_is_rejected(self):
with pytest.raises(ValueError, match="String should have at least 1 character"):
TestRunModel(id="1", path="")

def test_path_is_resolved_relative_to_the_scenario_file(self, tmp_path: Path, slurm_system: SlurmSystem):
(tmp_path / "tests").mkdir()
(tmp_path / "tests" / "nccl.toml").write_text(
"""
name = "nccl"
description = "desc"
test_template_name = "NcclTest"

[cmd_args]
docker_image_url = "fake://url/nccl"
"""
)
scenario_path = tmp_path / "scenario.toml"
scenario_path.write_text("") # only its parent directory matters for resolution
parser = TestScenarioParser(scenario_path, slurm_system, {}, {})

test_info = TestRunModel(id="1", path="tests/nccl.toml")
tdef = parser._prepare_tdef(test_info)

assert tdef.name == "nccl"
assert isinstance(tdef, NCCLTestDefinition)
assert tdef.cmd_args.docker_image_url == "fake://url/nccl"

def test_scenario_level_overrides_are_merged_over_the_referenced_file(
self, tmp_path: Path, slurm_system: SlurmSystem
):
(tmp_path / "nccl.toml").write_text(
"""
name = "nccl"
description = "desc"
test_template_name = "NcclTest"

[cmd_args]
docker_image_url = "fake://url/nccl"
"""
)
scenario_path = tmp_path / "scenario.toml"
scenario_path.write_text("")
parser = TestScenarioParser(scenario_path, slurm_system, {}, {})

test_info = TestRunModel(id="1", path="nccl.toml", cmd_args=CmdArgs.model_validate({"nthreads": 42}))
tdef = parser._prepare_tdef(test_info)

assert tdef.cmd_args.nthreads == 42
assert tdef.cmd_args.docker_image_url == "fake://url/nccl"

def test_missing_referenced_file_raises_a_clear_error(self, tmp_path: Path, slurm_system: SlurmSystem):
scenario_path = tmp_path / "scenario.toml"
scenario_path.write_text("")
parser = TestScenarioParser(scenario_path, slurm_system, {}, {})

test_info = TestRunModel(id="1", path="does-not-exist.toml")
with pytest.raises(TestScenarioParsingError) as exc_info:
parser._prepare_tdef(test_info)

assert exc_info.match("does not exist")

def test_full_scenario_toml_with_path_reference(self, tmp_path: Path, slurm_system: SlurmSystem):
(tmp_path / "nccl.toml").write_text(
"""
name = "nccl"
description = "desc"
test_template_name = "NcclTest"

[cmd_args]
docker_image_url = "fake://url/nccl"
"""
)
scenario_path = tmp_path / "scenario.toml"
scenario_path.write_text(
"""
name = "test"

[[Tests]]
id = "1"
path = "nccl.toml"
"""
)
parser = TestScenarioParser(scenario_path, slurm_system, {}, {})

scenario = parser.parse()

assert scenario.test_runs[0].test.name == "nccl"
Loading