Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pin hash in DownloadTensorFlowSentimentModel() #4951

Merged
merged 1 commit into from Mar 19, 2020

Conversation

@justinormont
Copy link
Member

justinormont commented Mar 18, 2020

Changes DownloadTensorFlowSentimentModel() to download from a specific commit ID instead of master of the dotnet/machinelearning-testdata repo.

Otherwise older versions of the ML.NET code will break when the remote repo is reorganized, or the files renamed.

Further explanation in comment below: #4951 (comment)

@justinormont justinormont requested a review from dotnet/mlnet-core as a code owner Mar 18, 2020
@mstfbl
mstfbl approved these changes Mar 19, 2020
Copy link
Member

mstfbl left a comment

:shipit:

@harishsk

This comment has been minimized.

Copy link
Member

harishsk commented Mar 19, 2020

Sorry, I am not sure I follow. Can you please explain what you mean by "Otherwise older versions of the code will break when the repo is reorganized, or the files renamed." ?

/// </remarks>
public static string DownloadTensorFlowSentimentModel()
{
string remotePath = "https://github.com/dotnet/machinelearning-testdata/raw/master/Microsoft.ML.TensorFlow.TestModels/sentiment_model/";
string remotePath = "https://github.com/dotnet/machinelearning-testdata/raw/296625f4e49d50fcd6a48a0d92bea7584e198c0f/Microsoft.ML.TensorFlow.TestModels/sentiment_model/";

This comment has been minimized.

Copy link
@justinormont

justinormont Mar 19, 2020

Author Member

(tagged this on a line of code to allow for conversation)

@harishsk commented 2020-03-19T17:43:02Z

Sorry, I am not sure I follow. Can you please explain what you mean by "Otherwise older versions of the code will break when the repo is reorganized, or the files renamed." ?

The code here grabs a file from https://github.com/dotnet/machinelearning-testdata/raw/master/Microsoft.ML.TensorFlow.TestModels/sentiment_model/imdb_word_index.csv.

This is a link to master in another repo (dotnet/machinelearning-testdata).

Let's say in year, someone is still using the current version of ML.NET. When that version grabs the linked file, it will still use that same URL. But in the mean time, the master branch of that repo will have changed, and perhaps breaking that link. For instance if they decide to re-organize the datasets in the repo, the URL will no longer work.

Now if we link to a specific commit in that repo, the changes to master (eg. them reorganizing the datasets in the repo) will not break the commit-pinned link this PR uses.

This comment has been minimized.

Copy link
@harishsk

harishsk Mar 19, 2020

Member

Thanks! That make sense.

@justinormont justinormont merged commit 9982416 into dotnet:master Mar 19, 2020
17 checks passed
17 checks passed
MachineLearning-CI Build #20200318.2 succeeded
Details
MachineLearning-CI (Centos_x64_NetCoreApp30 Debug_Build) Centos_x64_NetCoreApp30 Debug_Build succeeded
Details
MachineLearning-CI (Centos_x64_NetCoreApp30 Release_Build) Centos_x64_NetCoreApp30 Release_Build succeeded
Details
MachineLearning-CI (MacOS_x64_NetCoreApp21 Debug_Build) MacOS_x64_NetCoreApp21 Debug_Build succeeded
Details
MachineLearning-CI (MacOS_x64_NetCoreApp21 Release_Build) MacOS_x64_NetCoreApp21 Release_Build succeeded
Details
MachineLearning-CI (Ubuntu_x64_NetCoreApp21 Debug_Build) Ubuntu_x64_NetCoreApp21 Debug_Build succeeded
Details
MachineLearning-CI (Ubuntu_x64_NetCoreApp21 Release_Build) Ubuntu_x64_NetCoreApp21 Release_Build succeeded
Details
MachineLearning-CI (Windows_x64_NetCoreApp21 Debug_Build) Windows_x64_NetCoreApp21 Debug_Build succeeded
Details
MachineLearning-CI (Windows_x64_NetCoreApp21 Release_Build) Windows_x64_NetCoreApp21 Release_Build succeeded
Details
MachineLearning-CI (Windows_x64_NetCoreApp30 Debug_Build) Windows_x64_NetCoreApp30 Debug_Build succeeded
Details
MachineLearning-CI (Windows_x64_NetCoreApp30 Release_Build) Windows_x64_NetCoreApp30 Release_Build succeeded
Details
MachineLearning-CI (Windows_x64_NetFx461 Debug_Build) Windows_x64_NetFx461 Debug_Build succeeded
Details
MachineLearning-CI (Windows_x64_NetFx461 Release_Build) Windows_x64_NetFx461 Release_Build succeeded
Details
MachineLearning-CI (Windows_x86_NetCoreApp21 Debug_Build) Windows_x86_NetCoreApp21 Debug_Build succeeded
Details
MachineLearning-CI (Windows_x86_NetCoreApp21 Release_Build) Windows_x86_NetCoreApp21 Release_Build succeeded
Details
WIP Ready for review
Details
license/cla All CLA requirements met.
@justinormont justinormont deleted the justinormont:fixhotlinktomaster branch Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.