Fix Unigram trainer prune loss using vocab size instead of per-piece alternatives count - #2346
Open
mayuriphad wants to merge 1 commit into
Open
Fix Unigram trainer prune loss using vocab size instead of per-piece alternatives count#2346mayuriphad wants to merge 1 commit into
mayuriphad wants to merge 1 commit into
Conversation
prune_sentence_pieces computed logsum_alt using alternatives.len() (the size of the whole vocabulary vector) instead of alternatives[id].len() (the number of resegmentation alternatives for the one piece being scored). This was a porting mistake from SentencePiece's alternatives[i].size(), and it inflates the prune loss of high-frequency pieces by a factor tied to vocab size rather than to how many alternatives that piece actually has, changing which pieces survive pruning during Unigram training. Adds a regression test that constructs two candidate pieces with different alternative counts and frequencies, calibrated so that the correct formula and the buggy formula rank them in opposite order.
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.
Bug
Fixes #2069.
UnigramTrainer::prune_sentence_pieces(tokenizers/src/models/unigram/trainer.rs) is a Rust port of SentencePiece'sPruneSentencePieces. The loss computation for a candidate pieceidestimates the new corpus-frequency sum if that piece were removed and its frequency reassigned to its resegmentation alternatives:alternativesisVec<Vec<usize>>, one inner vector per vocabulary piece, soalternatives.len()is justpieces.len()(the whole vocabulary size) — not the number of alternatives forid. The comment right above it even saysalternatives[i].size(), matching SentencePiece's actual code (src/unigram_model_trainer.cc), so the[id]index was dropped when this was ported.Since
freq[id] * (alternatives.len() - 1)scales with total vocab size instead of the piece's own (usually 1-2) alternatives, this inflates the loss of high-frequency pieces by an amount tied to vocab size, which changes which pieces survive pruning. This was already reported in #1536 and re-filed with a fuller repro in #2069 (including an end-to-end comparison on a real corpus showing the trained vocabularies differ deterministically between the two formulas).Fix
Use the per-piece alternative count instead of the whole-vocabulary size:
This branch only runs when
alternatives[id]is non-empty, so the- 1cannot underflow.Test plan
Added
test_prune_sentence_pieces_uses_per_piece_alternatives_lenintokenizers/src/models/unigram/trainer.rs, which callsprune_sentence_piecesdirectly with a small hand-constructed vocabulary containing two candidate pieces ("abcdef", freq 50, 6 alternatives; "xyz", freq 110, 3 alternatives) sized so only one of the two survives pruning. The relative frequencies/alternative counts are chosen (and verified by hand, see comments in the test) so the correct formula ranks "abcdef" above "xyz", while the buggy formula ranks them in the opposite order — this makes the test fail against the pre-fix code and pass against the fix.I verified the fix by careful reading of
prune_sentence_piecesplus the SentencePiece reference implementation, and by manually working out the loss values for both formulas for the test's inputs (shown in the test's comments). I was not able to build/run the Rust test suite in my current environment (nocargo/rustcavailable), so I was not able to executecargo test -p tokenizers unigram::trainerlocally — please run it in CI. Happy to iterate if the numbers don't line up exactly once it runs.