-
Notifications
You must be signed in to change notification settings - Fork 25
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
[Bugfix] Point OnnxGz files to model.onnx
but download and extract model.onnx.tar.gz
#357
Conversation
model.onnx
but download and extract model.onnx.tar.gz
Raise FileNotFoundError if model.onnx could not be extracted/found Unzip ModelGz irrespective of parent dir existence Fix Typing in a few places Use generators with all Do not rely on `super.path` as the `_path` variable is mangled in superclass Write Invariants and post conditions for OnnxGz
19c582c
to
caad65c
Compare
Fix OnnxGz path for cases when Model is created from local files Move `not isinstance(file, Directory)` check first to avoid calling `.path` on Directory objects unwillingly Add tests with mobilenet to check Onnxgz extraction using local files and model stub
… extracted in place of the onnx_model
Fix test harness to work with model.onnx.tar.gz and model.data files
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'm pretty sure the file we want to unzip is model.tar.gz and not model.onnx.tar.gz if the purpose of this PR is to extract the model.data file.
The 2B codegen stub shows model.tar.gz as 6.4GB and model.onnx.tar.gz as only 594KB.
In general, I think it would be a good idea to upload a small model to sparsezoo that has its data stored externally so we can catch the issue in the test suite without having to run giant models
making a comment here that the linked model needs to be updated on sparsezoo; and the onnx model |
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.
Two comments. otherwise LGTM
- Update info log -> debug - Update assumption in docstring
Big LLMS are generally saved with external data; this external data along with model.onnx live inside
model.onnx.tar.gz
tarball on sparsezoo; recently we added support to download the tarfile and emulate a normal onnx model file using a wrapper classOnnxGz
; when adding this support a bug was introduced wheremodel.onnx
was present twice in the model path; leading to errors while creating ort sessions asTEST PLAN:
Test Script:
Before this PR: The onnx_model_path had an extra model.onnx in it's path leading to errors like:
https://github.com/neuralmagic/deepsparse/actions/runs/5869604261/job/15914935604?pr=1150#step:7:138
After this PR the test script runs successfully, and the base test errors on deepsparse side are green