Skip to content
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

Onnx load model #5782

Merged
merged 9 commits into from
May 18, 2021
Merged

Onnx load model #5782

merged 9 commits into from
May 18, 2021

Conversation

michaelgsharp
Copy link
Member

Fixes #5766 and #5764 by letting the user specify a temp path location, which defaults to the current location to keep existing behavior, and by make sure the temp onnx model file gets cleaned up by opening it with a delete on close flag.

@michaelgsharp michaelgsharp requested a review from a team May 3, 2021 16:54
@michaelgsharp michaelgsharp self-assigned this May 3, 2021
@@ -348,7 +347,7 @@ public static OnnxModel CreateFromBytes(byte[] modelBytes)
public static OnnxModel CreateFromBytes(byte[] modelBytes, int? gpuDeviceId = null, bool fallbackToCpu = false,
IDictionary<string, int[]> shapeDictionary = null)
{
var tempModelFile = Path.GetTempFileName();
var tempModelFile = Path.Combine(MLContext.TempFilePath, Path.GetRandomFileName());
Copy link
Member

Choose a reason for hiding this comment

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

Path.Combine(MLContext.TempFilePath, Path.GetRandomFileName());

I am seeing this is repeated. would make sense to have a property in MLContext to return the complete random file path?

Copy link
Member

Choose a reason for hiding this comment

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

If we add this, it should be internal. We don't want to expose it publicly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Its a new random file path each time, so we probably wouldn't want it to be a property. I could add an internal method that returned that though.

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

Added some minor comments. otherwise, LGTM.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

The new property shouldn't be static. We should change it to an instance property.

@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

Merging #5782 (0313706) into main (43c49f6) will increase coverage by 0.04%.
The diff coverage is 78.78%.

@@            Coverage Diff             @@
##             main    #5782      +/-   ##
==========================================
+ Coverage   68.35%   68.40%   +0.04%     
==========================================
  Files        1131     1131              
  Lines      241210   241203       -7     
  Branches    25039    25039              
==========================================
+ Hits       164887   164989     +102     
+ Misses      69819    69727      -92     
+ Partials     6504     6487      -17     
Flag Coverage Δ
Debug 68.40% <78.78%> (+0.04%) ⬆️
production 63.03% <80.76%> (+0.05%) ⬆️
test 89.24% <71.42%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Microsoft.ML.AutoML/API/RankingExperiment.cs 69.69% <ø> (ø)
...rc/Microsoft.ML.AutoML/API/RegressionExperiment.cs 58.06% <ø> (ø)
src/Microsoft.ML.Core/Data/IHostEnvironment.cs 97.56% <ø> (ø)
src/Microsoft.ML.TensorFlow/TensorflowTransform.cs 79.48% <0.00%> (ø)
src/Microsoft.ML.Vision/DnnRetrainTransform.cs 60.88% <0.00%> (ø)
...icrosoft.ML.PerformanceTests/FeaturizeTextBench.cs 0.00% <ø> (ø)
...t/Microsoft.ML.PerformanceTests/Harness/Configs.cs 0.00% <0.00%> (ø)
...t/Microsoft.ML.PerformanceTests/TextLoaderBench.cs 0.00% <ø> (ø)
src/Microsoft.ML.Data/MLContext.cs 90.47% <33.33%> (-2.03%) ⬇️
src/Microsoft.ML.AutoML/Experiment/Experiment.cs 72.38% <75.00%> (ø)
... and 27 more

@eerhardt eerhardt dismissed their stale review May 12, 2021 13:59

stale comments

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Yes, this is the direction I had in mind. A few more comments to fix, then after that I think this will be ready to be merged.

src/Microsoft.ML.AutoML/API/ExperimentSettings.cs Outdated Show resolved Hide resolved
src/Microsoft.ML.AutoML/Experiment/Experiment.cs Outdated Show resolved Hide resolved
src/Microsoft.ML.OnnxTransformer/OnnxUtils.cs Outdated Show resolved Hide resolved
src/Microsoft.ML.Vision/ImageClassificationTrainer.cs Outdated Show resolved Hide resolved
test/Microsoft.ML.AutoML.Tests/AutoFitTests.cs Outdated Show resolved Hide resolved
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work here.

@michaelgsharp michaelgsharp merged commit 7fafbf3 into dotnet:main May 18, 2021
@michaelgsharp michaelgsharp deleted the onnx-load-model branch May 18, 2021 16:19
darth-vader-lg pushed a commit to darth-vader-lg/ML-NET that referenced this pull request May 19, 2021
* fixed onnx temp model deleting

* random file path fixed

* updates from pr

* Changes from PR comments.

* Changed how auto ml caches.

* PR fixes.

* Update src/Microsoft.ML.AutoML/API/ExperimentSettings.cs

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

* Tensorflow fixes from PR comments

* fixed filepath issues

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
michaelgsharp added a commit that referenced this pull request May 27, 2021
…#5796)

* Raised the limit of recursions in the creation of the CodedInputStream in the OnnxTransformer (as the default value in the Google.Protobuf). Otherwise some models cannot be loaded (ex. TF2 Efficentdet).

* Updated arcade to the latest version (#5783)

* updated arcade to the latest version

* updated eng/common correctly

* Fixed benchmark test.

* Use dotnet certificate (#5794)

* Use dotnet certificate

* Update 3.1 SDK

Co-authored-by: Prashanth Govindarajan <prgovi@microsoft.com>
Co-authored-by: Michael Sharp <51342856+michaelgsharp@users.noreply.github.com>

* Arm build changes (#5789)

* arm testing

* initial commit with build working on arm64

* windows changes

* build fixes for arm/arm64 with cross compilation

* cross build instructions added

* renamed arm to Arm. Changed TargetArchitecture to default to OS architecture

* fixed some formatting

* fixed capitilization

* fixed Arm Capitilization

* Fix cross-compilation if statement

* building on apple silicon

* removed non build related files

* Changes from PR comments. Removal of FastTreeNative flag.

* Changes from pr comments.

* Fixes from PR comments.

* Changed how we are excluding files.

* Onnx load model (#5782)

* fixed onnx temp model deleting

* random file path fixed

* updates from pr

* Changes from PR comments.

* Changed how auto ml caches.

* PR fixes.

* Update src/Microsoft.ML.AutoML/API/ExperimentSettings.cs

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

* Tensorflow fixes from PR comments

* fixed filepath issues

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

Co-authored-by: Michael Sharp <51342856+michaelgsharp@users.noreply.github.com>
Co-authored-by: Matt Mitchell <mmitche@microsoft.com>
Co-authored-by: Prashanth Govindarajan <prgovi@microsoft.com>
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No way to specify temp file location.
3 participants