Fix completeness gate: distinguish inapplicable metrics from real failures (#78)

Author: MaximilianAnzinger

src/metrics/base.py

@@ -18,6 +18,15 @@
 from .phase import Phase, PhaseInput, PhaseOutput
 
 
+class MetricNotApplicableError(Exception):
+    """Raised when a metric legitimately does not apply to a question.
+
+    Distinct from a failure: the cell was never scorable (e.g. distractor
+    quality on a true/false item), so completeness accounting must not treat
+    it as data loss. Every other exception is a real loss.
+    """
+
+
 class MetricScope(str, Enum):
     """Defines the scope at which a metric operates."""
 

src/metrics/distractor.py

@@ -5,7 +5,7 @@
 from pydantic import BaseModel, ConfigDict, Field
 
 from ..models.quiz import QuestionType, QuizQuestion
-from .base import BaseMetric, MetricScope
+from .base import BaseMetric, MetricNotApplicableError, MetricScope
 from .phase import Phase, PhaseInput
 
 QUALITY_SCORES = {
@@ -102,7 +102,7 @@ def _build_analyze_prompt(inp: PhaseInput) -> str:
         question = inp.question
 
         if question.question_type not in (QuestionType.SINGLE_CHOICE, QuestionType.MULTIPLE_CHOICE):
-            raise ValueError(
+            raise MetricNotApplicableError(
                 f"Distractor quality cannot be evaluated for {question.question_type.value} questions. "
                 "Only single_choice and multiple_choice are supported."
             )

src/runners/benchmark.py

@@ -8,7 +8,7 @@
 from ..evaluators.base import LLMProvider, TransientLLMError
 from ..evaluators.factory import LLMProviderFactory
 from ..evaluators.ollama import OllamaProvider
-from ..metrics.base import BaseMetric, MetricScope
+from ..metrics.base import BaseMetric, MetricNotApplicableError, MetricScope
 from ..metrics.registry import MetricRegistry
 from ..models.config import BenchmarkConfig
 from ..models.instruction import QuizInstructions
@@ -191,11 +191,26 @@ def _evaluate_quiz_level(
                 }
             )
             return None
+        except MetricNotApplicableError as e:
+            self.logger.info(
+                "Metric %s not applicable to quiz %s: %s", metric.name, quiz.quiz_id, e
+            )
+            self._cell_failures.append(
+                {
+                    "category": "skipped",
+                    "metric": metric.name,
+                    "evaluator": evaluator.model_name,
+                    "quiz_id": quiz.quiz_id,
+                    "question_id": None,
+                    "error": str(e),
+                }
+            )
+            return None
         except Exception as e:  # noqa: BLE001
             self.logger.error("Error evaluating quiz %s: %s", quiz.quiz_id, e)
             self._cell_failures.append(
                 {
-                    "category": "skipped",
+                    "category": "failed",
                     "metric": metric.name,
                     "evaluator": evaluator.model_name,
                     "quiz_id": quiz.quiz_id,
@@ -289,11 +304,26 @@ def _evaluate_question(
                 }
             )
             return None
+        except MetricNotApplicableError as e:
+            self.logger.info(
+                "Metric %s not applicable to question %s: %s", metric.name, question.question_id, e
+            )
+            self._cell_failures.append(
+                {
+                    "category": "skipped",
+                    "metric": metric.name,
+                    "evaluator": evaluator.model_name,
+                    "quiz_id": quiz.quiz_id,
+                    "question_id": question.question_id,
+                    "error": str(e),
+                }
+            )
+            return None
         except Exception as e:  # noqa: BLE001
             self.logger.error("Error evaluating question %s: %s", question.question_id, e)
             self._cell_failures.append(
                 {
-                    "category": "skipped",
+                    "category": "failed",
                     "metric": metric.name,
                     "evaluator": evaluator.model_name,
                     "quiz_id": quiz.quiz_id,
@@ -304,7 +334,7 @@ def _evaluate_question(
             return None
 
     def get_completeness_report(self) -> dict:
-        failed = [c for c in self._cell_failures if c["category"] == "transient"]
+        failed = [c for c in self._cell_failures if c["category"] in ("transient", "failed")]
         skipped = [c for c in self._cell_failures if c["category"] == "skipped"]
         present = self._attempted - len(self._cell_failures)
         return {

tests/test_runner.py

@@ -5,6 +5,7 @@
 
 import pytest
 
+from src.metrics.base import MetricNotApplicableError
 from src.models.config import MetricConfig
 from src.runners.benchmark import BenchmarkRunner
 
@@ -353,8 +354,7 @@ def test_evaluator_init_failure_raises(monkeypatch, sample_config):
     """A declared evaluator that cannot be created must abort the run, not be skipped.
 
     Silently dropping one leaves a sweep with fewer judges than planned, visible only in
-    metadata.json after the calls have been spent -- and two of the four models reported in
-    the paper are locally served.
+    metadata.json after the calls have been spent.
     """
     from src.evaluators.factory import LLMProviderFactory
     from src.runners.benchmark import BenchmarkRunner
@@ -519,7 +519,7 @@ def fail_first(*args, **kwargs):
 def test_skipped_error_does_not_fail_completeness(
     registered_metrics, mock_llm_provider, sample_config, sample_quiz
 ):
-    """A ValueError (e.g. distractor on true/false) is a skip, not a failure."""
+    """MetricNotApplicableError (distractor on true/false) is a skip, not a failure."""
     from dataclasses import replace
     from unittest.mock import patch
 
@@ -544,7 +544,7 @@ def test_skipped_error_does_not_fail_completeness(
     def skip_first(*args, **kwargs):
         call_count[0] += 1
         if call_count[0] == 1:
-            raise ValueError("Not applicable for true/false")
+            raise MetricNotApplicableError("Not applicable for true/false")
         return original_evaluate(*args, **kwargs)
 
     with patch.object(runner.metrics["clarity"], "evaluate", side_effect=skip_first):
@@ -554,3 +554,48 @@ def skip_first(*args, **kwargs):
     assert report["complete"]
     assert report["skipped"] == 1
     assert len(report["skipped_cells"]) == 1
+
+
+def test_truncation_error_fails_completeness(
+    registered_metrics, mock_llm_provider, sample_config, sample_quiz
+):
+    """max_tokens truncation is data loss, not a skip: it must flip `complete`.
+
+    Regression guard: 36 cells were lost to 'length limit was reached' truncation
+    yet the run reported complete and exited 0.
+    """
+    from dataclasses import replace
+    from unittest.mock import patch
+
+    config = replace(
+        sample_config,
+        runs=1,
+        metrics=[
+            MetricConfig(
+                name="clarity",
+                version="2.0",
+                evaluators=["mock_eval"],
+                parameters={},
+                enabled=True,
+            )
+        ],
+    )
+    runner = BenchmarkRunner(config)
+
+    original_evaluate = runner.metrics["clarity"].evaluate
+    call_count = [0]
+
+    def truncate_first(*args, **kwargs):
+        call_count[0] += 1
+        if call_count[0] == 1:
+            raise ValueError("Could not parse response content as the length limit was reached")
+        return original_evaluate(*args, **kwargs)
+
+    with patch.object(runner.metrics["clarity"], "evaluate", side_effect=truncate_first):
+        runner.run(quizzes=[sample_quiz], source_texts={"quiz_1": "text"})
+
+    report = runner.get_completeness_report()
+    assert not report["complete"], "truncation must not be absorbed as a skip"
+    assert report["failed"] == 1
+    assert report["skipped"] == 0
+    assert report["failed_cells"][0]["category"] == "failed"