Prototype using spans in Model - #7018
Closed
stephentoub wants to merge 2 commits into
Closed
Conversation
- Add a CancellationToken to CreateByModelNameAsync, allowing the download and parsing to be canceled. - Use ReadLineAsync(cancellationToken), which not only allows it to be canceled, but avoids ~100K task allocations - Fix Helpers.FromBase64String to support lines longer than 300 chars
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7018 +/- ##
=======================================
Coverage 68.81% 68.81%
=======================================
Files 1258 1259 +1
Lines 250643 250665 +22
Branches 25606 25608 +2
=======================================
+ Hits 172479 172501 +22
+ Misses 71540 71534 -6
- Partials 6624 6630 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Member
|
Closing this in favor of the following: #7035 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@tarekgh, this isn't for merging, but it shows appx what I had in mind for incorporating spans into Model (I know you're currently revising the surface area, so take this with a grain of salt). This eliminates a majority of the remaining allocation that occurs when using Tokenizer.CountTokens/EncodeToIds, as it avoids allocating strings for each token that's already in the cache.
Feel free to crib liberally from the second commit and close this PR. Ignore the first commit, which I submitted separately.