Skip to content

Read concord files through one shared function - #986

Draft
gaurav wants to merge 2 commits into
mainfrom
refactor/read-concord-file
Draft

Read concord files through one shared function#986
gaurav wants to merge 2 commits into
mainfrom
refactor/read-concord-file

Conversation

@gaurav

@gaurav gaurav commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

x = line.strip().split("\t") then [x[0], x[2]] appeared at eleven sites across nine modules — the n+1th-copy pattern AGENTS.md asks us to promote rather than extend. read_concord_file() now lives beside read_identifier_file() in babel_utils.py and every caller goes through it.

Call sites routed: model/cliques.py, createcompendia/chemicals.py (×2 — combine_unichem and build_untyped_compendia), protein.py, gene.py, taxon.py, publications.py, anatomy.py, processactivitypathway.py (×2), geneprotein.py.

Three contract choices, each load-bearing

  • Pairs are 2-tuples, not 2-sets. glom() treats them identically (it only iterates the group and has one isinstance branch, for frozenset); remove_overused_xrefs() unpacks for k, v in pairlist and so needs them ordered; and a 2-tuple is roughly half the resident size of a 2-set, which matters in the rules that read concords under mem=512G.
  • File order is preserved. glom() is order-sensitive: which pair merges first decides which later pair a unique_prefixes conflict rejects.
  • A non-blank row with fewer than three fields raises instead of being skipped. glom_from_files skipped them; the other ten sites raised IndexError. Skipping means a concord truncated by a killed job quietly builds smaller cliques, so raising everywhere is the fail-loud choice. No row in any concord under babel_outputs/intermediate or data/babel-1.18 is short, so this changes nothing on real data today.

Two API changes to be aware of

  • ConcordPairFilter narrows from (parts: list[str], infile, dicts) to (pair: tuple[str, str], infile, dicts). Both implementations only ever read columns 0 and 2. Filtering now happens after the whole file is read rather than per line, which is equivalent because dicts is only mutated by the glom() outside that loop — it does change between files, which is why the read stays per file. The same reasoning applies to processactivitypathway's inline UMLS gate; both places say so in a comment.
  • protein.py's per-million-line progress log becomes one line per file, so a 12 h rule keeps a liveness signal.

Also drops dead debug scaffolding this touched: chemicals.build_untyped_compendia carried a DRUGCENTRAL print block that indexed pairs[0] — an IndexError on an empty concord — to print nothing anyone reads.

Verification

The risk here is glom()'s order sensitivity, not the parse. So: replayed anatomy's compute_cliques_for_impact_report over babel_outputs/intermediate/anatomy on origin/main and on this branch — identical clique state, 371,805 members across 178,868 cliques, and identical type assignments.

Plus 11 new unit tests covering the contract above, including the malformed-line behaviour on real rows copied verbatim from intermediate/anatomy/concords/UMLS.

🤖 Generated with Claude Code

`x = line.strip().split("\t")` then `[x[0], x[2]]` appeared at eleven sites
across nine modules -- the n+1th-copy pattern AGENTS.md asks us to promote
rather than extend. read_concord_file() now lives beside read_identifier_file()
in babel_utils and every caller goes through it.

Three deliberate contract choices, each load-bearing:

  - Pairs come back as 2-tuples, not 2-sets. glom() treats them identically
    (it only iterates the group and has one isinstance branch, for frozenset),
    remove_overused_xrefs() unpacks `for k, v in pairlist` and so needs them
    ordered, and a 2-tuple is roughly half the resident size of a 2-set in the
    rules that read concords under mem=512G.
  - File order is preserved, because glom() is order-sensitive: which pair
    merges first decides which later pair a unique_prefixes conflict rejects.
  - A non-blank row with fewer than three fields raises instead of being
    skipped. glom_from_files skipped them; the other eight sites raised
    IndexError. Skipping means a concord truncated by a killed job quietly
    builds smaller cliques, so raising everywhere is the fail-loud choice.
    No row in any concord under babel_outputs/intermediate or data/babel-1.18
    is short, so this changes nothing on real data today.

ConcordPairFilter narrows from (parts: list[str], infile, dicts) to
(pair: tuple[str, str], infile, dicts). Both implementations only ever read
columns 0 and 2. glom_from_files now filters after reading the whole file
rather than per line, which is equivalent because `dicts` is only mutated by
the glom() outside that loop -- it does change between files, which is why the
read stays per file. Same reasoning applies to processactivitypathway's inline
UMLS gate, and both places say so in a comment.

Also drops dead debug scaffolding this touched: chemicals.build_untyped_compendia
carried a DRUGCENTRAL print block that indexed pairs[0] (an IndexError on an
empty concord) to print nothing anyone reads. protein.py's per-million-line
progress log becomes one line per file, so a 12h rule keeps a liveness signal.

Verified by replaying anatomy's compute_cliques_for_impact_report over
babel_outputs/intermediate/anatomy on origin/main and on this commit: identical
clique state, 371,805 members across 178,868 cliques, and identical type
assignments. That is the check that matters, since the risk here is glom()'s
order sensitivity rather than the parse itself.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
read_concord_file changed how combine_unichem derives prefixes_in_file, from a
per-line accumulation to a set comprehension over the returned pairs. Two
RuntimeError guards depend on that value -- an empty concord and a file mixing
prefixes in column 1 -- and neither was tested.

The subject prefix decides whether remove_overused_xrefs() runs for a file, so a
mixed file would silently apply one source's policy to another's rows. Verified
non-vacuous by mutation: deriving the prefix from only pairs[0], the plausible
wrong way to write this, fails the mixed-prefix test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@gaurav
gaurav force-pushed the refactor/read-concord-file branch from dbfb55b to 2608a78 Compare July 30, 2026 05:57
@gaurav gaurav added this to the Babel v1.19 milestone Jul 30, 2026
@gaurav
gaurav marked this pull request as draft July 30, 2026 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

1 participant