From 7157258f87d3b59fd345bbd7a6dee64deb11b8f7 Mon Sep 17 00:00:00 2001 From: Benny Johansson Date: Sat, 5 Sep 2026 23:58:20 +0100 Subject: [PATCH] fix(benchmark): restore the daily flywheel report with precompiled category patterns and failure alerts The benchmark job has been cancelled at its 30-minute timeout every night since 2026-09-01, inside "Score trailing 90d window". PR #457 routed that step through mech-analytics, so it now scores about 233k rows, and classify_category built a fresh regex per keyword per row. The keyword table holds exactly 512 entries, which is re._MAXCACHE, so every unmatched question evicted the whole regex cache and the next row recompiled all 512 patterns. Locally that was 776 s wall for the step; the CI runner did not finish in 30 minutes. Changes: - Compile one alternation per category at import time, each keyword still wrapped in its own word boundaries and category order preserved. The step now runs in about 68 s, dominated by the fetch, and produces identical scores: 0 diffs over 4020 comparisons on 1340 real titles, 0 diffs over 17864 comparisons on synthetic titles covering all 512 keywords, and identical trailing_scores.json on a shared row capture. - Add tests that pin the precompiled structure, count zero per-call re.search invocations, and check behaviour for all 512 keywords: each matches on its own into its category or an earlier one, and never inside a longer word. No wall-clock assertions. - Give the trailing-90d step an id and a 10-minute timeout so a slow scorer fails the step, which continue-on-error absorbs, instead of cancelling the job and skipping the report and Slack posts. A follow-up step emits a ::warning:: annotation when it fails, so a report shipped without its trailing-90d section is visible on the run summary. - Add a notify-failure job that runs on always(), reads every upstream job result, and posts one plain-text Slack message naming failed or cancelled jobs with the run URL, under the same dry-run gates as the success posts. Cancelled jobs skip their own later steps, so this has to be a separate job. - Document the failure post in the operator guide. --- .github/workflows/benchmark_flywheel.yaml | 81 +++++++++++++++ benchmark/DAILY_REPORT_OPERATOR_GUIDE.md | 2 + benchmark/datasets/fetch_production.py | 36 +++++-- benchmark/tests/test_fetch_production.py | 120 ++++++++++++++++++++++ 4 files changed, 233 insertions(+), 6 deletions(-) diff --git a/.github/workflows/benchmark_flywheel.yaml b/.github/workflows/benchmark_flywheel.yaml index 94f54d7eb..31625599a 100644 --- a/.github/workflows/benchmark_flywheel.yaml +++ b/.github/workflows/benchmark_flywheel.yaml @@ -447,7 +447,18 @@ jobs: # 7d files. So this step runs under both modes: legacy scans # local logs; mech-analytics fetches [now-90d, now] via # score_period_split_by_platform_from_mech_analytics. + # timeout-minutes here, not just continue-on-error: a job timeout + # cancels the job, so continue-on-error never gets to absorb it and + # every later step (Analyze, the Slack posts, triage) is skipped. + # That is how the flywheel went dark for four nights from + # 2026-09-01. A step budget converts a slow scorer into a failed + # step, which continue-on-error does absorb, so the report still + # ships minus its trailing-90d section. 10 minutes is generous: the + # mech-analytics path runs in about 70 seconds, dominated by the + # fetch. Raise it only if the fetch itself grows. - name: Score trailing 90d window + id: trailing_90d + timeout-minutes: 10 run: | if [ "$USE_MECH_ANALYTICS_ROWS" = "true" ]; then python -m benchmark.scorer \ @@ -468,6 +479,15 @@ jobs: fi continue-on-error: true + # continue-on-error keeps the job green when the step above fails + # or trips its 10-minute budget, so the report would ship without + # its trailing-90d section and nothing would say so. Surface that + # as a run annotation so it is visible on the run summary. + - name: Flag a missing trailing-90d section + if: steps.trailing_90d.outcome == 'failure' + run: | + echo "::warning title=Trailing 90d scoring failed::'Score trailing 90d window' failed or hit its 10-minute budget; today's report has no trailing-90d section. See that step's log." + - name: Analyze (Omenstrat) run: python -m benchmark.analyze --platform omen --include-tournament @@ -813,3 +833,64 @@ jobs: benchmark/results/tournament_predictions.jsonl benchmark/results/tournament_scored.jsonl retention-days: 90 + + # ----------------------------------------------------------------------- + # Job 4: Failure notification + # The success posts live inside the benchmark job and only fire when a + # report file exists, so a job that dies -- timeout, runner loss, an + # unguarded step error -- posts nothing at all and reads exactly like a + # quiet day. A cancelled job runs none of its own later steps, which is + # why this has to be a separate job rather than a final step. + # + # Plain curl rather than benchmark.notify_slack: importing that module + # pulls in the whole analyze/report stack, so it would need a checkout, + # a Python setup and a dependency install just to send one line. This + # job has to be the most reliable thing in the workflow. + # ----------------------------------------------------------------------- + notify-failure: + needs: [benchmark, tournament-fetch, check-secrets, tournament-run] + # Same dry-run switches as the success posts, so flipping + # notify_slack=false or ENABLE_SLACK_NOTIFY=false still silences + # everything. + if: always() && (github.event.inputs.notify_slack != 'false') && (vars.ENABLE_SLACK_NOTIFY != 'false') + runs-on: ubuntu-22.04 + timeout-minutes: 5 + + steps: + - name: Post failed or cancelled jobs to Slack + env: + SLACK_WEBHOOK_URL: ${{ secrets.BENCHMARK_SLACK_WEBHOOK_URL }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + # toJSON(needs) carries {"": {"result": "...", ...}} for + # every job in `needs` above, so adding a job to that list is + # the only edit a new job needs. + NEEDS_JSON: ${{ toJSON(needs) }} + run: | + set -euo pipefail + + broken=$(printf '%s' "$NEEDS_JSON" | jq -r ' + to_entries + | map(select(.value.result == "failure" or .value.result == "cancelled")) + | map("- \(.key): \(.value.result)") + | join("\n")') + + if [ -z "$broken" ]; then + echo "All upstream jobs finished clean; nothing to report." + exit 0 + fi + + echo "Failed or cancelled jobs:" + echo "$broken" + + if [ -z "${SLACK_WEBHOOK_URL:-}" ]; then + echo "::warning::BENCHMARK_SLACK_WEBHOOK_URL is unset; skipping the Slack post." + exit 0 + fi + + text=$(printf 'benchmark-flywheel did not complete.\n%s\n%s' "$broken" "$RUN_URL") + jq -n --arg text "$text" '{text: $text}' > payload.json + curl -sS --fail-with-body \ + -X POST \ + -H 'Content-Type: application/json' \ + --data @payload.json \ + "$SLACK_WEBHOOK_URL" diff --git a/benchmark/DAILY_REPORT_OPERATOR_GUIDE.md b/benchmark/DAILY_REPORT_OPERATOR_GUIDE.md index a0d11ca21..b891bc2dc 100644 --- a/benchmark/DAILY_REPORT_OPERATOR_GUIDE.md +++ b/benchmark/DAILY_REPORT_OPERATOR_GUIDE.md @@ -29,6 +29,8 @@ flowchart LR 🔴 `NO ACTION` = every deployed tool fails but demoting all would empty the platform → escalate, never act tool-by-tool. +A failed or cancelled job posts a separate message, `benchmark-flywheel did not complete`, naming the broken jobs with a link to the run. Treat anything else posted that morning as partial - open the run before acting. + ## 3. The two gates > **PROMOTE** a tournament tool: **n ≥ 30** and **`floor` > +0.04** and **`condAcc` ≥ 50%**. diff --git a/benchmark/datasets/fetch_production.py b/benchmark/datasets/fetch_production.py index 0c1dbd81e..9853c9cfc 100644 --- a/benchmark/datasets/fetch_production.py +++ b/benchmark/datasets/fetch_production.py @@ -1960,6 +1960,33 @@ def parse_tool_response(tool_response: Optional[str]) -> dict[str, Any]: # Category classification # --------------------------------------------------------------------------- +# One compiled alternation per category, in CATEGORY_KEYWORDS order. +# +# The classifier used to build a fresh r"\b\b" pattern string per +# keyword and hand it to re.search, relying on the interpreter's regex +# cache to amortise compilation. CATEGORY_KEYWORDS holds exactly 512 +# keywords, which is also re._MAXCACHE, so any question that matched +# nothing walked all 512 patterns and evicted the cache, forcing a full +# recompile on the next row. Over a 90-day scoring window (233k rows) +# that cost about 12 minutes of pure compile time and pushed the daily +# flywheel past its 30-minute job timeout. +# +# Each keyword keeps its own \b...\b wrapper, so per-keyword boundary +# semantics are unchanged; alternation matches exactly when at least one +# keyword matches, which is the only thing the classifier asks. Category +# order is preserved, so first-category-wins still holds. +CATEGORY_PATTERNS: tuple[tuple[str, re.Pattern[str]], ...] = tuple( + ( + category, + re.compile("|".join(r"\b" + re.escape(kw) + r"\b" for kw in keywords)), + ) + for category, keywords in CATEGORY_KEYWORDS.items() + # An empty keyword list would compile to the empty pattern, which + # matches everything. No category is empty today; skip rather than + # let a future edit silently swallow every question. + if keywords +) + def classify_category(question_text: str, platform: Optional[str] = None) -> str: """Classify a question into a category using word-boundary keyword matching. @@ -1980,12 +2007,9 @@ def classify_category(question_text: str, platform: Optional[str] = None) -> str """ text_lower = question_text.lower() matched = "other" - for category, keywords in CATEGORY_KEYWORDS.items(): - for kw in keywords: - if re.search(r"\b" + re.escape(kw) + r"\b", text_lower): - matched = category - break - if matched != "other": + for category, pattern in CATEGORY_PATTERNS: + if pattern.search(text_lower): + matched = category break if platform is None: diff --git a/benchmark/tests/test_fetch_production.py b/benchmark/tests/test_fetch_production.py index a789c2083..18f55ffa2 100644 --- a/benchmark/tests/test_fetch_production.py +++ b/benchmark/tests/test_fetch_production.py @@ -29,6 +29,8 @@ import pytest import requests from benchmark.datasets.fetch_production import ( + CATEGORY_KEYWORDS, + CATEGORY_PATTERNS, DEDUP_LOOKBACK_DAYS, DELIVERS_SCHEMA_LEGACY, DELIVERS_SCHEMA_PARSED, @@ -710,6 +712,124 @@ def test_no_keyword_match_returns_other_regardless_of_platform(self) -> None: assert classify_category("Will quux?", "polymarket") == "other" +class TestClassifyCategoryPrecompiled: + """The classifier must not recompile keyword patterns per call. + + CATEGORY_KEYWORDS holds 512 keywords, which is exactly + ``re._MAXCACHE``. Building a pattern string per keyword and leaning + on the interpreter's regex cache therefore thrashed that cache on + every unmatched question, and a 90-day scoring window spent about + 12 minutes inside ``re``. These tests pin the structural fix + (module-level compiled alternations) and the behaviour it must + leave untouched. Deliberately no wall-clock assertion: timing tests + flake in CI. + """ + + def test_patterns_are_precompiled_module_level_objects(self) -> None: + """Every category maps to a compiled pattern, in table order.""" + assert isinstance(CATEGORY_PATTERNS, tuple) + assert [category for category, _ in CATEGORY_PATTERNS] == [ + category for category, keywords in CATEGORY_KEYWORDS.items() if keywords + ] + for _, pattern in CATEGORY_PATTERNS: + assert isinstance(pattern, re.Pattern) + + def test_every_keyword_matches_only_as_a_whole_word(self) -> None: + """Each keyword still matches on its own and never inside a longer word. + + Behavioural rather than structural: any pattern shape is fine as + long as a keyword on its own classifies into its category (or an + earlier one, since the first category wins) and the same keyword + glued to letters on both sides does not. Keywords that are a + substring of another keyword are skipped for the glued check, + because the longer keyword may legitimately match there. + """ + order = list(CATEGORY_KEYWORDS) + all_keywords = [kw for kws in CATEGORY_KEYWORDS.values() for kw in kws] + substrings = { + kw + for kw in all_keywords + if any(kw != other and kw in other for other in all_keywords) + } + for category, keywords in CATEGORY_KEYWORDS.items(): + for kw in keywords: + bare = classify_category(f"will {kw} happen?") + assert bare != "other", f"{kw!r} no longer matches on its own" + assert order.index(bare) <= order.index(category), ( + f"{kw!r} classified as {bare!r}, after its own " + f"category {category!r}" + ) + if kw in substrings: + continue + glued = classify_category(f"will x{kw}y happen?") + assert glued != category, f"{kw!r} matched inside a longer word" + + def test_classify_does_not_walk_the_keyword_list_with_re_search( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """``classify_category`` issues no module-level ``re.search`` calls. + + A regression to the per-keyword loop would fire hundreds of + ``re.search`` calls here, each with a freshly built pattern + string. + + :param monkeypatch: pytest fixture, used to count ``re.search``. + """ + calls: list[Any] = [] + original_search = re.search + + def counting_search(*args: Any, **kwargs: Any) -> Any: + """Record the call, then delegate to the real ``re.search``. + + :param args: positional arguments forwarded verbatim. + :param kwargs: keyword arguments forwarded verbatim. + :return: whatever the real ``re.search`` returns. + """ + calls.append(args) + return original_search(*args, **kwargs) + + monkeypatch.setattr(re, "search", counting_search) + for question in ("Will Bitcoin hit $100k?", "Will quux blorp?"): + classify_category(question) + assert not calls, ( + f"classify_category made {len(calls)} re.search calls; " + "the per-keyword loop is back" + ) + + @pytest.mark.parametrize( + "question,platform,expected", + [ + # First category wins: "business" precedes "finance" in + # CATEGORY_KEYWORDS, so a question carrying keywords from + # both buckets classifies as business. + ("Will the CEO comment on the bitcoin price?", None, "business"), + # Word boundaries: "eth" must not match inside "something", + # "whether" or "ethics". + ("Will something whether ethics matter?", None, "other"), + # Punctuation inside a keyword survives re.escape: the + # hyphen in "peer-reviewed" is required, not a wildcard. + ("Will the study be peer-reviewed?", None, "science"), + ("Will peer reviewed studies rise?", None, "other"), + # Platform filter drops an off-taxonomy match to other. + ("Will the airline launch a new flight route?", "omen", "other"), + ("Will the airline launch a new flight route?", None, "travel"), + # Nothing matches at all. + ("Will quux blorp frobnicate?", None, "other"), + ("Will quux blorp frobnicate?", "polymarket", "other"), + ], + ) + def test_equivalence_fixture( + self, question: str, platform: Optional[str], expected: str + ) -> None: + """Hand-picked titles pinning the semantics precompilation preserves. + + :param question: market question to classify. + :param platform: scorer platform key, or ``None``. + :param expected: category the classifier must return. + """ + assert classify_category(question, platform) == expected + + # --------------------------------------------------------------------------- # _parse_request_context # ---------------------------------------------------------------------------