sparseml icon indicating copy to clipboard operation
sparseml copied to clipboard

Fixing both torch 1.9/1.12 tests

Open corey-nm opened this issue 3 years ago • 4 comments

This PR adds testing support for analyzer models and node shape for both torch 1.9 and 1.12.

  • Each has a data directory with both torch-1.9 and torch-1.12 directories.
  • Each directory contains a json of expected data, moved out of the python code into json

Test plan: ran unit tests locally with torch 1.9 and torch 1.12. Will be adding both to GH workflows in the feature branch

Why this is needed:

  • onnx exports change throughout each release. For example between 1.9.1 and 1.12.1, then ids of all nodes have changed, and in 1.12.0 some of the node shapes are null

corey-nm avatar Oct 03 '22 17:10 corey-nm

@bfineran @rahul-tuli @KSGulin @dbogunowicz assigned for review

github-actions[bot] avatar Oct 03 '22 17:10 github-actions[bot]

great questions!

what's the expected behavior on torch <1.9?

currently tests will fail with FileNotFoundError. we can explicitly mark with a skip depending on supported torch version, but then it may silently be skipped if you don't have the right one.

How do we plan on updating for future releases?

probably need to have a mapping of version -> file name. for versions that have different behavior we'd need different files with how i've set this up so far.

i still need to test for 1.12.0 and 1.12.1, they may actually have different behavior.

how many versions do we want to cover?

do we have a way to auto-manually update the json test files? (thinking something like we have in place here

currently no, i'd probably prefer to do that outside of these feature branches just to keep the changes to a minimum. also hesitant to enable auto creation of truth for unit tests, we probably just need to move to a different way of testing at that point

corey-nm avatar Oct 03 '22 19:10 corey-nm

Could we update the description of this PR, with a small note stating why the changes were needed, as in what changed b/w torch 1.9 and torch 1.12; It will be helpful when looking at the diff on a later date. Kudos on the code!

rahul-tuli avatar Oct 04 '22 03:10 rahul-tuli

do we have a way to auto-manually update the json test files? (thinking something like we have in place here

currently no, i'd probably prefer to do that outside of these feature branches just to keep the changes to a minimum. also hesitant to enable auto creation of truth for unit tests, we probably just need to move to a different way of testing at that point

it should just be adding 3 lines of code - if there were ever an update to the analyzer, or even just updating python version, someone would have to manually create the files in the same expected format. want to make it clear that 1) this wouldn't update the files every time, only when you run with the env var. 2) any changes to the gold files would still have to be checked in through review/version control

(accidentally edited instead of quote replied previous comment, reverted)

bfineran avatar Oct 04 '22 13:10 bfineran

Closing this and will be re-doing as:

  1. Download models from sparsezoo
  2. analyzer those. This will decouple it from torch versions and also test more edge cases

corey-nm avatar Oct 13 '22 14:10 corey-nm