Allow use of property-based row classes in ML.NET by dsyme · Pull Request #616 · dotnet/machinelearning

dsyme · 2018-07-31T13:20:14Z

This is WIP to address

Support ColumnAttribute on properties and not just fields #254 - "Support ColumnAttribute on properties and not just fields" and
F# Records not compatible with ML .NET #180 - "F# Records not compatible with ML .NET"

It builds on #600 and you can see the added diff between #600 and this PR here

Copying comment from here:

This would allow the use of ColumnNameAttribute and friends on both public fields (as today) and public properties that have getters/setters.

I've tested that allows an F# record definition to be successfully used, e.g. this test now passes in that branch. It would also allow C# classes that just use attributed public get/set properties to be used, many tests for that pattern would need to be added (e.g. covering the vector, channel and other cases)

It would be a change in spec because existing C# classes that use public fields plus some additional get/set properties may now have their get/set properties considered part of the schema, when they weren't before. I suppose this may mean the user has to add some NoColumn attributes on to these properties.

We could theoretically adjust the spec to be "if there are non-zero public fields, then use public fields. Otherwise, see if there are public get/set properties", but right now I've used the rule "combine the public fields and public get/set properties" as that seems more natural and allows gradual transition of field-based types to property-based types.

I believe this would address @terrajobst's concerns about the use of public fields, at least in the core schema model. There are other uses of GetFields() and field-reflection in the component model/catalog parameterization system which I haven't attempted to address.

Ivanidzo4ka · 2018-07-31T19:56:09Z

+
+            il.Emit(OpCodes.Ldarg_3);               // push arg3
+            il.Emit(OpCodes.Ldarg_1);               // push arg1
+            il.Emit(OpCodes.Call, propertyInfo.GetGetMethod());      // push [stack top].[propertyInfo]


Call [](start = 28, length = 4)

Should it be just call or callvirt?

For F# records "call" was fine, but you're right that for C# get/set we may need callvirt

We could just check the property, right? If the property is virtual or abstract, then callvirt. If it isn't virtual, then just call?

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

Ivanidzo4ka · 2018-07-31T19:56:43Z

+
+            il.Emit(OpCodes.Ldarg_1);               // push arg1
+            il.Emit(OpCodes.Ldarg_2);               // push arg2
+            il.Emit(OpCodes.Call, propertyInfo.GetSetMethod());      // [stack top-1].[propertyInfo] <- [stack top]


Call [](start = 28, length = 4)

Same question regarding Call or Callvirt

likewise, thanks

Ivanidzo4ka · 2018-07-31T20:10:22Z

-                        "Can't bind the IDataView column '{0}' of type '{1}' to field '{2}' of type '{3}'.",
-                        col.ColumnName, realColType, col.FieldInfo.Name, col.FieldInfo.FieldType.FullName);
+                        "Can't bind the IDataView column '{0}' of type '{1}' to field or property '{2}' of type '{3}'.",
+                        col.ColumnName, realColType, col.MemberInfo.Name, col.OutputType.FullName);


OutputType [](start = 78, length = 10)

This look something suspicious. In case of IsComputed = true you will return something else rather than FieldType or PropertyType. #Resolved

OK, will change this

Ivanidzo4ka · 2018-07-31T20:48:15Z

Thank you for your work @dsyme!
I had pretty much same changes in my local repo, but I haven't publish them yet because I wanted to have more test coverage like this:

machinelearning/test/Microsoft.ML.Tests/CollectionDataSourceTests.cs

Line 546 in bdb742d

public void RoundTripConversionWithArrays()

Basically to make sure you can read data into IDataView properly and if you decide to map IDataView on some class it get properly mapped.

I know this is mostly to unblock F#, but I think it would be nice to have tests where you cover private getter/setter, class which inherit property from base class, class which override, readonly property, private property, and I guess lot of other cases, just to knew what allowed and what not.

But if you need to be unblocked by this PR to continue working on F# support, I think tests coverage easily can be separate issue.

eerhardt · 2018-07-31T20:49:16Z

+                userType
+                .GetProperties(BindingFlags.Public | BindingFlags.Instance)
+                .Where(x => x.CanRead && x.CanWrite && x.GetIndexParameters().Length == 0)
+                .ToArray();


Shouldn't need to ToArray here, when you are just going to concat it with fieldInfos and then ToArray that as well.

I'll remove it, thanks

Ivanidzo4ka · 2018-07-31T20:56:17Z

+                var mappingAttr = memberInfo.GetCustomAttribute<ColumnAttribute>();
                if (mappingAttr == null)
-                    throw Contracts.Except($"{field.Name} is missing ColumnAttribute");
+                    throw Contracts.Except($"field or property {memberInfo.Name} is missing ColumnAttribute");


f [](start = 45, length = 1)

It's a totally nitpicking, but we prefer to start our sentences with Capital letter. #Resolved

Ivanidzo4ka · 2018-07-31T20:57:17Z

+                    break;
+
+                default:
+                    throw Contracts.ExceptNotSupp("expected a FieldInfo or a PropInfo");


PropInfo [](start = 77, length = 8)

is it worth to be shorten?
Also can you start sentence with capital letter? #Resolved

(nit) PropInfo => PropertyInfo

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

eerhardt · 2018-07-31T21:04:18Z


-                    if (fieldInfo == null)
+                    if (memberInfo == null)
                        throw Contracts.ExceptParam(nameof(userSchemaDefinition), "No field with name '{0}' found in type '{1}'",


No field => No field or property

eerhardt · 2018-07-31T21:31:58Z

+                    return (Delegate)methInfoProp.Invoke(null, new object[] { propertyInfo, assignmentOpCodeProp });
+
+                default:
+                    throw Contracts.ExceptNotSupp("expected a FieldInfo or a PropInfo");


PropInfo => PropertyInfo

dsyme · 2018-07-31T21:38:49Z

OK, all updated base on code review comments so far, thanks!

@Ivanidzo4ka I've added some initial testing here, is this the sort of thing you were after? https://github.com/dotnet/machinelearning/pull/616/files#diff-de10d1dc0f5785ee3a7f0053c465fbdcR632

eerhardt

I think this is looking in the right direction. Good work, @dsyme.

eerhardt · 2018-07-31T21:48:49Z

+            var mb = new DynamicMethod("Peek", null, args, typeof(TOwn), true);
+            var il = mb.GetILGenerator();
+            var minfo = propertyInfo.GetGetMethod();
+            var opcode = minfo.IsVirtual ? OpCodes.Callvirt : OpCodes.Call;


Do we need to handle IsAbstract?

I'm not 100% sure, I thought IsAbstract implies IsVirtual, but the MSDN docs don't say that so I'll add it

…to fs2

eerhardt · 2018-08-01T15:29:28Z

-            for (int index = 0; index < fields.Length; index++)
+            var userType = typeof(TInput);
+
+            var fieldInfos = userType.GetFields();


Should we be checking for public and instance fields here? Just like we are in GetProperties.

eerhardt

I think this looks good, thanks for the help, @dsyme.

@TomFinley @Zruty0 - any concerns on merging this?

dsyme · 2018-08-01T15:35:23Z

Should we be checking for public and instance fields here? Just like we are in GetProperties.

Public is the default - but yes, I think we should be checking for instance fields (I assume static fields cause failures at the moment) #Closed

Zruty0 · 2018-08-01T15:51:31Z

+
+                    default:
+                        throw Contracts.ExceptNotSupp("Expected a FieldInfo or a PropertyInfo");
+                }


As far as I understand, this clause is only present because the compiler complains that there must be a default clause, so we actually will never trigger it.
If that's the case, I suggest putting Contracts.Assert(false) before throwing, to signify that this is a perceived-never-to-execute code (as opposed to something we don't support not but may in the future). #Closed

Zruty0 · 2018-08-01T15:54:05Z

+                    break;
+
+                default:
+                    throw Contracts.ExceptNotSupp("Expected a FieldInfo or a PropertyInfo");


throw [](start = 20, length = 5)

Same here: if the code is never expected to execute, make it obvious to reader by putting Contracts.Assert(false) ahead of throw. #Closed

Zruty0 · 2018-08-01T15:56:48Z

+
+            il.Emit(OpCodes.Ldarg_3);               // push arg3
+            il.Emit(OpCodes.Ldarg_1);               // push arg1
+            il.Emit(opcode, minfo);                 // push [stack top].[propertyInfo]


push [stack top].[propertyInfo] [](start = 55, length = 31)

Please reconcile the comment with the code #Closed

I'm not quite sure what you mean here? The comment is roughly accurate, based on the similar code for fields #Closed

I meant that the line 111 now emits a 'call' opcode, but the comment still says push [stack top].[propertyInfo]. Oh, but what you mean is that the result of call will be that the property value will end up on top of the stack.
I suppose it's true, but I would expect the comment to look like // call [methodInfo] or something.

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

Zruty0 · 2018-08-01T15:57:05Z

+                    return (Delegate)methInfoProp.Invoke(null, new object[] { propertyInfo });
+
+                default:
+                    throw Contracts.ExceptNotSupp("Expected a FieldInfo or a PropertyInfo");


throw [](start = 20, length = 5)

Contracts.Assert(false) before throwing #Closed

Zruty0 · 2018-08-01T16:00:10Z

I think we should be consistent with public vs instance. If we do so for properties, do so for fields as well.
We should not fail on static properties, they should be ignored I would say.

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

Zruty0 · 2018-08-01T16:02:20Z

In general, we try to merge the completely tested code (unless we're blocking other work, in which case we can use discretion).
I would love to see all tests present. Are you planning to also test for virtual properties?

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

Zruty0 · 2018-08-01T16:03:17Z

In my view, we should require both getter and setter to be public. Either that, or we have to split InternalSchemaDefinition into incoming and outgoing one, and have different requirements for each, but I'd like to avoid this.

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

dsyme · 2018-08-01T18:48:08Z

@Zruty0 All updated - the tests have now been completed #Closed

dsyme · 2018-08-01T18:51:01Z

I will also add one more F# for another way to define F# properties #Closed

dsyme · 2018-08-01T18:53:15Z

I will also add one more F# for another way to define F# properties

Done

Zruty0

Zruty0 · 2018-08-02T15:48:27Z

@dsyme thanks a lot for the contribution! Looks good to me now.

dsyme added 6 commits July 30, 2018 16:12

add F# smoke test

fb536fd

records

e9040e9

update for code review

8ec2fd5

Merge branch 'fs1' into fs2

192c8ba

attempt properties

968766f

allow attributes to be used on properties

9bea7e8

dsyme mentioned this pull request Jul 31, 2018

Support ColumnAttribute on properties and not just fields #254

Closed

dsyme added 2 commits July 31, 2018 14:44

update test case

382906e

force a static reference to Microsoft.ML.Transforms

4e131b5

dsyme mentioned this pull request Jul 31, 2018

Plan for F# bug and testing #540

Closed

2 tasks

Ivanidzo4ka reviewed Jul 31, 2018

View reviewed changes

dsyme added the F# Support of F# language label Jul 31, 2018

eerhardt reviewed Jul 31, 2018

View reviewed changes

Ivanidzo4ka reviewed Jul 31, 2018

View reviewed changes

eerhardt reviewed Jul 31, 2018

View reviewed changes

dsyme added 2 commits July 31, 2018 22:14

fix failing test

31eefbc

fix loading test

b332952

eerhardt reviewed Jul 31, 2018

View reviewed changes

dsyme added 4 commits July 31, 2018 22:31

add some C# testing

47db808

merge master

d214c4e

merge master

dc76490

Merge branch 'fs1' into fs2

f920204

eerhardt reviewed Jul 31, 2018

View reviewed changes

add more C# tests

747e417

dsyme reopened this Aug 1, 2018

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

6487d49

…to fs2

eerhardt reviewed Aug 1, 2018

View reviewed changes

eerhardt approved these changes Aug 1, 2018

View reviewed changes

Zruty0 reviewed Aug 1, 2018

View reviewed changes

dsyme mentioned this pull request Aug 1, 2018

[WIP] Add F# samples and move C# samples to "csharp" dotnet/machinelearning-samples#32

Closed

7 tasks

add more tests

431ad89

add more tests

dd4efe4

Update ApiUtils.cs

3d53559

Zruty0 approved these changes Aug 2, 2018

View reviewed changes

Zruty0 merged commit f6934a0 into dotnet:master Aug 2, 2018

Zruty0 mentioned this pull request Aug 2, 2018

Calculated Feature #595

Closed

This was referenced Aug 2, 2018

Smoke test for F# #600

Closed

F# Records not compatible with ML .NET #180

Closed

Add F# samples dotnet/machinelearning-samples#36

Merged

eerhardt mentioned this pull request Aug 7, 2018

Add release notes for ML.NET 0.4 #656

Merged

This was referenced Oct 18, 2018

Can't use getter/setters in objects #352

Closed

Passing CollectionDataSource w/ SQL dependency throws "System.ArgumentOutOfRangeException: Source column not found" #399

Closed

ghost locked as resolved and limited conversation to collaborators Mar 29, 2022

Conversation

dsyme commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka commented Jul 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyme commented Jul 31, 2018

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

dsyme commented Aug 1, 2018 • edited by Zruty0 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zruty0 Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zruty0 Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zruty0 Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyme Aug 1, 2018 • edited by Zruty0 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zruty0 Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

dsyme commented Jul 31, 2018 •

edited

Loading

Ivanidzo4ka Jul 31, 2018 •

edited

Loading

Ivanidzo4ka Jul 31, 2018 •

edited

Loading

Ivanidzo4ka Jul 31, 2018 •

edited

Loading

dsyme commented Aug 1, 2018 •

edited by Zruty0

Loading

Zruty0 Aug 1, 2018 •

edited

Loading

Zruty0 Aug 1, 2018 •

edited

Loading

Zruty0 Aug 1, 2018 •

edited

Loading

dsyme Aug 1, 2018 •

edited by Zruty0

Loading

Zruty0 Aug 1, 2018 •

edited

Loading

dsyme commented Aug 1, 2018 •

edited by Zruty0

Loading

dsyme commented Aug 1, 2018 •

edited by Zruty0

Loading