Skip to content

Fixes #4299 related to ML.Samples and ML.Samples.GPU sharing the same Program.cs file - #4300

Merged
codemzs merged 7 commits into
dotnet:masterfrom
antoniovs1029:samplesgpu
Nov 8, 2019
Merged

Fixes #4299 related to ML.Samples and ML.Samples.GPU sharing the same Program.cs file#4300
codemzs merged 7 commits into
dotnet:masterfrom
antoniovs1029:samplesgpu

Conversation

@antoniovs1029

Copy link
Copy Markdown
Contributor

A simple change to fix #4299 . Simply added a new Program.cs file to the ML.Samples.GPU, independent of the one in ML.Samples.

@antoniovs1029
antoniovs1029 requested a review from a team as a code owner October 4, 2019 23:06
@antoniovs1029 antoniovs1029 changed the title Fixes #4299 related to ML.Samples causing problems in ML.Samples.GPU Fixes #4299 related to ML.Samples and ML.Samples.GPU sharing the same Program.cs file Oct 4, 2019
@codecov

codecov Bot commented Oct 5, 2019

Copy link
Copy Markdown

Codecov Report

Merging #4300 into master will decrease coverage by 0.66%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4300      +/-   ##
==========================================
- Coverage   74.72%   74.06%   -0.67%     
==========================================
  Files         906      905       -1     
  Lines      159275   159107     -168     
  Branches    17142    17128      -14     
==========================================
- Hits       119026   117846    -1180     
- Misses      35446    36499    +1053     
+ Partials     4803     4762      -41
Flag Coverage Δ
#Debug 74.06% <ø> (-0.67%) ⬇️
#production 69.23% <ø> (-0.87%) ⬇️
#test 90.11% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
src/Microsoft.ML.TensorFlow/TensorflowCatalog.cs 0% <0%> (-100%) ⬇️
src/Microsoft.ML.Vision/VisionCatalog.cs 3.84% <0%> (-96.16%) ⬇️
src/Microsoft.ML.Vision/DnnRetrainTransform.cs 0% <0%> (-57.27%) ⬇️
src/Microsoft.ML.TensorFlow/TensorFlowModel.cs 43.75% <0%> (-56.25%) ⬇️
src/Microsoft.ML.OnnxConverter/SaveOnnxCommand.cs 25.29% <0%> (-47.65%) ⬇️
src/Microsoft.ML.TensorFlow/TensorflowUtils.cs 42.1% <0%> (-28.43%) ⬇️
src/Microsoft.ML.TensorFlow/TensorflowTransform.cs 51.01% <0%> (-23.07%) ⬇️
src/Microsoft.ML.OnnxConverter/OnnxUtils.cs 66.77% <0%> (-18.61%) ⬇️
src/Microsoft.ML.OnnxConverter/OnnxContextImpl.cs 54.28% <0%> (-18.29%) ⬇️
...osoft.ML.Recommender/SafeTrainingAndModelBuffer.cs 61.97% <0%> (-16.91%) ⬇️
... and 10 more

{
public static void Main(string[] args) => RunAll();

internal static void RunAll()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this empty?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I wrote in the issue, I got runtime errors if I tried to run it with the original content of Program.cs. So I removed that.

Should I put that back into the method?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What were the errors? Did you run the samples on a machine with a GPU?
If there are errors, then the samples that throw them should be fixed.


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

@antoniovs1029 antoniovs1029 Oct 7, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My machine doesn't have a dedicated GPU. And now I realize that the exceptions were thrown because I don't have tensorflow installed either.

I will find a machine that has those to run the samples, and if it works then I will return the content of Program.cs.

Thanks for the feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I had checked this on another machine, and the samples run with the original content of the Program.cs file, so I've put that back in here.

So, please, approve this PR. Thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So today I got my machine with a GPU running. And I checked that this works as expected; everything went correctly. Please, approve this PR. Thanks!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for verifying that the samples run correctly!
Since they are, is this PR still needed? This file is identical to the one in Microsoft.ML.Samples, isn't it?


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

@antoniovs1029 antoniovs1029 Oct 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they are identical. The problem was that both projects shared the same file, and when editing the file under ML.Samples to run a specific sample, then some error tooltips appeared when editing it and some build errors occurred when trying to rebuild the solution after making those changes in that file. This happened because ML.Samples.GPU doesn't have access to the same things that ML.Samples.

So these errors didn't appear before, and after talking about it with @codemzs we agreed that I should add another Program.cs file in ML.Samples.GPU with the same content of the one in ML.Samples.

@codemzs codemzs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@codemzs
codemzs merged commit b26092e into dotnet:master Nov 8, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Problems caused by ML.Samples and ML.Samples.GPU sharing the same Program.cs file

3 participants