add weekly test timings refresh workflow#1587
Conversation
There was a problem hiding this comment.
Have added some comments. Additionally please look to run the workflow locally to confirm it is working as expected. One issue I feel in the overall design is that the 10% drift check should probably be at a test file level and not a profile level. Ideally I feel 1.) for new tests files add the new value 2.) for existing test files only update if delta > 10%
Agree on the fact that if delta is ever > 60% we should raise for manual review
| def total_wall_by_profile(doc: dict[str, dict[str, float]]) -> dict[str, float]: | ||
| """Total per-profile wall seconds (sum of every file's time). | ||
|
|
||
| This is the drift signal the weekly refresh workflow gates on. Crucially it |
There was a problem hiding this comment.
nit: make the comment terse
There was a problem hiding this comment.
We have repeated the same info multiple times on the timings being independent of shard count. We should follow DRY and keep this in a single place
| # 1. Runs scripts/regenerate_timings.py to rebuild timings from the last few | ||
| # distinct-SHA green integration.yml runs (median per-file wall time), and | ||
| # in the same tested script computes per-profile total-wall drift vs the | ||
| # current file. Total wall time is independent of shard count, so the drift |
There was a problem hiding this comment.
nit:
# decision never duplicates integration.yml's (dynamically computed) shard
# counts — a manual regen gets the identical numbers.
No need add detailed info here in the comments
| ) -> dict[str, float]: | ||
| """Per-profile percent change in total wall time from old to new timings. | ||
|
|
||
| Measured as an absolute percentage (direction-agnostic — a human reviews |
There was a problem hiding this comment.
nit: no need for the added explanation here. Also adding a new profile is a very very rare case
| required: false | ||
| default: "3" | ||
| type: string | ||
| force: |
There was a problem hiding this comment.
I think we can remove this flag, if we want to force a PR we can do this manually
| # instead of opening a PR with potentially bad data. | ||
| # 3. PRs are never auto-merged — bad timings degrade balancing silently, so a | ||
| # human always reviews the before/after numbers in the PR body. | ||
| name: Refresh Test Timings |
There was a problem hiding this comment.
We need to ensure that this can only run on main. We should not open PRs/refresh timings on branch code
| This is the automatable, all-profiles-at-once front end to | ||
| `refresh_timings.py` (which aggregates one profile from one run's junit XMLs). | ||
| It discovers recent green `integration.yml` runs, downloads every profile's | ||
| per-shard junit artifacts, and writes the **median** per-file wall time across |
Coverage report
This PR does not seem to contain any modification to coverable code. |
2 participants
Resolves #
Description
Automates the weekly refresh of .github/test_timings.json so integration shard balancing doesn't drift stale. Adds a scheduled Refresh Test Timings workflow that regenerates timings from the last few distinct-SHA green integration runs and opens a human-reviewed PR (never auto-merged) when timings change.
The merge decision lives in the tested regenerate_timings.py, not in workflow YAML, and is per test file: new files are added, existing files are rewritten only when they move >10% (keeps run-to-run noise out), and a >60% move on a non-trivial file is flagged for manual review instead of auto-opening a PR. Per-file means the logic never needs to know shard counts.
Checklist
Ran it locally end-to-end against the live repo:
python scripts/regenerate_timings.py --num-runs 3
--output /tmp/new_timings.json --old .github/test_timings.json --drift-out /tmp/drift.json
Caught and fixed 3 issues:
Note: only 1 run is currently within the 7-day retention window, so it aggregated a median-of-1 and drift looked large; with ≥3 runs weekly this stabilizes.
CHANGELOG.mdand added information about my change to the "dbt-databricks next" section./dbt-databricks-pr-ready(AI agent skill in.claude/skills/) and addressed its merge-readiness feedback