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

Test for Metadata Support In DataView Construction #1836

Merged
merged 6 commits into from Dec 11, 2018

Conversation

Projects
None yet
5 participants
@abgoswam
Member

abgoswam commented Dec 6, 2018

Fixes #1633

  • Inside the GetGetterDelegate we were invoking MarshalInvoke with GetGetter<int> . This does not take into consideration the generic return type ValueGetter<TDst>. Subsequently, the validation inside MarshalInvokeCheckAndCreate fails.

  • As part of the fix, we use reflection to invoke the generic GetGetter<TDst> with the appropriate type.

@abgoswam abgoswam requested review from yaeldekel and Ivanidzo4ka Dec 6, 2018

@abgoswam abgoswam changed the title from WIP : Test for Metadata Support In DataView Construction to Test for Metadata Support In DataView Construction Dec 6, 2018

@@ -946,7 +946,17 @@ public override ValueGetter<TDst> GetGetter<TDst>()
throw Contracts.ExceptNotImpl("Type '{0}' is not yet supported.", typeT.FullName);
}
internal override Delegate GetGetterDelegate() => Utils.MarshalInvoke(GetGetter<int>, MetadataType.RawType);
internal override Delegate GetGetterDelegate()

This comment has been minimized.

@yaeldekel

yaeldekel Dec 6, 2018

Member

GetGetterDelegate [](start = 35, length = 17)

I think there is an advantage to using Utils.MarshalInvoke. Since the return value is being cast to a Delegate anyway, would it work if we just changed the return type of GetGetter to be Delegate instead of ValueGetter? #Closed

This comment has been minimized.

@abgoswam

abgoswam Dec 10, 2018

Member

used Tom's tip to get around this


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

public void MetadataSupportInDataViewConstruction()
{
var data = ReadBreastCancerExamples();
using (var env = new ConsoleEnvironment(0))

This comment has been minimized.

@yaeldekel

yaeldekel Dec 6, 2018

Member

ConsoleEnvironment [](start = 33, length = 18)

Use MLContext instead. #Closed

MethodInfo genericMethodInfo = baseMethodInfo.MakeGenericMethod(genericArguments);
object classInstance = Activator.CreateInstance(genericClassType, Kind, Value, MetadataType);
return genericMethodInfo.Invoke(classInstance, new object[] { }) as Delegate;

This comment has been minimized.

@TomFinley

TomFinley Dec 6, 2018

Contributor

Please don't do this... make a new private method GetGetterDelegateCore that is generic on the type and returns a delegate. That method will do nothing but return a Delegate. Use MarshalInvoke on that.

Just as a general rule, if you ever, ever think that the solution to a problem is hard coding the name of a method as a string constant, something has almost certainly gone wrong. ;) #Resolved

This comment has been minimized.

@abgoswam

abgoswam Dec 11, 2018

Member

Thanks for the tip.


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

@TomFinley

Keep using MarshalInvoke please. The solution to these sorts of problems is not to not use marshal invoke, but just call it on a different method.

var idv = env.CreateDataView(data, mySchema);
// Test GetMetadataTypes.
var internalSchemaLabelMetadataTypes = idv.Schema.GetMetadataTypes(0).ToArray();

This comment has been minimized.

@TomFinley

TomFinley Dec 6, 2018

Contributor

GetMetadataTypes [](start = 66, length = 16)

This is new code. Please use the new Schema idioms for accessing metadata, not the old ISchema idioms. To keep using ISchema just adds more technical debt we'll have to clean up.

Here and everywhere in this method. #Resolved

This comment has been minimized.

@abgoswam

abgoswam Dec 10, 2018

Member

By old ISchema idioms, I believe you are referring to usages of GetMetadataTypes, GetMetaDataTypeOrNull and GetMetadata which are part of ISchema interface

I have replaced usages for the above 3 methods with the new Schema idiom for accessing metadata.


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

abgoswam added some commits Dec 10, 2018

var mySchema = new SchemaDefinition { labelColumnWithMetadata, featureColumnWithMetadata };
var idv = mlContext.CreateDataView(data, mySchema);
Assert.True(idv.Schema[0].Metadata.Schema[kindFloat].Type == coltypeFloat);

This comment has been minimized.

@yaeldekel

yaeldekel Dec 10, 2018

Member

Schema[kindFloat] [](start = 47, length = 17)

I would first assert that this is not null (here and in the next lines). #Closed

This comment has been minimized.

@yaeldekel

yaeldekel Dec 10, 2018

Member

Sorry, that's wrong because if metadata with that name did not exist, this would throw.

Instead, you should assert the same way you do in line 226,227: Assert that the name of Schema[0] is kindFloat, and the type of Schema[0] is coltypeFloat.


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

This comment has been minimized.

@yaeldekel

yaeldekel Dec 10, 2018

Member

Also, assert that Schema.Count is 2.


In reply to: 240396374 [](ancestors = 240396374,240392932)

@abgoswam abgoswam requested review from eerhardt and glebuk Dec 11, 2018

try
{
idv.Schema[1].Metadata.GetValue(kindFloat, ref retrievedReadOnlyMemoryVBuffer);

This comment has been minimized.

@eerhardt

eerhardt Dec 11, 2018

Member

Use Assert.Throws instead. #Resolved

{
thrown = true;
}
Assert.True(thrown);

This comment has been minimized.

@eerhardt

eerhardt Dec 11, 2018

Member

Assert.Throws #Resolved

These comments have been addressed, and @TomFinley is not available at the moment. Dismissing this review in order to make progress without @TomFinley.

@eerhardt

Just 2 minor comments about using Assert.Throws. Other than that, looks good.

var kindVBuffer = "Testing VBuffer as metadata.";
var valueVBuffer = new VBuffer<float>(4, new float[] { 4, 6, 89, 5 });
var metaFloat = new Microsoft.ML.Runtime.Api.MetadataInfo<float>(kindFloat, valueFloat, coltypeFloat);

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 11, 2018

Member

Microsoft.ML.Runtime.Api [](start = 32, length = 24)

You have this in your using namespaces, why you need to spell it out again? #Resolved

try
{
idv.Schema[1].Metadata.GetValue(kindFloat, ref retrievedReadOnlyMemoryVBuffer);
Assert.True(false, "Throw an error if attribute is applied to a field that is not an IChannel.");

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 11, 2018

Member

Throw an error if attribute is applied to a field that is not an IChannel. [](start = 36, length = 74)

WHAT? #Resolved

This comment has been minimized.

@abgoswam

abgoswam Dec 11, 2018

Member

This was surprising to me as well . Its present in internal repo, so I kept it as is.

Deleted. Using Assert.Throws instead


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

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 11, 2018

Member

My comment was about content of assert :)


In reply to: 240745243 [](ancestors = 240745243,240742454)

@@ -946,7 +946,15 @@ public override ValueGetter<TDst> GetGetter<TDst>()
throw Contracts.ExceptNotImpl("Type '{0}' is not yet supported.", typeT.FullName);
}
internal override Delegate GetGetterDelegate() => Utils.MarshalInvoke(GetGetter<int>, MetadataType.RawType);
private Delegate GetGetterCore<TDst>()

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 11, 2018

Member

GetGetterCore [](start = 25, length = 13)

I'm a bit confused.
Why do you need this function if all it does is returns GetGetter?
Why you can't call GetGetter directly instead of calling this GetGetterCore function? #Resolved

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 11, 2018

Member

Can you at least add comment which would justify presence of this function, to protect it from deletion?


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

This comment has been minimized.

@abgoswam

abgoswam Dec 11, 2018

Member

If one deletes the GetGetterCore method, and directly does MarshallInvoke on the GetGetter then the unit test will fail.

I have added a brief comment. Also, see Tom's tip on this.


In reply to: 240751239 [](ancestors = 240751239,240750686)

@abgoswam abgoswam merged commit e2e1aa8 into dotnet:master Dec 11, 2018

2 checks passed

MachineLearning-CI #20181211.6 succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment