Flag terms their ontology marks Not4Curation (#70) - #72
Conversation
Some ontologies keep terms for hierarchy completeness that they do not want used for annotation, and say so with a synonym (RGD's `Not4Curation`; OBO's `not_recommended_for_annotation`) rather than an obsoletion axiom. Such a term exists, has a matching label, and is reachable, so every check here passed it. XCO:0000294 (estrogen) reached three curation batches downstream before a human noticed. OntologyAccess gains entity_aliases() and find_not4curation_markers(): aliases are folded to lowercase alphanumerics and substring-matched against a configurable marker list. OLS reads the term payload the label lookup already fetched, so no extra round trip there. A term whose aliases could not be read (offline, no adapter, unsupported adapter, or an empty alias list) is tracked as *unchecked*, never treated as clean. All three plugins and EnumValidator run the check on every accepted value, so a positive enum-cache hit cannot hide the flag, and emit new ErrorModes (binding_not4curation, dynamic_enum_not4curation, permissible_value_not4curation) at ERROR by default; severity_overrides demotes them. check_not4curation / not4curation_markers are accepted by the plugin constructors, ValidationConfig, oak_config.yaml, and every CLI command as --check-not4curation/--no-check-not4curation. The CLI prints the unchecked CURIEs as a non-gating note. In the binding plugin the check only consults an adapter where the pipeline already would (dynamic enum, configured prefix, or offline), so a static-enum binding under an unconfigured prefix never triggers an ontology download. tests/data/test_ontology.obo gains TEST:0000008 (Not4Curation) and TEST:0000009 (not_recommended_for_annotation) for offline tests. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7nfDciLcyerPSeFwBWEH1
|
Claude finished @caufieldjh's task in 3m 52s —— View job Review: Flag terms their ontology marks Not4Curation
This is a well-shaped change. The design decisions that matter most are the right ones: the check runs on the accepted value (so a positive enum-cache hit can't hide the flag), "couldn't read synonyms" is tracked as unchecked rather than folded into a pass, the marker match is generic rather than a per-ontology allowlist, and
1. OLS: a label-only payload counts as clean, not unchecked (medium)
That is an asymmetry with the OAK path, where a thin/empty alias list is treated as unknown. If an OLS response ever omits or renames the synonym fields (OLS3 vs OLS4 shape differences, a client that drops the key, a Suggestion: require positive evidence that synonyms were actually present in the payload, e.g. return 2. Dynamic-enum path can force an ontology download that the cache previously avoided (performance)
The PR frames this as intentional ("no cache rebuild is needed to adopt it"), and it is the right correctness call. But the cost profile change is worth documenting in Related: aliases live only in 3. Duplicated markers in the message (low)An OLS payload carrying the same synonym under both 4. Accessors hand out live internal sets (low)
5. Config file beats the new CLI flag, silently (low)
6. Duplicated config parsing between plugin and validator (code quality)
7.
|
- OLS: a term payload with neither a `synonyms` nor an `obo_synonym` key is now reported as unchecked, not clean. The label alone made the alias list non-empty, so an unrecognized payload shape would have passed every term silently. - Dedupe aliases order-preservingly so a synonym listed under both OLS fields is named once in the message. - The unchecked/checked accessors return copies. - Precedence: an explicit constructor argument or CLI flag now wins over oak_config.yaml, which wins over the default. The CLI switch is tri-state (None = unset) so --no-check-not4curation is never silently ignored by a config file. - Both the plugin base and EnumValidator read the config keys through one shared parse_not4curation_config() helper. - The binding gate names the membership result type instead of suppressing the flag on any result. - Docs: precedence, the warm-cache cost, and the ERROR-only schema path. - Tests: real OLS payload shape, empty-vs-absent synonym fields, accessor copy, label-as-alias, flag-beats-config in constructor, config and CLI, invalid config values, and integration tests for the three XCO terms from the issue over sqlite:obo:xco and ols:xco. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7nfDciLcyerPSeFwBWEH1
|
Review addressed in b74f191. All eight findings and both test gaps.
Tests: |
|
Claude finished @caufieldjh's task in 3m 34s —— View job Review: Flag terms their ontology marks Not4Curation (re-review after
|
| # | Finding | Status |
|---|---|---|
| 1 | OLS label-only payload counted as clean | Fixed — oak_utils.py:893 returns None when neither synonyms nor obo_synonym is a key. Tests now pin the real OLS4 shape (synonyms list + obo_synonym: null), the explicitly-empty cases as clean, and the no-key case as unchecked. |
| 2 | Warm-cache cost change undocumented | Fixed — docs/caching.md and docs/plugin-reference.md both state the per-unique-term adapter read and point at --no-check-not4curation. |
| 3 | Duplicated markers in the message | Fixed — dict.fromkeys dedupe at oak_utils.py:950, test updated to expect one. |
| 4 | Accessors handed out live sets | Fixed — both return set(...) copies, with a test that mutating the snapshot doesn't move state. |
| 5 | Config file silently beat the CLI flag | Fixed — CLI options are Optional[bool] = None, _not4curation_explicit records what the constructor was actually given, and the file fills only unset keys. Precedence is per-key, so check_not4curation=True + file markers still uses the file's markers — good granularity, and tested both directions through the CLI. |
| 6 | Duplicated config parsing | Fixed — parse_not4curation_config() in utils, consumed by both BaseOntologyPlugin and EnumValidator, with doctests. _parse_bool_config_value correctly stays for the other keys. |
| 7 | EnumValidator hardcodes ERROR |
Documented — docs/plugin-reference.md now says the check is all-or-nothing behind validate-schema / validate-text-file. |
| 8 | Binding gate coupled to "any result" | Fixed — rejected = any(r.type == ErrorMode.BINDING_VALIDATION.value ...). I checked all four yields in _validate_against_enum (binding_plugin.py:537, 565, 596, plus the required-field one before it): all are binding_validation, so this is behaviorally identical today and now says what it means. |
Both test gaps I raised are closed too: @pytest.mark.integration tests against real XCO (sqlite + OLS), and test_label_counts_as_an_alias making the label-is-an-alias behavior explicit with a comment about custom markers.
Also verified while reading: the None → True resolution runs unconditionally in both BaseOntologyPlugin.__init__ (base.py:152) and EnumValidator._load_not4curation_config (which no longer early-returns on an empty config), so an absent oak_config.yaml can't leave the switch None/falsy. normalize_not4curation_markers([]) raises rather than silently matching nothing.
Remaining items (low)
a. The OLS integration test can fail for an unrelated reason. test_xco_flagged_term_via_ols (tests/test_not4curation.py:825) routes through a reachable_from closure with source_ontology: ols:xco. OAK's OLS adapter has thin graph support; if the closure comes back empty the binding yields binding_validation and the assertion fails on a membership problem, not the payload shape it exists to pin. Since integration tests are opt-in, that misdiagnosis could sit unnoticed. The purpose here is the OLS4 payload shape — asserting it directly (OntologyAccess(oak_adapter_string="ols:xco").find_not4curation_markers("XCO:0000294")) tests exactly that with no closure dependency. Fix this →
b. Two hard-coded XCO IDs are unverified. The PR body verified XCO:0000294, XCO:0000950 and XCO:0000000. The new integration tests add XCO:0000561 (asserted flagged) and XCO:0000004 (asserted a clean descendant of the root). Worth one runoak check before merge — a wrong guess here is a red integration suite that nobody sees until someone runs just test-full.
c. check_not4curation and cache_strategy now have opposite precedence. In the same oak_config.yaml, cache_strategy overrides the constructor while check_not4curation does not. That's the right call for a flag, but nothing in docs/configuration.md documents cache_strategy's precedence at all, so a reader has no way to know the two adjacent keys differ. One sentence next to the config example would close it.
d. A nonexistent term appears twice. In the binding path, _validate_not4curation runs before _validate_term_exists (binding_plugin.py:388 vs 400). A bogus CURIE under a configured prefix therefore lands in the unchecked set (no aliases) and then gets a term_not_found error — so the CLI's "could not be checked… run once online" note names a term that simply doesn't exist. Not a correctness bug (the run already fails), but the note reads better if term_not_found CURIEs are discarded from it.
e. Nits.
base.py:130—self._not4curation_explicit = (check_not4curation is not None, not4curation_markers is not None)is an opaque bool 2-tuple unpacked ~150 lines later, and at 103 chars is the longest line in the file (ruff's default rule set excludesE501, sojust formatwon't flag it). Two named attributes would read better at the use site.find_not4curation_markers's Returns clause (oak_utils.py:932-938) still lists only the old unchecked reasons; the new "OLS payload with no synonym field" case belongs there.test_unset_constructor_value_defaults_ontakes an unusedtmp_path.
Verdict: no blocking issue. The silent-pass path I flagged last round is closed, the precedence surprise is gone, and the degraded-run bookkeeping still holds. Items (a) and (b) are about integration tests that won't run in CI, so they're worth a few minutes now rather than a confusing failure later; (c)–(e) are polish.
· branch issue-70-not4curation
- The OLS integration test asserts the payload shape directly on OntologyAccess instead of through a reachable_from closure, so thin OLS graph support cannot turn a payload regression into a membership error. - A bound CURIE that does not resolve is left out of the Not4Curation check entirely: it is absent, term_not_found reports it, and it no longer lands in the "could not be checked" note. - docs/configuration.md states that cache_strategy (file overrides constructor) and check_not4curation (explicit wins over file) resolve in opposite directions, and why. - Nits: two named "explicit" attributes on the plugin base, the Returns clause of find_not4curation_markers lists the OLS no-synonym-field case, and an unused tmp_path is dropped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7nfDciLcyerPSeFwBWEH1
|
Second round addressed in 309469c.
|
|
Claude finished @caufieldjh's task in 3m 19s —— View job Review: Not4Curation check (third pass, after
|
| # | Finding | Status |
|---|---|---|
| a | OLS integration test routed the payload assertion through a reachable_from closure |
Fixed — test_xco_flagged_term_via_ols (tests/test_not4curation.py:836) now asserts on OntologyAccess directly, and adds the clean-root case. No closure dependency left. |
| b | XCO:0000561 / XCO:0000004 unverified |
Open — see below. |
| c | check_not4curation and cache_strategy have opposite precedence, undocumented |
Fixed — docs/configuration.md:31-39. I checked the claim against _load_oak_config_extras: cache_strategy / cache_enum_expansions / saturate_enum_caches assign unconditionally (file wins), the Not4Curation keys and severity_overrides merge per-key with the constructor on top. The doc is accurate, including the severity_overrides sentence. |
| d | A nonexistent CURIE appeared in the unchecked note | Fixed, with a caveat — see #1. |
| e | Nits (opaque bool 2-tuple, stale Returns clause, unused tmp_path) |
All fixed — _check_not4curation_explicit / _not4curation_markers_explicit read well at the use site (base.py:279-283), the Returns clause lists the OLS no-synonym-field case, tmp_path dropped. |
1. The new guard skips the check on any unresolved label, not just an absent term (low–medium)
binding_plugin.py:718-722 gates on self.get_ontology_label(field_value) is None. But OntologyAccess.get_label returns None for three different situations (oak_utils.py:632-656):
- the term is genuinely absent (adapter returns
None, OLS 404) — the case you meant to exclude; - an adapter error that isn't a service outage:
raise_if_service_unavailabledeclines,_log_lookup_failurelogs,label = None; - a term that resolves but carries no
rdfs:label.
In the default path (not strict, not offline) _validate_term_exists never runs, so in the latter two cases the Not4Curation check is now skipped with no result and no unchecked entry. That's the one property this feature is built around — "a degraded run does not look clean" — quietly not holding for the binding plugin. It's narrow (an adapter that fails at label() rather than at entity_aliases(); test_alias_lookup_failure_is_unchecked_not_crash covers the other order and still works), but it's the same class of silent pass as round-one finding #1.
My original suggestion was the subtractive form: let _validate_not4curation run as before and discard term_not_found CURIEs from the note afterwards. That keeps "couldn't read" and "isn't there" distinct, which the up-front guard collapses. If you prefer the guard, gate on the same predicate that makes _validate_term_exists emit term_not_found so the two can't disagree.
2. The test for the new guard never reaches it (low)
test_binding_nonexistent_term_is_not_found_not_unchecked (tests/test_not4curation.py:414) uses _validate_binding, whose schema binding_label_schema.yaml binds to BiologicalProcessEnum — a dynamic enum. TEST:0000999 therefore fails membership, _validate_against_enum yields binding_validation, and the rejected gate at binding_plugin.py:378 short-circuits before _validate_not4curation is ever called. Revert the guard and this test still passes.
The or in assert "term_not_found" in types or "binding_validation" in types reflects that: it's the binding_validation branch that fires, since _validate_term_exists is behind strict or offline and neither is set here. A test that actually exercises the guard needs a value that passes membership but doesn't resolve — a static enum with meaning: TEST:0000999 under the configured TEST prefix, or offline with the fake CURIE seeded into the enum cache.
3. No binding-plugin test asserts a non-empty unchecked set (low)
Every binding assertion on the bookkeeping (:322, :421, :824, :832) checks it's empty; the one positive case (:363) is the dynamic-enum plugin. So the binding path's degraded-run signal — now sitting behind the new guard — has no test holding it up. An offline binding run with a warm label cache (label cached, adapter refused → aliases None) is the natural case and would have caught #1 if the guard had been placed one line earlier.
4. XCO:0000561 and XCO:0000004 still unconfirmed (low)
The response to round two listed the fixes but didn't mention checking these two. They're asserted as flagged and clean-descendant-of-root respectively in @pytest.mark.integration tests that CI never runs, so a wrong ID is a red just test-full for whoever runs it next. One runoak -i sqlite:obo:xco info XCO:0000561 XCO:0000004 before merge settles it. (I tried OLS directly; outbound fetches need approval here.)
5. Nit: the guard costs a label lookup that wasn't happening
With validate_labels=False and non-strict, the binding path previously did no get_label call for an accepted value; it now does one per unique bound CURIE (memoized, and it writes to the label cache when cache_labels is on). Small next to the alias read it precedes, but it's a second adapter round trip for the same term when the alias source could have answered both questions — entity_aliases already returns the label as aliases[0] on the OAK path and reads it from the same OLS payload.
Verdict: no blocking bug. #1 and #2 are worth one more pass because together they mean a silent-skip path shipped with a test that would not notice its removal; #3–#5 are optional. Everything from the first two rounds that mattered is genuinely closed.
· branch issue-70-not4curation
…missing label The previous guard skipped the check whenever get_label returned None, which also covers a logged adapter failure and a term with no rdfs:label. In the lenient online path nothing else runs for those, so a degraded adapter looked clean. Now the existence check runs first and the marker check is skipped only when it actually emitted term_not_found. When existence is not checked (lenient, online) an unresolvable term is reported as unchecked, which is the truth. This also drops the extra label lookup the guard had added. Tests now reach the gate: a static-enum binding under the configured prefix with a bogus CURIE (strict: term_not_found only; lenient: unchecked), a flagged term through the same static enum, an adapter that fails at label() and entity_aliases() (lenient: unchecked; strict: term_not_found), and an offline binding run over warm label and enum caches that accepts the flagged term and lists it as unchecked. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7nfDciLcyerPSeFwBWEH1
|
Third round addressed in f00de7d. You were right on #1 and #2; the guard was wider than the problem and the test never reached it.
|
|
Claude finished @caufieldjh's task in 5m 58s —— View job Review: Not4Curation check (fourth pass, after
|
| # | Finding | Status |
|---|---|---|
| 1 | The guard skipped the check on any None label, not just an absent term |
Fixed — binding_plugin.py:374-387 runs _validate_term_exists first into exists_results; binding_plugin.py:399 gates on not_found = any(r.type == ErrorMode.TERM_NOT_FOUND.value ...). The get_ontology_label(...) is None guard is gone from _validate_not4curation. So a logged adapter failure, or a resolvable term with no rdfs:label, no longer disappears from the bookkeeping: in the lenient path it is recorded as unchecked, which is the honest answer, and in strict/offline it is an ERROR. The comment at :388-397 explains the two gates and the lenient case accurately. |
| 2 | The test for the guard never reached it | Fixed — _STATIC_BINDING_SCHEMA (tests/test_not4curation.py:414) binds through a static enum whose permissible values list both TEST:0000999 and TEST:0000008, under the configured TEST prefix. Membership therefore passes and only existence + the marker check decide. I checked that the tests now bite: test_binding_nonexistent_term_is_not_found_not_unchecked asserts exactly ["term_not_found"] and an empty unchecked set, so removing the not_found gate makes it fail on the second assertion. |
| 3 | No binding test asserted a non-empty unchecked set | Fixed — three now do: lenient fake CURIE (:474), the _LabelBoomAdapter case (:497), and the offline warm-cache twin (:526). The last one is the case I said would have caught #1. |
| 4 | XCO:0000561 / XCO:0000004 unverified |
Accepted — the round-two reply reports runoak-level detail (XCO:0000561 = antidepressant, aliases ['antidepressant', 'Not4Curation']; XCO:0000004 = walking on treadmill, XCO:0000000 among its ancestors). That's specific enough that I'll take it; I still can't reach OLS from here. |
| 5 | The guard added a label lookup | Fixed — dropped with the guard. In strict/offline the one get_label call now happens in _validate_term_exists, where it always did. |
I also traced test_binding_offline_warm_cache_accepts_but_reports_unchecked end to end, since nothing in greedy expansion (base.py:945-1046) or not4curation_markers_for writes the label cache. It works because BindingValidationPlugin.strict defaults to True: the online run's _validate_term_exists calls get_label, which save_to_caches TEST:0000008 into cache_dir, so the offline run finds it and emits no term_not_found. Worth knowing that the test's premise rests on that default rather than on anything explicit in the test.
Observations (all low)
1. The lenient "unresolvable → unchecked" path isn't in the docs. docs/plugin-reference.md:151-154 lists three reasons a term lands in the unchecked note (offline, no adapter, adapter without aliases). There's now a fourth: existence isn't being checked (strict=False, online) and the term doesn't resolve, so its aliases come back empty. That's the behavior test_binding_nonexistent_term_lenient_is_unchecked pins, and it's the one case where the note can name a CURIE that simply doesn't exist. One sentence would close the gap between the docs and binding_plugin.py:394-397. Fix this →
2. Result order changed. term_not_found now precedes binding_not4curation and the label results for the same value. I grepped the suite for order-sensitive assertions (results[0], [r.type for r in ...]) and found none that mix the two types, so this is safe in-tree — but a downstream consumer that reads results[0] (the dismech audit scripts this replaces, for instance) may see a different first result on a value that trips both.
3. Strict mode can report a flagged term as merely "not found." If an adapter fails at label() but would have answered entity_aliases(), strict mode emits term_not_found and the marker never surfaces. The run fails either way, so the outcome is right; only the diagnostic is less specific. Not worth code, and the alternative (running the marker check anyway) reintroduces the double-report you just removed.
4. exists_results is now materialized eagerly. list(...) instead of yield from at binding_plugin.py:375. Identical behavior — the LinkML validator drains the generator — and it's needed for the gate. Just noting the laziness loss is deliberate.
5. Nit: three tests now reach into plugin.ontology._adapter_cache (:162, :518, plus the OLS ones at :199-243). Consistent with the existing style, but a two-line _install_adapter(access, prefix, adapter) helper would make the intent obvious and give one place to change if the cache moves.
Verdict: ready to merge from my side. The silent-pass path is closed, the test that guards it now actually exercises the gate, and the binding path finally has positive coverage of its degraded-run signal. Item 1 is a docs sentence; 2–5 are informational. Since I can't run anything here, the only real precondition is your confirmation that just test (and ideally just pytest-integration for the five XCO cases) is green on f00de7d.
· branch issue-70-not4curation
1 participant
Closes #70.
Downstream prior art: monarch-initiative/dismech#8472 (the incident) and monarch-initiative/dismech#9097 (the stopgap
scripts/not4curation_audit.py, which this replaces upstream).What
Some ontologies keep terms for hierarchy completeness that they do not want used for annotation, and say so with a synonym, not a deprecation axiom. RGD's ontologies (XCO, CMO, MMO, RS) write
Not4Curation; some other OBO ontologies writenot_recommended_for_annotation. Such a term exists, has a matching label, is not obsolete, and is reachable, so every check here passed it.XCO:0000294(estrogen/estrogen analog) reached three dismech curation batches this way.This adds the check at the validator level, following the
is_obsolete()pattern.How
OntologyAccess.entity_aliases()andfind_not4curation_markers(): every alias is folded to lowercase alphanumerics and substring-matched against a marker list (defaultnot4curation,notforcuration,notrecommendedforannotation). Generic on purpose: no per-ontology allowlist. For OLS the synonyms come from the term payload the label lookup already fetched, so no extra round trip.ErrorModes (binding_not4curation,dynamic_enum_not4curation,permissible_value_not4curation) atERROR;severity_overridesdemotes them.check_not4curation/not4curation_markersare accepted by the plugin constructors,ValidationConfig,oak_config.yaml, and every CLI command (--check-not4curation/--no-check-not4curation).BindingValidationPlugin,DynamicEnumPlugin(both cache strategies),PermissibleValueMeaningPlugin, andEnumValidator(sovalidate-schemaandvalidate-text-filetoo). A positive enum-cache hit therefore cannot hide the flag; no cache rebuild is needed to adopt it.plugin.get_not4curation_unchecked()exposes it and the CLI prints a non-gating note listing the CURIEs.Verified against the real
sqlite:obo:xcodatabase:XCO:0000294andXCO:0000950are flagged in a binding,XCO:0000294as a permissible valuemeaning, andXCO:0000000passes.Tests
tests/data/test_ontology.obogainsTEST:0000008(Not4Curation) andTEST:0000009(not_recommended_for_annotation) underbiological_process.tests/test_not4curation.py(59 tests, offline) covers marker normalization, both conventions, the unchecked bookkeeping (offline, fake term, alias lookup failure, OLS payload), every plugin in both cache strategies, the cache-does-not-hide-it case, severity override and switch-off through constructor andoak_config.yaml, the out-of-scope static-enum case, and the CLI flag on all four commands. Full suite, doctests, mypy, ruff, and the OLS integration tests for obsolete/fake IDs pass.Docs
Reference section in
docs/plugin-reference.md(linked from the binding, data, schema, CLI, caching, and configuration pages), README feature line, and aCLAUDE.mdnote.🤖 Generated with Claude Code
https://claude.ai/code/session_01X7nfDciLcyerPSeFwBWEH1