-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Updating the ApplyOnnxModel transform to meet the API parameter ordering standards #3086
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,15 +33,15 @@ public static OnnxScoringEstimator ApplyOnnxModel(this TransformsCatalog catalog | |
/// Applies a pre-trained Onnx model. | ||
/// </summary> | ||
/// <param name="catalog">The transform's catalog.</param> | ||
/// <param name="modelFile">The path of the file containing the ONNX model.</param> | ||
/// <param name="outputColumnName">The output column resulting from the transformation.</param> | ||
/// <param name="inputColumnName">The input column.</param> | ||
/// <param name="modelFile">The path of the file containing the ONNX model.</param> | ||
/// <param name="gpuDeviceId">Optional GPU device ID to run execution on, <see langword="null" /> to run on CPU.</param> | ||
/// <param name="fallbackToCpu">If GPU error, raise exception or fallback to CPU.</param> | ||
public static OnnxScoringEstimator ApplyOnnxModel(this TransformsCatalog catalog, | ||
string modelFile, | ||
string outputColumnName, | ||
string inputColumnName, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
should this default to the outputColumnName like the other inputColumns? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went back and forth on this. I don't think we need to have this as an "in-place" transform because we expect models to create new columns, similar to the TensorFlow transform. |
||
string modelFile, | ||
int? gpuDeviceId = null, | ||
bool fallbackToCpu = false) | ||
=> new OnnxScoringEstimator(CatalogUtils.GetEnvironment(catalog), new[] { outputColumnName }, new[] { inputColumnName }, modelFile, gpuDeviceId, fallbackToCpu); | ||
|
@@ -50,15 +50,15 @@ public static OnnxScoringEstimator ApplyOnnxModel(this TransformsCatalog catalog | |
/// Applies a pre-trained Onnx model. | ||
/// </summary> | ||
/// <param name="catalog">The transform's catalog.</param> | ||
/// <param name="modelFile">The path of the file containing the ONNX model.</param> | ||
/// <param name="outputColumnNames">The output columns resulting from the transformation.</param> | ||
/// <param name="inputColumnNames">The input columns.</param> | ||
/// <param name="modelFile">The path of the file containing the ONNX model.</param> | ||
/// <param name="gpuDeviceId">Optional GPU device ID to run execution on, <see langword="null" /> to run on CPU.</param> | ||
/// <param name="fallbackToCpu">If GPU error, raise exception or fallback to CPU.</param> | ||
public static OnnxScoringEstimator ApplyOnnxModel(this TransformsCatalog catalog, | ||
string modelFile, | ||
string[] outputColumnNames, | ||
string[] inputColumnNames, | ||
string modelFile, | ||
int? gpuDeviceId = null, | ||
bool fallbackToCpu = false) | ||
=> new OnnxScoringEstimator(CatalogUtils.GetEnvironment(catalog), outputColumnNames, inputColumnNames, modelFile, gpuDeviceId, fallbackToCpu); | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see at line 20 where session is loaded. Does the
ApplyOnnxModel
reload the model? If it does then there needs to be a way to reuse thesession
object created above. #WontFixThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good point. Can you file an issue on this? We can do this as we start to fix outstanding issues in the ONNX implementation.
In reply to: 268890248 [](ancestors = 268890248)