[ONNX] Squeeze operator should give an error when trying to apply to a dimension with shape > 1 #38476

yaeldekel · 2020-05-14T14:39:52Z

The ONNX spec for the Squeeze operator:

Remove single-dimensional entries from the shape of a tensor. Takes a parameter axes with a list of axes to squeeze. If axes is not provided, all the single dimensions will be removed from the shape. If an axis is selected with shape entry not equal to one, an error is raised.

Currently, as explained in issue #36796, it is possible to export such a model to ONNX, and this results in an exception from ONNX runtime.

Fixes #36796.

dr-ci · 2020-05-14T14:51:12Z

💊 CI failures summary and remediations

As of commit a0d1116 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 78 times.

neginraoof · 2020-05-26T15:43:49Z

You can catch flake8 issues locally using command: flake8 .
Thanks!

ezyang · 2020-05-26T20:05:34Z

@neginraoof when this is ready to land ping and I can land it

houseroad

Could you rebase to master and make sure all related tests passing?

houseroad

Please make sure all the ci passing:

Lint / flake8-py3 (pull_request)
onnx

neginraoof

Thanks! I added a few comments.

torch/onnx/utils.py

torch/onnx/symbolic_opset9.py

test/onnx/test_pytorch_onnx_onnxruntime.py

facebook-github-bot

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

neginraoof

LGTM. Thanks!

neginraoof · 2020-06-30T18:56:43Z

@yaeldekel
Test failures don't look related. Can you verify this?

houseroad

Could you reabse to the master and rerun the CI?

neginraoof · 2020-07-10T04:29:18Z

torch/onnx/utils.py

Can we add attributes within the addNode c++ method?

houseroad · 2020-07-10T20:00:49Z

torch/onnx/symbolic_opset11.py

Can we directly check the self's size instead of inserting IF-else block?

The size is not always available at export time, so we needed to add an if-else block.

houseroad

Please resolve the conflict.

houseroad · 2020-08-07T17:10:00Z

We still have conflict, could you rebase the PR to master? @yaeldekel

houseroad · 2020-08-14T06:56:40Z

Could you please resolve the conflicts?

… outputs in opset 9

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 · 2020-08-18T02:15:09Z

@houseroad merged this pull request in 3c5e396.

vadimkantorov · 2020-08-18T09:54:35Z

@yaeldekel Could you please explain the new behavior? Does it now emulate PyTorch behavior in ONNX? Does it now prohibit the squeeze(dim) on non-singleton dim from PyTorch?

troymyname · 2024-04-03T19:20:12Z

So...what's the outcome of this issue?

pytorchbot added the open source label May 14, 2020

jerryzh168 requested a review from houseroad May 15, 2020 20:51

jerryzh168 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 15, 2020

ezyang requested review from neginraoof and removed request for houseroad May 26, 2020 20:05

yaeldekel requested a review from apaszke as a code owner May 27, 2020 06:27

yaeldekel force-pushed the squeeze branch from a2ab18b to dba3b1a Compare May 27, 2020 09:12

houseroad requested changes Jun 2, 2020

View reviewed changes

yaeldekel force-pushed the squeeze branch from 66a1e7f to c67322d Compare June 2, 2020 14:36

houseroad reviewed Jun 4, 2020

View reviewed changes

yaeldekel force-pushed the squeeze branch from 2097ee6 to 81fe6ef Compare June 4, 2020 12:20

yaeldekel force-pushed the squeeze branch from 81fe6ef to 56183d0 Compare June 15, 2020 14:15

neginraoof reviewed Jun 18, 2020

View reviewed changes

torch/onnx/utils.py Outdated Show resolved Hide resolved

torch/onnx/symbolic_opset9.py Outdated Show resolved Hide resolved

torch/onnx/symbolic_opset9.py Outdated Show resolved Hide resolved

test/onnx/test_pytorch_onnx_onnxruntime.py Outdated Show resolved Hide resolved

houseroad self-requested a review June 22, 2020 06:02

facebook-github-bot reviewed Jun 22, 2020

View reviewed changes

yaeldekel force-pushed the squeeze branch from 8a5732d to 844fb69 Compare June 30, 2020 06:24

neginraoof approved these changes Jun 30, 2020

View reviewed changes

houseroad reviewed Jul 6, 2020

View reviewed changes

yaeldekel force-pushed the squeeze branch from 844fb69 to d3e4e90 Compare July 7, 2020 10:33

neginraoof reviewed Jul 10, 2020

View reviewed changes

torch/onnx/utils.py Outdated

Copy link

Contributor

neginraoof Jul 10, 2020 •

edited

Loading

Choose a reason for hiding this comment

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

Can we add attributes within the addNode c++ method?

houseroad reviewed Jul 10, 2020

View reviewed changes

bzinodev self-requested a review July 16, 2020 19:48

houseroad self-requested a review July 30, 2020 17:10

houseroad reviewed Jul 30, 2020

View reviewed changes

bzinodev approved these changes Aug 4, 2020

View reviewed changes

houseroad self-requested a review August 7, 2020 17:10

yaeldekel force-pushed the squeeze branch from 9103d16 to 0390725 Compare August 8, 2020 07:04

yaeldMS added 17 commits August 14, 2020 10:56

Disallow squeeze on dimensions > 1, per ONNX spec.

ac4889b

add select test

6787000

add test for select

831c961

add If operator in opset11

f929b6b

continue attempts

3b73efb

Add If node for squeeze

fad6ad4

run clang-format

c5e549c

fix export in opset 9 because if doesn't support different shapes for…

4f8c865

… outputs in opset 9

fix flake issues

1ef51e9

fix flake issues

b626be1

Fix test failure

9dc1157

Change opset 9 logic of Squeeze

694be51

fix flake8 issues

9c09090

address code review comments

7370fd1

Move addNode to a different file

5b5db6c

run clang-format

c43ed1b

clang format

a0d1116

yaeldekel force-pushed the squeeze branch from f8689e7 to a0d1116 Compare August 14, 2020 08:02

facebook-github-bot reviewed Aug 17, 2020

View reviewed changes

facebook-github-bot closed this in 3c5e396 Aug 18, 2020

facebook-github-bot added the merged label Aug 18, 2020

mruberry added the Merged label Oct 28, 2020

[ONNX] Squeeze operator should give an error when trying to apply to a dimension with shape > 1 #38476

[ONNX] Squeeze operator should give an error when trying to apply to a dimension with shape > 1 #38476

Uh oh!

Conversation

yaeldekel commented May 14, 2020

Uh oh!

dr-ci bot commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

neginraoof commented May 26, 2020

Uh oh!

ezyang commented May 26, 2020

Uh oh!

houseroad 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!

neginraoof left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

neginraoof left a comment

Choose a reason for hiding this comment

Uh oh!

neginraoof commented Jun 30, 2020

Uh oh!

houseroad left a comment

Choose a reason for hiding this comment

Uh oh!

neginraoof Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

houseroad Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

yaeldekel Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

houseroad left a comment

Choose a reason for hiding this comment

Uh oh!

houseroad commented Aug 7, 2020

Uh oh!

houseroad commented Aug 14, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Aug 18, 2020

Uh oh!

vadimkantorov commented Aug 18, 2020

Uh oh!

troymyname commented Apr 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

dr-ci bot commented May 14, 2020 •

edited

Loading

neginraoof Jul 10, 2020 •

edited

Loading