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
19 changes: 15 additions & 4 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 @@ -145,7 +156,7 @@ def check_test_name_or_type_is_set(self):
)
else:
if self.test_template_name:
raise ValueError("'test_template_name' must not be set if 'test_name' is set.")
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
114 changes: 111 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,109 @@ 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_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) as exc_info:
TestRunModel(id="1", path="")
assert exc_info.match("String should have at least 1 character")
Comment on lines +944 to +947

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -eu

printf '%s\n' '--- target test ---'
sed -n '930,955p' tests/test_test_scenario.py

printf '%s\n' '--- Ruff configuration and PT011 references ---'
rg -n --hidden -g '!*.lock' -g '!node_modules' -g '!dist' \
  'PT011|flake8-pytest|pytest.raises|ruff' pyproject.toml setup.cfg tox.ini .ruff.toml ruff.toml tests 2>/dev/null || true

printf '%s\n' '--- Ruff availability ---'
if command -v ruff >/dev/null 2>&1; then
  ruff --version
  ruff check --select PT011 tests/test_test_scenario.py
else
  echo 'ruff is not available'
fi

Repository: NVIDIA/cloudai

Length of output: 24029


Pass the message to pytest.raises.

Ruff PT011 flags this test. Move the message match into pytest.raises and remove the separate assertion.

🧰 Tools
🪛 Ruff (0.16.1)

[warning] 945-945: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception

(PT011)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_test_scenario.py` around lines 944 - 947, Update
test_empty_path_is_rejected to pass the expected error-message pattern directly
via pytest.raises, and remove the separate exc_info.match assertion.

Source: Linters/SAST tools


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, {}, {})
model = TestScenarioModel.model_validate(toml.loads(scenario_path.read_text()))

tdef = parser._prepare_tdef(model.tests[0])

assert tdef.name == "nccl"
Loading