Skip to content

Issue 4047, improve error message give expected type #5189

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

Merged
merged 4 commits into from
Jun 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/Microsoft.ML.Data/Data/DataViewUtils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1123,9 +1123,11 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
Ch.CheckParam(IsColumnActive(column), nameof(column), "requested column not active.");
Ch.CheckParam(column.Index < _colToActive.Length, nameof(column), "requested column is not active or valid for the Schema.");

var getter = _getters[_colToActive[column.Index]] as ValueGetter<TValue>;
var originGetter = _getters[_colToActive[column.Index]];
var getter = originGetter as ValueGetter<TValue>;
if (getter == null)
throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " +
$"expected type: '{originGetter.GetType().GetGenericArguments().First()}'.");
return getter;
}
}
Expand Down Expand Up @@ -1312,9 +1314,11 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
Ch.CheckParam(IsColumnActive(column), nameof(column), "requested column not active");
Ch.CheckParam(column.Index < _colToActive.Length, nameof(column), "requested column not active or is invalid for the schema. ");

var getter = _getters[_colToActive[column.Index]] as ValueGetter<TValue>;
var originGetter = _getters[_colToActive[column.Index]];
var getter = originGetter as ValueGetter<TValue>;
if (getter == null)
throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " +
$"expected type: '{originGetter.GetType().GetGenericArguments().First()}'.");
return getter;
}
}
Expand Down
6 changes: 4 additions & 2 deletions src/Microsoft.ML.Data/DataLoadSave/Binary/BinaryLoader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2035,9 +2035,11 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
{
Ch.CheckParam(column.Index < _colToActivesIndex.Length, nameof(column), "requested column not active.");

var getter = _pipeGetters[_colToActivesIndex[column.Index]] as ValueGetter<TValue>;
var originGetter = _pipeGetters[_colToActivesIndex[column.Index]];
var getter = originGetter as ValueGetter<TValue>;
if (getter == null)
throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " +
$"expected type: '{originGetter.GetType().GetGenericArguments().First()}'.");
return getter;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System;
using System.Data.Common;
using System.Linq;
using Microsoft.ML.Internal.Utilities;
using Microsoft.ML.Runtime;

Expand Down Expand Up @@ -158,9 +159,11 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
Ch.CheckParam(column.Index < _getters.Length, nameof(column), "requested column not valid.");
Ch.Check(IsColumnActive(column));

var fn = _getters[column.Index] as ValueGetter<TValue>;
var originFn = _getters[column.Index];
var fn = originFn as ValueGetter<TValue>;
if (fn == null)
throw Ch.Except("Invalid TValue in GetGetter: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue in GetGetter: '{typeof(TValue)}', " +
$"expected type: '{originFn.GetType().GetGenericArguments().First()}'.");
return fn;
}

Expand Down
7 changes: 5 additions & 2 deletions src/Microsoft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -318,9 +319,11 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
Ch.CheckParam(column.Index < _getters.Length, nameof(column), "requested column not valid.");
Ch.Check(IsColumnActive(column));

var fn = _getters[column.Index] as ValueGetter<TValue>;
var originFn = _getters[column.Index];
var fn = originFn as ValueGetter<TValue>;
if (fn == null)
throw Ch.Except("Invalid TValue in GetGetter: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue in GetGetter: '{typeof(TValue)}', " +
$"expected type: '{originFn.GetType().GetGenericArguments().First()}'.");
return fn;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,8 @@ public override ValueGetter<VBuffer<TValue>> GetGetter<TValue>()
{
ValueGetter<VBuffer<TValue>> getter = _getter as ValueGetter<VBuffer<TValue>>;
if (getter == null)
throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " +
$"expected type: '{_getter.GetType().GetGenericArguments().First().GetGenericArguments().First()}'.");
Comment on lines 694 to +697
Copy link
Member

@antoniovs1029 antoniovs1029 Jun 2, 2020

Choose a reason for hiding this comment

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

In here we expect a VBuffer, and that's why you're getting .GetGenericArguments() twice, which is fine to get a descriptive exception.

But what happens if _getter is, let's say, of type ValueGetter<int> whereas we expect one of type ValueGetter<VBuffer<float>> ? since the getter we're getting isn't a VBuffer<> then another exception will be thrown when calling the second .First() because the second .GetGenericArguments() would return an empty array. This other exception would be more confusing than the original one, so please add the necessary logic to handle this case correctly in the places where we expect a VBuffer. 😄 #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check out the definition of _getter: private readonly ValueGetter<VBuffer> _getter;


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

return getter;
}

Expand Down Expand Up @@ -883,9 +884,11 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
Ch.CheckParam(column.Index <= _colToActivesIndex.Length && IsColumnActive(column), nameof(column), "requested column not active");
Ch.AssertValue(_getters[_colToActivesIndex[column.Index]]);

var getter = _getters[_colToActivesIndex[column.Index]] as ValueGetter<TValue>;
var originGetter = _getters[_colToActivesIndex[column.Index]];
var getter = originGetter as ValueGetter<TValue>;
if (getter == null)
throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " +
$"expected type: '{originGetter.GetType().GetGenericArguments().First()}'.");
return getter;
}
}
Expand Down
9 changes: 6 additions & 3 deletions src/Microsoft.ML.Data/DataView/AppendRowsDataView.cs
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,12 @@ public sealed override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Colu
{
Ch.CheckParam(column.Index <= Getters.Length && IsColumnActive(column), nameof(column), "requested column not active");

if (!(Getters[column.Index] is ValueGetter<TValue>))
throw Ch.Except($"Invalid TValue in GetGetter: '{typeof(TValue)}'");
return Getters[column.Index] as ValueGetter<TValue>;
var originGetter = Getters[column.Index];
var getter = originGetter as ValueGetter<TValue>;
if (getter == null)
throw Ch.Except($"Invalid TValue in GetGetter: '{typeof(TValue)}', " +
$"expected type: '{originGetter.GetType().GetGenericArguments().First()}'.");
return getter;
}

/// <summary>
Expand Down
6 changes: 4 additions & 2 deletions src/Microsoft.ML.Data/DataView/ArrayDataViewBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -325,9 +325,11 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
Ch.Check(column.Index < Schema.Count);
Ch.Check(column.Index < _active.Length && _active[column.Index], "the requested column is not active");

var columnValue = _view._columns[column.Index] as Column<TValue>;
var originColumnValue = _view._columns[column.Index];
var columnValue = originColumnValue as Column<TValue>;
if (columnValue == null)
throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " +
$"expected type: '{originColumnValue.Type.RawType}'.");

return
(ref TValue value) =>
Expand Down
3 changes: 2 additions & 1 deletion src/Microsoft.ML.Data/DataView/BatchDataViewMapperBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,8 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
Ch.Assert(getter != null);
var fn = getter as ValueGetter<TValue>;
if (fn == null)
throw Ch.Except("Invalid TValue in GetGetter: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue in GetGetter: '{typeof(TValue)}', " +
$"expected type: '{getter.GetType().GetGenericArguments().First()}'.");
return fn;
}

Expand Down
6 changes: 4 additions & 2 deletions src/Microsoft.ML.Data/DataView/CacheDataView.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1206,9 +1206,11 @@ public sealed override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Colu
Ch.CheckParam(column.Index <= _colToActivesIndex.Length && IsColumnActive(column), nameof(column), "requested column not active");
Ch.Check(_colToActivesIndex[column.Index] < _getters.Length);

var getter = _getters[_colToActivesIndex[column.Index]] as ValueGetter<TValue>;
var originGetter = _getters[_colToActivesIndex[column.Index]];
var getter = originGetter as ValueGetter<TValue>;
if (getter == null)
throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " +
$"expected type: '{originGetter.GetType().GetGenericArguments().First()}'.");
return getter;
}

Expand Down
3 changes: 2 additions & 1 deletion src/Microsoft.ML.Data/DataView/DataViewConstructionUtils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,8 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
Contracts.AssertValue(getter);
var fn = getter as ValueGetter<TValue>;
if (fn == null)
throw Host.Except("Invalid TValue in GetGetter for column #{0}: '{1}'", column, typeof(TValue));
throw Host.Except($"Invalid TValue in GetGetter for column #{column}: '{typeof(TValue)}', " +
$"expected type: '{getter.GetType().GetGenericArguments().First()}'.");
return fn;
}
}
Expand Down
11 changes: 7 additions & 4 deletions src/Microsoft.ML.Data/DataView/RowToRowMapperTransform.cs
Original file line number Diff line number Diff line change
Expand Up @@ -333,10 +333,12 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
if (isSrc)
return Input.GetGetter<TValue>(Input.Schema[index]);

Contracts.Assert(_getters[index] != null);
var fn = _getters[index] as ValueGetter<TValue>;
var originFn = _getters[index];
Contracts.Assert(originFn != null);
var fn = originFn as ValueGetter<TValue>;
if (fn == null)
throw Contracts.Except("Invalid TValue in GetGetter: '{0}'", typeof(TValue));
throw Contracts.Except($"Invalid TValue in GetGetter: '{typeof(TValue)}', " +
$"expected type: '{originFn.GetType().GetGenericArguments().First()}'.");
return fn;
}

Expand Down Expand Up @@ -402,7 +404,8 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
Ch.Assert(getter != null);
var fn = getter as ValueGetter<TValue>;
if (fn == null)
throw Ch.Except("Invalid TValue in GetGetter: '{0}'", typeof(TValue));
throw Contracts.Except($"Invalid TValue in GetGetter: '{typeof(TValue)}', " +
$"expected type: '{getter.GetType().GetGenericArguments().First()}'.");
return fn;
}

Expand Down
12 changes: 8 additions & 4 deletions src/Microsoft.ML.Data/DataView/Transposer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,8 @@ public override ValueGetter<VBuffer<TValue>> GetGetter<TValue>()
_getter = GetGetterCore();
ValueGetter<VBuffer<TValue>> getter = _getter as ValueGetter<VBuffer<TValue>>;
if (getter == null)
throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " +
$"expected type: '{_getter.GetType().GetGenericArguments().First().GetGenericArguments().First()}'.");
return getter;
}

Expand Down Expand Up @@ -1167,9 +1168,11 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
{
Contracts.Check(IsColumnActive(column) && column.Index < _getters.Length);
Contracts.AssertValue(_getters[column.Index]);
var fn = _getters[column.Index] as ValueGetter<TValue>;
var originFn = _getters[column.Index];
var fn = originFn as ValueGetter<TValue>;
if (fn == null)
throw Contracts.Except("Invalid TValue in GetGetter: '{0}'", typeof(TValue));
throw Contracts.Except($"Invalid TValue in GetGetter: '{typeof(TValue)}', " +
$"expected type: '{originFn.GetType().GetGenericArguments().First()}'.");
return fn;
}

Expand Down Expand Up @@ -1501,7 +1504,8 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu

var getter = _getter as ValueGetter<TValue>;
if (getter == null)
throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " +
$"expected type: '{_getter.GetType().GetGenericArguments().First()}'.");
return getter;
}

Expand Down
3 changes: 2 additions & 1 deletion src/Microsoft.ML.Data/Scorers/RowToRowScorerBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,8 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
Ch.Assert(getter != null);
var fn = getter as ValueGetter<TValue>;
if (fn == null)
throw Ch.Except("Invalid TValue in GetGetter: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue in GetGetter: '{typeof(TValue)}', " +
$"expected type: '{getter.GetType().GetGenericArguments().First()}'.");
return fn;
}
}
Expand Down
8 changes: 5 additions & 3 deletions src/Microsoft.ML.Data/Transforms/GenerateNumberTransform.cs
Original file line number Diff line number Diff line change
Expand Up @@ -439,10 +439,12 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
if (isSrc)
return Input.GetGetter<TValue>(Input.Schema[index]);

Ch.Assert(_getters[index] != null);
var fn = _getters[index] as ValueGetter<TValue>;
var originFn = _getters[index];
Ch.Assert(originFn != null);
var fn = originFn as ValueGetter<TValue>;
if (fn == null)
throw Ch.Except("Invalid TValue in GetGetter: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue in GetGetter: '{typeof(TValue)}', " +
$"expected type: '{originFn.GetType().GetGenericArguments().First()}'.");
return fn;
}

Expand Down
4 changes: 3 additions & 1 deletion src/Microsoft.ML.Data/Transforms/LabelConvertTransform.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Linq;
using System.Text;
using System.Threading;
using Microsoft.ML;
Expand Down Expand Up @@ -223,7 +224,8 @@ public override ValueGetter<VBuffer<TValue>> GetGetter<TValue>()
{
ValueGetter<VBuffer<TValue>> getter = _getter as ValueGetter<VBuffer<TValue>>;
if (getter == null)
throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " +
$"expected type: '{_getter.GetType().GetGenericArguments().First().GetGenericArguments().First()}'.");
return getter;
}
}
Expand Down
6 changes: 4 additions & 2 deletions src/Microsoft.ML.Data/Transforms/NAFilter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -426,9 +426,11 @@ private bool TryGetColumnValueGetter<TValue>(int col, out ValueGetter<TValue> fn
return false;
}

fn = _values[index].GetGetter() as ValueGetter<TValue>;
var originFn = _values[index].GetGetter();
fn = originFn as ValueGetter<TValue>;
if (fn == null)
throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " +
$"expected type: '{originFn.GetType().GetGenericArguments().First()}'.");
return true;
}

Expand Down
3 changes: 2 additions & 1 deletion src/Microsoft.ML.Data/Transforms/PerGroupTransformBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,8 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
Ch.Assert(getter != null);
var fn = getter as ValueGetter<TValue>;
if (fn == null)
throw Ch.Except("Invalid TValue in GetGetter: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue in GetGetter: '{typeof(TValue)}', " +
$"expected type: '{getter.GetType().GetGenericArguments().First()}'.");
return fn;
}

Expand Down
6 changes: 4 additions & 2 deletions src/Microsoft.ML.Data/Transforms/RangeFilter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -321,9 +321,11 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu

if (column.Index != Parent._index)
return Input.GetGetter<TValue>(column);
var fn = GetGetter() as ValueGetter<TValue>;
var originFn = GetGetter();
var fn = originFn as ValueGetter<TValue>;
if (fn == null)
throw Ch.Except("Invalid TValue in GetGetter: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue in GetGetter: '{typeof(TValue)}', " +
$"expected type: '{originFn.GetType().GetGenericArguments().First()}'.");

return fn;
}
Expand Down
8 changes: 6 additions & 2 deletions src/Microsoft.ML.Data/Transforms/RowShufflingTransformer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using System.Threading.Tasks.Dataflow;
Expand Down Expand Up @@ -731,9 +732,12 @@ public override ValueGetter<TValue> GetGetter<TValue>(DataViewSchema.Column colu
{
Ch.CheckParam(column.Index < _colToActivesIndex.Length, nameof(column));
Ch.CheckParam(_colToActivesIndex[column.Index] >= 0, nameof(column), "requested column not active");
ValueGetter<TValue> getter = _getters[_colToActivesIndex[column.Index]] as ValueGetter<TValue>;

var originGetter = _getters[_colToActivesIndex[column.Index]];
ValueGetter<TValue> getter = originGetter as ValueGetter<TValue>;
if (getter == null)
throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue));
throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " +
$"expected type: '{originGetter.GetType().GetGenericArguments().First()}'.");
Comment on lines +735 to +740
Copy link
Member

@antoniovs1029 antoniovs1029 Jun 2, 2020

Choose a reason for hiding this comment

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

(pinned to code to allow for threaded conversation)

In general, I think that the approach you used to fix this issue is fine, and it's actually a very good addition to make our exception messages more clear.

The only potential problem I see with it is that maybe this will end up throwing another exception: for example, the _getters in this method is an array of Delegates (i.e. it's not an array of ValueGetter<>'s) so the cast as ValueGetter<TValue> could potentially fail not only if the the generic parameter is mistaken, but also in the case that _getters[i] isn't a ValueGetter<> to begin with. If it isn't a ValueGetter<>, and if it's not a delegate with generic types, then the .First() call in here will throw an exception because the .GetGenericArguments() method would have thrown an empty array.

I haven't checked the other places were you've made changes, but some of them do have an original _getter which is already a ValueGetter<> (so this wouldn't be a problem in those cases) but in other cases the original _getter is a Delegate instead of ValueGetter<> (so potentially they might not have GenericArguments, and then throw another exception).

I'm not sure if I'm explaining myself.

In any case, I also don't know if we should worry about this problem... that other exception would only be thrown in some very fringe case (which I'm not sure if it's actually possible to reach that case), and in any way we were already throwing an exception. So maybe this is unimportant. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your concern, please check out the implementation of this _getters, mainly method CreateGetterDelegate in same class and you will find below implementation, so it is guaranteed you will get ValueGetter for this Delegate but the real problem is you are using wrong TValue type.

        private ValueGetter<TValue> CreateGetterDelegate<TValue>(ShufflePipe pipe)
        {
            Ch.AssertValue(pipe);
            Ch.Assert(pipe is ShufflePipe<TValue>);
            var pipe2 = (ShufflePipe<TValue>)pipe;
            ValueGetter<TValue> getter =
                (ref TValue value) =>
                {
                    Ch.Assert(_pipeIndex == _pipeIndices[_circularIndex]);
                    pipe2.Fetch(_pipeIndex, ref value);
                };
            return getter;
        }

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

return getter;
}
}
Expand Down
Loading