Fix sequential few-shot sampling mutating the shared few-shot pool#1308
Open
ErenAta16 wants to merge 1 commit into
Open
Fix sequential few-shot sampling mutating the shared few-shot pool#1308ErenAta16 wants to merge 1 commit into
ErenAta16 wants to merge 1 commit into
Conversation
`_init_fewshot_sampling_sequential` rotated the list returned by `fewshot_docs()` in place. That list is the task's memoized `_fewshot_docs`, returned by reference, so the rotation mutated shared state: across variance seeds the offsets accumulated (every seed after the first selected the wrong examples) and the per-seed cache entries all aliased one over-rotated list. Copy the pool before rotating, matching `_init_fewshot_sampling_random`. The existing sequential test compared against `fewshot_docs()` after sampling, so it read back the mutated pool and passed despite the bug; update it to check the rotation against the original order, and add regression tests for pool integrity and seed independence.
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.
Fixes #1307.
FewShotSampler._init_fewshot_sampling_sequentialrotated the list returned bytask.fewshot_docs()in place to offset the selection per seed. That list is the task's memoized_fewshot_docs, returned by reference, so the rotation mutated shared state. When an evaluation runs several few-shot seeds against the same task (variance estimation), each seed rotated the already-rotated pool, so the offsets accumulated and every seed after the first selected the wrong examples. The per-seed cache stores a reference rather than a copy, so the cached selections all aliased one over-rotated list, and the pool itself was left rotated after the run.The fix copies the pool before rotating, matching what
_init_fewshot_sampling_randomalready does. Only thesequentialmethod is affected:randomcopies the pool, andbalancedbuilds its own per-label lists and never mutates it.Repro on
mainbefore the fix:Tests
The existing
test_fewshot_sampler[sequential]compared the result againstfewshot_docs()after sampling, so it read back the mutated pool and passed despite the bug. I updated it to check the rotation against the original order, and added two regression tests:test_sequential_fewshot_does_not_mutate_shared_pool: the memoized pool is unchanged after sampling.test_sequential_fewshot_seeds_are_independent: each seed selects the correct rotation from the original order.Both new tests fail on
mainand pass with the fix.