Conversation
Recommendation 2 of tool-docs/implementation-improvements-from-benchmarking.md: enforce --validation-query-timeout where queries actually execute.
Every anomaly preflight is a SELECT ... LIMIT 100 paired with a *_count aggregate. Both scan the whole graph -- the LIMIT bounds what comes back, not what is examined -- so on the 17.1M-triple benchmark graph the pair cost 97.0 s and 94.6 s to report the same zero. Three such scans were 294.1 s of the 298.1 s that all fourteen preflights cost, against 17.1 s for the thirteen semantic queries they exist to protect. When the sample returns fewer rows than its limit it enumerated every match, so the exact total is the number of rows it returned. Deriving it is arithmetic, not approximation. At or above the limit the count is genuinely unknown and the aggregate still runs. The derived record carries derived/derivedFrom/derivedReason, and benchmark.csv gains a derived column, so a 0.0 s row is never mistaken for an impossibly fast scan when the timings are analysed. Recommendation 3 of implementation-improvements-from-benchmarking.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The interface suggests the representation is the performance decision. The measurement says the opposite. Same thirteen questions, same machine: changing the ENGINE spans three orders of magnitude -- QLever 1.09 s, Comunica 23.30 s, HDT-backed 44.83 s, native pycottas 1402.37 s -- while changing the ARTIFACT under one engine moves nothing: on a 17.1M-triple graph QLever took 17.11 s over N-Triples, 17.16 s over HDT, 16.97 s over COTTAS, under 5% apart. So the default that mattered was the slow one. The benchmark campaign had to pass --validation-engine qlever by hand on every cell large enough for it to matter; this makes that the default and documents the separation in the CLI help and docs/validation.md, so the representation is chosen for size and build cost and the engine for speed. Recommendation 10 of implementation-improvements-from-benchmarking.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The expanded representation emits per sample per record, so its cost is records x samples and is invisible in the input file size: 1000G_phase3_chr20.vcf.gz is 327 MB gzipped and, at 1,812,841 records x 2,504 samples, needs roughly 2 TB against a 189 GB volume. The benchmark harness guarded that in its own shell (bm_skip_if_cohort_scale); the tool did not, so the same file run directly filled the disk with no warning. The guard estimates peak workspace from two coefficients fitted on the sample ladder -- 25 triples per (record x sample), converging to 24.997 by 2504 samples, and 18.3 peak bytes per triple rounded to 20 -- compares it against actual free space, and refuses above 75% of it. The message names the condensed alternative and --allow-cohort-expansion rather than just failing. It only ever refuses expanded. Condensed is ~S + (V x F) and grew 0.9% in total across that same 2504-fold change in samples, so the cohort file still converts in the representation that can hold it. The guard sits ahead of the helper TSVs, which are themselves records x samples work. Recommendation 7 of implementation-improvements-from-benchmarking.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The wrapper always removed them. It removed them at the end of the per-input iteration, which is after the representation build -- 90% of end-to-end wall time. On the whole-file HG005 cell that meant a 2.63 GB TSV set whose last reader finished at 00:03:51 stayed on disk for the remaining 14.8 hours, inside a peak workspace of 16.23 GB that the run held within 90% of for 4.47 h. Every TSV consumer -- RMLStreamer, the sample emitter, the header emitter, the record-detail emitter -- has run by the point this now removes them, which is worth about 2.6 GB of that peak. The end-of-iteration sweep stays as an idempotent safety net over the same helper, and --keep-tsv still suppresses both. Recommendation 5 of implementation-improvements-from-benchmarking.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The campaign could say the build was 90% of end-to-end wall time and no more. Three quantities were missing, and each blocked a decision: Per-chunk and per-merge timings existed -- StageRunner collected them all along -- and were dropped at the wrapper boundary, so of HDT's 19,480 s on the whole HG005 file the split between chunk conversion and ~8 rounds of pairwise merging was unknown. They are now persisted, bucketed by stage kind, with merge rounds counted per representation. The shared chunk pass was never timed. One decompress-and-chunk feeds every requested representation, and each chunk is unlinked once all have consumed it -- but with no timing, the archived records could not show that. The identical chunk_count and chunk_input_bytes under both methods reads equally as one pass or two. It is one; chunk_stream_seconds now says so, with the clock stopped across the yield so a consumer's build time is never charged to the stream. max_rss_kb was null for both representations, because GNU time is not in the image, leaving the stage taking 90% of the run reporting no memory while the mapping stage at under 0.3% reported 1.3-1.8 GB. A getrusage(RUSAGE_CHILDREN) high-water fallback fills it, tagged with its source. Container-volume peak workspace -- invisible to the host trace, and never to be added to it -- is derived from the free-space samples the runner already takes. Recommendation 4 of implementation-improvements-from-benchmarking.md, and a prerequisite for 1 and 2 of its section 2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The mutation-score experiment injected 113 targeted corruptions and the query suite detected 96 -- 0.850. Of the 17 it missed, 7 fall into four classes the report itself attributes to the published SHACL profile: corrupt_allele_value, corrupt_record_index, corrupt_value_item_allele and corrupt_sample_index_expanded. Closing them takes the score to 0.912 with no new oracle and no new query, because the check was already written. It had simply never run. --shacl-shapes needed a path into a vocabulary checkout, so zero of the 62 validation runs in the benchmark campaign enabled it. The shapes and the ontology bundle they need for sh:class are now vendored in vcf_rdfizer_data, digest-pinned in VOCABULARY_PROVENANCE.json with a test that re-checks them against a sibling checkout when one is present, and applied by default. The default is size-gated at 512 MiB rather than unconditional: pyshacl loads the whole graph into memory, and an unknown size counts as too large -- skipping a check is recoverable, an OOM mid-run is not. --no-shacl opts out; --shacl-shapes overrides both. Recommendation 1 of implementation-improvements-from-benchmarking.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reproduced twice and archived. Native pycottas answered 18 of 27 queries against the condensed encoding of a 10,000-record fixture and then ran 41 h, and on a second attempt about 10 h, on q05_sample_genotype_counts without completing -- both at roughly 190% CPU. QLever answered all thirteen questions on that same cell in about a second each, and the sibling expanded encoding finished every engine including pycottas in 185-194 min. The defect is native pycottas x condensed encoding x a genotype-level query, and it cost 51 hours of unattended runtime to find. Naming cottas explicitly is now refused, with the measurement quoted and both alternatives named. --validation-engine all instead drops it with a warning and validates on the other three: asking for 'all' is a request for breadth, and failing the whole run serves that worse -- the campaign's 06_equivalence used 'all' and hung, where this completes and says why. --allow-cottas-condensed attempts it anyway. This is the interim, not the fix. The diagnosis belongs upstream in pycottas; recommendation 11 keeps it open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things the validation stage flagged on the benchmark corpus. One was a real defect; the other was the check being wrong, and that distinction matters more than either fix. The defect: a header-only VCF lost the sample columns its #CHROM line declares. Q9 reported six missing predicates and an rdf:type count of 40 against 42, Q10 reported SampleSet and VCFSample missing, and the run exited non-zero rather than claiming success. The cause is narrow -- SampleRecordStream learns the source file name from the first data row, so with no rows it stays empty and all three emitters, which key their file IRI on it, return immediately. Sample columns are declared by the header, not by the data, so the name now falls back to the header-lines table. Verified on the awkward_no_records fixture: 0 triples before, 8 after, and every predicate and class the oracle expected. The false positive: preflight_missing_token_conformance reported 20 surviving . literals on the 100,000-record HG005 cell. The published shapes permit a bare dot on exactly two predicates -- vcfc:fieldNumber, whose pattern is ^([0-9]+|A|R|G|LA|LR|LG|P|M|[.])$ because Number=. is VCF's variable-cardinality token, and vcfc:genotypeString, where a fully-missing call is literally . -- and neither carries vcfc:Null because neither is missing. The check flagged conformant values; it now excludes those two predicates, in both the sample and its count. Recommendation 9 of implementation-improvements-from-benchmarking.md. The manuscript's claim that the graph retains 20 bad literals does not hold and needs correcting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The campaign's tier-1 run reported 86 links from 86 records, and that number cannot be read. Tier 1 rewrites an identifier the VCF already carries: it cannot fail on a record with an rsID and cannot succeed on one without, so 86/86 describes the fixture, not the linker. Tier 2 made the opposite point: it ran cleanly and produced zero links, because the bundled demo intervals do not overlap the fixture, so executing correctly and linking nothing were the same row in the output. Each linker now records eligible_records (the population that produced a join key), linked_subjects, and their ratio, so a yield has a denominator. Each also records what its assertion rests on: tier 1 an identifier rewrite that is NOT position-verified, tier 2 a coordinate match against a digest-pinned reference, tier 3 a service resolution with a recorded response digest. The linkset node carries all three into the graph, so a consumer reading vcfl:sameVariantAs, a strong predicate that a stale rsID in a re-annotated call set will produce confidently and wrongly, can see its basis without leaving the data. This is the measurable half of recommendation 8. The other half is an experiment, not code: a real cohort against a released dbSNP, with recall against an independent bcftools isec overlap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Caught by the first real pipeline run on bench-1, which the local Docker-less checks could not reach: test-1k failed validation with violationCount 0 and violationPaths [vcfc:fieldSource, vcfc:fieldVersion]. Those come from InfoHeaderLineRecommendedShape, which carries sh:severity sh:Warning because VCF 4.5 *recommends* Source and Version on INFO declarations rather than requiring them. pyshacl sets conforms=False for a result at any severity, so keying the verdict on conforms alone failed a conformant graph -- and, now that shapes are on by default, would have failed almost every real VCF. The verdict is now the violation count. pyshacl's own conforms flag is kept verbatim in the report, because a graph with warnings only is conforms=False and status=PASS, which is exactly the distinction the severities encode. Advisory results are counted and named alongside. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by reading the real report from the bench-1 run. pyshacl 0.30.1 writes each result as a 'Validation Result in <Component>' block with an indented 'Severity: sh:Violation|sh:Warning|sh:Info' line. This module looked for lines starting 'Constraint Violation', which that format never produces, so violationCount was always 0 and the verdict fell through to pyshacl's conforms flag. The layer was therefore broken in both directions at once: it could not report a real violation, and conforms=False failed any graph carrying a mere recommendation. The archived campaign never noticed because SHACL was never enabled in it. parse_shacl_results splits the report into blocks and classifies each by its Severity line. Violations block the run; warnings and info are counted and sampled separately. A block with no severity line is Unknown and treated as blocking, because the safe reading of a parser bug is not 'conformant'. The pre-0.30 'Constraint Violation' spelling still parses, so a downgrade does not silently stop reporting. test/fixtures/shacl-report-warnings-only.txt is the verbatim report from that run: six sh:Warning results for vcfc:fieldSource and vcfc:fieldVersion, zero violations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found on bench-1 after the sample-set fix landed: q09 and q10 passed, but preflight_sample_gt_inventory still failed on awkward_no_records, expecting sampleIdCount 1 and finding 0. vcfc:sampleId lives on SampleCall -- one per sample per record -- so a header-only VCF has no call to carry it and the count is structurally zero. Expecting sampleCount there fails a correct graph. The file-scoped sample set is a different thing and the census queries check it; with the emitter fix those now pass. Condensed is unaffected: its sample set and sampleId literals are file-scoped, so only the per-record vectors go to zero. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The first VM run reported peak_volume_workspace_bytes of 126,956,531,712 -- 127 GB, the host disk's used bytes -- for a build whose scratch was a single 11.9 MB chunk. shutil.disk_usage on /work describes the backing device, and free-space deltas are no better because anything else on the host moves them. StageRunner now samples the size of the /work tree around each stage, and the peak is the largest tree seen. Device-level figures are kept, since a build that fills the volume needs them to say so, but they are no longer mistaken for this build's footprint. A missing workspace reports None rather than a confident 0, and symlinks are not followed so a link into a mounted output tree cannot inflate the figure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measured on bench-1, on a 2,000-triple graph:
vcf-core-vocabulary.shacl.ttl 1.7s
vcf-core-consistency.shacl.ttl 33.7s
vcf-core-vocabulary-sparql.shacl.ttl 73.2s
That reverses the design and corrects a claim. The four mutation classes
the shape layer exists for -- corrupt_allele_value, corrupt_record_index,
corrupt_value_item_allele, corrupt_sample_index_expanded -- are covered by
the consistency and SPARQL profiles, NOT by the core one. The core profile
constrains vcfc:sampleIndex to exist once as an integer >= 1 and says
nothing about two samples sharing a value; uniqueness is a sh:sparql rule
("Record indices must be unique within a VCF file") in the SPARQL profile,
and value agreement against REF/ALT is in the consistency profile.
So defaulting to the core profile alone, as the previous commit did,
detected none of them: the 0.850 -> 0.912 figure belonged to a profile set
that was not being loaded. But those two profiles self-join the graph, so
they cannot be the default either -- 63x the core cost at 2,000 triples,
and widening with size.
--shacl-profile core (default) stays cheap and keeps the 512 MiB gate.
--shacl-profile full adds both, gated to 16 MiB, where two minutes for four
mutation classes is a reasonable trade. --shacl-shapes takes a
comma-separated list, and validate_shacl merges several profiles into one
graph.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every mutation score this repository has ever reported is a query-only score: validate_graph ran the preflight and core queries and never called validate_shacl at all. So the claim that the shape layer closes four of the ten undetected classes could not be checked here, only argued from reading the profiles. VCF_RDFIZER_MUTATION_SHACL=core|full now folds the shape verdict in. A sh:Violation counts as a detection; an execution failure does not, so a missing pyshacl cannot inflate the score with detections nothing made. The report gains shaclProfile, detectedByShacl, detectedByShaclIds and detectedByQueriesOnly, so the number can be attributed to a layer instead of standing as one opaque total. Unset, nothing changes: the default run still reports 96/113, so every archived score stays comparable. A known_undetected mutation that the shape layer catches is recorded rather than failed. Closing one is the point of enabling it, and the catalogue still describes the query layer's coverage, which is what an unqualified run measures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`emit_record_detail` gated the allele layer on `info_representation == "structured"`, on the reasoning that the Number=A/R/G value items are what join to the allele resources. The expanded sample layer joins to them too: every `vcfc:GenotypeAlleleCall` carries `vcfc:calledAllele` pointing at `<record>/allele/<index>`, and it emits that edge regardless of the INFO representation. So `--info-representation raw --sample-representation expanded` produced a graph whose allele references were never described — 1,972 dangling objects per 1,000 records in the covering-set benchmark, with zero `vcfc:alleleIndex` triples anywhere in the file. The SPARQL oracle counts `calledAllele` edges without following the join, so all ten covering-set rows exited 0 through the whole campaign; the shape layer reported it on first contact, as 5,916 violations on `alleleIndex`, `alleleKind` and `alleleValue`. Replace the gate with `allele_layer_required()`, a disjunction over both representation axes, and pass the sample representation down from full mode. `raw` + `condensed` still omits the layer: condensed emits no `calledAllele`, so nothing references an allele there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixing the emitter alone moved the three covering-set failures from a shape violation to a census MISMATCH: the graph gained the 1,972 allele triples it had been missing, and `expected_census` -- which still merged the allele layer only under `info_representation == "structured"` -- reported them as extra rows, together with the 1,972 `rdf:type` triples they carry. Split the allele-layer counters out of `emitted_record_counters`, the way the genotype layer already was, and merge them when structured INFO *or* the expanded sample representation asks for them. Everything the emitter writes inside its `emit_alleles` branch moves with them, the contig and assembly links included, or the two sides would disagree again on the next raw-INFO run. `validation_fixtures.build_graph` mirrored the old rule too; it now calls `allele_layer_required` so the fixture harness cannot drift from the wrapper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…items Brings in #14-#17. The overlap that matters is CottasEngine: dev still had the pycottas subprocess form, main replaced it with the comunica-backed endpoint. Git merges the two cleanly because they touch different regions, and the comunica implementation wins -- verified here that no COTTASStore or COTTAS_QUERY_RUNNER remnant survives, so the merged engine is coherent rather than merely conflict-free.
The guard that refused it was correct when it was written: native pycottas answered 18 of 27 queries against the condensed encoding and then ran 41 hours, and ~10 hours on a second attempt, on q05_sample_genotype_counts without finishing. Both attempts are archived. That engine no longer exists. COTTAS is answered through comunica over DuckDB, and the cell the guard was built around -- test-10k, condensed, all four engines -- now completes in 99.8 minutes with every engine agreeing on every artifact. 06_equivalence as a whole ran in 8.98 h. Leaving the guard in would have been worse than a stale comment. It drops cottas from `--validation-engine all` with a warning rather than an error, and 06_equivalence runs with `all` -- so the two condensed cells would have lost the engine the experiment exists to compare, and the run would have looked complete. The merge that brought the new engine in was textually clean and would have shipped exactly that. Removes the two resolution helpers, the --allow-cottas-condensed opt-in, and the call site. The flag landed after v3.0.3 and was never released, and the benchmark harness never passed it, so nothing external depends on it. The guard's test file is replaced rather than deleted: the new one asserts the helpers and the flag stay gone and that `all` keeps every engine, so a reintroduction has to be deliberate instead of arriving as a merge artifact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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.
Brings the 19 commits on
devintomain, plus the merge of #14–#17 and one fix that the merge itself made necessary.What is in it
The SHACL shape layer and its cheap/full profiles, the vendored vocabulary bundle, per-experiment SHACL policy, the census oracle learning the emitter's allele rule, QLever as the validation default, a cohort-scale refusal before the volume fills, TSV intermediates freed at last reader, and severity-aware pyshacl parsing.
The part that needs review
devstill had the pycottasCottasEngine;mainhad replaced it with the comunica-backed endpoint. Git merged the two cleanly — they touch different regions — and the comunica implementation won. I verified noCOTTASStoreorCOTTAS_QUERY_RUNNERremnant survives, so the merged engine is coherent rather than merely conflict-free.But
devalso added a guard refusing COTTAS against a condensed graph, written while the hang was real. That guard survived the merge, and it is now false:06_equivalencewholeLeaving it in would have been worse than a stale comment. It drops cottas from
--validation-engine allwith a warning rather than an error, and06_equivalenceruns withall— so the two condensed cells would have silently lost the engine the experiment exists to compare, and the run would have looked complete. A textually clean merge would have shipped exactly that.So this removes the two resolution helpers, the
--allow-cottas-condensedopt-in and the call site. The flag landed afterv3.0.3and was never released; the benchmark harness never passed it. The guard's test file is replaced rather than deleted — the new one asserts the helpers and flag stay gone and thatallkeeps every engine, so a reintroduction has to be deliberate.Verification
860 unit tests. One failure, pre-existing and not from this branch:
test_the_vendored_copies_match_the_vocabulary_checkoutreports the vendoredvcf-core-vocabulary.bundle.ttlat 2.1.2 against a local vocabulary checkout at 2.1.3. It was 2.1.2 before this merge too, and the test skips without a sibling checkout, so CI will not see it — but the vendored bundle being a patch behind is worth a decision before the release.🤖 Generated with Claude Code