feat: make GEPA --optimize work end-to-end with real coverage scoring - #66
Merged
Conversation
…t KG builds
The `tablassert agent --optimize` GEPA path was broken against real dspy 3.2.1
(gepa_metric took one arg but dspy.GEPA binds five), and the agent's KG builds
failed on relative workdirs. This makes the optimization path real and the agent
build KGs reliably.
GEPA optimization:
- gepa_metric now satisfies dspy.GEPA's 5-arg metric contract (gold, pred, trace,
pred_name, pred_trace) while keeping the legacy single-bundle call the offline
suite relies on.
- The metric scores each proposed config with REAL fullmap coverage (a build_and_audit
head-sample) when the dataset example carries a `fullmap` (+ optional `workdir`/`head`),
so GEPA optimizes the genuine objective instead of a validity-only proxy.
- run_gepa splits a fast task LM (--task-model) from the strong reflection LM, configures
dspy with the task LM, and forwards --gepa-threads.
- make_dspy_lm adds reasoning-model-safe defaults (max_tokens=16000, temperature=1.0) and
a request timeout (600s) so a truncated/stalled call can't hang the optimizer.
- CLI: --task-model and --gepa-threads; --instructions-out is resolved to an absolute path
(GEPA's parallel builds chdir the process cwd).
Agent KG builds:
- build_and_audit resolves its workdir to an absolute path. A relative workdir made
build_pipeline's `.tablassert/store` parquets resolve against the wrong base once the
build chdir'd -> the build failed -> false 0.0 coverage -> every article SKIPPED.
- The supervisor presents absolute table paths to the inner agent so source.local resolves
in the build workdir.
- build_agent raises the local executor timeout (30s -> 600s) so a large-table build_and_audit
is not killed mid-build (which also stranded the fullmap redb lock).
- build_and_audit retries its coverage measurement on transient failure, and fullmap lookups
retry on transient redb lock contention ("Database already open").
Adds a GEPA-optimized prompt + example dataset under examples/agent/, regression tests for the
5-arg contract / real-coverage metric / task-LM split / num_threads / make_dspy_lm defaults, and
docs. Verified: the agent maps PMC11947420 (coverage 0.9993, 2865 nodes / 4995 edges) and the
produced KGX validates (biolink categories/predicates + provenance, 0 dangling endpoints). Full
suite: 725 passed.
…PED) Prompt hardening (pushed the GEPA-optimized prompt further): - File handling & robustness: verify each table/worksheet via read_table before authoring a section; skip missing/empty/unmappable files gracefully (no retrying a broken path); use the EXACT case/space-sensitive worksheet name; use only the candidate-table absolute paths (never fabricate a path). - Predicate choice: pick the most-specific valid biolink predicate (gene_associated_with_condition, correlated_with, expressed_in, biomarker_for, has_sequence_variant, affects...), falling back to associated_with/related_to only when nothing specific fits. - Reading tool output: build_and_audit/map_coverage return a JSON STRING; parse with yaml.safe_load (yaml is imported and parses JSON) — never 'import json' (unauthorized) and never index the raw string. Return a config that already maps well instead of over-editing. QC assay (qwen3.8-max-preview, improved prompt, 10 diverse PMCs spanning 9 predicate types): - 10/10 MAPPED, 0 SKIPPED, mean best coverage 0.974, all first-attempt. - 8/10 specific predicates; 2 generic associated_with fallbacks (PMC13161869, PMC12900646) flagged for review. - examples/agent/QC_REPORT.md: per-PMC derived config + predicate/encodings/provenance + KG node/edge counts + sample edges + aggregate metrics, for manual QC.
…ses poor->acceptable) Built an automated QC loop (replaces manual QC): - examples/agent/qc/qc_report.py: deterministic per-PMC QC report (config + predicate/encodings/ provenance + KG counts + sample edges + aggregate metrics). - examples/agent/qc/qc_reviewer.py: LLM-as-judge (qwen3.8-max-preview) that critiques each derived config + table + KG sample across predicate_appropriateness / encoding_correctness / provenance / coverage / other_mistakes and proposes a prompt improvement per PMC. Iterative improvement: - Round 1 (10 PMCs): 10/10 MAPPED, mean cov 0.974; reviewer scored predicate_appropriateness 1.71/3, other_mistakes 1.43/3 (weak). Recurring mistakes: over-interpretation, hard-coded objects, dropped statistical annotations, wrong object columns, generic predicates. - Added a Quality-principles section to the prompt to address these. Round 2: 10/10 MAPPED, mean cov 0.989, but the reviewer flagged wrong prioritize guesses (a wrong prioritize is worse than none). - Refined the prioritize guidance: add prioritize ONLY when confident, otherwise OMIT it. Round 3 (the 3 worst PMCs): 3/3 MAPPED, mean cov 0.981; PMC8017771 + PMC12900646 improved poor->acceptable (PMC13172311 splice table remains poor — genuinely hard event-level data). - examples/agent/QC_REVIEW.md: latest round's per-PMC review for inspection. Key learning captured: the coverage metric measures resolution RATE, not correctness, so high coverage can mask wrong-entity resolution; the LLM reviewer has run-to-run variance, so trends matter more than single-round absolute scores.
…ble config derivation
📝 WalkthroughWalkthroughThe agent workflow adds derive modes, path handling, coverage retries, execution timeouts, and configurable GEPA evaluation. New prompts, datasets, QC scripts, and reports support optimization and mapping review. ChangesAgent optimization and quality workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as agent CLI
participant RunGEPA as run_gepa
participant GEPA as DSPy GEPA
participant Bundle as _gepa_bundle_from_dspy
participant Audit as build_and_audit
CLI->>RunGEPA: pass task model, dataset, and thread count
RunGEPA->>GEPA: configure task and reflection LMs
GEPA->>Bundle: evaluate candidate prediction
Bundle->>Audit: run head or full candidate build
Audit-->>Bundle: return validity and coverage
Bundle-->>GEPA: return metric score
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/tablassert/agent.py (1)
2295-2324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType
derive_modeas aLiteralto catch typos at check time.
make_toolsandrun_supervisorcomparederive_modeagainst the literal strings"derive_only"and"derive_coverage". An unrecognized value (for example a hyphenated"derive-only") silently falls through to the"full"branch instead of failing loud. Since the project runs Pyright, declarederive_mode: Literal["full", "derive_only", "derive_coverage"]in both signatures so an invalid caller value is caught statically instead of silently changing behavior at runtime.♻️ Proposed refactor
- derive_mode: str = "full", + derive_mode: Literal["full", "derive_only", "derive_coverage"] = "full",Also applies to: 2518-2519
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tablassert/agent.py` around lines 2295 - 2324, Update the derive_mode annotations in both make_tools and run_supervisor to use Literal["full", "derive_only", "derive_coverage"], importing Literal from typing as needed. Keep the existing mode comparisons and behavior unchanged so invalid caller values are rejected by Pyright instead of falling through to the full mode.docs/agent.md (1)
301-307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify what
--gepa-threadsactually parallelizes.This text states
--gepa-threads"parallelizes GEPA's evaluation pool," which is true only for the LM forward pass that proposes each candidate config. The actual coverage-scoring build (build_and_audit) runs under a process-wide lock (_GEPA_BUILD_LOCKinagent.py) becauseos.chdiris process-global, so builds for concurrent candidates are fully serialized regardless of thread count. Since the build is documented elsewhere as the expensive step (up to ~60s for a large sheet), users tuning--gepa-threadsfor speed should know it mainly parallelizes LM calls, not the build/coverage cost.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/agent.md` around lines 301 - 307, Revise the --gepa-threads description in docs/agent.md to state that it primarily parallelizes GEPA candidate LM forward passes, while build_and_audit coverage scoring remains serialized by _GEPA_BUILD_LOCK because os.chdir is process-global. Clarify that increasing the thread count does not parallelize the expensive build/coverage step.src/tablassert/cli.py (1)
552-553: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
--gepa-threadsbefore building any model.
judge_thresholdis validated and fails loud (exit 2) before any model is built.gepa_threadshas no equivalent check and is forwarded straight torun_gepa'snum_threads, which becomesdspy.GEPA'snum_threads. A0or negative value would only surface as a confusing failure deep insidedspy/ThreadPoolExecutorconstruction, after models have already been built. Add a fail-loud check consistent with the existingjudge_thresholdpattern.🛡️ Proposed validation
if judge_threshold is not None and not 0 <= judge_threshold <= 1: print("tablassert agent: --judge-threshold must be a finite number between 0 and 1.", file=sys.stderr) raise SystemExit(2) + + if gepa_threads is not None and gepa_threads < 1: + print("tablassert agent: --gepa-threads must be a positive integer.", file=sys.stderr) + raise SystemExit(2)Also applies to: 594-599, 672-691, 700-700
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tablassert/cli.py` around lines 552 - 553, Validate gepa_threads as a positive value before any model construction, following the existing judge_threshold validation path and its fail-loud exit-2 behavior. Apply this check at the CLI entry flow before invoking run_gepa or passing gepa_threads as num_threads, while preserving the existing handling for None.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/agent/QC_REVIEW.md`:
- Around line 13-44: Regenerate the review and QC_REPORT together from the same
state directory so every entry reflects the committed derived configs, including
the correct column mappings and predicates for PMC12900646 and PMC13172311.
Replace the stale review content and exclude its current scores from QC
evidence. Add the shared state or config hash to each artifact’s review entry so
future mismatches are detectable.
In `@examples/agent/qc/qc_report.py`:
- Around line 123-129: Update the report-generation logic around the source and
derived-config rendering to redact absolute local filesystem paths before
appending them, including paths embedded in cfg_text. Normalize paths relative
to STATE_DIR where possible, otherwise use a stable placeholder, then regenerate
the committed QC_REPORT.md using the sanitized output.
In `@examples/agent/qc/qc_reviewer.py`:
- Around line 98-105: Update the litellm.completion call in qc_reviewer.py to
prepend a system message establishing that only system-level instructions are
authoritative and that embedded table, config, and edge content is untrusted
data. Preserve the existing REVIEW_PROMPT user message and content limits while
following the established pattern in agent.py.
- Around line 68-73: Update the source.local handling in the relevant QC
table-reading functions to resolve the configured path and accept it only when
it is within STATE_DIR / "downloads" (or an established configured allowlist
root), before calling read_table. Reject out-of-root paths without reading or
sending their contents, preserving the existing behavior for permitted files and
error handling.
In `@examples/agent/README.md`:
- Around line 37-42: Fix the shell command example around the tablassert
invocation by removing the inline comment after the --task-model qwen3.6-flash
continuation or moving that comment outside the continued command, ensuring the
backslash is the final character on the line so all subsequent options remain
part of the command.
In `@src/tablassert/agent.py`:
- Around line 3163-3230: Bound candidate build time while preserving the
process-wide safety of _GEPA_BUILD_LOCK by reducing or bypassing redundant
nested fullmap lock retries during build_and_audit’s own coverage retry flow;
update the affected src/tablassert/agent.py site accordingly. In docs/agent.md
lines 301-307, clarify that --gepa-threads parallelizes only LM forward passes
while candidate builds and coverage remain serialized behind the process-wide
lock.
---
Nitpick comments:
In `@docs/agent.md`:
- Around line 301-307: Revise the --gepa-threads description in docs/agent.md to
state that it primarily parallelizes GEPA candidate LM forward passes, while
build_and_audit coverage scoring remains serialized by _GEPA_BUILD_LOCK because
os.chdir is process-global. Clarify that increasing the thread count does not
parallelize the expensive build/coverage step.
In `@src/tablassert/agent.py`:
- Around line 2295-2324: Update the derive_mode annotations in both make_tools
and run_supervisor to use Literal["full", "derive_only", "derive_coverage"],
importing Literal from typing as needed. Keep the existing mode comparisons and
behavior unchanged so invalid caller values are rejected by Pyright instead of
falling through to the full mode.
In `@src/tablassert/cli.py`:
- Around line 552-553: Validate gepa_threads as a positive value before any
model construction, following the existing judge_threshold validation path and
its fail-loud exit-2 behavior. Apply this check at the CLI entry flow before
invoking run_gepa or passing gepa_threads as num_threads, while preserving the
existing handling for None.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 97e5ebc3-57c9-4d73-8c50-e064b64bb3a9
📒 Files selected for processing (14)
docs/agent.mddocs/cli.mdexamples/agent/QC_REPORT.mdexamples/agent/QC_REVIEW.mdexamples/agent/README.mdexamples/agent/gepa-dataset.yamlexamples/agent/optimized_instructions.yamlexamples/agent/qc/qc_report.pyexamples/agent/qc/qc_reviewer.pysrc/tablassert/agent.pysrc/tablassert/cli.pysrc/tablassert/fullmap.pytests/test_agent_cli.pytests/test_agent_eval.py
- derive_mode typed as Literal[full|derive_only|derive_coverage] (make_tools, run_supervisor) - --gepa-threads < 1 fails loud (exit 2) before any model is built - coverage retry no longer re-retries exhausted fullmap lock contention (is_lock_contention) - docs: --gepa-threads parallelizes LM forward passes only; builds serialized on _GEPA_BUILD_LOCK - README: move inline comment off the continued shell line - qc_report.py: redact absolute paths (<state-dir>/<local-path>); qc_reviewer.py: untrusted-data system message + downloads-dir allowlist for source.local + bounded litellm timeout - QC_REPORT.md + QC_REVIEW.md regenerated together from the same state dir; each entry carries a shared config sha256 so future report/review/config drift is detectable
Owner
Author
|
All findings addressed in 8f7ad15: Actionable
Nitpicks
Verification: |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
examples/agent/qc/qc_reviewer.py (2)
185-190: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winInclude
config_sha256on failed Markdown entries.The hash is assigned to every review after the
try/except, but the failure branch omits it from the Markdown heading. A failed entry cannot then be matched withQC_REPORT.md.Proposed fix
- lines.append(f"\n## {pmc} — review failed: {rv.get('error') or 'parse error'}\n") + lines.append( + f"\n## {pmc} — review failed: {rv.get('error') or 'parse error'} " + f"(config sha256: `{rv.get('config_sha256', '-')}`)\n" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/agent/qc/qc_reviewer.py` around lines 185 - 190, Update the failed-review branch in the Markdown generation logic to include the review’s config_sha256 value in the heading, using the same fallback formatting as successful entries. Preserve the existing failure message and continue behavior while ensuring failed entries can be matched with QC_REPORT.md.
137-145: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the parsed reviewer response before rendering it.
json.loadschecks syntax only. Later code assumes each dimension is a mapping and accepts any numeric score. A valid response with a list for one dimension can abort report generation atdd.get. An out-of-range score can also corrupt/3aggregate metrics. Validate the response schema, score range0..3, and allowed quality values. Mark invalid responses as failed reviews.Proposed validation point
- return json.loads(text.strip()) + result = json.loads(text.strip()) + validate_review_result(result) + return resultApply the same validation to the fallback JSON path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/agent/qc/qc_reviewer.py` around lines 137 - 145, Update the reviewer-response parsing flow around the primary and fallback json.loads calls to validate the decoded object before rendering. Require each expected dimension to be a mapping with a score in the 0..3 range and an allowed quality value; reject any response that fails these checks, including fallback results, and return the existing parse_error-style failed-review result so report generation cannot access invalid values.examples/agent/README.md (1)
17-25: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the QC state-directory requirement.
Configs generated from the dataset use paths under
.tablassert/gepa/downloads, butqc_reviewer.pyonly reads paths under itsSTATE_DIR/downloads. Document this relationship or stage the tables under the QC directory, and add a generated-config test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/agent/README.md` around lines 17 - 25, Document that generated dataset configs reference tables under .tablassert/gepa/downloads while qc_reviewer.py resolves inputs beneath STATE_DIR/downloads, and ensure QC evaluation uses matching staged tables or an equivalent path relationship. Add a generated-config test covering this state-directory mapping and confirming the QC reviewer can read the referenced tables.
🧹 Nitpick comments (3)
tests/test_agent_cli.py (3)
224-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the invalid-thread test prove the pre-model ordering.
The test blocks
run_supervisor, but it does not blockmake_dspy_lm. It can pass if model construction occurs beforegepa_threadsvalidation. Patchtablassert.agent.make_dspy_lmto fail and assert that invalidgepa_threadsperforms no model construction.Suggested assertion
def fail_supervisor(*a: object, **k: object) -> object: raise AssertionError("run_supervisor must NOT run with an invalid --gepa-threads") + def fail_model_init(*a: object, **k: object) -> object: + raise AssertionError("make_dspy_lm must NOT run with an invalid --gepa-threads") + monkeypatch.setattr("tablassert.agent.run_supervisor", fail_supervisor) + monkeypatch.setattr("tablassert.agent.make_dspy_lm", fail_model_init)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_agent_cli.py` around lines 224 - 241, Update test_agent_gepa_threads_non_positive_exits_2 to monkeypatch tablassert.agent.make_dspy_lm with a failing stub, alongside the existing run_supervisor guard. Keep the invalid-thread invocation and exit-code/error assertions, ensuring the test proves neither model construction nor supervisor execution occurs before validation.
278-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the reflection LM configuration explicitly.
lm_callsrecords everymake_dspy_lmcall, but the test inspects onlylm_calls[0]. With separate task and reflection LMs, a regression can leave the reflection LM on the wrong backend while the first call still useslitellm. Assert the expected call count and inspect the reflection-LM record directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_agent_cli.py` around lines 278 - 300, Update test_agent_optimize_forwards_backend_to_dspy_lm to assert the expected number of make_dspy_lm calls, then inspect the specific reflection-LM call rather than assuming lm_calls[0] is the reflection configuration. Verify that this call receives model, base URL, and API key values plus backend="litellm", while preserving the existing forwarding assertions.
331-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an assertion for the default
timeout.The fake LM captures all production keyword names. The assertions inspect
captured[0], but they cover onlytemperatureandmax_tokens.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_agent_cli.py` around lines 331 - 342, Extend the assertions for the fake LM invocation captured by its __init__ to also verify the default timeout value in captured[0]. Keep the existing checks for temperature and max_tokens and use the expected production default for timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/agent/QC_REPORT.md`:
- Line 259: Complete the truncated YAML mapping in the affected configuration
entry so the Neu pattern uses the full replacement value “neutrophil” with valid
quoting and braces. Ensure CI parses every fenced configuration block to catch
YAML syntax errors.
In `@examples/agent/qc/qc_report.py`:
- Around line 20-32: Apply one complete path-redaction policy across all QC
artifacts: in examples/agent/qc/qc_report.py lines 20-32, update redact_paths to
use boundary-aware matching for arbitrary absolute local paths while preserving
public URLs and STATE_DIR normalization; in examples/agent/qc/qc_reviewer.py
lines 176-183, sanitize every string value, including parse-error content,
before JSON serialization; in lines 191-205, render those sanitized review
fields in per-PMC Markdown entries; and in lines 218-220, sanitize
reviewer-generated prompt improvements before writing Markdown. Reuse
redact_paths rather than introducing separate sanitization logic.
In `@examples/agent/qc/qc_reviewer.py`:
- Around line 78-89: Update get_table_summary to derive the required column
limit from each section’s configured subject, object, and annotation columns
instead of hard-coding max_cols=12, ensuring columns such as S and T are
included. Process every configured section, accumulate their readable summaries,
and return the combined result only after all sections have been examined rather
than returning after the first readable section.
---
Outside diff comments:
In `@examples/agent/qc/qc_reviewer.py`:
- Around line 185-190: Update the failed-review branch in the Markdown
generation logic to include the review’s config_sha256 value in the heading,
using the same fallback formatting as successful entries. Preserve the existing
failure message and continue behavior while ensuring failed entries can be
matched with QC_REPORT.md.
- Around line 137-145: Update the reviewer-response parsing flow around the
primary and fallback json.loads calls to validate the decoded object before
rendering. Require each expected dimension to be a mapping with a score in the
0..3 range and an allowed quality value; reject any response that fails these
checks, including fallback results, and return the existing parse_error-style
failed-review result so report generation cannot access invalid values.
In `@examples/agent/README.md`:
- Around line 17-25: Document that generated dataset configs reference tables
under .tablassert/gepa/downloads while qc_reviewer.py resolves inputs beneath
STATE_DIR/downloads, and ensure QC evaluation uses matching staged tables or an
equivalent path relationship. Add a generated-config test covering this
state-directory mapping and confirming the QC reviewer can read the referenced
tables.
---
Nitpick comments:
In `@tests/test_agent_cli.py`:
- Around line 224-241: Update test_agent_gepa_threads_non_positive_exits_2 to
monkeypatch tablassert.agent.make_dspy_lm with a failing stub, alongside the
existing run_supervisor guard. Keep the invalid-thread invocation and
exit-code/error assertions, ensuring the test proves neither model construction
nor supervisor execution occurs before validation.
- Around line 278-300: Update test_agent_optimize_forwards_backend_to_dspy_lm to
assert the expected number of make_dspy_lm calls, then inspect the specific
reflection-LM call rather than assuming lm_calls[0] is the reflection
configuration. Verify that this call receives model, base URL, and API key
values plus backend="litellm", while preserving the existing forwarding
assertions.
- Around line 331-342: Extend the assertions for the fake LM invocation captured
by its __init__ to also verify the default timeout value in captured[0]. Keep
the existing checks for temperature and max_tokens and use the expected
production default for timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 78c92100-0cbf-4ae7-931f-b3770d654fbf
📒 Files selected for processing (12)
docs/agent.mddocs/cli.mdexamples/agent/QC_REPORT.mdexamples/agent/QC_REVIEW.mdexamples/agent/README.mdexamples/agent/qc/qc_report.pyexamples/agent/qc/qc_reviewer.pysrc/tablassert/agent.pysrc/tablassert/cli.pysrc/tablassert/fullmap.pytests/test_agent_cli.pytests/test_fullmap.py
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/cli.md
- src/tablassert/fullmap.py
- docs/agent.md
- src/tablassert/cli.py
- src/tablassert/agent.py
- qc_reviewer: validate judge JSON schema (dimension mappings, score 0..3, quality enum) before rendering; failed entries carry config sha256; table summary derives max_cols from the configured subject/object/annotation columns (S/T no longer cut at 12) and accumulates ALL sections; every review string sanitized via redact_paths before JSON+markdown; --rerender rebuilds markdown from qc_review.json without re-querying the judge - qc_report: redact_paths generalized to any absolute local path (URL/ratio-safe lookbehind); config fence cap 3000 -> 8000 so no yaml block is truncated mid-mapping - README: document the QC state-dir/downloads allowlist relationship and the qc/ scripts - tests: new tests/test_example_qc.py (allowlist, column derivation, validation, redaction, committed-artifact yaml-fence parseability + shared sha256); CLI tests now prove no model is built before --gepa-threads validation, assert the single reflection-LM construction, and the dspy LM timeout default
Owner
Author
|
Round-2 findings addressed in df93ca2: Actionable
Nitpicks
Verification: |
…nent) pritamdeka/BioBERT-mnli-snli-scitail-mednli-stsb no longer exists (HF API 401); the real sentence-transformers repo is pritamdeka/BioBERT-mnli-snli-scinli-scitail-mednli-stsb (200, ~33k downloads). With the old id, get_biobert()'s download path could never succeed for a fresh cache.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/agent/qc/qc_reviewer.py (1)
21-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
OUT_JSON/OUT_MDcache a staleSTATE_DIRsnapshot.
OUT_JSONandOUT_MDare computed once, at import time, fromSTATE_DIR.get_table_summaryandredact_pathsinstead readSTATE_DIRlive at call time, so patchingSTATE_DIRafter import (as theqc_modstest fixture does) correctly reaches them. It does not reachOUT_JSON/OUT_MD.
main()andrerender()both read/write throughOUT_JSON/OUT_MD. A test that patchesSTATE_DIRand then callsmain()/rerender()will silently read/write at the import-time path instead of the intended directory. No current test exercisesmain()/rerender(), but that gap makes this easy to hit later.Derive both paths from
STATE_DIRat call time instead of caching them as module constants.♻️ Proposed fix
-OUT_JSON = STATE_DIR / "qc_review.json" -OUT_MD = STATE_DIR / "QC_REVIEW.md" +def _out_json() -> Path: + return STATE_DIR / "qc_review.json" + + +def _out_md() -> Path: + return STATE_DIR / "QC_REVIEW.md"Then replace the
OUT_JSON/OUT_MDreferences inmain()(state loading,.write_text, and the final print) andrerender()with_out_json()/_out_md():def main() -> None: ... reviews = cast("dict[str, dict]", _redact_strings(reviews)) _out_json().write_text(json.dumps(reviews, indent=1)) render_markdown(reviews) print(f"\nreview -> {_out_json()} and {_out_md()}") ... def rerender() -> None: reviews = cast("dict[str, dict]", _redact_strings(json.loads(_out_json().read_text()))) _out_json().write_text(json.dumps(reviews, indent=1)) render_markdown(reviews) print(f"re-rendered -> {_out_md()} (from {_out_json()})")
render_markdownitself also writesOUT_MD.write_text(...)at its current call site (line 288) and needs the same substitution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/agent/qc/qc_reviewer.py` around lines 21 - 23, Replace the import-time OUT_JSON and OUT_MD snapshots with call-time helpers such as _out_json() and _out_md() derived from the current STATE_DIR. Update all references in main(), rerender(), and render_markdown(), including reads, writes, and printed paths, so patched STATE_DIR values are honored consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@examples/agent/qc/qc_reviewer.py`:
- Around line 21-23: Replace the import-time OUT_JSON and OUT_MD snapshots with
call-time helpers such as _out_json() and _out_md() derived from the current
STATE_DIR. Update all references in main(), rerender(), and render_markdown(),
including reads, writes, and printed paths, so patched STATE_DIR values are
honored consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 98fd6e36-107b-4a03-b4c9-2ad6606f1cdd
📒 Files selected for processing (6)
examples/agent/QC_REPORT.mdexamples/agent/README.mdexamples/agent/qc/qc_report.pyexamples/agent/qc/qc_reviewer.pytests/test_agent_cli.pytests/test_example_qc.py
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/agent/QC_REPORT.md
- tests/test_agent_cli.py
- examples/agent/qc/qc_report.py
Owner
Author
|
Folded in a BioBERT HF-repo fix (82f788c): the judge-embedding download pointed at |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tablassert/qc.py`:
- Line 60: Update get_biobert() so the cache is keyed and validated by the model
repository and an immutable revision, rather than loading the shared
.tablassert/biobert path unconditionally. Ensure stale metadata or a mismatched
cache is ignored and the requested model is downloaded, then add a regression
test covering stale-cache reuse.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ec32f2a0-f763-4918-8713-d09c45b017d8
📒 Files selected for processing (3)
docs/api/qc.mdsrc/tablassert/qc.pytests/test_cover_qc.py
| else: | ||
| model = sentence_transformers.SentenceTransformer( # pyright: ignore | ||
| "pritamdeka/BioBERT-mnli-snli-scitail-mednli-stsb" | ||
| "pritamdeka/BioBERT-mnli-snli-scinli-scitail-mednli-stsb" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
fd -t f 'qc.py|test|tests' . | head -200
printf '%s\n' '--- qc.py outline ---'
ast-grep outline src/tablassert/qc.py
printf '%s\n' '--- qc.py relevant source ---'
cat -n src/tablassert/qc.py | sed -n '1,140p'
printf '%s\n' '--- cache/model references ---'
rg -n -C 4 'get_biobert|\.tablassert|MODEL|SentenceTransformer|biobert|revision' .Repository: SkyeAv/Tablassert
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- qc tests ---'
cat -n tests/test_qc.py | sed -n '1,260p'
printf '%s\n' '--- qc history/diff summary ---'
git diff --stat
git status --short
printf '%s\n' '--- model-related tracked files ---'
git ls-files | rg '(^|/)(qc|pyproject|uv.lock|test_qc|docs/api/qc)'
printf '%s\n' '--- dependency declarations ---'
rg -n -C 3 'sentence-transformers|qc' pyproject.toml uv.lock | head -160
printf '%s\n' '--- exact get_biobert callers ---'
rg -n -C 5 'get_biobert\(' src tests docsRepository: SkyeAv/Tablassert
Length of output: 14762
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- cache coverage tests ---'
cat -n tests/test_cover_qc.py | sed -n '1,125p'
printf '%s\n' '--- previous qc implementation ---'
git show HEAD^:src/tablassert/qc.py 2>/dev/null | sed -n '1,85p' || true
printf '%s\n' '--- recent commits ---'
git log --oneline -8
printf '%s\n' '--- current revision/config context ---'
cat -n pyproject.toml | sed -n '65,90p'
printf '%s\n' '--- cache-related documentation ---'
cat -n docs/api/qc.md | sed -n '114,132p'Repository: SkyeAv/Tablassert
Length of output: 9818
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- commit under review ---'
git show --stat --oneline HEAD
git show --format=fuller --no-ext-diff HEAD -- src/tablassert/qc.py tests/test_cover_qc.py
printf '%s\n' '--- parent/current model identifiers ---'
for rev in HEAD^ HEAD; do
printf '%s: ' "$rev"
git show "$rev:src/tablassert/qc.py" | sed -n '42,64p' | tr '\n' ' '
printf '\n'
done
printf '%s\n' '--- AST verifier: cache branch order and constructor arguments ---'
python3 - <<'PY'
import ast
from pathlib import Path
source = Path("src/tablassert/qc.py").read_text()
tree = ast.parse(source)
fn = next(
node for node in ast.walk(tree)
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef))
and node.name == "get_biobert"
)
if not fn.body or not isinstance(fn.body[0], ast.If):
raise SystemExit("get_biobert does not start with a conditional cache branch")
branch = fn.body[0]
test = ast.unparse(branch.test)
print("first branch test:", test)
for label, statements in (("cache-hit", branch.body), ("cache-miss", branch.orelse)):
calls = [
ast.unparse(node.value.func) + "(" + ", ".join(ast.unparse(arg) for arg in node.value.args) + ")"
for node in ast.walk(ast.Module(body=statements, type_ignores=[]))
if isinstance(node, ast.Expr) and isinstance(node.value, ast.Call)
]
print(label, "calls:", calls)
for node in ast.walk(ast.Module(body=statements, type_ignores=[])):
if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) and node.func.attr == "SentenceTransformer":
print(label, "SentenceTransformer args:", [ast.unparse(arg) for arg in node.args])
print(label, "SentenceTransformer keywords:", [(kw.arg, ast.unparse(kw.value)) for kw in node.keywords])
PYRepository: SkyeAv/Tablassert
Length of output: 4491
🌐 Web query:
SentenceTransformers SentenceTransformer revision parameter omitted latest revision official documentation
💡 Result:
In the SentenceTransformers library, the revision parameter in the SentenceTransformer class allows you to specify a particular version of a model (such as a branch name, tag name, or commit ID) hosted on the Hugging Face Hub [1][2]. When the revision parameter is omitted, it defaults to None [1][3]. In practice, this means the library will default to using the "main" revision (the default branch) of the model repository on the Hugging Face Hub, as inherited from the underlying Hugging Face Transformers loading mechanisms [4]. Key points regarding the revision parameter: - Purpose: It enables reproducible loading by specifying a fixed model version [5]. - Default behavior: Omitted arguments default to None, resulting in the default "main" branch being fetched [1][4]. - Implementation: The parameter is available in the SentenceTransformer constructor and is propagated to relevant Hugging Face Hub and Transformers functions to handle the specific versioning [2][5][6].
Citations:
- 1: https://sbert.net/docs/package_reference/sentence_transformer/model.html
- 2: https://github.com/huggingface/sentence-transformers/blob/main/sentence_transformers/sentence_transformer/model.py
- 3: https://sbert.net/docs/package_reference/base/model.html
- 4: https://huggingface.co/docs/transformers/en/main_classes/model
- 5: [
feat] Add revision to load a specific model version huggingface/sentence-transformers#2419 - 6: https://github.com/huggingface/sentence-transformers/blob/main/sentence_transformers/models/Transformer.py
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import ast
from pathlib import Path
source = Path("src/tablassert/qc.py").read_text()
tree = ast.parse(source)
fn = next(node for node in ast.walk(tree) if isinstance(node, ast.FunctionDef) and node.name == "get_biobert")
try_node = next(node for node in ast.walk(fn) if isinstance(node, ast.Try))
if_node = next(node for node in try_node.body if isinstance(node, ast.If))
print("cache condition:", ast.unparse(if_node.test))
def constructor_calls(statements):
result = []
for node in ast.walk(ast.Module(body=statements, type_ignores=[])):
if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) and node.func.attr == "SentenceTransformer":
result.append({
"args": [ast.unparse(arg) for arg in node.args],
"keywords": [(kw.arg, ast.unparse(kw.value)) for kw in node.keywords],
})
return result
print("cache-hit constructor:", constructor_calls(if_node.body))
print("cache-miss constructor:", constructor_calls(if_node.orelse))
print("cache-miss saves model:", any(
isinstance(node, ast.Call)
and isinstance(node.func, ast.Attribute)
and node.func.attr == "save"
for node in ast.walk(ast.Module(body=if_node.orelse, type_ignores=[]))
))
# A small model of the two branches. This does not import or execute repository code.
def modeled_loader(cache_exists, repo, cache_path):
calls = []
saves = []
if cache_exists:
calls.append(("local", cache_path))
else:
calls.append(("hub", repo))
saves.append(cache_path)
return calls, saves
repo = "pritamdeka/BioBERT-mnli-snli-scinli-scitail-mednli-stsb"
print("modeled existing-cache behavior:", modeled_loader(True, repo, ".tablassert/biobert"))
print("modeled missing-cache behavior:", modeled_loader(False, repo, ".tablassert/biobert"))
PYRepository: SkyeAv/Tablassert
Length of output: 598
Key the local cache by the model identity.
get_biobert() loads .tablassert/biobert before using the new repository identifier. A stale cache can load a different embedding model and silently change QC results.
Pin an immutable revision and store or validate the repository and revision in the cache metadata or path. Add a regression test for stale-cache reuse.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tablassert/qc.py` at line 60, Update get_biobert() so the cache is keyed
and validated by the model repository and an immutable revision, rather than
loading the shared .tablassert/biobert path unconditionally. Ensure stale
metadata or a mismatched cache is ignored and the requested model is downloaded,
then add a regression test covering stale-cache reuse.
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
1 participant
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.
tablassert agent --optimizenow runsdspy.GEPAend-to-end against real fullmap coverage instead of crashing or optimizing a validity-only proxy, and commits the hardened prompt it produced (10/10 MAPPED, 0.974 mean best coverage on the QC assay) with reproduction artifacts.GEPA metric + real coverage scoring
gepa_metricnow accepts dspy.GEPA's(gold, pred, trace, pred_name, pred_trace)positional shape; the old one-arg bundle made realdspy.GEPA(metric=gepa_metric)raiseTypeError: GEPA metric must accept five arguments(the offline suite passed only because it injects agepa_clsstub). The legacy one-dict call shape still works._gepa_bundle_from_dspyscores each candidateconfig_yamlwith real fullmap coverage viabuild_and_auditwhen the example carries afullmappath; falls back to the validity-only floor without one. Never raises — a bad candidate scores validity-only instead of aborting the compile.head: falserestores full fidelity. Examples may also carryworkdirso a candidate's relativesource.localresolves against the real download dir.os.chdiris process-global, so metric builds serialize on_GEPA_BUILD_LOCKwhile LLM forward passes stay parallel.LM split + CLI flags
--task-model: fast LM for GEPA's many program evaluations, with--model-idas the strong reflection LM (GEPA best practice); defaults to the reflection LM when unset.--gepa-threadsparallelizes the evaluation pool.GEPA_TASK_TEMPERATURE=0.3/GEPA_REFLECTION_TEMPERATURE=1.0— measured 3/3 schema-valid configs at 0.3 forqwen3.6-flashvs 1/3 at both 0.0 and 1.0 (schema validity is the metric's hard gate).make_dspy_lmnow passestemperature/max_tokens=16000/timeout=600so a reasoning model's internal CoT can't truncateconfig_yamlmid-output (which stalls the optimizer's output parsing).Build hardening (prerequisite)
build_agentraises the smolagents local-executor timeout from the 30s default to 600s (execution_timeout) — 30s killedbuild_and_auditon large tables (~60s for a 37k-row sheet) mid-build, stranding the fullmap redb lock and failing every later build.fullmap.pywraps redb-backed reads in_call_with_lock_retry(10 attempts, linear backoff) so transientDatabase already opencontention from a just-finished build can no longer surface as a false 0.0 coverage.build_and_auditresolvesworkdir(a relative one breaks.tablassert/storeparquets after the internalchdir), the supervisor presents absolute candidate-table paths, and the CLI resolves--instructions-outbefore GEPA's chdirs. Coverage measurement also retries 3× with gc + backoff on transient frame-reproduction failure instead of reporting a false 0.0.Derive modes
derive_mode:make_tools/run_supervisoracceptfull(default, unchanged) |derive_only(no fullmap tools → parallel derivations, serial build pass later) |derive_coverage(coverage feedback without the KGX build); derive modes terminate in the newDERIVEDstatus.derive_onlycan't see coverage while deriving (suboptimal for multi-sheet tables) — that's the documented trade-off.Artifacts + QC
examples/agent/: committedoptimized_instructions.yaml(feedback-derived:source.url-required rule,prioritizemapping table, per-error-code recovery cheat-sheet, source-path-fidelity rule) andgepa-dataset.yaml(two open-access PMC gene tables); loadable via--instructions-fileto skip the optimization cost.qc/qc_report.pyassay → 10/10 MAPPED, 0.974 mean best coverage (QC_REPORT.md);qc/qc_reviewer.pyLLM-as-judge pass over the final configs (QC_REVIEW.md: 1 good / 4 acceptable / 5 poor — the standing improvement backlog). Report and review are co-generated from the same state dir and cross-linked by a per-configsha256, so future report/review/config drift is detectable.CodeRabbit feedback (round 1)
derive_modeis nowDeriveMode = Literal["full", "derive_only", "derive_coverage"]in bothmake_toolsandrun_supervisor— a typo fails in pyright instead of silently falling through tofull.--gepa-threads< 1 exits 2 before any model is built, matching the--judge-thresholdfail-loud pattern.build_and_audit's coverage retry no longer retries lock-contention errors —_call_with_lock_retryalready burned its backoff budget before the error escaped (newfullmap.is_lock_contention); matters while holding_GEPA_BUILD_LOCK.--gepa-threadsdocumented as parallelizing LM forward passes only (builds stay serialized on_GEPA_BUILD_LOCK); the README shell example no longer carries an inline comment in a continuation line.qc_report.pyredacts absolute paths (<state-dir>/…/<local-path>);qc_reviewer.pyprepends an untrusted-data system message, readssource.localonly under the QCdownloads/dir, and bounds each litellm call (timeout=300,num_retries=2).CodeRabbit feedback (round 2)
validate_review_resultrequires dimension mappings, scores in0..3, andoverall_qualityingood|acceptable|poor; anything else is recorded as a failed review (both the primary and fallback JSON paths).config sha256too.get_table_summaryderivesmax_colsfrom the configuredsubject/object/annotation columns (PMC7206184'sS/Tcolumns are no longer cut at 12, capped atread_table's 40) and accumulates ALL sections instead of returning on the first readable one.redact_pathsgeneralized to any absolute local path (URL/ratio-safe lookbehind);qc_reviewer.pysanitizes every review string — including parse-error content — through it before JSON + markdown.3000 → 8000; new CI test parses every fenced yaml block in the committedQC_REPORT.md.downloads/, and the QC scripts only readsource.localunder their ownSTATE_DIR/downloads; point them at the same state dir or stage tables there.--gepa-threadstest now also stubsmake_dspy_lmto fail (proves NO model construction pre-validation); the backend-forwarding test asserts the single reflection-LM construction explicitly; the dspy-LM test asserts thetimeout=600default.Fixes
get_biobert()'s download path pointed atpritamdeka/BioBERT-mnli-snli-scitail-mednli-stsb, which no longer exists (HF API returns 401), so a fresh judge-embedding cache could never download. Corrected everywhere to the real sentence-transformers repopritamdeka/BioBERT-mnli-snli-scinli-scitail-mednli-stsb(API 200, ~33k downloads) insrc/tablassert/qc.py,docs/api/qc.md, andtests/test_cover_qc.py. The cache dir (.tablassert/biobert/) is name-independent, so no existing cache is orphaned.Docs
docs/agent.md:--optimizesection rewritten for the LM split and dataset fields (fullmap/workdir/head).docs/cli.md:--task-model,--gepa-threads, and extended--datasetrows.Testing
uv run pytest -q --no-cov→745 passed(full suite).uv run pytest tests/test_agent_cli.py tests/test_agent_eval.py -q --no-cov→47 passed— includes regressions for the five-arg metric contract, validity scoring without a fullmap, the head-sample default +head: falseoverride,task_lmwinning overreflection_lmindspy.configure, andnum_threadsforwarding.tests/test_example_qc.py→11 passed: downloads allowlist (reject-without-read + allowed-read), column-derivedmax_cols(floor 12 / cap 40 / fixed-value encodings ignored), multi-section accumulation, judge-response validation (bad shape / out-of-range score / bool score / bad quality),redact_pathsbehavior, committedQC_REPORT.mdyaml fences parse, and report↔review sha256 cross-check.uv run ruff check .,uv run ruff format --check .,uv run pyright→ all clean; pre-commit hooks (ruff/format/pyright/pytest) green on the final commit..tablassert/qc-assay: report re-run deterministically; review re-rendered offline fromqc_review.json(--rerender); sha cross-check →mismatches: none.Questions for the reviewer
derive_modeCLI surface. The derive modes are currently Python-API-only — add a--derive-modeflag + docs in this PR, or follow-up?Summary by CodeRabbit
New Features
Bug Fixes