Repository navigation
FIX: Canonicalize RandomTranslationConverter language order for seeded selection - #3804
Closed
Chen Yufeiyang (feiiiiii5) wants to merge 2 commits into
Closed
Chen Yufeiyang (feiiiiii5) wants to merge 2 commits into
Chen Yufeiyang (feiiiiii5) wants to merge 2 commits into
Conversation
…d selection The converter identifier hashed the languages in sorted order, but the seeded random choice used the constructor order, so two converters with the same language set and seed had the same identity yet produced different translations. Canonicalize the language pool once in __init__ and use it for both the identifier and the choice. Fixes microsoft#3717
Contributor
|
Thanks for the contribution. Lanre Shittu (@Shizoqua) claimed #3717 first and opened #3784 before this PR. I compared the current heads, and both implement the same canonical language ordering and seeded-selection regression. I did not find separate behavior here that needs to be carried over. I am closing this later duplicate so we can review one implementation. Thank you for understanding. |
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.
Description
Fixes #3717
RandomTranslationConverterbuilt its identifier from the language pool in sorted order, but the seeded random choice used the constructor order. Two converters with the same language set and seed therefore had the same identifier yet translated the same prompt differently ([French, Spanish]vs[Spanish, French]swapped the target language assignment), so the identity did not describe the behavior and replay/config comparison was misleading.The constructor now canonicalizes
self.languagesonce (case-folded sort) and both the identifier and the random selection use that single order. Reordering the same set no longer changes the hash or the seeded translation. The default 37-language pool is canonicalized too, so seeded selection in the default configuration is now order-independent as the identity contract already implied; there is no public signature change.Tests
New
test_random_translation_converter_canonicalizes_language_order_for_selectionconfigures seed 42 with both pool orders and asserts the identifier and the target prompt are identical.Not run: the full
make unit-testand CI matrix (local Python 3.14 venv; 3.11-3.13 not verified locally), and Windows.Tests and Documentation
No documentation describes language ordering; none changed.