Repository navigation
Conversation
dotnet-policy-service
Bot
added
the
community-contribution
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new missing-EOS behavior lacks corresponding test coverage.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Enables Unigram SentencePiece models without BOS/EOS tokens, including T5-style models.
Changes:
- Preserves absent BOS/EOS IDs as
-1. - Avoids mapping absent special tokens.
- Adds missing-BOS behavior tests.
| File | Description |
|---|---|
SentencePieceUnigramModel.cs |
Handles optional BOS/EOS IDs safely. |
SentencePieceTests.cs |
Tests T5-style models without BOS. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7757 +/- ##
==========================================
+ Coverage 69.89% 69.91% +0.01%
==========================================
Files 1487 1487
Lines 276323 276390 +67
Branches 28295 28297 +2
==========================================
+ Hits 193142 193238 +96
+ Misses 75694 75667 -27
+ Partials 7487 7485 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
Loading a Unigram SentencePiece model trained without BOS, such as T5 (
bos_id = -1), threwIndexOutOfRangeExceptionin theSentencePieceUnigramModelconstructor, whateveraddBeginningOfSentencewas. A model read from aModelProtonow treats an absent BOS or EOS as a model built from a vocabulary does: the id stays -1 instead of becoming 0, which is<pad>in T5, the piece is not mapped, and requesting it throwsArgumentException.Models from the Hugging Face hub, created with
Create(stream, addBeginningOfSentence: false, addEndOfSentence: false); the columns give the English sentences of the XNLI test split (1,670) that encode as in SentencePiece 0.2.2:bos_idgoogle-t5/t5-smallIndexOutOfRangeExceptiongoogle-t5/t5-baseIndexOutOfRangeExceptiongoogle/t5-v1_1-smallIndexOutOfRangeExceptiongoogle/flan-t5-smallIndexOutOfRangeExceptiongoogle/flan-t5-baseIndexOutOfRangeExceptiongoogle/mt5-smallIndexOutOfRangeExceptiongoogle/mt5-baseIndexOutOfRangeExceptiongoogle/long-t5-tglobal-baseIndexOutOfRangeExceptiongoogle/pegasus-xsumIndexOutOfRangeExceptiongoogle/umt5-smallFacebookAI/xlm-roberta-baseRequesting the BOS of such a model, as
Create(stream)does by default, now throwsArgumentExceptionforaddBeginningOfSentence, as when the model is built from a vocabulary; SentencePiece also rejects a BOS the model does not define. The two models with a BOS are unchanged.Four tests in
SentencePieceTestsbuild Unigram models without BOS, laid out as T5's, and without EOS, and cover encoding, decoding and the request for the missing token; all fail without the change.Fixes #7756