Remove and combine Microsoft.ML.UniversalModelFormat.Onnx with Microsoft.ML.Model.OnnxConverter.

[codemzs]

fixes #2721

[sfilipi]

I think it should be Microsoft.ML.Model.Onnx, just like the other namespace. The only type it contains, OnnxCSharpToProtoWrapper is internal. If we put it in a separate namespace, with just internal types, will appear as an empty namespace to the users.

[codemzs]

@sfilipi Some of us(@Ivanidzo4ka and @TomFinley ) had a discussion before to not have Microsoft.ML.Onnx namespace since its ambiguous because we have OnnxTransformer and OnnxCoverter now. That conversation let to the removal of Onnx namespace and creation of OnnxCoverter and OnnxTransformer namespace. Microsoft.ML.UniversalModelFormat.Onnx namespace is part of OnnxCoverter hence it needs to come under OnnxCoverter namespace.

[codecov]

Codecov Report

Merging #2722 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2722      +/-   ##
==========================================
+ Coverage   71.66%   71.68%   +0.01%     
==========================================
  Files         808      808              
  Lines      142253   142253              
  Branches    16138    16138              
==========================================
+ Hits       101950   101970      +20     
+ Misses      35864    35840      -24     
- Partials     4439     4443       +4

Flag	Coverage Δ
#Debug	71.68% <100%> (+0.01%)	⬆️
#production	67.93% <100%> (+0.01%)	⬆️
#test	85.85% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
src/Microsoft.ML.OnnxConverter/OnnxNodeImpl.cs	88.23% <ø> (ø)	⬆️
src/Microsoft.ML.OnnxConverter/OnnxContextImpl.cs	72% <ø> (ø)	⬆️
test/Microsoft.ML.Tests/OnnxConversionTest.cs	97.16% <ø> (ø)	⬆️
...Microsoft.ML.OnnxConverter/OnnxExportExtensions.cs	90.9% <ø> (ø)	⬆️
src/Microsoft.ML.OnnxConverter/SaveOnnxCommand.cs	68.35% <ø> (ø)	⬆️
src/Microsoft.ML.OnnxConverter/OnnxUtils.cs	80.68% <100%> (ø)	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	82.66% <0%> (-0.21%)	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	85.46% <0%> (-0.17%)	⬇️
src/Microsoft.ML.Maml/MAML.cs	26.21% <0%> (+1.45%)	⬆️
src/Microsoft.ML.Core/Data/ProgressReporter.cs	77.94% <0%> (+6.98%)	⬆️

[TomFinley]

@sfilipi Some of us(@Ivanidzo4ka and @TomFinley ) had a discussion before to not have Microsoft.ML.Onnx namespace since its ambiguous because we have OnnxTransformer and OnnxCoverter now. That conversation let to the removal of Onnx namespace and creation of OnnxCoverter and OnnxTransformer namespace. Microsoft.ML.UniversalModelFormat.Onnx namespace is part of OnnxCoverter hence it needs to come under OnnxCoverter namespace.

I think though @sfilipi's point is orthogonal. While it may be that I may have said this, I think my point was related to the assembly names and the names of nugets, not the names of the namespaces. Let's imagine a hypothetical world where the namespace was Microsoft.ML.Onnx. In a world where both namespaces were used for this, this would not interfere with my point about the need for separate assemblies. Indeed, I think we have defined classes in separate assemblies and nugets in the same namespace (the extension methods on MLContext being the most obvious example). So let us treat it as established that these are different matters.

I am not saying you are wrong and she is right -- I would need more thought to determine this -- I just think there was a misinterpretation about what I had said, and its scope. So therefore do not use my utterance in that PR or issue to determine what you think is correct or incorrect in this specific context, because I think it is based on a potential misinterpretation of the scope of my words.

[TomFinley]

Actually, there's a more germane point that I'd somehow forgotten in my first reply. @sfilipi as far as I can tell has shown greatest authority in the the namespace reconciliation effort, so we might take her word about what namespace should go where as having more authority in this matter. Not to say she is incontrovertible, any more than I am, but we might defer unless there is a strong reason to not. Which as far as I can tell in this case, does not exist. So, let our default mode to be to defer in matters of renaming.

[codemzs]

@TomFinley Not only did I change the name of the assembly from Microsoft.ML.Onnx to Microsoft.ML.OnnxConverter but I also changed the namespace in that assembly from Microsoft.ML.Model.Onnx to Microsoft.ML.Model.OnnxConverter, here is the PR that you signed-off on. It does not make sense to have a different namespace for a file in a project when its purpose is the same as other files. I strongly feel we should keep the namespace consistent and not introduce a new namespace for OnnxML.cs whose sole purpose is to create protobuf file.

Lets also take a step back and see where @sfilipi is coming from. @sfilipi probably noticed all the files in Microsoft.ML.Onnx assembly had the namespace Microsoft.ML.Model.Onnx except for OnnxMl.cs file which had its own Microsoft.ML.UniversalModelFormat.Onnx namespace and hence she proposed we change the namespace in OnnxML.cs to Microsoft.ML.Model.Onnx to keep it consistent. Later I changed Microsoft.ML.Model.Onnx namespace to Microsoft.ML.Model.OnnxConverter to make it more specific since we also changed the name of assembly from Microsoft.ML.Onnx to Microsoft.ML.OnnxConverter but forgot to change Microsoft.ML.UniversalModelFormat.Onnx namespace in OnnxMl.cs.

[TomFinley]

Just because I sign off on a PR that touches a file doesn't mean that I am saying that this file cannot be changed forever. Generally, I consider the finalization of the API to be an iterative process. If a PR gets us 90% of the way to where we want to be, I will still generally sign off, because I know the 10% can be done later. (E.g., work around calibrators, and wahtnot.) Regarding this *specifically I did not offer specific comment on the choice of this namespace. I also do not have a strong opinion about namespace names, but I think @sfilipi has been tracking this more carefully. So my point is that referring to me as an authority to deny a renaming request, is what I am trying to say we should not do. :)

For sure we have not insisted elsewhere that namespaces and assemblies need to be isolated. In fact we have considered it a bad idea. (See the usage of Microsoft.ML namespace.) Also if we wanted these two to somehow "work together" at some future point (which is entirely reasonable), we'd probably want them to have the same namespace. So I am somehat sympathetic to @sfilipi's argument. What do you think?

---

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

[codemzs]

@TomFinley Not sure what you are asking here but thanks for your input. I’ll discuss this offline tomorrow and resolve this issue.

[sfilipi]

The reason i proposed Microsoft.ML.Model.Onnx a month ago, is because most of the types dealing with Onnx were in that namespace.

I wrote the comment above under the impression that @codemzs was creating another namespace, without realizing that Microsoft.ML.Model.Onnx had changed to Microsoft.ML.Model.OnnxConverter.

Reading through the thread, and checking on the current state of the solution; the only namespace that groups Onnx converting types, is Microsoft.ML.Model.OnnxConverter. The Onnx Transform is in Microsoft.ML.Transforms; so IMO having those classes moved to Microsoft.ML.Model.OnnxConverter makes sense, and all those types being moved are internal.

I'll double-check my understanding of at @TomFinley comment about the two namespaces that can be separate assemblies, but same namespace, and write a separate issue about that.

[sfilipi]

[codemzs]

Thanks @sfilipi and @wschin