mirror of
https://github.com/Imbad0202/academic-research-skills.git
synced 2026-09-14 13:51:17 +08:00
fix(eval-harness): gate binding per-class thresholds + drop pending tasks from lift baseline (#328) (#336)
The absolute-threshold CI gate inspected only aggregate_metric.passed, so a PR
that regressed a binding per-class threshold (e.g. citation_extraction.false.accuracy
below 0.85 while aggregate stayed >= 0.90, or rq_framing_patterns.fnr above 0.30
while balanced_accuracy passed) passed the gate when it should block. run_evals
stamps per_class[].passed against those declared thresholds; failed_tasks() now
iterates per_class too, keyed <task>.<class>.<metric> to match the lift gate.
check_ranking_lift._flatten_report let a pending task's placeholder
aggregate_metric.value: 0.0 (from _pending_result) enter the lift baseline, so
once the task landed its real value read as a zero-baseline change spuriously
needing [ranking-regression-acknowledged]. It now skips non-measured tasks.
Both consumers use the same positive skip-unless-measured guard
(task.get("status", "measured") != "measured") so a future non-measured status
is excluded consistently rather than re-opening the baseline-pollution gap.
Tests: new scripts/test__eval_threshold_gate.py (9 gate cases incl. the
per-class-fail-aggregate-pass regression + symmetry cases) wired into the CI
pytest manifest; 5 _flatten_report status-filter tests in test_check_ranking_lift.py.
Closes #328
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
committed by
GitHub
parent
6286fd0b66
commit
501366f197
@@ -114,6 +114,10 @@ path = "scripts/test_run_evals.py"
|
||||
id = "v3.10-184-check-ranking-lift"
|
||||
path = "scripts/test_check_ranking_lift.py"
|
||||
|
||||
[[pytest]]
|
||||
id = "v3.10-184-eval-threshold-gate"
|
||||
path = "scripts/test__eval_threshold_gate.py"
|
||||
|
||||
[[pytest]]
|
||||
id = "v3.10-184-evals-citation-extraction"
|
||||
path = "scripts/test_evals_citation_extraction.py"
|
||||
|
||||
@@ -12,8 +12,9 @@ CLI::
|
||||
|
||||
python -m scripts._eval_threshold_gate <report.json>
|
||||
|
||||
prints a comma-separated list of ``<task>.aggregate.<metric>`` failures (empty
|
||||
line if none) to stdout.
|
||||
prints a comma-separated list of failed threshold keys —
|
||||
``<task>.aggregate.<metric>`` and ``<task>.<class>.<metric>`` (empty line if
|
||||
none) — to stdout.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
@@ -23,20 +24,37 @@ from typing import Any
|
||||
|
||||
|
||||
def failed_tasks(report: dict[str, Any]) -> list[str]:
|
||||
"""Return ``<task>.aggregate.<metric>`` for each measured task below threshold.
|
||||
"""Return one key per measured threshold that failed — aggregate AND per-class.
|
||||
|
||||
Only tasks with ``status == "measured"`` that declare a threshold (so the
|
||||
measurer set ``aggregate_metric.passed``) are gated. ``passed is False`` is
|
||||
the failure signal — a task without a threshold (``passed`` absent) is not
|
||||
gated, and a pending/skipped task is never gated.
|
||||
measurer set ``.passed``) are gated. ``passed is False`` is the failure signal
|
||||
— a metric without a threshold (``passed`` absent) is not gated, and a
|
||||
pending/skipped task is never gated.
|
||||
|
||||
Both axes are binding: manifests declare aggregate AND per-class thresholds
|
||||
(e.g. citation_extraction: aggregate ``accuracy >= 0.90`` plus per_class
|
||||
``accuracy >= 0.85``; rq_framing_patterns: ``balanced_accuracy >= 0.75`` plus
|
||||
``fnr <= 0.30`` / ``fpr <= 0.20`` as per_class rows). run_evals stamps
|
||||
``per_class[].passed`` against the per-class threshold, so the gate must honour
|
||||
it too — else a PR that regresses only a per-class metric passes the gate when
|
||||
it should block (#328). Aggregate failures use ``<task>.aggregate.<metric>``;
|
||||
per-class failures use ``<task>.<class_name>.<metric>`` (matching the lift
|
||||
gate's per-class key shape).
|
||||
"""
|
||||
failures: list[str] = []
|
||||
for task in report.get("per_task", []):
|
||||
if task.get("status") != "measured":
|
||||
# Skip-unless-measured (symmetric with check_ranking_lift._flatten_report).
|
||||
if task.get("status", "measured") != "measured":
|
||||
continue
|
||||
agg = task.get("aggregate_metric") or {}
|
||||
if agg.get("passed") is False:
|
||||
failures.append(f"{task['task_name']}.aggregate.{agg.get('metric', '?')}")
|
||||
for pc in task.get("per_class", []):
|
||||
if pc.get("passed") is False:
|
||||
failures.append(
|
||||
f"{task['task_name']}.{pc.get('class_name', '?')}."
|
||||
f"{pc.get('metric', '?')}"
|
||||
)
|
||||
return failures
|
||||
|
||||
|
||||
|
||||
@@ -102,10 +102,27 @@ def _is_zero_baseline_change(signed_lift: float | str) -> bool:
|
||||
def _flatten_report(report: dict[str, Any]) -> dict[tuple[str, str, str], dict[str, Any]]:
|
||||
"""Map (task, class, metric) -> {value, direction} across aggregate + per_class.
|
||||
|
||||
The aggregate metric is keyed with class == "aggregate".
|
||||
The aggregate metric is keyed with class == "aggregate". Pending/skipped tasks
|
||||
are excluded: run_evals._pending_result emits a placeholder
|
||||
``aggregate_metric.value: 0.0`` for a not-yet-landed task, and letting that into
|
||||
the baseline makes the real value (once the task lands) read as a zero-baseline
|
||||
change spuriously flagged as a regression to acknowledge, rather than a new
|
||||
metric with no baseline (#328 P2). A task with no ``status`` key is treated as
|
||||
measured (pre-status reports stay valid).
|
||||
"""
|
||||
flat: dict[tuple[str, str, str], dict[str, Any]] = {}
|
||||
for task in report.get("per_task", []):
|
||||
# Skip-unless-measured (matches _eval_threshold_gate.failed_tasks). A
|
||||
# pending/skipped task is emitted by run_evals._pending_result with a
|
||||
# placeholder ``aggregate_metric.value: 0.0``; letting it into the baseline
|
||||
# makes the real value (once the task lands) read as a zero-baseline change
|
||||
# spuriously flagged as a regression to acknowledge, rather than a new
|
||||
# metric with no baseline (#328 P2). The positive guard (rather than a
|
||||
# ``status in {"pending","skipped"}`` blocklist) means a future non-measured
|
||||
# status is excluded too, instead of silently polluting the baseline. A task
|
||||
# with no ``status`` key is treated as measured (pre-status reports stay valid).
|
||||
if task.get("status", "measured") != "measured":
|
||||
continue
|
||||
task_name = task["task_name"]
|
||||
agg = task.get("aggregate_metric")
|
||||
if agg:
|
||||
|
||||
@@ -0,0 +1,125 @@
|
||||
"""Tests for scripts/_eval_threshold_gate.py (#184 Delta 3 / Fix 1; #328 per-class gate).
|
||||
|
||||
The absolute-threshold gate must fail a PR when ANY declared binding threshold
|
||||
regresses — aggregate OR per-class. Manifests declare both (e.g.
|
||||
citation_extraction: aggregate accuracy >= 0.90 AND per_class accuracy >= 0.85),
|
||||
run_evals stamps ``per_class[].passed`` against the per-class threshold, and the
|
||||
gate must honour it. #328: the gate previously inspected only the aggregate, so a
|
||||
per-class-fail-but-aggregate-pass report passed the gate when it should block.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
from scripts import _eval_threshold_gate as gate
|
||||
|
||||
|
||||
def _task(task_name="citation_extraction", agg_metric="accuracy",
|
||||
agg_passed=True, per_class=None, status="measured"):
|
||||
entry = {
|
||||
"task_name": task_name,
|
||||
"status": status,
|
||||
"aggregate_metric": {"metric": agg_metric, "passed": agg_passed},
|
||||
}
|
||||
if per_class is not None:
|
||||
entry["per_class"] = per_class
|
||||
return entry
|
||||
|
||||
|
||||
def _report(*tasks):
|
||||
return {"per_task": list(tasks)}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Aggregate gate (pre-#328 behaviour, must stay)
|
||||
# ---------------------------------------------------------------------------
|
||||
def test_aggregate_pass_no_failure():
|
||||
assert gate.failed_tasks(_report(_task(agg_passed=True))) == []
|
||||
|
||||
|
||||
def test_aggregate_fail_reported():
|
||||
assert gate.failed_tasks(_report(_task(agg_passed=False))) == [
|
||||
"citation_extraction.aggregate.accuracy"
|
||||
]
|
||||
|
||||
|
||||
def test_no_threshold_not_gated():
|
||||
# aggregate without a ``passed`` key (task declares no threshold) is not gated
|
||||
t = {"task_name": "t", "status": "measured", "aggregate_metric": {"metric": "accuracy"}}
|
||||
assert gate.failed_tasks(_report(t)) == []
|
||||
|
||||
|
||||
def test_pending_task_never_gated():
|
||||
assert gate.failed_tasks(_report(_task(status="pending", agg_passed=False))) == []
|
||||
|
||||
|
||||
def test_unknown_nonmeasured_status_never_gated():
|
||||
# symmetric with the lift gate: a future non-measured status is skipped, not gated
|
||||
assert gate.failed_tasks(_report(_task(status="error", agg_passed=False))) == []
|
||||
|
||||
|
||||
def test_missing_status_treated_as_measured():
|
||||
# back-compat: a task with no status key is gated (pre-status reports stay valid)
|
||||
t = {"task_name": "t", "aggregate_metric": {"metric": "accuracy", "passed": False}}
|
||||
assert gate.failed_tasks(_report(t)) == ["t.aggregate.accuracy"]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Per-class gate (#328 — the hole)
|
||||
# ---------------------------------------------------------------------------
|
||||
def test_per_class_fail_with_aggregate_pass_is_reported():
|
||||
"""The #328 regression case: aggregate passes, a binding per-class fails.
|
||||
|
||||
e.g. citation_extraction.false.accuracy drops below 0.85 while the aggregate
|
||||
stays >= 0.90. The gate MUST surface it.
|
||||
"""
|
||||
report = _report(_task(
|
||||
agg_passed=True,
|
||||
per_class=[
|
||||
{"class_name": "true", "metric": "accuracy", "passed": True},
|
||||
{"class_name": "false", "metric": "accuracy", "passed": False},
|
||||
{"class_name": "unresolvable", "metric": "accuracy", "passed": True},
|
||||
],
|
||||
))
|
||||
assert gate.failed_tasks(report) == ["citation_extraction.false.accuracy"]
|
||||
|
||||
|
||||
def test_per_class_key_shape_matches_lift_gate():
|
||||
"""Per-class failures use the ``<task>.<class>.<metric>`` shape (matches the
|
||||
lift gate's key shape, not the aggregate's ``<task>.aggregate.<metric>``)."""
|
||||
report = _report(_task(
|
||||
task_name="rq_framing_patterns", agg_metric="balanced_accuracy",
|
||||
agg_passed=True,
|
||||
per_class=[
|
||||
{"class_name": "fnr", "metric": "fnr", "passed": False},
|
||||
{"class_name": "fpr", "metric": "fpr", "passed": True},
|
||||
],
|
||||
))
|
||||
assert gate.failed_tasks(report) == ["rq_framing_patterns.fnr.fnr"]
|
||||
|
||||
|
||||
def test_both_aggregate_and_per_class_fail_reported():
|
||||
report = _report(_task(
|
||||
agg_passed=False,
|
||||
per_class=[{"class_name": "false", "metric": "accuracy", "passed": False}],
|
||||
))
|
||||
assert gate.failed_tasks(report) == [
|
||||
"citation_extraction.aggregate.accuracy",
|
||||
"citation_extraction.false.accuracy",
|
||||
]
|
||||
|
||||
|
||||
def test_per_class_without_threshold_not_gated():
|
||||
# a per_class row that carries no ``passed`` key (no per-class threshold) is
|
||||
# not gated — only ``passed is False`` blocks
|
||||
report = _report(_task(
|
||||
agg_passed=True,
|
||||
per_class=[{"class_name": "true", "metric": "accuracy"}],
|
||||
))
|
||||
assert gate.failed_tasks(report) == []
|
||||
|
||||
|
||||
def test_per_class_on_pending_task_never_gated():
|
||||
report = _report(_task(
|
||||
status="pending", agg_passed=True,
|
||||
per_class=[{"class_name": "false", "metric": "accuracy", "passed": False}],
|
||||
))
|
||||
assert gate.failed_tasks(report) == []
|
||||
@@ -64,6 +64,65 @@ def _report(task="citation_extraction", agg_value=0.95, agg_metric="accuracy",
|
||||
}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _flatten_report status filter (#328 P2 — pending-task baseline pollution)
|
||||
# ---------------------------------------------------------------------------
|
||||
def test_flatten_includes_measured_task():
|
||||
report = {"per_task": [{
|
||||
"task_name": "citation_extraction", "status": "measured",
|
||||
"aggregate_metric": {"metric": "accuracy", "value": 0.95},
|
||||
}]}
|
||||
flat = crl._flatten_report(report)
|
||||
assert ("citation_extraction", "aggregate", "accuracy") in flat
|
||||
assert flat[("citation_extraction", "aggregate", "accuracy")]["value"] == 0.95
|
||||
|
||||
|
||||
def test_flatten_skips_pending_task():
|
||||
"""A not-yet-landed task is emitted by run_evals._pending_result with
|
||||
``status: "pending"`` and a placeholder ``aggregate_metric.value: 0.0``. That
|
||||
placeholder must NOT enter the baseline — else once the task is implemented and
|
||||
produces a real value, compute_signed_lift(baseline=0.0, …) hits the zero-
|
||||
baseline branch and the brand-new metric is spuriously flagged as a regression
|
||||
needing acknowledgement (#328 P2)."""
|
||||
report = {"per_task": [{
|
||||
"task_name": "future_phase2_task", "status": "pending",
|
||||
"aggregate_metric": {"metric": "accuracy", "value": 0.0,
|
||||
"direction": "higher_is_better"},
|
||||
}]}
|
||||
flat = crl._flatten_report(report)
|
||||
assert flat == {}, "pending placeholder metric must not enter the baseline"
|
||||
|
||||
|
||||
def test_flatten_skips_skipped_task():
|
||||
report = {"per_task": [{
|
||||
"task_name": "skipped_task", "status": "skipped",
|
||||
"aggregate_metric": {"metric": "accuracy", "value": 0.0},
|
||||
}]}
|
||||
assert crl._flatten_report(report) == {}
|
||||
|
||||
|
||||
def test_flatten_skips_unknown_nonmeasured_status():
|
||||
"""A future non-measured status (e.g. "error") is excluded too — the positive
|
||||
skip-unless-measured guard, not a pending/skipped blocklist, is what prevents
|
||||
a new status from silently polluting the baseline again (#328 P2)."""
|
||||
report = {"per_task": [{
|
||||
"task_name": "errored_task", "status": "error",
|
||||
"aggregate_metric": {"metric": "accuracy", "value": 0.0},
|
||||
}]}
|
||||
assert crl._flatten_report(report) == {}
|
||||
|
||||
|
||||
def test_flatten_missing_status_treated_as_measured():
|
||||
"""Back-compat: a task with no ``status`` key (pre-status reports) still
|
||||
flattens — only an explicit pending/skipped status is dropped."""
|
||||
report = {"per_task": [{
|
||||
"task_name": "legacy_task",
|
||||
"aggregate_metric": {"metric": "accuracy", "value": 0.88},
|
||||
}]}
|
||||
flat = crl._flatten_report(report)
|
||||
assert ("legacy_task", "aggregate", "accuracy") in flat
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Gate semantics
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user