Fixed and Added unit tests for EnsureResourceAsync hanging issue by mstfbl · Pull Request #4943 · dotnet/machinelearning

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

privateconststringDefaultUrl="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

privatestaticstringCheckValidDownload(IChannelch,stringfilePath,stringurl,refbooldeleteNeeded)
{
// 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.
varinfo=newFileInfo(filePath);
if(info.Length>4096)
returnnull;
stringerror=null;
using(varr=newStreamReader(filePath))
{
vartext=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;
returnerror;
}

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 ).

This reverts commit 63b3f33.

…it test file

harishsk

🕐

….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.

harishsk

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

privateconststringDefaultUrl="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

privatestaticstringCheckValidDownload(IChannelch,stringfilePath,stringurl,refbooldeleteNeeded)
{
// 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.
varinfo=newFileInfo(filePath);
if(info.Length>4096)
returnnull;
stringerror=null;
using(varr=newStreamReader(filePath))
{
vartext=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;
returnerror;
}

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 ).

This reverts commit 63b3f33.

…it test file

harishsk

🕐

….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.

harishsk

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

privateconststringDefaultUrl="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

privatestaticstringCheckValidDownload(IChannelch,stringfilePath,stringurl,refbooldeleteNeeded)
{
// 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.
varinfo=newFileInfo(filePath);
if(info.Length>4096)
returnnull;
stringerror=null;
using(varr=newStreamReader(filePath))
{
vartext=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;
returnerror;
}

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 ).

This reverts commit 63b3f33.

…it test file

harishsk

🕐

….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.

harishsk

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

privateconststringDefaultUrl="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

privatestaticstringCheckValidDownload(IChannelch,stringfilePath,stringurl,refbooldeleteNeeded)
{
// 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.
varinfo=newFileInfo(filePath);
if(info.Length>4096)
returnnull;
stringerror=null;
using(varr=newStreamReader(filePath))
{
vartext=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;
returnerror;
}

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 ).

This reverts commit 63b3f33.

…it test file

harishsk

🕐

….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.

harishsk

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

privateconststringDefaultUrl="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

privatestaticstringCheckValidDownload(IChannelch,stringfilePath,stringurl,refbooldeleteNeeded)
{
// 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.
varinfo=newFileInfo(filePath);
if(info.Length>4096)
returnnull;
stringerror=null;
using(varr=newStreamReader(filePath))
{
vartext=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;
returnerror;
}

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 ).

This reverts commit 63b3f33.

…it test file

harishsk

🕐

….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.

harishsk

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

privateconststringDefaultUrl="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

privatestaticstringCheckValidDownload(IChannelch,stringfilePath,stringurl,refbooldeleteNeeded)
{
// 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.
varinfo=newFileInfo(filePath);
if(info.Length>4096)
returnnull;
stringerror=null;
using(varr=newStreamReader(filePath))
{
vartext=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;
returnerror;
}

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 ).

This reverts commit 63b3f33.

…it test file

harishsk

🕐

….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.

harishsk

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

privateconststringDefaultUrl="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

privatestaticstringCheckValidDownload(IChannelch,stringfilePath,stringurl,refbooldeleteNeeded)
{
// 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.
varinfo=newFileInfo(filePath);
if(info.Length>4096)
returnnull;
stringerror=null;
using(varr=newStreamReader(filePath))
{
vartext=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;
returnerror;
}

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 ).

This reverts commit 63b3f33.

…it test file

harishsk

🕐

….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.

harishsk

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

privateconststringDefaultUrl="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

privatestaticstringCheckValidDownload(IChannelch,stringfilePath,stringurl,refbooldeleteNeeded)
{
// 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.
varinfo=newFileInfo(filePath);
if(info.Length>4096)
returnnull;
stringerror=null;
using(varr=newStreamReader(filePath))
{
vartext=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;
returnerror;
}

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 ).

This reverts commit 63b3f33.

…it test file

harishsk

🕐

….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.

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 frank-dong-ms-zz and harishsk March 14, 2020 00:52

justinormont reviewed Mar 14, 2020
View reviewed changes

Comment threadsrc/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs

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

Comment threadsrc/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated

eerhardt reviewed Mar 16, 2020
View reviewed changes

Comment threadsrc/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated

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

Comment threadsrc/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated

justinormont reviewed Mar 16, 2020
View reviewed changes

Comment threadsrc/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated

justinormont reviewed Mar 16, 2020
View reviewed changes

Comment threadtest/Microsoft.ML.Core.Tests/UnitTests/TestUtilities.cs Outdated

Edits, added 5 min timeout, and debugging requested url
2c4d22e

justinormont reviewed Mar 17, 2020
View reviewed changes

Comment threadsrc/Microsoft.ML.TensorFlow/TensorflowUtils.cs Outdated

justinormont reviewed Mar 17, 2020
View reviewed changes

Comment threadsrc/Microsoft.ML.Transforms/Text/WordEmbeddingsExtractor.cs Outdated

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

Comment threadtest/Microsoft.ML.Core.Tests/UnitTests/TestResourceDownload.cs Outdated

justinormont reviewed Mar 17, 2020
View reviewed changes

Comment threadsrc/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated

justinormont reviewed Mar 17, 2020
View reviewed changes

Comment threadsrc/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated

justinormont reviewed Mar 17, 2020
View reviewed changes

Comment threadtest/Microsoft.ML.Core.Tests/UnitTests/TestResourceDownload.cs

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

Comment threadsrc/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated

harishsk reviewed Mar 18, 2020
View reviewed changes

Comment threadsrc/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated

harishsk reviewed Mar 18, 2020
View reviewed changes

Comment threadsrc/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated

harishsk reviewed Mar 18, 2020
View reviewed changes

Comment threadsrc/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated

harishsk reviewed Mar 18, 2020
View reviewed changes

Comment threadsrc/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated

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

Comment threadsrc/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs Outdated

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

Comment threadsrc/Microsoft.ML.Core/Utilities/ResourceManagerUtils.cs

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 eerhardt, harishsk and yaeldekel 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 issueMar 24, 2020

harishsk reviewed Mar 25, 2020
View reviewed changes

Comment threadsrc/Microsoft.ML.Transforms/Text/WordEmbeddingsExtractor.cs

harishsk approved these changes Mar 25, 2020
View reviewed changes

mstfbl merged commit 22c7ac8 into dotnet:masterMar 25, 2020

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