feat(eval): golden Q&A matching in core — the producer for GOLDEN_EXPECTED_ANSWER (closes #428)

[evekhm]

Extracts golden Q&A matching from scripts/quality_report.py into bigquery_agent_analytics.golden_matching, per #428 — completing the U4 story: #378 shipped the consumer (per_session_context + CategoricalContextSource.GOLDEN_EXPECTED_ANSWER); this ships the producer.

What's in core now:

• embed_texts(texts, model=None, batch_size=50, max_attempts=5) — L2-normalised vectors with bounded 429/503 retry (the resilience added after an embedding quota burst aborted a live gate run during Skill-evolution lab: remove the #358/#359 workarounds (U6, closes #360) #385's AE8).
• DEFAULT_GOLDEN_THRESHOLD = 0.92.
• match_golden_qa(question_by_key, golden_qa, threshold) → per-key judge-context mapping (expected answers + out-of-scope decline notes) ready for evaluate_categorical(per_session_context=..., context_source=GOLDEN_EXPECTED_ANSWER), plus per-key match metadata. Keys are opaque: exact ResolvedTraceSelectors on the BigQuery path, session ids locally.

Same-PR thinning: the script keeps its public/test surface via aliases and delegates everything; the embedding-retry tests move to tests/test_golden_matching.py alongside new matching tests (threshold behavior, decline notes, empty inputs, opaque keys).

Follow-up (noted on #428): a server-side embedding path via ai_ml_integration's AI.EMBED/ML.DISTANCE would remove the genai embedding dependency entirely. Refs #378, #385, #63.

[caohy1988]

Code review: golden Q&A matching in core

Verdict: request changes — one real trap, plus a pattern break. The extraction itself is verbatim (I diffed the moved embed_texts/match_golden_qa against the originals) and the new tests are good, including the opaque-key test. CI is green across Python 3.10–3.14. Findings, most important first:

1. The _embed_texts alias is a silent monkeypatch trap (medium). The comment says the aliases "keep this module's public/test surface stable", but patching quality_report._embed_texts no longer affects match_golden_qa — the SDK function calls golden_matching.embed_texts through its own module global. This PR's own test change proves the break: test_golden_matching_preserves_exact_selector_keys had to be re-pointed at golden_matching. Any downstream host that patches or wraps quality_report._embed_texts (the skill-evolution-lab host is exactly the kind of consumer that does this) will silently regress to real network embedding calls. Since the alias no longer delivers what its comment promises and nothing in the module calls it (zero call sites by grep), I'd delete it so downstream code breaks loudly instead. It also lacks the # noqa: F401 its unused sibling EMBEDDING_MODEL carries.

2. First module-level SDK imports in the script (medium-low). On main, scripts/quality_report.py defers every bigquery_agent_analytics import into functions — a consistent, clearly deliberate pattern (zero module-level SDK imports before this PR). The four new mid-file # noqa: E402 imports make the whole SDK package tree (pydantic, google-cloud-bigquery, google-adk via __init__) a hard import-time dependency, so even --help pays it. Moving them into the functions that use them restores the pattern and drops the noqa suppressions.

3. EMBEDDING_MODEL is now frozen at package import (low). os.getenv("EMBEDDING_MODEL", ...) moved to module level in golden_matching, which __init__.py imports eagerly — so the env var is captured the moment anyone imports bigquery_agent_analytics, and setting it programmatically afterward does nothing. Previously it was captured at script import, which at least followed .env loading in host flows. Reading the env inside embed_texts (as the model=None default) keeps the knob live.

4. Cosmetic, pre-existing but now public API: the retry log prints attempt, max_attempts - 1, so with max_attempts=5 the first retry logs "(1/4)". Worth a one-line fix while the code is being promoted into core.

Cross-PR note: this PR and #431 both insert their export block at the same anchor in __init__.py (right before "# Categorical Views") — a guaranteed conflict for whichever merges second; the quality_report.py hunks don't overlap and should auto-merge.

[caohy1988]

Review: Approve with nits

Verified locally: diffed the deleted block in scripts/quality_report.py (normalized for the _embed_texts/_DEFAULT_GOLDEN_THRESHOLD renames) against the new golden_matching.py — the move is verbatim; the only difference is the threshold constant moving above embed_texts(). Nothing was lost in the −202 deletions. Ran both affected test files: 135 passed. The producer output shape matches #378's consumer (_validated_context_mapping in categorical_evaluator.py:263-281 accepts exactly what match_golden_qa emits). Retry logic is correct and bounded.

Findings

MINOR — backward-compat claim slightly overstated: monkeypatch surface moved (scripts/quality_report.py:353)
The script binds embed_texts as _embed_texts, but match_golden_qa now resolves embed_texts in golden_matching's module globals. Before this PR, patching quality_report_module._embed_texts intercepted embeddings for match_golden_qa; now it silently does nothing (the in-repo test was updated at tests/test_quality_report_helpers.py:304-306, so the authors know). Any external host/test patching the script's _embed_texts will break quietly. Suggest a one-line comment on the alias noting it's re-export-only, or a release note.

MINOR — EMBEDDING_MODEL env-var read timing moved earlier
The script used to read EMBEDDING_MODEL at its own import; now it's captured when bigquery_agent_analytics/__init__.py eagerly imports golden_matching (i.e., at SDK import). A host that sets the env var after importing the SDK gets the old default. Also, rebinding quality_report_module.EMBEDDING_MODEL (now an F401 alias) no longer affects the embedding model. Edge-case, but a real semantic change from the "aliases keep the surface stable" claim.

NIT — test_keys_are_opaque uses a key type the consumer would reject (tests/test_golden_matching.py:132)
Uses ("identity", "scope") (a tuple) as the opaque key, but _validated_context_mapping raises TypeError for anything that isn't exactly ResolvedTraceSelector or str (categorical_evaluator.py:271-281). The test proves opacity with a key that would fail downstream; use a real ResolvedTraceSelector to demonstrate the actual contract.

NIT — docstring overclaims key preservation (golden_matching.py:106)
Says "per_session_context preserves each input key", but keys with empty/None questions are silently dropped (:116) and get no metadata entry either. Pre-existing behavior, but now a public API docstring — worth one clause: "keys with empty questions are omitted."

NIT — unused import math left behind (scripts/quality_report.py:59)
math was only used by the extracted _embed_texts; no math. references remain.

NIT — parameter name contradicts the description/issue (golden_matching.py:92)
Both the PR body and #428 specify match_golden_qa(question_by_key, ...); the code keeps question_by_sid. Keeping the old name is defensible for compat, but the description is inaccurate and sid undercuts the "opaque keys" story.

NIT — dead try/except ImportError guard (__init__.py:394-410)
The module imports only stdlib at module level and google-genai (a hard dependency) lazily inside embed_texts — the guard can never fire. Harmless and consistent with the file's style; just noting it buys nothing.

Positives

• Move verified verbatim; threshold semantics (>=, first-best tie-breaking, 0.92 default, round(score, 4)) unchanged.
• Retry logic correct: bounded at max_attempts, immediate re-raise on non-429/503, backoff capped at 2+4+8+16s per batch; tests cover all three branches with call-count assertions.
• The two script call sites still pass ResolvedTraceSelector keys on the BQ path and session-id strings locally, then feed per_session_context + CategoricalContextSource.GOLDEN_EXPECTED_ANSWER — exactly what the consumer validates.

Review by @caohy1988's assistant (Kimi Code CLI).

[caohy1988]

Code review: current head a5dfd7e

Verdict: Ready with fixes. The extraction preserves the existing matching and retry behavior, and the relevant local suites pass (135 passed). One public-boundary issue should be fixed before merge.

P2 - validate public matching controls

src/bigquery_agent_analytics/golden_matching.py:61-63,141

The new public APIs accept invalid control values with inconsistent or silent results:

• embed_texts(..., batch_size=-1) returns an empty vector list.
• embed_texts(..., batch_size=0) raises the implementation-level range() error.
• embed_texts(..., max_attempts=0) raises UnboundLocalError because resp is never assigned.
• match_golden_qa(..., threshold=-0.1) can match everything; threshold=NaN matches nothing.

Fix: require batch_size >= 1 and max_attempts >= 1, and require a finite threshold in [0.0, 1.0]. Raise ValueError with the invalid value and add zero/negative/infinity/NaN boundary tests.

Everything else reviewed cleanly: opaque keys are preserved, out-of-scope decline context is correct, transient 429/503 retries remain bounded, package-root exports work, and script callers delegate to the core module.

Merge coordination: PR #431 edits the same export location in src/bigquery_agent_analytics/__init__.py. Rebase after #431 lands, retain both export blocks, and rerun the combined suite.

[caohy1988]

Open-PR sweep recheck at unchanged head a5dfd7e.

No commits have landed since my full current-head review, and the current CI checks remain green. The Ready with fixes verdict still applies: the public matching controls still need validation for positive integer batch/retry values and a finite threshold in [0, 1] so invalid inputs do not return empty output, leak range() errors, or reach the uninitialized retry path.

Full current-head review: #432 (comment)

Coordination note: #431 still edits overlapping evaluator/export surfaces, so whichever PR lands second should be refreshed against the first.

[caohy1988]

Requesting changes at exact head a5dfd7e. The focused suites pass (135 tests) and current CI is green, but the public matching controls still need validation.

At src/bigquery_agent_analytics/golden_matching.py:61, batch_size, max_attempts, and threshold flow into range/retry/comparison logic without validation. Zero or negative batch sizes cause implementation-level errors or silent no-work behavior; zero attempts can leave resp unbound; and non-finite or out-of-range thresholds silently produce nonsensical matching.

Please require positive batch_size and max_attempts, plus a finite threshold in [0.0, 1.0], raise ValueError for invalid values, and add boundary/non-finite tests. This was independently revalidated on the current head.

[evekhm-odyssey-bot]

Both outstanding items from the changes-requested review are addressed at head 0398e24:

P2 — validate public matching controls (fixed in 0398e24)

golden_matching.py now validates all three public controls exactly as requested:

• embed_texts raises ValueError (naming the invalid value) for batch_size < 1 and max_attempts < 1, before any client construction — so batch_size=-1 no longer silently returns an empty vector list, batch_size=0 no longer leaks the implementation-level range() error, and max_attempts=0 can no longer reach the unbound-resp path.
• match_golden_qa raises ValueError unless threshold is finite and in [0.0, 1.0] (math.isfinite + range check), so threshold=-0.1 can no longer match everything and NaN can no longer silently match nothing. Validation runs before the empty-input early return, so a bad threshold never silently succeeds just because there was nothing to match.
• Both docstrings now carry a Raises: section documenting the contract.

New boundary tests (parametrized) cover every case named in the review: batch_size 0/−1, max_attempts 0/−3, threshold −0.1 / 1.1 / +inf / −inf / NaN, plus accepted-bound tests proving batch_size=1, max_attempts=1, and threshold 0.0/1.0 still work (the comparison stays >= at both bounds).

Merge coordination with #431 (done in c012401)

#431 landed, so the branch is merged up to current main (e6e763c). The predicted __init__.py conflict at the shared anchor was resolved by retaining both export blocks — evaluation rubrics first (as on main), golden matching after it, both before "# Categorical Views".

Verification at 0398e24: combined suite tests/test_golden_matching.py + tests/test_quality_report_helpers.py + tests/test_evaluation_rubrics.py → 153 passed (135 pre-existing + 12 new boundary tests + 6 rubrics tests from the merge-up); package-root import exports all five symbols from both blocks; pyink --check and isort --check-only clean on the changed files.

[caohy1988]

LGTM - approved at exact head 0398e24.

I re-reviewed the complete extraction, the merged #431 export block, and the latest public-control fix. The previously requested zero/negative batch and retry cases plus non-finite/out-of-range thresholds now fail early with documented ValueErrors. The combined exact-head suites pass (153 tests), git diff --check is clean, and current CI is green across formatting, build, and Python 3.10-3.14.

One nonblocking public-API edge remains: positive non-integral batch_size or max_attempts values pass the new < 1 checks and later raise TypeError from range(). Please tighten those controls to positive integers (and add 1.5 tests) in this PR if convenient or as a small follow-up. This does not block approval because the documented count controls work correctly for their supported integer usage.