Movement and Internalization Phase 1 #1587

TomFinley · 2018-11-09T07:46:57Z

Internalization and sometimes movement of many types. Ongoing work related to #1519 as we try to limit user exposure to internal infrastructure types.

Move Sweeper types into Sweeper assembly, out of Core.
Internalize all command line related infrastructure, including ArgumentAttribute. Move different types into different files.
Internalize all types in Core's Utils folder.
Considerable cleanup of TimeSeries abstractions to hide them since they made public some things in Core's Utils.
Internalize PFA export support.
Internalize ONNX export support.
Internalize all the base cursor classes.
Internalize all ITreeEnsemble and related types.
Clean up some pointless code.
Make ModelSave/Load contexts uninstantiable.

The commits are mostly structured where I'm removing one sort of thing at a time, so if you want to break it up, just compare adjacent commits. It works pretty well, except sometimes I found I hadn't quite completed the job of a prior commit at a later point and didn't feel like rewriting history, so there is a small amount of pollution.

There's no particular reason why I started where I started or stopped where I stopped, except that it's the end of the day.

TomFinley · 2018-11-09T07:49:21Z

public interface IMlState

I just moved this over here so it wasn't polluting core, but, this needs to go.

Refers to: src/Microsoft.ML.Core/EntryPoints/IMlState.cs:12 in 1898bab. [](commit_id = 1898bab, deletion_comment = False)

src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/MulticlassLogisticRegression.cs

TomFinley · 2018-11-09T07:53:44Z

src/Microsoft.ML.PipelineInference/Microsoft.ML.PipelineInference.csproj

@@ -15,9 +15,9 @@
  <ItemGroup>
    <ProjectReference Include="..\Microsoft.ML.Api\Microsoft.ML.Api.csproj" />
    <ProjectReference Include="..\Microsoft.ML.Core\Microsoft.ML.Core.csproj" />
+    <ProjectReference Include="..\Microsoft.ML.Legacy\Microsoft.ML.Legacy.csproj" />


Hi @justinormont , shortly we will be removing this dependency, since we are preparing to delete that project. Pursuant to #689, any code that cannot be easily rewritten to adapt itself to that will have to be removed.

TomFinley · 2018-11-09T07:58:36Z

src/Microsoft.ML.Data/Model/ModelSaveContext.cs

    /// amount of boiler plate code. It can also be used when saving to a single stream,
-    /// for implementors of ICanSaveInBinaryFormat.
+    /// for implementors of <see cref="ICanSaveInBinaryFormat"/>.
    /// </summary>
    public sealed partial class ModelSaveContext : IDisposable


This is somewhat disappointing. I wanted to actually internalize this class altogether. But, I can't, because of the interface ICanSaveModel, which must be a public interface due to the fact that IDataTransform does and ITransformer should descend from it, and the second at least needs co/contravariance. It may be that I will be able to internalize ModelLoadContext in the future. I already made the constructor on this thing public so people can't just go creating them willy nilly, but I should probably just internalize everything public here with a BestFriend.

Zruty0 · 2018-11-09T18:23:12Z

src/Microsoft.ML.Data/Scorers/SchemaBindablePredictorWrapper.cs

@@ -402,7 +416,7 @@ public override void SaveAsPfa(BoundPfaContext ctx, RoleMappedSchema schema, str
            Contracts.Assert(ctx.TokenOrNullForName(outputNames[1]) == probToken.ToString());
        }

-        public override bool SaveAsOnnx(OnnxContext ctx, RoleMappedSchema schema, string[] outputNames)
+        private protected override sealed bool SaveAsOnnxCore(OnnxContext ctx, RoleMappedSchema schema, string[] outputNames)


sealed [](start = 35, length = 6)

sealed?

I guess that's completely redundant since the class itself is sealed. I'll remove it, though perhaps this can be in phase 2 if things otherwise look nice?

sure

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

Zruty0 · 2018-11-09T18:24:57Z

src/Microsoft.ML.FastTree/TreeEnsembleFeaturizer.cs

@@ -15,6 +15,7 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
+using Microsoft.ML.Runtime.TreePredictor;


using [](start = 0, length = 5)

sort

Zruty0

sfilipi · 2018-11-10T06:01:36Z

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

@@ -4,7 +4,8 @@

 namespace Microsoft.ML.Runtime.CommandLine
 {
-    public static class SpecialPurpose
+    [BestFriend]
+    internal static class SpecialPurpose


internal [](start = 4, length = 8)

maybe a one line comment about what this is used, like you did for the other ones.

I don't know, you mean the other classes in this folder that I bore out of CmdParser.cs? I didn't actually add any documentation to them, if they had documentation before I got there, then I copied it out with the class, but this one I guess didn't have any. If it's all the same to you I'll just leave it as it was since I wasn't quite doing a documentation pass, and since it's internal code I'm not certain I see the urgency? Though in principle it should be documented if this is some concept we want to keep...

sfilipi · 2018-11-10T06:09:58Z

src/Microsoft.ML.Data/Properties/AssemblyInfo.cs

+[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.TimeSeries" + PublicKey.Value)]
+[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.Transforms" + PublicKey.Value)]
+
+[assembly: WantsToBeBestFriends]


WantsToBeBestFriends [](start = 11, length = 20)

:)

My professionalism cannot be doubted! 😄

eerhardt · 2018-11-12T17:37:47Z

public interface IMlState

Can we log an issue for it? Or can it be added to an existing issue for refactoring the EntryPoints infrastructure?

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

Refers to: src/Microsoft.ML.Core/Data/IMlState.cs:12 in 1d61e0b. [](commit_id = 1d61e0b, deletion_comment = False)

eerhardt · 2018-11-12T17:41:21Z

src/Microsoft.ML.Core/Properties/AssemblyInfo.cs

 [assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.Predictor.Tests" + PublicKey.TestValue)]
+[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.InferenceTesting" + PublicKey.TestValue)]
+[assembly: InternalsVisibleTo(assemblyName: "Microsoft.ML.StaticPipelineTesting" + PublicKey.TestValue)]


The one thing I am concerned about with this plan is that our tests will be able to reference the internal types in our product. This means that we won't be able to ensure that we are exposing the necessary public API for our customer scenarios.

Is it possible to only use IVT between our product assemblies, but not between our tests assemblies and the product?

eerhardt · 2018-11-12T17:47:27Z

src/Microsoft.ML.Ensemble/Trainer/IModelCombiner.cs

@@ -7,6 +7,8 @@

 namespace Microsoft.ML.Runtime.Ensemble
 {
+    public delegate void SignatureModelCombiner(PredictionKind kind);


The other "signature" delegates became internal. Can this be internal as well?

TomFinley self-assigned this Nov 9, 2018

TomFinley requested review from shauheen, eerhardt, yaeldekel, sfilipi and Zruty0 November 9, 2018 07:46

TomFinley changed the title ~~Tfinley/move sweeper~~ Movement and Internalization Phase 1 Nov 9, 2018

TomFinley commented Nov 9, 2018

View reviewed changes

src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/MulticlassLogisticRegression.cs Show resolved Hide resolved

TomFinley commented Nov 9, 2018

View reviewed changes

TomFinley force-pushed the tfinley/MoveSweeper branch from 140920f to 95734e2 Compare November 9, 2018 17:33

Zruty0 reviewed Nov 9, 2018

View reviewed changes

Zruty0 approved these changes Nov 9, 2018

View reviewed changes

TomFinley added 12 commits November 9, 2018 13:12

Internalization of Utils. Move out Sweeper types.

0bd3788

Internalize command line related infrastructure.

325727a

Remove a handful pointless interfaces.

3ed0828

Internalize base cursor classes.

d579200

Internalize PFA support.

8478934

Finish internalizing PFA support. Internalize ONNX support.

9c9c3a4

Polish on PFA and creation of model save/load contexts.

fe52676

Internalize ITree and related types.

4908a87

Remove misuse of CheckAtModel.

5891083

Finish forgotten utils since I apparently got distracted

e3e2e51

Contracts, misc fix

0d3a10b

Adjust code analyzer test to account for contracts now being internal

1d61e0b

TomFinley force-pushed the tfinley/MoveSweeper branch from 47135b7 to 1d61e0b Compare November 9, 2018 21:45

sfilipi reviewed Nov 10, 2018

View reviewed changes

sfilipi approved these changes Nov 10, 2018

View reviewed changes

TomFinley merged commit 2a029ea into dotnet:master Nov 10, 2018

TomFinley deleted the tfinley/MoveSweeper branch November 10, 2018 16:05

eerhardt reviewed Nov 12, 2018

View reviewed changes

TomFinley mentioned this pull request Nov 15, 2018

Movement and Internalization Phase 2 #1626

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Movement and Internalization Phase 1 #1587

Movement and Internalization Phase 1 #1587

TomFinley commented Nov 9, 2018 •

edited

Loading

TomFinley commented Nov 9, 2018

TomFinley Nov 9, 2018

TomFinley Nov 9, 2018

Zruty0 Nov 9, 2018

TomFinley Nov 9, 2018

Zruty0 Nov 9, 2018

Zruty0 Nov 9, 2018

Zruty0 left a comment

sfilipi Nov 10, 2018

TomFinley Nov 10, 2018

sfilipi Nov 10, 2018

TomFinley Nov 10, 2018

eerhardt commented Nov 12, 2018

eerhardt Nov 12, 2018

eerhardt Nov 12, 2018

Movement and Internalization Phase 1 #1587

Movement and Internalization Phase 1 #1587

Conversation

TomFinley commented Nov 9, 2018 • edited Loading

TomFinley commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Nov 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley commented Nov 9, 2018 •

edited

Loading