Skip to content

Fix remote eval reruns by preserving upsert IDs - #763

Open
paultancre-bt wants to merge 2 commits into
mainfrom
fix/remote-eval-upsert-id
Open

Fix remote eval reruns by preserving upsert IDs#763
paultancre-bt wants to merge 2 commits into
mainfrom
fix/remote-eval-upsert-id

Conversation

@paultancre-bt

@paultancre-bt paultancre-bt commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Why

Fixes COR-84.

The playground sends a stable upsert_id for each row/eval column so reruns replace the existing result. Python's EvalCase.from_dict() drops this field, and the eval runner creates a fresh root record on every run. The resulting records appear as duplicate grid rows with stale outputs. The JS SDK already honors upsert_id.

This addresses the SDK cause identified during review of the earlier UI workaround, braintrust#20332.

Repro

  1. Configure a dataset-backed playground with two Python remote evals.
  2. Run a row, then rerun it in either eval column.
  3. Although the request supplies the same upsert_id, Python writes a different root record ID, leaving both results in the grid.

The regression test reproduces this by running an evaluator twice with the same upsert_id and checking the logged root record IDs. It fails before the fix.

Fix

Preserve optional upsert_id in EvalCase and its input TypedDicts. The first trial uses that value as its root record ID; additional trials derive deterministic UUIDs from the upsert ID and trial index. This keeps the single-trial playground behavior and lets each additional trial replace its own result on rerun without overwriting other trials. Both experiment-backed and parent-context evaluations use this logic. Missing or empty IDs continue to generate fresh record IDs; dataset row IDs and origins retain their existing meaning.

This prevents future duplicates after SDK upgrade; it does not remove historical duplicate records. This PR changes Python only; the analogous JS multi-trial behavior needs a separate fix.

Test

  • All 48 rerun regression cases pass, covering dictionary/dataclass inputs, experiment/parent-context execution, supplied/missing/empty upsert IDs, and global/per-row trial counts. The tests verify distinct stable IDs per trial, updated output, preserved origin, and matching root/child outputs. The multi-trial cases fail with the bare upsert ID reused across trials.
  • Full core suite: 850 passed, 63 skipped, 12 expected failures.
  • All 35 type tests pass, including the optional field on dataclass and TypedDict inputs; pyright and mypy pass.
  • Ruff lint, formatting, and commit hooks pass.
  • No manual browser run with the patched SDK yet.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8715337d74

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".

Comment thread py/src/braintrust/framework.py Outdated
Comment on lines +1652 to +1653
if datum.upsert_id:
base_event["id"] = datum.upsert_id

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve distinct IDs for repeated trials

When an eval case supplies upsert_id and its effective trial_count is greater than one, every concurrently scheduled trial receives this same root ID. Because each new SpanImpl initially emits a replacement rather than a merge, the trials race to overwrite one logical row instead of producing the distinct results promised by trial_count, potentially leaving a root and child spans from different trials. Derive an ID that is stable per trial (for example from upsert_id and trial_index) rather than applying the bare ID to every trial.

Useful? React with 👍 / 👎.

@AbhiPrasad Abhijeet Prasad (AbhiPrasad) Sep 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have the same issue in the js sdk?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from what I found no, the JS is not impacted, Python drops upsert_id; JS already preserves it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(yes for the P1),JS SDK also assigns the same upsert_id to every trial, so with trialCount > 1, trials can overwrite each other’s root record. Let me make another PR to correct this is the JS SDK as well (fix here)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants