diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 986416a..5488a65 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -882,6 +882,26 @@ XML remains available with the existing stale-cache warning and is not rewritten it may still lack table rows. A later process retries the refresh. Existing stale HTML rejection remains unchanged. +Entries that record *no* content carry their own stamp. A cache entry written +as `content_type: unavailable` is a claim about what an extractor could not +find, so an extractor fix can make it wrong -- and because such an entry carries +a current `extractor_version`, nothing would otherwise re-test it and the +missing text would stay missing permanently. Those entries now carry +`absent_content_version: 1`, and a missing or older stamp causes a one-time +refresh on the next validation fetch. Entries that do have content are +unaffected, so the refresh is confined to the entries that could be wrong +rather than falling on the whole cache. Version 1 is reading `OtherAbstract`, +where PubMed keeps abstracts contributed by other indexing programs (`PIP`, +`KIE`, `NASA`, `AIDS`, mostly on pre-1990 records); before it, such a record was +stored as having no content at all and a refresh deleted the abstract the cache +already held (issue #88). + +One gap is worth knowing if you set `source_extra_fields` for `PMID`: a record +with no abstract is then stored as `summary` carrying the extra-fields blob +rather than as `unavailable`, so it falls outside this stamp and is not +re-tested. Clear such entries by hand if you were running that setting before +this version. + **A cache-wide refresh adds one line to every enriched entry.** Both index providers now set `access_type` where they previously left it unset, so a refreshed public entry gains a `full_text_access_type: open` line. It means the diff --git a/src/linkml_reference_validator/etl/reference_fetcher.py b/src/linkml_reference_validator/etl/reference_fetcher.py index 4b7a3d6..0e90f19 100644 --- a/src/linkml_reference_validator/etl/reference_fetcher.py +++ b/src/linkml_reference_validator/etl/reference_fetcher.py @@ -93,6 +93,26 @@ #: XML table extraction changes only XML caches, independent of HTML acceptance. XML_EXTRACTION_CACHE_VERSION = 1 +#: Version the claim "this reference has no content at all", scoped to +#: ``content_type: unavailable``. Such an entry records what an extractor could +#: not find, so an extractor fix can make it wrong -- and because the entry +#: carries a current ``extractor_version`` it would otherwise never be re-tested +#: and the text would stay lost. Version 1: reading ``OtherAbstract``, where +#: PubMed keeps PIP/KIE/NASA/AIDS abstracts; before it, such a record was stored +#: as having no content and a refresh deleted the abstract already cached for it +#: (issue #88). Scoped rather than bumping EXTRACTOR_CACHE_VERSION because an +#: entry that records content cannot be wrong in this way, and a blanket bump +#: would re-fetch every cached reference to find the ones that can be. +#: +#: The converse does not hold, and the stamp is deliberately general rather than +#: targeted: `unavailable` is emitted by doi, url, clinicaltrials, json_api, +#: entrez and ppr as well as pmid, and the OtherAbstract fix cannot help any of +#: those. In the dismech cache 1,499 of the 1,735 such entries are DOI, so most +#: of this refresh is cost rather than repair. That is accepted -- the stamp +#: means "this no-content claim was made by extractor version N", which a later +#: fix to any source will want, and it is still a 3% refresh against 100%. +ABSENT_CONTENT_CACHE_VERSION = 1 + #: A cache file's frontmatter delimiter: a line that is exactly ``---``. #: Splitting on the bare string instead lets any *value* containing ``---`` - a #: URL reference_id, a title - truncate the block, which loses every field after @@ -1307,6 +1327,25 @@ def _save_to_disk( xml_version = (reference.metadata or {}).get("xml_extraction_version") if reference.content_type == "full_text_xml" and type(xml_version) is int: lines.append(f"xml_extraction_version: {xml_version}") + # Written from the constant, unlike the HTML and XML stamps, which come + # from `reference.metadata` so a metadata-only rewrite preserves the + # original. The invariant that makes that safe: nothing reaches a save + # path holding an `unavailable` entry it did not just produce. + # + # `_load_from_disk` filters stale entries out, so an `unavailable` entry + # that survives a cache hit already carries the current stamp and + # re-writing it is a no-op -- and at version 2 a stamp-1 entry is stale, + # so the cache-hit branch is never taken for it at all. + # + # The two paths that *do* hold an untouched cached entry are + # `_stale_fallback` and `_preserve_cached_full_text`, and both are safe + # only because they document that the entry is deliberately not + # re-saved. Those are the contracts a future change would have to + # violate: re-saving an untouched `unavailable` entry would certify it as + # re-tested when it was not, recreating #88 one version up. Move this to + # metadata if such a path is ever added. + if reference.content_type == "unavailable": + lines.append(f"absent_content_version: {ABSENT_CONTENT_CACHE_VERSION}") if reference.title: lines.append(f"title: {self._quote_yaml_value(reference.title)}") if reference.authors: @@ -1639,6 +1678,22 @@ def _is_stale_cache_entry(cls, content_text: str) -> bool: if type(xml_version) is not int or xml_version < XML_EXTRACTION_CACHE_VERSION: return True + # Scoped to `unavailable`, which leaves one gap: with + # `source_extra_fields["PMID"]` configured, a record with no abstract is + # stored as `summary` carrying the extra-fields blob, so an + # OtherAbstract-only record damaged by #88 lands outside this rule and is + # never re-tested. Widening to `summary` is not worth it -- most summary + # entries have nothing to do with a missing abstract, so it would cost a + # re-fetch of all of them. A deployment using that setting should clear + # its affected entries by hand. + if isinstance(metadata, dict) and metadata.get("content_type") == "unavailable": + absent_version = metadata.get("absent_content_version") + if ( + type(absent_version) is not int + or absent_version < ABSENT_CONTENT_CACHE_VERSION + ): + return True + # A newer stamp is not stale: an older tool reading a cache written by a # newer one should leave it alone rather than re-fetch it on every run. return False diff --git a/src/linkml_reference_validator/etl/sources/pmid.py b/src/linkml_reference_validator/etl/sources/pmid.py index 99a2177..a38401c 100644 --- a/src/linkml_reference_validator/etl/sources/pmid.py +++ b/src/linkml_reference_validator/etl/sources/pmid.py @@ -204,13 +204,54 @@ def _parse_authors(self, author_list: list) -> list[str]: """ return [str(author) for author in author_list if author] + @staticmethod + def _render_abstract_sections(element: Any) -> Optional[str]: + """Join an abstract element's ``AbstractText`` nodes into prose. + + Structured abstracts label each section (e.g. ``Label="METHODS"``); the + label is preserved as a ``"METHODS:"`` prefix and sections are joined by + blank lines, mirroring how PubMed renders them as text. + + Args: + element: An ``Abstract`` or ``OtherAbstract`` node + + Returns: + The joined prose, or None if the element carries no text + + Examples: + >>> from bs4 import BeautifulSoup + >>> xml = ''' + ... We ran a trial. + ... It worked. + ... ''' + >>> element = BeautifulSoup(xml, "xml").find("Abstract") + >>> PMIDSource._render_abstract_sections(element) + 'METHODS: We ran a trial.\\n\\nRESULTS: It worked.' + """ + sections = [] + for node in element.find_all("AbstractText"): + text = node.get_text().strip() + if not text: + continue + label = node.get("Label") + sections.append(f"{label}: {text}" if label else text) + + joined = "\n\n".join(sections) + return joined if joined else None + def _parse_abstract(self, soup: BeautifulSoup) -> Optional[str]: """Parse the abstract from a PubMed article XML document. - Reconstructs the abstract prose from ``Abstract/AbstractText`` nodes. - Structured abstracts label each section (e.g. ``Label="METHODS"``); - the label is preserved as a ``"METHODS:"`` prefix and sections are - joined by blank lines, mirroring how PubMed renders them as text. + Reads ``Abstract`` when the record has one. Otherwise falls back to + ``OtherAbstract``, which is where PubMed keeps abstracts contributed by + other indexing programs -- ``PIP``, ``KIE``, ``NASA``, ``AIDS`` -- mostly + on pre-1990 records. Those are genuine abstracts, and reading only + ``Abstract`` reports no content at all for such a record, which on a + refresh deletes the text a cache entry already held (issue #88). + + ``OtherAbstract`` also carries translations, so an English one is + preferred; a translated abstract is still used when it is the only text + available, because reporting nothing would blank the entry. Args: soup: Parsed PubMed article XML @@ -229,22 +270,43 @@ def _parse_abstract(self, soup: BeautifulSoup) -> Optional[str]: >>> xml = 'A summary.' >>> PMIDSource()._parse_abstract(BeautifulSoup(xml, "xml")) 'A summary.' + >>> xml = ''' + ... An indexer's summary. + ... ''' + >>> PMIDSource()._parse_abstract(BeautifulSoup(xml, "xml")) + "An indexer's summary." """ abstract = soup.find("Abstract") + if abstract: + rendered = self._render_abstract_sections(abstract) + if rendered: + return rendered - if not abstract: + others = soup.find_all("OtherAbstract") + if not others: return None - sections = [] - for node in abstract.find_all("AbstractText"): - text = node.get_text().strip() - if not text: - continue - label = node.get("Label") - sections.append(f"{label}: {text}" if label else text) + # Prefer English; OtherAbstract is also where translations live, and a + # French rendering would stop an English snippet matching its source. + # `Language` is #IMPLIED with an `eng` default in the PubMed DTD, so an + # attribute-less node is English rather than an unknown translation. + def _is_english(node: Any) -> bool: + language = node.get("Language") + if isinstance(language, list): # bs4 may split a multi-valued attr + language = language[0] if language else None + return str(language or "eng").strip().lower() in ("eng", "en") + + english: list[Any] = [] + rest: list[Any] = [] + for node in others: + (english if _is_english(node) else rest).append(node) + + for node in english + rest: + rendered = self._render_abstract_sections(node) + if rendered: + return rendered - joined = "\n\n".join(sections) - return joined if joined else None + return None def _read_entrez( self, diff --git a/tests/test_absent_abstract_recovery.py b/tests/test_absent_abstract_recovery.py new file mode 100644 index 0000000..91a4eb2 --- /dev/null +++ b/tests/test_absent_abstract_recovery.py @@ -0,0 +1,140 @@ +"""An entry that claims no content must be re-testable after the extractor improves. + +Issue #88: the extractor read only ``Abstract`` and ignored ``OtherAbstract``, +so a record whose abstract PubMed keeps under ``OtherAbstract`` was written as +``content_type: unavailable`` -- deleting, on refresh, the abstract the cache +already held. + +Fixing the extractor does not by itself recover those entries. They carry the +current ``extractor_version``, so ``_is_stale_cache_entry`` calls them fresh and +they are never re-fetched: the text stays deleted permanently. They need their +own staleness stamp, scoped the way ``html_full_text_version`` and +``xml_extraction_version`` already are, so the refresh cost falls on the +entries that could be wrong rather than on the whole cache. +""" + +from __future__ import annotations + +from linkml_reference_validator.etl.reference_fetcher import ( + ABSENT_CONTENT_CACHE_VERSION, + EXTRACTOR_CACHE_VERSION, + HTML_FULL_TEXT_CACHE_VERSION, + XML_EXTRACTION_CACHE_VERSION, + ReferenceFetcher, +) + + +def _entry(content_type: str, *, absent_version: int | None = None) -> str: + """A cache entry that is current in every respect except what a test varies. + + The per-content-type stamps are filled in so a test of the absent-content + rule is not accidentally measuring the pre-existing HTML or XML rules. + """ + lines = [ + "---", + "reference_id: PMID:5697815", + f"extractor_version: {EXTRACTOR_CACHE_VERSION}", + ] + if absent_version is not None: + lines.append(f"absent_content_version: {absent_version}") + if content_type == "full_text_html": + lines.append(f"html_full_text_version: {HTML_FULL_TEXT_CACHE_VERSION}") + if content_type == "full_text_xml": + lines.append(f"xml_extraction_version: {XML_EXTRACTION_CACHE_VERSION}") + lines += [f"content_type: {content_type}", "---", "", "## Content", ""] + return "\n".join(lines) + + +def test_unavailable_entry_without_the_stamp_is_stale(): + """This is the entry #88 damaged: current extractor stamp, but no content.""" + assert ReferenceFetcher._is_stale_cache_entry(_entry("unavailable")) + + +def test_unavailable_entry_with_the_current_stamp_is_fresh(): + """Once re-tested by a fixed extractor it must stop being re-fetched.""" + assert not ReferenceFetcher._is_stale_cache_entry( + _entry("unavailable", absent_version=ABSENT_CONTENT_CACHE_VERSION) + ) + + +def test_a_newer_stamp_is_not_stale(): + """An older tool reading a newer cache leaves it alone, as elsewhere.""" + assert not ReferenceFetcher._is_stale_cache_entry( + _entry("unavailable", absent_version=ABSENT_CONTENT_CACHE_VERSION + 1) + ) + + +def test_entries_that_do_have_content_are_untouched(): + """The stamp is scoped to ``unavailable`` so the blast radius stays small. + + A blanket ``EXTRACTOR_CACHE_VERSION`` bump would invalidate every cached + reference -- 57,551 files in the dismech cache against the 1,735 that claim + no content -- for a bug that can only have produced the latter. + """ + for content_type in ( + "abstract_only", + "full_text_xml", + "full_text_html", + "full_text_pdf", + "summary", + "structured_record", + ): + assert not ReferenceFetcher._is_stale_cache_entry(_entry(content_type)), ( + f"{content_type} should not be invalidated by the absent-content stamp" + ) + + +def test_a_freshly_written_absent_entry_is_not_immediately_stale(tmp_path): + """The emitter and the staleness rule must agree, or the fix does nothing. + + Without the emitted stamp every ``unavailable`` entry would be re-fetched on + every run forever; without the staleness rule the already-damaged ones would + never be re-fetched at all. This is the round trip. + """ + from linkml_reference_validator.models import ( + ReferenceContent, + ReferenceValidationConfig, + ) + + fetcher = ReferenceFetcher(ReferenceValidationConfig(cache_dir=tmp_path)) + fetcher._save_to_disk( + ReferenceContent( + reference_id="PMID:4869291", + content_type="unavailable", + title="A record with no abstract anywhere", + content="", + ) + ) + + written = next(tmp_path.glob("*.md")).read_text() + assert f"absent_content_version: {ABSENT_CONTENT_CACHE_VERSION}" in written + assert not ReferenceFetcher._is_stale_cache_entry(written) + + +def test_an_entry_with_content_is_not_stamped(tmp_path): + """The emitter's scoping needs its own test; the staleness rule's is separate. + + ``test_entries_that_do_have_content_are_untouched`` exercises + ``_is_stale_cache_entry``, so deleting the ``content_type == "unavailable"`` + guard in ``_save_to_disk`` leaves the whole suite green. What escapes is not + a wrong answer but cache-wide churn: an unconditional stamp adds a line to + every entry on refresh, the same diff-on-every-file problem + ``troubleshooting.md`` already calls out for ``full_text_access_type``. + """ + from linkml_reference_validator.models import ( + ReferenceContent, + ReferenceValidationConfig, + ) + + fetcher = ReferenceFetcher(ReferenceValidationConfig(cache_dir=tmp_path)) + fetcher._save_to_disk( + ReferenceContent( + reference_id="PMID:38463381", + content_type="abstract_only", + title="A record that does have an abstract", + content="Real abstract text.", + ) + ) + + written = next(tmp_path.glob("*.md")).read_text() + assert "absent_content_version" not in written diff --git a/tests/test_sources.py b/tests/test_sources.py index a4d886e..012b70a 100644 --- a/tests/test_sources.py +++ b/tests/test_sources.py @@ -420,6 +420,156 @@ def test_parse_abstract_absent(self, source): assert source._parse_abstract(soup) is None + def test_parse_abstract_falls_back_to_other_abstract(self, source): + """A record whose only abstract is in ``OtherAbstract`` must still be read. + + PubMed keeps abstracts contributed by other indexing programs (PIP, KIE, + NASA, AIDS) in ``OtherAbstract`` rather than ``Abstract``. Most are + pre-1990 records. Reading only ``Abstract`` reports no content for them, + which on a refresh *deletes* the abstract a cache entry already held + (issue #88). + """ + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + '' + "Glucose absorption raises sodium uptake." + "", + "xml", + ) + + assert source._parse_abstract(soup) == "Glucose absorption raises sodium uptake." + + def test_parse_abstract_prefers_abstract_over_other_abstract(self, source): + """``Abstract`` wins when both are present; the fallback is a fallback.""" + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + "" + "The real abstract." + '' + "An indexer's summary." + "" + "", + "xml", + ) + + assert source._parse_abstract(soup) == "The real abstract." + + def test_parse_abstract_prefers_english_other_abstract(self, source): + """``OtherAbstract`` also holds translations; snippets are matched in English. + + Picking the first element would return the French here, so an English + snippet would stop matching its own source. + """ + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + "" + '' + "Un resume en francais." + "" + '' + "An English summary." + "" + "", + "xml", + ) + + assert source._parse_abstract(soup) == "An English summary." + + def test_parse_abstract_takes_non_english_other_abstract_over_nothing(self, source): + """With no English option, a translated abstract still beats no content. + + Reporting nothing would blank the cache entry; the text is at least the + paper's own summary, and content_type still says abstract_only. + """ + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + '' + "Un resume en francais." + "", + "xml", + ) + + assert source._parse_abstract(soup) == "Un resume en francais." + + def test_parse_abstract_keeps_labels_in_other_abstract(self, source): + """A structured ``OtherAbstract`` keeps its section labels like any other.""" + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + '' + 'We ran a trial.' + 'It worked.' + "", + "xml", + ) + + assert source._parse_abstract(soup) == ( + "METHODS: We ran a trial.\n\nRESULTS: It worked." + ) + + def test_parse_abstract_falls_through_an_empty_abstract_element(self, source): + """A present-but-empty ``Abstract`` must not short-circuit the fallback. + + This is a distinct branch from "no ``Abstract`` at all", and the one that + regresses if the guard is ever rewritten back to ``if not abstract:`` + returning early on element presence alone. + """ + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + "" + "" + '' + "The only real text here." + "" + "", + "xml", + ) + + assert source._parse_abstract(soup) == "The only real text here." + + def test_parse_abstract_treats_a_language_less_other_abstract_as_english( + self, source + ): + """``Language`` is ``#IMPLIED`` with an ``eng`` default in the PubMed DTD. + + Real PIP records often omit it, so the ``or "eng"`` default is + load-bearing: without it an attribute-less node would be treated as an + unknown translation and deprioritised. + """ + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + "" + '' + "No language attribute at all." + "" + '' + "Un resume en francais." + "" + "", + "xml", + ) + + assert source._parse_abstract(soup) == "No language attribute at all." + + def test_parse_abstract_skips_empty_other_abstract(self, source): + """An empty ``OtherAbstract`` yields None rather than an empty string.""" + from bs4 import BeautifulSoup + + soup = BeautifulSoup( + '' + "" + "", + "xml", + ) + + assert source._parse_abstract(soup) is None + def test_parse_abstract_empty(self, source): """An Abstract with only empty AbstractText should yield None.""" from bs4 import BeautifulSoup