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.

Sign up for GitHub

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

Jump to bottom

[ONNX] Remove usage of isCompleteTensor() in symbolic functions #48162

Closed
wants to merge 18 commits into from
Closed

[ONNX] Remove usage of isCompleteTensor() in symbolic functions #48162

wants to merge 18 commits into from

Conversation

BowenBao
Copy link
Collaborator

isCompleteTensor() only returns true when both scalar type and shape is present. All dimensions in the shape must be static. This high requirement is unnecessary for many use cases such as when only rank or scalar type needs to be known.

@facebook-github-bot facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Nov 18, 2020
@dr-ci
Copy link

dr-ci bot commented Nov 18, 2020 •
edited

💊 CI failures summary and remediations

As of commit dc65d83 (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 49 times.

jiafatom
jiafatom reviewed Nov 18, 2020
View reviewed changes
torch/onnx/symbolic_helper.py Show resolved Hide resolved
torch/onnx/symbolic_opset8.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
torch/onnx/symbolic_opset9.py Outdated Show resolved Hide resolved
torch/onnx/symbolic_opset9.py Outdated Show resolved Hide resolved
@zhangguanheng66 zhangguanheng66 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 Nov 18, 2020
@codecov
Copy link

codecov bot commented Nov 19, 2020 •
edited

Codecov Report

Merging #48162 (dc65d83) into master (6000481) will decrease coverage by 0.01%.
The diff coverage is 23.25%.

@@            Coverage Diff             @@
##           master   #48162      +/-   ##
==========================================
- Coverage   80.74%   80.72%   -0.02%     
==========================================
  Files        1870     1870              
  Lines      201741   201836      +95     
==========================================
+ Hits       162889   162928      +39     
- Misses      38852    38908      +56

jiafatom
jiafatom approved these changes Nov 19, 2020
View reviewed changes
Copy link
Contributor

@jiafatom jiafatom left a comment

Choose a reason for hiding this comment

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

LGTM!

neginraoof
neginraoof reviewed Nov 23, 2020
View reviewed changes
Copy link
Contributor

@neginraoof neginraoof left a comment

Choose a reason for hiding this comment

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

Thanks for all the updates! Looks good. Just left a couple minor comments.

torch/onnx/symbolic_opset9.py Show resolved Hide resolved
torch/onnx/symbolic_opset9.py Show resolved Hide resolved
torch/onnx/symbolic_opset9.py Show resolved Hide resolved
@BowenBao
Copy link
Collaborator Author

lint error is unrelated and happening for all PRs at the moment.

@jiafatom jiafatom mentioned this pull request Dec 2, 2020
Closed
neginraoof
neginraoof approved these changes Dec 4, 2020
View reviewed changes
Copy link
Contributor

@neginraoof neginraoof left a comment

Choose a reason for hiding this comment

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

Looks good. Please maybe rebase for CI failures.

facebook-github-bot
facebook-github-bot reviewed Dec 4, 2020
View reviewed changes
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
facebook-github-bot reviewed Dec 7, 2020
View reviewed changes
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
facebook-github-bot reviewed Dec 7, 2020
View reviewed changes
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
facebook-github-bot reviewed Dec 9, 2020
View reviewed changes
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 e5a98c5.

@thiagocrepaldi thiagocrepaldi mentioned this pull request Jul 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@neginraoof neginraoof neginraoof approved these changes

@jiafatom jiafatom jiafatom approved these changes

@bzinodev bzinodev bzinodev approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@houseroad houseroad Awaiting requested review from houseroad

Assignees
No one assigned
Labels
cla signed Merged module: onnx Related to torch.onnx oncall: jit Add this issue/PR to JIT oncall triage queue open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@BowenBao @facebook-github-bot @neginraoof @jiafatom @bzinodev @zhangguanheng66 @pytorchbot