Fixed and Added unit tests for EnsureResourceAsync hanging issue #4943

mstfbl · 2020-03-13T23:33:31Z

Fixed EnsureResourceAsync by removing the helper function ResourceManagerUtils.CheckValidDownload and by using WebClientResponseUri (an extension of WebClient). Also added unit test for EnsureResourceAsync.

ResourceManagerUtils.EnsureResourceAsync(...) appends the relative URL of a resource to the $MlNetResourcesUrl environment variable (equal to "https://aka.ms/mlnet-resources/" by default).

machinelearning/src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs

Line 34 in cdb1e4b

private const string DefaultUrl = "https://aka.ms/mlnet-resources/";

For example, this occurs in the unit test WordEmbeddingsTest.TestWordEmbeddings(), where sentimend.emd is downloaded from https://aka.ms/mlnet-resources/Text/Sswe/sentiment.emd .

There is a possibility of this combined URL not correctly pointing to a resource, which is why this CheckValidDownload function exists:

machinelearning/src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs

Lines 168 to 184 in cdb1e4b

    
           private static string CheckValidDownload(IChannel ch, string filePath, string url, ref bool deleteNeeded) 
        
           { 
        
               // If the relative url does not exist, aka.ms redirects to www.microsoft.com. Make sure this did not happen. 
        
               // If the file is big then it is definitely not the redirect. 
        
               var info = new FileInfo(filePath); 
        
               if (info.Length > 4096) 
        
                   return null; 
        
               string error = null; 
        
               using (var r = new StreamReader(filePath)) 
        
               { 
        
                   var text = r.ReadToEnd(); 
        
                   if (text.Contains("<head>") && text.Contains("<body>") && text.Contains("microsoft.com")) 
        
                       error = $"The url '{url}' does not exist. Url was redirected to www.microsoft.com."; 
        
               } 
        
               deleteNeeded = error != null; 
        
               return error; 
        
           }

This function was not correctly warning that the HTML of www.microsoft.com was downloaded (instead of an intended data file), as the size of microsoft.com is bigger than 4 kilobytes, and the site should have the sub-strings <head and <body (not and ).

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs

This reverts commit 63b3f33.

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs

test/Microsoft.ML.Core.Tests/UnitTests/TestUtilities.cs

src/Microsoft.ML.TensorFlow/TensorflowUtils.cs

src/Microsoft.ML.Transforms/Text/WordEmbeddingsExtractor.cs

…it test file

test/Microsoft.ML.Core.Tests/UnitTests/TestResourceDownload.cs

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs

test/Microsoft.ML.Core.Tests/UnitTests/TestResourceDownload.cs

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs

harishsk

🕐

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs

….ms"

mstfbl · 2020-03-24T08:10:40Z

I have added a check to ensure only resources downloaded from the "aka.ms" host can use EnsureResourceAsync(). I have also edited the unit test for EnsureResourceAsync() to reflect this.

src/Microsoft.ML.Transforms/Text/WordEmbeddingsExtractor.cs

harishsk

Update ResourceManagerUtils.cs

fedd23c

mstfbl requested a review from justinormont March 13, 2020 23:33

mstfbl requested a review from a team as a code owner March 13, 2020 23:33

mstfbl requested review from harishsk and frank-dong-ms-zz March 14, 2020 00:52

justinormont reviewed Mar 14, 2020

View reviewed changes

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Show resolved Hide resolved

mstfbl added 6 commits March 15, 2020 18:49

Added TestDownloadFromLocal

85f10af

Added TestDownloadError

63b3f33

Revert "Added TestDownloadError"

e16b7d6

This reverts commit 63b3f33.

Edit EnsureResourceAsync and its dependencies

2caf810

Edited TestDownloadFromLocal and re-added TestDownloadError()

6e05a87

Disabling TestDownloadFromLocal and TestDownloadError

69b9827

eerhardt reviewed Mar 16, 2020

View reviewed changes

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated Show resolved Hide resolved

eerhardt reviewed Mar 16, 2020

View reviewed changes

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated Show resolved Hide resolved

mstfbl added 2 commits March 16, 2020 11:23

Edits

6e5b246

Re-activated TestDownloadError and TestDownloadFromLocal

cd56549

justinormont reviewed Mar 16, 2020

View reviewed changes

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated Show resolved Hide resolved

justinormont reviewed Mar 16, 2020

View reviewed changes

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated Show resolved Hide resolved

justinormont reviewed Mar 16, 2020

View reviewed changes

test/Microsoft.ML.Core.Tests/UnitTests/TestUtilities.cs Outdated Show resolved Hide resolved

Edits, added 5 min timeout, and debugging requested url

2c4d22e

justinormont reviewed Mar 17, 2020

View reviewed changes

src/Microsoft.ML.TensorFlow/TensorflowUtils.cs Outdated Show resolved Hide resolved

justinormont reviewed Mar 17, 2020

View reviewed changes

src/Microsoft.ML.Transforms/Text/WordEmbeddingsExtractor.cs Outdated Show resolved Hide resolved

mstfbl mentioned this pull request Mar 17, 2020

Added working version of checking whether file is available for access #4938

Merged

Removed timeouts, and re-added Resource download tests in separate un…

2f67666

…it test file

justinormont reviewed Mar 17, 2020

View reviewed changes

test/Microsoft.ML.Core.Tests/UnitTests/TestResourceDownload.cs Outdated Show resolved Hide resolved

justinormont reviewed Mar 17, 2020

View reviewed changes

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated Show resolved Hide resolved

justinormont reviewed Mar 17, 2020

View reviewed changes

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated Show resolved Hide resolved

justinormont reviewed Mar 17, 2020

View reviewed changes

test/Microsoft.ML.Core.Tests/UnitTests/TestResourceDownload.cs Show resolved Hide resolved

Edits

8bf03c8

mstfbl changed the title ~~Fixed CheckValidDownload in ResourceManagerUtils.cs~~ Fixed and Added unit tests for DownloadFromUrlWithRetryAsync hanging issue Mar 17, 2020

harishsk reviewed Mar 18, 2020

View reviewed changes

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated Show resolved Hide resolved

harishsk reviewed Mar 18, 2020

View reviewed changes

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated Show resolved Hide resolved

harishsk reviewed Mar 18, 2020

View reviewed changes

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated Show resolved Hide resolved

harishsk reviewed Mar 18, 2020

View reviewed changes

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated Show resolved Hide resolved

harishsk reviewed Mar 18, 2020

View reviewed changes

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated Show resolved Hide resolved

harishsk suggested changes Mar 18, 2020

View reviewed changes

Edits for reviews, removing hardcodings of status codes

95514c4

harishsk reviewed Mar 18, 2020

View reviewed changes

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated Show resolved Hide resolved

Removing paranthesis from one-liner if statement

93b5454

mstfbl requested review from harishsk and justinormont March 18, 2020 07:16

mstfbl added 3 commits March 18, 2020 01:26

Update TestResourceDownload.cs

a54c7e0

Update TestResourceDownload.cs

b8d5094

Nit fix + test case fixes

38fc48f

harishsk reviewed Mar 18, 2020

View reviewed changes

src/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated Show resolved Hide resolved

mstfbl added 4 commits March 18, 2020 14:25

Update ResourceManagerUtils.cs

666d328

Update ResourceManagerUtils.cs

a9e1b5d

Update ResourceManagerUtils.cs

d460db7

Update ResourceManagerUtils.cs

ed3c6fc

mstfbl requested review from harishsk, yaeldekel and eerhardt March 19, 2020 05:11

mstfbl added 2 commits March 23, 2020 17:13

Added checking for the host of the download absoluteURL euqaling "aka…

d7b43ed

….ms"

Edit TestResourceDownload

d9cdc07

mstfbl changed the title ~~Fixed and Added unit tests for DownloadFromUrlWithRetryAsync hanging issue~~ Fixed and Added unit tests for EnsureResourceAsync hanging issue Mar 24, 2020

harishsk reviewed Mar 25, 2020

View reviewed changes

src/Microsoft.ML.Transforms/Text/WordEmbeddingsExtractor.cs Show resolved Hide resolved

harishsk approved these changes Mar 25, 2020

View reviewed changes

mstfbl merged commit 22c7ac8 into dotnet:master Mar 25, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed and Added unit tests for EnsureResourceAsync hanging issue #4943

Fixed and Added unit tests for EnsureResourceAsync hanging issue #4943

mstfbl commented Mar 13, 2020 •

edited

Loading

harishsk left a comment

mstfbl commented Mar 24, 2020

harishsk left a comment

	private static string CheckValidDownload(IChannel ch, string filePath, string url, ref bool deleteNeeded)
	{
	// If the relative url does not exist, aka.ms redirects to www.microsoft.com. Make sure this did not happen.
	// If the file is big then it is definitely not the redirect.
	var info = new FileInfo(filePath);
	if (info.Length > 4096)
	return null;
	string error = null;
	using (var r = new StreamReader(filePath))
	{
	var text = r.ReadToEnd();
	if (text.Contains("<head>") && text.Contains("<body>") && text.Contains("microsoft.com"))
	error = $"The url '{url}' does not exist. Url was redirected to www.microsoft.com.";
	}
	deleteNeeded = error != null;
	return error;
	}

Fixed and Added unit tests for EnsureResourceAsync hanging issue #4943

Fixed and Added unit tests for EnsureResourceAsync hanging issue #4943

Conversation

mstfbl commented Mar 13, 2020 • edited Loading

harishsk left a comment

Choose a reason for hiding this comment

mstfbl commented Mar 24, 2020

harishsk left a comment

Choose a reason for hiding this comment

mstfbl commented Mar 13, 2020 •

edited

Loading