Skip to content

Add reproducible ranker calibration harness - #190

Open
heymrbox wants to merge 8 commits into
mainfrom
heymrbox/cac-80-cac-53-follow-up-calibrate-ranker-defaults-against-captured
Open

Add reproducible ranker calibration harness#190
heymrbox wants to merge 8 commits into
mainfrom
heymrbox/cac-80-cac-53-follow-up-calibrate-ranker-defaults-against-captured

Conversation

@heymrbox

@heymrbox heymrbox commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add a reproducible replay harness that sweeps all five ranker knobs through the production LookupRoute implementation.
  • Check in an explicitly synthetic boundary fixture, generated per-knob curves, deterministic replay clock, and CI drift verification.
  • Keep existing production defaults unchanged until representative captured data supports a retune.

Linked issues

N/A - tracked internally; no public GitHub issue.

Checklist

Vendor-neutral naming (required - see CONTRIBUTING.md)

  • No prohibited vendor identity or domains in core surfaces.
  • No cloud-specific integration added outside adapters.
  • No prohibited domains or namespaces in samples or defaults.
  • Pre-commit naming guard passed.

Quality

  • Every human-authored commit includes a matching DCO Signed-off-by trailer.
  • make reuse-lint passes.
  • make build and make test pass locally.
  • make lint is clean.
  • make manifests generate produces no drift.
  • New behavior has unit tests; calibration package coverage is 98.9%.
  • N/A - no operator-facing API or install surface changed.
  • CI is green. Fresh run is in progress for 789191c.

Contracts

  • Calibration methodology and provenance are documented in the design docs.
  • Backward compatibility preserved; production defaults and wire behavior are unchanged.
  • N/A - proto was not changed.
  • N/A - CRD API types and the proto contract were not changed.

Signed-off-by: Weiwei Zheng <heymrbox@gmail.com>
@linear-code

linear-code Bot commented Aug 19, 2026

Copy link
Copy Markdown

CAC-80

CAC-53

@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • .github/workflows/
    • .github/workflows/ci.yml [reviewed]
  • Root
    • Makefile [reviewed]
  • docs/design/
    • docs/design/grpc-contract.md [reviewed]
    • docs/design/lookuproute-ranking.md [reviewed]
  • docs/reference/
    • docs/reference/reason-codes.md [reviewed]
  • hack/ranker-calibration/
    • hack/ranker-calibration/main.go [reviewed]
  • internal/index/
    • internal/index/ranking.go [reviewed]
    • internal/index/ranking_test.go [reviewed]
  • internal/index/calibration/
    • internal/index/calibration/README.md [reviewed]
    • internal/index/calibration/calibration.go [reviewed]
    • internal/index/calibration/calibration_test.go [reviewed]
  • internal/index/calibration/testdata/
    • internal/index/calibration/testdata/c1_synthetic_result.json [skipped — generated]
    • internal/index/calibration/testdata/c1_synthetic_trace.json [reviewed]
  • site/content/en/docs/reference/
    • site/content/en/docs/reference/reason-codes.md [reviewed]

Findings

Blocking

None.

Should-fix

  • internal/index/ranking.go:19 — The PR changes production defaults based solely on a hand-constructed synthetic trace whose expected outcomes encode the desired boundary decisions. The PR itself acknowledges no production trace exists, so this demonstrates that the harness can select a predetermined tuple rather than providing evidence that these defaults improve representative workloads.
  • internal/index/calibration/calibration.go:340 — Replay anchors timestamps with time.Now() but lets the production index use separate real-time clock reads. Runtime delay therefore changes freshness and age calculations, making close ranking decisions and calibration boundaries timing-dependent instead of reproducing the observation’s exact point in time; inject a fixed replay clock.

Nit

  • docs/design/lookuproute-ranking.md:877 — The statement that CI prevents “documentation” from silently diverging is too strong. Tests connect the trace, generated result, and code defaults, but no check parses the numeric defaults from these documentation pages.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: Clean
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: internal/index/ranking.go:19, internal/index/calibration/calibration.go:340, docs/design/lookuproute-ranking.md:877

Verdict

changes-requested

@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.96610% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/index/calibration/calibration.go 97.93% 3 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

Signed-off-by: Weiwei Zheng <heymrbox@gmail.com>
@heymrbox

Copy link
Copy Markdown
Contributor Author

Addressed the deterministic replay and documentation findings in 95063e3.

  • Replay now injects each observation timestamp into the production index clock, and replica timestamps come directly from the trace.
  • Added a regression test for a TENANT_HOT candidate one millisecond inside the configured age window.
  • Narrowed the documentation claim: CI and tests keep the trace, generated result, and code defaults aligned; prose remains review-driven.

Verification: make pre-pr passed, including race tests, builds, generated-code drift, and sample admission.

The synthetic-trace/defaults concern remains open because it needs a product decision: keep the explicitly provisional defaults selected by the synthetic fixture, or retain the previous production defaults until a sanitized captured C1 trace is available.

@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • .github/workflows/
    • .github/workflows/ci.yml [reviewed]
  • Root
    • Makefile [reviewed]
  • docs/design/
    • docs/design/grpc-contract.md [reviewed]
    • docs/design/lookuproute-ranking.md [reviewed]
  • docs/reference/
    • docs/reference/reason-codes.md [reviewed]
  • hack/ranker-calibration/
    • hack/ranker-calibration/main.go [reviewed]
  • internal/index/
    • internal/index/index.go [reviewed]
    • internal/index/ranking.go [reviewed]
    • internal/index/ranking_test.go [reviewed]
  • internal/index/calibration/
    • internal/index/calibration/README.md [reviewed]
    • internal/index/calibration/calibration.go [reviewed]
    • internal/index/calibration/calibration_test.go [reviewed]
  • internal/index/calibration/testdata/
    • internal/index/calibration/testdata/c1_synthetic_result.json [skipped — generated]
    • internal/index/calibration/testdata/c1_synthetic_trace.json [reviewed]
  • site/content/en/docs/reference/
    • site/content/en/docs/reference/reason-codes.md [reviewed]

Findings

Blocking

  • internal/index/ranking.go:19 — Production defaults are changed based solely on a synthetic, hand-constructed trace — The trace explicitly says it does not represent production traffic (internal/index/calibration/testdata/c1_synthetic_trace.json:7), yet its expected outcomes determine the new defaults. Boundary cases can verify the harness, but they cannot substantiate calibration of production behavior; retain the existing defaults until captured or otherwise representative data supports changing them.

Should-fix

None.

Nit

None.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: internal/index/ranking.go:19, internal/index/calibration/testdata/c1_synthetic_trace.json:7
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): Clean

Verdict

changes-requested

Signed-off-by: Weiwei Zheng <heymrbox@gmail.com>
@heymrbox

Copy link
Copy Markdown
Contributor Author

Addressed the patch-coverage gap in 24e2579 by covering trace, sweep, observation, and marshal validation/error paths. The calibration package now reports 98.9% statement coverage locally.

Verification: go test -cover ./internal/index/calibration and the full pre-push make ci gate passed.

@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • .github/workflows/
    • [reviewed] .github/workflows/ci.yml
  • Root
    • [reviewed] Makefile
  • docs/design/
    • [reviewed] docs/design/grpc-contract.md
    • [reviewed] docs/design/lookuproute-ranking.md
  • docs/reference/
    • [reviewed] docs/reference/reason-codes.md
  • hack/ranker-calibration/
    • [reviewed] hack/ranker-calibration/main.go
  • internal/index/calibration/
    • [reviewed] internal/index/calibration/README.md
    • [reviewed] internal/index/calibration/calibration.go
    • [reviewed] internal/index/calibration/calibration_test.go
    • [reviewed] internal/index/calibration/testdata/c1_synthetic_result.json
    • [reviewed] internal/index/calibration/testdata/c1_synthetic_trace.json
  • internal/index/
    • [reviewed] internal/index/index.go
    • [reviewed] internal/index/ranking.go
    • [reviewed] internal/index/ranking_test.go
  • site/content/en/docs/reference/
    • [reviewed] site/content/en/docs/reference/reason-codes.md

No generated files were changed.

Findings

Blocking

None.

Should-fix

  • internal/index/calibration/testdata/c1_synthetic_trace.json:5 — Production defaults are being changed based solely on a hand-constructed synthetic fixture whose outcomes select the proposed tuple. The fixture explicitly does not represent production traffic, so its 100% result cannot substantiate changing routing behavior for every deployment; retain the existing defaults until captured or independently derived evidence is available, or clearly frame these as policy choices rather than calibration results.
  • hack/ranker-calibration/main.go:42 — The new command’s write/check control flow has no command-level tests. Add coverage for stale-result failure, current-result success, output generation, and malformed input so regressions in the CI gate itself are caught rather than only exercising the calibration library.

Nit

  • hack/ranker-calibration/main.go:53 — Result generation writes directly to the destination, so an interrupted run can leave a truncated checked-in artifact. Writing a sibling temporary file and renaming it would make regeneration atomic.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: internal/index/calibration/testdata/c1_synthetic_trace.json:5
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: hack/ranker-calibration/main.go:42, hack/ranker-calibration/main.go:53

Verdict

changes-requested

Signed-off-by: Weiwei Zheng <heymrbox@gmail.com>
@heymrbox heymrbox changed the title Calibrate ranker defaults with replay harness Add reproducible ranker calibration harness Aug 20, 2026
@heymrbox

Copy link
Copy Markdown
Contributor Author

Addressed the blocking synthetic-data/defaults finding in 789191c.

  • Restored the existing production defaults: PressureWeight 1.0, TenantHotMinHitRate 0.1, and TenantHotMaxAge 5m.
  • Decoupled the synthetic fixture candidate from DefaultRankerConfig.
  • Updated the calibration docs to state that the synthetic result verifies the harness only and cannot drive production defaults without representative captured data.

Verification: make pre-pr and make cover-check pass; production default code and reference docs now have no drift from main.

@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • .github/workflows/
    • .github/workflows/ci.yml [reviewed]
  • docs/design/
    • docs/design/lookuproute-ranking.md [reviewed]
  • hack/ranker-calibration/
    • hack/ranker-calibration/main.go [reviewed]
  • internal/index/
    • internal/index/index.go [reviewed]
    • internal/index/ranking_test.go [reviewed]
  • internal/index/calibration/
    • internal/index/calibration/README.md [reviewed]
    • internal/index/calibration/calibration.go [reviewed]
    • internal/index/calibration/calibration_test.go [reviewed]
  • internal/index/calibration/testdata/
    • internal/index/calibration/testdata/c1_synthetic_result.json [reviewed]
    • internal/index/calibration/testdata/c1_synthetic_trace.json [reviewed]
  • /
    • Makefile [reviewed]

Findings

Blocking

None.

Should-fix

  • internal/index/calibration/calibration.go:66 — The trace has only one reported_at_ms, which replay uses for both prefix freshness and statsReported. Production deliberately maintains these clocks independently, so captured observations involving liveness/prefix refreshes can be replayed with materially different pressure, freshness, and TENANT_HOT behavior; record separate prefix and statistics timestamps.

Nit

  • internal/index/calibration/calibration.go:137 — Millisecond values are validated only as positive before conversion to time.Duration, so sufficiently large ttl_ms or tenant_hot_max_age_ms values overflow and silently alter replay behavior. Reject values exceeding the representable duration range.
  • internal/index/index.go:269 — Exported WithClock accepts a nil function and leaves the index to panic during normal operations. Either reject/ignore nil or document the non-nil requirement.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: Clean
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: internal/index/calibration/calibration.go:66, internal/index/calibration/calibration.go:137, internal/index/index.go:269

Verdict

changes-requested

Signed-off-by: Weiwei Zheng <heymrbox@gmail.com>
@heymrbox

Copy link
Copy Markdown
Contributor Author

Addressed the latest review in d30cc3a.

The replay trace now carries independent prefix-freshness and replica-stat timestamps, and replay feeds them into the index as separate observations. That lets calibration model fresh cache locality alongside stale pressure data correctly.

I also tightened the harness around the review nits: millisecond durations reject overflow, a nil injected clock safely keeps the default clock, result files are replaced atomically, and command-level tests cover generate/check/stale/malformed flows.

Verification passed locally:

  • make pre-pr
  • make cover-check
  • pre-push make ci gate

@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • .github/workflows/
    • [reviewed] .github/workflows/ci.yml
  • Repository root
    • [reviewed] Makefile
  • docs/design/
    • [reviewed] docs/design/lookuproute-ranking.md
  • hack/ranker-calibration/
    • [reviewed] hack/ranker-calibration/main.go
    • [reviewed] hack/ranker-calibration/main_test.go
  • internal/index/
    • [reviewed] internal/index/index.go
    • [reviewed] internal/index/ingest_test.go
    • [reviewed] internal/index/ranking_test.go
  • internal/index/calibration/
    • [reviewed] internal/index/calibration/README.md
    • [reviewed] internal/index/calibration/calibration.go
    • [reviewed] internal/index/calibration/calibration_test.go
  • internal/index/calibration/testdata/
    • [skipped — generated] internal/index/calibration/testdata/c1_synthetic_result.json
    • [reviewed] internal/index/calibration/testdata/c1_synthetic_trace.json

Findings

Blocking

None.

Should-fix

  • internal/index/calibration/calibration.go:149 — Require at least one observation of each supported kind — A trace containing only prefix or only tenant-hot observations is accepted, causing the absent class’s knobs to be selected solely by tie-breaking while still presenting a five-knob “best” calibration.
  • internal/index/calibration/calibration.go:197 — Validate ttft_budget_ms and unused matched_tokens values — Negative TTFT budgets are silently interpreted as no SLO, and negative matched-token counts are accepted whenever reported_prefix is false; malformed captured data can therefore contaminate calibration without failing ingestion.

Nit

None.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: Clean
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: internal/index/calibration/calibration.go:149, internal/index/calibration/calibration.go:197

Verdict

changes-requested

Signed-off-by: Weiwei Zheng <heymrbox@gmail.com>
@heymrbox

Copy link
Copy Markdown
Contributor Author

Addressed the two validation findings in 2c4eed2.

Calibration traces now have to include at least one prefix observation and one tenant-hot observation, so every supported ranking mode contributes evidence to the selected five-knob result. Input validation also rejects negative ttft_budget_ms values and negative matched_tokens, including when a replica does not report the prefix.

Focused regression tests cover each rejected shape, and the full make pre-pr plus pre-push make ci gates passed.

@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • .github/workflows/
    • .github/workflows/ci.yml [reviewed]
  • Repository root
    • Makefile [reviewed]
  • docs/design/
    • docs/design/lookuproute-ranking.md [reviewed]
  • hack/ranker-calibration/
    • main.go [reviewed]
    • main_test.go [reviewed]
  • internal/index/
    • index.go [reviewed]
    • ingest_test.go [reviewed]
    • ranking_test.go [reviewed]
  • internal/index/calibration/
    • README.md [reviewed]
    • calibration.go [reviewed]
    • calibration_test.go [reviewed]
  • internal/index/calibration/testdata/
    • c1_synthetic_result.json [skipped — generated]
    • c1_synthetic_trace.json [reviewed]

Findings

Blocking

None.

Should-fix

  • internal/index/calibration/calibration.go:384 — Replay ingests every recorded prefix and statistic without removing entries already older than the production TTL — LookupRoute filters stale exact matches, but stale entries remain in servingByScope and can enable TENANT_HOT; stale stats likewise survive because the replay never runs an eviction sweep. Captured traces containing TTL-expired state can therefore calibrate against candidates production would normally have evicted.

Nit

  • internal/index/calibration/calibration.go:176 — Sweep validation accepts finite float64 values too large for the production float32 knobs — conversion at lines 365–368 can yield infinity and produce invalid ranking arithmetic. Validate float32 representability before replaying the grid.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: Clean
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: internal/index/calibration/calibration.go:176, internal/index/calibration/calibration.go:384

Verdict

changes-requested

Signed-off-by: Weiwei Zheng <heymrbox@gmail.com>
@heymrbox

Copy link
Copy Markdown
Contributor Author

Addressed the replay-state findings in c25c817.

Replay now reconstructs the post-sweep production state at each observation timestamp: prefix and statistics reports at or beyond the configured TTL are excluded before lookup, so stale serving entries cannot create a tenant-hot candidate. Sweep values that would overflow the production ranker’s float32 knobs are also rejected before conversion.

Added regressions for TTL-expired serving state and oversized pressure/bias values. make pre-pr, the calibration drift check, and the pre-push make ci gate all pass.

@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • .github/workflows/
    • .github/workflows/ci.yml [reviewed]
  • Repository root
    • Makefile [reviewed]
  • docs/design/
    • docs/design/lookuproute-ranking.md [reviewed]
  • hack/ranker-calibration/
    • main.go [reviewed]
    • main_test.go [reviewed]
  • internal/index/
    • index.go [reviewed]
    • ingest_test.go [reviewed]
    • ranking_test.go [reviewed]
  • internal/index/calibration/
    • README.md [reviewed]
    • calibration.go [reviewed]
    • calibration_test.go [reviewed]
  • internal/index/calibration/testdata/
    • c1_synthetic_result.json [reviewed]
    • c1_synthetic_trace.json [reviewed]

Findings

Blocking

  • internal/index/calibration/calibration.go:431 — Captured observations cannot distinguish an actual miss from an unobserved counterfactual — ObservedHit is a required boolean for every replica, but a normal captured request reveals the outcome only for the replica actually selected. Unobserved alternatives therefore become false and bias every candidate configuration that would choose another replica; add outcome-availability/selection information and an appropriate evaluation method, or require and document experimentally measured outcomes for every candidate.

Should-fix

  • internal/index/calibration/calibration.go:60prefix_hash is modeled as a JSON string and converted directly to bytes, despite the production contract defining it as engine-opaque bytes. Arbitrary non-UTF-8 hashes cannot round-trip through this schema; use a byte representation such as base64-backed []byte.
  • internal/index/calibration/calibration.go:186 — Sweep validation rejects zero for SLOTightTTFTMs and TenantHotMaxAge, although zero is a supported production kill switch. This prevents the harness from evaluating the disabled configurations and conflicts with the design contract at docs/design/lookuproute-ranking.md:840.
  • internal/index/calibration/calibration.go:384 — Fabricated serving-prefix keys share the same unrestricted byte namespace as real engine hashes. A requested hash equal to serving/<observation>/<replica> converts a declared non-holder into a prefix match; choose and validate a collision-free synthetic key per observation.

Nit

None.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: internal/index/calibration/calibration.go:60, internal/index/calibration/calibration.go:186
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: internal/index/calibration/calibration.go:431, internal/index/calibration/calibration.go:384

Verdict

changes-requested

Signed-off-by: Weiwei Zheng <heymrbox@gmail.com>
@heymrbox

Copy link
Copy Markdown
Contributor Author

Addressed the trace-contract findings in 6280a83.

The harness no longer treats an unobserved counterfactual as a miss. Every replica row must explicitly declare outcome_available: true; captured traces must measure each candidate under an equivalent cache snapshot, while synthetic traces may define ground truth by construction. Incomplete rows are rejected.

Prefix hashes are now opaque []byte values with base64 JSON encoding and a non-UTF-8 round-trip test. Serving-only entries use a request-derived, length-extending key that cannot equal the requested hash, with a regression for the old collision shape. The sweep also accepts zero for the production TTFT and tenant-hot kill switches.

The design note and fixture were updated, and make pre-pr, the drift check, focused coverage, and the pre-push make ci gate pass.

@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • .github/workflows/
    • [reviewed] .github/workflows/ci.yml
  • Repository root
    • [reviewed] Makefile
  • docs/design/
    • [reviewed] docs/design/lookuproute-ranking.md
  • hack/ranker-calibration/
    • [reviewed] hack/ranker-calibration/main.go
    • [reviewed] hack/ranker-calibration/main_test.go
  • internal/index/
    • [reviewed] internal/index/index.go
    • [reviewed] internal/index/ingest_test.go
    • [reviewed] internal/index/ranking_test.go
  • internal/index/calibration/
    • [reviewed] internal/index/calibration/README.md
    • [reviewed] internal/index/calibration/calibration.go
    • [reviewed] internal/index/calibration/calibration_test.go
  • internal/index/calibration/testdata/
    • [reviewed] internal/index/calibration/testdata/c1_synthetic_result.json
    • [reviewed] internal/index/calibration/testdata/c1_synthetic_trace.json

Findings

Blocking

None.

Should-fix

None.

Nit

None.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: Clean
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): Clean

Verdict

approve

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.

1 participant