-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Load onnx model from Stream of bytes #7254
Conversation
var tempModelFile = Path.Combine(((IHostEnvironmentInternal)env).TempFilePath, Path.GetRandomFileName()); | ||
using (var fileStream = File.Create(tempModelFile)) | ||
{ | ||
modelBytes.Seek(0, SeekOrigin.Begin); |
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.
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.
@michaelgsharp what are your thoughts on this?
modelBytes.Seek(0, SeekOrigin.Begin); | ||
modelBytes.CopyTo(fileStream); | ||
} | ||
return new OnnxModel(tempModelFile, gpuDeviceId, fallbackToCpu, |
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.
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.
Thats something I want to do as well, but this will at least unblock people and then I can come back and look into it. I am not sure if ONNX supports that or not (though they probably do).
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.
just a suggestion you may decide not to apply :-)
Instead of exposing CreateFromStream
, would it make sense to expose a new constructor for OnnxModel
which take the stream and make the implementation inside this constructor as the one provided here? This will avoid exposing extra method if we decided later to support streams in OnnxModel. This is minor point though. I am fine either way.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7254 +/- ##
==========================================
+ Coverage 68.77% 68.78% +0.01%
==========================================
Files 1462 1463 +1
Lines 272261 272407 +146
Branches 28176 28183 +7
==========================================
+ Hits 187254 187386 +132
- Misses 77764 77778 +14
Partials 7243 7243
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Fixes #6591 by adding an overload API to allow the ONNX model to be passed in as a Stream of bytes.