Isolate ONNX implementations in separate DLL #462

TomFinley · 2018-06-30T07:02:01Z

Fixes #162.

Previously, the ONNX infrastructure and implementations (including refs to protobuf) were in a central DLL. This gave us a dependency on a separate somewhat large project (protobuf), that was only of interest to people saving ONNX models.

By having the components save themselves through abstract classes rather than actual instantiable classes (OnnxContext became an abstract class with a hidden OnnxContextImpl implementation, NodeProto and OnnxUtils became the OnnxNode abstract class with, likewise, a hidden implementation), there is no need for any "direct" dependency on protobuf.

All implementation classes become internal classes of the Microsoft.ML.Onnx project. (This was previously called Microsoft.ML.UniversalFormat due to historical reasons that no longer make sense.) The only public classes in that project are the entry-points and commands inside SaveOnnxCommand.cs, which instantiate actual implementors of those interfaces, then pass to ONNX savable components.

Also, I opportunistically improved documentation on those public interfaces (though even with docs the interfaces would scarcely make sense to someone unfamiliar with ONNX), and improved the code.

TomFinley · 2018-06-30T07:09:39Z

src/Microsoft.ML.Data/DataLoadSave/CompositeDataLoader.cs

@@ -87,7 +86,7 @@ private static VersionInfo GetVersionInfo()
        /// Returns the underlying data view of the composite loader.
        /// This can be used to programmatically explore the chain of transforms that's inside the composite loader.
        /// </summary>
-        internal IDataView View { get { return _view; } }
+        public IDataView View { get; }


public [](start = 8, length = 6)

Not sure about this one..

TomFinley · 2018-06-30T07:10:01Z

/// This data model component is savable as Onnx.

Should be ONNX. #Closed

Refers to: src/Microsoft.ML.Data/Model/Onnx/ICanSaveOnnx.cs:22 in 23f62cf. [](commit_id = 23f62cf, deletion_comment = False)

TomFinley · 2018-06-30T16:31:54Z

Build OSX10.13 Release #Closed

TomFinley · 2018-07-02T16:18:05Z

@dotnet-bot test OSX10.13 Release

Ivanidzo4ka · 2018-07-02T19:22:22Z

src/Microsoft.ML.Data/Model/Onnx/IOnnxNode.cs

+    {
+        void AddAttribute(string argName, double value);
+        void AddAttribute(string argName, IEnumerable<double> value);
+        void AddAttribute(string argName, IEnumerable<float> value);


IEnumerable value); [](start = 42, length = 26)

IEnumerable value); [](start = 42, length = 26)

so everyone has a pair of T value and IEnumerable method, but float has only IEnumerable
I don't know is it wrong or not, but it just stands out. #Resolved

Yes. I think it's because double param will work for both float and double, but IEnumerable<double> will only work for IEnumerable<double>, not also IEnumerable<float>. #Resolved

Ivanidzo4ka · 2018-07-02T19:24:30Z

src/Microsoft.ML.Data/Model/Onnx/IOnnxNode.cs

+        void AddAttribute(string argName, IEnumerable<DvText> value);
+        void AddAttribute(string argName, IEnumerable<string> value);
+        void AddAttribute(string argName, string value);
+        void AddAttribute(string argName, bool value);


My "OCD" wants them to be formatted in either all T value methods first, and then all IEnumerable values methods, or as couples of T value method + IEnumerable value.
#Resolved

eerhardt · 2018-07-02T19:55:42Z

pushing the DLL into its own nuget so we could remove the protobuf dependency. However I am not sure how to do that. Maybe @eerhardt can help me here.

Sure, it is actually pretty simple once you know how. You can duplicate/emulate #392, which adds the LightGBM nupkg.

Add <packageName>.nupkgproj and <packageName>.symbols.nupkgproj files under a pkg\<packageName> folder.
Set the TFM, PackageDescription properties and references in <packageName.nupkgproj. (Be sure to ProjectReference="Microsoft.ML.nupkgproj").
In your Microsoft.ML.Onnx.csproj, set <IncludeInPackage>Microsoft.ML.Onnx</IncludeInPackage> to put the output of that .csproj in the appropriate NuGet package.

eerhardt · 2018-07-02T20:02:41Z

src/Microsoft.ML.Data/Model/Onnx/IOnnxContext.cs

+    /// 
+    /// 
+    /// </summary>
+    public interface IOnnxContext


Would it be better to make an abstract class instead? That way we can add more members to this type in the future without having to make an IOnnxContext2 type.

Maybe, maybe. Though in this case, given that the point is that I want none of its implementation to be in here, I'm not certain that I see the benefit of a base class. Is there some article I could read that expresses this principle?

I think what I'd like to do is lock down this interface/base-class. Is there some way I can do that? Make a base class that no one can easily descend from, except some sort of "approved" implementing assembly? (In this case the onnx project, and also possibly some sort of test assembly.)

https://msdn.microsoft.com/en-us/library/ms229013(v=vs.100).aspx

Do favor defining classes over interfaces.
In later versions of your library, you can safely add new members to classes; you cannot add members to interfaces without breaking existing code.
Do use abstract (MustInherit in Visual Basic) classes instead of interfaces to decouple the contract from implementations.

Also, this isn't official msdn, but it appears to be copied from the book

https://www.developer.com/design/article.php/10925_3573356_3/Type-Design-Guidelines-for-Reusable-NET-Libraries.htm.

Is there some way I can do that? Make a base class that no one can easily descend from, except some sort of "approved" implementing assembly?

You can't do it with an interface, as anyone can implement the interface. But you can do it with an abstract class by making the constructor internal.

I'm not sure I get the point about interfaces vs. base classes. If the class would only have abstract methods (as these would), I'm not sure I see how the problem is any better with base classes. And to the extent that you can "solve" it by introducing implementations of methods you introduce the fragile base class problem.

internal from what I understand works only for restricting access to the same assembly. This certainly doesn't cover this case, since the entire point is that the implementation should be in a different assembly. Anything based on signed DLLs, perhaps?

If the class would only have abstract methods (as these would), I'm not sure I see how the problem is any better with base classes.

The point is, you can add members (properties, non-abstract methods) to a class without being a breaking change. New methods can be virtual, and your implementation class override them. And the abstract class can no-op, throw, return a default value, etc.
Once you ship an interface, you can never add new members to it.

Now in this case, if you restrict the class so that ONLY you can inherit from it (like using an internal constructor), then you can also add abstract methods to the base class in future versions. Because you know that no one else can inherit from it.

internal from what I understand works only for restricting access to the same assembly. This certainly doesn't cover this case, since the entire point is that the implementation should be in a different assembly.

You can use InternalsVisibleTo to get around this, if you want.

eerhardt · 2018-07-02T20:04:03Z

src/Microsoft.ML.Data/Model/Onnx/IOnnxNode.cs

+    /// That method creates a with inputs and outputs, but this object can modify the node further
+    /// by adding attributes (in ONNX parlance, attributes are more or less constant parameterizations).
+    /// </summary>
+    public interface IOnnxNode


Similar question as IOnnxContext. Would it be better if this was an abstract class?

TomFinley · 2018-07-02T22:51:32Z

All right, I think I did it correctly, let me know. But I'm not certain. How can I test it out? So if I just run build.cmd everything builds successfully, but then again if I run it after introducing deliberate errors into the nupkgproj or the project file to include in some non-existent package it also builds without complaint.

I guess I've never tried to build the nugets directly from here, not sure if there's some other script other than build.cmd I need to do that.

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

eerhardt · 2018-07-02T22:54:50Z

I guess I've never tried to build the nugets directly from here, not sure if there's some other script other than build.cmd I need to do that.

See https://github.com/dotnet/machinelearning/blob/master/docs/building/windows-instructions.md#building-from-the-command-line

build.cmd - builds the assemblies
build.cmd -runTests - builds the assemblies and runs tests
build.cmd -buildPackages called after a normal “build.cmd” will create the NuGet packages with the assemblies in “bin"

TomFinley · 2018-07-02T22:58:02Z

Ah nice. I feel like I should have known that. Yes seems to work. Thanks Eric!

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

TomFinley · 2018-07-03T00:36:27Z

@dotnet-bot test OSX10.13 Release

eerhardt · 2018-07-03T15:14:17Z

pkg/Microsoft.ML.Onnx/Microsoft.ML.Onnx.nupkgproj

+
+  <PropertyGroup>
+    <TargetFramework>netstandard2.0</TargetFramework>
+    <PackageDescription>ML.NET component for Exporting ONNX Models</PackageDescription>


Do we envision using this package/library for more than just exporting an ONNX model? For example, if I want to load/execute an ONNX model, would that be through a separate library?

(nit) does Exporting need to be capitalized?

Do we envision using this package/library for more than just exporting an ONNX model? For example, if I want to load/execute an ONNX model, would that be through a separate library?

I'm not certain. If I had to guess, this hypothetical execution would involve linking to some native library in some fashion (Lotus?). I don't see much opportunity for code reuse in that case of course, so that suggests a different library.

does Exporting need to be capitalized?

No.

Sounds good. We can adjust the package description later, if we add other functionality.

eerhardt · 2018-07-03T15:21:19Z

src/Microsoft.ML.Data/Model/Onnx/OnnxContext.cs

+        /// <param name="name">The name of the operator, which ought to be something returned from <see cref="OnnxContext.GetNodeName(string)"/></param>
+        /// <param name="domain">The domain of the ONNX operator, if non-default</param>
+        /// <returns>A node added to the in-progress ONNX graph, that attributes can be set on</returns>
+        public static OnnxNode CreateNode(this OnnxContext ctx,


Do we need this to be an extension method anymore? Can't it just be a regular method on OnnxContext?

…implementation to separate DLL.

Remove needless delegate

Forgot one dependency in Tests -> Onnx

…Review comments.

* Improve ML.NET version handling to actually rely on the assembly vs. a constant string we have to change. * Tighten checks of arguments to the context implementation.

Ivanidzo4ka · 2018-07-03T22:08:10Z

src/Microsoft.ML.Data/Model/Onnx/OnnxContext.cs

+    /// given to a component, all other components up to that component have already attempted to express themselves in
+    /// this context, with their outputs possibly available in the ONNX graph.
+    /// 
+    /// 


you probably don't need this lines.

Probably not. But you never know.

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

Ivanidzo4ka · 2018-07-03T22:21:12Z

src/Microsoft.ML.Onnx/OnnxContextImpl.cs

+                }
+            }
+
+            string columnName = _columnNameMap.Single(kvp => string.Compare(kvp.Value, variableName) == 0).Key;


string.Compare(kvp.Value, variableName) == 0 [](start = 61, length = 44)

curious, why you prefer string.Compare to just '==' #Resolved

I don't, not quite sure what happened here. I'm fairly certain I wouldn't have written that...

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

Ivanidzo4ka

TomFinley · 2018-07-04T00:23:21Z

@dotnet-bot test Linux Debug

eerhardt

Abstraction of ONNX exporting to interfaces, and isolation of actual implementation to separate DLL. Creation of a new NuGet to isolate Protobuf dependency.

TomFinley requested review from codemzs, Ivanidzo4ka, shauheen, eerhardt and yaeldekel June 30, 2018 07:02

TomFinley commented Jun 30, 2018

View reviewed changes

TomFinley requested a review from dotnet-bot June 30, 2018 20:47

Ivanidzo4ka reviewed Jul 2, 2018

View reviewed changes

eerhardt reviewed Jul 2, 2018

View reviewed changes

TomFinley force-pushed the tfinley/OnnxIsolation branch from 0677f4f to 23f62cf Compare July 2, 2018 22:47

TomFinley force-pushed the tfinley/OnnxIsolation branch 2 times, most recently from f25025a to 4851898 Compare July 3, 2018 15:02

eerhardt reviewed Jul 3, 2018

View reviewed changes

Tom Finley and others added 6 commits July 3, 2018 09:27

Abstraction of ONNX exporting to interfaces, and isolation of actual …

0de0311

…implementation to separate DLL.

Documentation changes, minor bug fixed

671084e

Remove needless delegate

Change "universal model format" to "ONNX".

971eda6

Forgot one dependency in Tests -> Onnx

Add reference to Core.Tests to ONNX project for entry-point catalog

4bed416

Onnx -> ONNX in prose text

1ba5cbd

Update console project reference to use ONNX project. Add new nuget. …

d32598c

…Review comments.

TomFinley and others added 2 commits July 3, 2018 09:27

* Make extension points abstract classes.

8d1c469

* Improve ML.NET version handling to actually rely on the assembly vs. a constant string we have to change. * Tighten checks of arguments to the context implementation.

CreateNode now regular method. Exporting => exporting.

9429d88

TomFinley force-pushed the tfinley/OnnxIsolation branch from 4851898 to 9429d88 Compare July 3, 2018 16:34

Ivanidzo4ka reviewed Jul 3, 2018

View reviewed changes

Ivanidzo4ka approved these changes Jul 3, 2018

View reviewed changes

Remove extra lines, no string.Compare silliness

689f1d9

eerhardt approved these changes Jul 5, 2018

View reviewed changes

TomFinley merged commit 52cc874 into dotnet:master Jul 5, 2018

TomFinley deleted the tfinley/OnnxIsolation branch July 5, 2018 20:07

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Isolate ONNX implementations in separate DLL #462

Isolate ONNX implementations in separate DLL #462

TomFinley commented Jun 30, 2018 •

edited

Loading

TomFinley Jun 30, 2018

TomFinley commented Jun 30, 2018 •

edited

Loading

TomFinley commented Jun 30, 2018 •

edited

Loading

TomFinley commented Jul 2, 2018

Ivanidzo4ka Jul 2, 2018 •

edited by TomFinley

Loading

TomFinley Jul 2, 2018 •

edited

Loading

Ivanidzo4ka Jul 2, 2018 •

edited by TomFinley

Loading

eerhardt commented Jul 2, 2018

eerhardt Jul 2, 2018

TomFinley Jul 2, 2018

TomFinley Jul 2, 2018

eerhardt Jul 2, 2018

TomFinley Jul 3, 2018

eerhardt Jul 3, 2018

eerhardt Jul 2, 2018

TomFinley Jul 2, 2018

TomFinley commented Jul 2, 2018

eerhardt commented Jul 2, 2018

TomFinley commented Jul 2, 2018

TomFinley commented Jul 3, 2018

eerhardt Jul 3, 2018

TomFinley Jul 3, 2018

eerhardt Jul 3, 2018

eerhardt Jul 3, 2018

Ivanidzo4ka Jul 3, 2018

TomFinley Jul 4, 2018

Ivanidzo4ka Jul 3, 2018 •

edited by TomFinley

Loading

TomFinley Jul 4, 2018 •

edited

Loading

Ivanidzo4ka left a comment

TomFinley commented Jul 4, 2018

eerhardt left a comment

Isolate ONNX implementations in separate DLL #462

Isolate ONNX implementations in separate DLL #462

Conversation

TomFinley commented Jun 30, 2018 • edited Loading

Choose a reason for hiding this comment

TomFinley commented Jun 30, 2018 • edited Loading

TomFinley commented Jun 30, 2018 • edited Loading

TomFinley commented Jul 2, 2018

Ivanidzo4ka Jul 2, 2018 • edited by TomFinley Loading

Choose a reason for hiding this comment

TomFinley Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Jul 2, 2018 • edited by TomFinley Loading

Choose a reason for hiding this comment

eerhardt commented Jul 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley commented Jul 2, 2018

eerhardt commented Jul 2, 2018

TomFinley commented Jul 2, 2018

TomFinley commented Jul 3, 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

Ivanidzo4ka Jul 3, 2018 • edited by TomFinley Loading

Choose a reason for hiding this comment

TomFinley Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

TomFinley commented Jul 4, 2018

eerhardt left a comment

Choose a reason for hiding this comment

TomFinley commented Jun 30, 2018 •

edited

Loading

TomFinley commented Jun 30, 2018 •

edited

Loading

TomFinley commented Jun 30, 2018 •

edited

Loading

Ivanidzo4ka Jul 2, 2018 •

edited by TomFinley

Loading

TomFinley Jul 2, 2018 •

edited

Loading

Ivanidzo4ka Jul 2, 2018 •

edited by TomFinley

Loading

Ivanidzo4ka Jul 3, 2018 •

edited by TomFinley

Loading

TomFinley Jul 4, 2018 •

edited

Loading