Skip to content

fix(test): bfs_latency_v3 smoke flake — bump iterations + decouple from gate verdict (#792) - #793

Merged
github-actions[bot] merged 2 commits into
mainfrom
fix/issue-792-bfs-latency-smoke-flake
May 14, 2026
Merged

fix(test): bfs_latency_v3 smoke flake — bump iterations + decouple from gate verdict (#792)#793
github-actions[bot] merged 2 commits into
mainfrom
fix/issue-792-bfs-latency-smoke-flake

Conversation

@robotrocketscience

Copy link
Copy Markdown
Owner

Closes #792

Two atomic commits unblock the pytest (3.13) smoke flake that's been silently blocking PRs (most recently #790).

Root cause

test_main_end_to_end_smoke calls main([... "--queries", "4", "--iterations", "2", "--warmup", "1", ...]) → 8 timed samples per arm. _percentile uses nearest-rank semantics; with n=8 the p95 index resolves to ceil(0.95*8) = 8 = max sample. So p95 = p99 = max_sample, and a single noisy timer reading on a shared CI runner trips the delta_p95 ≤ 50ms gate. The test then trips on assert rc == 0.

The file docstring already declares the intent:

"These tests exercise the corpus generator, query generator, percentile helper, and end-to-end CLI flow on a tiny (40 beliefs / 4 topics) corpus. Latency numbers are not asserted — only schema, determinism, and gate-result shape."

But assert rc == 0 is precisely a latency-numbers assertion. Defence in depth fixes both halves.

Commit 1 — bump iterations 2→10, warmup 1→2

40 samples per arm → p95 = sample[37], no longer max. Local pytest-3.13 runtime: 0.79s (no measurable regression; the earlier 16.7s was a cold-venv build, not the test itself).

Commit 2 — decouple smoke from gate verdict

  • assert rc == 0assert rc in (0, 1) (both = harness ran end-to-end and emitted JSON).
  • Add shape assertions on payload["gate"]: every key is present, numeric values are numeric, *_pass keys are bool.
  • Cross-check (rc == 0) is gate["passed"] so a flake can't decouple the two without something more interesting going wrong.

Dedicated test_evaluate_gate_* fixtures above already cover gate logic deterministically with hand-rolled ArmResult objects — gate verdict is not the smoke's job.

Verification

  • uv run --python 3.13 pytest tests/test_bfs_latency_v3.py -q11 passed in 1.10s.
  • Both commits SSH-signed, no Co-Authored-By lines per repo convention.
  • Diff: 1 file, +23/-6.
  • Discretion grep on diff vs github/main: CLEAN.

Out of scope

  • Production-bench gate thresholds (GATE_DELTA_P50_MS, GATE_DELTA_P95_MS, GATE_MAX_OVER_MEDIAN_RATIO) — those are the real benchmark's contract for ≥1000-sample runs.
  • _percentile semantics — nearest-rank is correct at production sample sizes.

@robotrocketscience robotrocketscience added the author-mondragon Authored by parallel session mondragon label May 14, 2026
@coderabbitai

coderabbitai Bot commented May 14, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@robotrocketscience has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 3 minutes and 52 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5b646012-1ed9-494c-9933-2d95374c419d

📥 Commits

Reviewing files that changed from the base of the PR and between eff8a1c and b2287ef.

📒 Files selected for processing (1)
  • tests/test_bfs_latency_v3.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/issue-792-bfs-latency-smoke-flake

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@robotrocketscience robotrocketscience added the attn:review Needs review (PR open, awaiting reviewer) label May 14, 2026
@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:jojo:2026-05-14T16:15:44Z]

With --queries 4 --iterations 2, the smoke produces 8 samples per
arm. nearest-rank p95 with n=8 resolves to the max sample, so a
single noisy timer reading determines the entire percentile — and
on GitHub Actions runners sharing CPU with neighbours, the slowest
sample can spike to 700-1100ms, blowing the 50ms delta_p95 gate
and tripping the test's 'assert rc == 0'.

Raising to 10 iterations gives 40 samples per arm; p95 lands at
the 38th-largest sample, so a single outlier no longer determines
the verdict. Bumping warmup 1→2 stabilises the first-sample cold
path (HRR + BM25F cache build).

Local pytest-3.13 runtime: 0.66s → 16.7s. Still smoke-scale —
acceptable defence against the recurring CI flake documented in
issue #792.
…shape (#792)

The file docstring says: 'Latency numbers are not asserted — only
schema, determinism, and gate-result shape.' But the assertion
'assert rc == 0' contradicts that — rc encodes gate.passed which
is a latency assertion. The dedicated test_evaluate_gate_* fixtures
above already cover gate logic with hand-rolled ArmResults; the
smoke's job is the CLI plumbing.

Change:
  - assert rc == 0  →  assert rc in (0, 1)
  - add shape assertions on payload['gate']: delta_p50_ms,
    delta_p95_ms, tail_ratio types numeric; *_pass keys all bool.
  - cross-check rc encoding: '(rc == 0) is gate.passed' — flake
    cannot decouple the two without something more interesting
    going wrong.

This matches the stated test intent and survives any future
GitHub-runner noise event without re-blocking PRs. Belt-and-
braces against the n=8 → p95=max-sample race fixed by the
prior commit.
@robotrocketscience
robotrocketscience force-pushed the fix/issue-792-bfs-latency-smoke-flake branch from 9cd16e6 to b2287ef Compare May 14, 2026 16:17
@robotrocketscience

Copy link
Copy Markdown
Owner Author

Review — approving

Targeted defence-in-depth on the bench-smoke flake. Both halves are necessary and sufficient:

  • Iterations 2→10 / warmup 1→2 ⇒ 40 samples per arm. Nearest-rank p95 index resolves to ceil(0.95*40) = 38, so p95 is no longer max-of-sample. Doesn't eliminate runner noise, but breaks the "one slow sample dominates" mode.
  • assert rc == 0assert rc in (0, 1) restores the smoke's stated contract (file docstring: "Latency numbers are not asserted — only schema, determinism, and gate-result shape"). Gate-verdict assertions move to the dedicated test_evaluate_gate_* fixtures above, which use hand-rolled ArmResult objects and are timing-independent.

The (rc == 0) is gate["passed"] invariant is the right load-bearing check — it pins rc/payload consistency without binding either to a specific value, so the smoke catches a real bug (rc and JSON drift apart) while tolerating runner noise.

Two atomic commits, both SSH-signed; +23/-6 in one file; discretion grep on rebased diff clean; CI all green on the rebased SHAs (f7bcf2b3 / b2287ef6). Rebased onto current main (post-#790) because the original tip was behind 81b458a.

Minor nit (not blocking): the new shape assertion enumerates delta_p50_ms / delta_p95_ms / tail_ratio / *_pass / passed but doesn't cover anything else the gate dict might grow later. The schema is currently complete, so this is forward-only.

Labeling ready-to-merge.

@robotrocketscience robotrocketscience added the ready-to-merge Trigger merge-train: FF main to this PR's head label May 14, 2026
@github-actions
github-actions Bot merged commit b2287ef into main May 14, 2026
27 checks passed
@github-actions github-actions Bot removed the ready-to-merge Trigger merge-train: FF main to this PR's head label May 14, 2026
@github-actions

Copy link
Copy Markdown

merge-train: merged b2287efmain via FF push.

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:jojo:2026-05-14T16:21:03Z]

@robotrocketscience
robotrocketscience deleted the fix/issue-792-bfs-latency-smoke-flake branch May 20, 2026 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

attn:review Needs review (PR open, awaiting reviewer) author-mondragon Authored by parallel session mondragon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(test): bfs_latency_v3 smoke flakes — thin-sample p95 = max trips the 50ms delta gate on CI

1 participant