-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #7271 AOT for ML.Tokenizers #7272
Conversation
@dotnet-policy-service agree company="Microsoft" |
@eiriktsarpalis could you please have a quick look at this change. The change is just changing the json deserialization code to use the source generator. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7272 +/- ##
=======================================
Coverage 68.81% 68.81%
=======================================
Files 1461 1461
Lines 272405 272400 -5
Branches 28176 28176
=======================================
Hits 187442 187442
+ Misses 77727 77725 -2
+ Partials 7236 7233 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
src/Microsoft.ML.Tokenizers/Model/ModelSourceGenerationContext.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
Thanks for the review :) |
/ba-g unrelated infrastructure failure. |
Fixes #7271
This PR makes ML.Tokenizers project AOT compatible.
ML.Tokenizers is made to use SourceGenerationContext for deserializing Json.
I had to create a helper class
Vocabulary
in order to register aJsonConverter
for it.Before the change, we have following Aot warnings on the calls to Json.Deserialize:
After the change, warnings are no more :)
Note that
netstandard2.0
framework is not Aot compatible as trimming is only supported for .NET 6 and later. Therefore, in order to test the compatibility for Microsoft.ML.Tokenizers project, you need to set the TargetFramework to net8.0.Then you can add
<PublishAot>true</PublishAot>
.e.g. Microsoft.ML.Tokenizers.csproj
Then building specifically for
Microsoft.ML.Tokenizers
should result in warnings if the project is not AOT compatible.We are excited to review your PR.
So we can do the best job, please check:
Fixes #nnnn
in your description to cause GitHub to automatically close the issue(s) when your PR is merged.