From 6934f3c0cf71a84d0272bdb3ce0d8a561fc23edf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Quang=20H=C3=B2a?= Date: Tue, 26 May 2026 11:31:37 +0700 Subject: [PATCH] fix(db): include techniques in result-row uniqueness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #1 left benchmark_results_unique and eval_results_unique unchanged when it demoted spec_method to a per-measurement jsonb. With two MTP variants (mtp_layers=4 vs 6) now sharing one config_id, rows with the same (workflow_run, config, isl, osl, conc) but different techniques collided on the existing constraint and the later insert overwrote the earlier one. First post-deploy ingest of gemma4 run 26387431571 landed 6 of ~24 expected rows for exactly this reason. Migration 007 adds techniques to both unique constraints and re- truncates so re-ingest from scratch is clean. The bulk insert paths also need techniques in their ON CONFLICT clause and in their in-batch dedup discriminator — fixed in benchmark-ingest.ts and eval-ingest.ts. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../007_techniques_in_uniqueness.sql | 32 +++++++++++++++++++ packages/db/src/etl/benchmark-ingest.ts | 16 +++++++--- packages/db/src/etl/eval-ingest.ts | 5 ++- 3 files changed, 45 insertions(+), 8 deletions(-) create mode 100644 packages/db/migrations/007_techniques_in_uniqueness.sql diff --git a/packages/db/migrations/007_techniques_in_uniqueness.sql b/packages/db/migrations/007_techniques_in_uniqueness.sql new file mode 100644 index 000000000..c6e8a5441 --- /dev/null +++ b/packages/db/migrations/007_techniques_in_uniqueness.sql @@ -0,0 +1,32 @@ +-- ============================================================ +-- Migration 007 — include techniques in result-row uniqueness +-- ============================================================ +-- +-- Migration 006 made techniques a per-measurement dimension but left +-- benchmark_results_unique and eval_results_unique unchanged. Result: two +-- measurements at the same (workflow_run, config, isl, osl, conc) but with +-- DIFFERENT techniques (e.g. mtp_layers=4 vs 6 — same gemma4 deployment, two +-- MTP variants) collide on insert and the later one silently overwrites the +-- earlier one. +-- +-- Hot-fix discovered during the first post-006 ingest: gemma4 run 26387431571 +-- landed only 6 of ~24 expected rows because n4 and n6 variants now share a +-- config_id and clobbered each other. +-- +-- Fix: add the techniques jsonb to both unique constraints. Postgres b-tree +-- works on jsonb (used in latest_benchmarks PK already), so this is a clean +-- add. Also re-truncate so the re-ingest after this migration is from scratch. + +alter table benchmark_results drop constraint benchmark_results_unique; +alter table benchmark_results add constraint benchmark_results_unique + unique (workflow_run_id, config_id, benchmark_type, isl, osl, conc, techniques); + +alter table eval_results drop constraint eval_results_unique; +alter table eval_results add constraint eval_results_unique + unique (workflow_run_id, config_id, task, isl, osl, conc, techniques); + +-- Wipe partially-clobbered data; auto-ingest will repopulate. +truncate benchmark_results, eval_results, eval_samples, run_stats, + availability, changelog_entries, server_logs, workflow_runs + restart identity cascade; +truncate configs restart identity cascade; diff --git a/packages/db/src/etl/benchmark-ingest.ts b/packages/db/src/etl/benchmark-ingest.ts index f61fc1a8e..258138f09 100644 --- a/packages/db/src/etl/benchmark-ingest.ts +++ b/packages/db/src/etl/benchmark-ingest.ts @@ -29,9 +29,16 @@ export async function bulkIngestBenchmarkRows( // Postgres rejects ON CONFLICT DO UPDATE if the same conflict key appears // more than once in a single batch. Deduplicate within the batch, keeping - // the last occurrence (last metrics for each unique config/isl/osl/conc). + // the last occurrence. The conflict key must include techniques — two MTP + // variants at the same (config, isl, osl, conc) are legitimately distinct + // measurements and must not collapse to one row (migration 007). const seen = new Map(); - for (const r of rows) seen.set(`${r.configId}-${r.isl}-${r.osl}-${r.conc}`, r); + for (const r of rows) { + // parseTechniques() builds the techniques object with deterministic key + // order, so a plain JSON.stringify is a stable dedup discriminator. + const techniquesKey = JSON.stringify(r.techniques); + seen.set(`${r.configId}-${r.isl}-${r.osl}-${r.conc}-${techniquesKey}`, r); + } const deduped = [...seen.values()]; const configIds = deduped.map((r) => r.configId); @@ -58,11 +65,10 @@ export async function bulkIngestBenchmarkRows( unnest(${sql.array(images)}), unnest(${sql.array(metricsJsons)}::jsonb[]), unnest(${sql.array(techniquesJsons)}::jsonb[]) - on conflict (workflow_run_id, config_id, benchmark_type, isl, osl, conc) + on conflict (workflow_run_id, config_id, benchmark_type, isl, osl, conc, techniques) do update set metrics = excluded.metrics, - image = excluded.image, - techniques = excluded.techniques + image = excluded.image returning (xmax = 0) as inserted, id `; diff --git a/packages/db/src/etl/eval-ingest.ts b/packages/db/src/etl/eval-ingest.ts index c01715014..b607319b2 100644 --- a/packages/db/src/etl/eval-ingest.ts +++ b/packages/db/src/etl/eval-ingest.ts @@ -38,10 +38,9 @@ export async function ingestEvalRow( ${p.isl}, ${p.osl}, ${p.conc}, ${p.lmEvalVersion}, ${sql.json(p.metrics)}, ${sql.json(p.techniques)} ) - on conflict (workflow_run_id, config_id, task, isl, osl, conc) + on conflict (workflow_run_id, config_id, task, isl, osl, conc, techniques) do update set - metrics = excluded.metrics, - techniques = excluded.techniques + metrics = excluded.metrics returning id, (xmax = 0) as inserted `; return { outcome: row.inserted ? 'new' : 'dup', id: row.id };