FIX: make combine_list order-deterministic and drop the duplicated workaround - #2614
Merged
Roman Lutz (romanlutz) merged 3 commits intoSep 14, 2026
Conversation
…rkaround
combine_list returned list(set(list1 + list2)), so the order of the result varied
between processes under hash randomization:
['illegal', 'bias', 'violence', 'harmful', 'pii']
['pii', 'bias', 'violence', 'illegal', 'harmful']
['harmful', 'illegal', 'bias', 'pii', 'violence']
The codebase already knew. seed_dataset.py carried a private _merge_unique with
this docstring:
Used instead of ``utils.combine_list`` because the latter goes through
``set()`` and is nondeterministic across processes for non-trivial inputs.
and the tests for combine_list compared with set(result) == {...} rather than
asserting a result, because there was no order to assert.
Use dict.fromkeys instead, which deduplicates while preserving first-occurrence
order, and accept None as empty so the function covers what _merge_unique did.
_merge_unique is then removed and its single call site uses combine_list.
combine_list is public API, but going from an arbitrary order to a defined one
cannot break a caller that was already correct.
Tests now assert the exact order, including a case that pins first-occurrence
ordering across both arguments.
Contributor
Author
|
@microsoft-github-policy-service agree |
Roman Lutz (romanlutz)
requested changes
Sep 11, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep dataset-specific empty metadata normalization at the call site and cover both contracts with regression tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Roman Lutz (romanlutz)
approved these changes
Sep 14, 2026
Roman Lutz (romanlutz)
left a comment
Roman Lutz (romanlutz)
left a comment
Contributor
There was a problem hiding this comment.
just took care of it myself to speed it up. Thanks a ton for this contribution!
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
2 participants
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.
combine_listreturnslist(set(list1 + list2)), so the order of the result varies between processes under hash randomization:The codebase already knows
pyrit/models/seeds/seed_dataset.pycarries a private_merge_uniquewhose docstring says:And the existing tests compare with
set(result) == {...}rather than asserting a result, because there was no order to assert.Changes
Use
dict.fromkeys, which deduplicates while preserving first-occurrence order, and acceptNoneas empty so the function covers everything_merge_uniquedid._merge_uniqueis then removed and its single call site usescombine_list.combine_listis public API, but moving from an arbitrary order to a defined one cannot break a caller that was already correct.Tests now assert exact order, including a case that pins first-occurrence ordering across both arguments, and cover the
Noneinputs.Verification
PYTHONHASHSEED=randomnow return an identical resultpytest tests/unit: 16955 passed, 126 skipped (16953 onmain, +2 new tests, nothing broken)ruff format --check,ruff check: clean