Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
==========================================
- Coverage 94.86% 94.80% -0.07%
==========================================
Files 50 51 +1
Lines 4321 4347 +26
==========================================
+ Hits 4099 4121 +22
- Misses 222 226 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟢 Approval recommended
The documentation changes are reviewed; the only finding is a non-blocking nit about an issue reference.
Pull request overview
Documents the CTIM XTCE parsing benchmark as a manual performance regression guard.
Changes:
- Adds CTIM benchmark context, history, budget, and comparison workflow.
- Documents benchmark guidance for contributors.
- Records the update in the unreleased changelog.
File summaries
| File | Summary |
|---|---|
docs/source/developers.md |
Adds benchmark guidance and workflow. |
docs/source/benchmarking.md |
Documents CTIM’s regression-guard purpose. |
CHANGELOG.md |
Records the documentation update. |
Review details
Suppressed comments (1)
docs/source/benchmarking.md:293
- The PR description says follow-up issue #285 tracks designing this CI gate, but repository issue #285 is actually about invalid
CITATION.cffmetadata and is unrelated. Please correct the issue reference in the PR description (or omit it) so the rationale for leaving enforcement manual remains traceable.
budget is currently a manual step. If you are touching `space_packet_parser/xtce/`, run the
comparison above before opening a PR.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| These run as part of the normal test suite and in CI, but no timing threshold is enforced anywhere — | ||
| a performance regression will not fail a build. |
There was a problem hiding this comment.
This is really what we should do. Documentation is not harmful, but the real solution to this is to check benchmarks against thresholds. If a set threshold is exceeded, that should fail the build. This changes the scope of this PR but that is OK. We should implement the real solution.
| {py:class}`~space_packet_parser.xtce.definitions.XtcePacketDefinition` across all the files you | ||
| parse, rather than re-reading the XTCE document per file or per process. | ||
|
|
||
| ### The CTIM Definition as a Regression Guard |
There was a problem hiding this comment.
This is too verbose. We should rely on CI tests checking this regression state, not a document that likely no one will read. Again, this is changing the scope of the PR but it's clear that we should actually check this in CI.
The CTIM benchmark in tests/benchmark/test_xtce_parsing_benchmarks.py exists to keep a specific, already-fixed performance bug from returning, but nothing said so anywhere in the documentation. Early versions resolved each SequenceContainer's BaseContainer by searching the whole ContainerSet by name and then re-parsing the referenced container from scratch, entry list and all, with no memoization. Because all 38 of CTIM's concrete containers share one abstract base, that base was re-parsed 38 times and loading the definition took on the order of twelve seconds. Threading a container lookup table through SequenceContainer.from_xml dropped it by more than two orders of magnitude; it now loads in the mid-30s of milliseconds. The benchmarking page now records that history, states the 100 ms alarm line the benchmark exists to protect, and documents the --benchmark-autosave / --benchmark-compare workflow, which is the only way pytest-benchmark actually detects a regression. The developer documentation's testing section now points at the benchmark suite. The 100 ms budget is deliberately NOT enforced anywhere. CI runs the benchmarks as part of the normal suite and discards their timings; a bare timing assert would be flaky across the CI matrix and stored baselines are machine-specific. A note documents that enforcement gap honestly rather than papering over it, and a follow-up issue tracks designing a real gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Add per-(platform, Python version) time thresholds to all 9 tests in tests/benchmark/, checked via a shared assert_within_threshold helper in tests/benchmark/conftest.py - Run the coverage-instrumented test step with --benchmark-disable (coverage distorts timings) and add a dedicated CI step that runs tests/benchmark/ uninstrumented to enforce the thresholds - Update developers.md and benchmarking.md to describe the enforced gate, and trim the CTIM-specific regression prose now that CI enforces the budget generically across the whole suite - Update the CHANGELOG entry to describe enforcement instead of documentation-only Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
7535087 to
eba14e5
Compare
- Update the four slow benchmarks' Linux/3.13 thresholds using actual ubuntu-latest timings from CI run 35263961684, which failed 3 of them: the local devcontainer undercounts GitHub's shared runner by ~2-2.5x for these benchmarks - Widen the "default" fallback for untested (OS, Python version) combinations to 3x the real measured number instead of 4x a local figure that itself understated the true cost - No change to the 5 fast (nanosecond-scale) benchmarks, which passed the same CI run comfortably under half their threshold - Note in developers.md that Linux/3.13 thresholds are now grounded in real CI data, not just local measurement Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Add --benchmark-json to the benchmark regression gate step - Add a step, run even when the gate fails, that renders each benchmark's mean/min/max/rounds as a markdown table in $GITHUB_STEP_SUMMARY, keyed by the runner's OS and Python version Gives real as-run numbers for all 15 matrix cells without digging through job logs, so the threshold tables in tests/benchmark/*.py can be tightened per platform once enough runs have accumulated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- test_benchmark_complex_xtce_definition_parsing: 0.04s -> 0.07s. A macOS run in CI run 35266958957 produced a single 52.99ms round against an 11.1ms mean, exceeding the old default threshold on its max even though the mean-based gate never fired. - test_benchmark_complex_packet_parsing: 0.022s -> 0.035s. Both a macOS run (23.45ms) and a Windows run (22.23ms) produced individual rounds that exceeded the old default threshold, against means of ~7-10ms. Both tests show higher round-to-round variance than the other 7 benchmarks, which stay well under their thresholds on both mean and max across all 15 CI matrix cells. No change to the (Linux, 3.13) thresholds or the other 7 tests, since nothing there was violated and a single sample per platform isn't enough to justify tightening. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fold each round's setup (file seek + fresh generator) into the benchmarked callable itself, so it's safe to call repeatedly without a separate pedantic setup hook. This lets these two tests drop their fixed rounds=20 in favor of pytest-benchmark's own calibration, same as the other two benchmarks in this suite already do. complex_packet_parsing now auto-calibrates to ~300 rounds locally (vs. the previous fixed 20), giving a substantially more stable mean. simple_packet_parsing settles at the 5-round floor since each round takes ~200ms. Verified with a 5-run noise probe (mean + 10% margin thresholds, run locally, reverted after): no failures, but two tests came within 5-6% of that tight threshold from ordinary run-to-run noise alone, confirming a flat 10% margin isn't safe for the actual committed thresholds. No threshold values changed by this commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
tests/benchmark/test_xtce_parsing_benchmarks.pyexists to keep a specific, already-fixed performance bug from coming back, but nothing in the documentation said so. The benchmarking page presented CTIM's 37 ms as a neutral data point, no document stated the "under 100 ms" budget the test's own docstring refers to, anddevelopers.mdnever mentioned that a benchmark suite exists at all.This is a documentation-only change. Three edits:
docs/source/benchmarking.md— a new### The CTIM Definition as a Regression Guardsubsection appended to the existing## XTCE Definition Parsing Performancesection. It records CTIM's structure (1668 kB / 19,283 XML elements, 9,493Parameterdeclarations across only 15 distinct parameter types, 39SequenceContainers, 9,559ParameterRefEntryreferences), explains why that shape makes it a good canary, tells the history, states the budget, and documents the--benchmark-autosave/--benchmark-compareworkflow — which is the only way pytest-benchmark actually detects a regression.docs/source/developers.md— a### Benchmarkssubsection under## Testing, pointing contributors attests/benchmark/and the save/compare workflow before they change anything underspace_packet_parser/xtce/orpackets.py, with a link to the benchmarking page.CHANGELOG.md— a### Changedentry under## [Unreleased].The existing CTIM table row is left alone; 37 ms is correct within noise.
The regression being documented
Early versions resolved a
SequenceContainer'sBaseContainerby searching the wholeContainerSetby name and then parsing the referenced container from scratch, entry list and all, with nothing memoizing the result. All 38 of CTIM's concrete containers declare the same abstract base, so that base was re-parsed 38 times, and the same pattern applied toContainerRefEntry. Loading the definition took on the order of twelve seconds. Threading a container lookup table throughSequenceContainer.from_xmldropped it by more than two orders of magnitude, and the benchmark was added in that same change. Notably, the bug was invisible to the correctness tests, which passed the whole time it was present — which is precisely why the benchmark is worth explaining rather than leaving as a bare number.The budget is NOT enforced by CI — deliberately
Worth being explicit, because it is the main judgement call here. CI runs the benchmarks as part of the normal
pytestinvocation and then discards the timings entirely. Nothing is saved, compared, or gated. A reader could reasonably assume a benchmark running in CI is a gate; it is not.Adding enforcement was considered and deliberately left out of scope. A bare
assert benchmark.stats.stats.mean < 0.1would be evaluated across the full CI matrix — Windows, macOS, five Python versions, shared runners — and is a likely source of intermittent red builds; a CI-side--benchmark-compare-failneeds durable, machine-matched baseline storage, which is a real design question and not one a documentation ticket should answer by hand. Rather than paper over the gap, a MyST{note}on the benchmarking page states it plainly and tells contributors touchingspace_packet_parser/xtce/to run the comparison manually. Follow-up issue #287 tracks designing an actual gate.Measured number
Re-measured on this branch before writing it down: 34.1 ms mean (min 33.0, max 38.8, 23 rounds), and 35.1 ms mean in a full-suite run. That is inside the page's existing hardware caveat — all numbers on that page are declared as measured in the project devcontainer on Apple Silicon — so no new caveat paragraph was added. The measured figure appears exactly once, in a sentence that hedges it; the load-bearing 100 ms number is stated as a policy decision rather than a measurement, so it does not rot when hardware changes.
Verification
pre-commit run --all-filespasses; prettier made no changes.make htmlbuilds clean with no new warnings.docs/build/html/benchmarking.htmlthat the{note}renders as<div class="admonition note">and not as literal text — 6.2.0 shipped a fix for exactly that rendering bug, so this uses the MyST fence, not> [!NOTE]. Both new tables render as tables.### Benchmarksappears indevelopers.htmland itsbenchmarking.mdlink resolved tobenchmarking.html.ctim_xtce_v1.xmlitself rather than taken on trust.Closes #147
🤖 Generated with Claude Code