Obsolete API #1686

wschin · 2018-11-20T18:32:16Z

We are planning removing classical APIs defined in Microsoft.ML.Legacy. The commits are organized so please read them one-by-one.

Fixes #1684.

1. Disable warning 612 reported in test file 2. Remove Microsoft.ML.PipelineInference project 3. Remove two commands used in pipeline inference. - InferRecipesCommand.cs - InferSchemaCommand.cs Note that we add [Obsolete] to everything in Microsoft.ML.Legacy but auto-generated APIs.

shauheen · 2018-11-20T19:41:34Z

this should fix #1684 #Resolved

eerhardt · 2018-11-20T19:44:33Z

Microsoft.ML.sln

@@ -15,8 +15,6 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.ML.Core.Tests", "
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.ML.CpuMath", "src\Microsoft.ML.CpuMath\Microsoft.ML.CpuMath.csproj", "{46F2F967-C23F-4076-858D-33F7DA9BD2DA}"
 EndProject
-Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.ML.PipelineInference", "src\Microsoft.ML.PipelineInference\Microsoft.ML.PipelineInference.csproj", "{2D7391C9-8254-4B8F-BF26-FADAF8F02F44}"


Doing this will make it not build anymore. Is that what we intend? If yes, we may as well delete the code IMO. Someone can get it from source control history if they need it. #Resolved

Yes, InferencePipeline needs to be removed. I can build my changes locally. Do you mean that CI will still crash? #Resolved

I think "will not build" is meant literally, as in, it will not build, not it cannot build. But of course either way removing it is as @eerhardt suggests is the right thing to do.

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

It's removed in my PR now.

In reply to: 235445964 [](ancestors = 235445964,235221248)

eerhardt · 2018-11-20T19:45:43Z

public static class MacroUtils

Can you just put an [Obsolete] attribute on the whole class? #Resolved

Refers to: src/Microsoft.ML.Legacy/Runtime/EntryPoints/MacroUtils.cs:12 in beb1bb6. [](commit_id = beb1bb6, deletion_comment = False)

1. Make everything in CSharpApi.cs obsolete by modifying CSharpApiGenerator.cs and CSharpGeneratorUtils.cs. 2. Some checked-in [Obsolete] are removed because they conflict with those automatically generated in CSharpApi.cs. 3. Some classes in Microsoft.ML.Legacy are still not obsolete so we addd [Obsolete] to them. 4. Supress warnings 612 and 618 (warning 44 for F# test) for tests and benchmarks.

wschin · 2018-11-21T01:35:58Z

public static class MacroUtils

Sure.

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

Refers to: src/Microsoft.ML.Legacy/Runtime/EntryPoints/MacroUtils.cs:12 in beb1bb6. [](commit_id = beb1bb6, deletion_comment = False)

TomFinley · 2018-11-21T15:58:19Z

public static class OneVersusAllMacro

The others were obsolete, is this one not?

So while the legacy API is bad, we need to get a catalog of those entry-points published in legacy that need to be preserved and migrate them, so that NimbusML or somesuch continues to function. (My belief is that other entry-point consumers predate ML.NET., so those would not be a concern.) I am not certain whether this is one of them or not, but I know some of these are actually used so we must catalog those, and migrate them somehow. I'm not suggesting this as a necessary precondition to your PR, but I'm wondering if we've thought about it.

Refers to: src/Microsoft.ML.Legacy/Runtime/EntryPoints/OneVersusAllMacro.cs:21 in 702fc85. [](commit_id = 702fc85, deletion_comment = False)

TomFinley · 2018-11-21T16:04:33Z

src/Microsoft.ML.Legacy/Runtime/EntryPoints/JsonUtils/ExecuteGraphCommand.cs


 namespace Microsoft.ML.Runtime.EntryPoints.JsonUtils
 {
+    [Obsolete]
    internal sealed class ExecuteGraphCommand : ICommand


ExecuteGraphCommand [](start = 26, length = 19)

General entry-point utilities like this that were incorrectly moved into this doomed project must be borne out. This includes, at least, this command, the graph runner, and I am pretty sure the manifest utils.

So let's draw a distinction, there's the CSharpApi that was generated out of entry-points. That's bad and needs to get removed. The facilities though for entry-points by themselves are not bad and should be preserved. So any "viral" effects from any references to the contents of CSharpApi.cs or any other similarly structured classes (e.g., the duplicate evaluators that were written meant to be used within these pipelines) should be marked obsolete. Similar story for the code-gen. But this by itself should not have been marked obsolete.

Ok. I will only mark things referring to CSharpApi.cs obsolete and move entry point utilities to a new project, Microsoft.ML.EntryPoint.

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

TomFinley · 2018-11-21T16:05:56Z

src/Microsoft.ML.Legacy/Runtime/EntryPoints/ModelOperations.cs


 namespace Microsoft.ML.Runtime.EntryPoints
 {
+    [Obsolete]
    public static class ModelOperations


ModelOperations [](start = 24, length = 15)

This would be an example of a set of entry-points that does not reference the legacy namespaces/projects at all, and should have been borne out somewhere where it will be preserved. I am fairly certain that NimbusML uses this.

wschin · 2018-11-21T17:08:22Z

public static class OneVersusAllMacro

Do you know any efficient way to identify those function used elsewhere? Do I need to go through NimbusML?

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

Refers to: src/Microsoft.ML.Legacy/Runtime/EntryPoints/OneVersusAllMacro.cs:21 in 702fc85. [](commit_id = 702fc85, deletion_comment = False)

TomFinley · 2018-11-21T21:10:56Z

public static class OneVersusAllMacro

If you are looking to identify those entry-points here actually used in NimbusML, assuredly my expectation must be that you would have to look at NimbusML to determine this.

Perhaps it is unclear how to do this. Let's take the one in this file. The entry point name is declared here.

machinelearning/src/Microsoft.ML.Legacy/Runtime/EntryPoints/OneVersusAllMacro.cs

Line 138 in b509add

Name = "Models.OneVersusAll",

NimbusML generates Python calling functions using this autogenerated code, which was placed here in this file models_oneversusall.py.

If we search the NimbusML codebase to see if it is actually used anywhere, we see that it is. We will have to migrate it. Which means that instead of using the autogenerated C# API, we will when migrating this have to make it modify the entry-point graph itself just like macros used to do. (Not saying you'd do this latter work, someone else can, but it's important to at least identify those that must be migrated.)

Other entry-points may be easier; the macros might be the hardest cases. Others don't touch the legacy API at all, and so can be moved now (might be good to do so, or at least identify those that can be moved). would just for now actually move out of legacy since I think many of them will just work.

In reply to: 440741687 [](ancestors = 440741687,440717541)

Refers to: src/Microsoft.ML.Legacy/Runtime/EntryPoints/OneVersusAllMacro.cs:21 in 702fc85. [](commit_id = 702fc85, deletion_comment = False)

1. Things in Microsoft.ML.Legacy.Runtime are not obsolete. If they reference to obsolete CSharpApi.cs, we supress the warning 612. 2. Things in Microsoft.ML.Legacy have two destinies. Anything (a) referring CSharpApi.c, (b) being a part of CSharpApi.cs, or (c) using ILearningPipelineItem.cs are are obsolete. Otherwise, they are not obsolete.

…d project's binary files

wschin · 2018-11-22T00:23:02Z

public static class OneVersusAllMacro

Thank you Tom. I just draft a plan for moving those can be easily moved. The path specified at the left hand side of " ---> " will be moved to the right hand side.

Microsoft.ML.Legacy.Models ---> Microsoft.ML.EntryPoints.Models
a. *Metrics, ConfusionMatrix, and everything not using CSharpApi.cs
Microsoft.ML.Legacy.Runtime.EntryPoints.Codegen ---> Microsoft.ML.EntryPoints.Codegen
Microsoft.ML.Legacy.Runtime.EntryPoints.JsonUtils ---> Microsoft.ML.EntryPoints.JsonUtil
Microsoft.ML.Legacy.Runtime.EntryPoints ---> Microsoft.ML.EntryPoints
a. CVSplit
b. DataViewReference
c. FeatureCombiner
d. ModelOperations
e. TrainTestSplit
Microsoft.ML.Legacy.Experiment ---> Microsoft.ML.EntryPoints.Experiment
a. Experiment
Microsoft.ML.Legacy.Internal.Tools ---> Microsoft.ML.EntryPoints.Tools
Microsoft.ML.Legacy ---> Microsoft.ML.EntryPoints
a. PredictionModel
I will check NimbusML after moving those out.

In reply to: 440809174 [](ancestors = 440809174,440741687,440717541)

Refers to: src/Microsoft.ML.Legacy/Runtime/EntryPoints/OneVersusAllMacro.cs:21 in 702fc85. [](commit_id = 702fc85, deletion_comment = False)

eerhardt · 2018-11-26T19:21:10Z

src/Microsoft.ML.Legacy/Runtime/EntryPoints/MacroUtils.cs

@@ -7,6 +7,9 @@
 using System.Linq;
 using Microsoft.ML.Runtime.Data;

+// The warning #612 is disabled because the following code uses a lot of things in Legacy.Models while Legacy.Model is marked as obsolete.


When are we going to rewrite the below code? Is there an issue tracking it?

This one: #1565.

eerhardt · 2018-11-26T19:22:01Z

public sealed partial class TextLoaderRange

Can the 2 classes in this file be marked [Obsolete]? #Resolved

Refers to: src/Microsoft.ML.Legacy/Data/TextLoader.cs:16 in e831812. [](commit_id = e831812, deletion_comment = False)

eerhardt · 2018-11-26T19:22:38Z

public sealed partial class TextLoaderRange

Oh, nevermind. I see they are partial, and obsoleted in the code gen.

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

Refers to: src/Microsoft.ML.Legacy/Data/TextLoader.cs:16 in e831812. [](commit_id = e831812, deletion_comment = False)

TomFinley

This looks good to me thank you @wschin .

eerhardt · 2018-11-26T19:26:12Z

src/Microsoft.ML.Legacy/AssemblyRegistration.cs

@@ -50,15 +50,6 @@ private static bool LoadStandardAssemblies()
            _ = typeof(SweepCommand).Assembly; // ML.Sweeper
            _ = typeof(OneHotEncodingTransformer).Assembly; // ML.Transforms

-            // The following assemblies reference this assembly, so we can't directly reference them


This one line comment is still valid. Please don't delete it #Resolved

eerhardt · 2018-11-26T19:37:26Z

I think things like https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.Legacy/Models/BinaryClassificationMetrics.cs#L17 need to be marked [Obsolete] as well. #Resolved

eerhardt · 2018-11-26T19:40:20Z

test/Microsoft.ML.Predictor.Tests/Microsoft.ML.Predictor.Tests.csproj

@@ -26,4 +25,8 @@
    <NativeAssemblyReference Include="MklImports" />
  </ItemGroup>

+  <ItemGroup>
+    <Folder Include="Commands\" />


This should be deleted. #Resolved

eerhardt

wschin · 2018-11-26T19:52:29Z

Fixed. Everything under Microsoft.ML.Legacy.Models is obsolete now.

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

wschin added 2 commits November 19, 2018 16:51

Make most things in Microsoft.ML.Legacy assembly obsolete

04201f4

wschin self-assigned this Nov 20, 2018

wschin requested review from Zruty0 and TomFinley November 20, 2018 18:32

eerhardt reviewed Nov 20, 2018

View reviewed changes

wschin added 2 commits November 20, 2018 16:44

Merge branch 'master' into obsolete-api

6aabec6

Minor cleaning

702fc85

wschin changed the title ~~[WIP] Obsolete API~~ Obsolete API Nov 21, 2018

TomFinley reviewed Nov 21, 2018

View reviewed changes

Not to load PipelineInference because it no longer exists

afc2133

wschin added 3 commits November 21, 2018 14:15

Merge branch 'master' into obsolete-api

b7804df

Update CSharpApi.cs again because rebuilding didn't remove the delete…

e831812

…d project's binary files

eerhardt reviewed Nov 26, 2018

View reviewed changes

TomFinley approved these changes Nov 26, 2018

View reviewed changes

eerhardt reviewed Nov 26, 2018

View reviewed changes

eerhardt approved these changes Nov 26, 2018

View reviewed changes

wschin added 3 commits November 26, 2018 11:52

Address comments

0224426

Merge branch 'master' into obsolete-api

e4c1ac5

Merge branch 'master' into obsolete-api

e0c9f76

wschin merged commit 37176a1 into dotnet:master Nov 26, 2018

wschin deleted the obsolete-api branch November 26, 2018 23:05

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

Obsolete API #1686

Obsolete API #1686

wschin commented Nov 20, 2018 •

edited

Loading

shauheen commented Nov 20, 2018 •

edited by wschin

Loading

eerhardt Nov 20, 2018 •

edited by wschin

Loading

wschin Nov 21, 2018 •

edited

Loading

TomFinley Nov 21, 2018

wschin Nov 21, 2018

eerhardt commented Nov 20, 2018 •

edited by wschin

Loading

wschin commented Nov 21, 2018

TomFinley commented Nov 21, 2018 •

edited

Loading

TomFinley Nov 21, 2018 •

edited

Loading

wschin Nov 21, 2018

TomFinley Nov 21, 2018

wschin commented Nov 21, 2018

TomFinley commented Nov 21, 2018

wschin commented Nov 22, 2018

eerhardt Nov 26, 2018

wschin Nov 26, 2018

eerhardt commented Nov 26, 2018 •

edited by wschin

Loading

eerhardt commented Nov 26, 2018 •

edited

Loading

TomFinley left a comment

eerhardt Nov 26, 2018 •

edited by wschin

Loading

eerhardt commented Nov 26, 2018 •

edited by wschin

Loading

eerhardt Nov 26, 2018 •

edited by wschin

Loading

wschin Nov 26, 2018

eerhardt left a comment

wschin commented Nov 26, 2018

Obsolete API #1686

Obsolete API #1686

Conversation

wschin commented Nov 20, 2018 • edited Loading

shauheen commented Nov 20, 2018 • edited by wschin Loading

eerhardt Nov 20, 2018 • edited by wschin Loading

Choose a reason for hiding this comment

wschin Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

TomFinley Nov 21, 2018

Choose a reason for hiding this comment

wschin Nov 21, 2018

Choose a reason for hiding this comment

eerhardt commented Nov 20, 2018 • edited by wschin Loading

wschin commented Nov 21, 2018

TomFinley commented Nov 21, 2018 • edited Loading

TomFinley Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

wschin Nov 21, 2018

Choose a reason for hiding this comment

TomFinley Nov 21, 2018

Choose a reason for hiding this comment

wschin commented Nov 21, 2018

TomFinley commented Nov 21, 2018

wschin commented Nov 22, 2018

eerhardt Nov 26, 2018

Choose a reason for hiding this comment

wschin Nov 26, 2018

Choose a reason for hiding this comment

eerhardt commented Nov 26, 2018 • edited by wschin Loading

eerhardt commented Nov 26, 2018 • edited Loading

TomFinley left a comment

Choose a reason for hiding this comment

eerhardt Nov 26, 2018 • edited by wschin Loading

Choose a reason for hiding this comment

eerhardt commented Nov 26, 2018 • edited by wschin Loading

eerhardt Nov 26, 2018 • edited by wschin Loading

Choose a reason for hiding this comment

wschin Nov 26, 2018

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

wschin commented Nov 26, 2018

wschin commented Nov 20, 2018 •

edited

Loading

shauheen commented Nov 20, 2018 •

edited by wschin

Loading

eerhardt Nov 20, 2018 •

edited by wschin

Loading

wschin Nov 21, 2018 •

edited

Loading

eerhardt commented Nov 20, 2018 •

edited by wschin

Loading

TomFinley commented Nov 21, 2018 •

edited

Loading

TomFinley Nov 21, 2018 •

edited

Loading

eerhardt commented Nov 26, 2018 •

edited by wschin

Loading

eerhardt commented Nov 26, 2018 •

edited

Loading

eerhardt Nov 26, 2018 •

edited by wschin

Loading

eerhardt commented Nov 26, 2018 •

edited by wschin

Loading

eerhardt Nov 26, 2018 •

edited by wschin

Loading