Use resource manager to download meta files. Fixes #4234 #4314

ashbhandare · 2019-10-08T21:06:16Z

Previously, when the ImageClassification pipeline is run for the first time, the meta graph of the model (ResnetV2101 or InceptionV3) is downloaded, and in the subsequent runs, it is reused. If the run is interrupted while the download is in progress(eg.: by stopping), the protobuff is partially downloaded. This throws an error when this incomplete graph is attempted to be read in the subsequent runs.

This changes uses resource manager to download the meta file. Resource Manager downloads the file to a temporary file path(GUID) and then atomically moves the temporary file path to the actual path. Hence once a file path is created it is guaranteed to have all the contents (unless the user or someone goes and manipulates the file)

Fixes #4234

codecov · 2019-10-09T00:45:37Z

Codecov Report

❗ No coverage uploaded for pull request base (master@46f9a1f). Click here to learn what that means.
The diff coverage is 57.69%.

@@            Coverage Diff            @@
##             master    #4314   +/-   ##
=========================================
  Coverage          ?   74.59%           
=========================================
  Files             ?      878           
  Lines             ?   154279           
  Branches          ?    16876           
=========================================
  Hits              ?   115078           
  Misses            ?    34448           
  Partials          ?     4753

Flag	Coverage Δ
#Debug	`74.59% <57.69%> (?)`
#production	`70.18% <57.69%> (?)`
#test	`89.53% <ø> (?)`

Impacted Files	Coverage Δ
src/Microsoft.ML.Dnn/DnnCatalog.cs	`82.81% <25%> (ø)`
src/Microsoft.ML.Dnn/DnnUtils.cs	`67.21% <85.71%> (ø)`
#Resolved

eerhardt · 2019-10-09T14:24:16Z

src/Microsoft.ML.Dnn/DnnUtils.cs

@@ -89,6 +90,24 @@ internal static Session LoadTFSession(IExceptionContext ectx, byte[] modelBytes,
            }
            return new Session(graph);
        }
+        internal static void MaybeDownloadFile(Uri address, string fileName)
+        {
+            using (WebClient client = new WebClient())


See the remarks on https://docs.microsoft.com/en-us/dotnet/api/system.net.webclient

Important
We don't recommend that you use the WebClient class for new development. Instead, use the System.Net.Http.HttpClient class. #Resolved

src/Microsoft.ML.Dnn/DnnCatalog.cs

src/Microsoft.ML.Dnn/DnnUtils.cs

src/Microsoft.ML.Dnn/DnnCatalog.cs

codemzs · 2019-10-14T00:10:03Z

src/Microsoft.ML.Dnn/DnnUtils.cs

+                        File.Delete(fileName);
+                        var response = client.GetAsync(address).Result;
+                        using FileStream fileStream = new FileStream(fileName, FileMode.Create, FileAccess.Write, FileShare.None);
+                            using Stream contentStream = response.Content.ReadAsStreamAsync().Result;


using Stream contentStream = response.Content.ReadAsStreamAsync().Result; [](start = 28, length = 73)

align this line with 110, this is the pattern we follow, this isn't some loop :) #Resolved

codemzs · 2019-10-14T00:10:22Z

src/Microsoft.ML.Dnn/DnnUtils.cs

+                {
+                    var response = client.GetAsync(address).Result;
+                    using FileStream fileStream = new FileStream(fileName, FileMode.Create, FileAccess.Write, FileShare.None);
+                        using Stream contentStream = response.Content.ReadAsStreamAsync().Result;


using Stream contentStream = response.Content.ReadAsStreamAsync().Result; [](start = 24, length = 73)

align this line with 118 #Resolved

codemzs · 2019-10-14T00:12:01Z

test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs

+                    // Just by changing/selecting InceptionV3 here instead of 
+                    // ResnetV2101 you can try a different architecture/pre-trained 
+                    // model. 
+                    arch: ImageClassificationEstimator.Architecture.ResnetV2101,


ImageClassificationEstimator.Architecture.ResnetV2101, [](start = 25, length = 55)

please add another test for InceptionV3, also there is a way to run test twice using "theory" with different arguments. See https://stackoverflow.com/questions/22373258/difference-between-fact-and-theory-xunit-net #Resolved

codemzs · 2019-10-14T00:12:56Z

@ashbhandare Change looks great, just some minor comments and thanks for adding this tests that covers the scenario user was hitting even though it is a negative test but important one! #Resolved

codemzs · 2019-10-17T14:20:23Z

@ashbhandare Seems like the error is happening because of the new test you added for InceptionV3. Please note inception V3 downloads meta file and also checkpoint files. Have you figured out why this test is failing?

[xUnit.net 00:06:50.17] Microsoft.ML.Scenarios.TensorFlowScenariosTests.TensorflowRedownloadModelFile(arch: InceptionV3) [FAIL]
X Microsoft.ML.Scenarios.TensorFlowScenariosTests.TensorflowRedownloadModelFile(arch: InceptionV3) [10s 5ms]
Error Message:
System.AccessViolationException : Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
Stack Trace:
at Tensorflow.c_api.TF_TensorData(IntPtr tensor)

ashbhandare · 2019-10-18T21:38:04Z

This error cannot be consistently reproduced. I am working on finding the cause.

In reply to: 543197416 [](ancestors = 543197416)

codemzs

codemzs · 2019-10-27T23:45:17Z

@ashbhandare is your branch synced to master? It seems some builds are failing because it cannot find LoadImages with 4 arguments.

codemzs · 2019-10-27T23:57:46Z

@ashbhandare I have pushed commits into your branch to sync it with master, resolve build errors and fix some issues around disposing objects and waiting for task to finish. Just giving you heads up ...

…ile is not of expected size.

codemzs · 2019-10-28T02:05:05Z

@ashbhandare I realized we have a resource manager util function that is designed for this kind of scenario. It downloads the file to a temporary file path and then moves the file to the actual file path atomically (if the download was successful) hence when a file path is created it is guaranteed to have all the contents. I have updated the code to use that util function and also created short urls for all our meta files, this is useful incase we wanted to change the directed URL down the road without having to change the code.

eerhardt · 2019-10-28T15:44:21Z

src/Microsoft.ML.Dnn/DnnUtils.cs

-                }
-
+                DownloadIfNeeded(env, @"tfhub_modules.zip",currentDirectory,@"tfhub_modules.zip",timeout);
+                if (!Directory.Exists(@"tfhub_modules"))


Why download the .zip file, but then decide afterwards not to use it?

Shouldn't you check if (!Directory.Exists(@"tfhub_modules")) before downloaded the .zip file?

yea ok 👍🏽 i’ll fix that

…t#4314) * Add MaybeDownloadFile(), where check to redownload existing file if file is not of expected size. * Changed WebClient to HttpClient, renamed function to DownloadIfNeeded. * Added unit test. * Fixed newline after test * Removed asynchronous copy * added test for InceptionV3, fixed formatting. * Modify to call DownloadIfNeeded * fixed unit test, minor formatting * fix test and change after rebase * sync to master and use LoadRawImagesBytes instead of LoadImages. * Dispose HttpClient object and wait for task for finish. * clean up. * Use resource manager to download meta files. * remove test. * remove unused namespaces.

ashbhandare requested a review from a team as a code owner October 8, 2019 21:06

ashbhandare changed the title ~~WIP: Add functionality for re-downloading incomplete/corrupt pre-trained model files. Fixes #4234~~ WIP: Add functionality for re-downloading incomplete pre-trained model files. Fixes #4234 Oct 8, 2019

eerhardt reviewed Oct 9, 2019

View reviewed changes

codemzs reviewed Oct 9, 2019

View reviewed changes

src/Microsoft.ML.Dnn/DnnCatalog.cs Outdated Show resolved Hide resolved

codemzs reviewed Oct 9, 2019

View reviewed changes

src/Microsoft.ML.Dnn/DnnUtils.cs Outdated Show resolved Hide resolved

codemzs reviewed Oct 9, 2019

View reviewed changes

src/Microsoft.ML.Dnn/DnnUtils.cs Outdated Show resolved Hide resolved

ashbhandare changed the title ~~WIP: Add functionality for re-downloading incomplete pre-trained model files. Fixes #4234~~ Add functionality for re-downloading incomplete pre-trained model files. Fixes #4234 Oct 10, 2019

codemzs reviewed Oct 14, 2019

View reviewed changes

src/Microsoft.ML.Dnn/DnnCatalog.cs Outdated Show resolved Hide resolved

codemzs reviewed Oct 14, 2019

View reviewed changes

ashbhandare mentioned this pull request Oct 22, 2019

Redesign DnnCatalog methods API for ease of use and consistency. #4362

Merged

ashbhandare force-pushed the Issue4234 branch 2 times, most recently from d2e1dcb to 78ebb74 Compare October 25, 2019 23:52

codemzs approved these changes Oct 27, 2019

View reviewed changes

ashbhandare added 9 commits October 27, 2019 17:53

Add MaybeDownloadFile(), where check to redownload existing file if f…

3af04ad

…ile is not of expected size.

Changed WebClient to HttpClient, renamed function to DownloadIfNeeded.

22c414a

Added unit test.

728dc92

Fixed newline after test

6aea270

Removed asynchronous copy

95e37eb

added test for InceptionV3, fixed formatting.

c59067a

Modify to call DownloadIfNeeded

a9163e4

fixed unit test, minor formatting

090346c

fix test and change after rebase

34ba676

codemzs added 3 commits October 27, 2019 17:53

sync to master and use LoadRawImagesBytes instead of LoadImages.

81dba73

Dispose HttpClient object and wait for task for finish.

33a9d6e

clean up.

406ef49

codemzs force-pushed the Issue4234 branch from d9576fc to 406ef49 Compare October 28, 2019 00:53

codemzs added 2 commits October 27, 2019 18:54

Use resource manager to download meta files.

e4e53ec

remove test.

0124b71

codemzs changed the title ~~Add functionality for re-downloading incomplete pre-trained model files. Fixes #4234~~ Use resource manager to download meta files. Fixes #4234 Oct 28, 2019

remove unused namespaces.

e9f63f2

codemzs merged commit dfa7a52 into dotnet:master Oct 28, 2019

eerhardt reviewed Oct 28, 2019

View reviewed changes

ashbhandare deleted the Issue4234 branch October 28, 2019 18:26

ghost locked as resolved and limited conversation to collaborators Mar 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use resource manager to download meta files. Fixes #4234 #4314

Use resource manager to download meta files. Fixes #4234 #4314

ashbhandare commented Oct 8, 2019 •

edited by codemzs

Loading

codecov bot commented Oct 9, 2019 •

edited by codemzs

Loading

eerhardt Oct 9, 2019 •

edited by ashbhandare

Loading

codemzs Oct 14, 2019 •

edited by ashbhandare

Loading

codemzs Oct 14, 2019 •

edited by ashbhandare

Loading

codemzs Oct 14, 2019 •

edited by ashbhandare

Loading

codemzs commented Oct 14, 2019 •

edited by ashbhandare

Loading

codemzs commented Oct 17, 2019

ashbhandare commented Oct 18, 2019

codemzs left a comment

codemzs commented Oct 27, 2019

codemzs commented Oct 27, 2019 •

edited

Loading

codemzs commented Oct 28, 2019 •

edited

Loading

eerhardt Oct 28, 2019

codemzs Oct 28, 2019

Use resource manager to download meta files. Fixes #4234 #4314

Use resource manager to download meta files. Fixes #4234 #4314

Conversation

ashbhandare commented Oct 8, 2019 • edited by codemzs Loading

codecov bot commented Oct 9, 2019 • edited by codemzs Loading

Codecov Report

eerhardt Oct 9, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

codemzs Oct 14, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

codemzs Oct 14, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

codemzs Oct 14, 2019 • edited by ashbhandare Loading

Choose a reason for hiding this comment

codemzs commented Oct 14, 2019 • edited by ashbhandare Loading

codemzs commented Oct 17, 2019

ashbhandare commented Oct 18, 2019

codemzs left a comment

Choose a reason for hiding this comment

codemzs commented Oct 27, 2019

codemzs commented Oct 27, 2019 • edited Loading

codemzs commented Oct 28, 2019 • edited Loading

eerhardt Oct 28, 2019

Choose a reason for hiding this comment

codemzs Oct 28, 2019

Choose a reason for hiding this comment

ashbhandare commented Oct 8, 2019 •

edited by codemzs

Loading

codecov bot commented Oct 9, 2019 •

edited by codemzs

Loading

eerhardt Oct 9, 2019 •

edited by ashbhandare

Loading

codemzs Oct 14, 2019 •

edited by ashbhandare

Loading

codemzs Oct 14, 2019 •

edited by ashbhandare

Loading

codemzs Oct 14, 2019 •

edited by ashbhandare

Loading

codemzs commented Oct 14, 2019 •

edited by ashbhandare

Loading

codemzs commented Oct 27, 2019 •

edited

Loading

codemzs commented Oct 28, 2019 •

edited

Loading