Repository navigation
fix(eval): isolate hook tags per evaluation - #938
Conversation
Copying tags with list() turned a string into per-character tags and a dict into its keys, so invalid tags that used to fail validation were logged without an error. Only copy list, tuple, and set values and pass anything else through so validate_tags still rejects it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The constructor stored the caller's tags list as-is, so any caller that built DictEvalHooks directly still had hooks.tags.append() modify its own list. The tags setter already copies. Copy list, tuple, and set values in the constructor too, and drop the per-call-site copy in run_evaluator. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Eval metadata was only copied one level deep, so a task that changed a nested dict or list in hooks.metadata affected later trials, other rows sharing the same metadata, and the caller's dataset. Recursively copy plain dicts and lists per evaluation. Other objects stay shared so the copy stays cheap and can't fail on values that don't support copying. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
hooks.expected and the scorers shared the EvalCase's expected object, so a task that changed it affected later trials, other rows sharing the same value, and the caller's dataset. Give each evaluation its own copy of plain dicts and lists in expected, and use that copy for the hooks, scorers, logged span, and EvalResult. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The existing tag isolation tests run with max_concurrency=1. Add a test that keeps every trial of one row in flight at once, so sharing a tags list between concurrently running trials is caught. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copying tags into a list lets hooks.tags.append() work when an EvalCase gives tags as a tuple or set, which used to raise AttributeError. Add a test so this behavior can't quietly regress. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fold the separate tags, tuple/set tags, nested metadata, and expected isolation tests into one parametrized test that changes all three in the same task. Drop the hooks.tags.clear() test (it checks the same sharing as append), the direct DictEvalHooks constructor test (the eval tests go through the constructor now that the call-site copy is gone), and duplicate parameter cases. 33 test cases become 12, and undoing any one of the fixes still fails a test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- _copy_containers only recurses into dicts and lists, so plain values skip a function call (about 32% faster on small and large inputs). - Reword the DictEvalHooks tags comment: span validation runs on the original tags before the hooks are built, so the pass-through exists to avoid splitting a string or dict into tags. - Drop the concurrent-trials test. Each trial already gets its own copy in the constructor, so the sequential trials case catches the same regressions without the event and timeout machinery. - Use copy.copy for the test's tags snapshot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
Copying metadata and expected for every trial cost about 6ms per row and nearly doubled peak memory for evals with large reference data, since each EvalResult kept its own copy. Mutating nested metadata or expected from a task isn't a documented pattern, while hooks.tags.append() is. Keep the tags fix and go back to main's handling of metadata (shallow copy) and expected (shared). This also removes _copy_containers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Abhijeet Prasad (AbhiPrasad)
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2a42a501a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| elif isinstance(tags, (list, tuple, set)): | ||
| # Copy so hook mutations don't leak into the caller's tags. | ||
| self.update({"tags": list(tags)}) |
There was a problem hiding this comment.
When a task with list-backed initial tags appends to hooks.tags and then raises, this copy prevents that mutation from reaching the local tags variable. The only reassignment from hooks.tags occurs after the task completes successfully, so the exception path returns an EvalResult and logs a root span containing only the original tags; before this change, the shared list preserved the appended tags. Capture the copied hook tags on the failure path as well so isolation does not discard tags added while diagnosing a failed evaluation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in f37b20a. The error path now reads hooks.tags back and logs it with the error, and test_hooks_tags_are_kept_when_task_raises covers it. One correction: on main the appended tag only survived in EvalResult.tags because the list was shared. The logged root span never had it. With this fix it's in both.
Tags are now copied into the hooks, but only the success path read hooks.tags back. A task that appended a tag and then raised lost it from EvalResult.tags, which main only kept because the list was shared. Read hooks.tags on the error path too and log them with the error, so the root span also gets them (main never logged them there). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Supersedes #936 by Gareth.Y (@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.DictEvalHooksnow copies list, tuple and set tags in its constructor, the same way the existingtagssetter does, so every evaluation gets its own list. Other values are passed through unchanged. Invalid tags such astags="prod"therefore still raisetags must be a list, set, or tuple of stringsinstead of becoming per-character tags; that was a regression in the original #936 change. Tuple and set tags can now be appended to throughhooks.tags, which used to raiseAttributeError.Not changed
metadatakeeps main's one-level copy, andexpectedis still shared. An earlier version of this PR copied both all the way down for every trial. With a distinct 1,000-documentexpectedper 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 everyEvalResultkept its own copy. Mutating nested metadata orexpectedfrom a task isn't a documented pattern, so it was dropped.inputis still shared between trials.workflow_evalalready reloads each case from its store, so every trial gets fresh objects there.Testing
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 fix(eval): isolate mutable tags for each evaluation #936 regression.expectedbenchmark, performance matches main: about 0.4 ms per row and +4 MB peak memory.nox -s test_corepassed on the final head: 1033 passed, 106 skipped, 12 xfailed.make lintandpylintare clean.🤖 Generated with Claude Code
Co-authored by StarfolkAI (@starfolkai)[bot]