Skip to content

Commit 3f0e4f6

Browse files
committed
refactor: delete the LLM-key cost exclusion down to the column
The key list is gone from the code paths; take out what only existed to feed it. `trials.llm_key_hash` had exactly one reader — the exclusion probe — so with the probe gone the column, the fingerprinting module, and both stamping sites are dead weight. Removes `core/llm_key_fingerprint.py` and its test, the `llm_key_hash` column and ORM field, the settlement-time stamp in `trial_handler`, the local-runner stamp, and the `carl_tools` field allowlist entry. BYOK's own `key_hint` is a different feature and is untouched. `costexcl02` now drops the column alongside the table, under a short `lock_timeout` — dropping a column is catalog-only in Postgres but still takes a brief ACCESS EXCLUSIVE lock on a hot table, and failing fast beats queueing behind a long read with every writer stacked up behind us. One subtlety at the claim site: the conditional UPDATE there was never really about the hash. Its rowcount is the "this attempt is still ours" check that aborts a stale or reassigned worker before it does any expensive work. Kept as an UPDATE with the same predicate so the check stays atomic against a concurrent finish, now writing `updated_at`, since an UPDATE needs a SET. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zmdrcCqnkVM6hWjhPu2T9
1 parent c1cfda0 commit 3f0e4f6

9 files changed

Lines changed: 26 additions & 383 deletions

File tree

backend/carl_tools.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@
4444
"trial_s3_key",
4545
"harbor_result_path",
4646
"orig_s3_src",
47-
"llm_key_hash",
4847
"trajectory_summary",
4948
"trajectory_graph",
5049
"analysis",

oddish/alembic/versions/costexcl02_model_and_experiment_exclusions.py

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,16 @@
1010
capacity, free preview tiers) and an **experiment** that was comped.
1111
1212
Both correlate on columns ``trials`` already indexes -- ``trials.model`` and
13-
``trials.experiment_id`` -- so neither needs a new index on that hot table,
14-
and neither migration step touches it. ``deleted_at`` is the soft-delete
15-
tombstone; the partial UNIQUEs keep one live row per model / per experiment so
16-
a removed entry can be re-added.
13+
``trials.experiment_id`` -- so neither needs a new index on that hot table.
14+
``deleted_at`` is the soft-delete tombstone; the partial UNIQUEs keep one live
15+
row per model / per experiment so a removed entry can be re-added.
1716
18-
``trials.llm_key_hash`` is deliberately left in place: dropping a column off
19-
``trials`` is a separate, riskier change, and the column is inert once nothing
20-
reads it.
17+
``trials.llm_key_hash`` goes too. It existed only to correlate the key list,
18+
nothing else ever read it, and the exclusion lists that replace it correlate
19+
on columns ``trials`` already has. Dropping a column is a catalog-only change
20+
in Postgres, but it still needs a brief ACCESS EXCLUSIVE lock on a hot table,
21+
so it runs under a short ``lock_timeout``: failing fast and retrying beats
22+
queueing behind a long read and stalling every writer behind us.
2123
"""
2224

2325
from typing import Sequence, Union
@@ -70,14 +72,18 @@ def upgrade() -> None:
7072
ON cost_excluded_experiments (experiment_id) WHERE deleted_at IS NULL
7173
"""
7274
)
73-
# The key-hash list is gone from the code paths above; drop it last so a
74-
# failed create leaves the old feature's data intact for a re-run.
75+
# The key-hash feature is gone from the code paths above; drop its table
76+
# and the trials column last, so a failed create leaves nothing half-done.
7577
op.execute("DROP TABLE IF EXISTS cost_excluded_llm_keys")
78+
op.execute("SET LOCAL lock_timeout = '5s'")
79+
op.execute("ALTER TABLE trials DROP COLUMN IF EXISTS llm_key_hash")
7680

7781

7882
def downgrade() -> None:
79-
# Recreates the key list's shape only. Its rows are not recoverable --
80-
# nothing else stored the hashes.
83+
# Recreates the key feature's shape only. Neither the exclusion rows nor
84+
# the per-trial hashes are recoverable -- nothing else stored either.
85+
op.execute("SET LOCAL lock_timeout = '5s'")
86+
op.execute("ALTER TABLE trials ADD COLUMN IF NOT EXISTS llm_key_hash VARCHAR(64)")
8187
op.execute(
8288
"""
8389
CREATE TABLE IF NOT EXISTS cost_excluded_llm_keys (

oddish/src/oddish/core/llm_key_fingerprint.py

Lines changed: 0 additions & 145 deletions
This file was deleted.

oddish/src/oddish/db/models.py

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1144,10 +1144,6 @@ class TrialModel(TimestampedMixin, Base):
11441144
tool_counts: Mapped[dict | None] = mapped_column(JSONB, nullable=True)
11451145
cost_usd: Mapped[float | None] = mapped_column(Float, nullable=True)
11461146

1147-
# SHA-256 of the platform provider API key this trial ran on, stamped at
1148-
# settlement (forward-only; NULL for pre-rollout / unresolved keys).
1149-
llm_key_hash: Mapped[str | None] = mapped_column(String(64), nullable=True)
1150-
11511147
# Per-phase timing breakdown (from Harbor's TrialResult TimingInfo)
11521148
phase_timing: Mapped[dict | None] = mapped_column(JSONB, nullable=True)
11531149

oddish/src/oddish/worker/local_runner.py

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@
4545
from oddish.core.harbor_artifacts import cache_write_tokens_from_trajectory
4646
from oddish.core.task_browse_summary import refresh_task_browse_summaries
4747
from oddish.core.cost_basis import CANCELLED_HARBOR_STAGE
48-
from oddish.core.llm_key_fingerprint import platform_key_hash_for_provider
4948
from oddish.db.models import WorkerJobKind, WorkerJobModel, WorkerJobStatus
5049
from oddish.db.storage import resolve_task_directory
5150
from oddish.model_pricing import is_native_cost_trusted, settle_cost_usd
@@ -867,7 +866,6 @@ async def _upload_task_dir(event: TrialHookEvent) -> None:
867866
# Local-mode trials land in the same cost accounting as queue
868867
# trials, so stamp the platform key hash here too (see
869868
# workers/queue/trial_handler settlement).
870-
trial.llm_key_hash = platform_key_hash_for_provider(provider)
871869
log_unpriced_trial_if_needed(
872870
cost_usd=trial.cost_usd,
873871
trial_id=trial.id,

oddish/src/oddish/workers/queue/trial_handler.py

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@
3939
get_session,
4040
utcnow,
4141
)
42-
from oddish.core.llm_key_fingerprint import trial_llm_key_hash
4342
from oddish.core.task_browse_summary import refresh_task_browse_summaries
4443
from oddish.db.storage import get_storage_client, resolve_task_directory
4544
from oddish.model_pricing import is_native_cost_trusted, settle_cost_usd
@@ -516,9 +515,6 @@ async def _prepare_trial_run(
516515
trial.total_tool_calls = None
517516
trial.tool_counts = None
518517
trial.cost_usd = None
519-
# llm_key_hash deliberately survives this reset: it is the last
520-
# attempt's funding key and the best prediction for the retry.
521-
# Settlement overwrites it with the actual key.
522518
trial.phase_timing = None
523519
trial.has_trajectory = False
524520
trial.attempts += 1
@@ -706,8 +702,6 @@ def _settle_trial_metering(
706702
if preserve_checkpointed_cost and prev_cost_usd is not None:
707703
if trial.cost_usd is None or trial.cost_usd < prev_cost_usd:
708704
trial.cost_usd = prev_cost_usd
709-
# Attribute spend to the BYOK overlay or platform key that funded the run.
710-
trial.llm_key_hash = trial_llm_key_hash(provider, byok_env)
711705
return prev_cost_usd, provider, native_cost_trusted
712706

713707

@@ -1526,22 +1520,20 @@ async def run_trial_job(
15261520
agent=prepared_trial.trial_agent,
15271521
)
15281522
byok_env = byok_resolution.env if byok_resolution else None
1529-
funding_key_hash = trial_llm_key_hash(
1530-
settings.get_provider_for_trial(
1531-
prepared_trial.trial_agent, prepared_trial.trial_model
1532-
),
1533-
byok_env,
1534-
)
1535-
stamp = update(TrialModel).where(
1523+
# Claim guard: re-assert that this attempt is still ours before doing any
1524+
# expensive work. A conditional UPDATE (not a SELECT) so the check is
1525+
# atomic against a concurrent finish or reassignment; ``updated_at`` is
1526+
# simply the column it writes, since an UPDATE needs a SET.
1527+
claim = update(TrialModel).where(
15361528
TrialModel.id == trial_id,
15371529
TrialModel.finished_at.is_(None),
15381530
TrialModel.attempts == prepared_trial.trial_attempt,
15391531
)
15401532
if worker_id is not None:
1541-
stamp = stamp.where(TrialModel.current_worker_id == worker_id)
1533+
claim = claim.where(TrialModel.current_worker_id == worker_id)
15421534
async with get_session() as session:
1543-
stamped = await session.execute(stamp.values(llm_key_hash=funding_key_hash))
1544-
if not getattr(stamped, "rowcount", 0):
1535+
claimed = await session.execute(claim.values(updated_at=utcnow()))
1536+
if not getattr(claimed, "rowcount", 0):
15451537
return
15461538

15471539
# Determine task path: download from S3 if needed, or use local path

oddish/tests/test_harbor_runner.py

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1627,7 +1627,6 @@ def test_store_trial_results_settles_metering_after_quota_cancel(monkeypatch):
16271627
cache_write_tokens=None,
16281628
output_tokens=None,
16291629
cost_usd=0.25,
1630-
llm_key_hash=None,
16311630
phase_timing=None,
16321631
has_trajectory=False,
16331632
current_worker_id=None,
@@ -1645,11 +1644,6 @@ async def _fake_trial_session(
16451644
yield SimpleNamespace(), trial
16461645

16471646
monkeypatch.setattr(trial_handler, "_trial_session", _fake_trial_session)
1648-
monkeypatch.setattr(
1649-
trial_handler,
1650-
"trial_llm_key_hash",
1651-
lambda *_args: "settled-key-hash",
1652-
)
16531647

16541648
outcome = harbor_runner.HarborOutcome(
16551649
reward=1.0,
@@ -1691,7 +1685,6 @@ async def _fake_trial_session(
16911685
assert trial.cache_write_tokens == 10
16921686
assert trial.output_tokens == 50
16931687
assert trial.cost_usd == 0.25
1694-
assert trial.llm_key_hash == "settled-key-hash"
16951688
assert stored == (True, False)
16961689

16971690

@@ -1703,7 +1696,6 @@ def test_store_trial_results_ignores_stale_cancelled_attempt(monkeypatch):
17031696
superseded_by_trial_id=None,
17041697
input_tokens=7,
17051698
cost_usd=0.25,
1706-
llm_key_hash="current-key",
17071699
)
17081700

17091701
@asynccontextmanager
@@ -1733,11 +1725,7 @@ async def _fake_trial_session(*_args, **_kwargs):
17331725
)
17341726

17351727
assert stored == (True, False)
1736-
assert (trial.input_tokens, trial.cost_usd, trial.llm_key_hash) == (
1737-
7,
1738-
0.25,
1739-
"current-key",
1740-
)
1728+
assert (trial.input_tokens, trial.cost_usd) == (7, 0.25)
17411729

17421730

17431731
@pytest.mark.asyncio

0 commit comments

Comments
 (0)