Keep approximate months in pub_month (issue #14) - #15
Merged
Merged
Conversation
The DocumentMetadataAPI spec contradicts itself: its prose asks for "capitalized three-letter abbreviations", but its own worked example for PMID:8000234 emits `"pub_month": "Sep-Dec"`. We were following the prose and shipping `""` for every season or range (issue #14). Follow the example instead. `month_to_abbrev` becomes `normalize_month`: it still folds a month *name* ("03", "March", "Sept", "sep" -> "Mar"/"Sep") but returns anything else verbatim. The `raw.isalpha()` guard is what separates the two -- without it the 3-character prefix match silently truncated "Sep-Dec" to "Sep", so this was a latent corruption, not just a gap. Only an out-of-range *number* still becomes "". The second source is `_month_from_medline_date`, the month-side sibling of the existing year recovery. Its mandatory whitespace after the year is load-bearing: it stops "1999-2000" -- a year range with no month at all -- from yielding "-2000". `pub_day` stays blank, as the spec example has it. `_PUB_MONTH_SQL` is now generated by `_normalize_month_sql`, because the same CASE applies to both sources. It uses `\p{L}+` rather than `[A-Za-z]+`: Python's `str.isalpha()` is Unicode-aware, and the twins must not disagree on a non-ASCII spelling. `test_pub_month_sql_matches_python` iterates the cross product of the two inputs, since the SQL falls through from one to the other. `validate` routes efetch's side through the same function, reading `<Month>`, `<Season>` and `<MedlineDate>` -- PubMed uses all three renderings for the same record, and without this the fix would only relocate the disagreement. Verified against live PubMed: both renderings of PMID:8000234 now export "1994"/"Sep-Dec"/"" and compare equal. Also fixes a latent bug next door: `_VALID_MONTHS` was built from `calendar.month_abbr`, which is `LC_TIME`-dependent -- exactly what `export._MONTH_ABBR` was frozen to avoid. Under a non-English locale the `month-format` check would have warned on every record in the export. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PubMed serves the same record three ways: the baseline holds `<MedlineDate>1994 Sep-Dec</MedlineDate>`, efetch renders it as `<Year>1994</Year><Season>Sep-Dec</Season>`, and ordinary records use `<Month>`. We read the first and the third; `<Season>` was dropped entirely, so a record carrying it exported a blank month even after the previous commit. The DTD makes `<Month>` and `<Season>` mutually exclusive -- PubDate is `((Year, ((Month, Day?) | Season)?) | MedlineDate)` -- so the season can share the existing `pub_month` column instead of needing one of its own, and a schema migration with it. One `or` in `_raw_pubdate`. Note the split in when this lands: the `MedlineDate` half of the fix is export-only and needs no reload, but this half only takes effect for files loaded after the change. The test uses inline XML rather than a fourth fixture article, which would ripple through the record counts asserted in test_export, test_cli and test_validate. How common `<Season>` actually is in the *archival* files is still unmeasured -- every case observed so far has been the `MedlineDate` form in the baseline, with `<Season>` coming only from efetch. Recorded as open in FUTURE.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FUTURE.md said `pub_month`/`pub_day` were "deliberately still empty ... Revisit only if a consumer needs an approximate month more than it needs correctness." A consumer now does, and the spec's own example agrees, so record what shipped rather than leaving the old reasoning to be read as current. `pub_day` stays blank by design. Also records the `<Season>` prevalence question as open, and updates the two CLAUDE.md bullets that named `month_to_abbrev` and the pinning test by their old names. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 7, 2026
This was referenced Aug 20, 2026
Closed
gaurav
added a commit
that referenced
this pull request
Aug 20, 2026
…DuckDB with `--memory-limit` (#9) ## Summary Makes a cluster run something you can launch, watch and size from its own output, instead of five commands pasted out of `slurm/README.md` into a `screen` session. Four strands: - **`slurm/` gains a real pipeline** — one `sbatch` script per step, plus a `submit.sh` that runs a single step or chains them all with `--dependency=afterok`. - **The long-running steps narrate themselves** — `load` and `export` gained rate/elapsed/current-RSS on their progress lines (a minute apart, not ten seconds), and `validate` — previously ~11 minutes of silence — gained a start line, byte-based progress with an ETA, and per-phase lines. - **The DuckDB memory story is corrected and given a knob** — a group-level `--memory-limit`, and the retraction of two claims about DuckDB's defaults that were both wrong. - **The suite runs in CI** — a `tests` workflow on every pull request, across the two Python versions that matter: 3.11, the floor `pyproject.toml` requires for `hashlib.file_digest`, and 3.14, which is what `uv run python --version` reports on the cluster. ## The Slurm pipeline ```bash ./slurm/submit.sh load # one step; read its log, then decide ./slurm/submit.sh all # the lot, each gated on the last succeeding ./slurm/submit.sh --dry-run all # print the sbatch commands, submit nothing ``` `slurm/` now holds `01-download` … `05-validate.sbatch`, a `config.sh` of shared settings (all overridable from the environment: `SHARDS=32 ./slurm/submit.sh export`), and `submit.sh`. Logs go to `data/logs/`, which is already gitignored. Steps are separate jobs rather than one, because their resource shapes are nothing alike: `load` wants a long single-core job, `export` wants a big node for 23 minutes, `validate` wants a small one with internet. A single allocation sized for all three would hold the export's node for the load's several hours. Chaining with `afterok` (plus `--kill-on-invalid-dep=yes` — see below) means a failure cancels what follows, which is the automated form of reading each log before starting the next — but only for failures the exit status reports, so submitting one step at a time is still the careful mode and is still fully supported. The export gains the most: it no longer waits interactively for a big node to free up. `submit.sh` fails *before* submitting where it can: it checks you are at the repo root, creates the log directory (sbatch refuses to start when `--output`'s directory is missing, with an error that does not name the path), and rejects a chain ending in an online `validate` when `NCBI_EMAIL` is unset — better than discovering that after the load has run for hours. **Five bugs in these scripts would have fired on the cluster and not locally.** Two came out of running them, and are the failure modes shell has and Python does not: - Expanding an empty array under `set -u` is an "unbound variable" error on bash 3.2 — and it sat on the export's "cannot create the spill directory" fallback, i.e. a second failure on the path that handles the first. - `config.sh` used `:=` for `DUCKDB_TEMP_DIR`, so its own documented "set it empty to use DuckDB's default" silently restored the default instead. One more is a shell bug of the same kind, and the remaining two are Slurm semantics that this branch had assumed rather than checked. All three are invisible from a dev box, and the last two are invisible from `--dry-run` as well: - **The exclusion of today's manifest compared paths as strings.** `MANIFEST_DIR` is environment-overridable, so `data/manifests/` reaches `05-validate.sbatch` as readily as `data/manifests` — and the trailing slash makes `$manifest` read `data/manifests//pmids-<today>.txt.gz` against a single slash from `find`. The same file, never string-equal, so today's manifest became the baseline and was then overwritten by `--manifest`: `drops_since_previous` compared the export against itself and reported zero drops however many there were. Excluded by `find -name`, i.e. by base name, the spelling stops mattering. - **`--dependency=afterok` does not cancel anything on its own.** Slurm's default for a dependency that can never be satisfied is to leave the dependent *pending forever* with reason `DependencyNeverSatisfied`; it cancels only where the site set `kill_invalid_depend` in `slurm.conf`. This script's header, its closing hint and `slurm/README.md` all promised cancellation, so a load failing at hour three would have left the export and validate squatting in the queue until someone noticed. `--kill-on-invalid-dep=yes` on every chained job is what makes the promise true regardless of site config. - **`sbatch --parsable` prints `jobid;clustername` on a multi-cluster site.** Interpolated whole, the suffix lands inside `--dependency=afterok:12345;ht1`, which sbatch rejects — so the chain breaks at submit time on exactly the sites that are hardest to debug from here. The id is truncated at the first `;`. Separately, `all` combined with a named step (`./slurm/submit.sh all validate`) submitted that step twice, chained on itself. Two validates run against the same export, and the second rewrites the dated manifest and report and then — today's manifest now existing — diffs against the run before. The requested steps are deduped, first position winning, with a note on stderr. A third was suspected and measured *not* to be one, which is worth recording so it is not re-litigated: `[[ cond ]] && arr+=(...)` under `set -e` is harmless mid-script — the AND-OR list's non-final commands are exempt, so execution continues. It returns 1 only as the last statement of a script or function. The validate script still spells it as a plain `if`, as a hedge against someone later moving it or appending to the block, but no job would have died from the original form. `submit.sh` avoids associative arrays so it runs on bash 3.2 as well as 4+, which is also what let it be tested here. It stays shell rather than Python because the `.sbatch` scripts must be shell anyway and `source slurm/config.sh`; a Python submitter would mean either two config files or Python parsing shell to read one. ## `slurm/README.md` no longer repeats the allocations The `#SBATCH` headers are now the only place an allocation is written down, so a changed cluster profile is a one-file edit. The README keeps the **measurements** (the dated run tables, the 201.1 GiB peak, the 7m 38s-of-7m 57s shard-read breakdown) and the reasoning, because those are the *evidence for* the headers rather than a second copy of them — and a number nobody can trace is exactly how the two retractions below happened. Measurements are also append-only history, while settings are edits; the two want different homes. `test_readme_does_not_duplicate_the_allocations` stops runnable `srun --mem=…` lines creeping back. ## The "memory leak" is two things, neither a leak in our code A long load was reaching `peak RSS 42.1 GiB` by file 1201, well above the ~8 GiB `slurm/README.md` predicted. **1. `peak_rss_gib` reads `ru_maxrss`, a high-water mark that only ever rises.** Logged once per file it is the maximum over the *whole run so far*, not that file's footprint — so it climbs by construction and says nothing about what the process currently holds. (Verified: it does not drop after freeing 400 MB.) The docs presented it as a per-file figure, which is only true for the first file. Each file now logs current RSS beside it, which *can* fall and is the number that shows real growth: ``` loaded pubmed26n1201.xml.gz: 30000 articles, 0 deletions, 0 failed to parse, 0 book record(s) skipped (RSS 12.4 GiB, peak 42.1 GiB) ``` **2. DuckDB's buffer pool grows with the database, and its limit does not cover the rest of the process.** As the database grows DuckDB caches more of it, so RSS climbs run-long. Adds a group-level **`--memory-limit`** (`PUBMED2DB_DUCKDB_MEMORY_LIMIT`) alongside `--threads` and `--temp-dir` to buy headroom back. Note this is *not* about rescuing DuckDB from a node-sized cache — see below — but about the memory its limit does not cover: the lxml tree, the parsed records and the Arrow batch live in the same process and count against the same `--mem`, and a default claiming three quarters of the allocation leaves them the remaining quarter. What produced the original 42.1 GiB peak is now **unexplained**: if the buffer pool was capped to the allocation all along, the memory came from somewhere else. #25 (the loader holding a whole lxml DOM) is the leading candidate. ## Two retractions, one mistake made twice This repo believed DuckDB could not see a Slurm allocation and sized itself from the node. That was asserted for memory, then for CPUs, and **both were wrong** — measured on duckdb 1.5.4, on the cluster: | Setting | Believed | Measured | | --- | --- | --- | | `memory_limit` | ~80% of the node's physical RAM | ~76% of `--mem` (6.1 GiB under `--mem=8G`, 47.3 GiB under `--mem=62G`) — #36 | | `threads` | the machine's core count | `--cpus-per-task` (2 threads under `--cpus-per-task=2` on a 64-core node) — #38 | Both claims had reached the docs, the `--help` text and a filed issue before anyone ran the one-line `srun` probe that disproves them. So the corrections go beyond the prose: `--threads`' and `--memory-limit`'s help text, `db.connect`'s docstring, `README.md` and `slurm/README.md` all described the node-sized default and now describe the real one. `slurm/README.md` also stops telling operators to `export PUBMED2DB_THREADS="${SLURM_CPUS_PER_TASK:-4}"`, which set the pool to the size it already was. Worth keeping: DuckDB reads the cgroup's **CPU quota**, not an affinity mask — affinity in the measured run was the full 64. Anything deriving the allocation from `sched_getaffinity` (the natural way to write an auto-default) would still get it wrong. That closed the other half of #10, which is now closed as moot. `--threads` keeps its place with a new rationale: it is how you run *below* your allocation on a busy node, not a rescue from an oversubscription that does not happen. `AGENTS.md` carries both as a single entry built around the rule that survives them — *do not reason from "DuckDB cannot see the allocation"; measure it* — rather than around the two facts, since the facts are what changed and the reasoning error is what repeated. ## Progress lines size the next job Previously an ETA and nothing else. Now the rate (what scales to the remaining work) and elapsed (what you compare against the limit you asked for): ``` progress: 4/360 files this run, 356 remaining · 89.7 s/file · elapsed 5m 59s · ~8h 52m to go progress: 33,385,000/40,901,984 documents (81.6%) · 29.8k docs/s · elapsed 18m 40s · RSS 187.2 GiB · ~4m 14s remaining ``` Export gained current RSS too — it is the job that gets OOM-killed, so watching the ramp during the run is what tells you the next `--mem`. They also come a tenth as often: the export's gate was 10 s, which over a ~15-minute full-corpus run is ~90 near-identical lines. It is now 60 s, still a dozen-odd points to read the RSS ramp and the rate off. `tqdm` was considered and rejected: its `\r` updates are noise in a Slurm log, and logging already carries the timestamps you'd correlate against `sacct`. ## `validate` now narrates itself too It was ~11 minutes of silence, and the two things a run is silently misconfigured on — no database, no API key — only surfaced in the report at the end, by which time an anonymous run has already crawled at 3 req/s: ``` starting validation: 16 shard(s) in data/json, 42.3 GiB · database available · online with an NCBI API key (10 req/s) reading shards (structure check)... progress: 12,480,391 record(s), shard 5/16, 30.4% of 42.3 GiB read · elapsed 3m 04s · RSS 3.1 GiB · ~7m 02s remaining read 40,901,984 record(s) in 9m 58s (peak RSS 5.2 GiB) validation finished in 10m 51s (peak RSS 5.2 GiB) ``` The key itself is never logged, only that one was found. Progress is counted in **bytes of shard consumed**, not records — the record total is what that pass is computing, and counting shards alone would print nothing at all for the default single-shard export; reading through a raw binary handle keeps the position in the units `st_size` is in, for gzipped shards as well as plain. Each phase after the read is announced, which is what distinguishes "still reading shards" (local, CPU-bound) from "hung on an NCBI call" — and the Entrez phase is announced only when the run is actually online, so an `--offline` run does not contradict its own start line. The gate on those lines reads the clock once per record, which looks like waste in the hottest loop here and is not: `time.monotonic()` costs ~25 ns against ~2.8 µs of per-record work, so counting records instead would save ~1 s of a 7m 38s corpus read — and would space the lines by record size rather than by time, which is the one property a log read at a glance needs. A comment in `check_structure` says so, because the swap has already been proposed once. Measuring in bytes means stat'ing the shards, which happens once up front now rather than three times in three places. Two of those calls sat *outside* the truncated-shard guard `main` added, so a shard replaced between `find_shards` and the stat would have killed a run minutes from finishing — the half-written export the guard exists to report. `_shard_sizes` reports 0 for a shard it cannot stat; the read then fails inside the guard and records it as unreadable. ## PMID manifests are written, and dated `drops_since_previous` is the only check that catches two same-sized exports whose *contents* differ, and it had never run on real data: no corpus run had written a manifest for a later one to diff against. The documented commands are where that gets remembered, so `05-validate.sbatch` now passes `--manifest` on every run — and picks the newest *earlier* manifest as `--previous-manifest` automatically, skipping today's (by base name) so a same-day re-run cannot diff against itself. Manifests go to `data/manifests/pmids-<date>.txt.gz`, not into the export directory. #32 framed the location as "the export republishes in place", but the export's stale sweep only globs `pubmed_metadata_*.ndjson*`, so a manifest under `data/json/` **survives** — and then looks current while describing the previous corpus, which is worse than being deleted. The sharper reason is the flags: with one shared name the obvious next invocation passes the same path as both `--previous-manifest` and `--manifest`, overwriting the file it just diffed against and making run N vs N+2 impossible. **A failed run writes no manifest.** Making `--manifest` the default path for every cluster run changed what a failure costs: a manifest is the *next* run's baseline, and a truncated or half-written export — the case the structure check exists to catch — yields a short PMID set. Adopt that as the baseline and the next run reports the recovered corpus as "N added" while the real drops go unnoticed, which is the one comparison the manifest exists for. So `run_validation` skips the write when the report's status is `fail`, says so in the log, and reports `inputs.manifest_written` as `null` rather than naming a file it did not write. A `warn` run still writes one, deliberately. The common warning is "Entrez was unreachable", which is a statement about the network and says nothing about the PMID set the shard read built — and refusing on it would mean a cluster with flaky egress never accumulates a baseline at all. Only `fail` means the set itself is suspect. **The newest earlier report is fed back too**, on the same rule. The script archives a dated `validation_report-<today>.json`, but until now never passed an earlier one as `--previous-report` — so the `vs-previous` coverage check reported `skip` on every run it launched, however many reports had piled up. Unlike the manifest, a *failed* run's report stays eligible: `vs-previous` compares coverage percentages rather than absolute counts, so a short export surfaces as a drift warning on the next run rather than as a silent pass, and the dated report that caused it is right there to read. **The NCBI API key is handed over as an environment variable, never as `--api-key`.** `argv` is world-readable through `ps` on a shared node, so a credential on the command line is a credential on display — which also made the script's own "the key itself is never logged" comment half-true. The CLI's option already declares `envvar="NCBI_API_KEY"`, so the two are equivalent to `click` and only one of them leaks. Exported only when non-empty: exported empty, `click` sees `""` — a key that is present but blank — instead of nothing at all. `NCBI_EMAIL` stays an explicit flag, being contact details rather than a credential. Verified end to end against a small export: the manifest lands at the nested path, and a second run with one PMID removed reports `1 dropped (0 explained by a recorded deletion), 1 added`. ## Verification - **The full suite passes on both CI Pythons**: 188 passed, 1 skipped (the `/proc` current-RSS check, which is expected to skip on macOS and always runs on the cluster) on 3.11 and on 3.14. 64 of the 189 are new here — main has 125. - New `tests/test_util.py` pins the peak-vs-current distinction directly: that `peak_rss_gib` does not fall after memory is freed, and that `current_rss_gib` is either `None` or a plausible value at or below the high-water mark. - The `load`, `export` and `validate` progress branches are each covered by a test that forces the interval to zero — they otherwise never execute in the suite, so a mismatched `%`-format argument would first surface partway through a 20-minute production run. - A shard that vanishes between `find_shards` and the read is asserted to land in `unreadable_shards` while the surviving shard is still read, and an `--offline` run is asserted never to log "against Entrez". Both were mutation-checked. - `--memory-limit` is asserted to actually *shrink* DuckDB's default rather than sit alongside it, comparing parsed byte values. The cap is derived from the *observed* default rather than hard-coded, because this PR's own correction is that the default follows the cgroup — a fixed 1GB is only below it on a machine with enough memory, and in a small container the comparison inverts and fails a perfectly correct run. The band around the requested value is loose on purpose: DuckDB does not echo this setting back verbatim, reporting a 6.2 GiB request as `6.1 GiB` and a 1GB one as `953.6 MiB`. - New `tests/test_slurm_scripts.py` covers the shell: `bash -n` on every script, that each declares its own `#SBATCH` allocation and `set -euo pipefail`, that `all` chains the five steps in order with `afterok`, and that an online `validate` without `NCBI_EMAIL` is refused before anything is submitted. Four mutations were checked to fail it (drop `set -e`, reorder the chain, restore an `srun` allocation to the README, remove the email guard). - Each of the five late fixes above is pinned by its own test, and each was confirmed to fail against the pre-fix code: a trailing slash on `MANIFEST_DIR` still excludes today's manifest; every chained job carries `--kill-on-invalid-dep=yes` and the first does not; `all validate` submits validate once; a federated `12345;ht1` id is stripped before it reaches a dependency; a failed run writes no manifest while a warned run still does. The federated-id test is the only one that runs `submit.sh`'s real submit path, against a stub `sbatch` on `PATH` — `--parsable`'s output is invisible from `--dry-run`. ## Not verified **The suggested memory limits.** `LOAD_MEMORY_LIMIT` and `EXPORT_MEMORY_LIMIT` are starting points chosen to leave headroom, not measured optima. Too low trades OOM risk for spilling, which shows up as a collapsed s/file rate. #37. **The sbatch scripts against real `sbatch`.** Everything here was exercised through `submit.sh --dry-run`, a stubbed `uv` and a stubbed `sbatch`; no job has actually been submitted on `ht1`. The dependency chain in particular is verified only as the `sbatch` command lines it would produce — including the two Slurm behaviours corrected above, where the *fix* is a flag whose effect only a real failed chain can demonstrate. #39 stays open until a real run. ## Issues Three were closed during this work rather than by merging, because measurement settled them: **#36** and **#38** (both probed on the cluster), and **#10** (moot once both premises were disproved). **The rebuild this branch is for has its own checklist: milestone [2026aug20](https://github.com/TranslatorSRI/pubmed2db/milestone/1).** It was triaged against this branch, and splits into one decision and a set of measurements. Listed here as it stood when this PR was written; the milestone is authoritative. **Decide before the run — it changes what the export ships:** - **#14** — `pub_year` recovers from a free-text `MedlineDate` on `main`, but `pub_month` does not preserve approximate months. The fix is **PR #15**, which is open, based on `faster-json-export` rather than `main`, and currently conflicting. Unless it lands first, this export ships without approximate months and `PMID:8000234` still will not reproduce the DocumentMetadataAPI example. A fine outcome if chosen; an expensive one to discover after a whole-corpus run. **Watch during the run — it is the only chance to gather the evidence:** - **#11** — the ~91 s/file slowdown against a documented ~5–6 s/file. The new `s/file` progress line is what answers it. Its premise was corrected in place: it blamed the node-sized `memory_limit` that #36 disproved, so the 42.1 GiB peak is now *unexplained*, with #25 the leading candidate. - **#37** — tune `LOAD_MEMORY_LIMIT` / `EXPORT_MEMORY_LIMIT`. The `RSS` ramp and the `s/file` rate are the two signals. - **#39** — nothing has run under real `sbatch` yet. Closing it is the job of the first `./slurm/submit.sh all`. - **#24** — a live risk rather than a design wart: `export_json` deletes stale shards *before* writing, so an export that dies partway destroys the current good one. Mitigable for this run without code, by exporting to a dated `EXPORT_DIR` and swapping once `validate` passes. - **#29** — Parquet stays unmeasured unless deliberately submitted; `04-export.sbatch` runs JSON only. **Check afterwards, against the finished export:** - **#28** — the corpus-scale confirmation of the `pub_year` backfill. "The next rebuild" it waits for is this one. - **#32** — the manifest is now written on every documented run, but `drops_since_previous` still cannot fire until one rebuild writes a manifest and the *next* one reads it. A property of the data, not of this branch. Not in the milestone, but downstream of what this branch measured: - **#13** — 7m 38s of `validate`'s 7m 57s is the single-threaded shard read. This branch measured it and left it alone. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Brings in PR #12's COPY-based JSON export and the fixes that followed it. Simpler than the last merge in this stack, because this branch was already based on the COPY rewrite — its `_PUB_MONTH_SQL` was already SQL, not the Python `_document` loop. Three resolutions needed judgment: **The whitespace trim had to be woven into the generator, not pasted next to it.** main fixed `_PUB_MONTH_SQL` to strip what Python's `.strip()` strips (SQL `trim(x)` takes spaces only), while this branch replaced that constant with `_normalize_month_sql(expr)`, a generator applied to two sources — the `pub_month` column and the `MedlineDate` remainder. Taking either side whole would have dropped the other: every `trim` in the generated SQL now names `_WS`, including the `ELSE` passthrough this branch added, so `"\tSep-Dec"` survives as `"Sep-Dec"` on both sides. `test_pub_month_sql_matches_python` already iterates the cross product of month × MedlineDate inputs; the union of both sides' cases now runs through it, 276 combinations. Mutation-checked. **CLAUDE.md became AGENTS.md on main again**, and again git flagged no conflict on AGENTS.md, so this branch's three documented decisions would have been lost silently: the approximate-month rule and its `isalpha()` guard, the cross-product pinning, and why `validate` takes `_MONTH_ABBR` from the exporter rather than `calendar.month_abbr`. Ported. **FUTURE.md's `MedlineDate` item was resolved on one side and extended on the other.** This branch closed the "pub_month is deliberately empty" half; main added the PMCID item (#33) and an issue number to the ELocationID one. Both kept. `validate._VALID_MONTHS`: both sides independently replaced `calendar.month_abbr` for the same locale reason. This branch's version is a superset — abbreviations, seasons and ranges — and already carries main's reasoning, so it stands. Checked every merged file for definitions lost, duplicated, or silently emptied — the three failure modes of the previous merge. The only names missing are this branch's own renames (`month_to_abbrev` → `normalize_month`/`pub_month`, and their tests), each with its replacement present. Full suite: 214 passed, 1 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Preserves approximate PubMed months and seasons in JSON exports, resolving issue #14.
Changes:
- Normalizes month names while preserving seasons and ranges.
- Parses
<Season>and recovers months fromMedlineDate. - Aligns validation, tests, schema comments, and documentation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/pubmed2db/export.py |
Implements month normalization and fallback extraction. |
src/pubmed2db/parse.py |
Reads <Season> into pub_month. |
src/pubmed2db/validate.py |
Validates and compares approximate months consistently. |
src/pubmed2db/schema.sql |
Clarifies pub_month storage. |
tests/test_export.py |
Covers normalization and SQL parity. |
tests/test_parse.py |
Covers season parsing. |
tests/test_validate.py |
Covers efetch date renderings. |
FUTURE.md |
Updates date-fidelity status. |
AGENTS.md |
Documents month-handling decisions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
DuckDB's RE2 reads `\s` as `[\t\n\f\r ]` — no `\v`, no non-ASCII space —
where Python's `re` includes both. With `\s` on both sides,
"1998\xa0Spring" gave `Spring` from the Python twin and "" from the SQL.
`validate._text` collapses whitespace before the efetch side reaches
pub_month, so efetch always renders `Spring`: the disagreement surfaced as
a pub_month mismatch, which is a CORE_FIELDS error — the same gating
failure the `_WS` note describes for `<Month>`.
`[\s\p{Z}\x0B]` tracks Python's `\s` on every case probed, including the
em space and the form feed. The pinning test could not have caught this:
every entry in _MEDLINE_INPUTS used a plain space.
Also `raw.isascii() and raw.isdigit()` rather than `isdigit()` alone.
`"²".isdigit()` is True while `int("²")` raises, and this function now
sees unconstrained MedlineDate CDATA rather than only a `<Month>` element,
so that ValueError would escape efetch_documents and abort a whole
validation run. The guard doubles as an exact match for the SQL twin's
`[0-9]+`: `"١٢"` is both isdigit() and isdecimal(), so Python alone
answered `Dec` where the export passes `١٢` through verbatim.
Both mutation-checked against the cross-product test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two documentation gaps the review found, neither a behaviour change. README's date bullet still listed only Year/Month/Day/MedlineDate. `parse._raw_pubdate` also reads `<Season>` into the pub_month column, so a Parquet consumer reading `article.pub_month` sees `Winter` or `Sep-Dec`. schema.sql, AGENTS.md and FUTURE.md had been updated; the README is the user-facing one that had not. And `_month_from_medline_date` now says why "1998 Dec-1999 Jan" exports a month field containing a year. It mirrors what PubMed wrote and what its API renders back rather than inventing a tidier "Dec-Jan" that no source says, and validate's closed _VALID_MONTHS excludes it deliberately so such records stay visible as warnings. Export and checker disagreeing there is the design. It was written down only in the PR description, which is why it came back as a review finding; a reader of the function now sees it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three places said "revisit if a consumer asks for a different representation" without naming the answer already in flight. PR #17 adds a verbatim `pub_date` carrying PubMed's own string, the way esummary's `pubdate` does, so a consumer rendering a citation has a clean string and the three parsed fields stay parsed conveniences. Also records the part that is genuinely open, rather than leaving it implied: shuchenliu asked on #14 whether DocumentMetadataAPI consumers are comfortable receiving `"pub_month": "Dec-1999 Jan"`, and that has not been answered. `pub_date` gives them something better to read; it does not settle whether the odd month is acceptable beside it. Filed alongside the gzip and shard-naming questions in FUTURE.md's ingest-contract item, which is the list to work through before the next production export. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The question — whether DocumentMetadataAPI consumers accept a pub_month carrying a year — was raised on #14 and never answered. #14's code half is fixed by this PR, so leaving it there meant it would close along with the issue while still being open. Now its own issue, cross-referenced from #14's banner, from FUTURE.md's ingest-contract list, from AGENTS.md, and from the docstring of the function that produces the value. The docstring also names the change an answer of "blank it instead" would need, since that is a one-line gate here and has to happen before a whole-corpus run rather than after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gaurav
added a commit
that referenced
this pull request
Aug 20, 2026
Brings in #15 (which this branch was cut from, before its review round) plus the fixes that followed it. Two resolutions needed judgment, and one of them was a live bug rather than a textual clash: **`_PUB_DATE_SQL` reintroduced the divergence main had just fixed for `pub_month`.** This branch was cut before that fix, so its new SQL used bare `trim()` while the Python twin uses `.strip()` — and SQL `trim` strips spaces alone. Measured, not assumed: a `MedlineDate` of "\t1998 Spring" and a `pub_day` of "\n15" each made `pub_date` disagree with `pub_date()`. Every `trim` in the expression now names `_WS`, and the twins agree across all 1,680 combinations of the four date inputs. This is the second time the same mistake has arrived through a merge, so `AGENTS.md` now says it about `_PUB_DATE_SQL` specifically rather than leaving it as a general rule about the month SQL. **A branch test globbed `*.ndjson`.** Gzip became the export default in #12, which reached main after this branch was cut, so `test_month_format_accepts_what_the_export_can_emit` failed on StopIteration for all nine of its cases. Switched to the gzip-aware `_append_to_shard` helper that already exists in the file. CLAUDE.md became AGENTS.md on main, as in the previous two merges of this stack, and again with no conflict flagged on AGENTS.md itself. This branch's three decisions are ported: `pub_date` as the fidelity guarantee (including why `_PUB_DATE_SQL` calls `_normalize_month_sql` rather than `_PUB_MONTH_SQL`), `esummary` as the rendering to consult for "what should we emit", and PMID sampling as the way to measure corpus-wide frequency. FUTURE.md keeps both sides' new bullets. Checked every merged file for definitions lost, duplicated or emptied. Two flags, both false positives: tests/test_cli.py is byte-identical to main, and test_mismatch_kind_classification is a genuine one-line parametrized assertion. Full suite: 231 passed, 1 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gaurav
added a commit
that referenced
this pull request
Aug 20, 2026
The `_WS` fix in #15 closed half this gap and left the other half open, and my merge commit for this branch asserted it was closed. It is not: `str.strip()` is Unicode-aware, so a leading `\xa0` or trailing ` ` still split the SQL and Python twins. Three expressions were affected, and one of them fails badly: - `_PUB_DATE_SQL` *branches* on whether medline_date is blank. A value of a single non-breaking space is non-empty to an ASCII trim and empty to `.strip()`, so one side emitted the whitespace verbatim while the other assembled the date from year/month/day — two entirely different values, not a formatting difference. - `_PUB_MONTH_SQL` returned `'\xa0Mar'` where `pub_month` returned `'Mar'`. - `_PUB_YEAR_SQL`'s `^\s*(\d{4})` has the same RE2-vs-Python split, so `"\xa01998 Spring"` recovered its year in validate and exported blank. Found by the new test cases rather than by review. Replaced with `_strip_sql`, a regex strip over the same `[\s\p{Z}\x0B]` class `_MEDLINE_SEP_SQL` already used — one helper, so there is no longer a second spelling of "remove whitespace" to keep in step. Verified equal to `str.strip()` over the shapes the tests use. The input lists gained Unicode whitespace at the *edges*. Every entry was ASCII, and the odd characters that did exist sat internally, which is why 1,680 combinations passed over a bug that a single leading `\xa0` shows. Batched the three twin tests into one query each: the widened 4-way product is ~17k combinations, which at one `con.execute` apiece took the suite from 20s to 64s. Registering the inputs as a table and projecting the expression over them is the same assertions in 3s. Both fixes mutation-checked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gaurav
added a commit
that referenced
this pull request
Aug 20, 2026
FUTURE.md said this was unmeasured and floated the possibility that the <Season> half of #15 was pure insurance — that every observed case was <MedlineDate> in the baseline with <Season> appearing only from efetch. It is not. zgrep over two real files: pubmed26n1334.xml.gz (baseline) carries 6 <Season> against 41 <MedlineDate> and 4,989 <PubDate>; pubmed26n1595.xml.gz (update) carries 1 against 232 and 18,079. Every one was <Year> + <Season> (Winter, Fall) rather than a range, which is exactly the shape pub_date has to converge with the <MedlineDate> form. So ~0.1% of baseline records — small enough that the corpus figure is still worth taking from the first full load, large enough that dropping the branch would silently blank the month on thousands of records. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gaurav
added a commit
that referenced
this pull request
Aug 20, 2026
#17) #15 has landed, so this targets `main` directly and has been merged with it. Addresses the cross-year case raised on [#14](#14 (comment)). ## The problem `pub_year`/`pub_month`/`pub_day` are parsed conveniences, and **no arrangement of three fields represents a cross-year range**. PMID:10188493 is `<MedlineDate>1998 Dec-1999 Jan</MedlineDate>`, which we split into: ```json "pub_year": "1998", "pub_month": "Dec-1999 Jan", "pub_day": "" ``` Lossless — the two concatenate back to the source — but a field named `pub_month` holding a year is a wart, and every alternative split just moves the ambiguity somewhere else. ## NCBI already solved this `esummary` ships a verbatim `pubdate` on every record, alongside a normalized `sortpubdate`: | PMID | `pubdate` | `sortpubdate` | | --- | --- | --- | | 30690000 | `"2019 Mar 15"` | `2019/03/15` | | 8000234 | `"1994 Sep-Dec"` | `1994/09/01` | | 10188493 | `"1998 Dec-1999 Jan"` | `1998/01/01` | | 35504184 | `"2022 Aug 1"` | `2022/08/01` | This PR takes the first half. **`pub_date` carries PubMed's own string verbatim, on every record.** The three parsed fields stay exactly as they are. Consumers rendering a citation read `pub_date`; consumers sorting or filtering read `pub_year`; neither parses the other's output. It fixes every lossy shape at once — seasons, bare year ranges, `"n.d."` — not just the one that prompted it. ## The rule ``` pub_date = MedlineDate verbatim, if present | otherwise: Year + normalize_month(Month or Season) + normalize_day(Day) "1998 Dec-1999 Jan" (MedlineDate) -> "1998 Dec-1999 Jan" 1994 + <Season>Sep-Dec -> "1994 Sep-Dec" <- converges with the line above 2019 + Mar + 15 -> "2019 Mar 15" 2022 + Aug + <Day>01</Day> -> "2022 Aug 1" <- unpadded, as esummary renders it 2019 + <Month>13</Month> + 15 -> "2019" <- an unusable month takes the day with it 2019 (year only) -> "2019" nothing -> "" ``` Two normalizations on the assembled branch, both there so the output equals what `esummary` returns rather than what the XML happens to spell. `<Month>` may be `03`, `March` or `Sept`; `<Day>` is zero-padded in the archive (`PMID:35504184` is `<Day>01</Day>`) where `esummary` renders `1`. Without the day half, most records with a day below the tenth would have differed from NCBI on a field whose entire justification is matching NCBI. And a day whose month normalized away is dropped rather than floated up beside the year — `"2019 15"` reads as a date and is not one; NCBI's `sortpubdate` drops precision in the same situation rather than guessing. **The convergence is the load-bearing property.** PubMed serves the same record as `<Year>+<Season>` from efetch and as a bare `<MedlineDate>` in the baseline; both must produce one string, or `validate` reads every such record as a mismatch. That's asserted directly rather than left implied. ## Why `pub_year` keeps the *leading* year Left unchanged at `1998`, not `1999`, and the reasoning is now written into `_year_from_medline_date`'s docstring where a reader will look. The trailing year is arguably the better semantic answer — the issue mostly reached readers in Jan 1999 — but: 1. **NCBI's own `sortpubdate` uses the leading year** on every cross-year shape sampled (`"1997 Dec-1998 Jan"` → 1997, `"1987-1988"` → 1987). A consumer joining our `pub_year` against anything Entrez-derived would disagree otherwise. 2. Every other shape already uses it; a trailing-year rule would fire only when two years appear, making one rare shape behave unlike the rest. 3. It's **~0.07% of PubMed** — 4 of 5,773 uniformly sampled records, and three of those were bare `"1987-1988"` ranges we already handle. The problem shape proper is ~1 in 5,773 (~7k records of 40.9M). Anyone who needs `1999` can read it from `pub_date`. ## Verification - `test_pub_date_sql_matches_python` — 4-way cross product of year × month × day × MedlineDate (17,670 cases), the same twin-pinning pattern as `pub_month`. The 4-way product matters: a disagreement can hide in any pairing, notably an absent month leaving a double space in one implementation but not the other. Every input list carries Unicode whitespace at an edge as well as internally — see "The whitespace bug, again" below for why that distinction is the whole game. The three twin tests each run as one query over a registered table rather than one `execute` per combination; at that size the naive form cost 45s of suite time. - `test_pub_date_matches_ncbi_pubdate` — offline table pinned to esummary's **real** output for the PMIDs above, including both renderings of 8000234. - **End-to-end:** PMIDs 30690000, 8000234 and 10188493 through `parse → load → export`, then diffed against live esummary — `pub_date` byte-identical for all three, and 8000234's two renderings converge. - `validate` compares `pub_date` against Entrez as a **soft** field — reported, never able to fail the run. See "Comparing dates: two problems, fixed in two places" below. - `test_pub_date_matches_ncbi_pubdate` — an offline table pinned to esummary's **real** output, including `PMID:35504184`'s unpadded day and both renderings of `8000234`. Checked against live esummary while writing it, not from memory. - 235 tests pass, 1 skipped, on both CI Pythons (3.11 and 3.14). ## The whitespace bug, again — and this time it flipped a branch SQL `trim(x)` strips spaces and nothing else; Python's `.strip()` strips all whitespace. #15 fixed that for `pub_month` by naming an explicit ASCII set, which closed half the gap: `.strip()` is *Unicode*-aware, so a leading `\xa0` or a trailing `\u2003` still split the twins. Three expressions were affected, and `pub_date` fails worst of the three because it **branches** on whether the `MedlineDate` is blank — a value of one non-breaking space is non-empty to an ASCII trim and empty to `.strip()`, so one implementation emitted the whitespace verbatim while the other assembled the date from year/month/day. Two entirely different values, not a formatting difference. `_PUB_MONTH_SQL` had the same gap (`'\xa0Mar'` against `'Mar'`), and adding the edge cases surfaced a third instance nothing had flagged: `_PUB_YEAR_SQL`'s `^\s*(\d{4})` has the identical RE2-versus-Python split, so `"\xa01998 Spring"` recovered its year in `validate` and exported blank. All three now go through one `_strip_sql` helper over the same `[\s\p{Z}\x0B]` class `_MEDLINE_SEP_SQL` already used, so there is no second spelling of "remove whitespace" left to drift. The reason the tests never caught any of it: every input was ASCII, and the odd characters that did exist sat *internally*, never at an edge — 1,680 combinations passing over a bug that one leading `\xa0` reveals. ## Comparing dates: two problems, fixed in two places Conflating these is easy and produces the wrong fix, so they are separated deliberately. **Rendering.** efetch does not serve the archival string — it serves a re-serialization, so `efetch_documents` *reconstructs* one, normalizing the month, which is exactly what makes the `<Year>+<Season>` rendering converge with the `<MedlineDate>` one. Where the baseline holds a full month name the two therefore cannot agree as strings: `"1998 September"` exported against `"1998 Sep"` reconstructed. Both correct, and no amount of work on the *export* can close it, because the export is right on both sides of it. `validate._normalize_date` folds date spellings **at comparison time only** — nothing about the export changes, both strings still ship exactly as they are, but the report stays about the data rather than about how the two sides happen to write it. The zero-padded day is the same problem in a different component, and `normalize_day` catches the half the export controls. **Correlation.** `pub_date` is derived from the same three columns as `pub_year`/`pub_month`/`pub_day`. Gating on it would let one *genuine* date disagreement — a wrong year, say — contribute four mismatches instead of three, tightening the FAIL threshold on exactly the records most likely to trip it. Normalization does nothing about this, and `validate.py` already makes the same argument for keeping the `identifiers` comparison advisory. So `pub_date` is a **`SOFT_FIELDS`** entry: compared, reported, never fatal — for the second reason, not the first. The docstring there says so at length, because putting it in `CORE_FIELDS` looks obviously right and someone will try it. ## Notes - **No schema change, no reload** — `medline_date` and the three components are already stored; this is entirely export-layer. **A re-export is required** to ship it. - **Size:** one string field × 40.9M records, ~800MB on a 16GB export (~5%). - **A day with no usable month is dropped, not floated up.** `("2019", "13", "15")` used to render `"2019 15"`, which reads as a date to the consumer this field exists for and is not one. `normalize_month` blanks an out-of-range number and those do reach the export. NCBI's `sortpubdate` drops precision in the same situation rather than producing a plausible wrong answer. - **`validate` compares `pub_date` normalized, and that is comparison-only.** `_normalize_date` exists so the report is about the data; it never touches what the export writes. - **Field count is now twelve**, locked by `test_expected_fields_matches_spec`. Nine are DocumentMetadataAPI's; `id`, `identifiers` and now `pub_date` are ours. Worth raising on NCATSTranslator/Core-Components-Working-Group#15, where the field set is already under discussion (URLs, authors). - **This defuses #16** — with the raw string preserved, normalizing `"September-December"` → `"Sep-Dec"` inside `pub_month` becomes cosmetic. - **Not in scope:** `sortpubdate`, the other half of NCBI's design. Nothing downstream range-filters yet; recorded in FUTURE.md. ## TODO Both pre-merge items are settled: - [x] **`<Season>` prevalence in the archival XML — measured, and it is rare but real.** It *does* appear, so reading it is load-bearing rather than insurance: `pubmed26n1334.xml.gz` (baseline) carries 6 `<Season>` against 41 `<MedlineDate>` and 4,989 `<PubDate>`; `pubmed26n1595.xml.gz` (update) carries 1 against 232 and 18,079. Every one was `<Year>` + `<Season>` (`Winter`, `Fall`) rather than a range — the shape `pub_date` has to converge with the `<MedlineDate>` form. ~0.1% of baseline records: small enough that the corpus figure is worth taking from the first full load, large enough that dropping the branch would silently blank the month on thousands of records. FUTURE.md now records the numbers instead of the open question. - [x] **Field name stays `pub_date`**, for consistency with the neighbouring `pub_year`/`pub_month`/`pub_day` and with the repo's snake_case fields throughout. It diverges from NCBI's exact `pubdate`, which the docstring already notes it mirrors. Still worth confirming on [CCWG#15](NCATSTranslator/Core-Components-Working-Group#15) alongside the rest of the field set. The remaining items are tracked elsewhere rather than here, since none can close in this PR: - **#16** — whether to normalize month names inside a range. This PR defuses it: with the raw string preserved in `pub_date`, `"September-December"` → `"Sep-Dec"` is cosmetic rather than a fidelity question. Needs a full corpus load; expect a won't-fix unless the counts surprise. - **FUTURE.md, "No `sortpubdate` equivalent"** — the other half of NCBI's design, deliberately not taken. Needs a consumer that wants range-filtering, plus its own call on what to emit for an unparseable range. - **#42** — re-baseline the export's `--mem` and `--time`; the twelfth field's ~800MB is unmeasured and folds into that run. - **#43** — whether DocumentMetadataAPI consumers accept the shapes this export ships. `pub_date` makes the field count twelve, which is a third consumer-visible change alongside the gzip default and the shard renaming. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
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.
#12 has landed, so this now targets
maindirectly and has been merged with it. #17 is stacked on top of this one — it adds the verbatimpub_datefield, and reading them together is what makes the"Dec-1999 Jan"case below read as a design rather than a wart.Closes #14. Follow-ups: #16 (normalizing names inside a range), #17 (
pub_date).The problem
The DocumentMetadataAPI spec contradicts itself. Its prose says "Send month names as capitalized three-letter abbreviations", but its own worked example for PMID:8000234 emits:
We were following the prose and shipping
"pub_month": ""for every season or range. This PR follows the example: take the month from the input and pass it through unchanged, normalizing only the values that genuinely are month names.There was also a latent corruption hiding behind the gap.
month_to_abbrevmatched on a 3-character prefix, so a value like"Sep-Dec"would have been silently truncated to"Sep"— we only never saw it because the season never reached the function.The rule
The column wins when both are present (
pub_month("Mar", "1998 Spring")→"Mar"); theMedlineDateis a fallback, not an override.Month names fold
"Mar""Mar""3"/"03"/" 3 ""Mar""March""Mar""Sept""Sep""sep"/"SEPTEMBER""Sep""13"""Everything else passes through — this is the fix
"Sep-Dec""Sep-Dec""Sep"— silently truncated"Jul-Aug""Jul-Aug""Jul"— silently truncated"Spring""Spring""""Winter""Winter""""Winter-Spring""Winter-Spring""""September-December""September-December"""— see #16"Jul-August""Jul-August"""— see #16"Sept-Dec""Sept-Dec"""— see #16Recovered from a free-text
MedlineDate"1994 Sep-Dec""Sep-Dec""1998 Spring""Spring"" 2001 Winter""Winter""1998 September""Sep""1998 Dec-1999 Jan""Dec-1999 Jan""Dec-Jan""1999-2000""""-2000""n.d."/"Spring 1998""""12345"""pub_daystays blank throughout, as the spec example has it.How it's built
Three commits, each green on its own:
month_to_abbrev→normalize_month, with anisalpha()guard separating "is a month name" from "isn't";_month_from_medline_dateas the month-side sibling of the existing year recovery. Its mandatory whitespace after the year is load-bearing: it stops"1999-2000"yielding"-2000".<Season>— PubMed serves the same record three ways, and we read only two. The DTD makes<Month>and<Season>mutually exclusive, so the season shares the existingpub_monthcolumn — no schema column, no migration.<Season>in the README's date bullet, which is the one user-facing place that still listed onlyYear/Month/Day/MedlineDate. A Parquet consumer readingarticle.pub_monthnow seesWinteras readily asMar.Merging
mainadded a fourth strand. TheCOPYrewrite had fixed_PUB_MONTH_SQLto strip what Python's.strip()strips (SQLtrim(x)takes spaces alone), while this branch had replaced that constant with_normalize_month_sql(expr)— a generator applied to two sources. Taking either side whole would have dropped the other, so everytrimin the generated SQL now names_WS, including theELSEpassthrough this branch adds.Verification
test_pub_month_sql_matches_pythoniterates the cross product of 27 month × 15 MedlineDate inputs (405 cases) — the SQL falls through from one source to the other, so they interact. Includes a non-ASCII spelling, which is why the SQL uses\p{L}+and not[A-Za-z]+: Python'sstr.isalpha()is Unicode-aware and the twins must not disagree.parse → load → exportin both renderings — efetch's<Year>1994</Year><Season>Sep-Dec</Season>and the archival<MedlineDate>1994 Sep-Dec</MedlineDate>. Both emit1994/Sep-Dec/"", reproducing the spec example.validateagainst live PubMed:pub_monthno longer appears in the mismatch tally. Its efetch side now reads all three renderings through the exporter's own function — without that the fix would only relocate the disagreement.Two ways the twins disagreed, both found by review
The Python/SQL pair is only worth having if it actually agrees, and the cross-product test had blind spots in both directions. Both fixes are mutation-checked.
The
MedlineDateseparator meant different things in the two regex engines. DuckDB's RE2 reads\sas[\t\n\f\r ]— no\v, no non-ASCII space — where Python'sreincludes both. So"1998\xa0Spring"gaveSpringfrom the Python twin and""from the SQL. Becausevalidate._textcollapses whitespace before the efetch side reachespub_month, efetch always rendersSpring, and the disagreement surfaced as apub_monthmismatch — a CORE_FIELDS error on a correctly-exported record, the same gating failure the<Month>tab case had already caused once.[\s\p{Z}\x0B]tracks Python's\son every shape probed, em space and form feed included. The test could not have caught it: every_MEDLINE_INPUTSentry used a plain space as the separator.normalize_monthcould raise on input this PR is what widens."²".isdigit()isTruewhileint("²")raises, and the function's input goes from a<Month>element (constrained toJan..Dec/01..12) to arbitrary<MedlineDate>CDATA. TheValueErrorwould have escapedefetch_documentsand aborted a whole validation run rather than being recorded as one odd record. Guarded withraw.isascii() and raw.isdigit(), which is also the exact match for the SQL twin's[0-9]+—"١٢"is bothisdigit()andisdecimal(), soisdecimal()alone would have left Python answeringDecwhere the export passes١٢through.Also fixed
validate._VALID_MONTHSwas built fromcalendar.month_abbr, which isLC_TIME-dependent — exactly whatexport._MONTH_ABBRwas frozen to avoid. Under a non-English locale themonth-formatcheck would have warned on every record in the export. Now built from the exporter's tuple, and broadened to months, seasons and ranges of those, so odder shapes like"Dec-1999 Jan"stay visible as warnings rather than being blessed.Rollout
A re-export is required for this to reach consumers. A reload is required only for
<Season>records — and only if the archival XML actually carries them, which is still unmeasured: every case observed so far is the<MedlineDate>form in the baseline, with<Season>coming only from efetch.zgrep -c "<Season>"over a baseline file would settle it; noted as open in FUTURE.md.Deliberately not in scope
"September-December"stays verbatim rather than becoming"Sep-Dec". Tracked in Decide whether to normalize month names insidepub_monthranges #16 with the query to decide it and the sketch to implement it. It may describe zero records: PubMed's own convention in<Season>/<MedlineDate>is already 3-letter.pub_dayfor range dates — the spec example wants""."Dec-1999 Jan"— that would mean inventing a value PubMed never wrote. It is what PubMed wrote and what its API renders back, so the export mirrors it, andvalidatewarns on it deliberately:_VALID_MONTHSis a closed set that admits months, seasons and ranges of those but not this, so cross-year shapes stay visible instead of being blessed. Export and checker disagreeing here is the design, which had been recorded only in this description — it came back as a review finding, so it now lives in_month_from_medline_date's docstring and inAGENTS.mdtoo. The real answer to the wart is a different field, not a tidier month: Add a verbatimpub_datefield for dates the parsed fields can't hold #17 adds a verbatimpub_datecarrying PubMed's own string, the wayesummary'spubdatedoes.Open, and worth answering before the rebuild
@shuchenliu asked on #14 whether DocumentMetadataAPI and its consumers are comfortable receiving
"pub_month": "Dec-1999 Jan", and that has not been answered. #17'spub_datedoes not close it — it gives consumers a clean string to render from, which is the better answer, but whether the oddpub_monthis acceptable alongside it is a separate yes/no from the same people.Split out as #43 so it does not close along with #14 when this lands, and cross-referenced from
FUTURE.md's "Confirm the Node Annotator JSON contract" list — with two other things the next production export changes that consumers cannot see coming: shards are gzipped by default now, and shard filenames lost their zero-padding. All three want the same conversation, before the rebuild rather than after it.If the answer is "blank it instead", the change is a one-line gate in
_month_from_medline_daterestricting theMedlineDatefallback to the closed setvalidate._VALID_MONTHSalready accepts — named in that function's docstring, and cheap, but it has to be made before a whole-corpus run rather than after.🤖 Generated with Claude Code