Skip to content

Fix CI Performance Tests - #42

Open
drasmart wants to merge 10 commits into
mainfrom
feature/ci-perf-tests-fix
Open

Fix CI Performance Tests#42
drasmart wants to merge 10 commits into
mainfrom
feature/ci-perf-tests-fix

Conversation

@drasmart

@drasmart drasmart commented Sep 10, 2026

Copy link
Copy Markdown

Changes

  • Move run-performance-tests.ps1 (already coupled) from common-ci/rust to here.
  • Download Asn file from Azure.
  • Add dryrun and common-ci-ref to nightly-performance.yml.
  • Stop hiding failures of compare-performance.ps1.

@drasmart

Copy link
Copy Markdown
Author

Nightly Performance job:

@drasmart
drasmart marked this pull request as ready for review September 10, 2026 10:41
@oleksandrlazarenko-pi

Copy link
Copy Markdown
Contributor

Review notes

Reviewed against main, cross-checked with common-ci/steps/compare-performance.ps1, the ip-intelligence-data submodule pins, and this workflow's Actions run history.

Blocking

1. Dropping continue-on-error: true silently disables the history upload.github/workflows/nightly-performance.yml:162

When the compare step fails, Escape Branch Name and Upload Graphed Performance Results are skipped, because neither has if: always(). That artifact is the only source of future baselines, so the run's own figure never enters history.

Confirmed empirically on run 34464099806: steps 14 and 15 report skipped.

The consequence only bites once there are ≥10 data points: a genuine regression exits 1, the regressed figure never enters history, and every subsequent nightly then compares against the same stale baseline and fails forever. Adding if: always() to both steps keeps the loud failure without wedging the trend.

2. The ASN download resolves against the caller's CWD, not $RepoNameci/run-performance-tests.ps1:13

The ip-intelligence-cxx/ip-intelligence-data path is relative to wherever the script happens to be invoked from rather than to $RepoName. The script's own documented local invocation (-RepoName "." from the repo root) now throws at Push-Location before any benchmark runs.

Non-blocking

  • nightly-performance.yml:169dryrun only suppresses git push; the workflow still uploads publish_performance_results@<branch>, so a dry run on main injects its figure into the real performance history. That contradicts the new comment at lines 149-152.
  • ci/run-performance-tests.ps1:24 — deleting the ASN file and calling get-lite-file-from-azure.ps1 only restores it when the ip-intelligence-data pin is ≥ 60ec85a. Older pins fetch only 51Degrees-LiteV41.ipi, leaving 51DEGREES_IPI_PATH dangling — and this PR adds the ipi-cxx-ref input that makes older pins reachable.
  • ci/run-performance-tests.ps1:19 — the pre-delete cannot force a refresh: the Azure script gates its download on the .gz archive, not the .ipi, and is called without -Force. On a persisted workspace a stale archive is silently re-extracted; on a failed download the tracked LFS file is already gone.
  • nightly-performance.yml:159 — the comment justifying continue-on-error: false cites an exit code 2 for infrastructure failure, but compare-performance.ps1 only ever exits 0 or 1, so infrastructure failures are indistinguishable from regressions.
  • ci/missing-ci-scripts.md:67 — "behaviour of the nightly run is unchanged" and "the two copies are identical" are both falsified by this PR itself (ASN block, dryrun, refs, continue-on-error).
  • nightly-performance.yml:197-ErrorAction Stop is a no-op, since the callee sets $ErrorActionPreference = "Stop" itself. Looks like a leftover from the reverted $PSNativeCommandUseErrorActionPreference attempt.
  • nightly-performance.yml:98 — the benchmark corpus is no longer version-pinned (live Azure blob per run). A published data-file update can shift throughput past the 10%/2σ band and, with continue-on-error false, fail the nightly with no way to re-baseline.

Checked and sound

The git -C rust checkout -- examples/Cargo.lock fix (both lockfiles are tracked, /test-results is gitignored, the sys build scripts only read from the manifest dir); the throughput regexes against the examples' println! formats, and the "last match wins" comment matching the IPI example's single-then-multi report order; -DryRun $DryRun binding (-DryRun exists on common-ci main with a [bool] type); inputs.dryrun || 'false' for schedule, dispatch-true and dispatch-false; the empty ref: on actions/checkout falling back to the default branch; and the omitted description on the dryrun input.

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.

2 participants