default Model.onnx_model to target onnx.model.tar.gz #355
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change fixes failing test `tests/sparsezoo/objects/test_directory.py ...F....`
…creation Add model.onnx.tar.gz to expected files Update test_unzipping to force unzip, as the parent directory is already present
rahul-tuli
previously approved these changes
Aug 15, 2023
bfineran
commented
Aug 15, 2023
src/sparsezoo/objects/directories.py
Outdated
| @@ -280,16 +280,17 @@ class OnnxGz(Directory): | |||
| """ | |||
| Special class to handle onnx.model.tar.gz files. | |||
| Desired behavior is that all information about files included in the tarball are | |||
| available however, when the file.path is accessed, it will point only to the | |||
| `model.onnx` as this is the expected behavior for loading, additionally, | |||
| available however, when the file's `path` property is accessed, it will point only | |||
There was a problem hiding this comment.
annoying, but apostrophes ' are not well supported by Sphinx, they tend to parse these as unclosed strings
Satrat
reviewed
Aug 15, 2023
src/sparsezoo/model/model.py
Outdated
| files, directory_class=OnnxGz, display_name="model.onnx.tar.gz" | ||
| ) | ||
| self.onnx_model: File = ( | ||
| self._file_from_files(files, display_name="model.onnx.") |
There was a problem hiding this comment.
should this be "model.onnx" instead of "model.onnx." ?
bfineran
commented
Aug 15, 2023
rahul-tuli
reviewed
Aug 15, 2023
|
minor updates post approval, merging |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
currently, sparsezoo does not have great support for onnx models saved with external data out of the box. especially for user friendly and deepsparse compatible flows such as
Model.onnx_model.path.Better support will be added in a future backend update - to fix this now, this PR updates the default target for
Model.onnx_modelto be themodel.onnx.tar.gzwhere possible. A helper class is added to enforce that when a user tries to access the onnx model path, the compressed model is downloaded and unzipped and the reference to the model path points only to the extractedmodel.onnxfile where applicablenote - any users who have already tried to download models with external data will need to clear them from cache
test_plan:
Manually verified with both an external model and a legacy model (see below) - would be good to add these as unit tests, however the download times for the external models can take a while as they are >2Gb. Existing functionality should be thoroughly covered in sparsezoo, deepsparse, and sparseml testing