Skip to content

fix(benchmark): restore the daily flywheel report with precompiled category patterns and failure alerts - #465

Open
bennyjo wants to merge 1 commit into
mainfrom
gnhf/restore-the-daily-be-88379b
Open

fix(benchmark): restore the daily flywheel report with precompiled category patterns and failure alerts#465
bennyjo wants to merge 1 commit into
mainfrom
gnhf/restore-the-daily-be-88379b

Conversation

@bennyjo

@bennyjo bennyjo commented Sep 5, 2026

Copy link
Copy Markdown

Summary

The daily benchmark flywheel has been cancelled at its 30-minute job timeout every night since 2026-09-01, so no report, Slack post, or tool-improvement triage has gone out since 2026-08-31. This PR fixes the cause, bounds the step so a slow scorer can never take the report down again, and adds a failure-path Slack alert so a dead run is visible the same morning.

Root cause

PR #457 routed the "Score trailing 90d window" step through mech-analytics. That step now scores about 233k rows, and classify_category in benchmark/datasets/fetch_production.py built a fresh regex per keyword per row. The keyword table holds exactly 512 keywords, which is also re._MAXCACHE, so every question that matched nothing evicted the whole regex cache and the next row recompiled all 512 patterns.

Measurement, flag on, 90-day window main this PR
Rows fetched 233k, 47 pages, ~50 s same
Scorer wall time (M-series laptop) 776 s 68 s
CPU time 733 s 41 s
Overall Brier 0.2244 0.2244

Runs 33463476689, 33585023041, 33712537373, 33830324961 all died this way. The last success was 33351447211 on 2026-08-31, before #457 merged.

What changed

  • Precompiled category patterns. CATEGORY_PATTERNS builds one compiled alternation per category at import time, each keyword still wrapped in its own \b...\b, category order preserved. classify_category searches those. First-category-wins, the platform filter, and the "other" fallback are unchanged. Empty keyword lists are skipped so they cannot compile to a match-everything pattern.
  • Tests. TestClassifyCategoryPrecompiled pins the module-level compiled tuple, counts zero per-call re.search invocations, checks all 512 keywords behaviourally (each matches on its own into its category or an earlier one, and never inside a longer word), and adds a hand-picked equivalence fixture. No wall-clock assertions.
  • Step timeout. "Score trailing 90d window" gets id: trailing_90d and timeout-minutes: 10. A slow scorer now fails the step, which the existing continue-on-error absorbs, so Analyze, the Slack posts and triage still run. 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.
  • Failure alert. A new notify-failure job runs if: always(), reads every upstream job's result, and posts one plain-text Slack message naming failed or cancelled jobs with the run URL to the existing BENCHMARK_SLACK_WEBHOOK_URL. It honours the same notify_slack / ENABLE_SLACK_NOTIFY gates as the success posts and uses plain curl, so it needs no checkout or Python setup. A cancelled job runs none of its own later steps, which is why this is a separate job. A manual cancel from the UI also posts; that is accepted, since a job timeout reports as cancelled without cancelling the run.
  • Docs. DAILY_REPORT_OPERATOR_GUIDE.md describes the new failure message.

Equivalence evidence

  • 1,340 distinct real titles from six 5,000-row pages spread over the trailing 90 days, classified old vs new for omen, polymarket and None: 4,020 comparisons, 0 diffs.
  • 4,466 synthetic titles covering every keyword in 8 lexical framings plus 364 cross-category ordering pairs and degenerate inputs, times four platform values: 17,864 comparisons, 0 diffs.
  • A second independent sweep (no-mistakes pipeline): 37,560 comparisons on live Omen and Polymarket titles plus probes, 0 mismatches.
  • End to end: 232k rows captured once and replayed through the scorer with the old and the new classifier. All three trailing_scores*.json files identical apart from generated_at.

Checks

  • pytest benchmark/tests: 1825 passed; the only failures are the 10 known ModuleNotFoundError: pypdf cases in test_prompt_replay.py that need the tool extras.
  • black, isort, flake8, mypy (--disallow-untyped-defs), pylint 10.00/10 and darglint clean on both touched Python files, with tomte v0.7.0's pinned configs.
  • Workflow parses; actionlint reports only the pre-existing empty choice option on line 54. The notify-failure run script was executed against a local fake webhook in five scenarios (all green, benchmark cancelled, tournament-run failed, secret unset, Slack HTTP 500).

After merge

Trigger the flywheel by workflow_dispatch rather than waiting for the 02:30 UTC cron, to get the day's report out and confirm the fix on the real runner. The ROI companion picks up fresh data on its own once one flywheel run succeeds.

Pipeline

⏭️ **intent** - skipped

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

✅ **Review** - 3 issues (1 warning, 2 infos) - 2 fixed in 7157258, 1 accepted as designed
  • Fixed in 7157258. test_every_keyword_is_word_boundary_wrapped is gone. Its replacement, test_every_keyword_matches_only_as_a_whole_word, is behavioural: for all 512 keywords it checks that the keyword on its own classifies into its category or an earlier one, and that the keyword glued inside a longer word does not (keywords that are substrings of another keyword are skipped for the glued check). Any equivalent pattern shape now passes. Original finding:
    • ⚠️ benchmark/tests/test_fetch_production.py:737 - test_every_keyword_is_word_boundary_wrapped asserts the exact source string of each compiled pattern (pattern.pattern == '|'.join(r'\b'+escape(kw)+r'\b')). This pins implementation shape rather than behaviour, and its stated rationale is wrong: \b is a zero-width assertion, so wrapping the whole alternation as \b(?:a|b)\b matches exactly the same inputs as \ba\b|\bb\b. Any equivalent refactor (grouping, sorting keywords longest-first, a trie-style pattern) would fail this test while preserving scores. The semantics it claims to protect are already covered by test_equivalence_fixture. Remove it, or replace it with a behavioural check such as: for every category and keyword, classify_category(f'will {kw} happen?') returns that category or an earlier one in table order, and classify_category(f'will x{kw}y happen?') does not return that category for a keyword that is not a substring of another keyword. The neighbouring test_patterns_are_precompiled_module_level_objects (isinstance tuple / re.Pattern) has the same structural flavour but is harmless; the re.search call-count test is an acceptable regression guard.
  • Fixed in 7157258. The step now has id: trailing_90d, and a following step gated on steps.trailing_90d.outcome == 'failure' emits a ::warning:: annotation, so a report shipped without its trailing-90d section is visible on the run summary. The Slack-summary variant was not taken: that goes through the LLM summariser prompt and is out of scope here. Original finding:
    • ℹ️ .github/workflows/benchmark_flywheel.yaml:460 - With the new step timeout, a scorer that exceeds 10 minutes is absorbed by continue-on-error, the job succeeds, and notify-failure stays quiet. The report then ships without its trailing-90d column set with no alert anywhere, so a repeat of the slow-fetch scenario the comment describes would be visible only to someone reading the digest. The step has no id, so nothing downstream can gate on steps.<id>.outcome. If that degradation should be surfaced, give the step an id and either emit a ::warning:: annotation or include a one-line note in the Slack summary when outcome == 'failure'. This adds behaviour beyond the stated intent, so it needs the author's call rather than an automatic fix.
  • Accepted, no change. The reviewer's own reasoning holds: a job timeout reports as cancelled without cancelling the run, so !cancelled() cannot be used. Manual-cancel posts are a known side effect and are documented in the job comment. Original finding:
    • ℹ️ .github/workflows/benchmark_flywheel.yaml:845 - notify-failure uses always(), which also runs when a human cancels the workflow run from the UI. A deliberate cancel will post 'benchmark-flywheel did not complete' with every upstream job listed as cancelled. This is the correct choice because a job-level timeout reports as 'cancelled' without cancelling the run, so !cancelled() cannot be used; the manual-cancel noise is an accepted side effect worth knowing about.
✅ **Test** - passed

✅ No issues found.

  • uv venv --python 3.10 .venv &amp;&amp; uv pip install pytest requests pyyaml pytest-asyncio (minimal env; removed afterwards)
  • .venv/bin/python -m pytest benchmark/tests/test_fetch_production.py -p no:cacheprovider -q -> 169 passed
  • classify_equivalence.py: loaded base-commit fetch_production.py via git show 83472062:benchmark/datasets/fetch_production.py and compared classify_category old vs new on 1,000 live Omen titles, 400 live Polymarket titles, 5,120 keyword-boundary/escaping probes, 3,000 cross-category pairs and 3,000 no-match noise rows, for platform None/omen/polymarket -> 37,560 comparisons, 0 mismatches; timed both on 7,400 rows -> 11.7x speedup
  • prefix_regression_repro.txt: counted re.search calls per classify_category call on base vs target module -> base issues 512 distinct patterns (== re._MAXCACHE) on an unmatched row, target issues 0
  • workflow_notify_failure_exercise.py: parsed benchmark_flywheel.yaml with PyYAML, asserted the 'Score trailing 90d window' step has timeout-minutes=10 with continue-on-error inside the 30-minute job budget, asserted notify-failure needs every other job and uses always(); extracted the step's run script from the parsed model and executed it under bash with GitHub-shaped NEEDS_JSON/RUN_URL/SLACK_WEBHOOK_URL against a local HTTP capture server in 5 scenarios (all success, benchmark cancelled, tournament-run failure with check-secrets skipped, webhook unset, Slack HTTP 500) -> all behavioural assertions passed
  • curl --version check that --fail-with-body is supported (local 8.7.1; ubuntu-22.04 runner ships 7.81, feature added in 7.76)
✅ **Document** - passed

✅ No issues found.

⚠️ **Lint** - 1 info
  • ℹ️ benchmark/runner.py:39 - Repo-wide mypy and pylint (via tomte tox -e mypy,pylint) fail with 7 import errors on packages.valory.skills.task_execution.utils.* in benchmark/runner.py, ipfs_loader.py, tools.py, tournament.py, tests/test_tournament.py, scripts/test_tool.py and scripts/test_tools.py. None of these files are touched by this change; the cause is that the task_execution skill is not checked in and autonomy packages sync has not been run in this worktree. Both changed Python files pass mypy (--disallow-untyped-defs) and pylint (10.00/10) when checked directly. Black, isort, flake8 and darglint pass repo-wide. The workflow YAML was only parse-checked; actionlint is not installed.
✅ **Push** - passed

✅ No issues found.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DDvwBtWkkLin9NVhHeTeGf

@bennyjo
bennyjo force-pushed the gnhf/restore-the-daily-be-88379b branch from b23a0bc to 136c375 Compare September 5, 2026 22:52
bennyjo added a commit that referenced this pull request Sep 5, 2026
- Replace the test that pinned each compiled pattern's source string with
  a behavioural check: every keyword classifies into its own category or
  an earlier one on its own, and never when glued inside a longer word.
  Any equivalent pattern shape now passes.
- Give the trailing-90d step an id and emit a ::warning:: annotation when
  it fails or hits its 10-minute budget, so a report shipped without its
  trailing-90d section is visible on the run summary instead of only in
  the digest.
…tegory 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.
@bennyjo
bennyjo force-pushed the gnhf/restore-the-daily-be-88379b branch from a762e56 to 7157258 Compare September 5, 2026 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant