Prepare the v1.0.0 rebuild: current dependency floors, and fixes to what the corpus run would silently have got wrong - #44
Open
gaurav wants to merge 5 commits into
Open
Prepare the v1.0.0 rebuild: current dependency floors, and fixes to what the corpus run would silently have got wrong#44gaurav wants to merge 5 commits into
gaurav wants to merge 5 commits into
Conversation
Raised together, re-locked with `uv lock --upgrade`, and verified: duckdb
1.1 -> 1.5.5, pyarrow 14 -> 25, click 8.1 -> 8.4, requests 2.31 -> 2.34,
tqdm 4.66 -> 4.70, lxml 5 -> 6.1, pytest 8 -> 9. Transitively that also
moves pystow 0.8.19 -> 0.9.0, which is what resolves the download cache
paths — checked that PYSTOW_HOME still lands files at
`<home>/pubmed/{baseline,updates}`, unchanged.
pubmed-downloader stays at `>=0.0.14,<0.1`: 0.0.14 is still the latest
release, so the ceiling has not been reached and the private-API reason
for it is unchanged.
The floors are now the versions this release is tested against rather
than the oldest that might work. For a corpus run that is the useful
guarantee — "it resolved to something older on your machine" is not a
debugging session anyone wants mid-export — and the export's SQL leans on
version-specific DuckDB behaviour (regexp classes, TRY_CAST,
PER_THREAD_OUTPUT options) that the twin tests only ever ran against one
version.
Re-checked the one claim that asks to be re-checked on a version bump:
DuckDB still rejects PARTITION_BY for FORMAT JSON on 1.5.5, so the note
in AGENTS.md stands and now cites the version the floor pins. The cgroup
measurements were taken on 1.5.4 and have *not* been re-taken — noted as
such rather than quietly inherited, with #42's cluster run as the place
to redo the probe.
Verified beyond the suite, since fixtures are small and this is a release
branch: a real baseline file plus an update file (23,052 articles)
through load -> journals -> export -> validate on the upgraded stack.
Export and validate both clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The summary line printed "compared but never fails the run (NLM Catalog source): journal_name, journal_abbrev, pub_date". True of the first two; false of pub_date, which is soft because efetch renders a date where we ship the archival string, and which has nothing to do with the NLM Catalog. The line was correct when SOFT_FIELDS held only the journal fields and quietly stopped being so when pub_date joined them. SOFT_FIELDS is now derived from SOFT_FIELD_REASONS, a reason -> fields mapping the summary iterates, so a field cannot be added to one without declaring the other. Same "derived, never restated" property the rest of that summary already has. Found by running validate over a real export rather than the fixtures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`config.sh` used `:=` for LOAD_MEMORY_LIMIT and EXPORT_MEMORY_LIMIT, so passing either empty silently restored the default — the opposite of what passing it empty means, and the exact bug DUCKDB_TEMP_DIR was fixed for in #9. Only that one was fixed; these two were written the same way and missed. It matters now because both defaults sit at or above DuckDB's own cgroup-derived default (48GB against ~48.6G of --mem=64G; 200GB against ~194G of --mem=256G), so the first thing anyone tuning #37 would try is clearing them to establish a baseline — and it would not have worked. `=` rather than `:=` on both. Empty is safe the whole way down: the CLI's option is falsy, so db.connect issues no SET, verified end to end. The test covers the rule rather than the two instances that were noticed, parametrized over every clearable setting, plus a companion asserting each still has a default. Mutation-checked by restoring `:=` on one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`--sample-size` is per *shard*, and since the COPY rewrite the export writes one shard per writer thread — so the total silently tracks `--cpus-per-task`. With `SHARDS` now derived from the allocation, the export drops from 16 shards to 8, which would have halved the Entrez field check from 240 records to 120 on the corpus run whose numbers matter most, with no flag changing to say so. 05-validate.sbatch divides VALIDATE_SAMPLE_TOTAL (240 — what the pre-2026 runs sampled as 15 x 16) by the shards actually on disk. Runs stay comparable across allocations, and a corpus with more shards than the target still samples every shard rather than rounding to zero. Clearable like the other knobs, in which case nothing is passed and the CLI's own per-shard default stands. Provisional for this rebuild: whether a fixed total is the right shape at all is worth revisiting once there are real numbers to look at. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"If both climb together and RSS never comes down, DuckDB's limit is too high" was written under the belief that DuckDB sized its buffer pool from the node. It reads the cgroup (#36), so the default is not the outsized thing that sentence assumed, and a climb that never comes down is as likely to be the memory its limit does not govern — the lxml tree, the parsed records, the Arrow batch. Give the reader the test instead of the conclusion: lower the limit and re-run, because that moves one half and not the other. Also say plainly that the 42.1 GiB reading has never been explained, since it was measured before the cgroup finding and originally attributed to a cause that turned out not to exist. Tracked in #37, with #25 as the leading candidate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The build and release branch for v1.0.0 — the corpus rebuild this work has been leading to. It will accumulate more than what is here now; this description tracks what has actually landed on it.
So far: current dependency floors, two
slurm/settings that silently did something other than what they said, and two pieces of guidance that said something the code no longer does. Both of the latter were found while working through the 2026aug20 milestone for things worth fixing before the run rather than after it, which is the only time either is cheap.Dependency floors, raised to what this release is built against
Everything moved together, re-locked with
uv lock --upgrade:>=1.1>=1.5.5>=14>=25>=8.1>=8.4>=2.31>=2.34>=4.66>=4.70>=5>=6.1>=8>=9pubmed-downloaderstays at>=0.0.14,<0.1. 0.0.14 is still the latest release, so the ceiling has not been reached, and the reason for it — we call_extract_articleand_ensure_urls, so any 0.0.x can break us — is unchanged.The floors are now the versions this release is tested against rather than the oldest that might resolve. For a whole-corpus run that is the guarantee worth having: "it picked up something older on your machine" is not a debugging session anyone wants several hours into an export, and the export's SQL leans on version-specific DuckDB behaviour (regexp character classes,
TRY_CAST,PER_THREAD_OUTPUToptions) that the SQL/Python twin tests have only ever run against one version.Transitively this also moves pystow 0.8.19 → 0.9.0, which is what resolves the download cache paths. Checked rather than assumed:
PYSTOW_HOMEstill lands files at<home>/pubmed/{baseline,updates}, unchanged, so existing--data-dirtrees are still found.Two Slurm settings that ignored what you passed
Neither is a new bug; both are things nobody had needed until the rebuild made them load-bearing.
LOAD_MEMORY_LIMITandEXPORT_MEMORY_LIMITcould not be cleared.config.shused:=, so passing either empty silently restored the default — the exact opposite of what passing it empty means, and the same mistakeDUCKDB_TEMP_DIRwas fixed for in #9. That one was fixed; these two were written the same way and missed.It matters now because of what the defaults are.
48GBsits against DuckDB's own cgroup-derived default of ~48.6G under--mem=64G, and200GBsits above ~194G under--mem=256G— so the load's knob does nothing and the export's takes headroom away from the lxml tree and Arrow batches rather than giving it back. Anyone tuning #37 would start by clearing them to establish a baseline, and that would not have worked. Now=on both, and empty is safe the whole way down: the CLI's option is falsy, sodb.connectissues noSET.--sample-sizeis per shard, so validate's Entrez check tracked the export's parallelism. Since theCOPYrewrite the export writes one shard per writer thread, andSHARDSderives from--cpus-per-task— so the export drops from 16 shards to 8, which would have halved the field check from 240 sampled records to 120 on the corpus run whose numbers matter most, with no flag changing to say so.05-validate.sbatchnow dividesVALIDATE_SAMPLE_TOTAL(240, what the pre-2026 runs sampled as 15 × 16) by the shards actually on disk, so runs stay comparable across allocations and a corpus with more shards than the target still samples every shard rather than rounding to zero.Both are pinned by tests over the rule rather than the instance: every clearable setting is parametrized, with a companion asserting each still has a default, and the sample division is checked at 16, 8, 1 and 500 shards. All mutation-checked.
04-export.sbatch's--cpus-per-task=8is deliberately not changed. Raising it would restore 16 shards and match the historical measurement tables, but it also changes the resource shape of the OOM-prone step on its first post-COPYrun — the wrong variable to perturb when #42 exists to measure exactly that. The sample-size fix addresses the actual harm; the shard count differing from those tables is now just something to note when reading them.Two version-pinned claims, re-checked
AGENTS.mdcarried one claim that explicitly asks to be re-tested on a version bump, and one that cannot be:PARTITION_BYforFORMAT JSONis still rejected on 1.5.5 —Binder Error: Unknown option for COPY ... TO ... (FORMAT JSON): "partition_by". So--shardsremains a maximum rather than an exact count, and the note now cites the version the floor pins instead of "≤ 1.5.4".Guidance that outlived its diagnosis
slurm/README.md's "Reading the logged numbers" told you what to conclude from a memory climb: "If both climb together andRSSnever comes down, DuckDB's limit is too high — lowerPUBMED2DB_DUCKDB_MEMORY_LIMITrather than raising--mem." That was written when we believed DuckDB sized its buffer pool from the node. It reads the cgroup (#36), so the default is not the outsized thing that sentence assumed, and a climb that never comes down is as likely to be the memory DuckDB's limit does not govern — the lxml tree, the parsed records, the Arrow batch.It now gives the reader the test rather than the conclusion: lower the limit and re-run, because that moves one half and not the other. If
RSSplateaus lower, it was the buffer pool; if it climbs the same way, lowering further only buys spilling.The same passage now says plainly that the
peak 42.1 GiBreading it is built around has never been explained. It was measured before the cgroup finding and attributed to a cause that turned out not to exist. Which half grows is one of the questions the next full load answers (#37); #25 — the loader holding a whole lxml DOM per file — is the leading candidate for the other half.This is the third thing on this branch that was correct when written and stopped being correct without anyone touching it, after the
SOFT_FIELDSsummary line below and the version-pinned claims above. Worth noticing as a pattern rather than three coincidences: each was a sentence about the code rather than the code itself, and nothing checks those.Verified on real data, not just fixtures
The suite's fixtures are a handful of records, which is the wrong scale to trust a dependency bump on. One baseline file plus one update file — 23,052 articles — through
load → journals → export → validateon the upgraded stack. Load, journal refresh, gzipped export and offline validation all clean. 249 tests pass, 1 skipped, on both CI Pythons.That run also found a bug the fixtures could not, fixed here: the validation summary printed
compared but never fails the run (NLM Catalog source): journal_name, journal_abbrev, pub_date. True of the journal fields; false ofpub_date, which is soft because efetch renders a date where we ship the archival string. The line was right whenSOFT_FIELDSheld only the journal fields and quietly stopped being right whenpub_datejoined them.SOFT_FIELDSis now derived from a reason → fields mapping that the summary iterates, so a field cannot be added to one without declaring the other.Worth knowing before the corpus run
The real-data run produced the first measurement of the
month-formatwarning's volume: 5 warnings in 23,052 records (~0.02%), frompub_monthvalues like"Sep-Oct 01"(a month range carrying a day),"1st Quarter"and"Jun-Jun". All are passed through verbatim by design, andvalidateflagging them is the checker working as intended rather than a defect — but it means a corpus run should expect aWARNstatus from this check, not aPASS, so don't setVALIDATE_FAIL_ON_WARN=1expecting a clean pass. Recorded on #43, which it also widens: the oddpub_monthshapes are not just the"Dec-1999 Jan"case that issue was filed for.Before the run
Not code — decisions and environment checks that are cheap now and expensive once a multi-hour job is in flight. The milestone's other open issues (#11, #28, #32, #37, #39) genuinely are "run it and read the logs" and need nothing here.
export_jsondeletes the existing shards before writing new ones, so an export that dies partway takes the current good one with it — and it is the whole-corpus job on a big node, where Slurm will not grow--memmid-flight.EXPORT_DIR=data/json-$(date +%Y%m%d) ./slurm/submit.sh export, then the same forvalidate, then swap. Also keeps the old export around for Confirm the pub_year backfill at corpus scale after the next rebuild #28's query.--time. Load is running ~15x slower than slurm/README.md claims — re-measure #11 measured ~91 s/file; a full baseline year is ~1,300 files, so ≈33 hours against a 24-hour limit. A killed load resumes cleanly —needs_loadskips what is already processed — so this is worth knowing rather than fixing, but not at hour 24.32GB/160GB) to get a signal for Tune PUBMED2DB_DUCKDB_MEMORY_LIMIT against a real corpus load #37, or cleared (LOAD_MEMORY_LIMIT=) for a clean baseline. The current values are the one option that teaches nothing. Tune PUBMED2DB_DUCKDB_MEMORY_LIMIT against a real corpus load #37 now spells out what the run should watch for, and that lowering the limit is itself the experiment that says which half of the process is growing./local/scratch/duckdb_tmpexists onht1—config.sh's default spill directory. The export has a fallback, but discovering it through the fallback is not the plan.NCBI_EMAIL(submit.shrefuses an onlinevalidatechain without it) andNCBI_API_KEYif available — it takes validate from 3 to 10 req/s, roughly 35 minutes down to 11.uv runhonours the lockfile — butmainresolves duckdb 1.5.4 and this branch resolves 1.5.5, and only one of those is what the suite now tests.