Repository navigation
test(perf): measure the offload heartbeat against its own idle baseline, not a 7.5ms clock (#15221) - #15248
Merged
Conversation
…ne, not a 7.5ms clock (#15221)
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml.
…d the residual false-failure path (#15221)
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
test_a_scan_in_a_process_does_not_delay_the_event_loopended withassert in_process < 0.005 * 1.5— a 7.5 ms ceiling on a 5 ms heartbeat, 1.5x headroom,on the CI runner itself. Its own message said "something is still contending for this
process", which is simultaneously the defect it guards (the scan not being offloaded) and
the runner's ordinary weather. Through an absolute clock the two are the same event, which
is why it fired at 50.6 ms on a PR whose entire diff was two session-ownership test files.
Does #15207's work unit transfer? No — and this is the substance of the change. The
quantity here is contention, not duration, and a CPU-throughput yardstick fails it twice:
reference_workload()is starved by exactly thecontention under test. Numerator and denominator inflate together, the ratio holds at
~1.0 whether or not the scan is offloaded, and the detector is gone. That is the trap
the issue asked about, and it is fatal.
quantity — CPU throughput, not event-loop wakeup latency. Measured here across six
consecutive samples it swung 1.43 ms – 3.27 ms: a 2.3x spread in the denominator
alone, wider than the 2.0x signal the assertion has to resolve. It would manufacture
flake rather than absorb it.
What replaces it. The heartbeat's own idle baseline: the same 5 ms sleep, on the same
loop, in the same process, in windows a fraction of a second either side of the scan. Same
quantity, same moment, differing only in whether a scan was in flight — the #14930 move
(measure the delta the assertion always meant), not the #15207 one. Over the same six
samples the baseline held to ±1.4% where the CPU unit swung 2.3x.
Same discipline as #15207 in form: a ratio, a recorded measurement on every run, an
explicit vacuity guard, and a down-only budget. Different yardstick, because the quantity
is different. The new helper is a sibling of
assert_within_work_budgetin the same moduleso the next contention site does not re-derive it.
What Changed
autobot_shared/perf_work_budget.pyassert_within_baseline_ratio(measured_ms, baseline_ms, budget_ratio, what): assertsa measurement against a caller-supplied baseline of the same quantity. Reports on every
run by both test(perf): startup benchmarks assert absolute wall-clock budgets and fail on runner load — passed and failed on the same code minutes apart #15055 routes — a
[perf #15221]stdout line and aperf_baseline_ratiojunit property.
load-tolerance algebra rests on and the residual false-failure path it leaves open (see
Verification), and two things the caller owns rather than the helper: a settling gap
before a trailing baseline window, and the choice of statistic over each window.
baseline.
autobot-backend/code_intelligence/shared/process_offload_test.py_median_tick_delay_during_scan→_tick_latency_during_scan, returning a_TickLatencywith the busy median, the idle median, both tick counts, and the pool's availability.
Idle ticks are collected in two 0.3 s windows bracketing the scan.
assert_within_baseline_ratio(busy, idle, 1.5, …)replaces it.The existing in-process-vs-in-thread comparison is unchanged.
_TICK_BUDGET_VS_IDLE = 1.5, derived from measurement and documented as down-only:offloaded readings 0.992–1.023, in-thread 1.994–2.024.
test_the_responsiveness_budget_cannot_pass_without_a_measurement— the vacuity probe.recording_work_units) added, as in the benchmarks file.settling gap deliberately — this scan leaves nothing taxing the loop behind it, and a
workload that did would need one, or its tail would land in the baseline and shrink the
ratio.
pytest.skipfor "process pool unavailablehere" could never fire — the helper reset
_pool_unavailable_reasontoNonebefore thecaller read it. On a host that forbids process creation the scan runs in-thread silently
and the test reported a regression that was really an environment — the same
environment-vs-defect confusion this issue is about. The reason is now captured before
the reset and returned.
Verification
A genuine regression still fails.
run_directory_scanwas mutated to force the scanin-process (
pool = Noneafter_get_pool()), reintroducing #12866:The relative assertion fires first, so a second run with it neutralised isolates the new one:
Both mutations reverted from pristine copies and confirmed with
diff -q;git statusshows only the two intended files.
Load tolerance — a bounded claim, not an absolute one. Not established by generating
load: this machine runs live CI and the issue forbids it. Established by the algebra of the
measure plus repeated observation, with its premise stated.
Under an external delay L the baseline reads
5+Land the measurement5+L+tax, so theratio is
1 + tax/(5+L), decreasing in L. That holds only where L is comparableacross the baseline and measurement windows — and the three windows are temporally
disjoint, spanning roughly one to two seconds end to end, on the CI runner itself. A load
burst from a sibling job that lands inside the
busywindow and is gone before the trailingidle window inflates the numerator with no matching rise in the denominator, and pushes the
ratio up. That is a real false-failure path this change does not exclude.
What it buys is a much narrower one. The old absolute ceiling tripped on any load bump;
this requires a spike timed to miss both bracketing windows, and to move the median of a
~0.5 s window without moving either adjacent ~0.3 s one. The baseline is taken as two
windows bracketing the measurement rather than one before it precisely so a monotone drift
is shared by both terms. So the honest guarantee is "much less likely to fire on load",
not "impossible" — a red here is evidence to investigate, not proof of a regression on its
own. Both the helper docstring and the test say so, so the next person to hit one does not
go hunting for a defect that is not there.
Five consecutive runs reported 0.992, 1.001, 1.009, 1.010, 1.012 against a budget of 1.5:
a 2% spread with 47% headroom, while the regression sits at 1.99.
Vacuity probe —
test_the_responsiveness_budget_cannot_pass_without_a_measurementasserts
AssertionErrorfor a zero measurement, a negative measurement, an absentmeasurement, a zero baseline, and both terms absent; then a positive control
(
5.1 / 5.0→ 1.020x) proves the assertion is not one that can never pass. The≥10 ticks per windowguards cover the "heartbeat never ran" case upstream of the median.Sweep — every
*_test.pyunderautobot-backend/code_intelligence/was searched fortime.time/time.monotonic/time.perf_counter/timeit/elapsed/duration/latency/_msand each hit classified. Exactly one absolute wall-clock assertion exists in the tree —
the one this PR replaces. One file in the tree takes a real clock delta at all
(
process_offload_test.py). Everything else matching those tokens is a parsed orconstructed literal, a config default, or a key-presence check. The benchmarks file's own
remaining drifting budgets are #15235's and are untouched here.
Tests —
process_offload_test.py: 9 passed (8 pre-existing + the vacuity probe), fivefurther repeats of the load-bearing test green.
system_benchmarks_performance_test.pystill collects its 14 tests against the extended module.
Not verified / known limits
checks. The local interpreter sits below 37 declared dependency floors, so a local pass is
weaker evidence than CI.
median, so a regression that delays only a minority of ticks still will not move it.
Not introduced here, but it is now the documented behaviour of a shared helper, and the
docstring tells the next caller to pick a high percentile where a tail is what matters.
Refs #15055, #15207, #15235. Closes #15221.
Model Used
Claude Opus 5 (1M context)