-
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
fix #6949 #6951
fix #6949 #6951
Conversation
Interesting, the addition of machinelearning/src/Microsoft.ML.TorchSharp/NasBert/Models/NasBertEncoder.cs Lines 258 to 264 in d0d8569
Looks like those were meant to refer to this internal class:
Maybe we just rename that @michaelgsharp? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #6951 +/- ##
==========================================
- Coverage 68.80% 68.80% -0.01%
==========================================
Files 1249 1249
Lines 249644 249686 +42
Branches 25481 25485 +4
==========================================
+ Hits 171778 171797 +19
- Misses 71275 71294 +19
- Partials 6591 6595 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@ericstj any classes directly involved in the TorchSharp model itself need to match names exactly with the name that was used to save the weights file. We could try and rename this file and see if things still load, but if its at all saved in the weights file its not a class that we can rename. I'll test that out and add a comment here about it. |
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.
Looks good to me.
/backport to release/3.0 |
Started backporting to release/3.0: https://github.com/dotnet/machinelearning/actions/runs/7481061541 |
How would we be persisting the name of this static class in our saved file? Still might be worth renaming since we try to avoid name collisions in our codebases. |
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.Fix #6949