Much more core internalization (Phase 3) #1870
Conversation
…d fallout thereby
TomFinley
requested review from
Ivanidzo4ka,
eerhardt,
yaeldekel,
artidoro,
sfilipi and
glebuk
| @@ -188,28 +188,28 @@ public void GetOutputToPath(GraphRunner runner, string varName, string path, Tlc | |||
|
|
|||
| } | |||
|
|
|||
| private void SaveArrayToFile<T>(List<T> array, TlcModule.DataKind kind, string partialPath, string extension) | |||
| where T : class | |||
| private void SaveArrayToFile(PredictorModel[] array, string partialPath, string extension) | |||
There was a problem hiding this comment.
Wow, that's an interesting method. I wonder how it came to being?..
There was a problem hiding this comment.
No idea. This was one of the few non-mechanical changes in this PR. Obviously I simply could not let this stand when I stumbled across it. 😄
TomFinley
changed the title
| @@ -264,7 +265,8 @@ public object CreateArguments() | |||
| /// <summary> | |||
| /// A description of a single entry point. | |||
| /// </summary> | |||
| public sealed class EntryPointInfo | |||
| [BestFriend] | |||
| internal sealed class EntryPointInfo | |||
There was a problem hiding this comment.
heads up that if we build entry-point based tooling around ML.Net, it would be convenient to be able to access this, rather than replicate it in his own data models.
There was a problem hiding this comment.
That may be @sfilipi , however this does not align with the current thinking around entry-points as I understand it, which involves (1) getting the entry-point manifest and then (2) auto-generating an API out of that manifest. Of course, we could imagine that a C# API to entry-points might be something worth having, but I generally prefer to have a solid scenario in mind before I start designing an API for it. Anyway: AFAIK it's our intention that any dependency-injection style stuff not be part of the V1 API, unless that has changed in the last week or so.
| { | ||
| return _entryPoints.AsEnumerable(); | ||
| } | ||
|
|
||
| public bool TryFindEntryPoint(string name, out EntryPointInfo entryPoint) | ||
| [BestFriend] | ||
| internal bool TryFindEntryPoint(string name, out EntryPointInfo entryPoint) |
There was a problem hiding this comment.
same as above, they could prove useful a public.
There was a problem hiding this comment.
AFAIK, the plan around ComponentCatalog is that the only publicly accessible member on it will be the thing to register assemblies, as well as whatever we wind up doing to register for custom transformers, right?
I think if we go function by function by what "might" prove useful we'll identify everything as potentially useful -- after all, everything was written for a reason. I think though we might want to be a bit more picky about those scenarios we're actually trying to hit for v1.
| private readonly IDataView _chain; | ||
|
|
||
| /// <summary> | ||
| /// The input schema that this transform model was originally instantiated on. | ||
| /// Note that the schema may have columns that aren't needed by this transform model. | ||
| /// If an IDataView exists with this schema, then applying this transform model to it | ||
| /// If an <see cref="IDataView"/> exists with this schema, then applying this transform model to it | ||
| /// shouldn't fail because of column type issues. | ||
| /// REVIEW: Would be nice to be able to trim this to the minimum needed somehow. Note | ||
| /// however that doing so may cause issues for composing transform models. For example, | ||
| /// if transform model A needs column X and model B needs Y, that is NOT produced by A, | ||
| /// then trimming A's input schema would cause composition to fail. |
There was a problem hiding this comment.
want to put this as a simple comment, outside of the summary node?
There was a problem hiding this comment.
I'm not sure. It's a summary comment on an internal class, so I'm not too excited about it? I mean, as you see I've fixed it insofar as I have an actual reference to the type now, which is surely an improvement, but beyond that I'm not too excited?
You're talking about the review comment, or something else?
ghost
locked as resolved and limited conversation to collaborators
Continuuation of #1626 , which as usual continues #1519.
Most of
Core, but not all, is now internalized. Most significant remainders includeRoleMappedSchemaandMetadataUtils. Some of these had fairly long reaching side effects (most notably, the internalization ofRoleMappedData).