DEV-1605: version OTF references by encoder model + stamp consumed version - #61
Conversation
…rsion Adds an encoder-model version axis to the on-the-fly SLayer reference layout (`slayer_models_otf/<benchmark>/<version>/<db>/`) and records which encoded version a consumer run used, end to end (local + cloud). - paths.slayer_models_otf_root gains an optional `version=` segment (above <db>) + a label validator; version=None is the legacy parent so existing `root / db_name` call sites are unchanged. - model_string.encoder_version_slug derives the default version label (strip provider + leading `claude-`, slashes -> __). - reference_build writes a self-describing `_encoder_meta.json` (model / framework / version / fp / built_at / settings) next to the marker; the in-process build lock is keyed by the versioned target dir. - The legacy flat fallback is ABOLISHED: the consumer resolves a concrete version (single -> use, explicit -> must exist, 2+ -> error). Threaded through the 4 claude_sdk_otf* agents, run.py (--pre-encoded-version), and the cloud (cli/driver/ray_app, concrete version resolved at submit so upload/download/merge agree; GCS prefixes stay <db>-keyed). - The consumed reference is stamped onto each per-task SubmissionAnnotation (additive optional field, NO schema_version bump) and aggregated into a per-db `consumed_references` list in the run manifest. - scripts/migrate_otf_references.py relocates legacy flat refs into the versioned layout (version derived from _setup_usage.json; `unknown` + warning when underivable; idempotent; dry-run). Tests: mechanical contracts only (no prompt-content tests). Full non-integration suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DEV-1605 Version OTF references by encoder model + stamp consumed version into run manifest
Version OTF references + record which version a run consumedProblemToday there is exactly one slot per Separately, a consumer run is not self-describing about which encoded
Net: you can't store two versions, and from the local Proposed change1. Version segment in the path (default = encoder model slug)Add a version segment:
2.
|
|
Warning Review limit reached
More reviews will be available in 9 minutes and 3 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds version-scoped OTF reference paths and metadata, migrates legacy flat references into versioned layouts, threads ChangesDEV-1605 versioned OTF reference flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
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/run.py (1)
323-357: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject
pre_encoded_versionunless the source is OTF.
pre_encoded_versionis now exposed on the public APIs and CLI, but nothing validates that it is only used withpre_encoded_source="otf". In the supplied resolver,versionis ignored forsource="custom"and never consulted for on-the-fly runs, so a misconfigured call silently drops the version pin instead of failing fast.Suggested guard
def _validate_slayer_setup( *, slayer_setup: str, framework: str, query_mode: str, mode: str, pre_encoded_source: str | None = None, + pre_encoded_version: str | None = None, ) -> None: + if pre_encoded_version is not None and pre_encoded_source != "otf": + raise ValueError( + "--pre-encoded-version requires --pre-encoded-models otf; " + f"got {pre_encoded_source!r}." + ) + validate_pre_encoded_source(pre_encoded_source)Then thread
pre_encoded_versionthrough the existing_validate_slayer_setup(...)call sites.Also applies to: 1028-1074, 1172-1255, 1993-2004, 2088-2112
🤖 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 323 - 357, `pre_encoded_version` is accepted by the public runner APIs but is not being validated against `pre_encoded_source`, so misconfigured calls can silently ignore the version pin. Add a fast-fail guard in the runner path around `_make_runner`/`_validate_slayer_setup` to reject any non-OTF use of `pre_encoded_version`, and thread that argument through the existing `_validate_slayer_setup(...)` call sites so the same rule is enforced consistently wherever the runner is built.
🧹 Nitpick comments (3)
tests/test_dev1605_migration.py (1)
57-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression cases for malformed usage JSON and dry-run collisions.
Please cover wrong-shaped
_setup_usage.jsonand pre-existing destination paths so the migration fallback and dry-run contracts don’t regress.src/bird_interact_agents/cloud/ray_app.py (1)
329-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the encode-version fallback into one shared resolver.
This branch now duplicates the same
encode_version or encoder_version_slug(agent_model)rule thatsrc/bird_interact_agents/cloud/upload_back.pyapplies before walking the post-run reference root. If those rules ever drift, the actor can download/build one versioned root and upload back from another. A shared helper would keep worker setup and upload-back locked to the same version contract.🤖 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 329 - 345, The encode-version fallback is duplicated and can drift between _otf_version_for in ray_app.py and the upload-back logic in upload_back.py. Extract the shared “encode_version or encoder_version_slug(agent_model)” resolution into one helper and have both call sites use it so worker setup and post-run upload-back resolve the same version contract.tests/cloud/test_dev1605_cloud_version.py (1)
67-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPersist the derived encode version in the manifest instead of asserting
None.Locking the default contract to
m["encode_version"] is Nonemeans worker setup, upload-back, and fetch/merge all have to re-derive the slug fromagent_modelindependently. Given the version axis is supposed to be concrete by submit time, I'd rather havebuild_manifest()store the resolved slug and assert that here.🤖 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/cloud/test_dev1605_cloud_version.py` around lines 67 - 81, The manifest version test is still asserting that encode_version stays None, but build_manifest() should persist the resolved encode slug instead. Update the test around driver.build_manifest to expect the derived encode_version value based on agent_model rather than None, and keep pre_encoded_version aligned with the existing contract. Use the build_manifest function and the manifest keys encode_version and pre_encoded_version to locate the change.
🤖 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 `@scripts/build_otf_references.py`:
- Around line 49-56: The version resolution in resolve_build_version currently
treats an explicit empty version the same as an omitted flag because it relies
on truthiness. Update the check to distinguish None from an empty string so an
explicit --version "" is preserved as user-supplied and does not fall back to
encoder_version_slug(agent_model). Keep the returned provenance flag accurate in
resolve_build_version so callers can tell when the version was explicitly
provided, even if empty.
In `@scripts/migrate_otf_references.py`:
- Around line 66-76: The `_setup_usage.json` parsing in the migration helper can
crash on valid JSON with the wrong shape because `_get_encoder_version` assumes
`json.loads()` returns a dict and that each `breakdown` entry is a dict. Update
the parsing logic to defensively validate `data` and each `row` before calling
`.get()` in `_get_encoder_version` (and the same related parsing path referenced
by the follow-up range), and fall back to `_UNKNOWN, False` whenever the
structure is not the expected dict/list-of-dicts shape.
- Around line 143-155: The dry-run path in migrate_otf_references.py is
reporting moves without checking whether the destination already exists, so it
can disagree with a real run. Update the logic around the destination handling
in the move/report flow so the same collision check used after computing dest is
applied before adding or keeping a move in the report; if dest exists, have
dry_run skip or remove that move the same way the non-dry-run path does. Use the
existing migrate_otf_references flow and its report.moves handling to keep
dry-run and real-run behavior aligned.
In `@src/bird_interact_agents/agents/_pre_encoded.py`:
- Around line 188-197: The `_pre_encoded.py` metadata handling in the
`meta_fp.is_file()` block only treats read/JSON failures as best-effort, but it
still assumes the parsed value is a mapping with a valid string `encoder_model`.
Update the `ConsumedReference` path so malformed `_encoder_meta.json` values are
also tolerated: verify the decoded JSON is an object/dict, safely extract
`encoder_model`, and fall back to `"unknown"` when it is missing, null, or not a
string before constructing `ConsumedReference`.
In `@src/bird_interact_agents/cloud/cli.py`:
- Around line 371-387: The `--pre-encoded-version` handling in `cloud/cli.py` is
overwriting a user-supplied version with `None` when `_dbs_for_instances(...)`
returns no DBs. Update the version-selection logic in this CLI path so
`ns.pre_encoded_version` is only replaced when `_resolved` is non-empty, and
otherwise preserve the explicit value already present on
`ns.pre_encoded_version`; use the existing `_dbs_for_instances`,
`resolve_otf_version`, and `ns.pre_encoded_version` flow as the place to fix it.
- Around line 360-364: `encoder_version_slug()` in `submit` can raise
`ValueError` before the existing error handling, so invalid derived encode
versions currently escape as a traceback. Move this derivation into the same
`try/except` path used by the CLI parsing logic in `cli.py`, and convert the
failure into an `argparse` error for `ns.encode_version` / `ns.agent_model` so
malformed model strings exit cleanly with a normal CLI message.
In `@src/bird_interact_agents/model_string.py`:
- Around line 64-69: The encoder_version_slug helper currently only rejects an
empty slug, so it can still return dot-segment values like "." and ".." for
inputs such as anthropic/. and anthropic/.., which breaks the “safe single path
segment” contract. Update encoder_version_slug in model_string.py to explicitly
reject "." and ".." (along with the empty string) before returning the slug, and
keep the ValueError message aligned with the existing explicit-version guidance
so callers fail early and consistently.
---
Outside diff comments:
In `@src/bird_interact_agents/run.py`:
- Around line 323-357: `pre_encoded_version` is accepted by the public runner
APIs but is not being validated against `pre_encoded_source`, so misconfigured
calls can silently ignore the version pin. Add a fast-fail guard in the runner
path around `_make_runner`/`_validate_slayer_setup` to reject any non-OTF use of
`pre_encoded_version`, and thread that argument through the existing
`_validate_slayer_setup(...)` call sites so the same rule is enforced
consistently wherever the runner is built.
---
Nitpick comments:
In `@src/bird_interact_agents/cloud/ray_app.py`:
- Around line 329-345: The encode-version fallback is duplicated and can drift
between _otf_version_for in ray_app.py and the upload-back logic in
upload_back.py. Extract the shared “encode_version or
encoder_version_slug(agent_model)” resolution into one helper and have both call
sites use it so worker setup and post-run upload-back resolve the same version
contract.
In `@tests/cloud/test_dev1605_cloud_version.py`:
- Around line 67-81: The manifest version test is still asserting that
encode_version stays None, but build_manifest() should persist the resolved
encode slug instead. Update the test around driver.build_manifest to expect the
derived encode_version value based on agent_model rather than None, and keep
pre_encoded_version aligned with the existing contract. Use the build_manifest
function and the manifest keys encode_version and pre_encoded_version to locate
the change.
🪄 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: 0f0186d0-21ac-48df-a402-0f71e05abc5a
📒 Files selected for processing (32)
scripts/build_otf_references.pyscripts/migrate_otf_references.pysrc/bird_interact_agents/agents/_pre_encoded.pysrc/bird_interact_agents/agents/claude_sdk_otf/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract_v1/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_v1/agent.pysrc/bird_interact_agents/cloud/cli.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/cloud/upload_back.pysrc/bird_interact_agents/eval/annotation_schema.pysrc/bird_interact_agents/eval/grade_in_place.pysrc/bird_interact_agents/harness.pysrc/bird_interact_agents/model_string.pysrc/bird_interact_agents/paths.pysrc/bird_interact_agents/run.pysrc/bird_interact_agents/slayer_otf/encoder_types.pysrc/bird_interact_agents/slayer_otf/reference_build.pytests/cloud/test_dev1605_cloud_version.pytests/cloud/test_driver.pytests/cloud/test_ray_app.pytests/cloud/test_upload_back.pytests/test_cloud_paths_unchanged.pytests/test_dev1586_pre_encoded.pytests/test_dev1605_annotation_stamp.pytests/test_dev1605_build_script.pytests/test_dev1605_consumer_version.pytests/test_dev1605_encoder_meta.pytests/test_dev1605_migration.pytests/test_dev1605_paths_version.pytests/test_dev1605_slug.py
- Codex (major): the in-cloud `pydantic_ai_otf_encode` encoder now builds into the versioned root `slayer_models_otf/<benchmark>/<version>/<db>` and stamps `_encoder_meta.json`, so its cloud-built references are found by the versioned upload-back / merge instead of being orphaned. `encode_version` threads run.py → ray_app → agent. - Centralise the `encode_version or slug(agent_model)` rule in one shared `model_string.resolve_encode_version` used by the agent, ray_app, upload_back, and driver (no drift between build/download/upload/merge). - Validation hardening: `encoder_version_slug` rejects `.`/`..`/`\` slugs; `resolve_build_version` treats explicit `--version ""` as operator-supplied (not omitted); cloud submit routes a bad encode model through `p.error`; preserve an explicit `--pre-encoded-version` when no DBs resolve; reject `--pre-encoded-version` unless `--pre-encoded-models otf` (via `_validate_slayer_setup`). - Defensive JSON parsing: malformed `_setup_usage.json` / `_encoder_meta.json` fall back to `unknown` instead of crashing; migration dry-run honours destination collisions. - Regression tests for all of the above. Full non-integration suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
- Local pre-encoded otf runs now enforce the SAME "one run consumes one encoder version" cross-DB guard the cloud submit applies: run_evaluation resolves the version across all selected DBs once and errors on a mix, so local and cloud behave identically. - resolve_encode_version distinguishes an explicit value by `is not None` (not truthiness), so a deliberate `--version ""` is preserved as operator-supplied and fails loudly at the label validator — matching resolve_build_version. Cloud submit validates the resolved label inside p.error so a bad/empty --version exits cleanly, not with a traceback. - Regression tests for both. Full non-integration suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
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/ray_app.py (1)
336-339: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail fast when an OTF consumer has no resolved version.
For
slayer_setup="pre-encoded"+pre_encoded_source="otf", returningNonehere can send artifact resolution into the legacyversion=Nonelayout instead of enforcing the new version-pinned flow. Please reject missing/emptypre_encoded_versionbefore returning.Suggested guard
if cfg.get("slayer_setup") == "pre-encoded" and ( cfg.get("pre_encoded_source") == "otf" ): - return cfg.get("pre_encoded_version") + version = cfg.get("pre_encoded_version") + if not version: + raise ValueError( + "pre_encoded_version is required for pre-encoded OTF runs" + ) + return str(version)🤖 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 336 - 339, The version resolution in ray_app.py should fail fast for the OTF pre-encoded path instead of returning a missing value. In the pre-encoded branch of the version lookup logic, add a guard for pre_encoded_version being absent or empty when slayer_setup is "pre-encoded" and pre_encoded_source is "otf", and raise an error before any return. Update the helper that handles cfg lookups in the same flow so only a valid resolved version is returned for the version-pinned artifact path.
🤖 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.
Outside diff comments:
In `@src/bird_interact_agents/cloud/ray_app.py`:
- Around line 336-339: The version resolution in ray_app.py should fail fast for
the OTF pre-encoded path instead of returning a missing value. In the
pre-encoded branch of the version lookup logic, add a guard for
pre_encoded_version being absent or empty when slayer_setup is "pre-encoded" and
pre_encoded_source is "otf", and raise an error before any return. Update the
helper that handles cfg lookups in the same flow so only a valid resolved
version is returned for the version-pinned artifact path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e2e2a291-c4df-4691-8add-0c641c36d0fd
📒 Files selected for processing (15)
scripts/build_otf_references.pyscripts/migrate_otf_references.pysrc/bird_interact_agents/agents/_pre_encoded.pysrc/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.pysrc/bird_interact_agents/cloud/cli.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/cloud/upload_back.pysrc/bird_interact_agents/model_string.pysrc/bird_interact_agents/run.pytests/test_dev1605_build_script.pytests/test_dev1605_migration.pytests/test_dev1605_review_fixes.pytests/test_one_shot_run.pytests/test_pydantic_ai_otf_encode_agent.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/test_dev1605_build_script.py
- src/bird_interact_agents/cloud/upload_back.py
- scripts/build_otf_references.py
- src/bird_interact_agents/cloud/cli.py
- scripts/migrate_otf_references.py
- src/bird_interact_agents/cloud/driver.py
- src/bird_interact_agents/agents/_pre_encoded.py
- migrate_otf_references: `_derive_version` / `_backfill_encoder_meta` now
require a non-empty STRING `model` in the `setup_encoder` breakdown row, so
a malformed-but-valid `{"model": 123}` falls back to 'unknown' instead of
crashing `encoder_version_slug`'s `.partition()` and aborting the migration.
- Regression test for the non-string-model case. Full suite green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Local `run.py` now exposes the encode-side `--version` flag (mirror of the build-script / cloud-submit `--version`), threaded args.encode_version → run_evaluation → _make_runner → PydanticAIOtfEncodeAgent. Local on-the-fly encode runs can now build into separate version slots instead of only the default agent-model slug — closing the last local/cloud versioning-surface gap. - Regression test that _make_runner threads encode_version to the agent. Full suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Local CLI now fail-fast validates the encode `--version` label (resolve + paths._validate_otf_version) inside the argparse validation try/except, so a bad label like `--version ""` / `--version "a/b"` exits with a clean CLI error before any task runs — mirroring cloud submit, instead of surfacing as per-task failed result rows inside the adapter's broad except. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sequel to DEV-1589 (#57). Based on the DEV-1589 branch so the diff is DEV-1605-only; retarget to
mainonce #57 merges.Problem
One slot per
(benchmark, db)— re-running the encoder with a different LLM overwrites the prior reference, and a consumer run isn't self-describing about which encoded version it used.What this does
1. Versioned layout
slayer_models_otf/<benchmark>/<version>/<db>/paths.slayer_models_otf_root(benchmark, version=None)— optional version segment above<db>+ a label validator.version=Nonereturns the legacy parent, so existingroot / db_namecall sites are unchanged.model_string.encoder_version_slug— default label from the encoder model (anthropic/claude-opus-4-7→opus-4-7,zai/glm-5.2→glm-5.2, slashes→__).scripts/build_otf_references.pygains--version(default = slug); per-version--forceslot.2.
_encoder_meta.jsonprovenanceRich
EncoderMeta(model / framework / version / benchmark / db / fp / built_at / settings) written next to_reference_fp.txt, before the marker. Optional — legacy callers unaffected.3. Consumer version resolution (legacy flat fallback abolished)
resolve_otf_version: single→use, explicit→must exist, 2+→error, 0→error. Threaded through all 4claude_sdk_otf*consumers,run.py(--pre-encoded-version), and the cloud.4. Consumed-version stamp
ConsumedReferenceon each per-taskSubmissionAnnotation(additive optional field — noschema_versionbump, old annotations still validate) + a per-dbconsumed_referenceslist in the run manifest (local + cloud).5. Full cloud versioning
Submit/resubmit forward
--pre-encoded-version/--version; the concrete version is resolved at submit so upload / download / merge agree; actor download, upload-back, and post-run merge are version-aware. GCS prefixes stay<db>-keyed (runs/<run_id>/already isolates versions across runs).6. Migration
scripts/migrate_otf_references.py(idempotent, dry-run) relocates legacy flat refs; version derived from_setup_usage.json;unknown+ warning when underivable; backfills_encoder_meta.json.Deviations from the issue (signed off)
<version>/<db>(version above db) — cleaner plumbing than the issue's literal<db>/<version>.Tests
Mechanical contracts only (no prompt-content tests). 8 new DEV-1605 test files + a cloud test. Full non-integration suite green (3516 passed). Codex reviewed both the plan and the tests-vs-plan; all findings folded in (incl. keeping
schema_version=1).🤖 Generated with Claude Code
Summary by CodeRabbit
--pre-encoded-versionand--versionoptions for consistent OTF version labeling across local and cloud runs.