Internalization of TensorFlowUtils.cs and refactored TensorFlowCatalog. #2672
Conversation
zeahmed
requested review from
Ivanidzo4ka,
TomFinley,
yaeldekel and
rogancarr
Codecov Report
@@ Coverage Diff @@
## master #2672 +/- ##
==========================================
- Coverage 71.54% 71.54% -0.01%
==========================================
Files 801 801
Lines 141855 141863 +8
Branches 16120 16120
==========================================
+ Hits 101485 101489 +4
- Misses 35920 35925 +5
+ Partials 4450 4449 -1
|
Codecov Report
@@ Coverage Diff @@
## master #2672 +/- ##
==========================================
+ Coverage 71.66% 71.68% +0.02%
==========================================
Files 809 809
Lines 142378 142401 +23
Branches 16119 16112 -7
==========================================
+ Hits 102030 102078 +48
+ Misses 35915 35890 -25
Partials 4433 4433
|
| /// </summary> | ||
| /// <param name="catalog">The transform's catalog.</param> | ||
| /// <param name="modelLocation">Location of the TensorFlow model.</param> | ||
| public static IEnumerable<(string, string, DataViewType, string[])> GetModelNodes(this TransformsCatalog.TensorFlowTransforms catalog, string modelLocation) |
There was a problem hiding this comment.
GetModelNodes [](start = 76, length = 13)
This doesn't need to be a part of the public API. #Resolved
There was a problem hiding this comment.
It is being use here.
If I don't expose it like this then I will have to make the internal one public. What do you suggest?
In reply to: 258728605 [](ancestors = 258728605)
If you do this, then this class doesn't need to have an In reply to: 465804802 [](ancestors = 465804802) Refers to: src/Microsoft.ML.TensorFlow/TensorFlowModelInfo.cs:46 in e5eef19. [](commit_id = e5eef19, deletion_comment = False) |
| /// </summary> | ||
| /// <param name="catalog">The transform's catalog.</param> | ||
| /// <param name="modelLocation">Location of the TensorFlow model.</param> | ||
| public static DataViewSchema GetModelSchema(this TransformsCatalog.TensorFlowTransforms catalog, string modelLocation) |
There was a problem hiding this comment.
GetModelSchema [](start = 37, length = 14)
I think GetModelSchema should take a TensorFlowModelInfo instead of a string
(so that users can use the already loaded model they got from LoadTensorFlowModel). #Resolved
| /// <summary> | ||
| /// List of operations for using TensorFlow model. | ||
| /// </summary> | ||
| public TensorFlowTransforms TensorFlow { get; } |
There was a problem hiding this comment.
TensorFlowTransforms [](start = 15, length = 20)
Please do not do this. Otherwise we will have an empty property unless someone imports the nuget, which is confusing and undesirable.
Please follow instead the pattern that we see in image processing. You'll note that we do not have an empty image processing nuget. Rather they are added to this catalog. Similar with ONNX scoring. You'll note that these both have extensions on this TransformsCatalog catalog. What we don't have are empty properties "Images" and "ONNX" littering our central object that users interact with to instantiate components.
This is defensible since we can take someone directly importing a nuget as a strong signal that they want to actually use those transforms. #Resolved
| /// </summary> | ||
| /// <param name="catalog">The transform's catalog.</param> | ||
| /// <param name="tensorFlowModel">The pre-loaded TensorFlow model. Please see <see cref="LoadTensorFlowModel(TransformsCatalog.TensorFlowTransforms, string)"/> to know more about loading model into memory.</param> | ||
| public static DataViewSchema GetInputSchema(this TransformsCatalog.TensorFlowTransforms catalog, TensorFlowModelInfo tensorFlowModel) |
There was a problem hiding this comment.
GetInputSchema [](start = 37, length = 14)
This API does not seem like any C# API I've seen. It seems more like a C API with a bunch of static functions that you call with a pointer to a structure, rather than anything like what I'd expect in an actual object oriented property. Why is not everything you've added as an extension method, except possibly the creation of what you call now this TensorFlowModelInfo, just a property or a method of that TensorFlowModelInfo? #Resolved
There was a problem hiding this comment.
Tom, I was under the impression that we wanted to have as much of the functionality as possible accessible from MLContext, and expose as few classes as possible, that's why I suggested exposing these methods here. Do you think the LoadTensorFlowModel API should still be an extension method on TransformsCatalog, or should we expose the TensorFlowUtils class for that?
In reply to: 258769868 [](ancestors = 258769868)
There was a problem hiding this comment.
I don't think that's how we've been doing things at all. We use MLContext for things like component creation, but once you, to give the most conspicuous example, create an estimator, the creation of a transformer, the getting of information out of the transformer, the usage of the transformer to transform a data view, that does not involve the MLContext.
You create a model. The model is the object, you use objects by calling methods and properties on those objects, just like everything else. Recall the title of #1098 that is I'd say the central MLContext... one MLContext to create them all, not to use them all.
I gave examples above about how we don't structure our APIs as they are structured here, and I think my reasoning is pretty solid. #Resolved
There was a problem hiding this comment.
Thanks @TomFinley, I hope I addressed your concern here and other places. I am changing it resolved for now.
In reply to: 259531678 [](ancestors = 259531678)
There was a problem hiding this comment.
Marking unresolved... I do not see that anything has changed at all. TensorFlowModel has no meaningful methods on it, everything is still being exposed via these extension methods on top of properties.
In reply to: 259545815 [](ancestors = 259545815,259531678)
There was a problem hiding this comment.
Thanks @TomFinley for feedback. I have refactored the code according to the your comments.
In reply to: 260027159 [](ancestors = 260027159,259545815,259531678)
There was a problem hiding this comment.
Hi @zeahmed, this requires two changes:
First, the API should be methods and properties on returned TensorFlowModelInfo (though please don't call it that -- TensorFlowModel will do). This will make it resemble things like the rest of ML.NET and every other C# library. The current system reminds me of how C libraries are structured.
Consider estimators, transforms, schema, etc. Note that we do not do this:
var trans = mlContext.Transforms.Fit(estimator, trainData);
var predData = mlContext.Transforms.Transform(trans, testData);
mlContext.Schema.GetColumn(mlContext.SchemaOperations.GetSchema(predData), 1);We do this.
var trans = estimator.Fit(trainData);
var predData = trans.Transform(testData);
predData.Schema[1];And we have been doing that and things like that since C++ was invented lo these many years ago. 😄 But seriously, we like our APIs looking like the latter, not the former. At the moment TensorFlowModelInfo is itself just a glorified string. Let's turn it into an actual object.
Second, get rid of TensorFlowCatalog. We should never have empty properties just lying around confusing people. Especially . MLContext.Model is far more appropriate. Remember: only one method here to get the "info," and ubsequent interactions and operations should be done on that object.
| /// </summary> | ||
| public class TensorFlowModelInfo | ||
| public sealed class TensorFlowModelInfo | ||
| { | ||
| internal TFSession Session { get; } | ||
| public string ModelPath { get; } |
There was a problem hiding this comment.
public [](start = 8, length = 6)
I think this can also be internal. #Resolved
| /// </summary> | ||
| public class TensorFlowModelInfo | ||
| public sealed class TensorFlowModelInfo |
There was a problem hiding this comment.
TensorFlowModelInfo [](start = 24, length = 19)
Would it be possible to rename this to TensorFlowModel? #Resolved
| @@ -45,7 +45,7 @@ public static void Example() | |||
| // Load the TensorFlow model once. | |||
| // - Use it for quering the schema for input and output in the model | |||
| // - Use it for prediction in the pipeline. | |||
| var modelInfo = TensorFlowUtils.LoadTensorFlowModel(mlContext, modelLocation); | |||
| var modelInfo = mlContext.Transforms.LoadTensorFlowModel(modelLocation); | |||
There was a problem hiding this comment.
mlContext.Transforms.LoadTensorFlowModel [](start = 28, length = 40)
This doesn't seem like a transform. This seems like you're loading a model, so should it be under "model?" #Resolved
There was a problem hiding this comment.
Sounds reasonable. I see some of the ONNX model related methods are available via model catalog. I will move it. Thanks!
In reply to: 259530418 [](ancestors = 259530418)
| @@ -45,7 +45,7 @@ public static void Example() | |||
| // Load the TensorFlow model once. | |||
| // - Use it for quering the schema for input and output in the model | |||
| // - Use it for prediction in the pipeline. | |||
| var modelInfo = TensorFlowUtils.LoadTensorFlowModel(mlContext, modelLocation); | |||
| var modelInfo = mlContext.Transforms.LoadTensorFlowModel(modelLocation); | |||
| var schema = modelInfo.GetModelSchema(); | |||
| var featuresType = (VectorType)schema["Features"].Type; | |||
There was a problem hiding this comment.
Features [](start = 51, length = 8)
Can we add a sample that uses modelInfo.GetInputSchema() to find out what the name of the input node is?
#Resolved
There was a problem hiding this comment.
I see its being used at a couple of places in the tests e.g.
| @@ -20,20 +19,20 @@ namespace Microsoft.ML.Transforms | |||
| /// </item> | |||
| /// </list> | |||
| /// </summary> | |||
| public class TensorFlowModelInfo | |||
| public sealed class TensorFlowModel | |||
There was a problem hiding this comment.
I think possibly maybe there's some misunderstanding. Just to be explicit, I expect to see methods used to query the model to be on the model, that is, they will be just methods and properties of the model. Creatingtransformers, querying the schema, or whatever, will be here. The implication is that nearly everything in TensorflowCatalog will be moved out of there, and into here. (Except for loading the model, which of course must be on the model operations catalog.) #Resolved
| @@ -59,7 +60,7 @@ public static class TensorflowCatalog | |||
| /// <param name="inputColumnName"> The name of the model input.</param> | |||
| /// <param name="outputColumnName">The name of the requested model output.</param> | |||
| public static TensorFlowEstimator ScoreTensorFlowModel(this TransformsCatalog catalog, | |||
There was a problem hiding this comment.
ScoreTensorFlowModel [](start = 42, length = 20)
This should be an operation on the model. #Resolved
| @@ -79,7 +80,7 @@ public static class TensorflowCatalog | |||
| /// </format> | |||
| /// </example> | |||
| public static TensorFlowEstimator ScoreTensorFlowModel(this TransformsCatalog catalog, | |||
There was a problem hiding this comment.
ScoreTensorFlowModel [](start = 42, length = 20)
Likewise on the model.. #Resolved
| @@ -102,7 +103,16 @@ public static class TensorflowCatalog | |||
| /// <param name="tensorFlowModel">The pre-loaded TensorFlow model.</param> | |||
| public static TensorFlowEstimator TensorFlow(this TransformsCatalog catalog, | |||
There was a problem hiding this comment.
TensorFlow [](start = 42, length = 10)
So this I find pretty confusing. Do we create estimators via this method, or do we work through the model object? #Resolved
Also, do we generally expose APIs creating estimators from Options? From what I've seen the signature is either all the arguments separately, or a params array of ColumnInfo objects. In reply to: 467198148 [](ancestors = 467198148) Refers to: src/Microsoft.ML.TensorFlow/TensorflowCatalog.cs:94 in fc188cd. [](commit_id = fc188cd, deletion_comment = False) |
I think we are exposing APIs with Options. e.g. . Do you think it should not be exposed?In reply to: 467552241 [](ancestors = 467552241,467198148) Refers to: src/Microsoft.ML.TensorFlow/TensorflowCatalog.cs:94 in fc188cd. [](commit_id = fc188cd, deletion_comment = False) |
In In reply to: 467668930 [](ancestors = 467668930,467552241,467198148) Refers to: src/Microsoft.ML.TensorFlow/TensorflowCatalog.cs:94 in fc188cd. [](commit_id = fc188cd, deletion_comment = False) |
@TomFinley's idea is not to have those methods in In reply to: 467939019 [](ancestors = 467939019,467668930,467552241,467198148) Refers to: src/Microsoft.ML.TensorFlow/TensorflowCatalog.cs:94 in fc188cd. [](commit_id = fc188cd, deletion_comment = False) |
There are I think two distinct things being discussed. One is the presence of advanced options. @yaeldekel is correct, we generally try to avoid having these advanced options where we can, but we definitely have it as part of the public surface area multiple places where it is unavoidable. It is more common in so-called trainer estimators rather than plain old featurizing estimators. If you can avoid it @zeahmed, I'd do so. But I could easily imagine that something as involved as an interface to TensorFlow might justify a bit more complexity. The second question I think is about whether creation of the estimators should be on the transform catalog. I'd say in this case no, since we create the model out of Nonetheless over the past week it's become clear to me that there is deep discomfort about this, so I'll try to address that, by telling you how I'm thinking about this. I tend to think of the introduction of Now then, it just so happens that often these "initial components" are estimators, but I don't attach any special significance to that. We have plenty of cases where this is not the case: operations on So we see, naturally, this progression oftentimes, of and sometimes this parallel structure for ingestion via loaders: As far as I am aware we have hitherto always had the I am saying, I think, that I don't view this chain as being in any way special to the more typical chain. Given that we're creating |
ghost
locked as resolved and limited conversation to collaborators
This PR fixes #2552 and also fixes #2572.
The discussion in these issues are taken as initial feedback to create this PR. The PR groups all the TensorFlow related operations into TensorFlowCatalog. This catalog now appears as a separate catalog in TransformsCatalog e.g.
ScoreTensorFlowModelthat appeared underTransformscatalog asnow appears under
TensorFlowasHow breaking is this change at this stage?
Also, the PR also include following methods in TensorFlow catalog.
There is also a suggestion to add these methods into
DataCatalog. Feedback is requested in this regard.CC: @TomFinley, @yaeldekel, @rogancarr, @Ivanidzo4ka