Explicit benchmark, relative-path sqlite fix, formula-aware encode ordering - #8
Merged
ZmeiGorynych merged 8 commits intoMay 29, 2026
Merged
Conversation
…are encode ordering
Three independent fixes surfaced while running LiveSQLBench locally:
A. Explicit benchmark (no None default). `paths.slayer_otf_cache_root` /
`slayer_models_otf_root` now require an explicit `benchmark` ("mini_interact"
or "livesqlbench"); a forgotten benchmark can no longer silently fall back to
— and mix artifacts with — mini-interact. "mini_interact" maps to the legacy
dirs + env vars, so on-disk layout and the cloud contract are unchanged. All
callers pass it explicitly: run.py, both on-the-fly agents, and the cloud
modules via new `driver._submit_benchmark(args)` / `ray_app._cloud_benchmark(cfg)`
helpers (derive from the run's dataset; "mini_interact" today, "livesqlbench"
automatically once a dataset is plumbed through).
B. Relative --db-path no longer breaks ingest. `orchestrator._phase1_ingest`
built `sqlite:////{path}`; with a relative path that resolved at the
filesystem root ("unable to open database file"). New
`portable_connection.absolute_sqlite_url` resolves to a canonical absolute
URL (env-independent) and the orchestrator canonicalizes the persisted
datasource YAML; run.py resolves --db-path once at the CLI boundary so the
README's relative commands work.
C. Encode ordering uses formula references; strict write gate. The KB
dependency DAG now unions `children_knowledge` with cross-references parsed
from each KB's `definition` formula (\text{ABBR} tokens resolved to other KB
ids), with ambiguous-abbreviation and raw-column suppression — so a dependent
is encoded after the columns it references exist. The validate-before-persist
gate drops the fail-open "engine unhealthy -> skip" escape that silently
persisted broken cross-refs; a failure always blocks (encoder self-corrects or
defers), and an infra/connection-class failure is logged at WARNING.
Full non-integration suite green (1413 passed). LiveSQLBench haiku smoke with a
relative --db-path ingests cleanly with no engine-unhealthy-skip warnings.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEnforce explicit benchmark tokens across path helpers and cloud/agent wiring; tighten validate-before-persist to always block on validation failures (infra-like failures WARN); canonicalize SQLite URIs; derive KB dependency edges from formula tokens; add deterministic benchmark-data upload/download; update image build and tests to the de-baked dataset model. ChangesBenchmark-scoped OTF artifacts and validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/bird_interact_agents/slayer_otf/reference_build.py`:
- Around line 158-179: _filter out derived/computed columns in
_raw_base_columns: when iterating models from storage.list_models and columns
from storage.get_model(...).columns, skip adding a column name if the column
object indicates it is derived (e.g. hasattr flags like c.is_derived or
c.derived) or if it has a formula/expression (getattr(c, "formula", None) or
getattr(c, "expression", None) is not None); only add truly base/raw column
names to the cols set so derived columns do not suppress KB term references.
Ensure checks are defensive (use getattr with defaults) so missing attributes
don’t raise.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b201a6d2-d486-4441-954f-989ac47624dd
📒 Files selected for processing (21)
src/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.pysrc/bird_interact_agents/agents/pydantic_ai_recursive/agent.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/cloud/upload_back.pysrc/bird_interact_agents/paths.pysrc/bird_interact_agents/run.pysrc/bird_interact_agents/slayer_otf/reference_build.pysrc/bird_interact_agents/slayer_pipeline/orchestrator.pysrc/bird_interact_agents/slayer_pipeline/portable_connection.pytests/cloud/test_driver.pytests/cloud/test_ray_app.pytests/cloud/test_upload_back.pytests/slayer_pipeline/test_orchestrator_conn.pytests/slayer_pipeline/test_portable_connection.pytests/test_cloud_paths_unchanged.pytests/test_literal_existence_validation.pytests/test_otf_rebuild_wiring.pytests/test_paths.pytests/test_pydantic_ai_otf_encode_agent.pytests/test_slayer_otf_reference_build.py
…t (v6→v7) Pure storage-format bump emitted by SLayer 0.6.10 on load/save (version 6→7 plus cosmetic re-wrapping of long description strings); no semantic change to any model, datasource, connection_string, or column. Committing the v7 copies keeps the committed models in sync with the pinned `motley-slayer>=0.6.10` and stops the test suite re-bumping them on every load (recurring git-status churn). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduce a single Benchmark registry (src/bird_interact_agents/benchmark.py) — a frozen Pydantic descriptor per benchmark (name, aliases, dataset marker, data root/file + env conventions, supported modes, one_shot, gold_required, per_task_db_isolation, container_data_dir) + get_benchmark() resolving canonical name / CLI alias / dataset marker. Underscore-canonical tokens (mini_interact, livesqlbench); mini-interact (hyphen) is a back-compat CLI alias. Local consumers now read the registry instead of scattered "livesqlbench" literals: - paths.py: generic benchmark_data_root/benchmark_data_file; the per-benchmark mini_interact_*/livesqlbench_* helpers become thin shims; _KNOWN_BENCHMARKS derives from the registry. - run.py: _validate_dataset_mode is registry-driven (mode in supported_modes); --dataset choices/default + canonical normalization; gold-required checks and benchmark_for_paths via get_benchmark; loader dispatch via load_benchmark_tasks. - harness.py: load_benchmark_tasks() — the single benchmark-aware loader dispatch both the local runner and (next) the cloud actor call. Foundation for cloud↔local benchmark symmetry. Full non-integration suite green (1423 passed); codex-reviewed (no findings). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ated) New cloud/benchmark_data.py: a benchmark's static dataset is uploaded ONCE to a stable content-hashed prefix `benchmark-data/<benchmark>/<hash>/` (upload-if- absent at submit) and downloaded if-absent per node — instead of being baked into every code image. A completeness marker is written LAST on upload and only trusted on download, so a partial/concurrent transfer can't be mistaken for a finished one (mirrors the OTF cache `_cache_fp.txt` invariant). Reuses the existing gcs.upload_dir_prefix / download_prefix primitives. Foundation for the cloud data-delivery rework (Phase 4); wired into driver/ray_app + image slimming in the follow-up commits. Tested with an in-memory GCS fake (6 tests); full suite green (1429 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/bird_interact_agents/run.py (1)
707-712: ⚡ Quick winMake the gold-file error benchmark-aware instead of hardcoding
livesqlbench.Line 707 and Line 1216 gate on
gold_required, but the error message hardcodes a single dataset name. This becomes misleading if another benchmark is markedgold_required=True.Proposed patch
- if get_benchmark(dataset).gold_required and not gold_file: + b = get_benchmark(dataset) + if b.gold_required and not gold_file: raise ValueError( - "--dataset livesqlbench requires --gold-file (the gated sidecar " + f"--dataset {b.name} requires --gold-file (the gated sidecar " "carrying sol_sql / external_knowledge / test_cases keyed by " "instance_id)", )- if get_benchmark(args.dataset).gold_required and not args.gold_file: + b = get_benchmark(args.dataset) + if b.gold_required and not args.gold_file: raise ValueError( - "--dataset livesqlbench requires --gold-file (the gated " + f"--dataset {b.name} requires --gold-file (the gated " "sidecar carrying sol_sql / external_knowledge / test_cases " "keyed by instance_id).", )Also applies to: 1216-1221
🤖 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/bird_interact_agents/run.py` around lines 707 - 712, The error message raised when a benchmark requires a gold file is hardcoded to "livesqlbench"; update the raise ValueError branches that check get_benchmark(dataset).gold_required and not gold_file (e.g., the checks around the callers of get_benchmark and the variables dataset/gold_file) to include the actual benchmark name or dataset variable instead of the hardcoded string—e.g., construct the message using dataset or get_benchmark(dataset).name and mention "--gold-file" and the expected contents (sol_sql / external_knowledge / test_cases keyed by instance_id) so the error is accurate for any benchmark with gold_required=True.
🤖 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/bird_interact_agents/harness.py`:
- Around line 218-222: The current code calls load_tasks(data_path, limit) which
applies the limit before respecting filter_ids, so requested instance_ids can be
dropped; change the logic in harness.py so you call load_tasks without the limit
(or with a very large/unbounded value), then if filter_ids is not None filter
the tasks by t.get("instance_id") in wanted, and only after that apply the limit
to the filtered tasks before returning; update the handling around the tasks
variable and the return to ensure limit is applied post-filtering (preserve
existing behavior when filter_ids is None).
In `@tests/test_benchmark.py`:
- Around line 53-56: The test test_benchmark_is_frozen should assert the
specific pydantic.ValidationError instead of the broad Exception: change the
pytest.raises(Exception) wrapper around the assignment to b.name to
pytest.raises(ValidationError) and add an import for ValidationError from
pydantic (or adjust existing imports) so the test verifies immutability raised
by ConfigDict(frozen=True) when get_benchmark() returns the model and b.name =
"mutated" triggers the expected ValidationError.
---
Nitpick comments:
In `@src/bird_interact_agents/run.py`:
- Around line 707-712: The error message raised when a benchmark requires a gold
file is hardcoded to "livesqlbench"; update the raise ValueError branches that
check get_benchmark(dataset).gold_required and not gold_file (e.g., the checks
around the callers of get_benchmark and the variables dataset/gold_file) to
include the actual benchmark name or dataset variable instead of the hardcoded
string—e.g., construct the message using dataset or get_benchmark(dataset).name
and mention "--gold-file" and the expected contents (sol_sql /
external_knowledge / test_cases keyed by instance_id) so the error is accurate
for any benchmark with gold_required=True.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8110e3cd-570c-4824-9da1-b57e6639ba31
📒 Files selected for processing (80)
slayer_models/alien/models/alien/observationalconditions.yamlslayer_models/alien/models/alien/observatories.yamlslayer_models/alien/models/alien/researchprocess.yamlslayer_models/alien/models/alien/signal_aidp.yamlslayer_models/alien/models/alien/signal_ccs.yamlslayer_models/alien/models/alien/signal_ccs_approx.yamlslayer_models/alien/models/alien/signal_cip_class.yamlslayer_models/alien/models/alien/signal_eci.yamlslayer_models/alien/models/alien/signal_hzsr.yamlslayer_models/alien/models/alien/signal_ier.yamlslayer_models/alien/models/alien/signal_is_anomalous_quantum.yamlslayer_models/alien/models/alien/signal_is_cip.yamlslayer_models/alien/models/alien/signal_is_directed_transmission.yamlslayer_models/alien/models/alien/signal_is_eit.yamlslayer_models/alien/models/alien/signal_is_frt.yamlslayer_models/alien/models/alien/signal_is_galactic_significance.yamlslayer_models/alien/models/alien/signal_is_habitable_zone_transmission.yamlslayer_models/alien/models/alien/signal_is_high_confidence.yamlslayer_models/alien/models/alien/signal_is_multi_channel_protocol.yamlslayer_models/alien/models/alien/signal_is_observation_verified.yamlslayer_models/alien/models/alien/signal_is_quantum_coherent.yamlslayer_models/alien/models/alien/signal_is_technosig_high_conf.yamlslayer_models/alien/models/alien/signal_is_technosignature.yamlslayer_models/alien/models/alien/signal_spei.yamlslayer_models/alien/models/alien/signaladvancedphenomena.yamlslayer_models/alien/models/alien/signalclassification.yamlslayer_models/alien/models/alien/signaldecoding.yamlslayer_models/alien/models/alien/signaldynamics.yamlslayer_models/alien/models/alien/signalprobabilities.yamlslayer_models/alien/models/alien/signals.yamlslayer_models/alien/models/alien/sourceproperties.yamlslayer_models/alien/models/alien/telescopes.yamlslayer_models/households/models/households/amenities.yamlslayer_models/households/models/households/households.yamlslayer_models/households/models/households/infrastructure.yamlslayer_models/households/models/households/locations.yamlslayer_models/households/models/households/properties.yamlslayer_models/households/models/households/service_types.yamlslayer_models/households/models/households/transportation_assets.yamlslayer_models/polar/models/polar/cabinenvironment.yamlslayer_models/polar/models/polar/chassisandvehicle.yamlslayer_models/polar/models/polar/communication.yamlslayer_models/polar/models/polar/comprehensive_environmental_adaptability_rating.yamlslayer_models/polar/models/polar/comprehensive_operational_reliability_indicator.yamlslayer_models/polar/models/polar/critical_infrastructure_protection_level.yamlslayer_models/polar/models/polar/critical_scientific_equipment_status.yamlslayer_models/polar/models/polar/energy_water_resource_integration_index.yamlslayer_models/polar/models/polar/engineandfluids.yamlslayer_models/polar/models/polar/equipment.yamlslayer_models/polar/models/polar/extreme_climate_adaptation_coefficient.yamlslayer_models/polar/models/polar/extreme_operating_conditions.yamlslayer_models/polar/models/polar/extreme_weather_ready.yamlslayer_models/polar/models/polar/life_support_reliability_classification.yamlslayer_models/polar/models/polar/life_support_system_reliability.yamlslayer_models/polar/models/polar/lightingandsafety.yamlslayer_models/polar/models/polar/location.yamlslayer_models/polar/models/polar/long_term_scientific_mission_viability.yamlslayer_models/polar/models/polar/operationmaintenance.yamlslayer_models/polar/models/polar/polar_base_energy_security_status.yamlslayer_models/polar/models/polar/polar_transportation_efficiency_coefficient.yamlslayer_models/polar/models/polar/polar_vehicle_safe_operation_conditions.yamlslayer_models/polar/models/polar/powerbattery.yamlslayer_models/polar/models/polar/resource_self_sufficiency_index.yamlslayer_models/polar/models/polar/scientific.yamlslayer_models/polar/models/polar/scientific_mission_success_probability.yamlslayer_models/polar/models/polar/sustainable_polar_operations.yamlslayer_models/polar/models/polar/thermalsolarwindandgrid.yamlslayer_models/polar/models/polar/transmission.yamlslayer_models/polar/models/polar/waterandwaste.yamlslayer_models/polar/models/polar/weatherandstructure.yamlsrc/bird_interact_agents/benchmark.pysrc/bird_interact_agents/harness.pysrc/bird_interact_agents/paths.pysrc/bird_interact_agents/run.pytests/test_benchmark.pytests/test_one_shot_mode.pytests/test_pydantic_ai_otf_encode_run_wiring.pytests/test_run_aggregation.pytests/test_run_db_integration.pytests/test_timing.py
✅ Files skipped from review due to trivial changes (58)
- slayer_models/alien/models/alien/signal_ier.yaml
- slayer_models/alien/models/alien/signal_eci.yaml
- slayer_models/polar/models/polar/life_support_reliability_classification.yaml
- slayer_models/alien/models/alien/signal_ccs.yaml
- slayer_models/polar/models/polar/lightingandsafety.yaml
- slayer_models/households/models/households/locations.yaml
- slayer_models/households/models/households/service_types.yaml
- slayer_models/households/models/households/households.yaml
- slayer_models/polar/models/polar/equipment.yaml
- slayer_models/alien/models/alien/sourceproperties.yaml
- slayer_models/alien/models/alien/signal_is_frt.yaml
- slayer_models/alien/models/alien/signaladvancedphenomena.yaml
- slayer_models/alien/models/alien/signaldynamics.yaml
- slayer_models/households/models/households/amenities.yaml
- slayer_models/alien/models/alien/observationalconditions.yaml
- slayer_models/alien/models/alien/signal_aidp.yaml
- slayer_models/polar/models/polar/scientific.yaml
- slayer_models/polar/models/polar/transmission.yaml
- slayer_models/alien/models/alien/signalprobabilities.yaml
- slayer_models/polar/models/polar/scientific_mission_success_probability.yaml
- slayer_models/alien/models/alien/signal_is_observation_verified.yaml
- slayer_models/alien/models/alien/signal_spei.yaml
- slayer_models/alien/models/alien/signal_is_galactic_significance.yaml
- slayer_models/polar/models/polar/location.yaml
- slayer_models/polar/models/polar/extreme_climate_adaptation_coefficient.yaml
- slayer_models/polar/models/polar/thermalsolarwindandgrid.yaml
- slayer_models/polar/models/polar/cabinenvironment.yaml
- slayer_models/households/models/households/transportation_assets.yaml
- slayer_models/alien/models/alien/signaldecoding.yaml
- slayer_models/alien/models/alien/signal_ccs_approx.yaml
- slayer_models/alien/models/alien/signal_is_high_confidence.yaml
- slayer_models/alien/models/alien/signal_is_habitable_zone_transmission.yaml
- slayer_models/alien/models/alien/observatories.yaml
- slayer_models/polar/models/polar/communication.yaml
- slayer_models/polar/models/polar/energy_water_resource_integration_index.yaml
- slayer_models/polar/models/polar/sustainable_polar_operations.yaml
- slayer_models/polar/models/polar/chassisandvehicle.yaml
- slayer_models/polar/models/polar/waterandwaste.yaml
- slayer_models/alien/models/alien/signal_cip_class.yaml
- slayer_models/polar/models/polar/critical_infrastructure_protection_level.yaml
- slayer_models/polar/models/polar/operationmaintenance.yaml
- slayer_models/polar/models/polar/comprehensive_environmental_adaptability_rating.yaml
- slayer_models/polar/models/polar/powerbattery.yaml
- slayer_models/alien/models/alien/signal_is_quantum_coherent.yaml
- slayer_models/polar/models/polar/resource_self_sufficiency_index.yaml
- slayer_models/polar/models/polar/critical_scientific_equipment_status.yaml
- slayer_models/alien/models/alien/telescopes.yaml
- slayer_models/alien/models/alien/researchprocess.yaml
- slayer_models/polar/models/polar/extreme_operating_conditions.yaml
- slayer_models/polar/models/polar/weatherandstructure.yaml
- slayer_models/alien/models/alien/signal_is_technosignature.yaml
- slayer_models/alien/models/alien/signal_is_technosig_high_conf.yaml
- slayer_models/polar/models/polar/polar_vehicle_safe_operation_conditions.yaml
- slayer_models/polar/models/polar/life_support_system_reliability.yaml
- slayer_models/polar/models/polar/comprehensive_operational_reliability_indicator.yaml
- slayer_models/alien/models/alien/signal_is_cip.yaml
- slayer_models/polar/models/polar/long_term_scientific_mission_viability.yaml
- tests/test_one_shot_mode.py
Contributor
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/bird_interact_agents/cloud/benchmark_data.py`:
- Around line 75-81: The current fast-path treats any existing _MARKER file in
dest as a cache hit; change the logic in the function that sets
dest/local_marker to read local_marker.read_text() and only return dest if that
content equals prefix; if it differs, remove or empty dest (e.g., delete files
under dest) before calling gcs.download_prefix(prefix, dest, client=client) and
then rewrite local_marker with prefix; keep using client = client or
gcs.default_gcs_client() and ensure you still call
local_marker.write_text(prefix) after a successful download.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab04e887-db5a-40ec-9493-7f40801e5f39
📒 Files selected for processing (2)
src/bird_interact_agents/cloud/benchmark_data.pytests/cloud/test_benchmark_data.py
The cloud submit/actor now carry the benchmark (and its gold sidecar) instead of being hardcoded mini-interact: - cli.py: --dataset (registry tokens + mini-interact alias) + --gold-file + one-shot mode; reuses the local _validate_dataset_mode / _validate_one_shot_framework gates; the audited-gold submit check is skipped for gold-sidecar benchmarks (livesqlbench has no audited_gold). - driver.py: build_manifest carries dataset + gold_file; _build_job_args / _build_resubmit_args pass --dataset/--gold-file to the actor; the instance→db map, db-grouped dispatch, and the deterministic-cache auto-build all read the benchmark's data root/file (registry-driven, mini_interact default for back-compat). - ray_app.py: the actor accepts --dataset/--gold-file and loads tasks via the benchmark-aware harness.load_benchmark_tasks (livesqlbench → gold merge + dataset stamp + SELECT filter); the audited-gold overlay stays mini-only. mini-interact behaviour is unchanged (it's the default everywhere). Still TODO for full livesqlbench cloud: per-node GCS dataset download + cluster env + image de-bake. Full suite green (1429); cloud tests +7 new (cli/driver). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bird_interact_agents/cloud/driver.py (1)
203-212:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on unknown dataset tokens in
_benchmark_for_dataset.This helper currently maps any unexpected non-
livesqlbenchvalue tomini_interact. That can silently misroute artifact roots and retry task ordering if a manifest contains an invalid dataset value. Keep the legacyNone -> mini_interactfallback, but reject unknown explicit tokens.Proposed fix
def _benchmark_for_dataset(dataset: str | None) -> str: @@ - return "livesqlbench" if dataset == "livesqlbench" else "mini_interact" + if dataset is None: + return "mini_interact" + if dataset in {"mini_interact", "mini-interact"}: + return "mini_interact" + if dataset == "livesqlbench": + return "livesqlbench" + raise ValueError(f"unknown dataset token in manifest: {dataset!r}")🤖 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/bird_interact_agents/cloud/driver.py` around lines 203 - 212, The function _benchmark_for_dataset currently maps any non-"livesqlbench" value to "mini_interact", which hides invalid explicit dataset tokens; change _benchmark_for_dataset so that dataset is allowed to be None (return "mini_interact"), returns "livesqlbench" only when dataset == "livesqlbench", and raises a ValueError (or similar) for any other non-None explicit token to fail fast and surface misconfigured manifests.
🤖 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/bird_interact_agents/cloud/ray_app.py`:
- Around line 1119-1120: The CLI currently accepts any string for --dataset and
lets failure occur later; update the argument parsing in ray_app.main() so the
parser (p) constrains --dataset with an explicit choices list of supported
tokens and then immediately canonicalizes the parsed value to the canonical
benchmark name (mirror cloud.cli behavior) before any further use; specifically
add choices/defaults on p.add_argument("--dataset", ...) and after args =
p.parse_args() call the same normalization function used by cloud.cli (e.g.,
cloud.cli.canonicalize_dataset or equivalent) to replace args.dataset with the
canonical name, and apply the same validation/normalization for the other parser
instance that defines --dataset/--gold-file (the other add_argument block around
the duplicate parser usage).
---
Outside diff comments:
In `@src/bird_interact_agents/cloud/driver.py`:
- Around line 203-212: The function _benchmark_for_dataset currently maps any
non-"livesqlbench" value to "mini_interact", which hides invalid explicit
dataset tokens; change _benchmark_for_dataset so that dataset is allowed to be
None (return "mini_interact"), returns "livesqlbench" only when dataset ==
"livesqlbench", and raises a ValueError (or similar) for any other non-None
explicit token to fail fast and surface misconfigured manifests.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 631dec69-4542-4b73-9f4b-155229bee1a9
📒 Files selected for processing (6)
src/bird_interact_agents/cloud/cli.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/ray_app.pytests/cloud/test_cli.pytests/cloud/test_driver.pytests/cloud/test_ray_app.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cloud/test_ray_app.py
…nchmark) The cloud runner no longer bakes the benchmark dataset into the image. It is uploaded once to a content-hashed GCS prefix at submit (benchmark_data.ensure_uploaded), recorded in the manifest, and downloaded per node (head before task-load + each worker before ingest) via ray_app.download_benchmark_data, which points the benchmark's data-root/data-file env vars at the downloaded tree. One image+cluster now serves any benchmark. The gated gold sidecar lives inside the data root, so it rides along in that GCS upload (driver._validate_gold_under_data_root fails fast if --gold-file is outside the root; _in_cluster_gold_file maps it to its container path, stored in the manifest so resubmit works anywhere). audited_gold/ corrections stay baked (code-like). - image.py: data_hash/image_tag/build_and_push drop mini_interact_root + the mini-interact build context; only audited_gold baked. - Dockerfile.cloud + cluster.yaml.j2: remove the dataset COPY and the pinned BIRD_DB_PATH/BIRD_DATA_PATH (set by the actor at runtime). - driver: submit ensure_uploads the dataset and threads --benchmark-data-prefix through manifest/job-args/resubmit; benchmark tokens resolve via the registry. - ray_app: run_pool cfg carries dataset + prefix + a benchmark-correct data_dir; _run_one_in_actor reads cfg["data_dir"] (not BIRD_DB_PATH). - benchmark_data/gcs: exclude .git/ from content_hash + upload (livesqlbench's data dir is its own checkout); ensure_uploaded refuses an empty/data-file-less root; ensure_downloaded requires the remote completeness marker. Full non-integration suite green. Mini-interact cloud behaviour preserved (prefix absent on pre-de-bake manifests → reuse the baked image, no download). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
@CodeRabbit review this! |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/bird_interact_agents/cloud/benchmark_data.py (1)
113-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate marker content before treating local cache as fresh.
At Line 113, any existing
_benchmark_data.markercauses an unconditional cache hit. Ifdestcontains data for an older prefix, this silently reuses stale files.Proposed fix
import hashlib +import shutil from pathlib import Path @@ def ensure_downloaded(prefix: str, dest: Path, *, client=None) -> Path: @@ dest = Path(dest) local_marker = dest / _MARKER if local_marker.is_file(): - return dest + if local_marker.read_text() == prefix: + return dest + shutil.rmtree(dest, ignore_errors=True) + dest.mkdir(parents=True, exist_ok=True) client = client or gcs.default_gcs_client() remote_marker = client.bucket(gcs.BUCKET_NAME).blob(prefix + _MARKER)🤖 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/bird_interact_agents/cloud/benchmark_data.py` around lines 113 - 124, The function currently treats any existing local_marker as a cache hit; change it to validate the marker contents by reading local_marker.read_text().strip() and comparing it to prefix (use exact string equality) before returning dest; if they differ, proceed to check remote_marker.exists(), download via gcs.download_prefix(prefix, dest, client=client) and then overwrite local_marker.write_text(prefix). Ensure you reference local_marker, _MARKER, prefix, dest, remote_marker and gcs.download_prefix in the fix.src/bird_interact_agents/cloud/ray_app.py (1)
1180-1225:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate and canonicalize
--datasetin the actor CLI before loading tasks.
ray_app.main()still accepts arbitrary dataset tokens and alias forms, so direct actor invocations can fail deep indownload_benchmark_data()/_load_task_data()instead of at parse time. This boundary should also reject missing--gold-filefor gold-required benchmarks before any download or task-load work starts.Suggested fix
+ from bird_interact_agents.benchmark import cli_dataset_tokens, get_benchmark + p = argparse.ArgumentParser() @@ - p.add_argument("--dataset", default="mini_interact") + p.add_argument("--dataset", choices=cli_dataset_tokens(), default="mini_interact") @@ args = p.parse_args(argv) + args.dataset = get_benchmark(args.dataset).name + if get_benchmark(args.dataset).gold_required and not args.gold_file: + p.error("--dataset livesqlbench requires --gold-file") actor_env_vars = _load_secrets_file(args.secrets_file)🤖 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/bird_interact_agents/cloud/ray_app.py` around lines 1180 - 1225, Validate and canonicalize args.dataset in ray_app.main() immediately after parsing (before calling download_benchmark_data or _load_task_data): map known alias tokens to their canonical dataset names (e.g., via the project's existing dataset-resolver helper or a small lookup), reject unknown tokens with a clear error and SystemExit, and if the resolved dataset is one that requires gold data then ensure args.gold_file is provided (raise a parse-time error if missing). Make these checks in the main flow right after parsing args (use symbols args.dataset and args.gold_file) so downloads and _load_task_data only run with a validated canonical dataset name.
🤖 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.
Duplicate comments:
In `@src/bird_interact_agents/cloud/benchmark_data.py`:
- Around line 113-124: The function currently treats any existing local_marker
as a cache hit; change it to validate the marker contents by reading
local_marker.read_text().strip() and comparing it to prefix (use exact string
equality) before returning dest; if they differ, proceed to check
remote_marker.exists(), download via gcs.download_prefix(prefix, dest,
client=client) and then overwrite local_marker.write_text(prefix). Ensure you
reference local_marker, _MARKER, prefix, dest, remote_marker and
gcs.download_prefix in the fix.
In `@src/bird_interact_agents/cloud/ray_app.py`:
- Around line 1180-1225: Validate and canonicalize args.dataset in
ray_app.main() immediately after parsing (before calling download_benchmark_data
or _load_task_data): map known alias tokens to their canonical dataset names
(e.g., via the project's existing dataset-resolver helper or a small lookup),
reject unknown tokens with a clear error and SystemExit, and if the resolved
dataset is one that requires gold data then ensure args.gold_file is provided
(raise a parse-time error if missing). Make these checks in the main flow right
after parsing args (use symbols args.dataset and args.gold_file) so downloads
and _load_task_data only run with a validated canonical dataset name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 964c102f-47e2-4eee-8644-d98274457245
📒 Files selected for processing (110)
Dockerfile.cloudslayer_models/alien/models/alien/observationalconditions.yamlslayer_models/alien/models/alien/observatories.yamlslayer_models/alien/models/alien/researchprocess.yamlslayer_models/alien/models/alien/signal_aidp.yamlslayer_models/alien/models/alien/signal_ccs.yamlslayer_models/alien/models/alien/signal_ccs_approx.yamlslayer_models/alien/models/alien/signal_cip_class.yamlslayer_models/alien/models/alien/signal_eci.yamlslayer_models/alien/models/alien/signal_hzsr.yamlslayer_models/alien/models/alien/signal_ier.yamlslayer_models/alien/models/alien/signal_is_anomalous_quantum.yamlslayer_models/alien/models/alien/signal_is_cip.yamlslayer_models/alien/models/alien/signal_is_directed_transmission.yamlslayer_models/alien/models/alien/signal_is_eit.yamlslayer_models/alien/models/alien/signal_is_frt.yamlslayer_models/alien/models/alien/signal_is_galactic_significance.yamlslayer_models/alien/models/alien/signal_is_habitable_zone_transmission.yamlslayer_models/alien/models/alien/signal_is_high_confidence.yamlslayer_models/alien/models/alien/signal_is_multi_channel_protocol.yamlslayer_models/alien/models/alien/signal_is_observation_verified.yamlslayer_models/alien/models/alien/signal_is_quantum_coherent.yamlslayer_models/alien/models/alien/signal_is_technosig_high_conf.yamlslayer_models/alien/models/alien/signal_is_technosignature.yamlslayer_models/alien/models/alien/signal_spei.yamlslayer_models/alien/models/alien/signaladvancedphenomena.yamlslayer_models/alien/models/alien/signalclassification.yamlslayer_models/alien/models/alien/signaldecoding.yamlslayer_models/alien/models/alien/signaldynamics.yamlslayer_models/alien/models/alien/signalprobabilities.yamlslayer_models/alien/models/alien/signals.yamlslayer_models/alien/models/alien/sourceproperties.yamlslayer_models/alien/models/alien/telescopes.yamlslayer_models/households/models/households/amenities.yamlslayer_models/households/models/households/households.yamlslayer_models/households/models/households/infrastructure.yamlslayer_models/households/models/households/locations.yamlslayer_models/households/models/households/properties.yamlslayer_models/households/models/households/service_types.yamlslayer_models/households/models/households/transportation_assets.yamlslayer_models/polar/models/polar/cabinenvironment.yamlslayer_models/polar/models/polar/chassisandvehicle.yamlslayer_models/polar/models/polar/communication.yamlslayer_models/polar/models/polar/comprehensive_environmental_adaptability_rating.yamlslayer_models/polar/models/polar/comprehensive_operational_reliability_indicator.yamlslayer_models/polar/models/polar/critical_infrastructure_protection_level.yamlslayer_models/polar/models/polar/critical_scientific_equipment_status.yamlslayer_models/polar/models/polar/energy_water_resource_integration_index.yamlslayer_models/polar/models/polar/engineandfluids.yamlslayer_models/polar/models/polar/equipment.yamlslayer_models/polar/models/polar/extreme_climate_adaptation_coefficient.yamlslayer_models/polar/models/polar/extreme_operating_conditions.yamlslayer_models/polar/models/polar/extreme_weather_ready.yamlslayer_models/polar/models/polar/life_support_reliability_classification.yamlslayer_models/polar/models/polar/life_support_system_reliability.yamlslayer_models/polar/models/polar/lightingandsafety.yamlslayer_models/polar/models/polar/location.yamlslayer_models/polar/models/polar/long_term_scientific_mission_viability.yamlslayer_models/polar/models/polar/operationmaintenance.yamlslayer_models/polar/models/polar/polar_base_energy_security_status.yamlslayer_models/polar/models/polar/polar_transportation_efficiency_coefficient.yamlslayer_models/polar/models/polar/polar_vehicle_safe_operation_conditions.yamlslayer_models/polar/models/polar/powerbattery.yamlslayer_models/polar/models/polar/resource_self_sufficiency_index.yamlslayer_models/polar/models/polar/scientific.yamlslayer_models/polar/models/polar/scientific_mission_success_probability.yamlslayer_models/polar/models/polar/sustainable_polar_operations.yamlslayer_models/polar/models/polar/thermalsolarwindandgrid.yamlslayer_models/polar/models/polar/transmission.yamlslayer_models/polar/models/polar/waterandwaste.yamlslayer_models/polar/models/polar/weatherandstructure.yamlsrc/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.pysrc/bird_interact_agents/agents/pydantic_ai_recursive/agent.pysrc/bird_interact_agents/benchmark.pysrc/bird_interact_agents/cloud/benchmark_data.pysrc/bird_interact_agents/cloud/cli.pysrc/bird_interact_agents/cloud/cluster.yaml.j2src/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/gcs.pysrc/bird_interact_agents/cloud/image.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/cloud/upload_back.pysrc/bird_interact_agents/harness.pysrc/bird_interact_agents/paths.pysrc/bird_interact_agents/run.pysrc/bird_interact_agents/slayer_otf/reference_build.pysrc/bird_interact_agents/slayer_pipeline/orchestrator.pysrc/bird_interact_agents/slayer_pipeline/portable_connection.pytests/cloud/conftest.pytests/cloud/test_benchmark_data.pytests/cloud/test_cli.pytests/cloud/test_cluster.pytests/cloud/test_driver.pytests/cloud/test_image.pytests/cloud/test_ray_app.pytests/cloud/test_upload_back.pytests/slayer_pipeline/test_orchestrator_conn.pytests/slayer_pipeline/test_portable_connection.pytests/test_benchmark.pytests/test_cloud_paths_unchanged.pytests/test_literal_existence_validation.pytests/test_one_shot_mode.pytests/test_otf_rebuild_wiring.pytests/test_paths.pytests/test_pydantic_ai_otf_encode_agent.pytests/test_pydantic_ai_otf_encode_run_wiring.pytests/test_run_aggregation.pytests/test_run_db_integration.pytests/test_slayer_otf_reference_build.pytests/test_timing.py
De-bake hardening: - benchmark_data.ensure_downloaded: gate the local-marker fast-path on read_text()==prefix; rmtree + re-download on mismatch (dest is benchmark-scoped, not hash-scoped, so a benchmark-version bump lands a new content-hash prefix into the same dir — the stale tree must not be reused). - driver._instance_ids_sorted_by_db: tolerate a missing local data file (input-order fallback) so resubmit works on a machine without the GCS-delivered dataset; DB-grouping is a dispatch optimization, not a gate. - ray_app.main: --dataset gets choices=cli_dataset_tokens() + canonicalization after parse (mirror cloud.cli), instead of failing late. Registry/benchmark-awareness: - Replace 3 hardcoded "livesqlbench" else "mini_interact" branches (recursive agent, otf_encode agent, upload_back) with get_benchmark(...).name. - run.py run_evaluation: gate the audited-gold overlay on `not gold_required` so a gated-gold benchmark (livesqlbench) isn't overlaid with audited_gold/<db> rows (mirrors cloud _load_task_data). - run.py: gold-file error message is now benchmark-aware (f-string) instead of hardcoding "livesqlbench". Loader/test: - harness.load_benchmark_tasks: apply `limit` AFTER `filter_ids` on the non-gold path, so requested instance_ids aren't truncated away first. - test_benchmark: narrow the frozen-model assertion to pydantic ValidationError. New tests cover the marker prefix-mismatch re-download, the resubmit data-file fallback, the actor --dataset canonicalize/reject, and the loader limit-after-filter. Full non-integration suite green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- driver._build_resubmit_args: pass --dataset / --benchmark-data-prefix only when the manifest carries them. A manifest without a `dataset` key pins an older image whose ray_app argparse predates --dataset, so resubmitting it must omit the flag and let the baked image run (back-compat). - benchmark_data.ensure_downloaded: serialize check→download→mark under a per-dest fcntl lock and clear any stale (older-prefix) or partial (crashed mid-download) tree under the lock, so concurrent same-VM actors (--actors-per-worker > 1) can never read a half-written dataset dir; marker written LAST stays the only completeness signal. - one-shot guard: replace the hardcoded dataset == "livesqlbench" check with get_benchmark(...).one_shot in run.py + both agents, so a future one-shot benchmark isn't rejected per-task after passing the registry-driven CLI gate. New tests cover the resubmit flag omission and the partial-dest re-download. Full non-integration suite green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 29, 2026
4 tasks
This was referenced Jun 30, 2026
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.
Summary
Three independent fixes surfaced while running LiveSQLBench-Base-Lite-SQLite locally:
Nonedefault).paths.slayer_otf_cache_root/slayer_models_otf_rootnow require an explicitbenchmark("mini_interact"or"livesqlbench"); a forgotten benchmark can no longer silently fall back to — and mix artifacts with — mini-interact."mini_interact"maps to the legacy dirs + env vars, so on-disk layout and the dev-1470 cloud contract are unchanged. Every caller passes it explicitly (run.py, both on-the-fly agents, and cloud via newdriver._submit_benchmark(args)/ray_app._cloud_benchmark(cfg)helpers that derive from the run's dataset —"mini_interact"today,"livesqlbench"automatically once a dataset is plumbed through).--db-pathno longer breaks ingest.orchestrator._phase1_ingestbuiltsqlite:////{path}; with a relative path that resolved at the filesystem root → "unable to open database file" (the README's own commands hit this). Newportable_connection.absolute_sqlite_urlproduces a canonical, env-independent absolute URL; the orchestrator canonicalizes the persisted datasource YAML;run.pyresolves--db-pathonce at the CLI boundary.children_knowledgewith cross-references parsed from each KB'sdefinitionformula (\text{ABBR}tokens → other KB ids), with ambiguous-abbreviation and raw-column suppression — so a dependent KB is encoded after the columns it references exist. The validate-before-persist gate drops the fail-open "engine unhealthy → skip" escape that silently persisted broken cross-refs: a failure now always blocks (encoder self-corrects or defers), and an infra/connection-class failure is logged at WARNING.Test plan
env -u SSH_AUTH_SOCK uv run --extra all --extra dev --extra pydantic-ai pytest→ 1413 passed, 94 skipped.None-raises (test_paths.py), cloud explicit-benchmark derivation + artifact-root spies (test_cloud_paths_unchanged.py),absolute_sqlite_url+ orchestrator relative-root canonicalization (test_portable_connection.py,test_orchestrator_conn.py), formula-derived edges + ambiguity/raw-column suppression + ordering (test_slayer_otf_reference_build.py), strict write gate blocks + infra-warning (test_pydantic_ai_otf_encode_agent.py).alien_1,--otf-rebuild) with a relative--db-path ../livesqlbench-base-lite-sqlite/(the README form): ingest passes (0 "unable to open"), 0 "engine unhealthy — skipping gate" warnings.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests