Make TextLoader.ArgumentsCore.Separator internal #2059

mareklinka · 2019-01-07T20:09:48Z

Since TextLoader.ArgumentsCore.Separator is only used for the command line interface, #2041 aims to make the field internal so that it doesn't show for API users. In order to facilitate this hiding, the CmdParser.GetArgumentInfo had to be updated to reflect on both public and private fields of the provided type.

At the same time, the API-serving SeparatorChars field was renamed to simply Separators.

Work in progress. More fields might be turned internal, depending on discussion in the issue.

Fixes #2041

Allow CmdParser.GetArgumentInfo to search through non-public fields

TomFinley · 2019-01-09T20:33:40Z

src/Microsoft.ML.Core/CommandLine/CmdParser.cs

@@ -466,7 +466,7 @@ private static ArgumentInfo GetArgumentInfo(Type type, object defaults)
            var map = new Dictionary<string, Argument>();
            Argument def = null;

-            foreach (FieldInfo field in type.GetFields())
+            foreach (FieldInfo field in type.GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance))


BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance [](start = 55, length = 68)

Cool. Thank you for working on this @mareklinka! As mentioned in the associated issue, we might need to do something like this in one or two more places.

mareklinka · 2019-01-12T09:49:45Z

As mentioned, I refactored the LightGbmArguments.UpdateParameters method to pay attention to the ArgumentAttribute. I wrote three simple unit tests as a safety net for the refactoring, these should be removed before merging as they don't provide any real value (and increase maintenance costs later on).

I was unsure whether to do any additional checks on the attribute (e.g. check Visibility, similar to what CmdParser.GetArgumentInfo does) - let me know if anything is necessary here, I'll add it.

cc @TomFinley

Ivanidzo4ka · 2019-01-16T03:33:05Z

Can you also touch

machinelearning/src/Microsoft.ML.Data/EntryPoints/InputBuilder.cs

Line 61 in 0c62e30

foreach (var fieldInfo in _type.GetFields())

? #Resolved

mareklinka · 2019-01-16T09:07:20Z

Just to let everyone involved here know, I will be be out of range of a computer for the next 7 days so if more changes are required, I'll work on them once I get back.

sfilipi · 2019-01-17T05:50:51Z

src/Microsoft.ML.LightGBM/LightGbmArguments.cs

+                    if (attribute == null)
+                    {
+                        continue;
+                    }


} [](start = 20, length = 1)

stylistic nit: no { } for one line instructions

Right, force of habit. Fixed.

sfilipi · 2019-01-17T05:51:09Z

src/Microsoft.ML.LightGBM/LightGbmArguments.cs

+
+            private ArgumentAttribute GetAttribute(FieldInfo field)
+            {
+                return field.GetCustomAttribute<ArgumentAttribute>(false);


since it is used only inside the function above, keep it as a delegate inside that function.

sfilipi · 2019-01-17T05:52:52Z

test/Microsoft.ML.Predictor.Tests/SmokeTests.cs

+
+namespace Microsoft.ML.RunTests
+{
+    public class SmokeTests


public class SmokeTests [](start = 3, length = 24)

were you planning to remove those?

sfilipi · 2019-01-17T05:53:53Z

thanks for working on this @mareklinka. It looks good to me, and i'll be good to sign off as soon as the SmokeTests.cs is gone.

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

Small style fixes

mareklinka · 2019-01-24T14:26:55Z

Alright, hopefully that should be everything. Removed the smoke tests and fixed the style issues. Merged with current master and resolved conflicts. Let's see if it checks out.

codecov · 2019-01-24T15:01:25Z

Codecov Report

Merging #2059 into master will decrease coverage by 0.01%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master    #2059      +/-   ##
==========================================
- Coverage   66.08%   66.06%   -0.02%     
==========================================
  Files         638      638              
  Lines      116423   116428       +5     
  Branches    14846    14847       +1     
==========================================
- Hits        76939    76921      -18     
- Misses      35231    35257      +26     
+ Partials     4253     4250       -3

TomFinley

This looks good to me, thank you @mareklinka! Sorry for delay...

Ivanidzo4ka

Make TextLoader.ArgumentsCore.Separator internal

fd01602

Allow CmdParser.GetArgumentInfo to search through non-public fields

TomFinley requested review from sfilipi, codemzs, yaeldekel, Ivanidzo4ka and TomFinley and removed request for sfilipi January 9, 2019 20:32

TomFinley reviewed Jan 9, 2019

View reviewed changes

Add support for reflecting internal fields from argument types

2987cd9

mareklinka force-pushed the feature/2041_HideSeparator branch from 16a5d4a to 2987cd9 Compare January 10, 2019 20:55

Refactor LightGbmArguments to pay attention to ArgumentAttribute

2c364dd

Make JsonUtils.InputBuilder reflect non-public fields

64166f8

najeeb-kazmi mentioned this pull request Jan 16, 2019

Make array argument names plural #2124

Merged

sfilipi reviewed Jan 17, 2019

View reviewed changes

mareklinka force-pushed the feature/2041_HideSeparator branch from 8ba84b7 to a2b2ebc Compare January 24, 2019 13:58

Remove smoke tests

f01c12f

Small style fixes

mareklinka force-pushed the feature/2041_HideSeparator branch from a2b2ebc to f01c12f Compare January 24, 2019 13:59

mareklinka changed the title ~~[WIP] Make TextLoader.ArgumentsCore.Separator internal~~ Make TextLoader.ArgumentsCore.Separator internal Jan 24, 2019

Merge branch 'master' into feature/2041_HideSeparator

a5da127

TomFinley approved these changes Jan 24, 2019

View reviewed changes

Ivanidzo4ka approved these changes Jan 24, 2019

View reviewed changes

TomFinley merged commit 1dd8c83 into dotnet:master Jan 25, 2019

mareklinka deleted the feature/2041_HideSeparator branch February 4, 2019 15:51

duzenko mentioned this pull request Feb 15, 2019

Example in README.md does not compile #2565

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make TextLoader.ArgumentsCore.Separator internal #2059

Make TextLoader.ArgumentsCore.Separator internal #2059

Uh oh!

mareklinka commented Jan 7, 2019

Uh oh!

TomFinley Jan 9, 2019 •

edited

Loading

Uh oh!

mareklinka commented Jan 12, 2019

Uh oh!

Ivanidzo4ka commented Jan 16, 2019 •

edited

Loading

Uh oh!

mareklinka commented Jan 16, 2019

Uh oh!

sfilipi Jan 17, 2019

Uh oh!

mareklinka Jan 24, 2019

Uh oh!

sfilipi Jan 17, 2019 •

edited

Loading

Uh oh!

sfilipi Jan 17, 2019

Uh oh!

mareklinka Jan 24, 2019

Uh oh!

sfilipi commented Jan 17, 2019

Uh oh!

mareklinka commented Jan 24, 2019

Uh oh!

codecov bot commented Jan 24, 2019

Uh oh!

TomFinley left a comment

Uh oh!

Ivanidzo4ka left a comment

Uh oh!

Uh oh!

Make TextLoader.ArgumentsCore.Separator internal #2059

Make TextLoader.ArgumentsCore.Separator internal #2059

Uh oh!

Conversation

mareklinka commented Jan 7, 2019

Uh oh!

TomFinley Jan 9, 2019 • 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!

mareklinka commented Jan 12, 2019

Uh oh!

Ivanidzo4ka commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mareklinka commented Jan 16, 2019

Uh oh!

sfilipi Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

mareklinka Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

sfilipi Jan 17, 2019 • 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!

sfilipi Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

mareklinka Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

sfilipi commented Jan 17, 2019

Uh oh!

mareklinka commented Jan 24, 2019

Uh oh!

codecov bot commented Jan 24, 2019

Codecov Report

Uh oh!

TomFinley left a comment

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TomFinley Jan 9, 2019 •

edited

Loading

Ivanidzo4ka commented Jan 16, 2019 •

edited

Loading

sfilipi Jan 17, 2019 •

edited

Loading