Skip to content
1 change: 1 addition & 0 deletions ZBaselines/Common/EntryPoints/core_ep-list.tsv
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
Data.DataViewReference Pass dataview from memory to experiment Microsoft.ML.Runtime.EntryPoints.InMemoryDataView ImportData Microsoft.ML.Runtime.EntryPoints.InMemoryDataView+Input Microsoft.ML.Runtime.EntryPoints.InMemoryDataView+Output
Data.IDataViewArrayConverter Create and array variable Microsoft.ML.Runtime.EntryPoints.CrossValidationBinaryMacro MakeArray Microsoft.ML.Runtime.EntryPoints.CrossValidationBinaryMacro+ArrayIDataViewInput Microsoft.ML.Runtime.EntryPoints.CrossValidationBinaryMacro+ArrayIDataViewOutput
Data.PredictorModelArrayConverter Create and array variable Microsoft.ML.Runtime.EntryPoints.CrossValidationBinaryMacro MakeArray Microsoft.ML.Runtime.EntryPoints.CrossValidationBinaryMacro+ArrayIPredictorModelInput Microsoft.ML.Runtime.EntryPoints.CrossValidationBinaryMacro+ArrayIPredictorModelOutput
Data.TextLoader Import a dataset from a text file Microsoft.ML.Runtime.EntryPoints.ImportTextData ImportText Microsoft.ML.Runtime.EntryPoints.ImportTextData+Input Microsoft.ML.Runtime.EntryPoints.ImportTextData+Output
Expand Down
26 changes: 26 additions & 0 deletions ZBaselines/Common/EntryPoints/core_manifest.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,31 @@
{
"EntryPoints": [
{
"Name": "Data.DataViewReference",
"Desc": "Pass dataview from memory to experiment",
"FriendlyName": null,
"ShortName": null,
"Inputs": [
{
"Name": "Data",
"Type": "DataView",
"Desc": "Pointer to IDataView in memory",
"Aliases": [
"data"
],
"Required": true,
"SortOrder": 1.0,
"IsNullable": false
}
],
"Outputs": [
{
"Name": "Data",
"Type": "DataView",
"Desc": "The resulting data view"
}
]
},
{
"Name": "Data.IDataViewArrayConverter",
"Desc": "Create and array variable",
Expand Down
28 changes: 28 additions & 0 deletions src/Microsoft.ML/CSharpApi.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,22 @@ public Microsoft.ML.Data.TextLoader.Output Add(Microsoft.ML.Data.TextLoader inpu
return output;
}

public Microsoft.ML.Data.DataViewReference.Output Add(Microsoft.ML.Data.DataViewReference input)
{
var output = new Microsoft.ML.Data.DataViewReference.Output();
Add(input, output);
return output;
}

public void Add(Microsoft.ML.Data.TextLoader input, Microsoft.ML.Data.TextLoader.Output output)
{
_jsonNodes.Add(Serialize("Data.TextLoader", input, output));
}

public void Add(Microsoft.ML.Data.DataViewReference input, Microsoft.ML.Data.DataViewReference.Output output)
{
_jsonNodes.Add(Serialize("Data.DataViewReference", input, output));
}
public Microsoft.ML.Models.AnomalyDetectionEvaluator.Output Add(Microsoft.ML.Models.AnomalyDetectionEvaluator input)
{
var output = new Microsoft.ML.Models.AnomalyDetectionEvaluator.Output();
Expand Down Expand Up @@ -1311,6 +1322,23 @@ public sealed partial class TextLoader
public string CustomSchema { get; set; }


public sealed class Output
{
/// <summary>
/// The resulting data view
/// </summary>
public Var<Microsoft.ML.Runtime.Data.IDataView> Data { get; set; } = new Var<Microsoft.ML.Runtime.Data.IDataView>();

}
}

public sealed partial class DataViewReference
{
/// <summary>
/// Location of the input file
/// </summary>
public Var<Microsoft.ML.Runtime.Data.IDataView> Data { get; set; } = new Var<Microsoft.ML.Runtime.Data.IDataView>();

public sealed class Output
{
/// <summary>
Expand Down
94 changes: 94 additions & 0 deletions src/Microsoft.ML/MemoryCollection.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using Microsoft.ML.Runtime;
using Microsoft.ML.Runtime.Api;
using Microsoft.ML.Runtime.Data;
using Microsoft.ML.Runtime.EntryPoints;
using Microsoft.ML.Runtime.Internal.Utilities;

namespace Microsoft.ML

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.ML [](start = 19, length = 3)

ML.Data #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TextLoader is part of just ML, should I change it as well?


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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question... seems they both should be in data. the argument went about like this - can a user has out of the box experience with just ML namespace.
I guess we can keep it in ML for now.


In reply to: 187221799 [](ancestors = 187221799,187221578)

@codemzs codemzs May 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move to Data. My PR will move TextLoader to ML.Data. #Resolved

{
public class MemoryCollection

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MemoryCollection [](start = 17, length = 16)

Is this a static class? Why does it exist at all? Seems like a very strange name for a class #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a helper class, see comment from Tom starting from "Just an observation".


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

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you have that static method on the MemoryCollectionLoader? If it must be a different class vs the MemoryCollectionLoader then can you define a MemoryCollectionLoader with no generic type arg?


In reply to: 187222607 [](ancestors = 187222607,187222326)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MemoryCollectionLoader is a generic class. Whole point to not type class twice. MemoryCollectionLoader.Create(Listdata) and MemoryCollection.Create(Listdata).


In reply to: 187223070 [](ancestors = 187223070,187222607,187222326)

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the create method anyway? What's wrong with a good old constructor? Text loader does not have one.


In reply to: 187223445 [](ancestors = 187223445,187223070,187222607,187222326)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For problem with "good old constructor," see earlier comment from the aforementioned Tom on "Just an observation." The more we can rely on the compiler to do the work for the user, the better, I'd say. Especially since this is merely a wrapper anyway, I don't see the point of making the user's job any harder than it has to be.


In reply to: 187226013 [](ancestors = 187226013,187223445,187223070,187222607,187222326)

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we must, then lest's just create a non-generic class with the same name as the generic class, make it static and have it contain those create methods. No need to pollute namespace with different confusing names.


In reply to: 187230636 [](ancestors = 187230636,187226013,187223445,187223070,187222607,187222326)

@TomFinley TomFinley May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I'd rather just not expose the implementations at all, have the static create methods return an object of the appropriate interface. I don't think anything is gained by exposing them.


In reply to: 187240158 [](ancestors = 187240158,187230636,187226013,187223445,187223070,187222607,187222326)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather just not expose the implementations at all, have the static create methods return an object of the appropriate interface
Done.


In reply to: 187352180 [](ancestors = 187352180,187240158,187230636,187226013,187223445,187223070,187222607,187222326)

{
/// <summary>
/// Creates memory collection loader.

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loader [](start = 38, length = 6)

loader from IList #Closed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that not obvious from the parameter type? Perhaps adding a "from ." or somesuch would be fine (added to both), but even that I'd say would be a bit too needlessly verbose.


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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's confusing that it says "Collection Loader" while it takes a List . perhaps we should rename it to a ListLoader. and split away from the StreamingEnumerableLoader


In reply to: 187230849 [](ancestors = 187230849,187221474)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User no longer see class, it just get interface, is it better?


In reply to: 187238467 [](ancestors = 187238467,187230849,187221474)

/// </summary>
public static MemoryCollectionLoader<T> Create<T>(IList<T> data) where T:class

@TomFinley TomFinley May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IList data [](start = 58, length = 13)

Just curious, is your reason for supporting IList separately just to support GetRowCount? Or is there a deeper reason I don't appreciate? #Closed

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shuffleabla = true.
But do you think users will care about the difference? If noone cares, we should just leave one with IEnumerable, as it is the grand-daddy of IList interface.


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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CanShuffle and MoveManyCore implementation is different. I have suspicion what access by index to element is slightly faster than access through enumerator. I would prefer to have this option available, especially if it's transparent for user. They just pass collection (IList or IEnumerable) through method.


In reply to: 187238508 [](ancestors = 187238508,187231105)

{
return new MemoryCollectionLoader<T>(data);
}

/// <summary>
/// Creates memory collection loader.

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loader [](start = 38, length = 6)

loader from IEnumerable. #Closed

/// </summary>
public static MemoryCollectionLoader<T> Create<T>(IEnumerable<T> data) where T : class

@TomFinley TomFinley May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loader [](start = 38, length = 6)

The word "Loader" is unfortunate. We've always used this too describe things that load from a stream. Loaders also have a data model, this thing won't. You're introducing new capabilities because the original design of the API that assumed that there would always be a data source situated from a stream and loaded through an IDataLoader was insufficient -- but this also means that we may want to call this something else. We could rename ILearningPipelineLoader to something else... as ILearningPipelineSource, perhaps? Then a ILearningPipelineLoader would then be a specialization of that source interface. Anyway, loader is not an appropriate name for this broader perspective we're introducing in your PR.

(Though I note that, somehow, that in this new API the ILearningPipelineLoader interface does not have a mechanism getting the data model out, that should probably be added... but that should probably be a separate issue. #Closed

{
return new MemoryCollectionLoader<T>(data);
}
}

/// <summary>
/// Allows you to convert your memory collection into IDataview.
/// </summary>
/// <typeparam name="TInput"></typeparam>
public class MemoryCollectionLoader<TInput> : ILearningPipelineLoader

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MemoryCollectionLoader [](start = 17, length = 22)

Why do we call it "Memory" We just know it has the Icollection as input. The ICollection can be backed by disk for example.
Why not just call it CollectionIDataViewLoader or CollectionLoader?
Acctually, since yo are capable of reading enums and lists and collectoins, why not call it EnumerableLoader? (enumerable is the base of collection and list) #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I summon you Ж)


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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's decide on name once we decide if we are ok with just supporting ienumerables. and not separate method for lists.


In reply to: 187223187 [](ancestors = 187223187,187222448)

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MemoryCollectionLoader [](start = 17, length = 22)

This class very closely mimics the ListDataView and StreamingDataView. the key difference between the two being that one is shuffleble and nother is not. Shoould we also create two top level classes here as well?
Then we can have proper names like ListLoader and StreamingEnumerationLoader.
If we get rid of the Create() method then we can just let them be without any MemoryLoader stub classes. #Pending

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key difference is implementation of interface. This is new API, I have to create wrappers for it.


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

where TInput : class
{
public ILearningPipelineStep ApplyStep(ILearningPipelineStep previousStep, Experiment experiment)
{
Contracts.Assert(previousStep == null);
_dataViewEntryPoint = new Data.DataViewReference();
var importOutput = experiment.Add(_dataViewEntryPoint);
return new MemoryCollectionPipelineStep(importOutput.Data);
}

private readonly IList<TInput> _listCollection;
private readonly IEnumerable<TInput> _enumerableCollection;

private Data.DataViewReference _dataViewEntryPoint;
private IDataView _dataView;

/// <summary>
/// Creates IDataview on top of collection
/// </summary>
public MemoryCollectionLoader(IList<TInput> collection)
{
Contracts.CheckParamValue(Utils.Size(collection) > 0, collection, nameof(collection), "Must be non-empty");
_listCollection = collection;
}

/// <summary>
/// Creates IDataview on top of collection
/// </summary>
public MemoryCollectionLoader(IEnumerable<TInput> collection)
{
Contracts.CheckValue(collection,nameof(collection), "Must be non-null");

@TomFinley TomFinley May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, "Must be non-null" [](start = 62, length = 20)

Remove this string literal, it is not necessary given that you're using this check. The messages for all these are standardized, and it's better for them to be consistent. #Closed

@TomFinley TomFinley May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

,na [](start = 43, length = 3)

Spacing, generally, use ctrl-k-d in VS to keep things formatted. #Closed

_enumerableCollection = collection;

}

public void SetInput(IHostEnvironment env, Experiment experiment)
{
if (_listCollection != null)
_dataView = ComponentCreation.CreateDataView(env, _listCollection);
if (_enumerableCollection != null)
_dataView = ComponentCreation.CreateStreamingDataView(env, _listCollection);

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_listCollection [](start = 75, length = 15)

Note that you are not using _enumerableCollection here. Likely a bug
Do we really need two fields here, one for list another for enumerable if only one is used at a time? Isn't a list an IEnumerable anyway? Why not just always store an _enumerable and just use that all the time?
If we want to use two (one is shuffleble and other is not) then we should split into two classes. Seem like this class tries to be two classes at once. #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for pointing that out, I update test to call both implementation and I also split class into two classes, to get rid of if condition


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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still want to have two different implementation for IList and IEnumerable, so I would prefer to keep it that way, but with two separate classes


In reply to: 187395933 [](ancestors = 187395933,187225560)

env.CheckValue(_dataView, nameof(_dataView));
experiment.SetInput(_dataViewEntryPoint.Data, _dataView);
}

private class MemoryCollectionPipelineStep : ILearningPipelineDataStep
{
public MemoryCollectionPipelineStep(Var<IDataView> data)
{
Data = data;
}

public Var<IDataView> Data { get; }
public Var<ITransformModel> Model => null;

@TomFinley TomFinley May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[](start = 45, length = 2)

Note you've put two space here. #Closed

}
}
}
37 changes: 37 additions & 0 deletions src/Microsoft.ML/Runtime/EntryPoints/InMemoryDataView.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.ML.Runtime;
using Microsoft.ML.Runtime.CommandLine;
using Microsoft.ML.Runtime.Data;
using Microsoft.ML.Runtime.EntryPoints;

[assembly: LoadableClass(typeof(void), typeof(InMemoryDataView), null, typeof(SignatureEntryPointModule), "InMemoryDataView")]
namespace Microsoft.ML.Runtime.EntryPoints
{
public class InMemoryDataView

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InMemoryDataView [](start = 17, length = 16)

Where is this being used? I don't see any references to this class in code or tests. #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a entrypoint :) Data.DataViewReference it get used in MemoryCollection.cs (At least it entry point wrapper)


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

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, silly me as looking for "InMemoryDataView" not "Data.DataViewReference"


In reply to: 187222365 [](ancestors = 187222365,187222065)

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InMemoryDataView [](start = 17, length = 16)

So to clarify, all this entrypoint does it turns input to output? Should we call it as such, something like a data passthrough or something?
Because in reality, does this EP cares what kind of idv is input? #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it doesn't. but I wouldn't call it DataPass entrypoint either, since it allow you pass only dataview from you code to experiment, and DataViewReference is already taken by entrypoint class.
DataViewPasser?


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

@glebuk glebuk May 14, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not DataViewReference or DataViewReferenceEp? Seems like very unrelated class name to the entrypoint name.


In reply to: 187395356 [](ancestors = 187395356,187240561)

{
public sealed class Input
{
[Argument(ArgumentType.Required, ShortName = "data", HelpText = "Pointer to IDataView in memory", SortOrder = 1)]

@TomFinley TomFinley May 11, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ShortName = "data" [](start = 45, length = 18)

Since shortname is same as longname, you can safely omit. #Resolved

public IDataView Data;
}

public sealed class Output
{
[TlcModule.Output(Desc = "The resulting data view", SortOrder = 1)]
public IDataView Data;
}

[TlcModule.EntryPoint(Name = "Data.DataViewReference", Desc = "Pass dataview from memory to experiment")]
public static Output ImportData(IHostEnvironment env, Input input)
{
Contracts.CheckValue(env, nameof(env));
var host = env.Register("DataViewReference");
env.CheckValue(input, nameof(input));
EntryPointUtils.CheckInputArgs(host, input);
return new Output { Data = input.Data };
}
}
}
3 changes: 1 addition & 2 deletions src/Microsoft.ML/TextLoader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,10 @@ private class TextLoaderPipelineStep : ILearningPipelineDataStep
public TextLoaderPipelineStep(Var<IDataView> data)
{
Data = data;
Model = null;
}

public Var<IDataView> Data { get; }
public Var<ITransformModel> Model { get; }
public Var<ITransformModel> Model => null;
}
}
}
Loading