diff --git a/problemtools/checks/testdata.py b/problemtools/checks/testdata.py index 84413468..2a1490fc 100644 --- a/problemtools/checks/testdata.py +++ b/problemtools/checks/testdata.py @@ -6,10 +6,11 @@ import glob import hashlib import os +from collections.abc import Callable, Iterator from pathlib import Path from ..context import Context -from ..diagnostics import Diagnostics, VerifyError +from ..diagnostics import Diagnostics from ..formatversion import FormatVersion from ..judge import validate_output from ..metadata import Metadata @@ -47,6 +48,16 @@ def check_testdata( has_custom_grader = graders.grader is not None has_default_grader = DEFAULT_GRADER is not None + if metadata.is_scoring(): + _warn_reject_score(testdata, diag) + + # Whether the selected output validator might emit an arbitrary score via score.txt, + # making a test case's score unbounded as far as _check_score_range is concerned. + custom_scoring_possible = ( + not output_validators.uses_default(format_version, metadata) and metadata.is_custom_score_allowed() + ) + _check_score_range(testdata, custom_scoring_possible, diag) + input_validation = InputValidationCache(input_validators, work_dir) input_validation.precompute(testdata, context) @@ -64,6 +75,127 @@ def check_testdata( ) +def _all_groups(group: TestDataGroup) -> Iterator[TestDataGroup]: + """`group` and all its descendant groups.""" + yield group + for subgroup in group.get_subgroups(): + yield from _all_groups(subgroup) + + +def _warn_reject_score(testdata: TestDataGroup, diag: Diagnostics) -> None: + """Warn about reject_score usage.""" + groups = list(_all_groups(testdata)) + + nonzero_reject = [(g, g.config['reject_score']) for g in groups if g.config['reject_score'] != 0] + if nonzero_reject: + example_group, example_score = nonzero_reject[0] + diag.warning( + f'{len(nonzero_reject)} testcase group(s) configure a non-zero reject_score (e.g. {example_group} ' + f'has reject_score {example_score:g}); submissions with non-AC final verdict always have score 0, ' + 'so this is usually a mistake' + ) + + +#: Score aggregators for `grading: default`, matching support/default_grader's `score_aggregators`. +#: All are monotonic non-decreasing in each argument, which is what makes _check_score_range below +#: correct: the range of an aggregate is the aggregator applied to the children's lower bounds, and +#: separately to their upper bounds. +_SCORE_AGGREGATORS: dict[str, Callable[[list[float]], float]] = { + 'sum': sum, + 'avg': lambda scores: sum(scores) / len(scores), + 'min': min, + 'max': max, +} + + +def _check_score_range(group: TestDataGroup, custom_scoring_possible: bool, diag: Diagnostics) -> tuple[float, float]: + """Recursively check `group`'s declared score `range` against what can be inferred from its + grading configuration and test data. Returns group's effective range.""" + children = group.items + if group.is_root and 'ignore_sample' in group.config['grader_flags'].split(): + children = [child for child in children if not (isinstance(child, TestDataGroup) and child.datadir.name == 'sample')] + + if not children: + aggregate = (0.0, 0.0) + elif group.config['grading'] == 'custom': + # A custom grader can't be reasoned about. + aggregate = (float('-inf'), float('inf')) + else: + child_ranges = [] + for child in children: + if isinstance(child, TestDataGroup): + child_ranges.append(_check_score_range(child, custom_scoring_possible, diag)) + elif custom_scoring_possible: + child_ranges.append((float('-inf'), float('inf'))) + else: + accept_score, reject_score = group.config['accept_score'], group.config['reject_score'] + child_ranges.append((min(accept_score, reject_score), max(accept_score, reject_score))) + + aggregator_name = 'sum' + for flag in group.config['grader_flags'].split(): + if flag in _SCORE_AGGREGATORS: + aggregator_name = flag # last one wins, matching default_grader + aggregator = _SCORE_AGGREGATORS[aggregator_name] + + aggregate = (aggregator([lo for lo, _hi in child_ranges]), aggregator([hi for _lo, hi in child_ranges])) + + score_range = group.config['range'] + try: + min_score, max_score = list(map(float, score_range.split())) + except Exception: + diag.error(f"Invalid format '{score_range}' for range: must be exactly two floats") + return aggregate + + if min_score > max_score: + diag.error(f"Invalid score range '{score_range}': minimum score cannot be greater than maximum score") + return aggregate + + return _warn_score_range(group, (min_score, max_score), aggregate, diag) + + +def _warn_score_range( + group: TestDataGroup, declared: tuple[float, float], aggregate: tuple[float, float], diag: Diagnostics +) -> tuple[float, float]: + """Compare `group`'s declared score range to what its `aggregate` says can actually be achieved, + warn about any mismatch, and return the effective (declared-trusting) range.""" + min_score, max_score = declared + agg_min, agg_max = aggregate + score_range = group.config['range'] + is_default_range = score_range == DEFAULT_CONFIG['range'] + if max_score < agg_min or min_score > agg_max: + diag.warning( + f"Declared score range '{score_range}' for {group} doesn't overlap with the computed range " + f'[{agg_min:g}, {agg_max:g}] at all' + ) + # We're in a bad state here, unclear what to return. Specified range probably ends up less spammy. + return (min_score, max_score) + elif min_score < agg_min or max_score > agg_max: + if is_default_range: + diag.warning( + f'No score range declared for {group}, but a range of [{agg_min:g}, {agg_max:g}] can be ' + f"computed from its grading configuration and test data; consider adding 'range: {agg_min:g} {agg_max:g}'" + ) + else: + diag.warning( + f"Declared score range '{score_range}' for {group} is looser than the computed range " + f'[{agg_min:g}, {agg_max:g}]; consider tightening it' + ) + elif group.is_root and is_default_range: + # The default range -inf, inf basically never makes sense. Encourage tighter even when we can't compute a recommendation + diag.warning( + f'No score range declared for {group}, and none could be computed automatically; as ' + 'the top-level group, its range is the overall score range for the problem -- consider ' + 'declaring one explicitly' + ) + elif group.is_root and min_score < 0: + diag.warning( + f"Declared score range '{score_range}' for {group} has a negative minimum; submissions with " + 'non-AC final verdict always have score 0, so a negative minimum is usually a mistake' + ) + + return (max(agg_min, min_score), min(agg_max, max_score)) + + def _check_group( group: TestDataGroup, context: Context, @@ -104,17 +236,8 @@ def _check_group( if group.config['on_reject'] not in ['break', 'continue']: diag.error(f"Invalid value '{group.config['on_reject']}' for on_reject policy") - if metadata.is_scoring(): - # Check grading - try: - score_range = group.config['range'] - min_score, max_score = list(map(float, score_range.split())) - if min_score > max_score: - diag.error(f"Invalid score range '{score_range}': minimum score cannot be greater than maximum score") - except VerifyError: - raise - except Exception: - diag.error(f"Invalid format '{score_range}' for range: must be exactly two floats") + # Score range validity and tightness are checked by _check_score_range, called once for the + # whole tree from check_testdata. if group.is_root: seen_secret = False diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 00000000..8dc9c443 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,40 @@ +"""Shared pytest fixtures and test doubles.""" + +import pytest + +from problemtools.diagnostics import Diagnostics + + +class RecordingDiagnostics(Diagnostics): + """A Diagnostics that records messages instead of emitting them, for asserting on in tests.""" + + def __init__(self) -> None: + self.messages: list[tuple[str, str]] = [] + + def error(self, msg: str, additional_info: str | None = None) -> None: + self.messages.append(('error', msg)) + + def warning(self, msg: str, additional_info: str | None = None) -> None: + self.messages.append(('warning', msg)) + + def info(self, msg: str) -> None: + pass + + def debug(self, msg: str) -> None: + pass + + def child(self, name: str) -> Diagnostics: + return self + + @property + def errors(self) -> int: + return len([m for m in self.messages if m[0] == 'error']) + + @property + def warnings(self) -> int: + return len([m for m in self.messages if m[0] == 'warning']) + + +@pytest.fixture +def diag() -> RecordingDiagnostics: + return RecordingDiagnostics() diff --git a/tests/test_score_range.py b/tests/test_score_range.py new file mode 100644 index 00000000..2dcaf47c --- /dev/null +++ b/tests/test_score_range.py @@ -0,0 +1,267 @@ +from pathlib import Path + +from problemtools.checks.testdata import _check_score_range +from problemtools.model.testdata import TestCase, TestDataGroup + +# Not test classes -- just named that way by the model. Tell pytest not to collect them. +TestCase.__test__ = False +TestDataGroup.__test__ = False + +INF = float('inf') + + +def make_testcase(name: str) -> TestCase: + path = Path(name) + return TestCase( + infile=path.with_suffix('.in'), + ansfile=path.with_suffix('.ans'), + path=path, + input_validator_flags=[], + output_validator_flags=[], + ) + + +def make_group( + name: str, + items: list[TestCase | TestDataGroup], + grader_flags: str = '', + grading: str = 'default', + range_: str = '-inf +inf', + accept_score: float = 1.0, + reject_score: float = 0.0, + is_root: bool = False, +) -> TestDataGroup: + return TestDataGroup( + name=name, + datadir=Path(name), + config={ + 'grading': grading, + 'grader_flags': grader_flags, + 'range': range_, + 'accept_score': accept_score, + 'reject_score': reject_score, + }, + is_root=is_root, + items=items, + ) + + +def test_group_sum_is_default_aggregator(diag): + group = make_group( + 'g', [make_testcase('a'), make_testcase('b'), make_testcase('c')], range_='0 30', accept_score=10, reject_score=0 + ) + assert _check_score_range(group, False, diag) == (0, 30) + assert diag.messages == [] + + +def test_group_avg_aggregator(diag): + group = make_group('g', [make_testcase('a')], grader_flags='avg', accept_score=10, reject_score=0) + assert _check_score_range(group, False, diag) == (0, 10) + + +def test_reject_above_accept(diag): + # Corner case: check we don't end up with a broken range if reject_score is above accept_score + group = make_group('g', [make_testcase('a')], accept_score=0, reject_score=5) + assert _check_score_range(group, False, diag) == (0, 5) + + +def test_custom_scoring_possible_is_unbounded(diag): + group = make_group('g', [make_testcase('a')], accept_score=10, reject_score=0) + assert _check_score_range(group, True, diag) == (-INF, INF) + + +def test_group_min_aggregator(diag): + group = make_group( + 'g', + [make_testcase('a'), make_testcase('b')], + grader_flags='min', + ) + # accept_score/reject_score are shared across a group's direct testcase children (1.0/0.0 here). + assert _check_score_range(group, False, diag) == (0, 1) + + +def test_group_max_aggregator(diag): + group = make_group('g', [make_testcase('a'), make_testcase('b')], grader_flags='max') + assert _check_score_range(group, False, diag) == (0, 1) + + +def test_last_aggregator_flag_wins(diag): + group = make_group('g', [make_testcase('a')], grader_flags='max min avg', accept_score=10, reject_score=0) + assert _check_score_range(group, False, diag) == (0, 10) + + +def test_custom_grading_is_unbounded(diag): + group = make_group('g', [make_testcase('a')], grading='custom') + assert _check_score_range(group, False, diag) == (-INF, INF) + + +def test_empty_group_scores_zero(diag): + group = make_group('g', []) + assert _check_score_range(group, False, diag) == (0, 0) + + +def test_nested_groups_compose(diag): + subtask1 = make_group( + 'g.subtask1', + [make_testcase('a'), make_testcase('b')], + grader_flags='min', + range_='0 50', + accept_score=50, + reject_score=0, + ) + subtask2 = make_group('g.subtask2', [make_testcase('c')], grader_flags='min', range_='0 50', accept_score=50, reject_score=0) + secret = make_group('g.secret', [subtask1, subtask2], range_='0 100') + assert _check_score_range(secret, False, diag) == (0, 100) + assert diag.messages == [] + + +def test_ignore_sample_at_root_skips_sample_group(diag): + sample = make_group('sample', [make_testcase('s')], accept_score=1000, reject_score=0) + secret = make_group('secret', [make_testcase('a')], accept_score=100, reject_score=0) + root = make_group('data', [sample, secret], grader_flags='ignore_sample', is_root=True) + assert _check_score_range(root, False, diag) == (0, 100) + + +def test_ignore_sample_is_a_no_op_below_root(diag): + sample = make_group('sample', [make_testcase('s')], accept_score=1000, reject_score=0) + secret = make_group('secret', [make_testcase('a')], accept_score=100, reject_score=0) + # Misconfigured (checks._check_group flags this separately). We just aggregate all children + non_root = make_group('g', [sample, secret], grader_flags='ignore_sample', is_root=False) + assert _check_score_range(non_root, False, diag) == (0, 1100) + + +# --- Declared range vs. what can be inferred --- + + +def test_no_warning_when_declared_matches_computed(diag): + group = make_group('g', [make_testcase('a')], range_='0 10', accept_score=10, reject_score=0) + assert _check_score_range(group, False, diag) == (0, 10) + assert diag.messages == [] + + +def test_looser_declared_range_warns_and_suggests_tightening(diag): + group = make_group('g', [make_testcase('a')], range_='0 100', accept_score=10, reject_score=0) + assert _check_score_range(group, False, diag) == (0, 10) + assert diag.messages == [ + ( + 'warning', + "Declared score range '0 100' for testcase group g is looser than the computed range [0, 10]; consider tightening it", + ) + ] + + +def test_no_declared_range_warns_and_suggests_one(diag): + group = make_group('g', [make_testcase('a')], accept_score=10, reject_score=0) # range left at the default + assert _check_score_range(group, False, diag) == (0, 10) + assert diag.messages == [ + ( + 'warning', + ( + 'No score range declared for testcase group g, but a range of [0, 10] can be computed from its ' + "grading configuration and test data; consider adding 'range: 0 10'" + ), + ) + ] + + +def test_narrower_declared_range_is_trusted_without_warning(diag): + # A "bad guarantee": the group's own children can clearly reach 100, but the author declared a + # narrower range. We don't warn -- that's a promise checked elsewhere (checks.submissions) -- + # but we do trust it for the returned (propagated) value. + group = make_group('g', [make_testcase('a')], range_='0 10', accept_score=100, reject_score=0) + assert _check_score_range(group, False, diag) == (0, 10) + assert diag.messages == [] + + +def test_narrower_declared_range_propagates_to_parent(diag): + # The parent's aggregate must reflect the child's effective (trusted) range, not its raw + # aggregate -- so a narrow declaration deep in the tree is reflected in ancestors' results too. + bad_child = make_group('g.secret', [make_testcase('a')], range_='0 10', accept_score=100, reject_score=0) + root = make_group('g', [bad_child], is_root=True) + assert _check_score_range(root, False, diag) == (0, 10) + # No warning at the child, but the root's default range is loose + assert diag.messages == [ + ( + 'warning', + ( + 'No score range declared for testcase group g, but a range of [0, 10] can be computed from its ' + "grading configuration and test data; consider adding 'range: 0 10'" + ), + ) + ] + + +def test_disjoint_declared_range_warns_distinctly(diag): + group = make_group('g', [make_testcase('a')], range_='0 10', accept_score=100, reject_score=50) + assert _check_score_range(group, False, diag) == (0, 10) + assert diag.messages == [ + ( + 'warning', + "Declared score range '0 10' for testcase group g doesn't overlap with the computed range [50, 100] at all", + ) + ] + + +def test_root_with_unbounded_aggregate_and_no_declared_range_still_warns(diag): + group = make_group('g', [make_testcase('a')], grading='custom', is_root=True) + assert _check_score_range(group, False, diag) == (-INF, INF) + assert diag.messages == [ + ( + 'warning', + ( + 'No score range declared for testcase group g, and none could be computed automatically; as the ' + 'top-level group, its range is the overall score range for the problem -- consider declaring one ' + 'explicitly' + ), + ) + ] + + +def test_non_root_with_unbounded_aggregate_and_no_declared_range_is_silent(diag): + group = make_group('g', [make_testcase('a')], grading='custom', is_root=False) + assert _check_score_range(group, False, diag) == (-INF, INF) + assert diag.messages == [] + + +def test_root_with_negative_minimum_and_no_other_issue_warns(diag): + group = make_group('g', [make_testcase('a')], accept_score=10, reject_score=-5, range_='-5 10', is_root=True) + assert _check_score_range(group, False, diag) == (-5, 10) + assert diag.messages == [ + ( + 'warning', + ( + "Declared score range '-5 10' for testcase group g has a negative minimum; submissions with " + 'non-AC final verdict always have score 0, so a negative minimum is usually a mistake' + ), + ) + ] + + +def test_negative_minimum_below_root_is_not_flagged(diag): + # negative scores for non-root test groups is a bit weird, but IMHO not weird enough to warn about + group = make_group('g', [make_testcase('a')], accept_score=10, reject_score=-5, range_='-5 10', is_root=False) + assert _check_score_range(group, False, diag) == (-5, 10) + assert diag.messages == [] + + +def test_negative_minimum_at_root_is_suppressed_by_other_warnings(diag): + # Warning about a negative range is low priority. If we can instead suggest to narrow the range, + # that's typically a better warning. + group = make_group('g', [make_testcase('a')], accept_score=10, reject_score=0, range_='-10 200', is_root=True) + assert _check_score_range(group, False, diag) == (0, 10) + assert len(diag.messages) == 1 + assert 'looser than the computed range' in diag.messages[0][1] + + +def test_invalid_range_format_errors_and_falls_back_to_aggregate(diag): + group = make_group('g', [make_testcase('a')], range_='not a range', accept_score=10, reject_score=0) + assert _check_score_range(group, False, diag) == (0, 10) + assert diag.errors == 1 + assert "Invalid format 'not a range'" in diag.messages[0][1] + + +def test_min_greater_than_max_errors_and_falls_back_to_aggregate(diag): + group = make_group('g', [make_testcase('a')], range_='10 0', accept_score=10, reject_score=0) + assert _check_score_range(group, False, diag) == (0, 10) + assert diag.errors == 1 + assert 'cannot be greater than maximum score' in diag.messages[0][1]