Stop redoing per-call setup work, and hand the untyped compendium over as JSONL - #992
Draft
gaurav wants to merge 2 commits into
Draft
Stop redoing per-call setup work, and hand the untyped compendium over as JSONL#992gaurav wants to merge 2 commits into
gaurav wants to merge 2 commits into
Conversation
…per call
write_compendium() constructs a NodeFactory and an InformationContentFactory on
every call, and the chemical build calls write_compendium once per entry in
config.yaml: chemical_outputs -- eight times. Neither construction was cached,
so both did their full setup work eight times per build for byte-identical
results:
- get_biolink_model_toolkit() (util.py) built a bmt Toolkit from
https://raw.githubusercontent.com/biolink/biolink-model/<ref>/biolink-model.yaml
on every call: a network fetch plus a linkml parse of the model. A build
uses exactly one biolink_version, so the cache holds one entry.
- InformationContentFactory.__init__ (node.py) read all of icRDF.tsv --
212 MB, 3,940,399 lines -- calling curies.Converter.compress() per line.
That is ~31.5M compress() calls per chemical build, seven-eighths of them
repeating identical work.
The icRDF load moves to a module-level _load_information_content() so it can be
cached per path; a test or tool pointed at a different icRDF file still gets its
own. The returned dict is now shared between factories, which is safe because
get_ic() only reads it -- self.ic was only ever written during __init__.
Neither function had a test. Adds five for the loader (URL-to-CURIE
compression, get_ic's minimum-across-identifiers and None cases, read-once-per-
path, and per-path keying) plus a network-marked one asserting the toolkit cache
is per version. The read-once test counts real open() calls rather than
asserting object identity, since identity alone would also pass if the loader
returned a shared default.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
partials/untyped_compendium is the handoff between untyped_chemical_compendia
and chemical_compendia -- the largest clique state in Babel, on the order of
20 GB at production scale. It held `f"{set(s)}\n"`, read back with
ast.literal_eval: Python source used as a wire format between two rules, which
is both the slowest deserializer in the standard library and unreadable by
anything that is not Python.
Measured over a replicated anatomy clique state (docs/rust-decision/), the
repr(set) round trip costs 2.4x what JSONL does -- 11.80s against 5.00s at
3,069,240 members. Extrapolated to the chemical pipeline's 256,427,006 CURIEs
that is roughly 9.5 minutes of chemical_compendia's 19,643s, which is more than
an in-process Rust boundary could save there even in principle.
The read and write are extracted into read_untyped_compendium() and
write_untyped_compendium() so the format lives in one place and can be tested
without standing up build_compendia's inputs.
Members are sorted within each line so a clique always serialises to the same
text and two builds' files can be compared with `sort | diff`. Line order is
NOT stable -- it follows set iteration -- and the docstring says so rather than
claiming a reproducibility the code does not deliver; sorting ~100M lines would
cost more than the diffability is worth.
Reading a pre-JSONL partial raises with the fix in the message. Snakemake will
not rebuild a partial that is newer than its inputs, so a tree carried over from
an older checkout arrives here still in the old format, and failing inside
json.loads would name neither the cause nor the remedy.
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.
Three of the five follow-ups from the #984–#988 review. All three are repeated or wasteful work in the hot rules, none needs Rust, and all are behaviour-preserving.
Repeated setup work in
write_compendium(commit 1)write_compendiumconstructs aNodeFactoryand anInformationContentFactoryon every call, and the chemical build calls it 8 times — once per entry inconfig.yaml: chemical_outputs. Neither construction was cached, so both did their full setup eight times per build for byte-identical results:get_biolink_model_toolkit()built abmtToolkit fromraw.githubusercontent.com/biolink/biolink-model/<ref>/biolink-model.yamlon every call — a network fetch plus a linkml parse. A build uses exactly onebiolink_version, so the cache holds one entry.InformationContentFactory.__init__read all oficRDF.tsv— 212 MB, 3,940,399 lines — callingcuries.Converter.compress()per line. That is ~31.5 Mcompress()calls per chemical build, seven-eighths of them repeating identical work.The icRDF load moves to a module-level
_load_information_content()so it can be cached per path — a test or tool pointed at a different icRDF file still gets its own. The dict is now shared between factories, which is safe becauseget_ic()only reads it;self.icwas only ever written during__init__.Neither function had a test. Adds five for the loader plus a network-marked one for the toolkit cache. The read-once test counts real
open()calls rather than asserting object identity, since identity alone would also pass if the loader returned a shared default.The
repr(set)handoff (commit 2)partials/untyped_compendiumis the boundary betweenuntyped_chemical_compendiaandchemical_compendia— the largest clique state in Babel, ~20 GB at production scale. It heldf"{set(s)}\n"read back withast.literal_eval: Python source used as a wire format between two rules, which is both the slowest deserializer in the standard library and unreadable by anything that is not Python.Measured over a replicated anatomy clique state (
docs/rust-decision/), therepr(set)round trip costs 2.4× what JSONL does — 11.80 s against 5.00 s at 3,069,240 members. Extrapolated to the chemical pipeline's 256,427,006 CURIEs that is roughly 9.5 minutes ofchemical_compendia's 19,643 s — more than an in-process Rust boundary could save there even in principle (#988).Read and write are extracted into
read_untyped_compendium()/write_untyped_compendium()so the format lives in one place and is testable without standing upbuild_compendia's inputs. Reading a pre-JSONL partial raises withrm <path>in the message: Snakemake will not rebuild a partial newer than its inputs, so a tree carried over from an older checkout arrives here in the old format, and failing insidejson.loadswould name neither the cause nor the remedy.One thing I got wrong and the test caught: I first claimed sorting members made the file byte-identical across rebuilds. It does not — line order still follows set iteration. The docstring now claims only what is true (
sort | diffworks) and says why sorting ~100M lines is not worth fixing.Also here
A commit from #986's lineage covering
combine_unichem's one-prefix-per-file guards, whichread_concord_filechanged the input to and which had no test.Deliberately not included
Two follow-ups turned out to be much larger than the issue list suggested, and both carry design decisions worth making explicitly rather than burying in this branch:
write_compendiumderiving output paths fromget_config()— 27 call sites across 12 modules, and it derives three output trees (compendia/,synonyms/,metadata/) fromcdir. The open question is what replaces it: a directory argument, three explicit paths, or paths threaded through asparams:the wayreports.snakefile:205-213already does. That choice shapes all 27 sites.generate_pubmed_concords—download_pubmedwrites a done-marker, so the ~1,500-file list is not known at DAG time. Sharding needs Snakemakecheckpoints, of which this repo has zero, plus a merge step for the accumulatingpmid_status. Not something to introduce blind, without being able to run the pipeline.Verification
425 unit tests pass;
ruff check,ruff format,snakefmtclean. Both caches were mutation-tested — removing@cachebreaks the read-once test, and deriving the unichem prefix frompairs[0]alone breaks the mixed-prefix test.🤖 Generated with Claude Code