Code generate TextLoader API and make it backward compatible with existing TextLoader API.

[codemzs]

*Code generate support for IDataLoader
*Make TextLoader API code generated so that it's at functional parity with the text loader in the ML.Net infrastructure.
*Move TextLoader API under Microsoft.ML.Data namespace
*Make TextLoader API backward compatible.
*Add error checking for invalid loader arguments such as ordinal, column names.
*Update baselines.
*Update samples with new loader API and backward compatibility with old loader API.

[glebuk]

@glebuk is added to the review. #Closed

[glebuk]

CustomTextLoader [](start = 36, length = 16)

Why not just textloader? Is this a user-viisible name? #Resolved

[codemzs]

TextLoader is the name for the new entrypoint for text loader that does not use the custom schema, I have renamed the old entrypoint for text loader to CustomTextLoader as per suggestion from @TomFinley #Resolved

[glebuk]

SDhould it be directly used by end users? Is visible? Should it be deprecated? How can we avoid confusion about what to use?

---

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

[codemzs]

TextLoader (New entrypoint API) can be used directly by the users, refer to Scenario3 that shows that. I can get rid of CustomTextLoader but I believe you didn't want it to be removed ...we should chat offline about this :) #Resolved

[TomFinley]

I think we should keep for now, but I'd like to find a way to mark as obsolete for a bit, clarify who was using it (if anyone) and give a fair chance for them to change their code or raise objections as to why we should keep it, then remove it as part of a separate PR if we judge that to be appropriate. #Resolved

[glebuk]

TextLoaderRange [](start = 28, length = 15)

nice! #Pending

[glebuk]

TypeToName [](start = 23, length = 10)

Don't we already have this function for all types in utils?
Also, isn;t text loader capable of loading other types like ints? #Resolved

[TomFinley]

Ah... I see TlcModule.GetDataType. That does the mappings listed here, plus a few more.

Even if for some reason we cannot use that, let's have this method use DataKind directly. We are specifying these things like "R4" and "BL", but these are not random string literals, these are the names of enums in our codebase, so we should just use the enums... the reason being, direct references are easier to be tracked with VS/Resharper, Roslyn analyzers, and other similar tools.

---

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

[codemzs]

Thanks but it seems we need to use TryGetDataKind.

---

In reply to: 186565505 [](ancestors = 186565505,186311810)

[codemzs]

public class Arguments : ArgumentsCore [](start = 7, length = 39)

revert #Resolved

[TomFinley]

NoSeal = true [](start = 100, length = 13)

Just curious, what is the utility of having the class unsealed? Do we expect/want people to inherit from the auto-generated code? #Resolved

[codemzs]

It is for convenience API for text loader. So that it can inherit all the loader settings but once we get rid of it then there would be no need for unsealing.

---

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

[TomFinley]

ct[] { fal [](start = 74, length = 10)

While we should probably change the code generator to produce more properly formatted code as we see here, I wonder if you intended to make this change? I suspect you did not, since I do not see that there was a corresponding change in CSharpApiGenerator.cs that would explain this format change. Perhaps something happened to this file that made your Visual Studio reformat it? I might choose to revert these changes in that file, since the next time that someone generates this file, we would revert back anyway, leading to a somewhat dirty blame. #Resolved

[TomFinley]

ILearningPipelineLoader [](start = 36, length = 23)

So I'm curious, I might have expected these "special" kinds like these to be detected using direct type comparisons, on entryPointInfo.InputKinds. That code would be considerably safer and simpler than searching strings, but does something make that impossible?

Having these detections be string comparisons makes me uneasy. This makes the usage invisible to any refactoring utilities (either within VS or Resharper), so from that perspective even a nameof would make me feel better. But this .Contains strategy also will catch things where the name is either a prefix or suffix of a type. (E.g., testing against IFoo will match IFooBar, which is not intentional.) #Resolved

[TomFinley]

We are keeping this around I suppose, but I wonder if we shouldn't have some mechanism for its eventual deprecation, since I imagine we don't plan to include it forever.

Typically I think this is accomplished using a System.ObsoleteAttribute. I wonder if the code-gen step could (1) check to see if this attribute is marked with that attribute and if so, migrate the same attribute over the autogenerated class or method (what is currently in CSharpApi.cs). I see you have changed CSharpApiGenerator.cs... that is, in the attribute list, if there is an ObsoleteAttribute with message "foo", declare [Obsolete("foo")] on the generated class.

It strikes me as a good time to maybe add that... should I hope be a few lines of code, unless I'm mistaken, since you could get the obsolete attribute, if any, from EntryPointInfo.Method.Attributes. #Resolved

[Ivanidzo4ka]

CustomTextLoader [](start = 43, length = 16)

why we need it if we have generic version with strictly types? #Resolved

[codemzs]

For the convenience text loader API.

---

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

[TomFinley]

            classBase += ", Microsoft.ML.ILearningPipelineItem";

This existing code are two more examples of things that should really be direct type checks, rather than looking for substrings. #Resolved

---

Refers to: src/Microsoft.ML/Runtime/Internal/Tools/CSharpApiGenerator.cs:844 in 51d5658. [](commit_id = 51d5658, deletion_comment = False)

[TomFinley]

This stray newline was probably changed accidentally, should be reverted. #Resolved

[TomFinley]

        public string Separator = "tab";

This should be changed so that programmatically people just specify a char directly, while maintaining this functionality. Add Visibility = CmdLineOnly to the attribute. Add another argument, that is char and has Visibility = EntryPointOnly. Change the convenience API so that it uses that functionality instead, as well as changing the code here that handles Separator to handle whatever that char parameter is. #Resolved

---

Refers to: src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs:298 in 51d5658. [](commit_id = 51d5658, deletion_comment = False)

[eerhardt]

I thought we were going to go with the API that was spec'd in https://github.com/dotnet/machinelearning/blob/master/Documentation/specs/mvp.md#training?

var options = CsvOptions.CreateFrom<HousePriceData>(separator: '\t', header: true);


namespace Microsoft.MachineLearning
{
    public enum DataType
    {
        Boolean,
        Int32,
        Single,
        Double,
        String        
    }
    public class CsvColumn
    {
        public CsvColumn(string name, DataType dataType, int ordinal);
        public string Name { get; }
        public DataType DataType { get; }
        public int Ordinal { get; }
    }
    public class CsvOptions
    {
        public static CsvOptions CreateFrom(char separator = ',', bool hasHeader = true, Type rowType);
        public static CsvOptions CreateFrom<T>(char separator = ',', bool hasHeader = true);

        public CsvOptions();
        public char Separator { get; set; }
        public bool HasHeader { get; set; }
        public CsvColumnCollection Columns { get; }
    }
    public class CsvColumnCollection : Collection<CsvColumn>
    {
        public CsvColumn Add(string name, DataType dataType);
        public CsvColumn Add(string name, DataType dataType, int ordinal);
    }
}

Do we still have plans on implementing this API?

[veikkoeeva]

On this format, should the types be just Type instead of custom enum? How about locales while reading or writing? Should these separate CSV specific types such as CsvColumn and CsvColumnCollecttion for CSV data be rolled to some more generic structure? I mean, it would feel plausible to think it's just a collection of data in row or column format (or some other format) while the data in files can be in any format (JSON, XML, whatnot). This is a simple example (but not that good since not heterogenous): https://gist.github.com/veikkoeeva/50c8f38ec46b0a3ce70467d16af00dc1, then ADO.NET is fuller taking into account types too: https://docs.microsoft.com/en-us/dotnet/api/system.data.datatable?view=netframework-4.7.2 (https://docs.microsoft.com/en-us/dotnet/framework/data/adonet/ado-net-datasets). I don't advocate taking a dependecy on ADO.NET, but might have food for thoughts.

In general readong or writing CSV is "easy" as https://stackoverflow.com/questions/5116604/read-csv-using-linq, in practice there are locale format spefic things (and just problems) and one might consider taking a dependency to a library (i.e. https://www.nuget.org/packages?q=CSV) and then having, say, an extension method such as .FromCsv(this FileInfo...): IEnumerable<TRecord> (and other appropriate convenience overloads). Thinking larger, if one could pull records from sources in IAsyncEnumerable fashion, when it comes widely available, it might build nicely on many sources of data.

[TomFinley]

@veikkoeeva the types being a type instead of an enum might be a good idea -- maybe something along the lines of a helper create method. I'd still like to have an enum in any such convenience, since it becomes clear what types are supported.

Regarding your other notes about more types, I certainly agree with that, but we have elsewhere not addressed these problems by changing TextLoader, but rather by introducing new implementations of IDataLoader (in the case where we're reading new formats, see e.g., PR 61), or just an IDataView in the case that nothing really is being loaded per se (see e.g., PR 106). Something like ADO.NET you could imagine having it, phrasing it as an IDataView implementation, then just plugging it in, and it should work. Of course, someone would have to write that implementation. (The name TextLoader is somewhat unfortunate, since it has, I've noticed, created the impression that it is meant to read any conceivable type of text, rather than conforming to its own specific format.)

@eerhardt thanks for sharing this. If the suggestion was we take this document as a guideline, then we should consider this PR or more precisely its successor PR 142 as perhaps an attempt to generate something in that spirit in that it solves the problem, but that being an actual attempt to solve the problem and undergoing peer review will necessarily diverge from a document like this.

I get the sense though that you're thinking this proposal should be taken literally. I also get the sense this proposal was written somewhat hurriedly. The following three flaws occur to me on cursory examination: Ordinal as int whiffed on vector valued columns, DataType needlessly duplicates the existing concept of DataKind (but less completely, it missed many types including those related to dates and times, most seriously, which people have already opened issues to
point out), and finally has Csv in its name despite the fact the separator/delimiter is configurable. Anyway, perhaps open an issue, these and other problems can be worked out more deliberatively. (Mayhap one is already opened on this subject, I don't know.)

[veikkoeeva]

You could also have a public collection SupportedTypes and have a check in construction that throws an error with appropriate error message if unsupported types are tried. The benefit of Type is in all the operations it's clear what is the type (and its semantics) after it has been constructed. I.e., checks and transformatios only in source -- possibly in the sink -- and plays nicely with the rest of the ecosystem.

By the way, if you look at ADO.NET, you notice there are vendor specific types and then just general ADO.NET types and there's some sort of a mapping between them when necessary. It looks like this. Notably there it's known that's all that's needed, but a more general purpose mapping would take the vendor invariant and use that to map types better (Oracle, for instance, doesn't have bool type).

In a way it looks like this is going into the direction of ADO.NET. If you look at here, it's like opening a source, reading it, transforming the results into some other format, and then processing it in some way. (And you can wrap that one like here and then offer a simple API like here (see ReflectionSelector that uses the mapping referred earlier) and by playing that idea, maybe many source could be handle with that by supplying an lambda).

I apologize if it's exhaustive I push various view points. I suspect, though, you need to explain these things in any event later to people asking -- at least if MachineLearning.NET becomes popular as we hope. :) I'd like to ask also (and did ponder in Gitter shortly), if your're building a composition pipeline with reflection, and want to use someting less fragile (judging from the comments), would MEF be an option? And if it would, then maybe using dependency injection like much of other frameworks and .NET ecosystem? And if not, maybe there needs to be documentation explaining how to do similarish things.

[Ivanidzo4ka]

Can you close this one? Or it's not a duplicate of #142

[codemzs]

This change was completed and checked-in as PR #142