Skip to content
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

[Relay][Frontend][ONNX] Fix reshape precompute, and type error #3230

Merged
merged 9 commits into from
Jun 17, 2019

Conversation

jroesch
Copy link
Member

@jroesch jroesch commented May 23, 2019

Fix the code which precomputes shapes, and repair type error by casting literal integers to int32.

@jroesch
Copy link
Member Author

jroesch commented May 23, 2019

cc @tqchen @zhreshold

@jroesch jroesch requested review from zhreshold and tqchen May 23, 2019 01:36
@tqchen
Copy link
Member

tqchen commented May 23, 2019

Please fix the lint error

@tqchen
Copy link
Member

tqchen commented May 24, 2019

@jroesch can you look into the CI error?

@jroesch
Copy link
Member Author

jroesch commented May 28, 2019

I disabled a few tests that require debugging version issues on CI. I propose we merge this, and I'll do a follow up pass afterwards.

See my recent issue #3247.

@zhreshold
Copy link
Member

The CI dependency version issue is really a headache now.

@jroesch
Copy link
Member Author

jroesch commented May 30, 2019

Yeah this is annoying, @zhreshold @tqchen can we bump the ONNX version?

@tqchen
Copy link
Member

tqchen commented May 30, 2019

Ideally, we should make the code compatible with both versions. Please try to do that and let us merge it in. Please send a PR to https://github.com/dmlc/tvm/blob/master/docker/install/ubuntu_install_onnx.sh#L24 to ping the onnx to a specific version. then we can try to upgrade the image

@jroesch
Copy link
Member Author

jroesch commented Jun 4, 2019

Ideally, we should make the code compatible with both versions. Please try to do that and let us merge it in. Please send a PR to https://github.com/dmlc/tvm/blob/master/docker/install/ubuntu_install_onnx.sh#L24 to ping the onnx to a specific version. then we can try to upgrade the image

The problem is that the operator support in ONNX and in the torch export functionality varies across versions, so tests have to be conditionally enabled if we want to test model support. I believe it is important we test the entire pipeline on our CI because these models seem to have a lot of failures on the discuss board lately.

@tqchen
Copy link
Member

tqchen commented Jun 14, 2019

#3374 updates the docker image to the latest state

check_torch_conversion(torchvision.models.resnet18, (1,3,224,224))
# check_torch_conversion(torchvision.models.resnet101, (1,3,224,224))

# def test_alexnet():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for these tests being commented out? Can we add a TODO if it's pending on some feature that isn't implemented yet?

@tqchen tqchen merged commit df6957a into apache:master Jun 17, 2019
@tqchen
Copy link
Member

tqchen commented Jun 17, 2019

Thanks @jroesch @zhreshold this PR has been merged. I need to fix issues in #3374 and will report back when that is ready

@tqchen
Copy link
Member

tqchen commented Jun 17, 2019

Note: I reverted this PR for now due to CI errors, please open a new PR to bring things back, sorry about the trouble.

@zhiics
Copy link
Member

zhiics commented Jun 17, 2019

The problem is because fusion was not performed here:
https://github.com/dmlc/tvm/pull/3230/files#diff-03149f7671cff8be6734838f7707a24dR417

@tqchen
Copy link
Member

tqchen commented Jun 18, 2019

NOTE, now that #3374 has been merged, you should be able to use the latest env

wweic pushed a commit to wweic/tvm that referenced this pull request Jun 26, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Jun 27, 2019
@jroesch jroesch deleted the onnx-shape-of branch February 4, 2021 04:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants