Skip to content

Conversation

@neginraoof
Copy link
Contributor

Export of view op with dynamic input shape is broken when using tensors with a 0-dim.
This fix removes symbolic use of static input size to fix this issue.

@dr-ci
Copy link

dr-ci bot commented Aug 25, 2020

💊 CI failures summary and remediations

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


  • 2/2 failures possibly* introduced in this PR
    • 2/2 non-CircleCI failure(s)

Extra GitHub checks: 1 failed


codecov.io: 1 failed


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 57 times.

@mruberry mruberry requested a review from houseroad August 26, 2020 04:22
@mruberry mruberry added module: onnx Related to torch.onnx triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Aug 26, 2020
@BowenBao
Copy link
Collaborator

failed tests may be related

Aug 27 19:29:00 FAILED test/onnx/test_pytorch_onnx_onnxruntime.py::TestONNXRuntime_opset7::test_view_dynamic_zero_dim
Aug 27 19:29:00 FAILED test/onnx/test_pytorch_onnx_onnxruntime.py::TestONNXRuntime_opset8::test_view_dynamic_zero_dim

@codecov
Copy link

codecov bot commented Sep 1, 2020

Codecov Report

Merging #43558 into master will increase coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #43558      +/-   ##
==========================================
+ Coverage   67.85%   67.87%   +0.01%     
==========================================
  Files         384      384              
  Lines       50020    50005      -15     
==========================================
- Hits        33942    33939       -3     
+ Misses      16078    16066      -12     
Impacted Files Coverage Δ
torch/onnx/symbolic_opset12.py 25.00% <0.00%> (ø)
torch/onnx/symbolic_opset8.py 30.00% <0.00%> (+1.34%) ⬆️
torch/onnx/symbolic_opset9.py 34.85% <0.00%> (+0.08%) ⬆️
torch/utils/_benchmark/utils/common.py 77.68% <0.00%> (-1.66%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6895d4...bb3b5b7. Read the comment docs.

@neginraoof
Copy link
Contributor Author

cc @bzinodev Jenkins failures are unrelated. Can we re-trigger these to unblock merge?

@BowenBao
Copy link
Collaborator

Test failures are related, seems related to my last suggestion.

@gchanan gchanan added this to the 1.7.0 milestone Sep 21, 2020
@BowenBao
Copy link
Collaborator

test failures seem related

@neginraoof
Copy link
Contributor Author

@BowenBao
Thanks! I updated the symbolic. Looks like the line size = sym_help._maybe_get_const(size, 'is') in view symbolic is required to handle cases where the view size is an empty list.

Copy link
Collaborator

@BowenBao BowenBao left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Contributor

@bzinodev merged this pull request in a77d633.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged module: onnx Related to torch.onnx open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants