PredictionEngine uses IRowToRowMapper, ITransformer can create IRowToRowMapper #973

TomFinley · 2018-09-21T01:18:18Z

In which we reduce the overhead of calls to Predict by avoiding the creation of cursors.

PredictionEngine used to create a "fake" data view under the hood, by wrapping a BatchPredictionEngine, but only feeding in one item. This was a fairly heavy operation involving spooling up of cursors, including heavy reflection based processing on each row, etc. Now we are able to avoid this by using an existing interface IRowToRowMapper.

The other major part of the change is, of course, changing transformers so they can produce IRowToRowMapper, which they previously did not. In most cases this is relatively straightforward since nearly all transformers of interest are using these IRowToRowMapper under the hood to support their computation.

IRowToRowMapper enhancements, unification of some interfaces.
ITransformer has IRowToRowMapper accessor.
PredictionEngine now uses IRowToRowMapper

The effects of this are most dramatic using prediction engines outside of the ML.NET v0.1 pipeline API (e.g., using IEstimator based pipelines, pre-ML.NET v0.1 API, and so forth). The effect of this is that when predicting, now actual computation dominates the cost, as opposed to reflection based stuff. 😄 Due to architectural limitations of pipeline API models, these do not see benefit.

Will perhaps update with timings later.

Fix #986.

Zruty0 · 2018-09-21T16:29:11Z

src/Microsoft.ML.Data/DataView/ChainedRowToRowMapper.cs

+    /// <summary>
+    /// A row-to-row mapper that is the result of a chained application of multiple mappers.
+    /// </summary>
+    public sealed class ChainedRowToRowMapper : IRowToRowMapper


Chained [](start = 24, length = 7)

Can we rename it either to CompositeRowToRowMapper (as this is similar to CompositeDataLoader) or RowToRowMapperChain (akin to TransformerChain and EstimatorChain) ? #Resolved

Let me do the composite thing, since as an implementor of IRowToRowMapper it should end with RowToRowMapper. :D

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

markusweimer · 2018-09-21T17:23:03Z

This PR does not reference an issue. Please fix this before merging.

Zruty0 · 2018-09-21T17:48:40Z

src/Microsoft.ML.Api/PredictionEngine.cs

+
+        ~PredictionEngine()
+        {
+            _disposer?.Invoke();


_disposer [](start = 12, length = 9)

Just so that we don't forget. If we were planning to make IRow disposable, does it call for making PredictionEngine also disposable? #Pending

Yeah, you're probably right about that.

I think we need to discuss this more thoroughly. Maybe I'll add an issue.

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

Zruty0 · 2018-09-21T17:49:18Z

src/Microsoft.ML.Api/DataViewConstructionUtils.cs

+            return new InputRow<TRow>(env, internalSchemaDefn);
+        }
+
+        public static IDataView LoadPipeWithPredictor(IHostEnvironment env, Stream modelStream, IDataView view)


public [](start = 8, length = 6)

maybe internal? or else document it :) #Closed

It is internal. This DataViewConstructionUtils is an internal class.

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

Zruty0 · 2018-09-21T17:52:31Z

src/Microsoft.ML.Api/DataViewConstructionUtils.cs

@@ -92,198 +360,126 @@ protected DataViewBase(IHostEnvironment env, string name, InternalSchemaDefiniti
                return new[] { GetRowCursor(predicate, rand) };
            }

-            public abstract class DataViewCursorBase : RootCursorBase, IRowCursor
+            public abstract class DataViewCursorBase : InputRowBase<TRow>, IRowCursor


DataViewCursorBase [](start = 34, length = 18)

all of this code is a clone of something, right? #Closed

Ah yes let me add a comment about that.

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

Zruty0 · 2018-09-21T17:52:56Z

src/Microsoft.ML.Api/TypedCursor.cs

@@ -26,6 +26,21 @@ public interface IRow<TRow> : IRow
        void FillValues(TRow row);
    }

+    /// <summary>
+    /// This interface is an <see cref="IRow"/> with 'stringly typed' binding.


stringly [](start = 54, length = 8)

stringly [](start = 54, length = 8)

I like the word, but you probably meant 'strongly' ? #Closed

I should have just copy-pasted. :D

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

Zruty0 · 2018-09-21T17:53:09Z

src/Microsoft.ML.Api/TypedCursor.cs

+    /// This interface is an <see cref="IRow"/> with 'stringly typed' binding.
+    /// It can accept values of type <typeparamref name="TRow"/> and present the value as a row.
+    /// </summary>
+    /// <typeparam name="TRow"></typeparam>


[](start = 8, length = 35)

[](start = 8, length = 35)

remove if empty #Closed

Zruty0 · 2018-09-21T17:55:08Z

src/Microsoft.ML.Api/TypedCursor.cs

@@ -26,6 +26,21 @@ public interface IRow<TRow> : IRow
        void FillValues(TRow row);
    }

+    /// <summary>


summary [](start = 9, length = 7)

could you outline the contract of that?
If we get getters before the first Accept, what happens?

Also, Accept is a bit off, because it implies some choice on our part (accept or not).
This mirrors 'FillValues', so maybe ExtractValues, or ReadValues? #Closed

Hmmm yes.. let's do Extract since that's a nice counterpoint to Fill.

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

Also good call I should have a check on Position.

In reply to: 219607464 [](ancestors = 219607464,219579240)

Zruty0 · 2018-09-21T17:58:22Z

src/Microsoft.ML.Api/TypedCursor.cs

+    /// It can accept values of type <typeparamref name="TRow"/> and present the value as a row.
+    /// </summary>
+    /// <typeparam name="TRow"></typeparam>
+    public interface IInputRow<TRow> : IRow


IInputRow [](start = 21, length = 9)

IInputRow [](start = 21, length = 9)

I want to codify a bit better the difference between IRow<T> and IInputRow<T>. The difference is mostly in how they are instantiated, right? The first one takes an existing row, and lets us read it into object, and the second one lets us take an object and treat it as a row?

So maybe we could have like IRowReadableAs<TRow> and IRowBackedBy<TRow> ? or something like that? #Closed

If you're comfortable changing the API interface signatures that's fine.

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

Zruty0 · 2018-09-21T17:59:20Z

src/Microsoft.ML.Core/Data/IEstimator.cs

+        /// <summary>
+        /// Constructs a row-to-row mapper based on an input schema. If <see cref="IsRowToRowMapper"/>
+        /// is <c>false</c>, then an exception should be thrown. If the input schema is in any way
+        /// unsuitable for constructing the mapper, an exception should likewise be thrown.


an exception should likewise be thrown [](start = 52, length = 38)

I think this should be checked in TestEstimatorCore somewhere #Closed

Yeah, you're probably right... let me check the schemas.

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

* ITransformer has IRowToRowMapper accessor.

Zruty0 · 2018-09-21T20:32:02Z

test/Microsoft.ML.TestFramework/DataPipe/TestDataPipeBase.cs

@@ -111,6 +112,14 @@ protected static TextLoader.Arguments MakeIrisTextLoaderArgs()
            {
                var schema = transformer.GetOutputSchema(data.Schema);

+                // If it's a row to row mapper, then the output name.
+                if (transformer.IsRowToRowMapper)


IsRowToRowMapper [](start = 32, length = 16)

How about adding mustFail's for GetRowToRowMapper on invalid data? #Resolved

That's pretty good, let's do that. Also I see I screwed up the comments here.

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

TomFinley · 2018-09-21T20:32:19Z

Thanks @markusweimer. I feel the overpowering urge to replace you with a script? 😀

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

Zruty0

sfilipi · 2018-09-21T20:42:54Z

src/Microsoft.ML.Data/DataView/CompositeRowToRowMapper.cs

+        {
+            Func<int, bool> toReturn = predicate;
+            for (int i = _innerMappers.Length - 1; i <= 0; --i)
+                toReturn = _innerMappers[i].GetDependencies(toReturn);


toReturn [](start = 16, length = 8)

does this look right? #ByDesign

Looks right to me. We're chaining the delegates backwards.

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

So the idea is that the predicates are (as elsewhere) predicates on the outputs, so if we are chaining the row-to-row mappers, then while we transform forwards, we have to check the dependencies in the opposite direction.

In reply to: 219622203 [](ancestors = 219622203,219621916)

markusweimer · 2018-09-21T20:47:36Z

Thanks @markusweimer. I feel the overpowering urge to replace you with a script? 😀

Please do!

sfilipi

eerhardt · 2018-09-24T14:41:59Z

src/Microsoft.ML.Api/PredictionEngine.cs

+            _outputRow = cursorable.GetRow(outputRow);
+        }
+
+        ~PredictionEngine()


We shouldn't have a finalizer without having IDisposable on an object. See https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern

Especially this part:

When the CLR needs to call a finalizer, it must postpone collection of the object’s memory until the next round of garbage collection (the finalizers run between collections). This means that the object’s memory (and all objects it refers to) will not be released for a longer period of time.

Therefore, relying exclusively on finalizers might not be appropriate in many scenarios when it is important to reclaim unmanaged resources as quickly as possible, when dealing with scarce resources, or in highly performant scenarios in which the added GC overhead of finalization is unacceptable.

TomFinley requested review from Ivanidzo4ka, eerhardt, sfilipi and Zruty0 September 21, 2018 01:18

Zruty0 reviewed Sep 21, 2018

View reviewed changes

TomFinley added 5 commits September 21, 2018 13:02

* IRowToRowMapper enhancements, unification of some interfaces.

7a13533

* ITransformer has IRowToRowMapper accessor.

Introduce InputSchema to IRowToRowMapper.

c9415bc

Ranking and wrappper

13d62c5

Prediction engine now uses IRowToRowMapper

76bfef2

Code review

dcc1e40

TomFinley force-pushed the tfinley/RowMap branch from 5d819b7 to dcc1e40 Compare September 21, 2018 20:03

Zruty0 reviewed Sep 21, 2018

View reviewed changes

Zruty0 approved these changes Sep 21, 2018

View reviewed changes

Improve testing

f4cdb37

TomFinley requested a review from zeahmed September 21, 2018 20:42

sfilipi reviewed Sep 21, 2018

View reviewed changes

sfilipi approved these changes Sep 21, 2018

View reviewed changes

TomFinley merged commit 46ba430 into dotnet:master Sep 21, 2018

TomFinley deleted the tfinley/RowMap branch September 21, 2018 21:43

eerhardt reviewed Sep 24, 2018

View reviewed changes

TomFinley mentioned this pull request Sep 25, 2018

Add Benchmark test for PredictionEngine #1014

Merged

TomFinley mentioned this pull request Nov 29, 2018

Name of MakePredictionFunction is confusing (to me). #1761

Closed

TomFinley mentioned this pull request Feb 25, 2019

Explicit implementation for IsRowToRowMapper and GetRowToRowMapper #2673

Merged

ghost locked as resolved and limited conversation to collaborators Mar 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PredictionEngine uses IRowToRowMapper, ITransformer can create IRowToRowMapper #973

PredictionEngine uses IRowToRowMapper, ITransformer can create IRowToRowMapper #973

TomFinley commented Sep 21, 2018 •

edited

Loading

Zruty0 Sep 21, 2018 •

edited by TomFinley

Loading

TomFinley Sep 21, 2018 •

edited

Loading

markusweimer commented Sep 21, 2018

Zruty0 Sep 21, 2018 •

edited by TomFinley

Loading

TomFinley Sep 21, 2018 •

edited

Loading

Zruty0 Sep 21, 2018 •

edited

Loading

TomFinley Sep 21, 2018 •

edited

Loading

Zruty0 Sep 21, 2018 •

edited

Loading

TomFinley Sep 21, 2018

Zruty0 Sep 21, 2018 •

edited

Loading

TomFinley Sep 21, 2018

Zruty0 Sep 21, 2018 •

edited

Loading

Zruty0 Sep 21, 2018 •

edited

Loading

TomFinley Sep 21, 2018

TomFinley Sep 21, 2018

Zruty0 Sep 21, 2018 •

edited

Loading

TomFinley Sep 21, 2018

Zruty0 Sep 21, 2018 •

edited

Loading

TomFinley Sep 21, 2018

Zruty0 Sep 21, 2018 •

edited by TomFinley

Loading

TomFinley Sep 21, 2018

TomFinley commented Sep 21, 2018

Zruty0 left a comment

sfilipi Sep 21, 2018 •

edited by TomFinley

Loading

TomFinley Sep 21, 2018

TomFinley Sep 21, 2018

markusweimer commented Sep 21, 2018

sfilipi left a comment

eerhardt Sep 24, 2018

PredictionEngine uses IRowToRowMapper, ITransformer can create IRowToRowMapper #973

PredictionEngine uses IRowToRowMapper, ITransformer can create IRowToRowMapper #973

Conversation

TomFinley commented Sep 21, 2018 • edited Loading

Zruty0 Sep 21, 2018 • edited by TomFinley Loading

Choose a reason for hiding this comment

TomFinley Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

markusweimer commented Sep 21, 2018

Zruty0 Sep 21, 2018 • edited by TomFinley Loading

Choose a reason for hiding this comment

TomFinley Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

TomFinley Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 21, 2018 • edited by TomFinley Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley commented Sep 21, 2018

Zruty0 left a comment

Choose a reason for hiding this comment

sfilipi Sep 21, 2018 • edited by TomFinley Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markusweimer commented Sep 21, 2018

sfilipi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley commented Sep 21, 2018 •

edited

Loading

Zruty0 Sep 21, 2018 •

edited by TomFinley

Loading

TomFinley Sep 21, 2018 •

edited

Loading

Zruty0 Sep 21, 2018 •

edited by TomFinley

Loading

TomFinley Sep 21, 2018 •

edited

Loading

Zruty0 Sep 21, 2018 •

edited

Loading

TomFinley Sep 21, 2018 •

edited

Loading

Zruty0 Sep 21, 2018 •

edited

Loading

Zruty0 Sep 21, 2018 •

edited

Loading

Zruty0 Sep 21, 2018 •

edited

Loading

Zruty0 Sep 21, 2018 •

edited

Loading

Zruty0 Sep 21, 2018 •

edited

Loading

Zruty0 Sep 21, 2018 •

edited

Loading

Zruty0 Sep 21, 2018 •

edited by TomFinley

Loading

sfilipi Sep 21, 2018 •

edited by TomFinley

Loading