Skip to content

Add adapter benchmarking and parity testing for reachable_from#63

Merged
cmungall merged 6 commits into
mainfrom
claude/oak-ltv-issue-diagnosis-023vbm
Jul 3, 2026
Merged

Add adapter benchmarking and parity testing for reachable_from#63
cmungall merged 6 commits into
mainfrom
claude/oak-ltv-issue-diagnosis-023vbm

Conversation

@cmungall

@cmungall cmungall commented Jul 3, 2026

Copy link
Copy Markdown
Member

Summary

This PR adds comprehensive benchmarking and correctness testing infrastructure for OAK adapters used in reachable_from validation. It includes a repeatable benchmark suite, deterministic parity tests, and documentation to guide adapter selection.

Key Changes

  • benchmarks/adapter_benchmark.py: New benchmarking tool that measures load time, cold/warm closure performance, ancestor throughput, and memory usage across adapters (simpleobo, pronto, owl, ubergraph). Generates a markdown comparison table. Includes isolated cache handling to make runs comparable across different environments.

  • tests/test_adapter_parity.py: New deterministic test suite that:

    • Generates a synthetic ontology with is_a and part_of relationships
    • Asserts local adapters (simpleobo, pronto, owl) agree on closure results
    • Verifies part_of predicates actually broaden closures
    • Tests remote scope invariants: OLS under-returns (subset), Ubergraph is a merged multi-ontology graph requiring prefix-filtering
  • benchmarks/README.md: Adapter selection guide documenting:

    • Performance/scope tradeoffs for each adapter
    • Two critical failure modes (OLS truncation, Ubergraph over-return)
    • Instructions for running benchmarks with examples
  • benchmarks/results/go_adapter_comparison.md: Reference benchmark results for full GO (51,975 classes), showing:

    • Single-ontology adapters agree on 4,075 is_a descendants
    • Ubergraph returns 35,651 (includes cross-ontology terms)
    • Performance comparison: pronto ~3.6s cold, simpleobo ~10s cold, owl ~145s (no cache), ubergraph ~2s warm
  • project.justfile: Added benchmark recipe to regenerate results table via just benchmark

Implementation Details

  • Benchmark uses gc.collect() and time.perf_counter() for accurate timing
  • Cache isolation via environment variables (PYSTOW_HOME, XDG_CACHE_HOME, OAKLIB_CACHE) ensures comparable cold/warm measurements
  • Synthetic ontology is deterministic (ternary tree + cross-links) for reproducible parity tests
  • Tests are marked @pytest.mark.integration for remote scope checks (OLS, Ubergraph)
  • Correctness is guarded deterministically in CI; performance is measured on demand (never asserted)

https://claude.ai/code/session_01HfXcWrUi8hqTeD1exGSZte

…losures

Choosing an OAK adapter for reachable_from closure validation affects both
correctness and performance, and the wrong choice fails silently (OLS truncates
-> false rejects, #55; Ubergraph is multi-ontology -> false accepts, #58; owl:
is O(V*E), #57). This adds two complementary pieces:

- tests/test_adapter_parity.py: deterministic, cache/timing-independent
  behavioral invariants safe for CI.
  * Offline: simpleobo / pronto / owl must agree on is_a and is_a+part_of
    closures over a synthetic ontology (also documents that pronto needs a
    part_of Typedef declaration).
  * Integration (network): OLS closure is a subset of ground truth (does not
    over-return); Ubergraph descendants span multiple ontologies and its
    GO-scoped subset is strictly smaller (must prefix-filter).

- benchmarks/: standalone, cache-isolated timing harness (adapter_benchmark.py)
  that reports cold+warm closures per adapter and regenerates the comparison
  table; README.md with the adapter selection guide and the two failure modes;
  results/go_adapter_comparison.md as a captured reference run. Wired up as
  `just benchmark`.

Performance is never asserted (it depends on download/HTTP caches); correctness
is. Full suite green: pytest, mypy, ruff.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HfXcWrUi8hqTeD1exGSZte
@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @cmungall's task in 1m 51s —— View job


Code Review: Adapter benchmarking + parity testing

  • Read all 5 changed files
  • Analyze parity tests for correctness
  • Analyze benchmark tooling
  • Post review

Overall this is a well-structured, well-documented PR. The separation of concerns — deterministic correctness in CI (test_adapter_parity.py), informational timing on demand (adapter_benchmark.py) — is exactly right, and the docstrings/README are genuinely useful as adapter-selection guidance. The synthetic ontology construction is clean and the part_of cross-link (node 39 → node 2, whose is_a ancestors are {12, 3, 0}) correctly forces a strict superset. Nice work.

A few things worth addressing:

🟠 Per-adapter RAM via ru_maxrss is misleading

benchmarks/adapter_benchmark.py:55-56 + :159

resource.getrusage(RUSAGE_SELF).ru_maxrss is the process-wide peak RSS and only ever increases. Since all adapters run sequentially in one process and prior adapters' objects aren't freed, res.rss_mb for adapter N is really "peak RAM after loading adapters 1..N", not adapter N's footprint.

The committed reference results confirm this is off: results/go_adapter_comparison.md:9-12 shows owl=2201 MB followed by ubergraph=217 MB. A monotonic peak can't drop, so those numbers can't have come from a single process run — they look assembled from separate runs. Either:

  • document that the RAM column requires one-adapter-per-process (e.g. run each adapter in its own invocation), or
  • measure a delta (ru_maxrss before/after, or RSS via psutil) and label it "peak delta", or
  • drop the RAM column from the multi-adapter table to avoid over-claiming.

Fix this →

🟡 Reference results appear partly hand-edited

benchmarks/results/go_adapter_comparison.md:11 shows (skipped) for owl's cold(+part_of), but the tool never emits that string — _fmt(None) returns - and the code always computes both predicate sets. Combined with the non-monotonic RAM above, the committed table doesn't match what just benchmark currently produces. Since the file header says "Regenerated by just benchmark", consider regenerating it verbatim, or add a note that it's a curated/annotated snapshot so a future just benchmark diff isn't surprising.

🟡 test_local_adapters_agree — vacuous when only one adapter loads + repeated rebuilds

tests/test_adapter_parity.py:143, :104-118

  • _local_adapters() is called inside every parametrization (3 sources × 2 preds × 2 directions = 12×), re-parsing the OBO/OFN each time. The ontology is tiny so it's not a real cost, but a session-scoped fixture (like synthetic_paths) would be cleaner and consistent with the rest of the file.
  • If owl is skipped and something ever reduced the dict to a single entry, the loop compares the reference against itself and passes trivially. Today simpleobo+pronto are always present so it's safe, but a guard makes the intent explicit:
    if len(adapters) < 2:
        pytest.skip("need ≥2 adapters to check parity")

🔵 Minor / nits

  • Non-table meta lineadapter_benchmark.py:175 emits | root: … | oaklib: … | as a single row with no header/separator, so it renders as a stray pipe-delimited line rather than a table (visible at results/go_adapter_comparison.md:5). A blockquote or bold caption would render more cleanly.
  • OAKLIB_CACHE uses setdefault while the others don't (adapter_benchmark.py:207-209). Intentional? If the goal is full isolation, an inherited OAKLIB_CACHE from the environment would silently defeat it. A short comment on why it's different would help.
  • sample relies on dict insertion order (adapter_benchmark.py:143, :150) — descendants is only populated when label == "is_a", which works because PREDICATE_SETS lists is_a first. Fine, just fragile if the dict order ever changes; a small comment would guard it.

Test coverage

Good deterministic coverage of the core invariants (local parity, part_of broadening, OLS-subset, Ubergraph-merged). One gap worth considering: there's no assertion that the source node is excluded from reachable_from closures (the semantics called out in CLAUDE.md). Adding assert source_node not in results[...] to test_local_adapters_agree would lock that behavior down cheaply.

None of these are blockers — the correctness-critical pieces (parity assertions, scope invariants) are solid. The main thing I'd fix before merge is the RAM measurement / reference-table mismatch, since it's the one place the artifact could mislead a reader about a real number.
claude/oak-ltv-issue-diagnosis-023vbm

Review feedback from claude[bot] on PR #63:

- Per-adapter RAM was measured with process-wide ru_maxrss, so numbers in a
  single multi-adapter run were cumulative/monotonic (and the committed table
  couldn't have come from one process). Each adapter now runs in its own
  subprocess (--worker); ru_maxrss is that adapter's true peak, and cache
  isolation is genuinely per-adapter.
- Downloader was missing a User-Agent and 403'd against the OBO hosts/proxy;
  now sends one (verified against a live fetch), so `just benchmark` works
  end-to-end.
- Meta line rendered as a stray table row -> now a "> Run:" blockquote caption.
- OAKLIB_CACHE set unconditionally like the other cache env vars (with a comment
  on why an inherited value would defeat isolation).
- Documented the is_a-first ordering the ancestor sample relies on.
- test_adapter_parity: local_adapters is now a session-scoped fixture with a
  `len < 2` skip guard so parity can't pass vacuously.

Not applied: the "assert source_node not in closure" suggestion -- this test
drives raw adapter.descendants (reflexive=True by OAK default); source exclusion
is plugin-level and already covered by test_reachable_from_progressive.py.

The committed reference table is regenerated authentically in a follow-up commit.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HfXcWrUi8hqTeD1exGSZte
@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @cmungall's task in 1m 46s —— View job


Re-review after the review-fix commit (ae819cf)

  • Read changed files
  • Verify prior review points were addressed
  • Check for new issues
  • Post updated review

Thanks for the thorough turnaround — the code side of the previous review is genuinely well handled. Verifying each point against ae819cf:

Prior finding Status
Per-adapter RAM via monotonic ru_maxrss ✅ Fixed — each adapter now runs in its own --worker subprocess, so ru_maxrss is that adapter's true peak. Clean solution.
Stray meta line rendered as a table row ✅ Fixed in code — now a > Run: blockquote caption
OAKLIB_CACHE used setdefault ✅ Fixed — set unconditionally, with a comment explaining why
is_a-first ordering the sample relies on ✅ Documented at PREDICATE_SETS and the sample site
Parity test could pass vacuously / repeated rebuilds ✅ Fixed — local_adapters is now session-scoped with a len(adapters) < 2 skip guard
Missing User-Agent → 403 on OBO hosts ✅ Fixed — Request(..., headers={"User-Agent": ...})
source_node not in closure assertion ➖ Reasonably declined — the test drives raw adapter.descendants (reflexive by OAK default); source-exclusion is plugin-level and covered by test_reachable_from_progressive.py. Agreed.

🟠 The reference table was not regenerated — it's now stale relative to the new code

benchmarks/results/go_adapter_comparison.md

The commit message says "The committed reference table is regenerated authentically in a follow-up commit" — but that follow-up isn't on the branch (ae819cf is the tip, and it only touches adapter_benchmark.py + tests/test_adapter_parity.py). So the table still shows the old output and no longer matches what the current code emits:

  • :5 still has the stray | root: … | oaklib: … | caches: isolated | row. The code now emits > Run: … as a blockquote (_render_markdown :197, :205).
  • :7 header still says … | is_a count | RAM |. The code now emits Peak RAM.
  • :11 still shows (skipped) for owl's Cold (+part_of) — the tool never emits that string (_fmt(None)-, and both predicate sets are always computed).
  • :5 caption still reads caches: isolated; the code now emits isolated, per-adapter subprocess.
  • The RAM column (owl 2201 MB → ubergraph 217 MB) is still the old hand-assembled, non-monotonic set — i.e. exactly the number the whole subprocess refactor was meant to make trustworthy, but the file doesn't reflect a real per-subprocess run.

Net: the code fix is correct, but the artifact it's supposed to produce is still the pre-fix version, so a reader diffing just benchmark output against this file will see it change in format on the very first run. Two clean options:

  1. Regenerate it verbatim with the new code (just benchmark, full GO — needs network + the ~145s owl walk), or
  2. If a live full-GO regen isn't practical in CI, add a one-line note at the top that it's a curated snapshot and update at minimum the header (Peak RAM), the > Run: caption, and the (skipped)- cell so the format matches the current tool.

Fix this →

🔵 Minor observations on the new subprocess code

  • _run_worker parses __RESULT__ from stdout (:257-260). Since the worker also prints downloading … / converting … to stdout, prefix-filtering is the right call — but if a worker crashes after those prints without emitting __RESULT__, the fallback error only shows the last 300 chars of stderr/stdout. That's a reasonable degradation; no change needed, just noting the failure path is stdout-order-dependent.
  • args.obo_workdir = tmp is set but never read (main, parent branch) — looks like a vestigial assignment now that _resolve_inputs takes the workdir explicitly. Safe to drop.
  • owl→ofn conversion (owl_path, :123-135) imports pyhornedowl lazily and converts once — good. Worth a note in benchmarks/README.md that the owl adapter requires py-horned-owl and does an RDF/XML→OFN conversion step, since that's a non-obvious dependency for anyone running just benchmark.

Assessment

The correctness-critical pieces (deterministic parity assertions, scope invariants) remain solid, and the RAM-measurement fix is the right design. The one thing to resolve before merge is the stale reference table — the commit promised a regeneration that didn't land, so the committed artifact currently contradicts the tool that's supposed to produce it. Everything else is polish.

Note: I couldn't execute pytest in this environment (command approval denied), so the parity-test verification above is from reading, not a live run.
claude/oak-ltv-issue-diagnosis-023vbm

Replaces the previously hand-assembled reference table (flagged in the PR #63
review) with verbatim output from `adapter_benchmark.py` on full GO (51,975
classes), each adapter in its own subprocess. Peak RAM is now genuinely
per-adapter (owl 2202 MB vs ubergraph 227 MB come from separate processes, so
non-monotonic values are expected and correct).

README: note that Peak RAM is per-subprocess, and that the Cold (+part_of)
column is measured after the is_a calls -- so for index-caching adapters
(simpleobo, pronto) it reflects incremental predicate cost, not a cold start.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HfXcWrUi8hqTeD1exGSZte
@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @cmungall's task in 1m 21s —— View job


Re-review after c7bc7f8 — benchmark table regeneration

  • Read changed files at current tip
  • Verify the stale reference-table concern is resolved
  • Check for new issues
  • Post updated review

The one outstanding blocker from the previous re-review — the reference table being stale relative to the refactored tool — is now resolved. c7bc7f8 regenerates benchmarks/results/go_adapter_comparison.md verbatim from the current code, and it matches _render_markdown output on every point the last review flagged:

Prior mismatch Status in c7bc7f8
Stray | root: … | meta row ✅ Now a > Run: … blockquote caption (:5), matching _render_markdown :205
Header said RAM ✅ Now Peak RAM (:7), matching :208
(skipped) cell for owl Cold (+part_of) ✅ Now a real number 246.75s (:11) — the tool never emits (skipped)
Caption caches: isolated ✅ Now isolated, per-adapter subprocess (:5), matching :324
Non-monotonic RAM looked hand-assembled ✅ Now genuinely per-subprocess — owl 2201 MB → ubergraph 226 MB is expected because each adapter runs in its own process (_run_worker), and the README/commit message document exactly this

The new numbers are also internally consistent with the tooling's documented semantics: simpleobo/pronto show Cold (+part_of)Cold (is_a) (incremental predicate cost after the index is built), while owl shows all closure columns large and similar (no adjacency/closure cache, re-walks every call). The README addition (> Reading the table: note) explains this well, so a reader won't misread the cheap +part_of column as a true cold start.

Verdict

This looks ready to merge. The correctness-critical pieces (deterministic parity assertions in test_adapter_parity.py, the OLS-subset / Ubergraph-merged scope invariants) remain solid, and the artifact now faithfully reflects the tool that produces it.

🔵 One remaining nit (non-blocking, unchanged from last round)

  • args.obo_workdir = tmp is deadadapter_benchmark.py:314 still assigns args.obo_workdir inside main(), but nothing reads it (_resolve_inputs takes the workdir explicitly on :315). Safe to drop. Fix this →

Note: I reviewed by reading the diff and current file state; I did not execute pytest/just benchmark in this environment (the benchmark needs network + a ~4min full-GO run). The table-format verification above is a direct comparison of the committed file against the current _render_markdown/main code.
claude/oak-ltv-issue-diagnosis-023vbm

cmungall commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

Thanks for the review — addressed in ae819cf and c7bc7f8.

🟠 Per-adapter RAM — fixed properly rather than papered over: each adapter now runs in its own subprocess (--worker), so ru_maxrss is that adapter's true peak and caches can't leak between adapters. You were right that the old table couldn't have come from one process — it was assembled from separate scratchpad runs.

🟡 Reference table — regenerated verbatim from adapter_benchmark.py on full GO (no more hand-assembly or (skipped)). Note the RAM values are now legitimately non-monotonic (e.g. owl 2202 MB vs ubergraph 227 MB) precisely because they come from separate processes.

🟡 Test fixturelocal_adapters is now a session-scoped fixture (parsed once) with the if len(adapters) < 2: pytest.skip(...) guard so parity can't pass vacuously.

🔵 Nits — meta line is now a > Run: blockquote caption; OAKLIB_CACHE is set unconditionally like the others (with a comment on why an inherited value would defeat isolation); the is_a-first ordering the ancestor sample relies on is commented.

Bonus — the downloader was 403ing (no User-Agent), so just benchmark couldn't actually fetch GO; fixed and verified against a live OBO fetch.

Source-exclusion suggestion — didn't apply this one: test_local_adapters_agree drives raw adapter.descendants, which is reflexive=True by OAK default (self is included, and all adapters agree on that). Source exclusion is LTV reachable_from plugin semantics and is already locked down in test_reachable_from_progressive.py (assert source_node not in expanded).

One interpretation caveat I added to the README: closures are measured sequentially, so Cold (+part_of) runs after the is_a calls — for index-caching adapters (simpleobo, pronto) that column reflects incremental predicate cost, not a cold start.


Generated by Claude Code

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

Adds benchmarking + parity/scope tests around OAK adapters used for reachable_from closure expansion, to make adapter selection evidence-based and to prevent correctness regressions (especially around remote providers and non-is_a predicates).

Changes:

  • Introduces deterministic local-parity tests (and opt-in integration tests) to ensure adapter closure behavior is consistent and scoped correctly.
  • Adds a standalone benchmark runner that measures load/closure timings and peak RSS per adapter in isolated subprocesses.
  • Documents adapter tradeoffs and checks in benchmarks/README.md, and checks in a reference benchmark results table.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/test_adapter_parity.py Adds deterministic parity tests for local adapters + integration invariants for OLS/Ubergraph scope.
benchmarks/adapter_benchmark.py Adds an on-demand benchmark tool to compare adapter closure performance and memory.
benchmarks/README.md Documents adapter selection guidance and how to run the benchmark.
benchmarks/results/go_adapter_comparison.md Adds a reference results table produced by the benchmark script.
project.justfile Adds a just benchmark recipe to regenerate the results table.

Comment thread tests/test_adapter_parity.py Outdated
Comment thread benchmarks/adapter_benchmark.py
Comment thread benchmarks/adapter_benchmark.py Outdated
Comment thread benchmarks/adapter_benchmark.py
Comment thread benchmarks/adapter_benchmark.py
- import resource guarded (absent on Windows); _max_rss_mb returns None when
  unavailable and normalizes ru_maxrss units (KiB on Linux, bytes on macOS).
- download URLs switched to https; urlopen given a 60s timeout.
- test_adapter_parity: PART_OF_CURIE now sourced from oaklib's PART_OF constant
  instead of a duplicate literal, so the synthetic ontology and query predicates
  can't drift.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HfXcWrUi8hqTeD1exGSZte
@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @cmungall's task in 2m 0s —— View job


Re-review after 0036bc3 — Copilot review fixes

  • Read latest commit changes
  • Verify Copilot's 5 points were addressed
  • Check for new issues
  • Post updated review

0036bc3 cleanly addresses all five points from the Copilot review. Verifying each against the current file state:

Copilot finding Status in 0036bc3
PART_OF_CURIE duplicates OAK's PART_OF constant → drift risk ✅ Fixed — PART_OF_CURIE = PART_OF (test_adapter_parity.py:39). Ontology construction (:57, :64, :76, :86) and query predicates (:133) now share one source, so they can't diverge.
import resource crashes on Windows ✅ Fixed — guarded try/except ImportError sets resource = None (adapter_benchmark.py:50-53); _max_rss_mb returns None when unavailable (:71-72).
ru_maxrss units differ (KiB Linux / bytes macOS) ✅ Fixed — divisor = 1024*1024 if sys.platform == "darwin" else 1024 (:74-76). Correct: bytes→MB on macOS, KiB→MB on Linux.
urlopen without timeout can hang ✅ Fixed — urlopen(req, timeout=60) (:122).
Plain-HTTP download URLs ✅ Fixed — both PURLs now https:// (:57-58).

End-to-end sanity of the RAM fix: AdapterResult.rss_mb is typed float | None (:95), dataclasses.asdictjson.dumps serializes None as null and round-trips through the worker boundary (:269, :311), and _fmt(r.rss_mb, " MB") already renders None as - (:202, :235, :344). So a Windows run degrades to - in the RAM column rather than crashing. The committed reference table (results/go_adapter_comparison.md) is unaffected — it was produced on Linux and still matches _render_markdown verbatim.

🔵 One non-blocking nit (carried over, still unaddressed)

  • args.obo_workdir = tmp is deadadapter_benchmark.py:323 assigns it inside main(), but nothing reads it (_resolve_inputs takes the workdir explicitly on :324). Safe to drop. Flagged in the prior re-review too; purely cosmetic. Fix this →

Verdict

The Copilot-review turnaround is correct and the fixes are real (not papered over — the Windows/macOS handling and the CURIE dedup both address the root cause). Combined with the earlier rounds (deterministic parity assertions, per-subprocess RAM, regenerated reference table), this PR looks ready to merge. The only outstanding item is the one cosmetic dead-assignment nit above.

Note: I verified by reading the diff and current file state. I could not execute pytest/just benchmark in this environment (no installed venv; the benchmark also needs network + a ~4min full-GO run), so the parity-test and cross-platform-RSS checks above are from code reading, not a live run.
claude/oak-ltv-issue-diagnosis-023vbm

cmungall commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

Addressed the Copilot review comments in 0036bc3:

  • PART_OF_CURIE duplication — now sourced from oaklib's PART_OF constant, so the synthetic ontology and query predicates can't drift.
  • resource on Windows — import is guarded; _max_rss_mb() returns None when resource is unavailable, so the Peak RAM column degrades to - instead of crashing.
  • ru_maxrss units — normalized (KiB on Linux, bytes on macOS) so RAM isn't misreported off-Linux.
  • urlopen timeout — added a 60s timeout so a stalled fetch can't hang a run.
  • HTTP → HTTPS — GO download URLs now use https://.

CI was already green across 3.10–3.13; these are cross-platform/robustness hardening for the benchmark tool.


Generated by Claude Code

@cmungall
cmungall requested a review from Copilot July 3, 2026 02:26
Vestigial write flagged in re-review: args.obo_workdir was assigned in main()
but never read (_resolve_inputs receives the workdir explicitly).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HfXcWrUi8hqTeD1exGSZte
@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @cmungall's task in 50s —— View job


Re-review after b829f4b — dead-assignment cleanup

  • Read latest commit changes
  • Verify the outstanding nit was addressed
  • Check for new issues
  • Post updated review

b829f4b removes the last outstanding item from every prior review round — the vestigial args.obo_workdir = tmp assignment in main(). The change is a clean 2-line deletion and is behaviorally correct:

  • _resolve_inputs receives the workdir explicitly as a parameter (adapter_benchmark.py:277, called at :322) and constructs its own BenchContext(args, workdir) — it never read args.obo_workdir.
  • grep obo_workdir over the file now returns no matches, so nothing downstream (including getattr-style access) depended on the attribute.
  • Path resolution still works: both parent mode (:322) and worker mode (:308) derive the workdir from Path(args.obo).parent if args.obo else Path(tmp), independent of the removed line.

Verdict

This PR is ready to merge. Tracing the full review history, every point raised has been resolved:

Item Status
Per-adapter RAM via monotonic ru_maxrss ✅ Each adapter runs in its own --worker subprocess (true peak)
Reference table stale / hand-assembled ✅ Regenerated verbatim from the refactored tool (c7bc7f8)
Parity test could pass vacuously session-scoped fixture + len(adapters) < 2 skip guard
Missing User-Agent → 403 on OBO hosts Request(..., headers={"User-Agent": ...})
Copilot: PART_OF_CURIE drift ✅ Sourced from OAK's PART_OF
Copilot: resource on Windows ✅ Guarded import, _max_rss_mb()None
Copilot: ru_maxrss units (Linux/macOS) ✅ Platform-aware divisor
Copilot: urlopen timeout / HTTP→HTTPS timeout=60, https:// PURLs
Dead args.obo_workdir assignment ✅ Removed in b829f4b

The correctness-critical pieces (deterministic parity assertions, OLS-subset / Ubergraph-merged scope invariants) remain solid, and the committed benchmark artifact faithfully reflects the tool that produces it. No new issues introduced by this commit.

Note: I verified by reading the diff and current file state. I did not execute pytest/just benchmark in this environment (no installed venv; the benchmark also needs network + a ~4min full-GO run), so the verification above is from code reading, not a live run.
claude/oak-ltv-issue-diagnosis-023vbm

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 5 out of 5 changed files in this pull request and generated 3 comments.

Comment thread benchmarks/adapter_benchmark.py Outdated
Comment thread benchmarks/adapter_benchmark.py
Comment thread tests/test_adapter_parity.py
…guard

- Module docstring referenced a nonexistent --isolate-caches flag; corrected to
  describe the actual --no-isolate-caches (isolation is the default).
- IS_A/PART_OF in the benchmark are now imported from oaklib.datamodels.vocabulary
  instead of hard-coded, matching the test and avoiding cross-version drift.
- OLS integration test now guards the raw-adapter call: it intentionally exercises
  the raw OAK OLS adapter (to characterize the #55 truncation, not LTV's
  _ols_descendants fallback), and skips cleanly if a given oaklib/OLS build lacks
  descendants() or returns nothing, rather than crashing or asserting vacuously.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HfXcWrUi8hqTeD1exGSZte
@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @cmungall's task in 1m 8s —— View job


Re-review after 3ba64b4 — Copilot review round 2

  • Read the latest commit changes
  • Verify Copilot's 3 round-2 comments were addressed
  • Check for new issues
  • Post updated review

3ba64b4 cleanly resolves all three points from the second Copilot review. Verifying each against the current file state:

Copilot round-2 finding Status in 3ba64b4
Module docstring referenced a nonexistent --isolate-caches flag ✅ Fixed — docstring now reads "cache isolation is on by default; disable with --no-isolate-caches" (adapter_benchmark.py:12), matching the actual argparse flag.
IS_A/PART_OF hard-coded as strings → drift from OAK vocabulary ✅ Fixed — now from oaklib.datamodels.vocabulary import IS_A, PART_OF (:57), and PREDICATE_SETS (:67-68) consumes them. This matches test_adapter_parity.py:39 (PART_OF_CURIE = PART_OF), so the benchmark, the synthetic ontology, and the query predicates now share a single source of truth.
OLS integration test may crash with AttributeError / treat empty as "no descendants" ✅ Fixed — the raw ols.descendants() call is wrapped in try/except (NotImplementedError, AttributeError) with a pytest.skip, plus an if not ols_desc: pytest.skip(...) guard (test_adapter_parity.py:206-215). The vacuous assert ols_desc was removed.

Details worth noting:

  • The vocabulary import is correctly placed after the resource try/except with a # noqa: E402 and an explanatory comment — no lint regression, and the "is_a first" ordering contract in PREDICATE_SETS is preserved (:65-68), so the ancestor-throughput sample still draws from the is_a set as documented.
  • The OLS test change is a genuine robustness fix, not a papering-over: it still asserts the real invariant (ols_desc <= truth) when descendants are available, and only skips when a given oaklib/OLS build can't produce an adapter-level closure to compare. The comment correctly frames this as characterizing the raw-adapter OLS adapter silently truncates reachable_from closures (upstream oaklib bug — tracked by INCATools/ontology-access-kit#893) #55 truncation, distinct from LTV's _ols_descendants fallback path — so the two concerns stay separately tested.
  • IS_A in OAK's vocabulary is rdfs:subClassOf and PART_OF is BFO:0000050, identical to the previously hard-coded values, so this commit changes the source of the constants without changing behavior or invalidating the committed reference table (results/go_adapter_comparison.md, produced on Linux, still matches _render_markdown).

Verdict

This PR is ready to merge. Tracing the full review history, every point from every round — the initial review, three re-review rounds, and both Copilot passes — has now been resolved:

Item Status
Per-adapter RAM via monotonic ru_maxrss ✅ Own --worker subprocess per adapter (true peak)
Reference table stale / hand-assembled ✅ Regenerated verbatim from the refactored tool
Parity test could pass vacuously session-scoped fixture + len(adapters) < 2 skip guard
Missing User-Agent → 403 on OBO hosts Request(..., headers={"User-Agent": ...})
resource on Windows / ru_maxrss units / urlopen timeout / HTTP→HTTPS ✅ Guarded import, platform-aware divisor, timeout=60, https://
Dead args.obo_workdir assignment ✅ Removed
Copilot r2: --isolate-caches docstring ✅ Corrected
Copilot r2: hard-coded IS_A/PART_OF ✅ Sourced from OAK vocabulary
Copilot r2: OLS raw-adapter call may crash ✅ Guarded with skip

The correctness-critical pieces (deterministic parity assertions, OLS-subset / Ubergraph-merged scope invariants) remain solid, and the benchmark artifact faithfully reflects the tool that produces it. No new issues introduced by this commit.

Note: I verified by reading the diff and current file state. I could not git fetch origin/main (command approval denied) or run pytest/just benchmark in this environment (no installed venv; the benchmark needs network + a ~4 min full-GO run), so the verification above is from code reading against the branch tip, not a live run.
claude/oak-ltv-issue-diagnosis-023vbm

cmungall commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

Addressed the 3 new Copilot comments in 3ba64b4:

  • Docstring --isolate-caches — corrected; the flag is --no-isolate-caches (isolation is the default), docstring now says so.
  • Hard-coded IS_A/PART_OF in the benchmark — now imported from oaklib.datamodels.vocabulary, matching the test, so the predicate CURIEs can't drift across OAK versions.
  • OLS test calling ols.descendants() directly — this is intentional: the test characterizes the raw OAK OLS adapter truncation from OLS adapter silently truncates reachable_from closures (upstream oaklib bug — tracked by INCATools/ontology-access-kit#893) #55, deliberately not routing through LTV's DynamicEnumPlugin._ols_descendants fallback (which would mask the adapter-level behavior we're asserting). I did add the robustness you flagged: it now skips cleanly (rather than crashing or asserting vacuously) if a given oaklib/OLS build lacks descendants() or returns nothing. Verified the test still passes against the current adapter.

Generated by Claude Code

@cmungall
cmungall merged commit 599f41c into main Jul 3, 2026
5 checks passed
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.

3 participants