bug fix 19374 - fix for upsample export #20116

pk-g · 2019-05-03T21:48:52Z

No description provided.

pk-g · 2019-05-03T21:50:02Z

bug fix #19374

houseroad

Looks good, can you add a test case to cover the code you add?

Here is an example: https://github.com/pytorch/pytorch/blob/master/test/onnx/test_pytorch_onnx_caffe2.py#L958

pk-g · 2019-05-06T23:38:35Z

Looks good, can you add a test case to cover the code you add?

Here is an example: https://github.com/pytorch/pytorch/blob/master/test/onnx/test_pytorch_onnx_caffe2.py#L958

Sure, thanks for the reference. Added the test to cover new code.

facebook-github-bot

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

spandantiwari · 2019-05-09T00:50:09Z

We should enable the test for Upsample in test_operators.py also.

pytorch/test/onnx/test_operators.py

Line 476 in 4d676d5

    
           @unittest.skip("Temporary - waiting for https://github.com/onnx/onnx/pull/1773.")

mchalecki · 2019-05-10T10:49:53Z

This bug occurs also in other upsample methods, especially bilinear. I think the fix should be also applied there.
Also why did you delete
@parse_args('v', 'is')

mchalecki · 2019-05-10T13:15:44Z

torch/onnx/symbolic.py

This shouldn't be deleted

I guess there has been updates to upsample that for the argument output_size it could be either a tensor or a int list. Thus the annotation becomes @parse_args('v', 'v') which can be ignored.

That's right, as @BowenBao mentioned adding @pars_args('v','v') will be redundant.

houseroad

See inline comments

pk-g · 2019-05-15T00:44:42Z

1. This bug occurs also in other upsample methods, especially bilinear. I think the fix should be also applied there.

@Alcaster Good point, we should definitely address other methods as well. But since in this PR we are trying to fix and unblock issue #19374 (which uses 'nearest' method), how about addressing other methods in a separate PR ?

houseroad · 2019-05-16T17:21:37Z

The failing onnx ci is legit. Please fix the failing tests, thanks.

yzhao30 · 2019-05-20T00:09:16Z

Does this also fix the "bilinear" upsampling export?

mchalecki · 2019-05-20T12:00:04Z

Does this also fix the "bilinear" upsampling export?

@yzhao30 unfortunately not. As mentioned by @pk-g it only unblocks issue #19374

facebook-github-bot

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

houseroad · 2019-05-20T21:22:47Z

@pk-g friendly ping for fix :-)

aidonchuk · 2019-05-22T14:15:39Z

Pls fix =)

pk-g · 2019-05-23T03:46:24Z

@Alcaster , @yzhao30 : I extended the fix for bilinear interpolation as well

facebook-github-bot

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

houseroad · 2019-05-23T17:33:33Z

@pk-g please rebase again to make sure the CI pass. The broken CI has been fixed already. Thanks!

facebook-github-bot

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

houseroad

LGTM, thanks for fixing the problem.

facebook-github-bot · 2019-05-24T02:16:29Z

@houseroad merged this pull request in 93d5503.

pytorchbot added the module: onnx Related to torch.onnx label May 3, 2019

pk-g mentioned this pull request May 3, 2019

ONNX export regression: examples/fast_neural_style fails to export (pytorch_nightly) #19374

Closed

pk-g force-pushed the peykash/upsample_export_fix branch 3 times, most recently from 250dfe6 to d7faf52 Compare May 3, 2019 23:31

ezyang requested a review from houseroad May 6, 2019 14:38

houseroad reviewed May 6, 2019

View reviewed changes

pk-g force-pushed the peykash/upsample_export_fix branch from d7faf52 to 5688c7e Compare May 6, 2019 23:36

pk-g closed this May 7, 2019

pk-g reopened this May 7, 2019

facebook-github-bot reviewed May 8, 2019

View reviewed changes

pk-g force-pushed the peykash/upsample_export_fix branch from 5688c7e to d21c1e7 Compare May 8, 2019 18:52

facebook-github-bot reviewed May 8, 2019

View reviewed changes

mchalecki reviewed May 10, 2019

View reviewed changes

houseroad requested changes May 10, 2019

View reviewed changes

hellock mentioned this pull request May 13, 2019

how to use the tensorboard to visualize the model? open-mmlab/mmdetection#623

Closed

pk-g force-pushed the peykash/upsample_export_fix branch from d21c1e7 to 4c75ed8 Compare May 15, 2019 00:37

facebook-github-bot reviewed May 20, 2019

View reviewed changes

pk-g force-pushed the peykash/upsample_export_fix branch from 4c75ed8 to 60ae121 Compare May 23, 2019 00:23

pk-g force-pushed the peykash/upsample_export_fix branch from 60ae121 to 1867cd0 Compare May 23, 2019 05:22

facebook-github-bot reviewed May 23, 2019

View reviewed changes

pk-g added 4 commits May 23, 2019 10:50

bug fix 19374 - fix for upsample export

11845cf

Adding tests for dynamic sizes

9c9245e

addressing PR feedback

77e5293

fixing tests

d7010eb

pk-g force-pushed the peykash/upsample_export_fix branch from 1867cd0 to d7010eb Compare May 23, 2019 18:31

facebook-github-bot reviewed May 23, 2019

View reviewed changes

houseroad approved these changes May 23, 2019

View reviewed changes

facebook-github-bot closed this in 93d5503 May 23, 2019

facebook-github-bot added the merged label May 24, 2019

ezyang added the open source label Jun 24, 2019

bpinaya mentioned this pull request Jul 16, 2019

upsample_bilinear2d issue when exporting to onnx #22906

Closed

mruberry added the Merged label Oct 28, 2020

bug fix 19374 - fix for upsample export #20116

bug fix 19374 - fix for upsample export #20116

Uh oh!

Conversation

pk-g commented May 3, 2019

Uh oh!

pk-g commented May 3, 2019

Uh oh!

houseroad left a comment

Choose a reason for hiding this comment

Uh oh!

pk-g commented May 6, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

spandantiwari commented May 9, 2019

Uh oh!

mchalecki commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mchalecki May 10, 2019

Choose a reason for hiding this comment

Uh oh!

BowenBao May 14, 2019

Choose a reason for hiding this comment

Uh oh!

pk-g May 15, 2019

Choose a reason for hiding this comment

Uh oh!

houseroad left a comment

Choose a reason for hiding this comment

Uh oh!

pk-g commented May 15, 2019

Uh oh!

houseroad commented May 16, 2019

Uh oh!

yzhao30 commented May 20, 2019

Uh oh!

mchalecki commented May 20, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

houseroad commented May 20, 2019

Uh oh!

aidonchuk commented May 22, 2019

Uh oh!

pk-g commented May 23, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

houseroad commented May 23, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

houseroad left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

mchalecki commented May 10, 2019 •

edited

Loading