diff --git a/docs/analysis.md b/docs/analysis.md index ce82365..e4e2254 100644 --- a/docs/analysis.md +++ b/docs/analysis.md @@ -50,6 +50,8 @@ output/analysis/20260621T111935Z/ effect_sizes.json error_taxonomy_by_strategy.json tradeoff_correctness_efficiency.json + failure_rates.json + survivor_bias.json figures/README.md ``` @@ -73,6 +75,10 @@ Every file has a top-level `status` string: A `reason` string explains which factor was underpopulated. Re-runs will populate the analysis automatically once the corpus grows; no code change is needed. +- `"empty"`: there were no experimental rows to analyse at all, so the + analysis had nothing to run on rather than a factor too sparse to fit. + The distinction from `"skipped"` is deliberate: `"empty"` means no data, + `"skipped"` means data that will not support this particular test. ### 3.1 `descriptive.json` @@ -140,6 +146,131 @@ Per-strategy medians on both dimensions: `entity_id_f1` (correctness) and `cost_usd` / `duration_ms` (efficiency). Consumed by the dashboard's Pareto view. +### 3.9 `failure_rates.json` + +Failure rate and cause breakdown per strategy, model, and tier, plus an +`overall` block. This is the reliability counterpart to the accuracy +tables: `error_taxonomy_by_strategy.json` scores the content of diagrams +that were produced, while this file covers the runs that produced nothing +scorable at all. The two populations are disjoint. + +Rates are **pooled counts**: the denominator is every run attempted in the +group, not a mean of per-cell rates. A per-cell mean would weight a cell +with one run as heavily as a cell with five, which for a rate is the wrong +grain. This deliberately differs from the F1 path, which aggregates per +cell because it averages a score rather than counting events. + +Every cause appears in every `causes` block, including the zeros: an +absent key would be ambiguous between "never happened" and "not measured". +Controls are excluded (they never call a model). + +Payload shape (abridged): + +```json +{ + "status": "ok", + "statistic": "pooled_failure_rate", + "overall": { + "n_runs": 6000, + "n_failed": 478, + "failure_rate": 0.0797, + "causes": { "schema_violation": 261, "truncation": 135, "...": 0 } + }, + "by_strategy": [ + { + "strategy": "sop_based", + "n_runs": 1500, + "n_failed": 151, + "failure_rate": 0.1007, + "causes": { "schema_violation": 85, "...": 0 } + } + ] +} +``` + +### 3.10 `survivor_bias.json` + +Per strategy, the primary DV under `valid_only` (survivors only) against +`intent_to_treat` (every run, failures scored 0.0). The gap between them +is the survivor bias: a strategy that fails often looks better under +`valid_only` because its failures were dropped rather than scored, and +`survivor_bias` measures exactly how much that dropping flatters it. + +Reported per strategy rather than as one pooled figure, because the bias +scales with each strategy's failure rate: a single number would hide the +comparison that matters. `n_cells_dropped` counts cells where every run +failed, so `valid_only` had nothing to average. + +--- + +## 3a. Failure taxonomy + +`failure_rates.json` classifies each invalid run into exactly one primary +cause. The classifier lives in `src/maestro/analysis/failures.py`. + +### What counts as a failure + +Two disjoint shapes, both rejected by `RunResult.success`: + +- an **errored** run: `error` is set, and the string is the evidence. +- a **silent-empty** run: `error` is `None` but the diagram is missing or + blank. A provider can return whitespace without raising, so this is a + real category, not a data defect. + +### Where the evidence lives + +On a failed run, `run_results.raw_response` is always `NULL`: the error +result is built before any text exists. The failing text is retained one +level down, on the `sub_results` row that failed. Classification therefore +reads the run's `error` plus the first failed sub-result's `raw_response`. + +This means historical runs can be classified retroactively with no model +re-invocation, which is why re-scoring was never needed for this analysis. + +### The causes + +| Cause | Meaning | +|---|---| +| `rate_limit` | Provider returned a rate-limit error. | +| `timeout` | Request exceeded the client deadline. | +| `api_error` | Generic provider API error (the SDKs' catch-all base class). | +| `safety_block` | Response blocked by a content or safety filter. | +| `empty_output` | Provider returned no text, or only whitespace. | +| `truncation` | Response stopped mid-structure, consistent with a token limit. | +| `parse_error` | Output was not parseable in the requested format. | +| `schema_violation` | Output parsed but broke the Mermaid output contract. | +| `orchestration_error` | The framework misbehaved, not the model output. | +| `unknown` | No rule matched. Visible by design rather than mis-filed. | + +### Classification rules + +Causes are not mutually exclusive in practice: a truncated response is +usually *also* a parse error, because the truncation is what broke the +parse. Rather than multi-label (which makes rates hard to sum and +compare), each failure gets one primary cause under a fixed precedence, +most specific first: + +1. **Infrastructure** (`rate_limit`, `timeout`, `safety_block`). If the + API never returned, nothing downstream is meaningful. +2. **`empty_output`**. A missing response cannot be a parse error. +3. **`schema_violation`**, before the generic parse rule: well-formed text + that violates the output contract is a different failure from text that + is not parseable at all. +4. **`parse_error`**, promoted to **`truncation`** when the parser message + is truncation-shaped (an unterminated string, or a structure that simply + stops) *and* the raw response is long enough that running out of tokens + is plausible. Without the raw text the two are indistinguishable, so the + conservative `parse_error` label stands: under-reporting truncation is + safer than inventing it. +5. **`orchestration_error`**. +6. **`api_error`** last among the infrastructure family, since a more + specific subclass above must win over the catch-all base class. +7. **`unknown`** as the explicit fallback. + +Adding a provider means checking whether its error prefixes are covered. +An unmatched prefix surfaces as `unknown` rather than being mis-filed, +which is the failure mode this ordering exists to prevent. + --- ## 4. `report.md` @@ -216,8 +347,10 @@ NULL regardless of operating system. The corpus can under-populate a factor (only one input tier, only one model, only two strategies). Analyses that need at least two levels of that factor return `status="skipped"` with a `reason` string that names -the underpopulated factor. Downstream code should check the status -before reading terms: +the underpopulated factor. An analysis with no experimental rows at all +returns `status="empty"` instead, and carries no `reason`: nothing was +too sparse to fit, there was simply nothing to fit. Downstream code +should check the status before reading terms: ```python import json @@ -226,7 +359,8 @@ payload = json.loads(open("output/analysis/.../anova_strategy_by_tier.json").rea if payload["status"] == "ok": interaction_p = payload["terms"]["strategy:tier"]["p"] else: - print(f"skipped: {payload['reason']}") + # "empty" carries no reason, so do not index it unconditionally. + print(f"{payload['status']}: {payload.get('reason', 'no experimental rows')}") ``` Every consumer (the report builder, the dashboard) uses this pattern; a diff --git a/src/maestro/analysis/__init__.py b/src/maestro/analysis/__init__.py index 5f74eb1..0384d69 100644 --- a/src/maestro/analysis/__init__.py +++ b/src/maestro/analysis/__init__.py @@ -3,6 +3,7 @@ # Statistical analysis pipeline. Re-exported so callers can do # ``from maestro.analysis import describe, anova_strategy`` without reaching # into the submodule. The CLI lives in maestro.analysis.__main__. +from maestro.analysis.failures import FailureCause, classify_failure from maestro.analysis.statistics import ( DEFAULT_CONVENTION, INTENT_TO_TREAT, @@ -16,9 +17,12 @@ describe, effect_sizes, error_taxonomy_by_strategy, + failure_rates, load_dataframe, + load_failure_dataframe, mixed_effects_robustness, posthoc_strategy, + survivor_bias, tradeoff_correctness_efficiency, ) @@ -51,8 +55,14 @@ "describe", "effect_sizes", "error_taxonomy_by_strategy", + "failure_rates", "load_dataframe", + "load_failure_dataframe", "mixed_effects_robustness", "posthoc_strategy", + "survivor_bias", "tradeoff_correctness_efficiency", + # failure classification + "FailureCause", + "classify_failure", ] diff --git a/src/maestro/analysis/__main__.py b/src/maestro/analysis/__main__.py index 41f2a06..09de3b8 100644 --- a/src/maestro/analysis/__main__.py +++ b/src/maestro/analysis/__main__.py @@ -32,9 +32,12 @@ describe, effect_sizes, error_taxonomy_by_strategy, + failure_rates, load_dataframe, + load_failure_dataframe, mixed_effects_robustness, posthoc_strategy, + survivor_bias, tradeoff_correctness_efficiency, ) from maestro.analysis.timestamps import format_for_display @@ -59,8 +62,14 @@ ("descriptive.json", describe), ("error_taxonomy_by_strategy.json", error_taxonomy_by_strategy), ("tradeoff_correctness_efficiency.json", tradeoff_correctness_efficiency), + ("survivor_bias.json", survivor_bias), ] +# failure_rates is wired separately rather than added to _ANALYSES: it needs +# the failure frame as well as the run frame, so it does not share the +# single-DataFrame signature every entry above has. +_FAILURE_RATES_FILE = "failure_rates.json" + # Convention-dependent analyses: (stem, callable). Each is emitted once per # scoring convention as ``__.json`` (content-based naming: # the filename states both the test and the convention, so a file is never @@ -105,6 +114,12 @@ "tradeoff_correctness_efficiency.json + effect_sizes__intent_to_treat.json", "Correctness vs. efficiency trade-off across strategies.", ), + ( + "reliability", + "failure_rates.json + survivor_bias.json", + "Cross-cutting: how often does a strategy produce nothing usable, " + "from what cause, and how much does the valid-only view flatter it?", + ), ( "robustness", "anova_strategy_by_model__intent_to_treat.json + " @@ -393,6 +408,7 @@ def main(argv: list[str] | None = None) -> int: # enforces that at the boundary instead of relying on a no-op commit. with get_readonly_connection(args.db) as conn: df = load_dataframe(conn) + failures = load_failure_dataframe(conn) if df.empty: print( @@ -407,6 +423,10 @@ def main(argv: list[str] | None = None) -> int: results[filename] = payload _write_json(run_dir / filename, payload) + failure_payload = failure_rates(df, failures) + results[_FAILURE_RATES_FILE] = failure_payload + _write_json(run_dir / _FAILURE_RATES_FILE, failure_payload) + # Convention-dependent analyses: one file per (analysis, convention). for stem, fn in _CONVENTION_ANALYSES: for convention in _CONVENTIONS: diff --git a/src/maestro/analysis/failures.py b/src/maestro/analysis/failures.py new file mode 100644 index 0000000..568f389 --- /dev/null +++ b/src/maestro/analysis/failures.py @@ -0,0 +1,201 @@ +""" +Failure-mode classification for runs that produced no valid output. + +The harness separates reliability (did a run produce anything usable) from +accuracy (how good was it). The accuracy side is analysed in detail by +``statistics.py``; without this module the whole reliability side collapses +to a single failure count, which is not enough to say *why* a strategy is +less reliable. + +## What counts as a failure + +Two disjoint shapes, both of which ``RunResult.success`` rejects: + +- an **errored** run: ``error`` is set, and the string is the classification + evidence. +- a **silent-empty** run: ``error`` is None but the diagram is missing or + blank. No error string exists, so these classify as ``EMPTY_OUTPUT`` from + the row shape alone. They are a real category, not a data defect: a + provider can return whitespace without raising. + +## Where the evidence lives + +For a failed *run*, ``run_results.raw_response`` is NULL: the top-level error +path builds its result before any text exists. The failing text is retained +one level down, on the ``sub_results`` row that failed. Classification +therefore reads the run's ``error`` string and, when a caller supplies it, +the failed sub-result's ``raw_response``. That is why ``classify_failure`` +takes the raw text as a separate optional argument rather than digging it +out of the run row: the run row does not have it. + +## Precedence + +Categories are not mutually exclusive in practice: a truncated response is +usually *also* a JSON parse error, because the truncation is what broke the +parse. Rather than multi-label (which makes rates hard to sum and compare), +each failure gets one primary cause under a fixed precedence, most specific +first. ``_RULES`` is ordered, and the first match wins: + +1. Infrastructure causes that preempt any output judgement (rate limit, + timeout, API error, safety block). If the API never returned, nothing + downstream is meaningful. +2. Empty output. A missing response cannot be a parse error. +3. Truncation, checked *before* the parse rules so a cut-off response is + attributed to the token limit that caused it rather than to the parse + error that is merely its symptom. +4. Schema and parse violations, the ordinary "model wrote the wrong thing" + causes. +5. Orchestration errors: the framework itself misbehaved. +6. ``UNKNOWN`` as the explicit fallback, so an unrecognised string is + visible in the breakdown instead of silently joining a real category. + +Adding a provider means checking whether its error prefixes are covered +here; an unmatched prefix shows up as ``UNKNOWN`` rather than being +mis-filed, which is the failure mode this ordering is built to avoid. +""" + +from __future__ import annotations + +import re +from enum import StrEnum + +# Heuristic ceiling for calling a response truncated. Applied only to text +# that also failed to parse: a long *valid* response is not truncated, and a +# short unparseable one is malformed rather than cut off. The value is a +# judgement call, so it is named rather than inlined at the comparison. +_TRUNCATION_MIN_CHARS = 200 + + +class FailureCause(StrEnum): + """ + Primary cause of one invalid generation. + + A ``StrEnum`` so the value serialises to a plain string in the JSON + breakdown and compares cleanly against DataFrame columns, matching how + ``Strategy`` and ``Tier`` are already handled in ``schemas.py``. + """ + + RATE_LIMIT = "rate_limit" + TIMEOUT = "timeout" + API_ERROR = "api_error" + SAFETY_BLOCK = "safety_block" + EMPTY_OUTPUT = "empty_output" + TRUNCATION = "truncation" + PARSE_ERROR = "parse_error" + SCHEMA_VIOLATION = "schema_violation" + ORCHESTRATION_ERROR = "orchestration_error" + UNKNOWN = "unknown" + + +def _rule(pattern: str) -> re.Pattern[str]: + """ + Compile one classification pattern. + + Every pattern is case-insensitive: the error text comes from vendor SDKs + and third-party frameworks, so its casing is not ours to rely on. A + provider rewording ``APIError`` to ``ApiError`` would otherwise push a + whole category into ``UNKNOWN`` silently. Centralised here so a new rule + cannot forget the flag. + """ + return re.compile(pattern, re.IGNORECASE) + + +# Ordered (cause, pattern) rules. Order *is* the precedence documented in the +# module docstring: the first match wins, so never reorder without re-reading +# it. Patterns are anchored on the literal prefixes the providers and +# strategies emit (``RateLimitError:``, ``EmptyResponse:``, ``invalid JSON:``, +# ...) rather than on loose keywords, so an unrelated message that merely +# mentions "timeout" in prose does not get mis-filed. +_RULES: tuple[tuple[FailureCause, re.Pattern[str]], ...] = ( + # 1. Infrastructure. These preempt everything: no usable output existed. + (FailureCause.RATE_LIMIT, _rule(r"\bRateLimitError\b")), + (FailureCause.TIMEOUT, _rule(r"\b(?:APITimeoutError|TimeoutError)\b")), + (FailureCause.SAFETY_BLOCK, _rule(r"\b(?:BlockedResponse|ContentFilter)\b")), + # 2. Empty output, before the parse rules: nothing to parse. + ( + FailureCause.EMPTY_OUTPUT, + _rule(r"\bEmptyResponse\b|\bempty output from provider\b"), + ), + # CrewAI surfaces an empty LLM reply as its own kickoff message rather + # than an EmptyResponse; it is the same underlying cause. + ( + FailureCause.EMPTY_OUTPUT, + _rule(r"Invalid response from LLM call\s*-\s*None or empty"), + ), + # 3. Schema violations. Checked before the generic parse rule because the + # structural Mermaid checks (empty label bracket, unbalanced subgraph) + # describe well-formed text that violates the output contract, which is a + # different failure from text that is not parseable at all. + ( + FailureCause.SCHEMA_VIOLATION, + _rule(r"empty node label bracket|unbalanced subgraph/end"), + ), + # 4. Parse errors: the model did not produce the requested format. + (FailureCause.PARSE_ERROR, _rule(r"\binvalid JSON\b|\bJSONDecodeError\b")), + # 5. Orchestration: the framework misbehaved, not the model output. + ( + FailureCause.ORCHESTRATION_ERROR, + _rule(r"Single-call invariant violated|kickoff raised"), + ), + # Generic API error last among the infrastructure family: APIError is the + # SDKs' catch-all base class, so a more specific subclass above must win. + (FailureCause.API_ERROR, _rule(r"\bAPIError\b")), +) + +# Signatures of a response cut off mid-token. An unterminated string or a +# structure that simply stops is what truncation looks like after the fact; +# the provider does not tell us the token limit was hit. Case-insensitive for +# the same reason as the rules above: the text is the json module's, not ours. +_TRUNCATION_PATTERN = _rule( + r"Unterminated string|Expecting value: line \d+ column \d+ \(char \d+\)" +) + + +def classify_failure( + error: str | None, + raw_response: str | None = None, +) -> FailureCause: + """ + Resolve one failed run to its single primary cause. + + ``error`` is the run's error string; ``raw_response`` is the failing text + from the sub-result that produced it, when available (see the module + docstring on why it is a separate argument). The raw text is used only to + separate truncation from an ordinary parse error, which the error string + alone cannot distinguish. + + A ``None``/blank ``error`` classifies as ``EMPTY_OUTPUT``: that is the + silent-empty shape, where a run has no error but no diagram either. + Callers must only pass runs that actually failed, since a *successful* + run also has ``error is None`` and would classify the same way. + """ + if error is None or not error.strip(): + return FailureCause.EMPTY_OUTPUT + + for cause, pattern in _RULES: + if pattern.search(error): + # Truncation masquerades as a parse error: same message, different + # cause. Promote it only with corroborating evidence, a long raw + # response that stops mid-structure, so an ordinary malformed + # short reply is not relabelled. + if cause is FailureCause.PARSE_ERROR and _looks_truncated( + error, raw_response + ): + return FailureCause.TRUNCATION + return cause + + return FailureCause.UNKNOWN + + +def _looks_truncated(error: str, raw_response: str | None) -> bool: + """ + Whether a parse failure is better explained by truncation. + + Requires both a truncation-shaped parser message and a raw response long + enough that running out of tokens is plausible. Without the raw text we + cannot tell the two apart, so we stay conservative and keep the parse-error + label: under-reporting truncation is safer than inventing it. + """ + if raw_response is None or len(raw_response) < _TRUNCATION_MIN_CHARS: + return False + return bool(_TRUNCATION_PATTERN.search(error)) diff --git a/src/maestro/analysis/statistics.py b/src/maestro/analysis/statistics.py index 2ac654e..06d3fdd 100644 --- a/src/maestro/analysis/statistics.py +++ b/src/maestro/analysis/statistics.py @@ -54,7 +54,12 @@ ANOVA conclusion survives a model that accounts for the crossed grouping structure directly instead of by pre-averaging. - **Error taxonomy**: descriptive characterization of the eight taxonomy - counts per strategy (exploratory; no inferential test). + counts per strategy (exploratory; no inferential test). This scores the + *content* of a diagram that was produced; the failure analysis below covers + runs that produced nothing scorable at all. The two are disjoint populations. +- **Failure modes**: per-strategy/model/tier failure rates broken down by + primary cause (see ``failures.py``), plus a survivor-bias number saying how + much the valid-only view flatters each strategy. - **Correctness/efficiency trade-off**: per-strategy correctness against cost and latency, plus a correctness-to-cost ratio. @@ -83,7 +88,8 @@ import sqlite3 from typing import TYPE_CHECKING, Any, Literal -from maestro.db.queries import fetch_analysis_rows +from maestro.analysis.failures import FailureCause, classify_failure +from maestro.db.queries import fetch_analysis_rows, fetch_failure_rows from maestro.experiment_config import CONTROL_STRATEGIES from maestro.schemas import Strategy @@ -892,6 +898,182 @@ def error_taxonomy_by_strategy(df: "pd.DataFrame") -> dict[str, Any]: return out +# --------------------------------------------------------------------------- +# Failure modes +# --------------------------------------------------------------------------- + + +def load_failure_dataframe(conn: sqlite3.Connection) -> "pd.DataFrame": + """ + Load every invalid run, classified by primary failure cause. Read-only. + + Adds a ``failure_cause`` column via ``classify_failure``. The frame is a + different population from ``load_dataframe``'s: one row per *failed* run + rather than per completed run, so the two are never concatenated. Failure + *rates* need both, which is why ``failure_rates`` takes the two frames. + + Returns an empty DataFrame when nothing failed, which is a legitimate + result (a perfect run), not an error. + """ + import pandas as pd + + rows = [dict(r) for r in fetch_failure_rows(conn)] + if not rows: + return pd.DataFrame() + df = pd.DataFrame(rows) + df["failure_cause"] = [ + classify_failure(r["error"], r["failing_raw_response"]).value for r in rows + ] + return df + + +def failure_rates(df: "pd.DataFrame", failures: "pd.DataFrame") -> dict[str, Any]: + """ + Failure rate and cause breakdown per strategy, model, and tier, mirroring + the grouping of the accuracy tables so the reliability and accuracy sides + of a strategy can be read against each other. + + Rates are **pooled counts**, not means of per-cell rates: the denominator + is every run attempted in the group. A per-cell mean would weight a cell + with one run as heavily as a cell with five, which for a rate is the wrong + grain. This deliberately differs from the F1 path, which aggregates per + cell because it is averaging a score rather than counting events. + + Controls are excluded: they never call a model, so their failure rate is + zero by construction and would dilute every pooled denominator it entered. + """ + exp = _experimental(df) + out: dict[str, Any] = { + "schema_version": SCHEMA_VERSION, + "analysis": "failure_rates", + "grouping": ["strategy", "model", "tier"], + "statistic": "pooled_failure_rate", + "causes": [c.value for c in FailureCause], + "excludes_controls": True, + "overall": {}, + "by_strategy": [], + "by_model": [], + "by_tier": [], + } + if exp.empty: + out["status"] = "empty" + return out + + exp_failures = _experimental(failures) if not failures.empty else failures + out["overall"] = _failure_summary(exp, exp_failures) + for field in ("strategy", "model", "tier"): + out[f"by_{field}"] = _failure_breakdown(exp, exp_failures, field) + out["status"] = "ok" + return out + + +def _failure_breakdown( + runs: "pd.DataFrame", failures: "pd.DataFrame", field: str +) -> list[dict[str, Any]]: + """One summary per observed level of ``field``, ordered by level.""" + entries: list[dict[str, Any]] = [] + for level, group in runs.groupby(field, dropna=False): + matching = ( + failures[failures[field] == level] if not failures.empty else failures + ) + entries.append({field: _to_native(level), **_failure_summary(group, matching)}) + return entries + + +def _failure_summary(runs: "pd.DataFrame", failures: "pd.DataFrame") -> dict[str, Any]: + """ + Counts, pooled rate, and per-cause counts for one group. + + ``causes`` carries every ``FailureCause`` member, including the zeros: a + cause absent from the output would be ambiguous between "never happened" + and "not measured", and the zeros are what make two groups comparable + column by column. + """ + n_runs = int(len(runs)) + n_failed = int(len(failures)) + counts = ( + failures["failure_cause"].value_counts().to_dict() if not failures.empty else {} + ) + return { + "n_runs": n_runs, + "n_failed": n_failed, + "failure_rate": (n_failed / n_runs) if n_runs else None, + "causes": {c.value: int(counts.get(c.value, 0)) for c in FailureCause}, + } + + +def survivor_bias( + df: "pd.DataFrame", convention: ScoringConvention = VALID_ONLY +) -> dict[str, Any]: + """ + Quantify survivor bias: does the surviving subset differ systematically + from the full set? + + Reports the primary DV under ``valid_only`` (survivors only) against + ``intent_to_treat`` (every run, failures scored 0.0), per strategy. The + gap between them *is* the bias: a strategy that fails often looks better + under valid_only, because its failures were dropped rather than scored, + and the difference measures exactly how much that dropping flatters it. + + Reported as a number per strategy rather than a single test statistic, + because the bias is not uniform: it scales with each strategy's failure + rate, so one pooled figure would hide the comparison that matters. + + The default convention names the survivor side explicitly; passing + ``INTENT_TO_TREAT`` is accepted but yields a zero gap by construction, + so it exists only for symmetry with the other convention-taking + functions rather than as a useful call. + """ + exp = _experimental(df) + out: dict[str, Any] = { + "schema_version": SCHEMA_VERSION, + "analysis": "survivor_bias", + "dependent_variable": PRIMARY_DV, + "survivor_convention": convention, + "reference_convention": INTENT_TO_TREAT, + "excludes_controls": True, + "by_strategy": [], + } + if exp.empty: + out["status"] = "empty" + return out + + all_runs = aggregate_experimental(df, INTENT_TO_TREAT) + survivors = aggregate_experimental(df, convention) + if all_runs.empty: + out["status"] = "empty" + return out + + for strategy, group in all_runs.groupby("strategy"): + kept = ( + survivors[survivors["strategy"] == strategy] + if not survivors.empty + else survivors + ) + all_mean = _to_native(group[PRIMARY_DV].mean()) + kept_mean = _to_native(kept[PRIMARY_DV].mean()) if not kept.empty else None + gap = ( + kept_mean - all_mean + if kept_mean is not None and all_mean is not None + else None + ) + out["by_strategy"].append( + { + "strategy": strategy, + "n_cells_all": int(len(group)), + "n_cells_survivors": int(len(kept)), + # Cells lost entirely: every run in them failed, so valid_only + # has nothing to average and the cell vanishes from the frame. + "n_cells_dropped": int(len(group)) - int(len(kept)), + "mean_all_runs": all_mean, + "mean_survivors": kept_mean, + "survivor_bias": gap, + } + ) + out["status"] = "ok" + return out + + # --------------------------------------------------------------------------- # Correctness / efficiency trade-off # --------------------------------------------------------------------------- diff --git a/src/maestro/db/queries.py b/src/maestro/db/queries.py index ef417a5..3492813 100644 --- a/src/maestro/db/queries.py +++ b/src/maestro/db/queries.py @@ -232,6 +232,56 @@ def fetch_all_results(conn: sqlite3.Connection) -> list[sqlite3.Row]: ).fetchall() +def fetch_failure_rows(conn: sqlite3.Connection) -> list[sqlite3.Row]: + """ + One row per *invalid* run, for failure-mode classification. Read-only. + + Covers both failure shapes ``RunResult.success`` rejects: an errored run, + and a run with no error but a missing or blank diagram. The WHERE clause + is the negation of the success condition used by ``fetch_completed_cells``, + so the two partition the run set and a row can never be counted as both. + + ``failing_raw_response`` comes from a correlated subquery over + ``sub_results`` rather than from ``run_results``: on a failed run the + top-level ``raw_response`` is always NULL, because the error result is + built before any text exists. The failing text is retained one level down, + on the sub-result whose step failed. The subquery takes the *first* failed + step by ``step_number``, which is the step that actually broke the run + (later steps never ran). It stays NULL when the framework produced no text + at all, which is itself classifiable evidence. + + Left-joining ``sub_results`` instead would multiply a run into one row per + sub-result and inflate every failure count; the subquery keeps the grain at + one row per run. + """ + return conn.execute( + """ + SELECT + c.run_id AS run_id, + c.strategy AS strategy, + c.model AS model, + c.example_id AS example_id, + c.tier AS tier, + c.run_number AS run_number, + r.error AS error, + ( + SELECT s.raw_response + FROM sub_results s + WHERE s.run_id = c.run_id + AND s.error IS NOT NULL + ORDER BY s.step_number + LIMIT 1 + ) AS failing_raw_response + FROM run_configs c + JOIN run_results r ON c.run_id = r.run_id + WHERE r.error IS NOT NULL + OR r.output_diagram_code IS NULL + OR TRIM(r.output_diagram_code) = '' + ORDER BY c.timestamp + """, + ).fetchall() + + def fetch_analysis_rows(conn: sqlite3.Connection) -> list[sqlite3.Row]: """ Three-way join (run_configs ⋈ run_results, then LEFT ⋈ metric_results) diff --git a/tests/analysis/test_failures.py b/tests/analysis/test_failures.py new file mode 100644 index 0000000..76cba82 --- /dev/null +++ b/tests/analysis/test_failures.py @@ -0,0 +1,473 @@ +""" +Tests for failure-mode classification (src/maestro/analysis/failures.py) and +the failure analyses in statistics.py. + +Two layers, deliberately separated: + + 1. ``classify_failure`` is pure, so it is tested directly against the error + strings the codebase actually emits. The strings are copied from the + provider/strategy error paths and from the production database, not + invented, so a reworded error message fails a test instead of silently + becoming ``UNKNOWN`` in a live run. + 2. The rate and survivor-bias functions are exercised through the *real* + schema (db.client SCHEMA + db.queries inserts), matching the approach in + test_statistics.py: the correlated sub_results subquery in + fetch_failure_rows is exactly the part worth testing against real SQL. +""" + +from __future__ import annotations + +import sqlite3 +import uuid + +import pytest + +pytest.importorskip("pandas") + +from maestro.analysis.failures import ( # noqa: E402 + FailureCause, + classify_failure, +) +from maestro.analysis.statistics import ( # noqa: E402 + failure_rates, + load_dataframe, + load_failure_dataframe, + survivor_bias, +) +from maestro.db.client import SCHEMA # noqa: E402 +from maestro.db.queries import ( # noqa: E402 + fetch_failure_rows, + insert_run_config, + insert_run_result, + insert_sub_result, +) +from maestro.schemas import ( # noqa: E402 + RunConfig, + RunResult, + Strategy, + SubResult, + Tier, +) + + +def _conn() -> sqlite3.Connection: + conn = sqlite3.connect(":memory:") + conn.row_factory = sqlite3.Row + conn.executescript(SCHEMA) + return conn + + +# --------------------------------------------------------------------------- +# classify_failure +# --------------------------------------------------------------------------- + +# Error strings taken verbatim from the provider error paths and from the +# production database, paired with the cause each must resolve to. +_REAL_ERRORS = [ + ( + "Step 3 (generate_mermaid) failed: Invalid generate_mermaid output on " + 'attempt 2: empty node label bracket (e.g. node_id[""])', + FailureCause.SCHEMA_VIOLATION, + ), + ( + "Step 3 (generate_mermaid) failed: Invalid generate_mermaid output on " + "attempt 2: unbalanced subgraph/end (6 subgraph, 5 end)", + FailureCause.SCHEMA_VIOLATION, + ), + ( + "Step 1 (extract_entities) failed: Invalid extract_entities output on " + "attempt 2: invalid JSON: Expecting property name enclosed in double " + "quotes: line 26 column 5 (char 500)", + FailureCause.PARSE_ERROR, + ), + ( + "Step 3 (generate_mermaid) failed: empty output from provider", + FailureCause.EMPTY_OUTPUT, + ), + ( + "Step 3 (generate_mermaid) failed: CrewAI kickoff raised on attempt 2: " + "Invalid response from LLM call - None or empty.", + FailureCause.EMPTY_OUTPUT, + ), + ( + "Step 3 (generate_mermaid) failed: Single-call invariant violated on " + "attempt 2: expected 1 new call, got 3", + FailureCause.ORCHESTRATION_ERROR, + ), + ("RateLimitError: 429 rate limit exceeded", FailureCause.RATE_LIMIT), + ("TimeoutError: request timed out", FailureCause.TIMEOUT), + ("APITimeoutError: deadline exceeded", FailureCause.TIMEOUT), + ("APIError: 500 internal server error", FailureCause.API_ERROR), + ("BlockedResponse: response blocked by safety filter", FailureCause.SAFETY_BLOCK), + ("EmptyResponse: openai returned no content", FailureCause.EMPTY_OUTPUT), + ("EmptyResponse: anthropic returned no text content", FailureCause.EMPTY_OUTPUT), +] + + +@pytest.mark.parametrize("error,expected", _REAL_ERRORS) +def test_classifies_real_error_strings(error: str, expected: FailureCause) -> None: + assert classify_failure(error) is expected + + +def test_no_error_is_empty_output() -> None: + """The silent-empty shape: no error string, but no diagram either.""" + assert classify_failure(None) is FailureCause.EMPTY_OUTPUT + assert classify_failure(" ") is FailureCause.EMPTY_OUTPUT + + +def test_unrecognized_error_is_unknown_not_misfiled() -> None: + """ + An unmatched string must surface as UNKNOWN. Silently absorbing it into a + real category is the failure mode the ordered rules exist to prevent. + """ + assert classify_failure("KrakenError: the kraken woke up") is FailureCause.UNKNOWN + + +def test_specific_api_error_beats_generic_api_error() -> None: + """ + APIError is the SDKs' catch-all base class, so a message naming a more + specific subclass must not be filed under the generic cause. + """ + assert classify_failure("RateLimitError: 429") is FailureCause.RATE_LIMIT + assert classify_failure("APITimeoutError: slow") is FailureCause.TIMEOUT + + +def test_truncation_promoted_over_parse_error_with_long_raw() -> None: + """ + A long response that stops mid-string is truncation, not a parse error: + the parse failure is the symptom, the token limit is the cause. + """ + error = ( + "Step 1 (extract_entities) failed: Invalid extract_entities output on " + "attempt 2: invalid JSON: Unterminated string starting at: line 194 " + "column 15 (char 4400)" + ) + assert classify_failure(error, "x" * 900) is FailureCause.TRUNCATION + + +def test_truncation_not_promoted_without_corroborating_raw() -> None: + """ + Without a long raw response there is no evidence of truncation, so the + conservative parse-error label stands. Under-reporting truncation is + safer than inventing it. + """ + error = ( + "Invalid extract_entities output on attempt 2: invalid JSON: " + "Unterminated string starting at: line 4 column 1 (char 40)" + ) + assert classify_failure(error, "short") is FailureCause.PARSE_ERROR + assert classify_failure(error, None) is FailureCause.PARSE_ERROR + + +def test_empty_output_precedes_parse_rules() -> None: + """Nothing came back, so there was nothing to parse.""" + assert ( + classify_failure("EmptyResponse: no content", "x" * 900) + is FailureCause.EMPTY_OUTPUT + ) + + +@pytest.mark.parametrize( + "error,expected", + [ + ("ratelimiterror: 429", FailureCause.RATE_LIMIT), + ("RATELIMITERROR: 429", FailureCause.RATE_LIMIT), + ("apierror: 500", FailureCause.API_ERROR), + ("ApiError: 500", FailureCause.API_ERROR), + ("emptyresponse: no content", FailureCause.EMPTY_OUTPUT), + ("Invalid Json: bad", FailureCause.PARSE_ERROR), + ("Empty Node Label Bracket", FailureCause.SCHEMA_VIOLATION), + ], +) +def test_classification_is_case_insensitive(error: str, expected: FailureCause) -> None: + """ + Error text originates in vendor SDKs and third-party frameworks, so its + casing is not ours to rely on: a provider rewording ``APIError`` to + ``ApiError`` must not silently push a whole category into UNKNOWN. + """ + assert classify_failure(error) is expected + + +def test_truncation_detection_is_case_insensitive() -> None: + """The truncation signatures come from the json module, not from us.""" + error = "invalid json: unterminated string starting at: line 9 column 2" + assert classify_failure(error, "x" * 900) is FailureCause.TRUNCATION + + +# --------------------------------------------------------------------------- +# fetch_failure_rows / failure_rates +# --------------------------------------------------------------------------- + + +def _insert_run( + conn: sqlite3.Connection, + *, + strategy: Strategy, + error: str | None, + diagram: str | None = "graph TD; a-->b", + sub_errors: list[tuple[int, str | None, str | None]] | None = None, + model: str = "model-a", + tier: Tier = Tier.SIMPLE, + run_number: int = 1, +) -> uuid.UUID: + """ + Insert one config+result, plus optional sub_results. + + ``sub_errors`` entries are (step_number, error, raw_response), letting a + test build the multi-step shape where the failing text lives on the + sub-result rather than the run row. + """ + run_id = uuid.uuid4() + insert_run_config( + conn, + RunConfig( + run_id=run_id, + strategy=strategy, + model=model, + example_id="ex_01", + tier=tier, + run_number=run_number, + ), + ) + insert_run_result( + conn, + RunResult( + run_id=run_id, + output_diagram_code=diagram, + prompt_tokens=10, + completion_tokens=10, + duration_ms=100, + cost_usd=0.001, + error=error, + ), + ) + for step, sub_error, raw in sub_errors or []: + insert_sub_result( + conn, + SubResult( + run_id=run_id, + step_number=step, + step_name=f"step_{step}", + output_text=None if sub_error else "ok", + raw_response=raw, + prompt_tokens=5, + completion_tokens=5, + duration_ms=50, + cost_usd=0.0005, + error=sub_error, + ), + ) + return run_id + + +def test_fetch_failure_rows_covers_both_failure_shapes() -> None: + """ + An errored run and a no-error-but-blank-diagram run are both failures; + a successful run is neither. + """ + conn = _conn() + _insert_run(conn, strategy=Strategy.SOP_BASED, error="APIError: 500") + _insert_run(conn, strategy=Strategy.SINGLE_AGENT, error=None, diagram=" ") + _insert_run(conn, strategy=Strategy.SINGLE_AGENT, error=None, diagram=None) + _insert_run(conn, strategy=Strategy.SINGLE_AGENT, error=None) + + rows = fetch_failure_rows(conn) + assert len(rows) == 3 + + +def test_failing_raw_response_comes_from_first_failed_step() -> None: + """ + The raw text is pulled from the earliest failed sub-result: that is the + step that broke the run, and later steps never ran. + """ + conn = _conn() + _insert_run( + conn, + strategy=Strategy.SOP_BASED, + error="Step 1 (extract_entities) failed: invalid JSON: Expecting value", + diagram=None, + sub_errors=[ + (1, "invalid JSON: Expecting value", "the raw step-1 text"), + (2, "downstream failure", "later text"), + ], + ) + rows = fetch_failure_rows(conn) + assert len(rows) == 1 + assert rows[0]["failing_raw_response"] == "the raw step-1 text" + + +def test_one_row_per_run_despite_many_sub_results() -> None: + """ + The correlated subquery must not multiply a run into one row per + sub-result, which would inflate every failure count. + """ + conn = _conn() + _insert_run( + conn, + strategy=Strategy.CREW_AI, + error="Step 1 failed: invalid JSON: Expecting value", + diagram=None, + sub_errors=[ + (i, "invalid JSON: Expecting value", f"raw {i}") for i in range(1, 6) + ], + ) + assert len(fetch_failure_rows(conn)) == 1 + + +def test_failure_rates_are_pooled_counts() -> None: + """ + Rate is failures over runs attempted in the group, not a mean of per-cell + rates: 1 failure in 4 runs is 0.25 regardless of how the runs split across + cells. + """ + conn = _conn() + for i in range(3): + _insert_run(conn, strategy=Strategy.SOP_BASED, error=None, run_number=i + 1) + _insert_run( + conn, + strategy=Strategy.SOP_BASED, + error="APIError: 500", + diagram=None, + run_number=4, + ) + + payload = failure_rates(load_dataframe(conn), load_failure_dataframe(conn)) + assert payload["status"] == "ok" + assert payload["overall"]["n_runs"] == 4 + assert payload["overall"]["n_failed"] == 1 + assert payload["overall"]["failure_rate"] == pytest.approx(0.25) + assert payload["overall"]["causes"]["api_error"] == 1 + + +def test_failure_rates_report_every_cause_including_zeros() -> None: + """ + A cause absent from the output would be ambiguous between "never happened" + and "not measured"; the zeros are what make groups comparable. + """ + conn = _conn() + _insert_run(conn, strategy=Strategy.SOP_BASED, error="APIError: 500", diagram=None) + payload = failure_rates(load_dataframe(conn), load_failure_dataframe(conn)) + assert set(payload["overall"]["causes"]) == {c.value for c in FailureCause} + assert payload["overall"]["causes"]["rate_limit"] == 0 + + +def test_failure_rates_exclude_controls() -> None: + """Controls never call a model, so they must not dilute the denominator.""" + conn = _conn() + _insert_run(conn, strategy=Strategy.SOP_BASED, error="APIError: 500", diagram=None) + for i in range(5): + _insert_run(conn, strategy=Strategy.NULL_CONTROL, error=None, run_number=i + 1) + + payload = failure_rates(load_dataframe(conn), load_failure_dataframe(conn)) + assert payload["overall"]["n_runs"] == 1 + assert payload["overall"]["failure_rate"] == pytest.approx(1.0) + strategies = {row["strategy"] for row in payload["by_strategy"]} + assert Strategy.NULL_CONTROL.value not in strategies + + +def test_failure_rates_empty_db_is_status_empty_not_crash() -> None: + conn = _conn() + payload = failure_rates(load_dataframe(conn), load_failure_dataframe(conn)) + assert payload["status"] == "empty" + + +def test_load_failure_dataframe_empty_when_nothing_failed() -> None: + """A perfect run is a legitimate result, not an error.""" + conn = _conn() + _insert_run(conn, strategy=Strategy.SOP_BASED, error=None) + assert load_failure_dataframe(conn).empty + + +# --------------------------------------------------------------------------- +# survivor_bias +# --------------------------------------------------------------------------- + + +def test_survivor_bias_is_zero_without_failures() -> None: + """ + With nothing dropped, the two conventions see the same runs, so the gap + must be exactly zero rather than merely small. + """ + pytest.importorskip("statsmodels") + conn = _conn() + for i in range(3): + _insert_run_with_metric(conn, f1=0.8, run_number=i + 1) + + payload = survivor_bias(load_dataframe(conn)) + assert payload["status"] == "ok" + row = payload["by_strategy"][0] + assert row["survivor_bias"] == pytest.approx(0.0) + assert row["n_cells_dropped"] == 0 + + +def test_survivor_bias_positive_when_failures_dropped() -> None: + """ + Dropping a failed run raises the surviving mean above the all-runs mean: + that gap is precisely how much valid_only flatters the strategy. + """ + pytest.importorskip("statsmodels") + conn = _conn() + _insert_run_with_metric(conn, f1=0.9, run_number=1) + # A failed run: no metric row, so intent_to_treat scores it 0.0 and + # valid_only drops it entirely. + _insert_run( + conn, + strategy=Strategy.SOP_BASED, + error="APIError: 500", + diagram=None, + run_number=2, + ) + + payload = survivor_bias(load_dataframe(conn)) + row = payload["by_strategy"][0] + assert row["mean_all_runs"] == pytest.approx(0.45) # (0.9 + 0.0) / 2 + assert row["mean_survivors"] == pytest.approx(0.9) + assert row["survivor_bias"] == pytest.approx(0.45) + + +def _insert_run_with_metric( + conn: sqlite3.Connection, + *, + f1: float, + run_number: int, + strategy: Strategy = Strategy.SOP_BASED, +) -> None: + """Insert a successful run carrying a metric row with the given F1.""" + from maestro.db.queries import insert_metric_result + from maestro.schemas import MetricResult + + run_id = _insert_run(conn, strategy=strategy, error=None, run_number=run_number) + insert_metric_result( + conn, + MetricResult( + run_id=run_id, + parses_valid=True, + entity_id_precision=f1, + entity_id_recall=f1, + entity_id_f1=f1, + entity_name_precision=0.0, + entity_name_recall=0.0, + entity_name_f1=0.0, + entity_lemma_precision=0.0, + entity_lemma_recall=0.0, + entity_lemma_f1=0.0, + relationship_relaxed_precision=0.0, + relationship_relaxed_recall=0.0, + relationship_relaxed_f1=0.0, + relationship_strict_precision=0.0, + relationship_strict_recall=0.0, + relationship_strict_f1=0.0, + entities_in_output=0, + entities_in_truth=0, + relationships_in_output=0, + relationships_in_truth=0, + missing_entities=0, + extra_entities=0, + false_entities=0, + duplicate_entities=0, + missing_relationships=0, + extra_relationships=0, + false_relationships=0, + duplicate_relationships=0, + ), + )