DataCommand classes shouldn't be part of the API

[eerhardt]

Today we have classes like:

• TrainCommand
• CrossValidationCommand
• Etc

These classes are meant to be invoked from the MAML command line, and not from a normal "API" user.

However, this isn't possible to tell these shouldn't be used because these classes are in the same assembly as the "TextLoader" class (Microsoft.ML.Data.dll), which is obviously part of the core API and is shown in almost all examples.

We should decide what to do with these classes. If we should move them out into a separate assembly/nuget package/etc, so users aren't confused that they shouldn't be using them.

/cc @TomFinley @Zruty0

[TomFinley]

Indeed. Not just DataCommand but all ICommand implementors.

One easy fix would be to simply take all concrete ICommand implementors and make them, as classes, internal. The command line works via dependency injection from start to end, and since ICommand is useful only to the command line, this should work just fine. This would not require any deep re-engineering, and as far as I am aware everything should still be able to work -- and if there's, say, any utility code useful outside of the assembly that should probably be lifted to some general utility class anyway.

I just tried this with ShowSchemaCommand, and dotnet MML.dll showSchema data=test\data\breast-cancer.txt was still able to instantiate and execute the command despite it and its arguments being internal.

This seems like it would make a fine general policy, that any structure whose only conceivable use is through dependency injection can hide behind an internal. For example, as @zeahmed's work in #405 and #491 progresses, it may be that Arguments objects become unnecessary sometimes, in which case they can safely be made internal. Methods to expose entry-points could have similar fate. Or, for that matter, all ModelLoadContext constructors or static create methods, since model loading works purely through dependency injection. Test assemblies could be granted access via InternalsVisibleTo attributes in cases where that is necessary.

Factoring out into a separate assembly is possible, even easy in many cases, but in some cases it is quite difficult and would require frontloading design of public API surfaces that would serve little other function except to facilitate the necessary calls from a different assembly. (E.g., if GamPredictorBase.VisualizationCommand was external, someone would have to frontload work on making GamPredictorBase's public surface expose the structures the visualizer currently just accesses privately). It also necessarily makes these components more conspicuous, not less, to have a nuget whose description is "don't use this please" vs. an invisible internal class buried in the assembly.

[eerhardt]

It also necessarily makes these components more conspicuous, not less, to have a nuget whose description is "don't use this please" vs. an invisible internal class buried in the assembly.

The thinking is they would live with the command line and/or GUI implementation. They wouldn’t be a regular NuGet package that a consumer would install.

[TomFinley]

OK. Other objection still stands. Also as I understand it, either way we're breaking the component model for commands specifically, among all DI components, despite the fact that the DI's primary reason for existing is to serve the command line. That doesn't seem to serve any purpose. Grouping code by what it is vs. its actual dependencies that it needs to function seems like a weaker, not a stronger architecture on the whole. Sometimes the easiest solution is sometimes the most architecturally sound. Thanks for bringing this up though!

[alexdegroot]

@TomFinley I'd argue a little bit against your idea of simply making all internal. If there's a need for a UI component like a Command Line, then there's another use case as well. If there absolutely no way you can think of that other use case(s), then re-consider your Console use case. Isn't it simply a hack?

When it comes to consuming the library, I'd very much like to consume a package with 'just what I need'. Even though this library is often consumed on a server or desktop with plenty of space(memory, disk), it's not a given that every runtime has this. If I run ML.NET on Windows Tiny(is it Core or Nano, or IoT, or Edge ;) ?) or even as WebAssembly then I want its libraries and runtime to be as small as possible. Linkers will drop unused libraries, but most likely won't reduce the size of used libraries. Especially if stuff is used implicitly.

I suggest therefore that your use cases, then base a packaging and finally a project structure on this. I therefore support @eerhardt idea to split it. And would love to do the work for it.

[Ivanidzo4ka]

We make all our commands internal. Feel free to reopen if you think it's worth discussion regarding separating them into separate assemblies