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

Contributing: Fix typos #4633

Merged
merged 4 commits into from Jan 8, 2020
Merged

Contributing: Fix typos #4633

merged 4 commits into from Jan 8, 2020

Conversation

@MaherJendoubi
Copy link
Contributor

MaherJendoubi commented Jan 7, 2020

Fixing some typos

@MaherJendoubi MaherJendoubi requested a review from dotnet/mlnet-automl as a code owner Jan 7, 2020
@antoniovs1029

This comment has been minimized.

Copy link
Member

antoniovs1029 commented Jan 7, 2020

Changing the name of the variable only in the places you did seems to not be enough, since all of the builds are failing with this error:

Experiment/SuggestedPipelineRunDetails/SuggestedPipelineRunDetail.cs(26,163): error CS1061: 'PipelineScore' does not contain a definition for 'RunSucceded' and no accessible extension method 'RunSucceded' accepting a first argument of type 'PipelineScore' could be found (are you missing a using directive or an assembly reference?)

@MaherJendoubi did you try to build this in your machine before opening the PR? I would think that this would have to fail on your machine too. Please, try to fix this renaming and check if it builds in your machine before updating this PR. Thanks!

@MaherJendoubi

This comment has been minimized.

Copy link
Contributor Author

MaherJendoubi commented Jan 7, 2020

@antoniovs1029 My mistake. I am working on fixing it.

Copy link
Member

justinormont left a comment

Thanks for the fix. Once the unit tests are passing it looks good to merge.

@justinormont justinormont requested a review from antoniovs1029 Jan 7, 2020
@justinormont

This comment has been minimized.

Copy link
Member

justinormont commented Jan 7, 2020

I think you're missing the one here:

// If after third run, all runs have failed so far, throw exception
if (_history.Count() == 3 && _history.All(r => !r.RunSucceded))
{
throw new InvalidOperationException($"Training failed with the exception: {_history.Last().Exception}");
}

And as @antoniovs1029 suggests, building locally will catch any missing renames. Since functionality isn't modified, I'm not expecting the unit tests to show anything interesting; though they're always nice to run, and further end-to-end testing of an AutoML pipeline can catch additional problems.

@MaherJendoubi

This comment has been minimized.

Copy link
Contributor Author

MaherJendoubi commented Jan 7, 2020

@justinormont actually, I have issues to compile the whole solution. But, I fixed the missing changes.

@antoniovs1029

This comment has been minimized.

Copy link
Member

antoniovs1029 commented Jan 8, 2020

@justinormont actually, I have issues to compile the whole solution. But, I fixed the missing changes.

@MaherJendoubi . Unfortunately the builds are still failing, as you can see here:
https://github.com/dotnet/machinelearning/pull/4633/checks

The error is

Experiment/SuggestedPipelineRunDetails/SuggestedPipelineRunDetail.cs(26,163): error CS1061: 'PipelineScore' does not contain a definition for 'RunSucceded' and no accessible extension method 'RunSucceded' accepting a first argument of type 'PipelineScore' could be found (are you missing a using directive or an assembly reference?)

So there are still places where variables need to be renamed.

Can you tell us what problem are you encountering when building ML.NET? I might be able to assist you so you can run things locally and make it easier for you to fix this problem. Thanks for your contributions!

@justinormont

This comment has been minimized.

Copy link
Member

justinormont commented Jan 8, 2020

You can also check on the errors in the CI tests. You can see a build error here:
https://dev.azure.com/dnceng/public/_build/results?buildId=475367&view=logs&j=ac97a0ee-ee90-5a55-af17-a5a589fa545f&t=fdd5b9c0-ee9c-5c91-333c-dcdd6c90934e&l=251

Which ends with:

/1/s/dir.traversal.targets(25,5): error : Build failed. See earlier errors. [/__w/1/s/build.proj]

Build FAILED.

EXEC : CMake warning :  [/__w/1/s/src/Native/build.proj]
Experiment/SuggestedPipelineRunDetails/SuggestedPipelineRunDetail.cs(26,163): error CS1061: 'PipelineScore' does not contain a definition for 'RunSucceded' and no accessible extension method 'RunSucceded' accepting a first argument of type 'PipelineScore' could be found (are you missing a using directive or an assembly reference?) [/__w/1/s/src/Microsoft.ML.AutoML/Microsoft.ML.AutoML.csproj]
/__w/1/s/dir.traversal.targets(25,5): error : Build failed. See earlier errors. [/__w/1/s/build.proj]
    1 Warning(s)
    2 Error(s)

Time Elapsed 00:06:

This error is pointing to this line:
https://github.com/MaherJendoubi/machinelearning/blob/1374699c73d374bacd6500f9db07b35d20ac62f7/src/Microsoft.ML.AutoML/Experiment/SuggestedPipelineRunDetails/SuggestedPipelineRunDetail.cs#L26

Renaming within Visual Studio is easier as it generally finds all occurrences when renaming.

Build instructions:
https://github.com/dotnet/machinelearning/blob/master/docs/project-docs/developer-guide.md


You can navigate to the build output by clicking on the warning/errors on Checks page (or by clicking the Details for the CI on this page):

image

@MaherJendoubi

This comment has been minimized.

Copy link
Contributor Author

MaherJendoubi commented Jan 8, 2020

@antoniovs1029 @justinormont I still have one error :
image

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 8, 2020

Codecov Report

❗️ No coverage uploaded for pull request base (master@354798d). Click here to learn what that means.
The diff coverage is 81.81%.

@@            Coverage Diff            @@
##             master    #4633   +/-   ##
=========================================
  Coverage          ?   75.66%           
=========================================
  Files             ?      938           
  Lines             ?   168766           
  Branches          ?    18228           
=========================================
  Hits              ?   127689           
  Misses            ?    36047           
  Partials          ?     5030
Flag Coverage Δ
#Debug 75.66% <81.81%> (?)
#production 71.25% <81.81%> (?)
#test 90.5% <ø> (?)
Impacted Files Coverage Δ
src/Microsoft.ML.AutoML/API/Pipeline.cs 94.44% <100%> (ø)
...edPipelineRunDetails/SuggestedPipelineRunDetail.cs 100% <100%> (ø)
....ML.AutoML/PipelineSuggesters/PipelineSuggester.cs 83.19% <75%> (ø)
src/Microsoft.ML.AutoML/Experiment/Experiment.cs 83.33% <75%> (ø)
@justinormont

This comment has been minimized.

Copy link
Member

justinormont commented Jan 8, 2020

@MaherJendoubi: I can build your branch, and the unit tests pass (on macOS). I'm just going to retry the CI checks; they seem rather unstable currently.

@MaherJendoubi

This comment has been minimized.

Copy link
Contributor Author

MaherJendoubi commented Jan 8, 2020

@antoniovs1029 Thank you for your feedback. Build and Run are successful for me using commandline but still having issues when building using VS2019 Preview Version 16.5.0 Preview 1.

@antoniovs1029 antoniovs1029 merged commit 0435f6b into dotnet:master Jan 8, 2020
32 of 34 checks passed
32 of 34 checks passed
MachineLearning-Test-Stable Build #20200107.12 had test failures
Details
MachineLearning-Test-Stable (Windows_x64_NetFx461 Release_Build) Windows_x64_NetFx461 Release_Build failed
Details
MachineLearning-CI Build #20200107.11 had test failures
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
MachineLearning-CodeCoverage Build #20200107.11 succeeded
Details
MachineLearning-CodeCoverage (Windows_x64 Build_Debug) Windows_x64 Build_Debug succeeded
Details
MachineLearning-Test-Stable (Centos_x64_NetCoreApp30 Debug_Build) Centos_x64_NetCoreApp30 Debug_Build succeeded
Details
MachineLearning-Test-Stable (Centos_x64_NetCoreApp30 Release_Build) Centos_x64_NetCoreApp30 Release_Build succeeded
Details
MachineLearning-Test-Stable (MacOS_x64_NetCoreApp21 Debug_Build) MacOS_x64_NetCoreApp21 Debug_Build succeeded
Details
MachineLearning-Test-Stable (MacOS_x64_NetCoreApp21 Release_Build) MacOS_x64_NetCoreApp21 Release_Build succeeded
Details
MachineLearning-Test-Stable (Ubuntu_x64_NetCoreApp21 Debug_Build) Ubuntu_x64_NetCoreApp21 Debug_Build succeeded
Details
MachineLearning-Test-Stable (Ubuntu_x64_NetCoreApp21 Release_Build) Ubuntu_x64_NetCoreApp21 Release_Build succeeded
Details
MachineLearning-Test-Stable (Windows_x64_NetCoreApp21 Debug_Build) Windows_x64_NetCoreApp21 Debug_Build succeeded
Details
MachineLearning-Test-Stable (Windows_x64_NetCoreApp21 Release_Build) Windows_x64_NetCoreApp21 Release_Build succeeded
Details
MachineLearning-Test-Stable (Windows_x64_NetCoreApp30 Debug_Build) Windows_x64_NetCoreApp30 Debug_Build succeeded
Details
MachineLearning-Test-Stable (Windows_x64_NetCoreApp30 Release_Build) Windows_x64_NetCoreApp30 Release_Build succeeded
Details
MachineLearning-Test-Stable (Windows_x64_NetFx461 Debug_Build) Windows_x64_NetFx461 Debug_Build succeeded
Details
MachineLearning-Test-Stable (Windows_x86_NetCoreApp21 Debug_Build) Windows_x86_NetCoreApp21 Debug_Build succeeded
Details
MachineLearning-Test-Stable (Windows_x86_NetCoreApp21 Release_Build) Windows_x86_NetCoreApp21 Release_Build succeeded
Details
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
@MaherJendoubi MaherJendoubi deleted the MaherJendoubi:patch-1 branch Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.