Expose the Onnx runtime option for setting the number of threads#5962
Expose the Onnx runtime option for setting the number of threads#5962michaelgsharp merged 5 commits intodotnet:mainfrom
Conversation
| bool fallbackToCpu = false, | ||
| int? interOpNumThreads = default, | ||
| int? intraOpNumThreads = default) | ||
| => new OnnxScoringEstimator(CatalogUtils.GetEnvironment(catalog), new[] { outputColumnName }, new[] { inputColumnName }, |
There was a problem hiding this comment.
This is where the API compat is failing. You will need either need to make a new method here, or make an onnx options class that encapsulates these.
@eerhardt do you think an onnx options class would be better?
There was a problem hiding this comment.
See #1798. The established pattern is:
- A "simple" public API that takes all the required arguments.
- An "advanced" overload that takes the required arguments plus an Options class.
And if (1) is sufficient, we can opt out of (2).
So, yes, I believe the new options should force us to create the Options class here.
| /// ]]> | ||
| /// </format> | ||
| /// </example> | ||
| public static OnnxScoringEstimator ApplyOnnxModel(this TransformsCatalog catalog, |
There was a problem hiding this comment.
Can we get a test added for the new API?
| bool fallbackToCpu = false, | ||
| int? interOpNumThreads = default, | ||
| int? intraOpNumThreads = default) | ||
| => new OnnxScoringEstimator(CatalogUtils.GetEnvironment(catalog), new[] { outputColumnName }, new[] { inputColumnName }, |
There was a problem hiding this comment.
See #1798. The established pattern is:
- A "simple" public API that takes all the required arguments.
- An "advanced" overload that takes the required arguments plus an Options class.
And if (1) is sufficient, we can opt out of (2).
So, yes, I believe the new options should force us to create the Options class here.
Codecov Report
@@ Coverage Diff @@
## main #5962 +/- ##
==========================================
+ Coverage 68.29% 68.30% +0.01%
==========================================
Files 1142 1143 +1
Lines 242803 242840 +37
Branches 25385 25386 +1
==========================================
+ Hits 165825 165878 +53
+ Misses 70296 70290 -6
+ Partials 6682 6672 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
michaelgsharp
left a comment
michaelgsharp
left a comment
There was a problem hiding this comment.
LGTM. Ill merge as soon as the CI is done.
ghost
locked as resolved and limited conversation to collaborators
5 participants
When this option is not specified, onnxruntime determines the value from the OS, but when running in a Docker container, the number of threads is determined based on the number of cores on the machine, instead of the number of cores allocated to the container, which causes a slow down in the performance.