Repository navigation
DataCommand classes shouldn't be part of the API #593
Description
Today we have classes like:
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
Activity
TomFinley commentedon Jul 28, 2018
Indeed. Not just
DataCommandbut allICommandimplementors.One easy fix would be to simply take all concrete
ICommandimplementors and make them, as classes,internal. The command line works via dependency injection from start to end, and sinceICommandis 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, anddotnet MML.dll showSchema data=test\data\breast-cancer.txtwas 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
Argumentsobjects become unnecessary sometimes, in which case they can safely be madeinternal. Methods to expose entry-points could have similar fate. Or, for that matter, allModelLoadContextconstructors or static create methods, since model loading works purely through dependency injection. Test assemblies could be granted access viaInternalsVisibleToattributes 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.VisualizationCommandwas external, someone would have to frontload work on makingGamPredictorBase'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 commentedon Jul 28, 2018
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 commentedon Jul 28, 2018
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 commentedon Jul 30, 2018
@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 commentedon Feb 13, 2019
We make all our commands internal. Feel free to reopen if you think it's worth discussion regarding separating them into separate assemblies