-
Notifications
You must be signed in to change notification settings - Fork 28
fix(benchmark): an accuracy run that produces no number must fail #454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1128,11 +1128,20 @@ def _summarize_and_log_metrics( | |
|
|
||
| logger.info(f"Completed in {perf_elapsed:.1f}s") | ||
| if ctx.accuracy_only: | ||
| acc_total = sum( | ||
| ds.dataset.num_samples() * ds.num_repeats | ||
| for ds in ctx.eval_configs | ||
| if ds.dataset_type == DatasetType.ACCURACY | ||
| ) | ||
| # Count what was actually evaluated, not what was loaded. An external | ||
| # scorer (SKIP_ENDPOINT_PHASE) evaluates its own clamped subset -- | ||
| # SWE-bench Verified loads all 500 rows but scores `num_instances` of | ||
| # them -- and reporting the loaded size instead contradicts the | ||
| # per-dataset `unit=` line in the same summary. | ||
| acc_total = 0 | ||
| for ec in ctx.eval_configs: | ||
| if ec.dataset_type != DatasetType.ACCURACY: | ||
| continue | ||
| external = effective_external_sample_count(ec) | ||
| if external is not None: | ||
| acc_total += external | ||
| else: | ||
| acc_total += ec.dataset.num_samples() * ec.num_repeats | ||
| logger.info(f"Accuracy-only: {acc_total} samples evaluated") | ||
| else: | ||
| logger.info( | ||
|
|
@@ -1208,6 +1217,53 @@ def finalize_benchmark(ctx: BenchmarkContext, bench: BenchmarkResult) -> None: | |
| # after the report artifacts so a write failure here can't discard them. | ||
| write_accuracy_results(ctx.report_dir, accuracy_scores) | ||
|
|
||
| # Every artifact is on disk; only now may the run be declared a failure. | ||
| _require_accuracy_numbers(ctx, accuracy_scores) | ||
|
|
||
|
|
||
| def _require_accuracy_numbers( | ||
| ctx: BenchmarkContext, accuracy_scores: list[dict[str, Any]] | ||
| ) -> None: | ||
| """Fail a run that was asked for accuracy and produced no number. | ||
|
|
||
| A scorer can complete every unit of work, exit cleanly, and still hand back | ||
| ``score=None`` -- an externally-scored run (``SKIP_ENDPOINT_PHASE``) whose | ||
| merge gate refused, a scorer whose responses never arrived. Without this | ||
| check the process exits 0, ``report.txt`` prints ``N/A``, and every wrapper | ||
| downstream (sbatch disposition, CI gate, dashboard) reads the run as a pass. | ||
| That failure shape -- rc=0, all work "done", no number -- is the one that | ||
| costs whole GPU allocations, so it is made loud here rather than left to | ||
| each caller to notice. | ||
|
|
||
| Scored-but-partial (a real number with ``complete=False``) is not failed: | ||
| the number exists and the entry already says it is partial. | ||
| """ | ||
| # A PERF-mode run skips externally-scored datasets entirely (they are never | ||
| # dispatched), so they are not owed a number here. | ||
| expected = { | ||
| ec.dataset_name | ||
| for ec in ctx.eval_configs | ||
| if ec.dataset_type == DatasetType.ACCURACY | ||
| and not (ctx.test_mode == TestMode.PERF and ec.scorer.SKIP_ENDPOINT_PHASE) | ||
| } | ||
| if not expected: | ||
| return | ||
|
|
||
| scored = { | ||
| entry["dataset_name"] | ||
| for entry in accuracy_scores | ||
| if entry.get("dataset_type") == DatasetType.ACCURACY.value | ||
| and entry.get("score") is not None | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is checking for |
||
| } | ||
| missing = sorted(expected - scored) | ||
| if missing: | ||
| raise ExecutionError( | ||
| "accuracy scoring produced no score for: " | ||
| + ", ".join(missing) | ||
| + f". Artifacts were preserved in {ctx.report_dir}; see " | ||
| "accuracy/accuracy_results.json for the per-dataset detail." | ||
| ) | ||
|
|
||
|
|
||
| def run_benchmark( | ||
| config: BenchmarkConfig, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -155,6 +155,45 @@ def score_single_sample(self, value, ground_truth): | |
| return 0.0 | ||
|
|
||
|
|
||
| class _ScorelessExternalScorer(Scorer, scorer_id="_test_scoreless_external"): | ||
| """Does all its work, exits cleanly, and returns no number. | ||
|
|
||
| Models the real shape of the SWE-bench fleet scorer whose merge gate | ||
| refuses: every unit reached a terminal record, nothing raised, and the | ||
| headline is ``None``. | ||
| """ | ||
|
|
||
| SKIP_ENDPOINT_PHASE = True | ||
|
|
||
| @classmethod | ||
| def external_sample_count(cls, extras): | ||
| return 2 | ||
|
|
||
| def score_single_sample(self, value, ground_truth): | ||
| return 0.0 | ||
|
|
||
| def score(self): | ||
| self.complete = False | ||
| return None, 1 | ||
|
|
||
|
|
||
| class _PartialButScoredScorer(Scorer, scorer_id="_test_partial_but_scored"): | ||
| """A real number flagged partial — reported, not failed.""" | ||
|
|
||
| SKIP_ENDPOINT_PHASE = True | ||
|
|
||
| @classmethod | ||
| def external_sample_count(cls, extras): | ||
| return 2 | ||
|
|
||
| def score_single_sample(self, value, ground_truth): | ||
| return 0.0 | ||
|
|
||
| def score(self): | ||
| self.complete = False | ||
| return 0.25, 1 | ||
|
|
||
|
|
||
| class _OrdinaryAccuracyScorer(Scorer, scorer_id="_test_ordinary_accuracy"): | ||
| def score_single_sample(self, value, ground_truth): | ||
| return 0.0 | ||
|
|
@@ -2563,6 +2602,129 @@ def _make_report(state: str) -> Report: | |
| ) | ||
|
|
||
|
|
||
| class TestAccuracyRunWithoutANumberFails: | ||
| """rc=0, all work done, no number is a FAILURE. | ||
|
|
||
| This is the regression that cost a whole 200-instance GPU run: the | ||
| distributed SWE-bench scorer drove all 20 units to terminal records, the | ||
| merge gate refused (17 abandoned), ``score()`` returned ``None``, | ||
| ``report.txt`` printed ``N/A`` — and the process exited 0, so the sbatch | ||
| wrapper wrote ``disposition=run completed``. Any scorer path that finishes | ||
| its units but produces no accuracy must fail loudly instead. | ||
| """ | ||
|
|
||
| def _eval_config(self, scorer, dataset, report_dir: Path, name: str): | ||
| return AccuracyConfiguration( | ||
| scorer=scorer, | ||
| extractor=None, | ||
| dataset_name=name, | ||
| dataset=dataset, | ||
| report_dir=report_dir, | ||
| ground_truth_column=None, | ||
| num_repeats=1, | ||
| dataset_type=DatasetType.ACCURACY, | ||
| ) | ||
|
|
||
| def _ctx(self, tmp_path, scorer, name="external_accuracy"): | ||
| dataset = _make_loaded_dataset() | ||
| return _make_benchmark_context( | ||
| config=OfflineConfig(**_OFFLINE_KWARGS), | ||
| report_dir=tmp_path, | ||
| test_mode=TestMode.ACC, | ||
| dataloader=dataset, | ||
| eval_configs=[self._eval_config(scorer, dataset, tmp_path, name)], | ||
| ) | ||
|
|
||
| @pytest.mark.unit | ||
| def test_scoreless_accuracy_run_raises(self, tmp_path): | ||
| ctx = self._ctx(tmp_path, _ScorelessExternalScorer) | ||
|
|
||
| with pytest.raises(ExecutionError, match="produced no score"): | ||
| finalize_benchmark(ctx, _make_benchmark_result(tmp_path)) | ||
|
|
||
| @pytest.mark.unit | ||
| def test_artifacts_survive_the_failure(self, tmp_path): | ||
| """The failure must not cost the evidence needed to diagnose it.""" | ||
| ctx = self._ctx(tmp_path, _ScorelessExternalScorer) | ||
|
|
||
| with pytest.raises(ExecutionError): | ||
| finalize_benchmark(ctx, _make_benchmark_result(tmp_path)) | ||
|
|
||
| results = json.loads( | ||
| (tmp_path / "accuracy" / "accuracy_results.json").read_text() | ||
| ) | ||
| entry = results["accuracy_scores"][0] | ||
| assert entry["dataset_name"] == "external_accuracy" | ||
| assert entry["score"] is None | ||
| assert entry["complete"] is False | ||
|
|
||
| @pytest.mark.unit | ||
| def test_one_scored_dataset_does_not_excuse_a_scoreless_peer(self, tmp_path): | ||
| """Averaging over datasets must not hide a dataset that scored nothing.""" | ||
| dataset = _make_loaded_dataset() | ||
| ctx = _make_benchmark_context( | ||
| config=OfflineConfig(**_OFFLINE_KWARGS), | ||
| report_dir=tmp_path, | ||
| test_mode=TestMode.ACC, | ||
| dataloader=dataset, | ||
| eval_configs=[ | ||
| self._eval_config(_SelfContainedScorer, dataset, tmp_path, "good"), | ||
| self._eval_config(_ScorelessExternalScorer, dataset, tmp_path, "bad"), | ||
| ], | ||
| ) | ||
|
|
||
| with pytest.raises(ExecutionError, match="bad"): | ||
| finalize_benchmark(ctx, _make_benchmark_result(tmp_path)) | ||
|
|
||
| @pytest.mark.unit | ||
| def test_partial_but_numeric_score_still_passes(self, tmp_path): | ||
| """A real number flagged incomplete is reportable, not a failure.""" | ||
| ctx = self._ctx(tmp_path, _PartialButScoredScorer) | ||
|
|
||
| finalize_benchmark(ctx, _make_benchmark_result(tmp_path)) | ||
|
|
||
| results = json.loads( | ||
| (tmp_path / "accuracy" / "accuracy_results.json").read_text() | ||
| ) | ||
| assert results["accuracy_scores"][0]["score"] == 0.25 | ||
| assert results["accuracy_scores"][0]["complete"] is False | ||
|
|
||
| @pytest.mark.unit | ||
| def test_perf_only_run_owes_no_accuracy_number(self, tmp_path): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sugguest to delete this test, this will never happen since we already have Maybe you want to test such |
||
| """PERF mode never dispatches an external scorer, so it owes nothing.""" | ||
| dataset = _make_loaded_dataset() | ||
| ctx = _make_benchmark_context( | ||
| config=OfflineConfig(**_OFFLINE_KWARGS), | ||
| report_dir=tmp_path, | ||
| test_mode=TestMode.PERF, | ||
| dataloader=dataset, | ||
| eval_configs=[ | ||
| self._eval_config( | ||
| _ScorelessExternalScorer, dataset, tmp_path, "external_accuracy" | ||
| ) | ||
| ], | ||
| ) | ||
|
|
||
| finalize_benchmark(ctx, _make_benchmark_result(tmp_path)) | ||
|
|
||
| @pytest.mark.unit | ||
| def test_external_scorer_sample_count_is_the_evaluated_count( | ||
| self, tmp_path, caplog | ||
| ): | ||
| """'N samples evaluated' must match the dataset's own unit= line. | ||
|
|
||
| SWE-bench Verified loads 500 rows and scores ``num_instances`` of them; | ||
| reporting 500 against a ``unit=200`` headline is how a wrong scope goes | ||
| unnoticed. | ||
| """ | ||
| ctx = self._ctx(tmp_path, _PartialButScoredScorer) | ||
|
|
||
| with caplog.at_level(logging.INFO, logger=execute_mod.logger.name): | ||
| finalize_benchmark(ctx, _make_benchmark_result(tmp_path)) | ||
|
|
||
| assert "Accuracy-only: 2 samples evaluated" in caplog.text | ||
|
|
||
|
|
||
| class TestScorerMethodSync: | ||
| """Ensure ScorerMethod enum stays in sync with the scorer registry.""" | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2]:Only to make this consistant with the notes, suggst to add
before external = effective_external_sample_count(ec).