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

Using invariance culture when converting to string #4635

Merged

Conversation

@LittleLittleCloud
Copy link
Contributor

LittleLittleCloud commented Jan 7, 2020

@LittleLittleCloud LittleLittleCloud requested a review from dotnet/mlnet-automl as a code owner Jan 7, 2020
@@ -145,7 +145,7 @@ public SweepableFloatParam(float min, float max, float stepSize = -1, int numSte

public override void SetUsingValueText(string valueText)
{
RawValue = float.Parse(valueText, CultureInfo.InvariantCulture);
RawValue = float.Parse(valueText);

This comment has been minimized.

Copy link
@justinormont

justinormont Jan 7, 2020

Member

I would instead push the culture invariant value into the sweepable parameter:

_valueText = _value.ToString("R");

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 9, 2020

Codecov Report

Merging #4635 into master will increase coverage by 0.39%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4635      +/-   ##
==========================================
+ Coverage   75.63%   76.02%   +0.39%     
==========================================
  Files         938      951      +13     
  Lines      168669   173945    +5276     
  Branches    18217    18885     +668     
==========================================
+ Hits       127574   132247    +4673     
- Misses      36065    36541     +476     
- Partials     5030     5157     +127
Flag Coverage Δ
#Debug 76.02% <100%> (+0.39%) ⬆️
#production 71.48% <100%> (+0.24%) ⬆️
#test 90.78% <ø> (+0.33%) ⬆️
Impacted Files Coverage Δ
src/Microsoft.ML.AutoML/Sweepers/Parameters.cs 84.32% <100%> (ø) ⬆️
src/Microsoft.ML.Data/Utilities/SlotDropper.cs 96.5% <0%> (-2.75%) ⬇️
...rc/Microsoft.ML.Featurizers/DateTimeTransformer.cs 89.42% <0%> (-1.16%) ⬇️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs 95.28% <0%> (-0.2%) ⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs 86.1% <0%> (-0.16%) ⬇️
...Microsoft.ML.Tests/Transformers/NormalizerTests.cs 100% <0%> (ø) ⬆️
...soft.ML.Tests/PermutationFeatureImportanceTests.cs 100% <0%> (ø) ⬆️
...ft.ML.Tests/Transformers/TimeSeriesImputerTests.cs 100% <0%> (ø)
src/Microsoft.ML.Featurizers/TimeSeriesImputer.cs 87.08% <0%> (ø)
....ML.Tests/Transformers/ToStringTransformerTests.cs 100% <0%> (ø)
... and 35 more
@justinormont

This comment has been minimized.

Copy link
Member

justinormont commented Jan 21, 2020

Closing/reopening to restart CI tests.

CI is failing w/ an odd Git checkout issue:

2020-01-21T17:01:18.2670411Z ##[command]git checkout --progress --force 36a6ca3
2020-01-21T17:01:18.2671965Z fatal: reference is not a tree: 36a6ca3
2020-01-21T17:01:18.2722749Z ##[error]Git checkout failed with exit code: 128

https://dev.azure.com/dnceng/public/_build/results?buildId=477842&view=logs&j=2302d3fe-129a-52d2-3973-322aaec0c902&t=12a19c0e-c998-51b7-645f-93b51cf4ff66&l=291

It sounds like it its trying to read from an non-existent commit hash. Perhaps restarting the CI will get the CI to read the current commit ID.

@sharwell

This comment has been minimized.

Copy link
Member

sharwell commented Jan 21, 2020

Requesting a review from @tannergooding

@sharwell sharwell requested a review from tannergooding Jan 21, 2020
@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Jan 21, 2020

This will break +/-Infinity as Invariant Culture is Infinity while other cultures may use things like Inf or (the latter is the default for en-us).

It is also likely to break users who expect or depend on the invariant culture working (a culture which uses , rather than . but gets 0.1 as the input may fail to parse or give an unexpected result).

I would recommend just updating the ToString call to also print using InvariantCulture as @justinormont suggested here: #4635 (comment)

@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Jan 21, 2020

Ah, nevermind. I see that the title no longer matches the change being provided. The actual change is updating ToString which should work as expected. 👍

@LittleLittleCloud LittleLittleCloud changed the title remove culture info Using invariance culture in toString Jan 21, 2020
@LittleLittleCloud LittleLittleCloud changed the title Using invariance culture in toString Using invariance culture when converting to string Jan 21, 2020
@justinormont

This comment has been minimized.

Copy link
Member

justinormont commented Jan 21, 2020

For the required MachineLearning-CI, one unit test is failing (randomly). The current unstable one is a TensorFlow test (TensorFlowImageClassificationDefault):

2020-01-21T18:41:12.4604444Z Starting test: Microsoft.ML.Scenarios.TensorFlowScenariosTests.TensorFlowImageClassificationDefault
2020-01-21T18:41:13.0889399Z   X Microsoft.ML.Scenarios.TensorFlowScenariosTests.TensorFlowImageClassificationWithPolynomialLRScheduling [17s 474ms]
2020-01-21T18:41:13.3391570Z   Error Message:
2020-01-21T18:41:13.3878708Z    System.Runtime.InteropServices.SEHException : External component has thrown an exception.
2020-01-21T18:41:13.3881327Z   Stack Trace:
2020-01-21T18:41:13.4477236Z      at Tensorflow.c_api.TF_SessionRun(IntPtr session, TF_Buffer* run_options, TF_Output[] inputs, IntPtr[] input_values, Int32 ninputs, TF_Output[] outputs, IntPtr[] output_values, Int32 noutputs, IntPtr[] target_opers, Int32 ntargets, IntPtr run_metadata, IntPtr status)
2020-01-21T18:41:13.4477710Z    at Microsoft.ML.TensorFlow.TensorFlowUtils.Runner.Run() in D:\a\1\s\src\Microsoft.ML.TensorFlow\TensorflowUtils.cs:line 491
2020-01-21T18:41:13.4478074Z    at Microsoft.ML.Vision.ImageClassificationTrainer.TrainAndEvaluateClassificationLayerCore(Int32 epoch, Single learningRate, Int32 featureFileStartOffset, ImageClassificationMetrics metrics, Int64[] labelTensorShape, Int64[] featureTensorShape, Int32 batchSize, Stream trainSetLabelReader, Stream trainSetFeatureReader, Byte[] labelBufferBytes, Byte[] featuresBufferBytes, Int32 labelBufferSizeInBytes, Int32 featureBufferSizeInBytes, Int32 featureFileRecordSize, LearningRateScheduler learningRateScheduler, DnnTrainState trainState, Runner runner, IntPtr featureBufferPtr, IntPtr labelBufferPtr, Action`2 metricsAggregator) in D:\a\1\s\src\Microsoft.ML.Vision\ImageClassificationTrainer.cs:line 1115
2020-01-21T18:41:13.4478827Z    at Microsoft.ML.Vision.ImageClassificationTrainer.TrainAndEvaluateClassificationLayer(String trainBottleneckFilePath, String validationSetBottleneckFilePath) in D:\a\1\s\src\Microsoft.ML.Vision\ImageClassificationTrainer.cs:line 999
2020-01-21T18:41:13.4479684Z    at Microsoft.ML.Vision.ImageClassificationTrainer.TrainModelCore(TrainContext trainContext) in D:\a\1\s\src\Microsoft.ML.Vision\ImageClassificationTrainer.cs:line 710
2020-01-21T18:41:13.4480104Z    at Microsoft.ML.Trainers.TrainerEstimatorBase`2.TrainTransformer(IDataView trainSet, IDataView validationSet, IPredictor initPredictor) in D:\a\1\s\src\Microsoft.ML.Data\Training\TrainerEstimatorBase.cs:line 157
2020-01-21T18:41:13.4480343Z    at Microsoft.ML.Trainers.TrainerEstimatorBase`2.Fit(IDataView input) in D:\a\1\s\src\Microsoft.ML.Data\Training\TrainerEstimatorBase.cs:line 77
2020-01-21T18:41:13.4480554Z    at Microsoft.ML.Data.EstimatorChain`1.Fit(IDataView input) in D:\a\1\s\src\Microsoft.ML.Data\DataLoadSave\EstimatorChain.cs:line 67
2020-01-21T18:41:13.4480901Z    at Microsoft.ML.Scenarios.TensorFlowScenariosTests.TensorFlowImageClassificationWithLRScheduling(LearningRateScheduler learningRateScheduler, Int32 epoch) in D:\a\1\s\test\Microsoft.ML.Tests\ScenariosWithDirectInstantiation\TensorflowTests.cs:line 1544
2020-01-21T18:41:13.4481204Z    at Microsoft.ML.Scenarios.TensorFlowScenariosTests.TensorFlowImageClassificationWithPolynomialLRScheduling() in D:\a\1\s\test\Microsoft.ML.Tests\ScenariosWithDirectInstantiation\TensorflowTests.cs:line 1474

https://dev.azure.com/dnceng/public/_build/results?buildId=491199&view=logs&j=dd8eddb6-ecc6-5f65-73e6-df90e5693b94

It's only failing under Windows_x64_NetCoreApp21 Debug_Build, so it's unlikely to be deterministic.

I'll rerun the failing CI leg and hopefully the randomness falls in our favor. The previous Git issue was deterministic and restarting the CI seems to cause it to pick up the right commit ID.

@LittleLittleCloud LittleLittleCloud merged commit fa75f4f into dotnet:master Jan 21, 2020
19 checks passed
19 checks passed
MachineLearning-CI Build #20200121.6 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 #20200121.6 had test failures
Details
MachineLearning-CodeCoverage (Windows_x64 Build_Debug) Windows_x64 Build_Debug succeeded
Details
WIP Ready for review
Details
license/cla All CLA requirements met.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.