Loading old model files is broken. #1290
Conversation
| { | ||
| using (var env = new LocalEnvironment() | ||
| .AddStandardComponents()) | ||
| using (var modelStream = File.OpenRead("OriginalVersionBinaryLoader.zip")) |
There was a problem hiding this comment.
OriginalVersionBinaryLoader [](start = 52, length = 27)
Do we have this file in repo?
There was a problem hiding this comment.
It comes through the TestModels NuGet package I added.
There was a problem hiding this comment.
Is this sustainable? Suppose Scott wants to add a couple more models to test back-compat. How can he do this?
In reply to: 226122081 [](ancestors = 226122081)
There was a problem hiding this comment.
Yes, it is sustainable. I am following the same pattern we use for ONNX and TensorFlow test models.
You check the model file into a folder in https://github.com/dotnet/machinelearning-testdata. For example, if Scott has more ML.NET model files, he adds them under https://github.com/dotnet/machinelearning-testdata/tree/master/Microsoft.ML.TestModels, sends a PR and merges. Then someone builds the NuGet package from that repo, and uploads it to our NuGet feed: https://dotnet.myget.org/feed/dotnet-core/package/nuget/Microsoft.ML.TestModels.
The NuGet packages are versioned, so as new versions are created (with new or updated model files), we can just bump the version number in this repo, and start using the new models in the test code.
Older model files are failing to load due to FpTail checks in the model validation code. These checks weren't happening in the old model loading code, and they now fail on some older models. Fix this by returning early when it is an older model, and there are no strings. This preserves the old behavior. Fix dotnet#1289
eerhardt
force-pushed
the
FixModelLoadBug
branch
from
3fe66fb to
d5407f7
Compare
| { | ||
| using (var env = new LocalEnvironment() | ||
| .AddStandardComponents()) | ||
| using (var modelStream = File.OpenRead("OriginalVersionBinaryLoader.zip")) |
There was a problem hiding this comment.
I might recommend naming this model w/ the version number which created it. Saying 'OriginalVersion' makes me think we'll have a plethora of non-comparable names like 'NewVersion', and 'NewVersionAfterUpdate'.
| using (var modelStream = File.OpenRead("OriginalVersionBinaryLoader.zip")) | |
| using (var modelStream = File.OpenRead("BinaryLoader-v3.11.0.0.zip")) |
There was a problem hiding this comment.
Good idea. I need to update the TestModels NuGet package, which I'm doing here: dotnet/machinelearning-testdata#8.
Once that is approved, merged, and published, I will update this PR.
|
Closing & reopening to restart CI tests. Failure spot was on a nuget download for OSX both debug & release: |
ghost
locked as resolved and limited conversation to collaborators
Older model files are failing to load due to FpTail checks in the model validation code. These checks weren't happening in the old model loading code, and they now fail on some older models.
Fix this by returning early when it is an older model, and there are no strings. This preserves the old behavior.
Fix #1289