Skip to content

Add babel-explorer: a CLI for querying Babel cross-references via DuckDB and NodeNorm - #1

Closed
gaurav wants to merge 113 commits into
mainfrom
basic-implementation-in-uv
Closed

Add babel-explorer: a CLI for querying Babel cross-references via DuckDB and NodeNorm#1
gaurav wants to merge 113 commits into
mainfrom
basic-implementation-in-uv

Conversation

@gaurav

@gaurav gaurav commented Dec 3, 2025

Copy link
Copy Markdown
Collaborator

Introduces babel-explorer, a CLI tool to query Babel intermediate files (Parquet) via DuckDB and NodeNorm. BabelDownloader handles caching and freshness, BabelXRefs handles querying, NodeNorm handles label enrichment, and cli.py wires them together with Click. Three commands: xrefs, ids and test-concord.

Endpoint configuration

Babel and NodeNorm endpoints are read from .env rather than hardcoded, so the repository ships only public URLs. BABEL_URL, BABEL_LOCAL_DIR, BABEL_CHECK_DOWNLOAD, NODENORM_URL and BABEL_ALLOW_VERSION_MISMATCH each have a matching command-line option, with precedence running flag > environment variable > .env > built-in default.

The committed .env.example carries the public Babel URL only, with a note telling Translator team members to contact the Babel developers for the Translator-specific URL.

Babel version handling

The release behind BABEL_URL is resolved from VERSION.txt, falling back to the final URL path segment for older trees that predate it, so latest/ resolves to whichever release it currently points at.

BABEL_LOCAL_DIR holds one Babel release at a time. When the release changes, last_checked is cleared from the .meta sidecars under <local_dir>/duckdb/ so the existing ETag path re-checks each cached file and re-downloads only what changed — the Parquet files themselves are never deleted, and the stored ETag is kept so an unchanged file costs one HEAD rather than a fresh multi-gigabyte download. Partial .tmp downloads are deleted, since they resume by byte offset with no If-Range validation and would otherwise splice two releases into one corrupt Parquet. This keeps Concord.parquet and Identifiers.parquet from being read together across two different Babel releases, which is the failure ETag alone does not prevent.

xrefs fails when NodeNorm's status endpoint reports a different babel_version than the Babel being queried, since labels and cliques would not match the cross-references. --allow-version-mismatch overrides it. The check runs only where NodeNorm is actually consulted — that is, under --labels. --recurse is served entirely by one WITH RECURSIVE DuckDB query and never touches NodeNorm, so it does not trigger the check.

WIP:

Blocked on Babel/NodeNorm deployments

The shipped default (BABEL_URL=https://stars.renci.org/var/babel/latest/) does not work end to end yet, because public Babel releases do not publish the DuckDB Parquet files. babel-explorer reports this explicitly rather than failing mid-download, but these still need doing:

  • Build a new public Babel that includes the DuckDB files (duckdb/Concord.parquet, duckdb/Identifiers.parquet), then confirm the shipped BABEL_URL default works end to end.
  • Publish the current Babel to its public endpoints.
  • Update NodeNorm Dev (https://nodenormalization-sri.renci.org/) to the latest Babel. Its status endpoint currently reports 2025sep1, so xrefs --labels fails the version check against any current Babel unless --allow-version-mismatch is passed.
  • Add a BABEL_URL repository secret so CI integration tests run against a Babel that publishes the Parquet files. Without it the 24 Parquet-dependent integration tests skip.

Linting

The repository had no [tool.ruff] section, so ruff ran with its default rule set and never checked import ordering. Rules are now E, F, I (import sorting) and UP (pyupgrade), with E501 left to the formatter and *.md excluded (ruff 0.16+ reformats Python inside Markdown code blocks). Line length stays at ruff's default of 88 rather than Babel's 120, which would have reflowed 12 of 15 files for no correctness gain.

CI passes --output-format github so lint failures annotate the diff inline, and keeps using uv run ruff rather than astral-sh/ruff-action — uv already resolves the ruff pinned in uv.lock, so CI and local runs share a version without extra plumbing. CLAUDE.md and README.md now say explicitly to run ruff check and ruff format before committing.

Smaller fixes folded in

  • ids gains --labels, so identifier records can carry NodeNorm labels instead of only raw Parquet columns. The label lands in a nodenorm_label field rather than label, because Identifiers.parquet has a label column of its own that would otherwise overwrite it in json/tsv/csv output.
  • --paths with --format json/tsv/csv is now rejected. It previously ignored the flag and emitted the full recursive cross-reference list, which looks like a successful --paths run but is not one.
  • The test data directory is now removed once all xdist workers finish. addopts = "-n auto" made every run parallel, and the old teardown was guarded on a "master" worker that never exists under xdist, so data/test/ survived every run.
  • .idea/ is gitignored.

Review fixes

A code review over the full branch turned up six defects, each fixed in its own commit with a regression test:

  • --recurse triggered the NodeNorm version check it no longer needs. Recursion moved into a single DuckDB query, but the guard still read labels or recurse, so plain xrefs … --recurse failed outright against the public NodeNorm (still on 2025sep1) unless --allow-version-mismatch was passed.
  • ids --labels silently dropped the NodeNorm label. Identifiers.parquet's own label column overwrote it when the record was flattened, so json/tsv/csv emitted the Babel label under label and lost the NodeNorm one; the console printed label= twice. The dataclass field is now nodenorm_label.
  • A version change forced a full re-download instead of an ETag re-check. Deleting the .meta sidecar skips the conditional-GET path entirely, so every latest/ rollover pulled Concord (~626 MB) and Identifiers (2 GB+) in full even when byte-identical — the opposite of the documented intent. Only last_checked is cleared now.
  • A stale .tmp could splice two releases into one corrupt Parquet. Resume is by byte offset with no If-Range; Ctrl-C is a BaseException and so escaped the tmp cleanup, leaving a partial file that the next run (after a release rollover) would append foreign bytes onto. .tmp files are now removed on a version change.
  • The HTTP 416 fast path poisoned its own cache entry, persisting the 416 error response's headers as the file's metadata. It now HEADs for the real ones.
  • The integration skip probe did not normalise BABEL_URL's trailing slash, so a slashless URL probed .../latestduckdb/Concord.parquet, 404'd, and silently skipped the whole integration suite — indistinguishable from the expected skip. An unreachable Babel server now skips rather than erroring out of the fixture.

Testing

220 unit tests pass (261 collected; the remainder are integration). Integration tests run against whatever BABEL_URL points at and skip when that release does not publish the Parquet files.

Verified against the live servers: the public default produces the missing-Parquet error; the internal latest/ resolves to 2026jul22 via VERSION.txt; a 2025nov192026jul22 flip expires the .meta sidecars while leaving Concord.parquet in place; the NodeNorm skew check fires before any download; and a full xrefs query returns cross-references end to end.

gaurav and others added 21 commits December 2, 2025 15:37
- Add IdentifierRecord dataclass to babel_xrefs.py (resolves TODO)
- Add 89 tests across 3 files: test_downloader (26), test_babel_xrefs (31), test_nodenorm (23)
- Unit tests (71) use mocks and run without network; integration tests (18) use real downloads/APIs
- Add session-scoped fixtures in conftest.py for shared Parquet file downloads
- Parametrize integration tests over tests/data/valid_curies.txt for easy expansion
- Add integration and slow pytest markers to pyproject.toml
- Update CLAUDE.md and README.md with testing documentation

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request implements a basic version of babel-explorer in Python using the uv package manager. It's a tool for querying Babel intermediate files to understand why biological/chemical identifiers are considered equivalent. The implementation includes a downloader for large Parquet files with MD5 validation and resume support, NodeNorm API integration for label enrichment, DuckDB-based cross-reference querying, and a Click-based CLI.

Changes:

  • Initial project structure with uv-based package management (pyproject.toml, Python 3.11+)
  • Core functionality: BabelDownloader with streaming downloads and MD5 validation, NodeNorm API client with LRU caching, BabelXRefs for DuckDB-based Parquet queries
  • CLI with three commands: xrefs, ids, and test-concord
  • Comprehensive test suite with 80 tests split between unit tests (mocked) and integration tests (real network calls)

Reviewed changes

Copilot reviewed 15 out of 19 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
pyproject.toml Project configuration with dependencies (click, duckdb, requests, tqdm) and pytest markers
.python-version Specifies Python 3.11 requirement
.gitignore Excludes /data directory for downloaded files
README.md User documentation with setup, usage examples, and testing instructions
CLAUDE.md AI assistant guidance documentation (contains outdated wget reference)
src/babel_explorer/cli.py Click-based CLI with xrefs, ids, and test-concord commands
src/babel_explorer/core/downloader.py Streaming file downloader with MD5 validation and resume capability
src/babel_explorer/core/nodenorm.py NodeNorm API client for identifier normalization
src/babel_explorer/core/babel_xrefs.py DuckDB-based cross-reference query engine (has frozen dataclass bug)
tests/conftest.py Session-scoped pytest fixtures for shared test resources
tests/constants.py Shared test constants and CURIE loader utility
tests/data/valid_curies.txt Parametrized test data (one CURIE)
tests/test_downloader.py 26 tests for BabelDownloader (22 unit, 3 integration, 1 slow)
tests/test_nodenorm.py 23 tests for NodeNorm (18 unit, 5 integration)
tests/test_babel_xrefs.py 31 tests for BabelXRefs (22 unit, 8 integration, 1 slow)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/babel_explorer/core/nodenorm.py Outdated
Comment thread src/babel_explorer/core/downloader.py Outdated
Comment thread src/babel_explorer/cli.py Outdated
Comment thread pyproject.toml Outdated
Comment thread src/babel_explorer/core/downloader.py Outdated
Comment thread src/babel_explorer/core/babel_xrefs.py Outdated
Comment thread src/babel_explorer/core/babel_xrefs.py Outdated
Comment thread src/babel_explorer/core/nodenorm.py Outdated
Comment thread CLAUDE.md Outdated
gaurav and others added 4 commits March 2, 2026 17:35
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
gaurav and others added 13 commits August 14, 2026 18:46
`addopts = "-n auto"` means every run is parallel, and the session fixture's
teardown was guarded on being the "master" worker -- which never happens under
xdist. data/test/ therefore survived every run, contrary to the comment saying
it was removed so the next run starts fresh.

Move the cleanup to pytest_sessionfinish, which the xdist controller runs after
all workers exit. That removes the race the guard existed to avoid (gw0 deleting
Concord.parquet while gw5 still reads it) without disabling cleanup, and still
fires on a non-parallel run, where there is no worker either.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
--paths has a renderer only for the console format. With --format json, tsv or
csv the flag was silently ignored and the full recursive cross-reference list
was emitted instead, which looks like a successful --paths run but is not one.

Fail with a usage error naming the alternative, checked before anything is
downloaded so the mistake costs nothing. Emitting paths as structured records
would be a feature rather than a fix; nothing asks for it yet.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`ids` had no NodeNorm integration at all, so IdentifierRecord output carried
only the raw Identifiers.parquet columns and there was no way to see what a
CURIE actually refers to without a second xrefs or test-concord call.

IdentifierRecord grows a label field, populated from NodeNorm when
--labels is passed, and rendered in double quotes immediately after the CURIE
per the console output convention. As with xrefs, the Babel version check runs
only when labels are requested, since that is the only time NodeNorm is
consulted.

An absent label is omitted from serialized output rather than emitted as an
empty string, matching the console convention and keeping TSV/CSV columns
stable for runs that did not ask for labels.

Also escape ids console output as Rich markup: Parquet values are arbitrary
text and a stray bracket would otherwise be swallowed as a style tag.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The `CURIE "label"` console convention had four independent implementations:
_fmt_label and _curie_str in cli.py, an inline copy in each of the xrefs and
test-concord console loops, and a hand-rolled escape in IdentifierRecord.__str__
that had already drifted (it escaped quotes but never rich markup).

formatting.py now owns it via escape_label(), curie_with_label() and
format_identifier_record(), so the convention and its escaping rules are
defined once. IdentifierRecord loses the console __str__ it should never have
carried in core/, and hl_curie/hl_curie_at_depth collapse into one depth-based
function — the boolean variant was just depth 0 or None, and every call site
was branching between the two.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
write_records took its CSV/TSV field names from the first row alone, so any run
where one record carried a label and another did not raised a ValueError inside
DictWriter on the first record with an extra key. This was already reachable
via `ids --labels` whenever NodeNorm knew some CURIEs but not others.

Field names are now the union of keys across all rows, with restval="" filling
the gaps. The omit-an-absent-label rule also moves off the literal field name
"label" and onto any field ending in it, so LabeledCrossReference's
subj_label/obj_label follow the same convention that ids already did — they
were being emitted as "" in JSON and TSV.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three N+1 patterns dominated runtime on anything larger than a toy query:

- Every labelled CURIE cost its own get_normalized_nodes round-trip, so
  `xrefs --labels --recurse` over a 500-CURIE clique issued ~500 sequential
  HTTPS requests. NodeNorm.normalize_curies() now prefetches a whole batch
  (100 CURIEs per request) and the per-CURIE accessors serve from cache.
- Multi-CURIE `xrefs` ran one full scan of the multi-gigabyte Concord.parquet
  per CURIE. One scan now matches every CURIE, with results bucketed back into
  the per-CURIE cache; a CURIE with no cross-references caches an empty list so
  it is not rescanned.
- _print_paths rebuilt the undirected neighbour map for each of the C(n,2)
  pairs, and build_depth_map built the same structure a third time. All three
  share one build_adjacency().

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
parse_duration spent 39 lines and four separately-worded error messages on
what one regex rejects in a single branch: empty, negative, and non-integer
values now share one message, and the bare-seconds path stops duplicating the
unit-suffix path.

BabelDownloader and NodeNorm each hand-rolled the same lazy-once cache as a
value field plus a _resolved flag, the flag existing only because the resolved
value may legitimately be None. functools.cached_property caches None too, so
both collapse to a single property.

Also extracts _write_meta(), which the tier-2 ETag refresh had been inlining
alongside _save_meta, and drops two parameters no caller ever passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Scoped to node_modules/ rather than /web so that frontend source added under
web/ is still tracked — .gitignore already anticipates web/src/lib/.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Recursive expansion moved into a single WITH RECURSIVE DuckDB query, so
--recurse no longer consults NodeNorm at all; only --labels does. Keeping
`labels or recurse` made plain `xrefs ... --recurse` fail outright against
the public NodeNorm, which is still built from an older Babel release.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Identifiers.parquet has its own `label` column, which from_row() puts into
extra_fields. record_to_dict() applies extra_fields after the dataclass
fields, so `ids --labels --format json` emitted the Babel label under `label`
and dropped the NodeNorm label entirely; console output printed `label=`
twice. Rename the dataclass field to `nodenorm_label` so both survive.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BabelDownloader appends a trailing slash to url_base, but the integration
skip probe joined BABEL_URL and the file path directly. A BABEL_URL without
a trailing slash HEADed ".../latestduckdb/Concord.parquet", got a 404, and
silently skipped the whole integration session — indistinguishable from the
expected skip on a public release. Also skip rather than error when the
Babel server is unreachable.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sync_cache_version() deleted the .meta sidecars, but get_downloaded_file()
only enters the ETag branch when a sidecar exists — so every release rollover
re-downloaded Concord (~626 MB) and Identifiers (2 GB+) in full even when
byte-identical, the opposite of what the docstring claimed. Clear only
last_checked and keep the ETag, so unchanged files cost one HEAD.

Also delete partial .tmp downloads on a version change: they are resumed by
byte offset with no If-Range validation, so a Ctrl-C (a BaseException, which
the tmp cleanup in get_downloaded_file does not catch) followed by a release
rollover would append the new release's bytes onto a prefix of the old one
and land a corrupt Parquet that passes every later freshness check.

And on HTTP 416, HEAD for the file's real headers instead of returning the
416 response's own — those describe the error body, and persisting them as
the file's metadata poisoned the very cache entry the fast path just
confirmed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@gaurav gaurav changed the title Add CLI for querying Babel cross-references via DuckDB and NodeNorm Add babel-explorer: a CLI for querying Babel cross-references via DuckDB and NodeNorm Aug 18, 2026
@gaurav
gaurav requested a balanced review from Copilot August 18, 2026 19:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 20 out of 25 changed files in this pull request and generated 4 comments.

Suppressed comments (7)

tests/test_babel_xrefs.py:400

  • get_curie_xref is now a regular method backed by BabelXRefs._xref_cache; it has no cache_clear attribute. Every integration test containing this call will raise AttributeError as soon as the Parquet fixture stops skipping. Replace all such calls with a supported cache-reset mechanism (or add a public reset helper).
    babel_xrefs.get_curie_xref.cache_clear()

src/babel_explorer/core/downloader.py:347

  • HTTP 416 means only that the requested range is unsatisfiable; it does not prove the local .tmp is complete. If a stale partial is larger than the current remote object, this branch returns success and get_downloaded_file promotes corrupted bytes to the final Parquet file. Compare the temporary file size with the HEAD Content-Length; delete and restart when they differ.
                    if response.status_code == 416:
                        self.logger.info(f"File already complete: {local_path}")
                        # The 416 headers describe the error body, not the file; saving
                        # them as this file's metadata would record a bogus
                        # content_length and force a full re-download on the next check.
                        head = requests.head(url, timeout=self.timeout)
                        head.raise_for_status()
                        return head.headers

.github/workflows/ci.yml:39

  • Repository secrets are not exposed as environment variables automatically. Even after adding the planned BABEL_URL secret, this step will keep using the public default and skip the Parquet-dependent integration suite. Pass the secret into the test process, retaining the public URL as a fallback when it is unset.
      - run: uv run pytest -v -m "integration and not slow"

src/babel_explorer/formatting.py:109

  • This renderer does not follow the repository’s console-label convention: it emits curie='A:1', nodenorm_label="label" instead of placing "label" immediately after the CURIE. Route the CURIE and label through curie_with_label() (formatting.py:87-97), then render the remaining Parquet fields separately.
    parts = [f"curie={record.curie!r}"]
    if record.nodenorm_label:
        parts.append(f'nodenorm_label="{escape_label(record.nodenorm_label)}"')
    parts.extend(f"{name}={value!r}" for name, value in record.extra_fields)

FUTURE.md:6

  • Batch NodeNorm lookup is implemented by NodeNorm.normalize_curies() and already used by all enrichment paths, so issue #12 is no longer future work. Remove this completed item and close/update the issue to keep the roadmap accurate.
- [#12](https://github.com/TranslatorSRI/babel-explorer/issues/12) — Batch NodeNorm lookups to reduce N round-trips when `--labels` is set

README.md:42

  • The version check does not run for plain xrefs; it runs only when NodeNorm is consulted by xrefs --labels or ids --labels. The current wording incorrectly tells users that every xrefs query can be rejected and omits the equivalent ids behavior.
`xrefs` refuses to run when NodeNorm was built from a different Babel release than the one being
queried, since the labels and cliques would not match the cross-references. Pass
`--allow-version-mismatch` to override.

src/babel_explorer/cli.py:307

  • For a single CURIE, --paths cannot produce a pair, but this is checked only inside _print_paths after the recursive DuckDB query has scanned the large Concord file and expanded the component. Reject fewer than two CURIEs here, before creating the downloader, to avoid an expensive query that can only print a warning.
    if paths:
        # Checked before anything is downloaded. Only the console renderer knows how to
        # lay out paths; the other formats would silently emit the full recursive xref
        # list instead, which looks like a successful --paths run but is not one.
        if fmt != "console":
            raise click.UsageError(
                f"--paths is only supported with --format console, not --format {fmt}. "
                f"Drop --paths to emit the full cross-reference list as {fmt}."
            )
        recurse = True

Comment on lines +185 to +187
identifier_parquet = self.downloader.get_downloaded_file(
"duckdb/Identifiers.parquet"
)
except OSError:
cached_version = None

if cached_version and cached_version != version:
Comment thread src/babel_explorer/core/downloader.py Outdated
Comment on lines +239 to +243
except requests.RequestException as e:
self.logger.warning(
f"HEAD request failed for {url}: {e}; assuming file is current"
)
return True
Comment on lines +437 to +444
# Download to a sibling .tmp file, then atomically replace the final destination.
# This ensures the final file is never partially written.
tmp_path = local_path_to_download_to + ".tmp"
try:
response_headers = self._download_with_retry(
url_to_download, tmp_path, chunk_size
)
os.replace(tmp_path, local_path_to_download_to)
gaurav and others added 4 commits August 31, 2026 15:55
Five ways the downloader could end up with a corrupt or stale
`duckdb/*.parquet` that then passed every later freshness check, because
`_save_meta` stamps whatever landed on disk with the *correct* remote ETag.
Once that happens the damage is permanent until the Babel release changes.

- A `.tmp` left by a killed process was resumed by byte offset. The only way
  to reach the download at all is that the remote bytes changed, so the
  server appended the new file's tail to the old file's prefix. Leftover
  `.tmp` files are now discarded before a download starts, and cleaned up on
  `BaseException` so a Ctrl-C leaves nothing resumable behind.
- Resumes within a run now send `If-Range` with the validator from the
  response they started writing from, so a file rebuilt mid-download restarts
  (HTTP 200, already handled) instead of splicing.
- HTTP 416 was read as "already complete", but it is also what the server
  returns when the remote file *shrank* below the resume offset. The local
  size is now checked against the remote `Content-Length` first.
- A stream that ended short of `Content-Length` without raising was promoted
  as complete. It now raises `IncompleteDownloadError` and is retried.
- A failed HEAD returned "unchanged", and the caller then refreshed
  `last_checked`. One flaky HEAD could therefore pin the previous release's
  Parquet as freshly validated for the whole freshness window, right after
  `sync_cache_version` cleared `last_checked` for a new release.
  `_etag_matches` becomes `_remote_unchanged` and returns `None` for "could
  not check": the cached file is still used, but nothing is restamped.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`record_to_dict` dropped any key ending in `label` whose value was falsy, to
implement the "omit an absent label rather than emit an empty one" convention.
But `extra_fields` is flattened in first, so Identifiers.parquet's own `label`
column — and any other `*_label` column Babel adds — was dropped whenever it
was empty or NULL. `ids --format json` then emitted some records with a
`label` key and some without, and a consumer doing `row["label"]` got a
KeyError.

The rule only ever meant the NodeNorm-derived labels, so name them:
nodenorm_label, subj_label, obj_label.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Records why a leftover `.tmp` is never resumed across runs, why in-run resumes
are conditional, where the two size checks sit, and why a failed HEAD does not
refresh `last_checked` — so none of it gets "optimised" back into a cross-run
resume or a fail-open freshness stamp.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The delete in `get_downloaded_file` is the safety guarantee; the sweep in
`sync_cache_version` is housekeeping for files that are never re-downloaded
and so never reach it. Neither covers the other's case, and dropping the wrong
one reintroduces silent Parquet corruption, so say which is which.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@gaurav gaurav closed this Sep 1, 2026
gaurav added a commit that referenced this pull request Sep 1, 2026
- nodenorm.py: Identifier.biolink_type str→list[str] to match NodeNorm API
- nodenorm.py: get_clique_identifiers returns [] instead of None; add return type annotation
- nodenorm.py: log debug message when get_identifier finds no exact match
- cli.py: parse_duration return type int|float; join biolink_type list for display
- tests: update assertions for new biolink_type type; add test-concord edge cases
  (unknown CURIE producing no output, multiple CURIEs queried independently)
- ci.yml: add workflow_dispatch trigger and integration-test job

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
gaurav added a commit that referenced this pull request Sep 1, 2026
Source files:
- nodenorm.py: module, Identifier class/from_dict, NodeNorm class/__init__/
  get_identifier/normalize_curie/get_clique_identifiers
- babel_xrefs.py: convert # comment to module docstring; CrossReference class/
  from_tuple/curies property; LabeledCrossReference class; IdentifierRecord.__str__;
  BabelXRefs class/__init__/get_curie_xref
- downloader.py: module, BabelDownloader.__init__, get_output_file
- cli.py: cli() group, test_concord() command

Test files (class docstrings only):
- test_babel_xrefs.py: TestCrossReference, TestLabeledCrossReference,
  TestIdentifierRecord, TestBabelXRefsInit
- test_nodenorm.py: TestIdentifier, TestNodeNormInit, TestNormalizeCurieMocked,
  TestGetIdentifierMocked, TestGetCliqueIdentifiersMocked

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
gaurav added a commit that referenced this pull request Sep 1, 2026
…D, type fixes

- nodenorm.py: Identifier is now frozen=True; rewrite from_dict as one-shot
  constructor to avoid post-construction mutation of lru_cache'd objects
- nodenorm.py: remove **kwargs from get_clique_identifiers — unhashable and unused,
  would raise TypeError if any kwarg was ever passed
- downloader.py: download to .tmp then os.replace() so the final file is never
  partially written; clean up .tmp on failure
- downloader.py: _etag_matches returns True (fail open) on HEAD network error
  instead of False, avoiding spurious 2GB re-downloads on transient failures
- cli.py: add nodenorm_url: str annotation in xrefs and test_concord; move
  test_concord inline comment to docstring
- tests: update test_returns_false_on_request_error → test_returns_true_on_request_error
- FUTURE.md: track CLI option deduplication refactor

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
gaurav added a commit that referenced this pull request Sep 1, 2026
- list→tuple on Identifier and LabeledCrossReference fields so frozen
  dataclasses are hashable (was a TypeError crash in get_curie_xrefs)
- NodeNorm(''): add early return in normalize_curie so empty URL truly
  skips all network calls as documented
- BabelDownloader: auto-append trailing slash to url_base so urljoin
  can't silently drop path segments
- CI: fix push trigger branch master → main
- Remove dead get_downloaded_dir method (lru_cache + NotImplementedError)
- parse_duration: reject negative values with a clear BadParameter error

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
gaurav added a commit that referenced this pull request Sep 1, 2026
…kDB and NodeNorm (#20)

babel-explorer is a CLI for asking Babel *why* two identifiers are
considered the same thing. It reads Babel's intermediate Parquet files
through DuckDB and, optionally, enriches the results with labels from
NodeNorm. `BabelDownloader` handles caching and freshness, `BabelXRefs`
handles querying, `NodeNorm` handles labels, and `cli.py` wires them
together with Click.

Supersedes #1, which GitHub closed and refused to reopen after this
branch's history was rewritten.

Closes #12.

## What's here

Three commands:

- **`xrefs`** — cross-references for one or more CURIEs. `--recurse`
expands transitively through a single `WITH RECURSIVE` DuckDB query;
`--paths` shows the shortest paths connecting the given CURIEs;
`--labels` adds NodeNorm labels and Biolink types.
- **`ids`** — identifier records from `Identifiers.parquet`, with
`--labels`.
- **`test-concord`** — compare a proposed concordance change against
NodeNorm's current cliques.

`xrefs` and `ids` also take `--format json|tsv|csv` for machine-readable
output. `--paths` is console-only, and both of its preconditions — a
console format, and at least two CURIEs — are checked before anything is
downloaded. Getting either wrong otherwise costs a multi-gigabyte
`Concord.parquet` download and a full recursive query before the run is
rejected, because `--paths` implies `--recurse`.

Failures from the two services this tool talks to are reported as errors
rather than tracebacks. `MissingBabelFileError` explains that a release
does not publish the DuckDB files; `requests.RequestException` reaching
the top means NodeNorm, since the downloader handles its own network
failures.

## Configuring which Babel release to query

A Babel release is addressed as a **releases directory plus a version**,
which is how both the public and internal trees are actually laid out —
one subdirectory per release, plus a `latest/` symlink:

| Variable | CLI option | Default |
|---|---|---|
| `BABEL_RELEASES_URL` | `--babel-releases-url` |
`https://stars.renci.org/var/babel/` |
| `BABEL_VERSION` | `--babel-version` | `latest` |
| — | `--babel-url` | *(overrides both)* |

Pinning a release is a one-word change rather than a URL edit, which
matters because pinning is the honest fix for a NodeNorm version
mismatch. Precedence runs **flag > environment variable > `.env` >
built-in default**, and only public URLs are committed.

`--babel-url` takes a complete URL for a tree that does not follow that
layout. It is **command-line only, with no `envvar=`, deliberately**:
two variables already feed the composed URL, and a third that silently
outranked both would make "which release am I actually querying?"
unanswerable from the environment alone. `cli()` warns if the
pre-refactor `BABEL_URL` is still set, so a stale `.env` fails loudly
instead of silently pointing somewhere else.

`compose_babel_url` lives in `core/downloader.py` rather than `cli.py`
because `tests/constants.py` needs the identical composition and must
not import Click to get it.

The committed template is `env.default` — visible in a plain `ls`,
unlike a dotfile.

## Babel version handling

The release is resolved from `VERSION.txt`, falling back to the final
URL path segment — which under this scheme is exactly `BABEL_VERSION`,
so a pinned release still resolves when `VERSION.txt` is unreachable,
while `latest` yields `None` as before.

`BABEL_LOCAL_DIR` holds one release at a time. When it changes, only
`last_checked` is cleared from the `.meta` sidecars, so the existing
ETag path re-checks each file and re-downloads only what actually
changed — the Parquet files are never deleted, and an unchanged file
costs one HEAD rather than a fresh multi-gigabyte download. The
`.babel-version` marker records the release the server *resolved* to
rather than the one requested, so `latest` and an equivalent pinned
version share a cache instead of thrashing it.

That marker is written **after** the cache catches up, not when the
change is spotted. It claims "the local cache holds this release", which
is only true once every cached file has been re-validated against it, so
`_write_version_marker_if_synced()` stamps it once no `.meta` sidecar is
still missing its `last_checked`. Writing it up front would leave a run
interrupted between `Concord.parquet` and `Identifiers.parquet` with a
marker naming the new release over a half-old cache, and the next run
would see it match and skip the refresh entirely. The cost is that a
cached file nobody asks for holds the marker back indefinitely, at one
HEAD per run; that is the honest answer, since the file really is still
from the previous release.

`--check-download never` suppresses re-checks *within* a release, not
across one. `_is_within_freshness()` therefore tests for a missing
`last_checked` **before** the `float("inf")` shortcut — reversed,
`never` would hand back the previous release's Parquet with no network
call at all, under a marker naming the new release, and nothing would
ever notice.

`--labels` refuses to mix a NodeNorm built from one release with
cross-references from another, since the result would be silently wrong
rather than obviously wrong. `--allow-version-mismatch` overrides it;
pinning `BABEL_VERSION` fixes it properly. `--recurse` never consults
NodeNorm, so it does not trigger the check.

## Partial downloads cannot corrupt the cache

A corrupt Parquet here is *permanent*: whatever lands on disk gets
stamped with the correct remote ETag and then passes every later
freshness check. Five routes to that are closed:

- A `.tmp` left by a killed process is discarded before each download
rather than resumed. Resume is by byte offset, the only way to reach the
download at all is that the remote bytes changed, and an orphaned `.tmp`
carries no record of which version its bytes came from. Cleanup catches
`BaseException`, so Ctrl-C leaves nothing resumable behind.
- In-run resumes send `If-Range`, so a file rebuilt mid-download
restarts instead of splicing two versions together.
- HTTP 416 counts as "already complete" only once the local size matches
the remote `Content-Length` — 416 is also what a server returns when the
file *shrank* below the resume offset.
- A stream ending short of `Content-Length` raises
`IncompleteDownloadError` and is retried, rather than being promoted as
complete.
- A failed HEAD returns "unknown", not "unchanged", and no longer
refreshes `last_checked`. One flaky HEAD could otherwise pin the
previous release's Parquet as freshly validated for the whole freshness
window.

`.tmp` files are deleted in two places on purpose; `CLAUDE.md` records
which one is the safety guarantee and which is housekeeping, so neither
gets removed later as redundant.

## Querying

DuckDB connections are ephemeral and in-memory, but "in-memory" is not
"touches no disk": a larger-than-memory query spills, and DuckDB's
default `temp_directory` is `.tmp` in the *current working directory*.
The recursive expansion materialises the whole Concord relation, so
every connection goes through `BabelXRefs._connect()`, which points
`temp_directory` at `<BABEL_LOCAL_DIR>/duckdb-spill/` — the directory
the user already chose to hold multi-gigabyte files, rather than
wherever they happened to be standing.

## What it deliberately does not do

- **Resume a download across runs.** A leftover `.tmp` cannot be proven
to belong to the file being fetched, so it is discarded. Making it safe
means persisting the validator alongside the `.tmp`; scoped in #15.
- **Enforce the no-private-URLs rule outside `env.default`.**
`TestCommittedConfigTemplate` guards the template, but the original leak
came through a default value in `cli.py`. A tree-wide scan is #25.
- **Reuse a DuckDB connection across queries.** A known performance
cost, deferred to #13. (Batching NodeNorm lookups *did* ship —
`normalize_curies()` collapses a clique into one request per 100 CURIEs,
which is why this closes #12.)

## Known limitations at v0.1.0

This ships knowingly non-functional against its own defaults, so people
can try the tool now rather than after the Babel side catches up. The
CLI says so clearly rather than failing mid-download.

- Public Babel releases do not publish `duckdb/Concord.parquet` or
`duckdb/Identifiers.parquet`. **#16 — must close before v1.0.0.**
- NodeNorm dev reports Babel `2025sep1` against public `2025dec11`, so
`--labels` fails the skew check against the defaults. #17
- CI therefore cannot run the 28 Parquet integration tests. #18
- NodeNorm integration tests fail rather than skip when the API is
unreachable. #19

## Testing

261 unit tests and 13 NodeNorm integration tests pass. 28
Parquet-dependent integration tests skip without a Babel release
publishing the files — that is the expected result, not a broken
environment.

Verified end to end against a Translator releases tree: the composed URL
resolves `2026jul22`, downloads `Concord.parquet`, writes a correct
`.babel-version` marker, and returns cross-references for
`MONDO:0004979`. The version-marker sequencing was verified separately
by simulating a `2025dec11` → `2026jul22` change under `--check-download
never`: both Parquet files are re-fetched, and the marker flips only
after the second one lands.

Note that `not slow` is **not** a promise of "small". `Concord.parquet`
is 4.6 GB in `2026jul22` and its tests are not marked `slow`, because
excluding them would leave the non-slow integration set covering nothing
that touches real data. This matters for sizing #18.

## Nothing is blocking this merge

Everything outstanding is tracked in #12, #13, #15, #16, #17, #18, #19
and #25. None of it makes what ships here wrong — #16 and #17 constrain
what the defaults can *do*, and both are stated plainly in the README,
`CHANGELOG.md` and the CLI's own error messages.

<details>
<summary><b>Review history</b> — three review rounds and a history
rewrite. Kept for anyone tracing why a particular line looks the way it
does; the durable conclusions are in the code comments, CLAUDE.md and
the sections above.</summary>

**First review round** found six defects, each fixed in its own commit
with a regression test: `--recurse` triggered the NodeNorm version check
it no longer needed; `ids --labels` silently dropped the NodeNorm label
because Identifiers.parquet's own `label` column overwrote it; a version
change forced a full re-download instead of an ETag re-check; a stale
`.tmp` could splice two releases into one corrupt Parquet; the HTTP 416
fast path persisted the error response's headers as the file's metadata;
and the integration skip probe did not normalise a slashless
`BABEL_URL`, so it probed `.../latestduckdb/...` and silently skipped
the entire integration suite.

**Second review round** found six more, all but one in the downloader's
resume path — the five now described under "Partial downloads cannot
corrupt the cache", plus `record_to_dict` dropping Identifiers.parquet's
own `label` column whenever it was empty, because the omit-when-absent
rule matched on a `label` name suffix rather than the three
NodeNorm-derived field names. That made `label` present on some rows of
a json/tsv/csv run and missing on others.

The first round's `.tmp` fix was narrower than it looked: it swept
`.tmp` files only on a Babel *version* change, which does not cover a
content change within one release or a rebuild in place. The second
round replaced it with an unconditional discard before every download.
Both deletes are kept, for different reasons, which is why `CLAUDE.md`
spells out which is which.

**Copilot review** found four threads and one suppressed comment, all
five real. Two were in the resume path and are folded into the rules
above: a retry sent a bare `Range` whenever no validator was known,
which is exactly the case where a splice cannot be detected afterwards;
and a 416 whose HEAD carried no `Content-Length` was read as "already
complete" when it is equally the answer for a file that shrank. Two
existing tests had encoded the first behaviour by seeding a partial file
with no validator — a state `get_downloaded_file` never produces — and
were rewritten to reach the resume the way production does. The others:
ten `get_curie_xref.cache_clear()` calls left over from an `lru_cache`
that no longer exists, raising `AttributeError` in integration tests
that skip and so never ran; `BABEL_ALLOW_VERSION_MISMATCH` missing from
`env.default`, which survived because the test guarding that rule listed
the settings by hand rather than reading them off the CLI; and this PR's
own batched NodeNorm lookups still listed as future work.

**Third review round** found seven, in five commits. Two were the same
defect from opposite ends and are the reason the version-marker rules
above are stated so explicitly: `_is_within_freshness()` returned `True`
on `float("inf")` before looking at `last_checked`, so the whole
cross-release refresh was a no-op under `--check-download never`; and
the marker was stamped before anything had been re-downloaded, so an
interrupted refresh looked complete to the next run. Both let
`Concord.parquet` and `Identifiers.parquet` be read together across two
Babel builds, which is precisely what the marker exists to prevent. The
other three code findings: `xrefs --paths` rejected a single CURIE only
inside `_print_paths`, after the 4.6 GB download; NodeNorm's
deliberately-uncaught HTTP errors reached the user as a stack trace; and
DuckDB spilled into the working directory.

Two were documentation drifting from the code — `CLAUDE.md` described
`@functools.lru_cache` in three places where none exists (disk caching
by ETag, `cached_property`, and per-instance dicts respectively), and
`CHANGELOG.md` carried a hard-coded test count that was already wrong,
in a repo whose own `CLAUDE.md` says not to record them because they
drift silently and then mislead.

**History rewrite (2026-09-01).** The internal releases URL had been the
hardcoded default from the initial commit until it moved into `.env`,
leaving it in 178 of 180 commits across six branches of a public
repository. Every commit was rewritten to remove it, verified by four
independent checks before anything was pushed, with every changed line
confirmed to be that substitution and nothing else. GitHub then refused
to reopen the affected PRs, because their original head commits no
longer exist — hence #20 here, and #21-#24 replacing #4, #6, #7 and #11.
The rewrite does not un-publish anything: GitHub retains pre-rewrite
objects reachable by SHA, and the tree the URL pointed at still serves
200 unauthenticated. Both are being handled outside this repo.

**Verifying the release caught a stale claim.** `Concord.parquet` is 4.6
GB in `2026jul22`, not the ~626 MB a fixture docstring had claimed, so
`pytest -m "integration and not slow"` — documented in two places as
avoiding 2 GB+ downloads — would in fact pull 4.6 GB. The docs were
corrected rather than the marker, and hard byte figures were dropped
from the fixture docstrings for the same reason the repo already gives
for test counts: they drift silently and then mislead.

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants