diff --git a/mlpstorage_py/benchmarks/base.py b/mlpstorage_py/benchmarks/base.py index 8384c521..41d6e0f4 100755 --- a/mlpstorage_py/benchmarks/base.py +++ b/mlpstorage_py/benchmarks/base.py @@ -786,7 +786,14 @@ def verify_benchmark(self) -> bool: self.verification = self.benchmark_run_verifier.verify() self.logger.verboser(f'Benchmark verification result: {self.verification}') - if not self.args.closed and not hasattr(self.args, "open"): + # Use getattr so we're resilient to args objects built in tests that + # may not define one or both attributes. Neither flag set => warn and + # skip formal verification (fixes #349: --open was previously + # indistinguishable from "nothing passed"). + closed_mode = getattr(self.args, 'closed', False) + open_mode = getattr(self.args, 'open', False) + + if not closed_mode and not open_mode: self.logger.warning(f'Running the benchmark without verification for open or closed configurations. These results are not valid for submission. Use --open or --closed to specify a configuration.') return True if not self.BENCHMARK_TYPE: @@ -806,11 +813,11 @@ def verify_benchmark(self) -> bool: sys.exit(1) if self.verification == PARAM_VALIDATION.OPEN: - if self.args.closed == False: - # "--open" was passed + if open_mode: self.logger.status(f'Running as allowed open configuration') return True else: + # closed_mode is True here self.logger.warning(f'Parameters allowed for open but not closed. Use --open and rerun the benchmark.') sys.exit(1) diff --git a/mlpstorage_py/benchmarks/kvcache.py b/mlpstorage_py/benchmarks/kvcache.py index 8072eb3a..f01f4f76 100755 --- a/mlpstorage_py/benchmarks/kvcache.py +++ b/mlpstorage_py/benchmarks/kvcache.py @@ -179,8 +179,10 @@ def _execute_run(self) -> int: Returns: Exit code from benchmark execution. """ - # Verify benchmark parameters if running for submission - if hasattr(self.args, 'closed') and self.args.closed: + # Verify benchmark parameters if running for submission (either + # --closed or --open). Previously only --closed triggered verification, + # so --open silently skipped it (related to #349). + if getattr(self.args, 'closed', False) or getattr(self.args, 'open', False): self.verify_benchmark() # Build the command diff --git a/mlpstorage_py/cli/common_args.py b/mlpstorage_py/cli/common_args.py index 1eaad497..23d56169 100755 --- a/mlpstorage_py/cli/common_args.py +++ b/mlpstorage_py/cli/common_args.py @@ -208,18 +208,22 @@ def add_universal_arguments(parser): help="Use the given Object API as the data access method, defaults to S3" ) - # Create a mutually exclusive group for closed/open options + # Create a mutually exclusive group for closed/open options. + # Both flags set their own independent boolean so downstream code can + # distinguish "--open passed", "--closed passed", and "neither passed". submission_group = standard_args.add_mutually_exclusive_group() submission_group.add_argument( "--open", - action="store_false", - dest="closed", + action="store_true", + dest="open", default=False, help="Run as an open submission" ) submission_group.add_argument( "--closed", action="store_true", + dest="closed", + default=False, help="Run as a closed submission" ) diff --git a/mlpstorage_py/tests/test_open_closed_flag_recognition.py b/mlpstorage_py/tests/test_open_closed_flag_recognition.py new file mode 100644 index 00000000..c3dadb61 --- /dev/null +++ b/mlpstorage_py/tests/test_open_closed_flag_recognition.py @@ -0,0 +1,235 @@ +#!/usr/bin/env python3 +""" +Regression tests for issue #349: ``--open`` was indistinguishable from "no flag". + +Before the fix: + +* ``--open`` was defined with ``action="store_false", dest="closed"``, so it + produced the same Namespace (``args.closed=False``, no ``args.open``) as + passing nothing at all. +* ``verify_benchmark()`` used ``hasattr(self.args, "open")`` which is + *always* False, so ``--open`` fell through to the "Running the benchmark + without verification for open or closed configurations" warning branch + and skipped formal verification. + +After the fix: + +* ``--open`` sets ``args.open=True`` as an independent boolean. +* ``verify_benchmark()`` uses ``getattr`` on both ``open`` and ``closed``; + the warning branch triggers only when **neither** flag is set. + +These tests pin that behavior. +""" + +import sys +from argparse import Namespace +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest + +from mlpstorage_py.config import PARAM_VALIDATION, BENCHMARK_TYPES + + +# --------------------------------------------------------------------------- +# Part 1: CLI parser produces distinguishable --open / --closed / neither +# --------------------------------------------------------------------------- + +class TestOpenClosedCLIFlags: + """The argparse definition must let callers tell the three cases apart.""" + + def _build_parser(self): + import argparse + from mlpstorage_py.cli.common_args import add_universal_arguments + parser = argparse.ArgumentParser() + add_universal_arguments(parser) + return parser + + def test_neither_flag_sets_both_false(self): + parser = self._build_parser() + # --file is required by a separate mutually-exclusive group; pass it + # to avoid the unrelated "one of --file --object required" error. + args = parser.parse_args(["--file"]) + assert getattr(args, "closed", None) is False + assert getattr(args, "open", None) is False + + def test_open_flag_sets_open_true_closed_false(self): + parser = self._build_parser() + args = parser.parse_args(["--file", "--open"]) + assert args.open is True + assert args.closed is False + + def test_closed_flag_sets_closed_true_open_false(self): + parser = self._build_parser() + args = parser.parse_args(["--file", "--closed"]) + assert args.closed is True + assert args.open is False + + def test_open_and_closed_are_mutually_exclusive(self): + parser = self._build_parser() + # argparse exits with SystemExit(2) on mutually-exclusive violation + with pytest.raises(SystemExit): + parser.parse_args(["--file", "--open", "--closed"]) + + +# --------------------------------------------------------------------------- +# Part 2: verify_benchmark() respects the new semantics +# --------------------------------------------------------------------------- + +def _make_benchmark(tmp_path, **arg_overrides): + """Construct a minimally-initialized Benchmark subclass for testing.""" + from mlpstorage_py.benchmarks.base import Benchmark + + class _Bench(Benchmark): + BENCHMARK_TYPE = BENCHMARK_TYPES.training + def _run(self): + return 0 + + defaults = dict( + debug=False, + verbose=False, + what_if=False, + stream_log_level="INFO", + results_dir=str(tmp_path), + model="unet3d", + command="run", + num_processes=8, + accelerator_type="h100", + allow_invalid_params=False, + closed=False, + open=False, + ) + defaults.update(arg_overrides) + + bench = _Bench.__new__(_Bench) + bench.args = Namespace(**defaults) + bench.logger = MagicMock() + # Silence logger methods the code calls + for lvl in ("debug", "info", "warning", "error", "status", + "verbose", "verboser", "ridiculous", "result"): + setattr(bench.logger, lvl, MagicMock()) + bench.benchmark_run_verifier = None + bench.run_datetime = "20260424_000000" + bench.verification = None + return bench + + +class TestVerifyBenchmarkOpenFlag: + """Fixes for issue #349 — the heart of the bug.""" + + def test_open_flag_does_not_hit_no_verification_warning(self, tmp_path): + """ + Regression test for #349: passing --open must NOT route through the + "Running the benchmark without verification" warning branch. + """ + bench = _make_benchmark(tmp_path, open=True, closed=False) + + with patch("mlpstorage_py.benchmarks.base.BenchmarkVerifier") as mock_cls: + mock_verifier = MagicMock() + mock_verifier.verify.return_value = PARAM_VALIDATION.OPEN + mock_cls.return_value = mock_verifier + + result = bench.verify_benchmark() + + assert result is True + # The "no verification" warning must NOT have been emitted. + for call in bench.logger.warning.call_args_list: + assert "without verification for open or closed" not in call.args[0], \ + "--open should route to formal verification, not the 'no verification' warning" + # A proper OPEN-allowed status message should have been emitted instead. + status_msgs = [c.args[0] for c in bench.logger.status.call_args_list] + assert any("allowed open configuration" in m for m in status_msgs), \ + "Expected 'Running as allowed open configuration' status message" + + def test_closed_flag_accepts_closed_verification(self, tmp_path): + bench = _make_benchmark(tmp_path, closed=True, open=False) + + with patch("mlpstorage_py.benchmarks.base.BenchmarkVerifier") as mock_cls: + mock_verifier = MagicMock() + mock_verifier.verify.return_value = PARAM_VALIDATION.CLOSED + mock_cls.return_value = mock_verifier + + result = bench.verify_benchmark() + + assert result is True + + def test_closed_flag_rejects_open_only_params(self, tmp_path): + """If user asked for --closed but params only qualify for OPEN, exit.""" + bench = _make_benchmark(tmp_path, closed=True, open=False) + + with patch("mlpstorage_py.benchmarks.base.BenchmarkVerifier") as mock_cls: + mock_verifier = MagicMock() + mock_verifier.verify.return_value = PARAM_VALIDATION.OPEN + mock_cls.return_value = mock_verifier + + with pytest.raises(SystemExit): + bench.verify_benchmark() + + def test_neither_flag_emits_no_verification_warning(self, tmp_path): + """ + Unchanged contract: when neither --open nor --closed is passed, the + benchmark runs with a warning and skips formal verification. + """ + bench = _make_benchmark(tmp_path, closed=False, open=False) + + with patch("mlpstorage_py.benchmarks.base.BenchmarkVerifier") as mock_cls: + mock_verifier = MagicMock() + mock_verifier.verify.return_value = PARAM_VALIDATION.CLOSED + mock_cls.return_value = mock_verifier + + result = bench.verify_benchmark() + + assert result is True + assert any( + "without verification for open or closed" in c.args[0] + for c in bench.logger.warning.call_args_list + ), "Default (neither flag) should warn that no verification is being performed" + + def test_invalid_params_exits_without_allow_flag(self, tmp_path): + bench = _make_benchmark(tmp_path, closed=True, open=False, + allow_invalid_params=False) + + with patch("mlpstorage_py.benchmarks.base.BenchmarkVerifier") as mock_cls: + mock_verifier = MagicMock() + mock_verifier.verify.return_value = PARAM_VALIDATION.INVALID + mock_cls.return_value = mock_verifier + + with pytest.raises(SystemExit): + bench.verify_benchmark() + + def test_invalid_params_allowed_with_flag(self, tmp_path): + bench = _make_benchmark(tmp_path, closed=True, open=False, + allow_invalid_params=True) + + with patch("mlpstorage_py.benchmarks.base.BenchmarkVerifier") as mock_cls: + mock_verifier = MagicMock() + mock_verifier.verify.return_value = PARAM_VALIDATION.INVALID + mock_cls.return_value = mock_verifier + + result = bench.verify_benchmark() + + assert result is True + + +# --------------------------------------------------------------------------- +# Part 3: kvcache triggers verification for --open too (not just --closed) +# --------------------------------------------------------------------------- + +class TestKVCacheOpenFlag: + """kvcache.py guard must run verification for both --open and --closed.""" + + def test_open_triggers_verification_in_kvcache_guard(self): + """ + Prior code was: ``if hasattr(self.args, 'closed') and self.args.closed`` + which never fired for --open. Post-fix it must fire for either flag. + """ + args_open = SimpleNamespace(open=True, closed=False) + args_closed = SimpleNamespace(open=False, closed=True) + args_neither = SimpleNamespace(open=False, closed=False) + + def should_verify(args): + return getattr(args, "closed", False) or getattr(args, "open", False) + + assert should_verify(args_open) is True + assert should_verify(args_closed) is True + assert should_verify(args_neither) is False diff --git a/tests/unit/test_benchmarks_base.py b/tests/unit/test_benchmarks_base.py index c3781162..5350550e 100755 --- a/tests/unit/test_benchmarks_base.py +++ b/tests/unit/test_benchmarks_base.py @@ -287,7 +287,7 @@ class TestBenchmarkVerifyBenchmark: @pytest.fixture def benchmark(self, tmp_path): - """Create a benchmark instance.""" + """Create a benchmark instance with --closed semantics.""" args = Namespace( debug=False, verbose=False, @@ -299,6 +299,7 @@ def benchmark(self, tmp_path): num_processes=8, accelerator_type='h100', closed=True, + open=False, allow_invalid_params=False ) @@ -343,8 +344,9 @@ def test_allows_invalid_with_flag(self, benchmark): assert result is True def test_exits_for_open_when_closed_required(self, benchmark): - """Should exit for OPEN verification when closed is required.""" + """Should exit for OPEN verification when --closed was passed.""" benchmark.args.closed = True + benchmark.args.open = False with patch('mlpstorage.benchmarks.base.BenchmarkVerifier') as mock_verifier_class: mock_verifier = MagicMock() @@ -355,8 +357,15 @@ def test_exits_for_open_when_closed_required(self, benchmark): benchmark.verify_benchmark() def test_allows_open_with_open_flag(self, benchmark): - """Should allow OPEN verification with --open flag.""" + """Should allow OPEN verification when --open was passed. + + Post-#349 fix: --open sets args.open=True (independent of args.closed). + Previously this test only set closed=False, which was indistinguishable + from "neither flag passed" and therefore did not actually exercise the + --open code path. + """ benchmark.args.closed = False + benchmark.args.open = True with patch('mlpstorage.benchmarks.base.BenchmarkVerifier') as mock_verifier_class: mock_verifier = MagicMock() @@ -367,10 +376,13 @@ def test_allows_open_with_open_flag(self, benchmark): assert result is True - def test_returns_true_with_closed_false_no_open_attr(self, tmp_path): - """Should return True and warn when closed=False and no 'open' attr (default state).""" - # This simulates the default state when neither --closed nor --open is passed - # The CLI sets closed=False as default, and hasattr(args, 'open') is False + def test_returns_true_with_neither_flag_set(self, tmp_path): + """Should return True and warn when neither --closed nor --open is passed. + + Post-#349 fix: the CLI parser now sets both args.closed=False and + args.open=False by default. The "no verification" warning branch + triggers only when *both* are False. + """ args = Namespace( debug=False, verbose=False, @@ -382,9 +394,9 @@ def test_returns_true_with_closed_false_no_open_attr(self, tmp_path): num_processes=8, accelerator_type='h100', closed=False, # Default when neither flag is passed + open=False, # Default when neither flag is passed allow_invalid_params=False ) - # Note: 'open' attribute should NOT be present for this test with patch('mlpstorage.benchmarks.base.generate_output_location') as mock_gen: mock_gen.return_value = str(tmp_path / "output") @@ -398,7 +410,7 @@ def test_returns_true_with_closed_false_no_open_attr(self, tmp_path): result = benchmark.verify_benchmark() - # Should return True early with warning (line 170-172 in base.py) + # Should return True early with warning (the "no verification" branch) assert result is True @@ -546,6 +558,7 @@ def test_full_workflow(self, tmp_path): num_processes=8, accelerator_type='h100', closed=True, + open=False, allow_invalid_params=False )