Skip to content

Tweak CreateByModelNameAsync - #7015

Merged
tarekgh merged 1 commit into
dotnet:mainfrom
stephentoub:tweakcreate
Feb 20, 2024
Merged

Tweak CreateByModelNameAsync#7015
tarekgh merged 1 commit into
dotnet:mainfrom
stephentoub:tweakcreate

Conversation

@stephentoub

Copy link
Copy Markdown
Member
  • 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

cc: @tarekgh

- 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

codecov Bot commented Feb 20, 2024

Copy link
Copy Markdown

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (f976424) 68.81% compared to head (2eadb5b) 68.80%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7015      +/-   ##
==========================================
- Coverage   68.81%   68.80%   -0.01%     
==========================================
  Files        1258     1258              
  Lines      250643   250662      +19     
  Branches    25606    25612       +6     
==========================================
- Hits       172479   172474       -5     
- Misses      71540    71555      +15     
- Partials     6624     6633       +9     
Flag Coverage Δ
Debug 68.80% <58.06%> (-0.01%) ⬇️
production 63.27% <58.06%> (-0.01%) ⬇️
test 88.44% <ø> (+<0.01%) ⬆️

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

Files Coverage Δ
...crosoft.ML.Tokenizers/Utils/Helpers.netstandard.cs 75.00% <100.00%> (+15.00%) ⬆️
src/Microsoft.ML.Tokenizers/Model/Tiktoken.cs 54.08% <33.33%> (-1.48%) ⬇️
src/Microsoft.ML.Tokenizers/Tokenizer.cs 82.64% <42.10%> (-0.97%) ⬇️

... and 7 files with indirect coverage changes

@tarekgh tarekgh 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.

LGTM. Thanks for the additions.

@tarekgh
tarekgh merged commit 2c9f775 into dotnet:main Feb 20, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

2 participants