[TVMC] Allow manual shape specification in tvmc #7366
Conversation
|
@leandron @jwfromm @mbrookhart Could you take a look at this PR and let me know what you think? |
There was a problem hiding this comment.
Hi @CircleSpin, thanks for the PR - I think shape definition from the outside is something we want to have in tvmc. So much so, that we have discussions going on in #7359.
I think we can use some ideas from the ongoing discussion in #7359, like the syntax to define shapes, and apply shape definition to more frontends, as you have covered here.
I suggest the first thing, we organise which PR takes care of which change.
|
Agree with @leandron. We can actually just keep one PR for both purposes, because PyTorch fix is more like a corner case of supporting custom shape. |
|
Funny that these two very similar PRs landed so close to one another. After reading both, I would argue in favor of choosing this implementation. I think its important to be able to specify a mapping between input name and shape. Not only is this important for constructing a shape dictionary for all non-pytorch frontends, but it also allows a subset of shapes to be overwritten. If a bert model for example had 4 default shapes, this approach would allow us to specify |
|
@CircleSpin @ekalda could you folks organize a plan to work on the one PR and close the other? |
|
From what I can tell, the functionality this PR implements is a superset of the one in my PR, so I'm happy to close mine. |
There was a problem hiding this comment.
Some suggestions (mostly around PyTorch), but I agree with others that it is good to have --shapes as a general parameter :)
Another suggestion (I didn't know where was a good place to add it):
I think it would make sense to add --shapes parameter to tvmc tune as well since tvmc tune also calls frontend.load_model. In fact, since PyTorch models rely on that parameter having a value, we wouldn't be able to tune them without that parameter being part of tvmc tune.
|
Thank you @ekalda for your super thorough feedback! 🏆 We've tried to incorporate it, and also included a test in test_frontends.py that imports a torchvision resnet18 using the new shape specification. I've also added shape inputs to tuning as you had mentioned. Could you take another look and let me know if you have any additional thoughts or feedback? 😄 |
|
Also cc @masahi to review PyTorch shape handling. |
There was a problem hiding this comment.
This LGTM. Thanks for making this change @CircleSpin, it will definitely be helpful to TVMC users!
There was a problem hiding this comment.
LGTM - thanks for the effort in implementing this feature @CircleSpin.
|
I think this one is ready for merge, right? Just need another CI run... |
@CircleSpin please fix or re-trigger the CI. |
|
@masahi the previous build error seems like a torch failure unrelated to TVM but is not at all descriptive. Have you run into that one before? |
It's not related to torch failure. It was because of the 2-hour timeout on CI and has been worked around. |
|
Looks like the new commit failed again in the same way, python aborting during construction of the torchvision model. |
|
Yeah looks wierd. Does it reproduce locally? Maybe CPU node has older torch and torchvision |
|
this works fine locally with the same version of torch and torchvision as CI. Not sure what the deal is. |
|
I don't think doing frontend tests during CPU unittest is a good idea. I suggest moving |
Some test needs to be done to exercise the integration between TVMC and the underlying APIs - we want to keep this to a minimum, and will be reducing the number of tests that actually do this as per previous discussions in #6831 (cc @d-smirnov). In #7304 I have the first test within TVMC that mocks all the APIs and just checks for arguments being passed - I intend to use that one as an example to motivate some refactoring in the TVMC tests to make them easier/faster to run. |
CircleSpin
force-pushed
the
tvmc_shapes
branch
from
c7d04ea
to
ac2dbd6
Compare
|
@comaniac it looks like now the CI issues are gone, can we merge this one when you have a moment? |
|
Thanks @CircleSpin @ekalda @leandron @jwfromm @masahi |
* add ability to optionally overide tvm shapes * add help documentation for --shapes * improve documentation * reformat test_compiler using black * Incorporate feedback from ekalda for better pytorch support and testing. * address feedback * switch input shape syntax to be more pythonic * add commentary * reformat common.py * fix lint issue * format common.py with black * torch/pytorch test hiccup * add -s to setup-pytest-env.sh for clearer error msgs Co-authored-by: Jocelyn <jocelyn@pop-os.localdomain>
|
Quite often I see the following failure in CI builds
Issue: #7471 |
* add ability to optionally overide tvm shapes * add help documentation for --shapes * improve documentation * reformat test_compiler using black * Incorporate feedback from ekalda for better pytorch support and testing. * address feedback * switch input shape syntax to be more pythonic * add commentary * reformat common.py * fix lint issue * format common.py with black * torch/pytorch test hiccup * add -s to setup-pytest-env.sh for clearer error msgs Co-authored-by: Jocelyn <jocelyn@pop-os.localdomain>
* add ability to optionally overide tvm shapes * add help documentation for --shapes * improve documentation * reformat test_compiler using black * Incorporate feedback from ekalda for better pytorch support and testing. * address feedback * switch input shape syntax to be more pythonic * add commentary * reformat common.py * fix lint issue * format common.py with black * torch/pytorch test hiccup * add -s to setup-pytest-env.sh for clearer error msgs Co-authored-by: Jocelyn <jocelyn@pop-os.localdomain>
Currently tvmc does not allow users to overriding specific shapes (such as batch sizes) and instead uses what is defined within the model file. In most cases this is most practical, but in there are some cases that would benefit from additional flexibility i.e. undefined shape sizes, fiddling with batch sizes. This PR adds support for manually specifying input shapes. :)