Loading old model files is broken. (#1290)

* Loading old model files is broken. 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
dotnet · Oct 18, 2018 · 14dadfe · 14dadfe
1 parent 9157cea
commit 14dadfe
Show file tree

Hide file tree

Showing 4 changed files with 82 additions and 12 deletions.
diff --git a/build/Dependencies.props b/build/Dependencies.props
@@ -1,24 +1,44 @@
 <Project>
+
+  <!-- Core Product Dependencies -->
   <PropertyGroup>
-    <GoogleProtobufPackageVersion>3.5.1</GoogleProtobufPackageVersion>
     <NewtonsoftJsonPackageVersion>10.0.3</NewtonsoftJsonPackageVersion>
-    <ParquetDotNetPackageVersion>2.1.3</ParquetDotNetPackageVersion>
-    <SystemThreadingTasksDataflowPackageVersion>4.8.0</SystemThreadingTasksDataflowPackageVersion>
     <SystemCodeDomPackageVersion>4.4.0</SystemCodeDomPackageVersion>
+    <SystemCollectionsImmutableVersion>1.5.0</SystemCollectionsImmutableVersion>
+    <SystemMemoryVersion>4.5.1</SystemMemoryVersion>
     <SystemReflectionEmitLightweightPackageVersion>4.3.0</SystemReflectionEmitLightweightPackageVersion>
-    <PublishSymbolsPackageVersion>1.0.0-beta-62824-02</PublishSymbolsPackageVersion>
+    <SystemThreadingTasksDataflowPackageVersion>4.8.0</SystemThreadingTasksDataflowPackageVersion>
+  </PropertyGroup>
+
+  <!-- Other/Non-Core Product Dependencies -->
+  <PropertyGroup>
+    <GoogleProtobufPackageVersion>3.5.1</GoogleProtobufPackageVersion>
     <LightGBMPackageVersion>2.2.1.1</LightGBMPackageVersion>
+    <MicrosoftMLScoring>1.1.0</MicrosoftMLScoring>
     <MlNetMklDepsPackageVersion>0.0.0.7</MlNetMklDepsPackageVersion>
+    <ParquetDotNetPackageVersion>2.1.3</ParquetDotNetPackageVersion>
     <SystemDrawingCommonPackageVersion>4.5.0</SystemDrawingCommonPackageVersion>
-    <BenchmarkDotNetVersion>0.11.1</BenchmarkDotNetVersion>
+    <SystemIOFileSystemAccessControl>4.5.0</SystemIOFileSystemAccessControl>
+    <SystemSecurityPrincipalWindows>4.5.0</SystemSecurityPrincipalWindows>
     <TensorFlowVersion>1.10.0</TensorFlowVersion>
-    <SystemCollectionsImmutableVersion>1.5.0</SystemCollectionsImmutableVersion>
-    <SystemMemoryVersion>4.5.1</SystemMemoryVersion>
+  </PropertyGroup>
+
+  <!-- Code Analyzer Dependencies -->
+  <PropertyGroup>
     <MicrosoftCodeAnalysisCSharpVersion>2.9.0</MicrosoftCodeAnalysisCSharpVersion>
     <MicrosoftCSharpVersion>4.5.0</MicrosoftCSharpVersion>
     <SystemCompositionVersion>1.2.0</SystemCompositionVersion>
-    <MicrosoftMLScoring>1.1.0</MicrosoftMLScoring>
-    <SystemIOFileSystemAccessControl>4.5.0</SystemIOFileSystemAccessControl>
-    <SystemSecurityPrincipalWindows>4.5.0</SystemSecurityPrincipalWindows>
   </PropertyGroup>
+
+  <!-- Build/infrastructure Dependencies -->
+  <PropertyGroup>
+    <PublishSymbolsPackageVersion>1.0.0-beta-62824-02</PublishSymbolsPackageVersion>
+  </PropertyGroup>
+
+  <!-- Test-only Dependencies -->
+  <PropertyGroup>
+    <BenchmarkDotNetVersion>0.11.1</BenchmarkDotNetVersion>
+    <MicrosoftMLTestModelsPackageVersion>0.0.2-test</MicrosoftMLTestModelsPackageVersion>
+  </PropertyGroup>
+
 </Project>
diff --git a/src/Microsoft.ML.Data/Model/ModelHeader.cs b/src/Microsoft.ML.Data/Model/ModelHeader.cs
@@ -483,6 +483,18 @@ public static bool TryValidate(ref ModelHeader header, BinaryReader reader, long
                 {
                     // No strings.
                     strings = null;
+
+                    if (header.VerWritten < VerAssemblyNameSupported)
+                    {
+                        // Before VerAssemblyNameSupported, if there were no strings in the model,
+                        // validation ended here. Specifically the FpTail checks below were skipped.
+                        // There are earlier versions of models that don't have strings, and 'reader' is
+                        // not at FpTail at this point.
+                        // Preserve the previous behavior by returning early here.
+                        loaderAssemblyName = null;
+                        ex = null;
+                        return true;
+                    }
                 }
                 else
                 {

diff --git a/test/Microsoft.ML.Core.Tests/Microsoft.ML.Core.Tests.csproj b/test/Microsoft.ML.Core.Tests/Microsoft.ML.Core.Tests.csproj
@@ -2,7 +2,7 @@
   <PropertyGroup>
     <DefineConstants>CORECLR</DefineConstants>
   </PropertyGroup>
-  
+
   <ItemGroup>
     <ProjectReference Include="..\..\src\Microsoft.ML.HalLearners\Microsoft.ML.HalLearners.csproj" />
     <ProjectReference Include="..\..\src\Microsoft.ML.Api\Microsoft.ML.Api.csproj" />
@@ -22,7 +22,11 @@
     <ProjectReference Include="..\..\src\Microsoft.ML.Legacy\Microsoft.ML.Legacy.csproj" />
     <ProjectReference Include="..\Microsoft.ML.TestFramework\Microsoft.ML.TestFramework.csproj" />
     <ProjectReference Include="..\Microsoft.ML.Tests\Microsoft.ML.Tests.csproj" />
-	<PackageReference Include="MlNetMklDeps" Version="$(MlNetMklDepsPackageVersion)" />
+  </ItemGroup>
+
+  <ItemGroup>
+    <PackageReference Include="MlNetMklDeps" Version="$(MlNetMklDepsPackageVersion)" />
+    <PackageReference Include="Microsoft.ML.TestModels" Version="$(MicrosoftMLTestModelsPackageVersion)" />
   </ItemGroup>
 
   <ItemGroup>

diff --git a/test/Microsoft.ML.Core.Tests/UnitTests/TestModelLoad.cs b/test/Microsoft.ML.Core.Tests/UnitTests/TestModelLoad.cs
@@ -0,0 +1,34 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using Microsoft.ML.Runtime.Data;
+using Microsoft.ML.Runtime.Model;
+using Microsoft.ML.TestFramework;
+using System.IO;
+using Xunit;
+
+namespace Microsoft.ML.Runtime.RunTests
+{
+    public class TestModelLoad
+    {
+        /// <summary>
+        /// Tests loading a model file that was saved using an older version still loads correctly.
+        /// </summary>
+        [Fact]
+        public void LoadOriginalBinaryLoaderModel()
+        {
+            using (var env = new LocalEnvironment()
+                .AddStandardComponents())
+            using (var modelStream = File.OpenRead(Path.Combine("TestModels", "BinaryLoader-v3.11.0.0.zip")))
+            using (var rep = RepositoryReader.Open(modelStream, env))
+            {
+                IDataLoader result = ModelFileUtils.LoadLoader(env, rep, new MultiFileSource(null), true);
+
+                Assert.Equal(2, result.Schema.ColumnCount);
+                Assert.Equal("Image", result.Schema[0].Name);
+                Assert.Equal("Class", result.Schema[1].Name);
+            }
+        }
+    }
+}