Auto.ML: Fix issue when parsing float string fails on pl-PL culture set using Regression Experiment

[ptelman]

Fixes #5162

Float parsing fails when culture = pl-PL despite the fact that it does not need to parse anything here.
Variable pset is in fact IParameterValue<float> - it has float Value property with correct float type which should be used.

[codecov]

Codecov Report

Merging #5163 into master will increase coverage by 0.00%.
The diff coverage is 78.12%.

@@           Coverage Diff           @@
##           master    #5163   +/-   ##
=======================================
  Coverage   74.11%   74.12%           
=======================================
  Files        1020     1020           
  Lines      190379   190396   +17     
  Branches    20453    20459    +6     
=======================================
+ Hits       141095   141126   +31     
+ Misses      43765    43756    -9     
+ Partials     5519     5514    -5     

Flag	Coverage Δ
#Debug	74.12% <78.12%> (+<0.01%)	⬆️
#production	69.90% <50.00%> (+0.01%)	⬆️
#test	87.69% <84.61%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...soft.ML.AutoML/Sweepers/SweeperProbabilityUtils.cs	72.72% <40.00%> (-2.28%)	⬇️
test/Microsoft.ML.AutoML.Tests/AutoFitTests.cs	92.64% <84.61%> (-1.16%)	⬇️
src/Microsoft.ML.AutoML/Sweepers/Parameters.cs	85.59% <100.00%> (+1.27%)	⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs	79.48% <0.00%> (-20.52%)	⬇️
src/Microsoft.ML.AutoML/API/AutoCatalog.cs	69.35% <0.00%> (-4.84%)	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	86.53% <0.00%> (-0.15%)	⬇️
...StandardTrainers/Standard/LinearModelParameters.cs	66.58% <0.00%> (+0.25%)	⬆️
...L.AutoML/TrainerExtensions/TrainerExtensionUtil.cs	86.36% <0.00%> (+1.65%)	⬆️
src/Microsoft.ML.AutoML/Sweepers/ISweeper.cs	73.41% <0.00%> (+2.53%)	⬆️
... and 5 more

[ptelman]

@justinormont @mstfbl can we do something to have that merged?

[antoniovs1029]

Hi, @ptelman . Thanks for finding this, and opening a Pull Request.

As the Contribution Guide states, it's preferable if you open an issue describing the bug you found so that we can better review the solution you're suggesting:

Your pull request needs to reference a filed issue. Please fill in the template that is populated for the pull request. Only pull requests addressing small typos can have no issues associated with them.

Please, remember to attach source code, a dataset, and the stack trace you got, so that we can have more context of the problem. Also, please include the version of ML.NET you're using.

Now, if I'm understanding your problem correctly, I don't think this PR would be the correct way to solve it. The changes you're introducing might solve your particular case with your particular pipeline and dataset, but those changes might also break the functionality for other scenarios.

I'm not very familiar with the Auto.ML code, but it seems, by the way it's written, that here:

machinelearning/src/Microsoft.ML.AutoML/Sweepers/SweeperProbabilityUtils.cs

Lines 104 to 108 in c0eeea9

	else if (sweepParam is FloatValueGenerator fvg)
	{
	// Normalizing all numeric parameters to [0,1] range.
	result.Add(fvg.NormalizeValue(new FloatParameterValue(pset.Name, float.Parse(pset.ValueText))));
	}

pset could be a IParameterValue<float> (such as in your case) but it could also be IParameterValue<long> or even a IParameterValue<string>, and its Value would have an encoding of a float, although the Value itself would be of type float, long or string, and that's why it was necessary to parse the .ValueText. With your change, now we'd only let IParameterValue<float> to parse floats and IParameterValue<int> to parse ints, which isn't the expected behavior.

As @justinormont suggested to you here #5163 (comment) the correct solution to this problem is to find where is the .ValueText being saved with the incorrect culture info. @justinormont has pointed you to this PR:

https://github.com/dotnet/machinelearning/pull/4635/files#diff-e7cdf28fd3f76a7910d3643f4d32ff7dL85

And upon looking to it I noticed that maybe this line could be the problem (although it's in LongParameterValue):

machinelearning/src/Microsoft.ML.AutoML/Sweepers/Parameters.cs

Line 86 in c0eeea9

_valueText = _value.ToString("D");

And the solution would be analogous to #4635 in that it would only need to change one line:
_valueText = _value.ToString("D", CultureInfo.InvariantCulture);

Can you please try out that one line change instead of the changes you made on this PR and see if that fixes your issue?

If it doesn't, then my suggestion would be to set breakpoints in the constructors of: FloatParameterValue, LongParameterValue, StringParameterValue until you find which is the unparsable string that is causing your exception.

Many thanks! 😄

[ptelman]

Hi, @ptelman . Thanks for finding this, and opening a Pull Request.

As the Contribution Guide states, it's preferable if you open an issue describing the bug you found so that we can better review the solution you're suggesting:

Your pull request needs to reference a filed issue. Please fill in the template that is populated for the pull request. Only pull requests addressing small typos can have no issues associated with them.

Please, remember to attach source code, a dataset, and the stack trace you got, so that we can have more context of the problem. Also, please include the version of ML.NET you're using.

Now, if I'm understanding your problem correctly, I don't think this PR would be the correct way to solve it. The changes you're introducing might solve your particular case with your particular pipeline and dataset, but those changes might also break the functionality for other scenarios.

I'm not very familiar with the Auto.ML code, but it seems, by the way it's written, that here:

machinelearning/src/Microsoft.ML.AutoML/Sweepers/SweeperProbabilityUtils.cs

Lines 104 to 108 in c0eeea9

	else if (sweepParam is FloatValueGenerator fvg)
	{
	// Normalizing all numeric parameters to [0,1] range.
	result.Add(fvg.NormalizeValue(new FloatParameterValue(pset.Name, float.Parse(pset.ValueText))));
	}

pset could be a IParameterValue<float> (such as in your case) but it could also be IParameterValue<long> or even a IParameterValue<string>, and its Value would have an encoding of a float, although the Value itself would be of type float, long or string, and that's why it was necessary to parse the .ValueText. With your change, now we'd only let IParameterValue<float> to parse floats and IParameterValue<int> to parse ints, which isn't the expected behavior.

As @justinormont suggested to you here #5163 (comment) the correct solution to this problem is to find where is the .ValueText being saved with the incorrect culture info. @justinormont has pointed you to this PR:

https://github.com/dotnet/machinelearning/pull/4635/files#diff-e7cdf28fd3f76a7910d3643f4d32ff7dL85

And upon looking to it I noticed that maybe this line could be the problem (although it's in LongParameterValue):

machinelearning/src/Microsoft.ML.AutoML/Sweepers/Parameters.cs

Line 86 in c0eeea9

_valueText = _value.ToString("D");

And the solution would be analogous to #4635 in that it would only need to change one line:
_valueText = _value.ToString("D", CultureInfo.InvariantCulture);

Can you please try out that one line change instead of the changes you made on this PR and see if that fixes your issue?

If it doesn't, then my suggestion would be to set breakpoints in the constructors of: FloatParameterValue, LongParameterValue, StringParameterValue until you find which is the unparsable string that is causing your exception.

Many thanks! 😄

Please find details here #5162
If pset is not IParameterValue<float>, it will parse using CultureInfo.InvariantCulture.
If it is, it seems that parsing is not needed as we have value already in place.
The issue was that InvariantCulture text was parsed not using InvariantCulture option.

[antoniovs1029]

Please find details here #5162
If pset is not IParameterValue<float>, it will parse using CultureInfo.InvariantCulture.
If it is, it seems that parsing is not needed as we have value already in place.
The issue was that InvariantCulture text was parsed not using InvariantCulture option.

Sorry I missed the original issue. I've added a link to it in the description of this PR.

Ok, after a second look at your solution, I think it's fine. I'll leave a couple of comments, though. Also, please add a new test testing your changes. You can add it in here:
https://github.com/dotnet/machinelearning/blob/master/test/Microsoft.ML.Sweeper.Tests/TestSweeper.cs

And test directly the ParameterSetAsFloatArray using something like this:

            Thread.CurrentThread.CurrentCulture = new CultureInfo("pl-PL");
            var mlContext = new MLContext(1);
            var host = ((IHostEnvironment)mlContext).Register("test")
            SweeperProbabilityUtils.ParameterSetAsFloatArray(host, new[] { new FloatValueGenerator(new FloatParamOptions() { Name = "foo", Min = 1, Max = 5 }) }, new ParameterSet(new IParameterValue[] { new LongParameterValue("lng", 1000), new FloatParameterValue("flt", 1.333f), new StringParameterValue("str", "1.3333") }));

Only please add more ValueGenerators and Parameters Values, and do it in a way that this new test would fail without the changes introduced on your PR. This way we have a test that will prevent us from introducing future bugs related to this.

Finally, please also change this inline to have InvariantCulture, in order to avoid problems in the future:

machinelearning/src/Microsoft.ML.AutoML/Sweepers/Parameters.cs

Line 86 in c0eeea9

_valueText = _value.ToString("D");

Many thanks! 😄

[frank-dong-ms-zz]

@ptelman Hi, are you still actively working on this issue now?

[antoniovs1029]

So I think #5455 needs this PR to be resolved? I guess overall it LGTM, so I think we can merge this. We can add the tests I've requested after merging it.

@justinormont since this is modifying AutoML code, can you please also take a look and approve this? thanks!

[antoniovs1029]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[antoniovs1029]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[justinormont]

LGTM

If possible we could also fix the long parameters by using CultureInvariant -- #5163 (comment). Otherwise it can go in to the follow-up PR.

[ptelman]

LGTM

If possible we could also fix the long parameters by using CultureInvariant -- #5163 (comment). Otherwise it can go in to the follow-up PR.

Should I commit the suggestion mentioned or it would be better to not change anything more in this PR?

[justinormont]

Sure. You can add the two parts of the long fix -- the setting InvariantCulture in the ToString() and the in the Parse().

Sorry, I don't know if I follow the other part - should I do anything more with the ToString() part?

Change this line:

machinelearning/src/Microsoft.ML.AutoML/Sweepers/Parameters.cs

Line 86 in d849ba4

_valueText = _value.ToString("D");

To:

      _valueText = _value.ToString("D", CultureInfo.InvariantCulture);

Does it make any difference? According to documentation it shouldn't add decimal places to long string, so separator shouldn't be included in the resulting string.

The difference is how the negative numbers are represented. @mstfbl and I created a quick test to demonstrate cultures where the printing of "0.5" and "-1" differ from the invariant culture -- https://dotnetfiddle.net/LtAtoi.

For example ar-SA uses ؜-1 for their negative numbers; fi-FI uses −1 (which is not a standard dash, but a U+2212); he-IL uses ‎‎-1 which looks like the invariant culture but has an invisible U+200E LEFT-TO-RIGHT MARK.

[dnfadmin]

All CLA requirements met.

[ptelman]

Sure. You can add the two parts of the long fix -- the setting InvariantCulture in the ToString() and the in the Parse().

Sorry, I don't know if I follow the other part - should I do anything more with the ToString() part?

Change this line:

machinelearning/src/Microsoft.ML.AutoML/Sweepers/Parameters.cs

Line 86 in d849ba4

_valueText = _value.ToString("D");

To:

      _valueText = _value.ToString("D", CultureInfo.InvariantCulture);

Does it make any difference? According to documentation it shouldn't add decimal places to long string, so separator shouldn't be included in the resulting string.

The difference is how the negative numbers are represented. @mstfbl and I created a quick test to demonstrate cultures where the printing of "0.5" and "-1" differ from the invariant culture -- https://dotnetfiddle.net/LtAtoi.

For example ar-SA uses ؜-1 for their negative numbers; fi-FI uses −1 (which is not a standard dash, but a U+2212); he-IL uses ‎‎-1 which looks like the invariant culture but has an invisible U+200E LEFT-TO-RIGHT MARK.

Thanks, I didn't know about that 👍