feat: add PubMed Central (PMC) spider for open-access articles#121
feat: add PubMed Central (PMC) spider for open-access articles#121vedant1711 wants to merge 2 commits into
Conversation
Add a `pmc` spider that collects open-access articles from PubMed Central using NCBI's public services: - esearch.fcgi finds PMC UIDs for each search term, restricted to the open-access subset and UW affiliation, with retstart/retmax pagination. - esummary.fcgi provides article metadata (title, authors, journal, DOI/PMID, publication date). - The PMC Open Access Subset on AWS (the pmc-oa-opendata S3 bucket) is listed per article to locate and download the full-text PDF over HTTPS. The web /pdf/ endpoint is avoided (it serves a bot-verification interstitial instead of the file) as is the legacy oa.fcgi service (its ftp:// links no longer resolve); the AWS Open Data bucket serves the same OA content reliably over HTTPS. Author names are reformatted from NCBI's "Surname Initials" form so ParsedAuthor parses them correctly. Records without a PDF in the OA Subset still yield metadata. Includes unit tests covering request building, esearch pagination, esummary parsing, and S3 PDF-key selection (version + neighbouring-id handling). Verified end-to-end against the live services: PDFs download as valid files. Co-Authored-By: Claude Opus 4.8 <[email protected]>
boyechko
left a comment
boyechko
left a comment
There was a problem hiding this comment.
The spider and the tests look great overall!
| @staticmethod | ||
| def _format_author_name(raw: str) -> str: | ||
| """Convert an NCBI ``"Surname Initials"`` name to ``"Surname, Initials"``. | ||
|
|
||
| NCBI returns author names as ``"Habell-Pallán M"`` or ``"Smith JA"``. | ||
| Reformatting to ``Last, First`` lets :class:`ParsedAuthor` parse the | ||
| surname and initials correctly. | ||
| """ | ||
| raw = raw.strip() | ||
| surname, _, initials = raw.rpartition(" ") | ||
| if surname and initials: | ||
| return f"{surname}, {initials}" | ||
| return raw | ||
|
|
||
| @classmethod | ||
| def _extract_authors(cls, record: dict[str, Any]) -> str | None: | ||
| """Encode the author list from an esummary record.""" | ||
| cleaned: list[ParsedAuthor] = [] | ||
| for entry in record.get("authors") or []: | ||
| if not isinstance(entry, dict): | ||
| continue | ||
| name = (entry.get("name") or "").strip() | ||
| if name: | ||
| cleaned.append(ParsedAuthor(cls._format_author_name(name))) | ||
|
|
||
| return ParsedAuthor.encode_author_string(cleaned) if cleaned else None |
There was a problem hiding this comment.
_format_author_name corrupts consortium / collective authors, and it can't be fixed here alone.
NCBI marks organizational authors with authtype: "CollectiveName" and gives them a multi-word name. Example: PMC11137454 returns "GeKeR Study Group". Run through rpartition(" ") it becomes "GeKeR Study, Group", and ParsedAuthor then drops "Group" — the encoded string ends up as just "GeKeR Study".
Passing the collective name through verbatim doesn't help either: ParsedAuthor.canonical_name always re-runs HumanName, which treats the last token as a surname ("GeKeR Study Group" → "Group, GeKeR Study"). And even if the spider stored the raw name, authorship_pipeline re-parses item.authors with ParsedAuthor.parse_author_string(...) and re-mangles it before it hits the DB. ParsedAuthor is HumanName, which has no concept of an organizational author — so this can't be fixed at the spider level.
Suggested scoped fix for this PR: filter CollectiveName entries out of the author string so we stop shipping garbled person-rows, and (optionally) keep the data in extra:
for entry in record.get("authors") or []:
if not isinstance(entry, dict):
continue
name = (entry.get("name") or "").strip()
if not name:
continue
if entry.get("authtype") == "CollectiveName":
extra.setdefault("collective_authors", []).append(name) # not a HumanName
continue
cleaned.append(ParsedAuthor(cls._format_author_name(name)))One caveat on placement: this references extra in the suggested snippet, but _extract_authors doesn't currently have extra in scope. So if we go with the "stash in extra" approach, the collective-name collection needs to happen where both the author list and extra are assembled (i.e. in _parse_record, or by having _extract_authors return the collective names alongside the encoded string). Or maybe we should just skip CollectiveNames, since they're not important for our purposes? It feels wrong to silently drop bibliographic data, though 😬.
Representing consortia as first-class authors properly (an organizational mode on ParsedAuthor that bypasses HumanName, surviving the encode/decode round-trip) is a cross-cutting change that also affects other spiders, so I'll add a broader issue for that. Worth a test with a CollectiveName entry either way.
There was a problem hiding this comment.
Good catch — confirmed against PMC11137454 (GeKeR Study Group, authtype: CollectiveName): it was encoding to GeKeR Study and re-parsing to Study, GeKeR.
Went with the scoped fix you suggested, keeping the data rather than dropping it: _extract_authors now skips CollectiveName entries, and a new _extract_collective_authors stashes them verbatim under extra["collective_authors"]. The collection happens in _build_extra so extra is in scope. First-class consortium modelling stays with #125. (e8ac7f0)
| ) | ||
| return None | ||
|
|
||
| # ``file_urls`` is populated later from the OA Web Service (see parse_oa); |
There was a problem hiding this comment.
Stale reference? There is no parse_oa method (it's parse_oa_listing), and the "OA Web Service" (oa.fcgi) is the route the PR explicitly rejected in favor of the S3 bucket. The comment misdirects a future maintainer to a non-existent method and the wrong mechanism.
There was a problem hiding this comment.
Fixed — the comment now points at parse_oa_listing / the OA Subset bucket instead of the removed parse_oa/oa.fcgi route. (e8ac7f0)
| def test_esummary_request_batches_uids(self, spider: PMCSpider) -> None: | ||
| request = spider._build_esummary_request("term", ["111", "222", "333"]) | ||
| parsed = urlparse(request.url) | ||
| query = parse_qs(parsed.query) | ||
|
|
||
| assert parsed.path.endswith("/esummary.fcgi") | ||
| assert query["id"] == ["111,222,333"] | ||
| assert request.callback == spider.parse_summary | ||
| assert request.meta == {"search_term": "term"} |
There was a problem hiding this comment.
The test exercises a spot with no logic. It calls _build_esummary_request directly with a hand-built uid list, so it only verifies the comma-join in that helper, which can't realistically break. The actual batching decision ("collect all UIDs from a page into one request") lives in parse, and this test never exercises it: change parse to for uid in uids: yield self._build_esummary_request(term, [uid]) and this stays green while the crawl fires one request per UID and blows the rate limit.
Driving parse instead covers the behavior the name claims:
def test_one_esummary_request_per_esearch_page(self, spider: PMCSpider) -> None:
esearch = _json_response(
"https://eutils.ncbi.nlm.nih.gov/entrez/eutils/esearch.fcgi",
{"esearchresult": {"count": "3", "idlist": ["111", "222", "333"]}},
meta={"search_term": "term", "retstart": 0},
)
requests = list(spider.parse(esearch))
assert parse_qs(urlparse(requests[0].url).query)["id"] == ["111,222,333"]There was a problem hiding this comment.
Agreed — replaced it with test_one_esummary_request_per_esearch_page, which drives parse and asserts a single esummary request per page with the UIDs joined. The for uid in uids: yield self._build_esummary_request(term, [uid]) regression you described would now fail it. (e8ac7f0)
| def test_format_author_name(self) -> None: | ||
| assert PMCSpider._format_author_name("Smith JA") == "Smith, JA" | ||
| assert PMCSpider._format_author_name("van der Berg J") == "van der Berg, J" | ||
| assert PMCSpider._format_author_name("Madonna") == "Madonna" | ||
|
|
||
| def test_extract_authors_empty(self) -> None: | ||
| assert PMCSpider._extract_authors({"authors": []}) is None | ||
| assert PMCSpider._extract_authors({}) is None |
There was a problem hiding this comment.
No test covers a CollectiveName author, which is exactly the bug in #125. The author tests here only exercise personal names (test_format_author_name, test_extract_authors_empty), so the consortium-mangling defect ships with a green suite: a real record like PMC11137454 ("GeKeR Study Group") currently comes out as "Group, GeKeR Study" and nothing catches it.
The highest-value addition is a black-box test at the item.authors level (not _format_author_name, which just enshrines the personal-name path) that feeds a CollectiveName entry and asserts the org name survives intact:
@pytest.mark.xfail(reason="#125")
def test_collective_authors_are_preserved(self, spider: PMCSpider) -> None:
record = {
**SAMPLE_RECORD,
"authors": [
{"name": "Smith JA", "authtype": "Author"},
{"name": "GeKeR Study Group", "authtype": "CollectiveName"},
],
}
item = self._single_item(spider, record, "9876543")
assert "GeKeR Study Group" in item.authorsThis would fail today, but that's the point: it documents the gap now and becomes the acceptance test that must go green with the #125 fix.
There was a problem hiding this comment.
Added both: test_collective_authors_routed_to_extra (passing — asserts the org name stays out of item.authors and lands in extra["collective_authors"]), and your suggested test_collective_authors_are_preserved as an xfail(reason="#125") acceptance test for the full fix. (e8ac7f0)
Review feedback on PR #121: - Route CollectiveName authors (e.g. PMC11137454 "GeKeR Study Group") out of the personal-author string, where HumanName mangled them into bogus person rows, and preserve them under extra["collective_authors"]. Proper first-class consortium authors remain tracked in #125. - Fix a stale docstring pointing at a removed parse_oa/oa.fcgi route; point to parse_oa_listing / the OA Subset bucket instead. - Replace the low-value _build_esummary_request unit test with one that drives parse, guarding the "one esummary request per page" behavior. - Add a CollectiveName test for the scoped fix, plus an xfail acceptance test for the full #125 fix.
2 participants
Summary
Adds a
pmcspider that collects open-access articles (metadata + full-text PDFs) from PubMed Central using NCBI's public services. It follows the existingTermSearchSpiderpattern (same family as the OSTI spider).Crawl flow
"<term>"[Affiliation] AND open access[filter]), paginated viaretstart/retmax.pmc-oa-opendataS3 bucket) — listed per article to locate and download the full-text PDF over HTTPS.Why the AWS OA Subset bucket for downloads
Two more obvious NCBI routes were tried and rejected because they don't actually work for programmatic download:
…/articles/PMC<id>/pdf/endpoint is served behind a bot-verification interstitial (returns HTML, not the PDF).oa.fcgiOA Web Service only returnsftp://links that no longer resolve (550 file-not-found).NCBI's PMC Open Access Subset on AWS serves the same OA content reliably over HTTPS. Each article lives under
PMC<id>.<version>/with the main PDF atPMC<id>.<version>.pdf; the version (not exposed by esummary) is discovered with a single S3 list request.Notes
"Surname Initials"form soParsedAuthorparses surname/initials correctly.NCBI_API_KEYenv var is included when present (raises the rate limit from 3→10 req/s);DOWNLOAD_DELAY=1keeps the unauthenticated crawl within limits.