Let a validation run select which queries to answer - #25
Merged
Merged
Conversation
Retrieval cost at scale was only measurable by running the entire query suite. Measured on the v3.1.0 17.1M-triple cell, per artifact, the thirteen core queries cost 17 s and the preflight set costs 201 s -- so timing the thirteen that Figure 6 reports meant paying twelve times over for numbers the figure does not contain. At 657M triples that is the difference between roughly 33 minutes and 7 hours, per artifact, per replicate. --queries (and --validation-queries on the wrapper) selects ids or the groups core/preflight/all. A subset deliberately does NOT produce a validation verdict. evaluate_validation decides PASS/MISMATCH from preflight gating, the sample/GT inventory and invariants computed across the whole set; handed a subset it would either fail on the missing keys or, worse, report a pass that only meant the queries you happened to ask for agreed. Subset runs therefore go through evaluate_timing_only and report TIMING_ONLY, and the summary omits the keys a validation consumer reads so that misreading one fails loudly instead of silently. What a subset keeps is the equality check: every selected core query is still compared against the cyvcf2 oracle and a disagreement still fails the run, so the protocol behind Figure 6 -- equality established before any timing is compared -- holds for a subset too. Asking for an anomaly preflight's _count alone pulls in its sample query, which is where the count is derived from; without that the graph would be re-scanned for a number already in hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Says what the flag saves (17 s of core queries against 201 s of preflight on the 17.1M cell), and why a subset reports TIMING_ONLY rather than a verdict. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Measured against main, the change added 65 executable lines to the runner and 3 to the wrapper. The original tests reached 69% of the runner's and 67% of the wrapper's; both are now 100%, and the gaps were where they always are. The wrapper's uncovered line was the one that matters: nothing checked that --validation-queries populates engine_options. That wiring lives in main(), which the earlier tests never entered -- they called run_validation_mode with engine_options already built, proving the container boundary and saying nothing about whether the flag ever reaches it. This project has already shipped that exact failure once: the runner honoured --info-representation, the wrapper never sent it, and every raw-INFO conversion validated against a structured oracle until a benchmark campaign noticed. The runner's uncovered lines were the selection filter and the TIMING_ONLY summary inside run_validation. Those now run for real against a faked engine and oracle, so the tests check that a subset shortens the executed query list, lands on the timing summary rather than the verdict summary, keeps canonical execution order, and still exits non-zero when an answer disagrees. The test added first is the one worth keeping longest: on a full selection the subset path's per-query comparisons are asserted equal to compare()'s, query by query, on both an agreeing and a disagreeing graph. That pins the property the feature rests on -- --queries changes which queries are checked, never how one is checked -- so the manuscript's "equality established before any timing was compared" holds for subset runs too. Two of these tests were passing for the wrong reason before being fixed: the runner's argument parser exits 2 on a missing /work scratch directory, so a rejection test that only asserted the exit code would have passed without ever reaching the query validation. It now asserts the message names the bad query. 35 tests, up from 10. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merged
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
2 participants
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.
Why
Measuring retrieval cost meant running the entire query suite. On the v3.1.0 17.1M-triple cell, per artifact:
q01–q13)Figure 6 of the manuscript reports the thirteen. Obtaining them cost twelve times over in numbers the figure does not contain. Extrapolated to the 657M-triple whole-genome graph that is the difference between roughly 33 minutes and 7 hours — per artifact, per replicate.
What
--querieson the runner,--validation-querieson the wrapper. Takes ids or the groupscore/preflight/all.The part worth reviewing
A subset does not produce a validation verdict, and that is deliberate.
evaluate_validationdecides PASS/MISMATCH from preflight gating, the sample/GT inventory, and invariants computed across the whole query set. Handed a subset it would either fail on the missing keys or — the real risk — report a pass that only meant the queries you happened to ask for agreed.So subset runs go through a separate
evaluate_timing_onlyand reportTIMING_ONLY. The summary omitscomparisonStatusandpreflightentirely, so a consumer that reads a subset run expecting a validation result fails to find one rather than reading it as a pass. The existing verdict path is untouched.What a subset keeps is the equality check. Every selected core query is still compared against the cyvcf2 oracle, and a disagreement still fails the run. The protocol behind Figure 6 — result equality established before any timing is compared — therefore holds for a subset too. A fast wrong answer is not a result.
One subtlety: asking for an anomaly preflight's
_countalone pulls in its sample query, because the count is derived from the sample. Without that the graph would be re-scanned for a number already in hand.Testing
test/test_query_selection_unit.py, 10 tests: group expansion, canonical ordering (execution order matters for the derived counts), the_countpull-in, dedup, rejection of unknown and empty selections, and the two timing-only properties — that an agreeing subset carries no verdict keys, and that a disagreement still fails.The existing
*_unit.pysuite passes unchanged.🤖 Generated with Claude Code