Introducing SentencePiece Unigram Tokenizer Model - #7390

Merged
tarekgh merged 9 commits into
dotnet:mainfrom
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Feb 25, 2025
Merged

Introducing SentencePiece Unigram Tokenizer Model#7390
tarekgh merged 9 commits into
dotnet:mainfrom
tarekgh:IntroducingSentencePieceUnigramTokenizerModel

Conversation

@tarekgh

@tarekghtarekgh commented Feb 18, 2025

Copy link
Copy Markdown
Member

Fixes#7186

We have been supporting SentencePiece Bpe model for a while, the change here is introducing the support to the SentencePiece Unigram tokenizer model.

Users can create a tokenizer instance by using a code like the following:

// download the tokenizer.model from the LLM model site like the following: // @"https://huggingface.co/sentence-transformers/paraphrase-multilingual-MiniLM-L12-v2/resolve/main/sentencepiece.bpe.model?download=true";usingStreamremoteStream=File.OpenRead(path/to/the/tokenizer/model/file);SentencePieceTokenizertokenizer=SentencePieceTokenizer.Create(remoteStream);

Notes around the change

  • Have done some refactoring code existed in the SentencePieceToknizer.cs file into a newly introduced file SentencePieceBaseModel.cs and SentencePieceBpeModel.cs. Most of the code in the newly introduced files are mostly not changed.
  • Added the new Unigram implementation into the SentencePieceUnigramModel.cs file.
  • Changed SentencePieceTokenizer.cs to work with the model abstraction and automatically handle both models Bpe and Unigram.
  • The change shouldn't introduce any behavior change to the existing model (i.e Bpe).

CopilotAI review requested due to automatic review settings February 18, 2025 20:10
@tarekgh

tarekgh commented Feb 18, 2025•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

CC @ericstj@luisquintanilla@LittleLittleCloud@michaelgsharp@stephentoub

CopilotAI reviewed Feb 18, 2025
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 11 out of 14 changed files in this pull request and generated 1 comment.

Files not reviewed (3)
  • THIRD-PARTY-NOTICES.TXT: Language not supported
  • eng/Versions.props: Language not supported
  • src/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs: Evaluated as low risk

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs Outdated
@tarekghtarekgh added this to the ML.NET 5.0 milestone Feb 18, 2025
@codecov

codecovBot commented Feb 18, 2025•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 75.07347% with 933 lines in your changes missing coverage. Please review.

Project coverage is 68.94%. Comparing base (e3219a9) to head (d244042).
Report is 5 commits behind head on main.

Files with missing linesPatch %Lines
...t.ML.Tokenizers/Model/SentencePieceUnigramModel.cs60.18%293 Missing and 47 partials ⚠️
...osoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs77.43%158 Missing and 32 partials ⚠️
...c/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs82.66%107 Missing and 19 partials ⚠️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04%73 Missing and 32 partials ⚠️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs56.77%61 Missing and 22 partials ⚠️
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs37.77%56 Missing ⚠️
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61%16 Missing and 5 partials ⚠️
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10%6 Missing and 6 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7390 +/- ##
==========================================
+ Coverage 68.88% 68.94% +0.06% 
==========================================
Files 1473 1480 +7 Lines 270814 273273 +2459 Branches 27884 28234 +350 ==========================================
+ Hits 186553 188419 +1866 - Misses 76985 77490 +505 - Partials 7276 7364 +88
FlagCoverage Δ
Debug68.94% <75.07%> (+0.06%)⬆️
production63.26% <71.73%> (+0.06%)⬆️
test89.46% <97.52%> (+0.06%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing linesCoverage Δ
.../Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs72.76% <ø> (ø)
...rc/Microsoft.ML.Tokenizers/Model/LlamaTokenizer.cs43.75% <ø> (-15.35%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceTokenizer.cs90.00% <ø> (+13.13%)⬆️
...icrosoft.ML.Tokenizers/Utils/Helpers.netcoreapp.cs39.47% <100.00%> (+1.63%)⬆️
...crosoft.ML.Tokenizers.Tests/DoubleArrayTrieTest.cs100.00% <100.00%> (ø)
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10% <97.10%> (ø)
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61% <59.61%> (ø)
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs70.63% <37.77%> (-20.46%)⬇️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs70.32% <56.77%> (-17.95%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04% <79.04%> (ø)
... and 3 more

... and 11 files with indirect coverage changes

ericstj
ericstj reviewed Feb 18, 2025
View reviewed changes

@ericstjericstj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't dig too deep just a few small observations and suggestions.

Comment threadsrc/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs Outdated
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs Outdated
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs Outdated
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs Outdated
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs
@ericstj
ericstj requested a review from CopilotFebruary 18, 2025 22:09
CopilotAI reviewed Feb 18, 2025
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/Microsoft.ML.Tokenizers/Normalizer/SentencePieceNormalizer.cs:392

  • In this loop, calling sp.Slice(1) does not update the variable 'sp', which may result in an infinite loop if the condition remains true. Consider reassigning the sliced span to 'sp' (e.g., sp = sp.Slice(1)).
while (isPrevSpace && sp.Length > 0 && sp[0] == (byte)' ') { sp.Slice(1); }

stephentoub
stephentoub reviewed Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs Outdated
stephentoub
stephentoub reviewed Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs Outdated
stephentoub
stephentoub reviewed Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs Outdated
stephentoub
stephentoub reviewed Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs Outdated
stephentoub
stephentoub reviewed Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs Outdated
stephentoub
stephentoub reviewed Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs Outdated
stephentoub
stephentoub reviewed Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs Outdated
luisquintanilla
luisquintanilla reviewed Feb 19, 2025
View reviewed changes

@luisquintanillaluisquintanilla left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo - SentencePieceUingramModel.cs should be SentencePieceUnigramModel.cs

ericstj
ericstj reviewed Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj reviewed Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
ericstj
ericstj reviewed Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj reviewed Feb 20, 2025
View reviewed changes
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
ericstj
ericstj reviewed Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs Outdated
ericstj
ericstj approved these changes Feb 21, 2025
View reviewed changes

@ericstjericstj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing feedback. This looks good to me. Please check if @michaelgsharp has feedback too.

michaelgsharp
michaelgsharp reviewed Feb 24, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
@build-analysisbuild-analysisBot mentioned this pull request Feb 24, 2025
Open
@tarekgh
tarekgh merged commit 2bd88b9 into dotnet:mainFeb 25, 2025
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Mar 27, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub left review comments
@luisquintanillaluisquintanillaluisquintanilla left review comments
Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@ericstjericstjericstj approved these changes
+1 more reviewer
@michaelgsharpmichaelgsharpmichaelgsharp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels

None yet

Projects

None yet

Milestone

ML.NET 5.0

Development

Successfully merging this pull request may close these issues.

Implement Sentencepiece Unigram tokenizer

6 participants

@tarekgh@stephentoub@ericstj@luisquintanilla@michaelgsharp
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Introducing SentencePiece Unigram Tokenizer Model - #7390
Merged
tarekgh merged 9 commits into
dotnet:mainfrom 
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Feb 25, 2025
Merged
Introducing SentencePiece Unigram Tokenizer Model#7390
tarekgh merged 9 commits into
dotnet:mainfrom 
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Conversation
@tarekgh
@tarekghtarekgh
commented
Feb 18, 2025

Copy link


Copy Markdown

Member
Fixes#7186
We have been supporting SentencePiece Bpe model for a while, the change here is introducing the support to the SentencePiece Unigram tokenizer model.
Users can create a tokenizer instance by using a code like the following:
// download the tokenizer.model from the LLM model site like the following: // @"https://huggingface.co/sentence-transformers/paraphrase-multilingual-MiniLM-L12-v2/resolve/main/sentencepiece.bpe.model?download=true";usingStreamremoteStream=File.OpenRead(path/to/the/tokenizer/model/file);SentencePieceTokenizertokenizer=SentencePieceTokenizer.Create(remoteStream);
Notes around the change
  • Have done some refactoring code existed in the SentencePieceToknizer.cs file into a newly introduced file SentencePieceBaseModel.cs and SentencePieceBpeModel.cs. Most of the code in the newly introduced files are mostly not changed.
  • Added the new Unigram implementation into the SentencePieceUnigramModel.cs file.
  • Changed SentencePieceTokenizer.cs to work with the model abstraction and automatically handle both models Bpe and Unigram.
  • The change shouldn't introduce any behavior change to the existing model (i.e Bpe).
CopilotAI
review requested due to automatic review settings
February 18, 2025 20:10
@tarekgh
tarekgh
commented
Feb 18, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
CC @ericstj@luisquintanilla@LittleLittleCloud@michaelgsharp@stephentoub
CopilotAI
reviewed
Feb 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 11 out of 14 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
  • THIRD-PARTY-NOTICES.TXT: Language not supported
  • eng/Versions.props: Language not supported
  • src/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs: Evaluated as low risk
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Outdated

@tarekghtarekgh
added this to the ML.NET 5.0 milestone
Feb 18, 2025
@codecov
codecovBot
commented
Feb 18, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 75.07347% with 933 lines in your changes missing coverage. Please review.
Project coverage is 68.94%. Comparing base (e3219a9) to head (d244042).

Report is 5 commits behind head on main.
Files with missing linesPatch %Lines
...t.ML.Tokenizers/Model/SentencePieceUnigramModel.cs60.18%293 Missing and 47 partials ⚠️
...osoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs77.43%158 Missing and 32 partials ⚠️
...c/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs82.66%107 Missing and 19 partials ⚠️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04%73 Missing and 32 partials ⚠️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs56.77%61 Missing and 22 partials ⚠️
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs37.77%56 Missing ⚠️
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61%16 Missing and 5 partials ⚠️
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10%6 Missing and 6 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7390 +/- ##
==========================================
+ Coverage 68.88% 68.94% +0.06% 
==========================================
Files 1473 1480 +7 Lines 270814 273273 +2459 Branches 27884 28234 +350 ==========================================
+ Hits 186553 188419 +1866 - Misses 76985 77490 +505 - Partials 7276 7364 +88 
FlagCoverage Δ
Debug68.94% <75.07%> (+0.06%)⬆️
production63.26% <71.73%> (+0.06%)⬆️
test89.46% <97.52%> (+0.06%)⬆️
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing linesCoverage Δ
.../Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs72.76% <ø> (ø)
...rc/Microsoft.ML.Tokenizers/Model/LlamaTokenizer.cs43.75% <ø> (-15.35%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceTokenizer.cs90.00% <ø> (+13.13%)⬆️
...icrosoft.ML.Tokenizers/Utils/Helpers.netcoreapp.cs39.47% <100.00%> (+1.63%)⬆️
...crosoft.ML.Tokenizers.Tests/DoubleArrayTrieTest.cs100.00% <100.00%> (ø)
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10% <97.10%> (ø)
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61% <59.61%> (ø)
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs70.63% <37.77%> (-20.46%)⬇️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs70.32% <56.77%> (-17.95%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04% <79.04%> (ø)
... and 3 more
... and 11 files with indirect coverage changes
ericstj
ericstj
reviewed
Feb 18, 2025
View reviewed changes
@ericstjericstj
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't dig too deep just a few small observations and suggestions.
Comment threadsrc/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs
@ericstj
ericstj
requested a review
from CopilotFebruary 18, 2025 22:09
CopilotAI
reviewed
Feb 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/Microsoft.ML.Tokenizers/Normalizer/SentencePieceNormalizer.cs:392
  • In this loop, calling sp.Slice(1) does not update the variable 'sp', which may result in an infinite loop if the condition remains true. Consider reassigning the sliced span to 'sp' (e.g., sp = sp.Slice(1)).
while (isPrevSpace && sp.Length > 0 && sp[0] == (byte)' ') { sp.Slice(1); }

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs
Outdated

luisquintanilla
luisquintanilla
reviewed
Feb 19, 2025
View reviewed changes
@luisquintanillaluisquintanilla
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo - SentencePieceUingramModel.cs should be SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs
Outdated

ericstj
ericstj
approved these changes
Feb 21, 2025
View reviewed changes
@ericstjericstj
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing feedback. This looks good to me. Please check if @michaelgsharp has feedback too.
michaelgsharp
michaelgsharp
reviewed
Feb 24, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
@build-analysisbuild-analysisBot
mentioned this pull request
Feb 24, 2025

Open

@tarekgh
tarekgh
merged commit 2bd88b9
into
dotnet:mainFeb 25, 2025
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Mar 27, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@luisquintanillaluisquintanillaluisquintanilla left review comments
Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@ericstjericstjericstj approved these changes

+1 more reviewer

@michaelgsharpmichaelgsharpmichaelgsharp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels


None yet


Projects


None yet


Milestone

ML.NET 5.0

Development

Successfully merging this pull request may close these issues.

Implement Sentencepiece Unigram tokenizer


6 participants

@tarekgh@stephentoub@ericstj@luisquintanilla@michaelgsharp

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Introducing SentencePiece Unigram Tokenizer Model - #7390
Merged
tarekgh merged 9 commits into
dotnet:mainfrom 
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Feb 25, 2025
Merged
Introducing SentencePiece Unigram Tokenizer Model#7390
tarekgh merged 9 commits into
dotnet:mainfrom 
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Conversation
@tarekgh
@tarekghtarekgh
commented
Feb 18, 2025

Copy link


Copy Markdown

Member
Fixes#7186
We have been supporting SentencePiece Bpe model for a while, the change here is introducing the support to the SentencePiece Unigram tokenizer model.
Users can create a tokenizer instance by using a code like the following:
// download the tokenizer.model from the LLM model site like the following: // @"https://huggingface.co/sentence-transformers/paraphrase-multilingual-MiniLM-L12-v2/resolve/main/sentencepiece.bpe.model?download=true";usingStreamremoteStream=File.OpenRead(path/to/the/tokenizer/model/file);SentencePieceTokenizertokenizer=SentencePieceTokenizer.Create(remoteStream);
Notes around the change
  • Have done some refactoring code existed in the SentencePieceToknizer.cs file into a newly introduced file SentencePieceBaseModel.cs and SentencePieceBpeModel.cs. Most of the code in the newly introduced files are mostly not changed.
  • Added the new Unigram implementation into the SentencePieceUnigramModel.cs file.
  • Changed SentencePieceTokenizer.cs to work with the model abstraction and automatically handle both models Bpe and Unigram.
  • The change shouldn't introduce any behavior change to the existing model (i.e Bpe).
CopilotAI
review requested due to automatic review settings
February 18, 2025 20:10
@tarekgh
tarekgh
commented
Feb 18, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
CC @ericstj@luisquintanilla@LittleLittleCloud@michaelgsharp@stephentoub
CopilotAI
reviewed
Feb 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 11 out of 14 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
  • THIRD-PARTY-NOTICES.TXT: Language not supported
  • eng/Versions.props: Language not supported
  • src/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs: Evaluated as low risk
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Outdated

@tarekghtarekgh
added this to the ML.NET 5.0 milestone
Feb 18, 2025
@codecov
codecovBot
commented
Feb 18, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 75.07347% with 933 lines in your changes missing coverage. Please review.
Project coverage is 68.94%. Comparing base (e3219a9) to head (d244042).

Report is 5 commits behind head on main.
Files with missing linesPatch %Lines
...t.ML.Tokenizers/Model/SentencePieceUnigramModel.cs60.18%293 Missing and 47 partials ⚠️
...osoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs77.43%158 Missing and 32 partials ⚠️
...c/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs82.66%107 Missing and 19 partials ⚠️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04%73 Missing and 32 partials ⚠️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs56.77%61 Missing and 22 partials ⚠️
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs37.77%56 Missing ⚠️
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61%16 Missing and 5 partials ⚠️
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10%6 Missing and 6 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7390 +/- ##
==========================================
+ Coverage 68.88% 68.94% +0.06% 
==========================================
Files 1473 1480 +7 Lines 270814 273273 +2459 Branches 27884 28234 +350 ==========================================
+ Hits 186553 188419 +1866 - Misses 76985 77490 +505 - Partials 7276 7364 +88 
FlagCoverage Δ
Debug68.94% <75.07%> (+0.06%)⬆️
production63.26% <71.73%> (+0.06%)⬆️
test89.46% <97.52%> (+0.06%)⬆️
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing linesCoverage Δ
.../Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs72.76% <ø> (ø)
...rc/Microsoft.ML.Tokenizers/Model/LlamaTokenizer.cs43.75% <ø> (-15.35%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceTokenizer.cs90.00% <ø> (+13.13%)⬆️
...icrosoft.ML.Tokenizers/Utils/Helpers.netcoreapp.cs39.47% <100.00%> (+1.63%)⬆️
...crosoft.ML.Tokenizers.Tests/DoubleArrayTrieTest.cs100.00% <100.00%> (ø)
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10% <97.10%> (ø)
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61% <59.61%> (ø)
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs70.63% <37.77%> (-20.46%)⬇️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs70.32% <56.77%> (-17.95%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04% <79.04%> (ø)
... and 3 more
... and 11 files with indirect coverage changes
ericstj
ericstj
reviewed
Feb 18, 2025
View reviewed changes
@ericstjericstj
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't dig too deep just a few small observations and suggestions.
Comment threadsrc/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs
@ericstj
ericstj
requested a review
from CopilotFebruary 18, 2025 22:09
CopilotAI
reviewed
Feb 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/Microsoft.ML.Tokenizers/Normalizer/SentencePieceNormalizer.cs:392
  • In this loop, calling sp.Slice(1) does not update the variable 'sp', which may result in an infinite loop if the condition remains true. Consider reassigning the sliced span to 'sp' (e.g., sp = sp.Slice(1)).
while (isPrevSpace && sp.Length > 0 && sp[0] == (byte)' ') { sp.Slice(1); }

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs
Outdated

luisquintanilla
luisquintanilla
reviewed
Feb 19, 2025
View reviewed changes
@luisquintanillaluisquintanilla
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo - SentencePieceUingramModel.cs should be SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs
Outdated

ericstj
ericstj
approved these changes
Feb 21, 2025
View reviewed changes
@ericstjericstj
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing feedback. This looks good to me. Please check if @michaelgsharp has feedback too.
michaelgsharp
michaelgsharp
reviewed
Feb 24, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
@build-analysisbuild-analysisBot
mentioned this pull request
Feb 24, 2025

Open

@tarekgh
tarekgh
merged commit 2bd88b9
into
dotnet:mainFeb 25, 2025
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Mar 27, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@luisquintanillaluisquintanillaluisquintanilla left review comments
Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@ericstjericstjericstj approved these changes

+1 more reviewer

@michaelgsharpmichaelgsharpmichaelgsharp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels


None yet


Projects


None yet


Milestone

ML.NET 5.0

Development

Successfully merging this pull request may close these issues.

Implement Sentencepiece Unigram tokenizer


6 participants

@tarekgh@stephentoub@ericstj@luisquintanilla@michaelgsharp

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Introducing SentencePiece Unigram Tokenizer Model - #7390
Merged
tarekgh merged 9 commits into
dotnet:mainfrom 
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Feb 25, 2025
Merged
Introducing SentencePiece Unigram Tokenizer Model#7390
tarekgh merged 9 commits into
dotnet:mainfrom 
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Conversation
@tarekgh
@tarekghtarekgh
commented
Feb 18, 2025

Copy link


Copy Markdown

Member
Fixes#7186
We have been supporting SentencePiece Bpe model for a while, the change here is introducing the support to the SentencePiece Unigram tokenizer model.
Users can create a tokenizer instance by using a code like the following:
// download the tokenizer.model from the LLM model site like the following: // @"https://huggingface.co/sentence-transformers/paraphrase-multilingual-MiniLM-L12-v2/resolve/main/sentencepiece.bpe.model?download=true";usingStreamremoteStream=File.OpenRead(path/to/the/tokenizer/model/file);SentencePieceTokenizertokenizer=SentencePieceTokenizer.Create(remoteStream);
Notes around the change
  • Have done some refactoring code existed in the SentencePieceToknizer.cs file into a newly introduced file SentencePieceBaseModel.cs and SentencePieceBpeModel.cs. Most of the code in the newly introduced files are mostly not changed.
  • Added the new Unigram implementation into the SentencePieceUnigramModel.cs file.
  • Changed SentencePieceTokenizer.cs to work with the model abstraction and automatically handle both models Bpe and Unigram.
  • The change shouldn't introduce any behavior change to the existing model (i.e Bpe).
CopilotAI
review requested due to automatic review settings
February 18, 2025 20:10
@tarekgh
tarekgh
commented
Feb 18, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
CC @ericstj@luisquintanilla@LittleLittleCloud@michaelgsharp@stephentoub
CopilotAI
reviewed
Feb 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 11 out of 14 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
  • THIRD-PARTY-NOTICES.TXT: Language not supported
  • eng/Versions.props: Language not supported
  • src/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs: Evaluated as low risk
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Outdated

@tarekghtarekgh
added this to the ML.NET 5.0 milestone
Feb 18, 2025
@codecov
codecovBot
commented
Feb 18, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 75.07347% with 933 lines in your changes missing coverage. Please review.
Project coverage is 68.94%. Comparing base (e3219a9) to head (d244042).

Report is 5 commits behind head on main.
Files with missing linesPatch %Lines
...t.ML.Tokenizers/Model/SentencePieceUnigramModel.cs60.18%293 Missing and 47 partials ⚠️
...osoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs77.43%158 Missing and 32 partials ⚠️
...c/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs82.66%107 Missing and 19 partials ⚠️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04%73 Missing and 32 partials ⚠️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs56.77%61 Missing and 22 partials ⚠️
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs37.77%56 Missing ⚠️
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61%16 Missing and 5 partials ⚠️
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10%6 Missing and 6 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7390 +/- ##
==========================================
+ Coverage 68.88% 68.94% +0.06% 
==========================================
Files 1473 1480 +7 Lines 270814 273273 +2459 Branches 27884 28234 +350 ==========================================
+ Hits 186553 188419 +1866 - Misses 76985 77490 +505 - Partials 7276 7364 +88 
FlagCoverage Δ
Debug68.94% <75.07%> (+0.06%)⬆️
production63.26% <71.73%> (+0.06%)⬆️
test89.46% <97.52%> (+0.06%)⬆️
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing linesCoverage Δ
.../Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs72.76% <ø> (ø)
...rc/Microsoft.ML.Tokenizers/Model/LlamaTokenizer.cs43.75% <ø> (-15.35%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceTokenizer.cs90.00% <ø> (+13.13%)⬆️
...icrosoft.ML.Tokenizers/Utils/Helpers.netcoreapp.cs39.47% <100.00%> (+1.63%)⬆️
...crosoft.ML.Tokenizers.Tests/DoubleArrayTrieTest.cs100.00% <100.00%> (ø)
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10% <97.10%> (ø)
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61% <59.61%> (ø)
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs70.63% <37.77%> (-20.46%)⬇️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs70.32% <56.77%> (-17.95%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04% <79.04%> (ø)
... and 3 more
... and 11 files with indirect coverage changes
ericstj
ericstj
reviewed
Feb 18, 2025
View reviewed changes
@ericstjericstj
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't dig too deep just a few small observations and suggestions.
Comment threadsrc/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs
@ericstj
ericstj
requested a review
from CopilotFebruary 18, 2025 22:09
CopilotAI
reviewed
Feb 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/Microsoft.ML.Tokenizers/Normalizer/SentencePieceNormalizer.cs:392
  • In this loop, calling sp.Slice(1) does not update the variable 'sp', which may result in an infinite loop if the condition remains true. Consider reassigning the sliced span to 'sp' (e.g., sp = sp.Slice(1)).
while (isPrevSpace && sp.Length > 0 && sp[0] == (byte)' ') { sp.Slice(1); }

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs
Outdated

luisquintanilla
luisquintanilla
reviewed
Feb 19, 2025
View reviewed changes
@luisquintanillaluisquintanilla
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo - SentencePieceUingramModel.cs should be SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs
Outdated

ericstj
ericstj
approved these changes
Feb 21, 2025
View reviewed changes
@ericstjericstj
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing feedback. This looks good to me. Please check if @michaelgsharp has feedback too.
michaelgsharp
michaelgsharp
reviewed
Feb 24, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
@build-analysisbuild-analysisBot
mentioned this pull request
Feb 24, 2025

Open

@tarekgh
tarekgh
merged commit 2bd88b9
into
dotnet:mainFeb 25, 2025
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Mar 27, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@luisquintanillaluisquintanillaluisquintanilla left review comments
Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@ericstjericstjericstj approved these changes

+1 more reviewer

@michaelgsharpmichaelgsharpmichaelgsharp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels


None yet


Projects


None yet


Milestone

ML.NET 5.0

Development

Successfully merging this pull request may close these issues.

Implement Sentencepiece Unigram tokenizer


6 participants

@tarekgh@stephentoub@ericstj@luisquintanilla@michaelgsharp

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Introducing SentencePiece Unigram Tokenizer Model - #7390
Merged
tarekgh merged 9 commits into
dotnet:mainfrom 
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Feb 25, 2025
Merged
Introducing SentencePiece Unigram Tokenizer Model#7390
tarekgh merged 9 commits into
dotnet:mainfrom 
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Conversation
@tarekgh
@tarekghtarekgh
commented
Feb 18, 2025

Copy link


Copy Markdown

Member
Fixes#7186
We have been supporting SentencePiece Bpe model for a while, the change here is introducing the support to the SentencePiece Unigram tokenizer model.
Users can create a tokenizer instance by using a code like the following:
// download the tokenizer.model from the LLM model site like the following: // @"https://huggingface.co/sentence-transformers/paraphrase-multilingual-MiniLM-L12-v2/resolve/main/sentencepiece.bpe.model?download=true";usingStreamremoteStream=File.OpenRead(path/to/the/tokenizer/model/file);SentencePieceTokenizertokenizer=SentencePieceTokenizer.Create(remoteStream);
Notes around the change
  • Have done some refactoring code existed in the SentencePieceToknizer.cs file into a newly introduced file SentencePieceBaseModel.cs and SentencePieceBpeModel.cs. Most of the code in the newly introduced files are mostly not changed.
  • Added the new Unigram implementation into the SentencePieceUnigramModel.cs file.
  • Changed SentencePieceTokenizer.cs to work with the model abstraction and automatically handle both models Bpe and Unigram.
  • The change shouldn't introduce any behavior change to the existing model (i.e Bpe).
CopilotAI
review requested due to automatic review settings
February 18, 2025 20:10
@tarekgh
tarekgh
commented
Feb 18, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
CC @ericstj@luisquintanilla@LittleLittleCloud@michaelgsharp@stephentoub
CopilotAI
reviewed
Feb 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 11 out of 14 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
  • THIRD-PARTY-NOTICES.TXT: Language not supported
  • eng/Versions.props: Language not supported
  • src/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs: Evaluated as low risk
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Outdated

@tarekghtarekgh
added this to the ML.NET 5.0 milestone
Feb 18, 2025
@codecov
codecovBot
commented
Feb 18, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 75.07347% with 933 lines in your changes missing coverage. Please review.
Project coverage is 68.94%. Comparing base (e3219a9) to head (d244042).

Report is 5 commits behind head on main.
Files with missing linesPatch %Lines
...t.ML.Tokenizers/Model/SentencePieceUnigramModel.cs60.18%293 Missing and 47 partials ⚠️
...osoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs77.43%158 Missing and 32 partials ⚠️
...c/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs82.66%107 Missing and 19 partials ⚠️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04%73 Missing and 32 partials ⚠️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs56.77%61 Missing and 22 partials ⚠️
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs37.77%56 Missing ⚠️
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61%16 Missing and 5 partials ⚠️
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10%6 Missing and 6 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7390 +/- ##
==========================================
+ Coverage 68.88% 68.94% +0.06% 
==========================================
Files 1473 1480 +7 Lines 270814 273273 +2459 Branches 27884 28234 +350 ==========================================
+ Hits 186553 188419 +1866 - Misses 76985 77490 +505 - Partials 7276 7364 +88 
FlagCoverage Δ
Debug68.94% <75.07%> (+0.06%)⬆️
production63.26% <71.73%> (+0.06%)⬆️
test89.46% <97.52%> (+0.06%)⬆️
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing linesCoverage Δ
.../Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs72.76% <ø> (ø)
...rc/Microsoft.ML.Tokenizers/Model/LlamaTokenizer.cs43.75% <ø> (-15.35%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceTokenizer.cs90.00% <ø> (+13.13%)⬆️
...icrosoft.ML.Tokenizers/Utils/Helpers.netcoreapp.cs39.47% <100.00%> (+1.63%)⬆️
...crosoft.ML.Tokenizers.Tests/DoubleArrayTrieTest.cs100.00% <100.00%> (ø)
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10% <97.10%> (ø)
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61% <59.61%> (ø)
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs70.63% <37.77%> (-20.46%)⬇️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs70.32% <56.77%> (-17.95%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04% <79.04%> (ø)
... and 3 more
... and 11 files with indirect coverage changes
ericstj
ericstj
reviewed
Feb 18, 2025
View reviewed changes
@ericstjericstj
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't dig too deep just a few small observations and suggestions.
Comment threadsrc/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs
@ericstj
ericstj
requested a review
from CopilotFebruary 18, 2025 22:09
CopilotAI
reviewed
Feb 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/Microsoft.ML.Tokenizers/Normalizer/SentencePieceNormalizer.cs:392
  • In this loop, calling sp.Slice(1) does not update the variable 'sp', which may result in an infinite loop if the condition remains true. Consider reassigning the sliced span to 'sp' (e.g., sp = sp.Slice(1)).
while (isPrevSpace && sp.Length > 0 && sp[0] == (byte)' ') { sp.Slice(1); }

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs
Outdated

luisquintanilla
luisquintanilla
reviewed
Feb 19, 2025
View reviewed changes
@luisquintanillaluisquintanilla
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo - SentencePieceUingramModel.cs should be SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs
Outdated

ericstj
ericstj
approved these changes
Feb 21, 2025
View reviewed changes
@ericstjericstj
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing feedback. This looks good to me. Please check if @michaelgsharp has feedback too.
michaelgsharp
michaelgsharp
reviewed
Feb 24, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
@build-analysisbuild-analysisBot
mentioned this pull request
Feb 24, 2025

Open

@tarekgh
tarekgh
merged commit 2bd88b9
into
dotnet:mainFeb 25, 2025
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Mar 27, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@luisquintanillaluisquintanillaluisquintanilla left review comments
Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@ericstjericstjericstj approved these changes

+1 more reviewer

@michaelgsharpmichaelgsharpmichaelgsharp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels


None yet


Projects


None yet


Milestone

ML.NET 5.0

Development

Successfully merging this pull request may close these issues.

Implement Sentencepiece Unigram tokenizer


6 participants

@tarekgh@stephentoub@ericstj@luisquintanilla@michaelgsharp

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Introducing SentencePiece Unigram Tokenizer Model - #7390
Merged
tarekgh merged 9 commits into
dotnet:mainfrom 
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Feb 25, 2025
Merged
Introducing SentencePiece Unigram Tokenizer Model#7390
tarekgh merged 9 commits into
dotnet:mainfrom 
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Conversation
@tarekgh
@tarekghtarekgh
commented
Feb 18, 2025

Copy link


Copy Markdown

Member
Fixes#7186
We have been supporting SentencePiece Bpe model for a while, the change here is introducing the support to the SentencePiece Unigram tokenizer model.
Users can create a tokenizer instance by using a code like the following:
// download the tokenizer.model from the LLM model site like the following: // @"https://huggingface.co/sentence-transformers/paraphrase-multilingual-MiniLM-L12-v2/resolve/main/sentencepiece.bpe.model?download=true";usingStreamremoteStream=File.OpenRead(path/to/the/tokenizer/model/file);SentencePieceTokenizertokenizer=SentencePieceTokenizer.Create(remoteStream);
Notes around the change
  • Have done some refactoring code existed in the SentencePieceToknizer.cs file into a newly introduced file SentencePieceBaseModel.cs and SentencePieceBpeModel.cs. Most of the code in the newly introduced files are mostly not changed.
  • Added the new Unigram implementation into the SentencePieceUnigramModel.cs file.
  • Changed SentencePieceTokenizer.cs to work with the model abstraction and automatically handle both models Bpe and Unigram.
  • The change shouldn't introduce any behavior change to the existing model (i.e Bpe).
CopilotAI
review requested due to automatic review settings
February 18, 2025 20:10
@tarekgh
tarekgh
commented
Feb 18, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
CC @ericstj@luisquintanilla@LittleLittleCloud@michaelgsharp@stephentoub
CopilotAI
reviewed
Feb 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 11 out of 14 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
  • THIRD-PARTY-NOTICES.TXT: Language not supported
  • eng/Versions.props: Language not supported
  • src/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs: Evaluated as low risk
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Outdated

@tarekghtarekgh
added this to the ML.NET 5.0 milestone
Feb 18, 2025
@codecov
codecovBot
commented
Feb 18, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 75.07347% with 933 lines in your changes missing coverage. Please review.
Project coverage is 68.94%. Comparing base (e3219a9) to head (d244042).

Report is 5 commits behind head on main.
Files with missing linesPatch %Lines
...t.ML.Tokenizers/Model/SentencePieceUnigramModel.cs60.18%293 Missing and 47 partials ⚠️
...osoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs77.43%158 Missing and 32 partials ⚠️
...c/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs82.66%107 Missing and 19 partials ⚠️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04%73 Missing and 32 partials ⚠️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs56.77%61 Missing and 22 partials ⚠️
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs37.77%56 Missing ⚠️
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61%16 Missing and 5 partials ⚠️
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10%6 Missing and 6 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7390 +/- ##
==========================================
+ Coverage 68.88% 68.94% +0.06% 
==========================================
Files 1473 1480 +7 Lines 270814 273273 +2459 Branches 27884 28234 +350 ==========================================
+ Hits 186553 188419 +1866 - Misses 76985 77490 +505 - Partials 7276 7364 +88 
FlagCoverage Δ
Debug68.94% <75.07%> (+0.06%)⬆️
production63.26% <71.73%> (+0.06%)⬆️
test89.46% <97.52%> (+0.06%)⬆️
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing linesCoverage Δ
.../Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs72.76% <ø> (ø)
...rc/Microsoft.ML.Tokenizers/Model/LlamaTokenizer.cs43.75% <ø> (-15.35%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceTokenizer.cs90.00% <ø> (+13.13%)⬆️
...icrosoft.ML.Tokenizers/Utils/Helpers.netcoreapp.cs39.47% <100.00%> (+1.63%)⬆️
...crosoft.ML.Tokenizers.Tests/DoubleArrayTrieTest.cs100.00% <100.00%> (ø)
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10% <97.10%> (ø)
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61% <59.61%> (ø)
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs70.63% <37.77%> (-20.46%)⬇️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs70.32% <56.77%> (-17.95%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04% <79.04%> (ø)
... and 3 more
... and 11 files with indirect coverage changes
ericstj
ericstj
reviewed
Feb 18, 2025
View reviewed changes
@ericstjericstj
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't dig too deep just a few small observations and suggestions.
Comment threadsrc/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs
@ericstj
ericstj
requested a review
from CopilotFebruary 18, 2025 22:09
CopilotAI
reviewed
Feb 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/Microsoft.ML.Tokenizers/Normalizer/SentencePieceNormalizer.cs:392
  • In this loop, calling sp.Slice(1) does not update the variable 'sp', which may result in an infinite loop if the condition remains true. Consider reassigning the sliced span to 'sp' (e.g., sp = sp.Slice(1)).
while (isPrevSpace && sp.Length > 0 && sp[0] == (byte)' ') { sp.Slice(1); }

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs
Outdated

luisquintanilla
luisquintanilla
reviewed
Feb 19, 2025
View reviewed changes
@luisquintanillaluisquintanilla
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo - SentencePieceUingramModel.cs should be SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs
Outdated

ericstj
ericstj
approved these changes
Feb 21, 2025
View reviewed changes
@ericstjericstj
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing feedback. This looks good to me. Please check if @michaelgsharp has feedback too.
michaelgsharp
michaelgsharp
reviewed
Feb 24, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
@build-analysisbuild-analysisBot
mentioned this pull request
Feb 24, 2025

Open

@tarekgh
tarekgh
merged commit 2bd88b9
into
dotnet:mainFeb 25, 2025
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Mar 27, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@luisquintanillaluisquintanillaluisquintanilla left review comments
Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@ericstjericstjericstj approved these changes

+1 more reviewer

@michaelgsharpmichaelgsharpmichaelgsharp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels


None yet


Projects


None yet


Milestone

ML.NET 5.0

Development

Successfully merging this pull request may close these issues.

Implement Sentencepiece Unigram tokenizer


6 participants

@tarekgh@stephentoub@ericstj@luisquintanilla@michaelgsharp

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Introducing SentencePiece Unigram Tokenizer Model - #7390
Merged
tarekgh merged 9 commits into
dotnet:mainfrom 
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Feb 25, 2025
Merged
Introducing SentencePiece Unigram Tokenizer Model#7390
tarekgh merged 9 commits into
dotnet:mainfrom 
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Conversation
@tarekgh
@tarekghtarekgh
commented
Feb 18, 2025

Copy link


Copy Markdown

Member
Fixes#7186
We have been supporting SentencePiece Bpe model for a while, the change here is introducing the support to the SentencePiece Unigram tokenizer model.
Users can create a tokenizer instance by using a code like the following:
// download the tokenizer.model from the LLM model site like the following: // @"https://huggingface.co/sentence-transformers/paraphrase-multilingual-MiniLM-L12-v2/resolve/main/sentencepiece.bpe.model?download=true";usingStreamremoteStream=File.OpenRead(path/to/the/tokenizer/model/file);SentencePieceTokenizertokenizer=SentencePieceTokenizer.Create(remoteStream);
Notes around the change
  • Have done some refactoring code existed in the SentencePieceToknizer.cs file into a newly introduced file SentencePieceBaseModel.cs and SentencePieceBpeModel.cs. Most of the code in the newly introduced files are mostly not changed.
  • Added the new Unigram implementation into the SentencePieceUnigramModel.cs file.
  • Changed SentencePieceTokenizer.cs to work with the model abstraction and automatically handle both models Bpe and Unigram.
  • The change shouldn't introduce any behavior change to the existing model (i.e Bpe).
CopilotAI
review requested due to automatic review settings
February 18, 2025 20:10
@tarekgh
tarekgh
commented
Feb 18, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
CC @ericstj@luisquintanilla@LittleLittleCloud@michaelgsharp@stephentoub
CopilotAI
reviewed
Feb 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 11 out of 14 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
  • THIRD-PARTY-NOTICES.TXT: Language not supported
  • eng/Versions.props: Language not supported
  • src/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs: Evaluated as low risk
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Outdated

@tarekghtarekgh
added this to the ML.NET 5.0 milestone
Feb 18, 2025
@codecov
codecovBot
commented
Feb 18, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 75.07347% with 933 lines in your changes missing coverage. Please review.
Project coverage is 68.94%. Comparing base (e3219a9) to head (d244042).

Report is 5 commits behind head on main.
Files with missing linesPatch %Lines
...t.ML.Tokenizers/Model/SentencePieceUnigramModel.cs60.18%293 Missing and 47 partials ⚠️
...osoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs77.43%158 Missing and 32 partials ⚠️
...c/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs82.66%107 Missing and 19 partials ⚠️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04%73 Missing and 32 partials ⚠️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs56.77%61 Missing and 22 partials ⚠️
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs37.77%56 Missing ⚠️
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61%16 Missing and 5 partials ⚠️
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10%6 Missing and 6 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7390 +/- ##
==========================================
+ Coverage 68.88% 68.94% +0.06% 
==========================================
Files 1473 1480 +7 Lines 270814 273273 +2459 Branches 27884 28234 +350 ==========================================
+ Hits 186553 188419 +1866 - Misses 76985 77490 +505 - Partials 7276 7364 +88 
FlagCoverage Δ
Debug68.94% <75.07%> (+0.06%)⬆️
production63.26% <71.73%> (+0.06%)⬆️
test89.46% <97.52%> (+0.06%)⬆️
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing linesCoverage Δ
.../Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs72.76% <ø> (ø)
...rc/Microsoft.ML.Tokenizers/Model/LlamaTokenizer.cs43.75% <ø> (-15.35%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceTokenizer.cs90.00% <ø> (+13.13%)⬆️
...icrosoft.ML.Tokenizers/Utils/Helpers.netcoreapp.cs39.47% <100.00%> (+1.63%)⬆️
...crosoft.ML.Tokenizers.Tests/DoubleArrayTrieTest.cs100.00% <100.00%> (ø)
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10% <97.10%> (ø)
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61% <59.61%> (ø)
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs70.63% <37.77%> (-20.46%)⬇️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs70.32% <56.77%> (-17.95%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04% <79.04%> (ø)
... and 3 more
... and 11 files with indirect coverage changes
ericstj
ericstj
reviewed
Feb 18, 2025
View reviewed changes
@ericstjericstj
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't dig too deep just a few small observations and suggestions.
Comment threadsrc/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs
@ericstj
ericstj
requested a review
from CopilotFebruary 18, 2025 22:09
CopilotAI
reviewed
Feb 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/Microsoft.ML.Tokenizers/Normalizer/SentencePieceNormalizer.cs:392
  • In this loop, calling sp.Slice(1) does not update the variable 'sp', which may result in an infinite loop if the condition remains true. Consider reassigning the sliced span to 'sp' (e.g., sp = sp.Slice(1)).
while (isPrevSpace && sp.Length > 0 && sp[0] == (byte)' ') { sp.Slice(1); }

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs
Outdated

luisquintanilla
luisquintanilla
reviewed
Feb 19, 2025
View reviewed changes
@luisquintanillaluisquintanilla
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo - SentencePieceUingramModel.cs should be SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs
Outdated

ericstj
ericstj
approved these changes
Feb 21, 2025
View reviewed changes
@ericstjericstj
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing feedback. This looks good to me. Please check if @michaelgsharp has feedback too.
michaelgsharp
michaelgsharp
reviewed
Feb 24, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
@build-analysisbuild-analysisBot
mentioned this pull request
Feb 24, 2025

Open

@tarekgh
tarekgh
merged commit 2bd88b9
into
dotnet:mainFeb 25, 2025
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Mar 27, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@luisquintanillaluisquintanillaluisquintanilla left review comments
Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@ericstjericstjericstj approved these changes

+1 more reviewer

@michaelgsharpmichaelgsharpmichaelgsharp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels


None yet


Projects


None yet


Milestone

ML.NET 5.0

Development

Successfully merging this pull request may close these issues.

Implement Sentencepiece Unigram tokenizer


6 participants

@tarekgh@stephentoub@ericstj@luisquintanilla@michaelgsharp

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Introducing SentencePiece Unigram Tokenizer Model - #7390
Merged
tarekgh merged 9 commits into
dotnet:mainfrom 
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Feb 25, 2025
Merged
Introducing SentencePiece Unigram Tokenizer Model#7390
tarekgh merged 9 commits into
dotnet:mainfrom 
tarekgh:IntroducingSentencePieceUnigramTokenizerModel
Conversation
@tarekgh
@tarekghtarekgh
commented
Feb 18, 2025

Copy link


Copy Markdown

Member
Fixes#7186
We have been supporting SentencePiece Bpe model for a while, the change here is introducing the support to the SentencePiece Unigram tokenizer model.
Users can create a tokenizer instance by using a code like the following:
// download the tokenizer.model from the LLM model site like the following: // @"https://huggingface.co/sentence-transformers/paraphrase-multilingual-MiniLM-L12-v2/resolve/main/sentencepiece.bpe.model?download=true";usingStreamremoteStream=File.OpenRead(path/to/the/tokenizer/model/file);SentencePieceTokenizertokenizer=SentencePieceTokenizer.Create(remoteStream);
Notes around the change
  • Have done some refactoring code existed in the SentencePieceToknizer.cs file into a newly introduced file SentencePieceBaseModel.cs and SentencePieceBpeModel.cs. Most of the code in the newly introduced files are mostly not changed.
  • Added the new Unigram implementation into the SentencePieceUnigramModel.cs file.
  • Changed SentencePieceTokenizer.cs to work with the model abstraction and automatically handle both models Bpe and Unigram.
  • The change shouldn't introduce any behavior change to the existing model (i.e Bpe).
CopilotAI
review requested due to automatic review settings
February 18, 2025 20:10
@tarekgh
tarekgh
commented
Feb 18, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
CC @ericstj@luisquintanilla@LittleLittleCloud@michaelgsharp@stephentoub
CopilotAI
reviewed
Feb 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 11 out of 14 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
  • THIRD-PARTY-NOTICES.TXT: Language not supported
  • eng/Versions.props: Language not supported
  • src/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs: Evaluated as low risk
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Outdated

@tarekghtarekgh
added this to the ML.NET 5.0 milestone
Feb 18, 2025
@codecov
codecovBot
commented
Feb 18, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 75.07347% with 933 lines in your changes missing coverage. Please review.
Project coverage is 68.94%. Comparing base (e3219a9) to head (d244042).

Report is 5 commits behind head on main.
Files with missing linesPatch %Lines
...t.ML.Tokenizers/Model/SentencePieceUnigramModel.cs60.18%293 Missing and 47 partials ⚠️
...osoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs77.43%158 Missing and 32 partials ⚠️
...c/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs82.66%107 Missing and 19 partials ⚠️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04%73 Missing and 32 partials ⚠️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs56.77%61 Missing and 22 partials ⚠️
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs37.77%56 Missing ⚠️
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61%16 Missing and 5 partials ⚠️
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10%6 Missing and 6 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #7390 +/- ##
==========================================
+ Coverage 68.88% 68.94% +0.06% 
==========================================
Files 1473 1480 +7 Lines 270814 273273 +2459 Branches 27884 28234 +350 ==========================================
+ Hits 186553 188419 +1866 - Misses 76985 77490 +505 - Partials 7276 7364 +88 
FlagCoverage Δ
Debug68.94% <75.07%> (+0.06%)⬆️
production63.26% <71.73%> (+0.06%)⬆️
test89.46% <97.52%> (+0.06%)⬆️
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing linesCoverage Δ
.../Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs72.76% <ø> (ø)
...rc/Microsoft.ML.Tokenizers/Model/LlamaTokenizer.cs43.75% <ø> (-15.35%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceTokenizer.cs90.00% <ø> (+13.13%)⬆️
...icrosoft.ML.Tokenizers/Utils/Helpers.netcoreapp.cs39.47% <100.00%> (+1.63%)⬆️
...crosoft.ML.Tokenizers.Tests/DoubleArrayTrieTest.cs100.00% <100.00%> (ø)
test/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs97.10% <97.10%> (ø)
...t.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs59.61% <59.61%> (ø)
src/Microsoft.ML.Tokenizers/Utils/Helpers.cs70.63% <37.77%> (-20.46%)⬇️
...L.Tokenizers/Normalizer/SentencePieceNormalizer.cs70.32% <56.77%> (-17.95%)⬇️
...soft.ML.Tokenizers/Model/SentencePieceBaseModel.cs79.04% <79.04%> (ø)
... and 3 more
... and 11 files with indirect coverage changes
ericstj
ericstj
reviewed
Feb 18, 2025
View reviewed changes
@ericstjericstj
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't dig too deep just a few small observations and suggestions.
Comment threadsrc/Microsoft.ML.Tokenizers/Model/CodeGenTokenizer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/ByteArrayComparer.cs
Outdated

Comment threadsrc/Microsoft.ML.Tokenizers/Utils/OrdinalUtf8StringComparer.cs
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
Comment threadsrc/Microsoft.ML.Tokenizers/Utils/DoubleArrayTrie.cs
@ericstj
ericstj
requested a review
from CopilotFebruary 18, 2025 22:09
CopilotAI
reviewed
Feb 18, 2025
View reviewed changes
CopilotAI
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/Microsoft.ML.Tokenizers/Normalizer/SentencePieceNormalizer.cs:392
  • In this loop, calling sp.Slice(1) does not update the variable 'sp', which may result in an infinite loop if the condition remains true. Consider reassigning the sliced span to 'sp' (e.g., sp = sp.Slice(1)).
while (isPrevSpace && sp.Length > 0 && sp[0] == (byte)' ') { sp.Slice(1); }

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs
Outdated

stephentoub
stephentoub
reviewed
Feb 19, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBpeModel.cs
Outdated

luisquintanilla
luisquintanilla
reviewed
Feb 19, 2025
View reviewed changes
@luisquintanillaluisquintanilla
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo - SentencePieceUingramModel.cs should be SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceUnigramModel.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadtest/Microsoft.ML.Tokenizers.Tests/UnigramTests.cs
ericstj
ericstj
reviewed
Feb 20, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceTokenizer.cs
Outdated

ericstj
ericstj
approved these changes
Feb 21, 2025
View reviewed changes
@ericstjericstj
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing feedback. This looks good to me. Please check if @michaelgsharp has feedback too.
michaelgsharp
michaelgsharp
reviewed
Feb 24, 2025
View reviewed changes
Comment threadsrc/Microsoft.ML.Tokenizers/Model/SentencePieceBaseModel.cs
@build-analysisbuild-analysisBot
mentioned this pull request
Feb 24, 2025

Open

@tarekgh
tarekgh
merged commit 2bd88b9
into
dotnet:mainFeb 25, 2025
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Mar 27, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@luisquintanillaluisquintanillaluisquintanilla left review comments
Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@ericstjericstjericstj approved these changes

+1 more reviewer

@michaelgsharpmichaelgsharpmichaelgsharp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels


None yet


Projects


None yet


Milestone

ML.NET 5.0

Development

Successfully merging this pull request may close these issues.

Implement Sentencepiece Unigram tokenizer


6 participants

@tarekgh@stephentoub@ericstj@luisquintanilla@michaelgsharp