Repository navigation
fix(eval): isolate mutable tags for each evaluation - #936
Closed
Gareth.Y (linjiaye929) wants to merge 1 commit into
Closed
Gareth.Y (linjiaye929) wants to merge 1 commit into
Gareth.Y (linjiaye929) wants to merge 1 commit into
Conversation
Gareth.Y (linjiaye929)
marked this pull request as ready for review
October 14, 2026 17:52
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Member
|
Thanks for the PR Gareth.Y (@linjiaye929)! Had to make some changes, so this PR is superceded by #938. |
Abhijeet Prasad (AbhiPrasad)
added a commit
that referenced
this pull request
Oct 14, 2026
Supersedes #936 by @linjiaye929. Their original commit is kept as the first commit on this branch. ## Summary Eval tasks can append to or clear `hooks.tags`, which is the documented way to change an evaluation's tags. The tags list was shared by reference across trials, across rows that share a list, and with the caller's dataset. One trial's tags therefore leaked into later trials, other rows, the logged results and the caller's own list. `DictEvalHooks` now copies list, tuple and set tags in its constructor, the same way the existing `tags` setter does, so every evaluation gets its own list. Other values are passed through unchanged. Invalid tags such as `tags="prod"` therefore still raise `tags must be a list, set, or tuple of strings` instead of becoming per-character tags; that was a regression in the original #936 change. Tuple and set tags can now be appended to through `hooks.tags`, which used to raise `AttributeError`. ## Not changed - `metadata` keeps main's one-level copy, and `expected` is still shared. An earlier version of this PR copied both all the way down for every trial. With a distinct 1,000-document `expected` per row, that took a 200-row eval from 0.4 to 6.5 ms per row and added about 98 MB of peak memory, because every `EvalResult` kept its own copy. Mutating nested metadata or `expected` from a task isn't a documented pattern, so it was dropped. - `input` is still shared between trials. - `workflow_eval` already reloads each case from its store, so every trial gets fresh objects there. ## Testing - With main's `framework.py`, 6 of the 10 new test cases fail: the list, tuple and set cases. All 10 pass with this change. The test that rejects string and dict tags passes on main too; it guards against the #936 regression. - On the same 200-row large-`expected` benchmark, performance matches main: about 0.4 ms per row and +4 MB peak memory. - `nox -s test_core` passed on the final head: 1033 passed, 106 skipped, 12 xfailed. - `make lint` and `pylint` are clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- sfk:bot-coauthor --> Co-authored by @starfolkai[bot] --------- Co-authored-by: Gareth.Y <91742523+linjiaye929@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Member
|
released with https://github.com/braintrustdata/braintrust-sdk-python/releases/tag/py-sdk-v0.46.1 - thanks again Gareth.Y (@linjiaye929) |
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
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
Copy the initial tags list for each evaluation task, so the documented
hooks.tags.append()/clear()operations remain local to that trial or row. The task and its scorers still see the same per-evaluation mutations.Previously, trials shared
datum.tags. With two trials and a scorer checking each trial's tag, the second trial inherited the first trial's changes: scores were[1, 0]instead of[1, 1]. Both saved results and the caller's input list were also modified. Separate rows that explicitly share an initial list have the same problem.The production change is one shallow list copy at the per-task boundary.
None, empty-list normalization and tag order are preserved; existing tag validation is unchanged. This does not change the global hook constructor or setter.Validation
On main
d46746c33613f655f6e177ca2e7967464e782fb3, using Python 3.12.14:test_framework.py: 136 passed, 2 skipped; independently repeated. Skips cover optional Pydantic support (not installed) and Python 3.14+ behavior.git diff --checkpassed.no_send_logs=True, disabled cache and a network/DNS guard (zero network attempts). No live service, model or provider validation.Full core/provider/nox suites and static mypy/Pyright checks were not run locally. AI assistance was used to implement and test this change.
Hosted CI update — 2026-10-14
On the unchanged head
4d1d13b6d528433bf4523e85acb8b23a4108177c, checks completed successfully on attempt 2: all 97 jobs succeeded, including lint, static checks, smoke tests and the nox matrix on Ubuntu/Windows. The separate integration-tests workflow was skipped on attempt 2; its soleopen-prjob was skipped, so it is not an integration-test pass. Earlier attempt-1action_requiredobservations are superseded by these actual run results. No merge or human approval is implied.