fix: derive ontologies_stubs DATA_INPUTS from what the collector reads - #840
Merged
Merged
Conversation
#839) `OntologiesStubsTransform` declared one mapping file and read eleven, via `stub_curie_collection.DEFAULT_MAPPING_PATHS`. `kgm-freshness-check` consults `DATA_INPUTS` and nothing else, so a change to any of the other ten left the output reported fresh when it was not. That is the #812 blind spot, and a sweep of every transform declaring `DATA_INPUTS` shows this was the last one carrying it. It was not hypothetical. The currently-failing `test_every_referenced_curie_ has_stub_node` concerns `PO:0009005`, which enters through `madin_environment_id_corrections.tsv` — an undeclared input — so the checker attributed the staleness entirely to code. It also had a live consequence for the rebuild ordering. `DEFAULT_MAPPING_PATHS [0]` is the unified SSSOM, so `ontologies_stubs` is *not* independent of an incoming MIM release, which the one-file declaration implied. On the strength of that declaration I was about to call it safe to re-run ahead of the SSSOM. Derived from the constant rather than restated, so the two cannot drift again. Five tests. Verified by reverting to the old literal and re-running: three fail, which is what makes them worth having. That exercise also caught a defect in one of them — `test_the_declaration_is_derived_rather_than_restated` split on the bare string "DATA_INPUTS", which appears in the prose of the comment above the assignment, so it captured the comment's own mention of `DEFAULT_MAPPING_PATHS` and passed against a hardcoded literal. Anchored on the assignment instead. Two of the five guard the derivation's premise rather than its output: that no declared path is absolute or missing (either would read as "nothing changed"), and that `collect_stub_curies` does not glob for files outside the constant, which would under-declare again while every other test kept passing. Co-Authored-By: Claude Opus 5 <[email protected]>
This was referenced Aug 20, 2026
Closed
…841) **#841 — a config mistake presented as an ImportError.** The derived declaration runs in a class body, so `relative_to` raising on a path outside the repo would surface at import, breaking every importer and pointing the traceback at a tuple comprehension rather than the entry that caused it. Such a path carries no git signal anyway, so it is skipped rather than raised on. **#842 was my error, and is closed as invalid.** I reported `DATA_INPUTS` as undocumented after grepping from the declaration line and not reading the twelve comment lines above it. The contract is documented: purpose, the #812 motivation, repo-relative paths, and the git-tracked constraint with the #797 reasoning for why commit time is used instead of mtime. One thing was genuinely missing, and it is exactly what #839 violated: that the list must cover *every* file read rather than a representative one. A partial declaration is silent and looks identical to a complete one. Added, with the recommendation to derive from a constant where one exists. Co-Authored-By: Claude Opus 5 <[email protected]>
`test_declared_paths_are_repo_relative_and_resolvable` checked `is_file()`. Its own docstring said the point was to catch a declaration that "looks complete but is inert" — and existence is not the property that makes one live. The checker resolves each entry through `_latest_commit`, which asks git. An untracked-but-present file returns `(None, None)` and is skipped in silence, so the old assertion passed on a declaration contributing no staleness signal at all. That is #839 one layer down: complete on paper, partly inert in fact. Verified the distinction rather than assuming it: with an untracked file under `mappings/`, the old `is_file()` check passes and the new tracked check fails. All 11 currently declared files are tracked, so there is no live defect — this closes the hole before `DEFAULT_MAPPING_PATHS` gains a generated or gitignored entry. Kept the repo-relative assertion, which fails differently: `REPO / "/abs"` resolves to the absolute path, so an absolute entry can succeed while measuring a file outside the repo. Co-Authored-By: Claude Opus 5 <[email protected]>
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
1 participant
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.
Closes #839.
The gap
OntologiesStubsTransformdeclared one mapping file:and read eleven, via
stub_curie_collection.DEFAULT_MAPPING_PATHS.kgm-freshness-checkconsultsDATA_INPUTSand nothing else, so a change to any of the other ten left the output reported fresh when it wasn't.That's the #812 blind spot — the same defect class that let a merged KG silently ship without the #778/#786 corrections. A sweep of every transform declaring
DATA_INPUTSshows this was the last one still carrying it; bacdive, mediadive, madin_etal, metatraits, microbedecoder and ctd all declare what they read.It was already biting, twice
The currently-failing
test_every_referenced_curie_has_stub_nodeconcernsPO:0009005, which enters throughmadin_environment_id_corrections.tsv— an undeclared input. So the checker attributed that staleness entirely to code.More consequentially, it gave the wrong answer to "what can be re-run ahead of the incoming MIM SSSOM".
DEFAULT_MAPPING_PATHS[0]is the unified mapping set, soontologies_stubsis SSSOM-dependent. On the strength of the one-file declaration I was about to call it safe to re-run early; it isn't, and doing so would have meant re-running it again after the SSSOM lands.The fix
Derived from the constant rather than restated, so the two cannot drift.
Tests
Five, and I verified them by reverting to the old literal and re-running — three fail, which is what makes them worth having.
That exercise caught a defect in one of my own tests:
test_the_declaration_is_derived_rather_than_restatedsplit on the bare string"DATA_INPUTS", which appears in the prose of the comment above the assignment, so it captured the comment's own mention ofDEFAULT_MAPPING_PATHSand passed against a hardcoded literal. Now anchored on the assignment. Without the negative run it would have shipped as a test that could never fail.Two of the five guard the derivation's premise rather than its output:
collect_stub_curiesdoesn't glob for files outside the constant, which would under-declare again while every other test here kept passingpoetry run tox: 1134 passed. The one failure,test_every_referenced_curie_has_stub_node, reproduces on master unchanged — it needskg transform -s ontologies_stubs, which is on hold for the SSSOM (and this PR is part of why that ordering is now correct).🤖 Generated with Claude Code