-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Fix RuntimeError of loading model with external tensor #6119
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
Conversation
|
All updated. Thank you for the review and this PR is ready. |
onnxruntime/core/graph/graph.cc
Outdated
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.
why it must be ended by '/'?
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.
https://github.com/onnx/onnx/blob/bd2ff65b775580d21ab38cb655a04854c348ff49/onnx/checker.cc#L133
It is required here because the code in ONNX does not handle path join...
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.
Then onnx should fix it! It is always safe to add an additional path separator there when you do path join. But, it seems a little bit wired to do it here without knowing the result will only be used in path join.
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.
And instead of calling std::ifstream, it should use fstat to test if a file exists: https://linux.die.net/man/2/fstat
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.
Makes sense. I am proposing a PR in ONNX to fix it before checking this PR in. Thank you for bringing this up.
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.
Here is the PR to fix that in ONNX: onnx/onnx#3214
|
A path join fix has been merged in ONNX: onnx/onnx#3214. Then this PR would be easier. Set the model directory without giving the separator and update ONNX commit. This PR is ready for review. Thanks. |
Description:
Motivation and Context
onnx/onnx#3157
Reload and save the onnx model with external data format will cause runtime error: