lasp / lasp/space_packet_parser
Enforce the CTIM XTCE definition load time budget in CI
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 39
- Forks
- 15
- Avg merge
- 2d 16h
- Merged PRs (30d)
- 12
Description
Context
#147 asked for the XTCE parsing benchmarking to be documented, and #286 does that: docs/source/benchmarking.md now explains that test_benchmark_ctim_xtce_parsing exists as a regression guard, records the history (loading ctim_xtce_v1.xml once took on the order of twelve seconds because each of its 38 concrete containers re-parsed their shared abstract base from scratch), and states the budget — loading that definition should take tens of milliseconds, and approaching 100 ms means something has regressed. It currently measures around 34 ms in the devcontainer.
What #286 deliberately did not do is make any of that true automatically.
CI runs pytest --color=yes --cov --cov-report=xml, which collects and executes tests/benchmark/ on every matrix combination — and then throws the timings away. Nothing is saved, nothing is compared, nothing is gated. .benchmarks/ exists but is empty and gitignored. A performance regression of any size, including another two-orders-of-magnitude one, will produce a fully green build.
This is a genuine trap rather than a cosmetic gap: a reader who sees a benchmark suite running in CI will reasonably assume it is a gate. #286 documents the gap honestly in a {note} and tells contributors touching space_packet_parser/xtce/ to run --benchmark-autosave / --benchmark-compare manually, but a manual step that depends on remembering to take it is not a guard. This issue is for designing a real one.
Driving Requirements
- A regression in XTCE definition load time on the scale of the one #147 refers to should fail a build rather than merge silently.
- Enforcement must not produce intermittent failures on unchanged code. A docs/CI change that makes the build flaky is worse than the status quo, because it trains people to re-run red builds.
- Whatever threshold is chosen should be maintainable — it should not need hand-editing every time a runner image or Python version changes.
Implementation Requirements
Two candidate approaches, both with real problems that need resolving before anything is implemented.
Option A — a hard assert in the benchmark test.
Add something like assert benchmark.stats.stats.mean < 0.1 to test_benchmark_ctim_xtce_parsing.
- Cheap, obvious, no new infrastructure, and the threshold lives next to the thing it guards.
- But it is evaluated on every cell of the CI matrix: Windows, macOS, and five Python versions, on shared GitHub-hosted runners with noisy neighbours and no performance SLA. The 100 ms line has roughly 3x headroom over the devcontainer measurement, which sounds generous until a cold, contended Windows runner is involved. Needs actual data — collect observed means per matrix cell across a number of runs before picking any number, rather than assuming 100 ms transfers.
- A single slow round can drag the mean; consider asserting on the median or minimum instead, since the minimum is the most noise-resistant statistic pytest-benchmark reports.
- May need to be restricted to one matrix cell (a single Linux + pinned Python job) rather than all of them.
Option B — --benchmark-compare-fail against a stored baseline.
Run pytest tests/benchmark/ --benchmark-compare --benchmark-compare-fail=mean:25% in CI.
- Catches relative regressions, which is the thing actually worth catching, and adapts to hardware instead of hardcoding an absolute number.
- But it needs a baseline to compare against, and that is the unsolved part. Baselines are only meaningful when compared against runs from the same machine — ephemeral CI runners are, by design, not the same machine. Options worth evaluating: committing baselines to the repo (churn, merge conflicts, and they still encode one machine's characteristics), caching them via
actions/cache(evictable, and the first run after an eviction has nothing to compare against), or generating the baseline in the same workflow run by also benchmarking the merge base (doubles benchmark runtime but makes the comparison genuinely same-machine — probably the most promising variant). - The percentage threshold needs the same empirical treatment as Option A's absolute one. Run-to-run variance on the current benchmark is already a few percent in a quiet devcontainer.
A reasonable outcome may be a narrow version of Option A — an absolute assert with a generous threshold, on one matrix cell only — on the grounds that the failure mode actually being guarded against is two orders of magnitude, not twenty percent, so a loose alarm line catches it while staying far away from the noise floor. But that should be a decision made from measurements, not assumed.
Considerations
- Scope should probably cover the whole
tests/benchmark/suite, not just CTIM. The packet-parsing benchmarks have the same gap. CTIM is the one with a documented budget and a documented incident behind it, so it is the natural first case. - If a threshold is added,
docs/source/benchmarking.mdand the### Benchmarkssection ofdocs/source/developers.mdboth currently state plainly that no timing threshold is enforced anywhere. Both must be updated, or they become actively misleading. - Benchmarks currently run inside the coverage-instrumented pytest invocation. Coverage tracing inflates and distorts timings, so any enforcement should probably run them in a separate, uninstrumented step (
--benchmark-only, no--cov), with the main test job skipping them via--benchmark-skip. Worth confirming how much coverage is currently perturbing the numbers. - Consider whether a failure should be blocking or advisory to begin with. Running the comparison and reporting it without failing, for a few weeks, would produce the per-runner variance data that both options need in order to pick a defensible threshold.
Related: #147, #286
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with tests/benchmark/, especially test_benchmark_ctim_xtce_parsing, and inspect the CI pytest --cov invocation and benchmark configuration. Measure variance across the relevant matrix cells before choosing between an absolute threshold and a stored or same-run baseline. Done means CI enforces a defensible load-time budget without flaky failures, and docs/source/benchmarking.md plus docs/source/developers.md describe the behavior accurately.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- github-actions, python
- Domain
- ci-cd, performance, testing-qa
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100