Add a ratcheted predicate domain/range audit (#301, #302) - #314
Conversation
validate-strict treats predicate_id as a bare string, so a CURIE whose ontological domain/range no causal node type can satisfy is a false type entailment that passes every gate. This adds scripts/audit_predicate_domains.py + an audit-predicate-domains recipe in the qc chain, flagging two classes: - MICROBE_DOMAIN_ON_NONORGANISM (#301): predicate transitively subPropertyOf METPO:2000001 (microbe domain) on a non-organism causal subject. 366 edges. The microbe-domain predicate set is derived at run time from the subPropertyOf closure in data/raw/metpo.owl, not a hand-maintained list. - ENABLES_RANGE_ON_TRAIT (#302): enables/RO:0002327 pointed at a TRAIT object when its biolink range is 'biological process or activity'. 164 edges. Neither is fixable without a per-family biological/ontology decision (#301, #302, #303), so it ships as a ratchet identical to audit-graphs: conf/predicate_domain_audit_baseline.tsv freezes the 530 known findings, any NEW violation fails. Registered in audit-derived-reports so a stale committed reports/predicate_domain_audit.tsv is caught too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @realmarcin's task in 4m 26s —— View job Review: ratcheted predicate domain/range auditVerified the substance rather than the description:
No 🔴 or 🟡. Three 🔵s, all optional: 🔵 The inherited range half is unchecked, and the first baseline row shows it. 🔵 🔵 Two things I'll note in the ratchet's favour, since they're the parts that usually go wrong: No Approving. |
![culturebot-reviewer[bot]](https://avatars.githubusercontent.com/u/233210009?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verified the substance, not just the description: METPO:2000001 really is rdfs:domain METPO:1000525 / rdfs:range METPO:1000526 in data/raw/metpo.owl:159-161; CausalNodeTypeEnum (src/traitmech/schema/traitmech.yaml:485) genuinely has no organism-ish member, so the unconditional flag on pid in microbe_domain is sound; all 94 subPropertyOf assertions use the rdf:resource form the ElementTree walk expects; corpus predicate_id prefixes are uniformly RO:/METPO:/biolink:/rdfs: with no biolink:enables, so hard-coding RO:0002327 leaks nothing today. Counts reconcile (366 + 164 = 530), baseline and report are byte-identical, and every baseline _key is unique — no collapsed suppressions. qc, pytest and pr-sanity are green.
The two parts of a ratchet that usually ship broken are both right here: _key's leading-token discriminator is subject--predicate_id-->object, so each edge keys independently and survives edits to the human-readable tail; and partition is factored out and unit-tested across all three --fail-on modes including 'any' ignoring the baseline.
No blocking findings. Three optional notes, detailed in the review comment:
-
The inherited range half is unchecked, and the first baseline row shows it — conf/predicate_domain_audit_baseline.tsv:2, edge low_pathogen_hazard --METPO:2000202(produces)--> bsl1_trait. METPO:2000202 inherits range METPO:1000526 (chemical) and the object is a TRAIT, but the report says only 'subject entails microbe'. The closure walk already has the parent chain, so it is nearly free, and a curator burning down #301 from this report will not see it. Declared out of scope in the PR body, so a follow-up.
-
_curie (scripts/audit_predicate_domains.py:98-101) passes non-METPO IRIs through unchanged. If METPO ever asserts an OBO-IRI property under 2000001, the closure holds the full IRI and never matches the corpus CURIE — a silent under-report in exactly the mechanism the docstring advertises as future-proof. Not live today.
-
tests/test_audit_predicate_domains.py:192-223 is nearly vacuous: the CLEAN constant is unused, and the test writes a graph that does flag, filters that defect out, then asserts the remainder is empty. The path it meant to prove is already covered by test_non_microbe_predicate_not_flagged.
No data/traits/** changes, so no history/ record is expected.
The CLEAN constant carried an enables->TRAIT edge, so it was not clean; test_clean_graph_has_no_findings wrote a separate inline graph, produced an ENABLES_RANGE_ON_TRAIT finding, then filtered that very defect out and asserted the remainder was empty — a near-vacuous test whose non-flagging path was already covered elsewhere. Rewrite CLEAN to point enables at a process and use a non-domain predicate, so audit() genuinely returns []. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @realmarcin's task in 1m 14s —— View job Re-review: ratcheted predicate domain/range audit
Only one commit landed since the last approval, The fix is correct. Nothing else changed. Script, baseline, report and justfile wiring are byte-identical to what I verified last round: Notes 1 (the inherited range half of the No No 🔴 or 🟡. Approving. |
There was a problem hiding this comment.
Re-review of the delta since the last approval: the only new commit, 94a3d10, is exactly the fix for note 3 and it is correct. CLEAN now carries chem --RO:0002327--> proc (object is BIOLOGICAL_PROCESS, so the range check does not fire) and proc --METPO:3000000--> tr (outside the METPO:2000001 closure in the test OWL), so test_clean_graph_has_no_findings asserts audit() == [] with no filtering. The vacuous inline graph and the dead CLEAN constant are both gone, and the test now proves the non-flagging path it was named for.
Nothing else in the PR changed - the script, baseline, report and justfile wiring are byte-identical to what I verified last time (METPO:2000001 domain/range in data/raw/metpo.owl:159-161, no organism member in CausalNodeTypeEnum, all 94 subPropertyOf assertions in the rdf:resource form the walk expects, 366 + 164 = 530 rows reconciling with unique _keys, and partition unit-tested across all three --fail-on modes). qc, pytest, pr-sanity and vendored-sync are green on the new head.
Notes 1 (the inherited range half of the METPO:2000001 closure is unchecked) and 2 (_curie passes non-METPO IRIs through, so an OBO-IRI subproperty would silently under-report) still stand as optional follow-ups. Neither is live against the current ontology and both were declared out of scope. No blocking findings.
No data/traits/** changes, so no history/ record is expected.
Approving.
The detail column carries a non-ASCII '⊑' (⊑ microbe) on 366 rows, but the report write, baseline write, and baseline read all used the platform default (locale) encoding. On a non-UTF-8 locale that raises UnicodeEncodeError writing the report and UnicodeDecodeError reading the baseline — a latent crash that only passed CI because the runner is UTF-8. Pin encoding='utf-8' on all three; the on-disk bytes are unchanged (report and baseline are byte-identical after regeneration). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @realmarcin's task in 2m 32s —— View job Re-review: ratcheted predicate domain/range audit
One new commit since the last approval, Nothing else in the PR moved. The substance I verified in the earlier rounds still holds: No 🔴 or 🟡. One new 🔵 on this commit, plus the two still-open ones: 🔵 The fix stops one line short of the read that actually dominates. 🔵 (carried) The inherited range half of the 🔵 (carried) Both were declared out of scope in the PR body and neither bites today. No Approving. |
There was a problem hiding this comment.
Re-review of the delta since the last approval. The only new commit, 6531ba1, pins encoding='utf-8' on load_baseline (scripts/audit_predicate_domains.py:226), the report write (:251) and the baseline write (:269). The premise checks out — U+2291 is on all 366 MICROBE_DOMAIN_ON_NONORGANISM rows and all three handles were on the platform default. On-disk bytes are unchanged as claimed: report and baseline are still the same blob (fcb9727) and still byte-identical to each other, and qc (whose audit-derived-reports step diffs the committed report against a fresh generation) is green on this head, along with pytest, pr-sanity and vendored-sync.
Nothing else moved. The substance verified in earlier rounds still holds: METPO:2000001 domain/range at data/raw/metpo.owl:159-161, no organism member in CausalNodeTypeEnum, all 94 subPropertyOf assertions in the rdf:resource form the walk expects, 366 + 164 = 530 rows with unique _keys, and partition unit-tested across all three --fail-on modes including 'any' ignoring the baseline.
No blocking findings. One new optional note, detailed in the review comment: the fix stops one line short of the read that dominates. scripts/audit_predicate_domains.py:143 is yaml.safe_load(path.read_text()), still unpinned, and 357 trait YAMLs contain non-ASCII. audit() runs at :248, before the newly-pinned write at :251, so on the exact non-UTF-8 locale the commit message defends against the script dies here first — and uncaught, since the except at :144 catches yaml.YAMLError, not UnicodeDecodeError. requires-python is >=3.10, so UTF-8 mode is not on by default across the supported range. Not reachable on CI or any UTF-8 dev box, so a suggestion rather than a defect, but it is the same failure mode the commit set out to close. scripts/audit_causal_graphs.py:131 and its three TSV handles have the identical pattern, so a single sweep is a reasonable alternative to a one-liner here.
The two carried notes still stand as out-of-scope follow-ups: the inherited range half of the METPO:2000001 closure is unchecked (conf/predicate_domain_audit_baseline.tsv:2 is the example), and _curie at :98-101 passes non-METPO IRIs through so an OBO-IRI subproperty would silently under-report. Neither is live against the current ontology.
No data/traits/** changes, so no history/ record is expected.
Approving.
… zero (#327, closes #301) (#332) * Re-ground the last 3 substrate-to-product edges, and harden the gate (#327) RO:0001001 (derives into) holds between an old and a new material entity where the new begins to exist as the old ceases and inherits the significant portion of its matter -- substrate-to-product chemistry. biolink maps it as biolink:derives_into. That is exactly what these edges say, so #327 resolves with an upstream term and no minting at all: cellulose --is hydrolyzed to--> cellobiose METPO:2000013 -> RO:0001001 thiosulfate --derives into--> sulfate METPO:2007800 -> RO:0001001 thiosulfate --derives into--> sulfide METPO:2007800 -> RO:0001001 The first was the defect #327 was filed for: METPO:2000013 reads agent-to-substrate, so grounding a substrate-to-product edge to it asserted that cellulose hydrolyses cellobiose. Its label is kept, since it names the mechanism the general relation does not. The other two had a CHEMICAL subject on `produces`, and a chemical does not bring another into existence the way an agent does. METPO:2007405 (oxidized to, proposals v2) was considered for the sulfate half and rejected twice over: it fits only the oxidised half, so it would split a matched pair from one disproportionation reaction, and it is an unminted placeholder where RO:0001001 is real. acetyl_phosphate --produces--> ATP was examined and deliberately NOT moved: ATP's matter comes mostly from ADP, only the phosphoryl group transfers, so it fails RO:0001001's "inherits the significant portion of the matter" test. Filed separately rather than forced. MICROBE_DOMAIN_ON_NONORGANISM: 1 -> 0. #301's 366 edges are fully retired. So the check is now a HARD GATE. It shipped as a ratchet over 530 findings (#314) because nothing was fixable without the decisions that have since landed. `just audit-predicate-domains` now passes --fail-on any, and conf/predicate_domain_audit_baseline.tsv is deleted rather than kept empty -- an empty baseline invites a --write-baseline run to refill it. Canaried: injecting one microbe-domain edge exits 1 with nothing to forgive it. The ratchet machinery in the script is deliberately kept, since it is what makes a future violation class landable the same way this one was. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Ground the second is-hydrolyzed-to edge, and close the gate's back door (PR #332 review) Three review findings, all valid. The new `is hydrolyzed to` mapping row also matched starch_degradation.yaml's `starch --is hydrolyzed to--> maltooligosaccharides`, a CHEMICAL->CHEMICAL edge with no predicate_id. Adding the row dropped it from the residual report because the grounding tool now considered it mapped, but nothing had grounded it -- so it sat in neither the grounded set nor the backlog. Grounded to RO:0001001; it is the same substrate-to-product shape as the cellulose edge. justfile:105 named RO:0002234 (has output) as what the last edge was re-grounded to. It was RO:0001001 (derives into). The script had it right, the recipe comment did not. docs/CURATION_PLAYBOOK.md said "Nothing in `just qc` catches it" and "#301 tracks the 366 edges that still carry them". Both were true when written and are now false: the audit is a qc member running --fail-on any, and the count is 0. Rewritten to describe a mistake the tooling prevents rather than a backlog to work around, mirroring what #323 did for the enables paragraph. Also acted on the 🔵, which was a real hole rather than a nit: --write-baseline can still recreate the deleted baseline, and a non-recipe invocation defaulted to --fail-on new, so it would have loaded that file and passed. Deleting the baseline while leaving the default permissive only hardened the path that goes through `just`. The default is now `any`; the ratchet is still available via an explicit --fail-on new for reintroducing this check over a NEW violation class. That flip broke test_write_baseline_freezes_then_passes, which had encoded the old contract -- the test was right to fail. Updated to assert both halves of the new one, plus a new test_default_fail_on_is_any pinning that a stray baseline cannot weaken a default run, so the hardening cannot be undone by an argparse edit. Canaried both paths: injecting one microbe-domain edge exits 1 via the recipe AND via a direct no-flag invocation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
What
Adds
scripts/audit_predicate_domains.pyand wires anaudit-predicate-domainsrecipe into the
qcchain. It closes the enforcement gap behind #301/#302:validate-strictseespredicate_idas a bare string, so a CURIE whoseontological domain/range no causal-graph node type can satisfy is a false
type entailment that passes every existing gate.
Two defect classes, both derived from the corpus + the vendored ontologies:
rdfs:subPropertyOf METPO:2000001(whose domain is microbe) on a non-organismcausal subject.
CausalNodeTypeEnumhas no organism member, so every suchedge entails
subject ⊑ microbe. 366 edges.enablesedges point at TRAIT nodes, violating RO:0002327's range — and theenablesmapping row is ungated #302) —enables/RO:0002327pointed at aTRAITobject, when biolink gives its range as 'biological process or activity'. A
disposition is not a process. 164 edges.
The microbe-domain predicate set is computed at run time by walking the
subPropertyOfclosure toMETPO:2000001indata/raw/metpo.owl— not ahand-maintained list — so a new METPO subproperty is covered the moment it's
vendored.
Why a ratchet, not a blocking gate
Neither class is fixable without a per-family biological decision (#301) or an
ontology decision on the
enablesrange (#302, #303). A blocking check would beun-landable today. So this is the same ratchet as
audit-graphs:conf/predicate_domain_audit_baseline.tsvfreezes the 530 known findings; anynew violation fails CI. This makes the invisible 530-edge class visible and
non-growing while the biological/ontology decisions land family by family — burn
the baseline down, then
just audit-predicate-domains --fail-on any.What was checked
(164
enablesedges point at TRAIT nodes, violating RO:0002327's range — and theenablesmapping row is ungated #302) = 530, matching both issues' figures. No drift since filing.just qcgreen end to end, including the new recipe andOK reports/predicate_domain_audit.tsv (vs git)fromaudit-derived-reports(the report is registered there, mirroring
causal_graph_audit.tsv).uv run pytest→ 416 passed.trait file → audit exits 1,
blocking: 1. File restored.audit-derived-reportsdiffflags a hand-mutated committed report as STALE.
microbe_domain_predicatesfails safe (returns just the root, no crash) if theOWL is unreadable; covered by a test.
Scope / what this does NOT do
It does not change any trait data or the ontology, and does not resolve #303's
donor/acceptor role loss — that needs the #302 decision. This is the
infrastructure step chosen so the backlog is frozen and visible before the
per-family fixes.
🤖 Generated with Claude Code