Skip to content

Commit e1bea1c

Browse files
committed
workloads/megatron_run: restrict bool sentinel to bool fields, add tests
Address two code review comments: - Restrict "true"/"false"/"" sentinel logic to fields declared with default="" (known boolean flags) and model_extra fields (boolean by convention). Typed string fields like tensorboard_dir pass their value through unchanged. - Add parameterized tests covering all sentinel cases, extra-field behavior, and the non-sentinel pass-through for string-valued fields.
1 parent 99d8b28 commit e1bea1c

2 files changed

Lines changed: 55 additions & 4 deletions

File tree

src/cloudai/workloads/megatron_run/megatron_run.py

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,14 +80,20 @@ def no_dashed_args(self):
8080

8181
@property
8282
def cmd_args(self) -> dict[str, Union[str, list[str]]]:
83+
# Fields with default="" are boolean flags; extra fields (model_extra) are also boolean by convention.
84+
bool_flags = {k for k, f in MegatronRunCmdArgs.model_fields.items() if f.default == ""}
85+
extra_keys = set(self.model_extra or {})
8386
args = self.model_dump(exclude_none=True, exclude={"docker_image_url", "run_script"})
8487
result: dict[str, Union[str, list[str]]] = {}
8588
for k, v in args.items():
8689
flag = f"--{k.replace('_', '-')}"
87-
if v == "false":
88-
continue
89-
elif v in ("true", ""):
90-
result[flag] = ""
90+
if k in bool_flags or k in extra_keys:
91+
if v == "false":
92+
continue
93+
elif v in ("true", ""):
94+
result[flag] = ""
95+
else:
96+
result[flag] = v
9197
else:
9298
result[flag] = v
9399
return result

tests/workloads/megatron_run/test_megatron_run.py

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,51 @@
2424
from cloudai.workloads.megatron_run import MegatronRunCmdArgs, MegatronRunTestDefinition
2525

2626

27+
class TestMegatronRunCmdArgs:
28+
BASE_ARGS = dict(docker_image_url="http://url", run_script=Path(__file__))
29+
30+
@pytest.mark.parametrize(
31+
("field", "value", "expected_flag", "expected_value"),
32+
[
33+
("log_timers_to_tensorboard", "", "--log-timers-to-tensorboard", ""),
34+
("log_timers_to_tensorboard", "true", "--log-timers-to-tensorboard", ""),
35+
("log_throughput", "true", "--log-throughput", ""),
36+
("log_memory_to_tensorboard", "true", "--log-memory-to-tensorboard", ""),
37+
],
38+
)
39+
def test_bool_flag_true_and_empty_emit_bare_flag(self, field, value, expected_flag, expected_value):
40+
args = MegatronRunCmdArgs(**self.BASE_ARGS, **{field: value})
41+
assert args.cmd_args[expected_flag] == expected_value
42+
43+
@pytest.mark.parametrize("field", ["log_timers_to_tensorboard", "log_throughput", "log_memory_to_tensorboard"])
44+
def test_bool_flag_false_omits_flag(self, field):
45+
args = MegatronRunCmdArgs(**self.BASE_ARGS, **{field: "false"})
46+
flag = f"--{field.replace('_', '-')}"
47+
assert flag not in args.cmd_args
48+
49+
def test_extra_bool_flag_true_emits_bare_flag(self):
50+
args = MegatronRunCmdArgs(**self.BASE_ARGS, mock_data="true")
51+
assert args.cmd_args["--mock-data"] == ""
52+
53+
def test_extra_bool_flag_false_omits_flag(self):
54+
args = MegatronRunCmdArgs(**self.BASE_ARGS, mock_data="false")
55+
assert "--mock-data" not in args.cmd_args
56+
57+
def test_ordinary_value_passed_through(self):
58+
args = MegatronRunCmdArgs(**self.BASE_ARGS, global_batch_size=32)
59+
assert args.cmd_args["--global-batch-size"] == 32
60+
61+
def test_string_field_with_non_sentinel_value_passed_through(self):
62+
args = MegatronRunCmdArgs(**self.BASE_ARGS, tensorboard_dir="/some/path")
63+
assert args.cmd_args["--tensorboard-dir"] == "/some/path"
64+
65+
def test_string_field_literal_true_not_treated_as_sentinel(self):
66+
# tensorboard_dir is a typed string field (default is a path, not ""),
67+
# so "true" must be passed as a literal value, not a bare flag.
68+
args = MegatronRunCmdArgs(**self.BASE_ARGS, tensorboard_dir="true")
69+
assert args.cmd_args["--tensorboard-dir"] == "true"
70+
71+
2772
class TestMegatronRunSuccessCheck:
2873
def setup_method(self) -> None:
2974
self.megatron_tdef = MegatronRunTestDefinition(

0 commit comments

Comments
 (0)