ApplyOnnxModel() throw runtime error in an ongoing sample for onnx conversion

[wangyems]

Can reproduce using 0b1a59c
The related file is OnnxConversion.cs.
Just curious why ML.NET model can make through but onnx model not.

[wangyems]

The background: The Microsoft.ML.SamplesUtils.DatasetUtils.LoadFeaturizedAdultDataset() defines a TextLoader using a wrong column type(float) but the actual data type is string(This fixed in #5195). When a user defined column type mismatches the real data type in file, the corresponding position of parsed-item array stores nothing(uninitialized/null)

The reason ApplyOnnxModel() throws runtime error is that the generated onnx model has some flaw: the attribute of OneHotEncoder node going after that wrong-type-column node does not have any mapping information(the dictionary size in OneHotEncoder is 0), and then a 3rd party API(in onnxruntime) detects it and returns error code. (I think this is the expected behavior)

Why ML.NET does not throw any error? Although each of the parsed item is uninitialized, the OneHotEncoder still has a non-empty dictionary(this term may not accurate), and mapping those nulls to a specific number(1) which silently let go a corner case in TextLoader.

This could result in a "flawed-trained" model and inference with accuracy drop, and it seems hard to debug.

[antoniovs1029]

Sorry, I'm not sure I'm following. Are you saying that while writing your sample in #5195 , you found that the TextLoader options used some of our samples are actually wrong, because it should be loading the "native-country" (col 13) as string instead of float?

machinelearning/src/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs

Lines 90 to 111 in c0eeea9

	var loader = mlContext.Data.CreateTextLoader(
	columns: new[]
	{
	new TextLoader.Column("age", DataKind.Single, 0),
	new TextLoader.Column("workclass", DataKind.String, 1),
	new TextLoader.Column("fnlwgt", DataKind.Single, 2),
	new TextLoader.Column("education", DataKind.String, 3),
	new TextLoader.Column("education-num", DataKind.Single, 4),
	new TextLoader.Column("marital-status", DataKind.String, 5),
	new TextLoader.Column("occupation", DataKind.String, 6),
	new TextLoader.Column("relationship", DataKind.String, 7),
	new TextLoader.Column("ethnicity", DataKind.String, 8),
	new TextLoader.Column("sex", DataKind.String, 9),
	new TextLoader.Column("capital-gain", DataKind.Single, 10),
	new TextLoader.Column("capital-loss", DataKind.Single, 11),
	new TextLoader.Column("hours-per-week", DataKind.Single, 12),
	new TextLoader.Column("native-country", DataKind.Single, 13),
	new TextLoader.Column("IsOver50K", DataKind.Boolean, 14),
	},
	separatorChar: ',',
	hasHeader: true
	);

Because the dataset it's loading (adult.txt) actually provides strings like United-States and Puerto-Rico for that column, which aren't floats, right?

And this is causing a problem, because when loading it as a float, what actually happens is that that column is loaded as only NaNs (because the values were unparsable as float), which then maps to "0" for every row in the OneHotEncoder, which then causes a problem because the saved OnnxModel has a dictionary of size 0 to save the OneHotEncoding, and which then causes a problem when calling ApplyOnnxModel because OnnxRuntime can't apply a mapping with a dictionary of size 0, right?

When a user defined column type mismatches the real data type in file, the corresponding position of parsed-item array stores nothing(uninitialized/null)

I believe this isn't entirely true. When the user defines a DataKind.Single column, but the column should actually be DataKind.String (like what I understand is your case), then what happens is that that column gets loaded as NaNs (which isn't null nor 0). This is done by design of the textloader: if it's not possible to parse a value for a Single or Double column, then NaN is returned. This is done by design.

But this only happens for Single or Doubles. If you change the datatype of the TextLoader to any Integer or Daytime or Boolean, then it will throw an exception when trying to parse the column that should be String. "InvalidOperationException: Could not parse value United-States in line 322, column native-country".

On the other hand, I believe that any Transform that deals with converting NaNs to KeyType is designed to map it as "0" for its Key, because 0 means "missing" for KeyType.

For a little more context on the above, see:
https://github.com/dotnet/machinelearning/blob/master/docs/code/IDataViewTypeSystem.md#missing-value

and:
https://github.com/dotnet/machinelearning/blob/master/docs/code/IDataViewTypeSystem.md#from-text

Now, the way I see it there are 2 possible solutions:

• We could add an option to the TextLoader to also fail when loading a single/double it can't parse. Although that wouldn't solve this particular problem, since it might still silently fail and the user won't notice this.
• We could change the Onnx export of OneHotEncoding so that if the dictionary is empty, then it doesn't add anything to the graph, or add something like a placeholder dictionary that won't actually be used, but be there only to avoid this exception.

[wangyems]

Thanks for the write-up! Yes we are on the same page of this issue and the link you shared is helpful:)
I prefer the first option that you provided but not quite follow why "it might still silently fail and the user won't notice this".

[antoniovs1029]

The first option I suggested, was to add a new option to the text loader such as "FailWithUnparsableFloat" (or something like that). If the user sets it to true, then we'll throw an exception if a float is unparsable, instead of reading it as NaN.

Problem is that the default of that option should be "FailtWithUnparsableFloat = false", because we'd want to keep the current behavior that TextLoader has as default. Then, I'm not sure if this would be useful, since the user should know in advance that they should set that option to True, in order to avoid running into the issue you describe in here.

[wangyems]

Yeah, that would be a breaking change. Since I neither think this is very much useful and this is even not a bug. I'll close this issue right now.