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

[ONNX] Enable constant folding for Shape #35386

Closed
wants to merge 14 commits into from

Conversation

@KsenijaS
Copy link
Contributor

@KsenijaS KsenijaS commented Mar 25, 2020

Enabled constant folding for onnx::Shape

@dr-ci
Copy link

@dr-ci dr-ci bot commented Mar 25, 2020

💊 CircleCI build failures summary and remediations

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


💚 💚 Looks good so far! There are no CircleCI 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.

See how this bot performed.

This comment has been revised 37 times.

Loading

test/onnx/test_utility_funs.py Outdated Show resolved Hide resolved
Loading
test/onnx/test_utility_funs.py Outdated Show resolved Hide resolved
Loading
@neginraoof
Copy link
Collaborator

@neginraoof neginraoof commented Mar 25, 2020

Also, please check the failures. Looks like test test/onnx/test_operators.py TestOperators.test_expand is failing.
Export of expand op in opset 9 uses ones_like op which uses shape op. This could be expected, please verify and update the expand expected graph file.

Loading

@ngimel ngimel added the triaged label Mar 25, 2020
@ngimel ngimel requested a review from houseroad Mar 25, 2020
Copy link
Collaborator

@neginraoof neginraoof left a comment

Thanks for the fixes. Please check if there's an onnxruntime test for shape and add that if not.

Loading

Copy link
Contributor

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

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

Loading

@KsenijaS
Copy link
Contributor Author

@KsenijaS KsenijaS commented Mar 31, 2020

@houseroad can you merge this PR?

Loading

@neginraoof
Copy link
Collaborator

@neginraoof neginraoof commented Apr 2, 2020

@KsenijaS could you rebase this PR please?

Loading

@KsenijaS
Copy link
Contributor Author

@KsenijaS KsenijaS commented Apr 7, 2020

@houseroad can you please review my PR, it seems that none of the failure are caused by my changes

Loading

Copy link
Member

@houseroad houseroad left a comment

not sure why we need to change onnx::gather part

Loading

@@ -295,7 +299,7 @@ c10::optional<at::Tensor> runTorchBackendForOnnx(
inputTensorValues[0], p, node->is(attr::axes), node->i(attr::keepdims));
return c10::optional<at::Tensor>(updated_val);
} else if (node->kind() == onnx::Gather) {
assert(inputTensorValues.size() == 1);
assert(inputTensorValues.size() == 2);
Copy link
Member

@houseroad houseroad Apr 7, 2020

Choose a reason for hiding this comment

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

why change this?

Loading

Copy link
Contributor Author

@KsenijaS KsenijaS Apr 7, 2020

Choose a reason for hiding this comment

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

gather has 2 inputs, that's why my tests were failing

Loading

Copy link
Collaborator

@neginraoof neginraoof Apr 7, 2020

Choose a reason for hiding this comment

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

This was a bug in gather implementation for constant folding. This node is required to have 2 inputs always (Note that this assertion as not blocking tests on CI, and was caught locally).

Loading

Copy link
Contributor Author

@KsenijaS KsenijaS Apr 8, 2020

Choose a reason for hiding this comment

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

@houseroad if you have no further comments, can this PR be merged?

Loading

@KsenijaS
Copy link
Contributor Author

@KsenijaS KsenijaS commented Apr 8, 2020

@houseroad can we merge this PR?

Loading

Copy link
Collaborator

@BowenBao BowenBao left a comment

LGTM, please update assert to TORCH_INTERNAL_ASSERT

Loading

torch/csrc/jit/passes/onnx/constant_fold.cpp Outdated Show resolved Hide resolved
Loading
Copy link
Contributor

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

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

Loading

Copy link
Member

@houseroad houseroad left a comment

Thanks

Loading

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Apr 12, 2020

@houseroad merged this pull request in 4f728c9.

Loading

1 similar comment
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Apr 12, 2020

@houseroad merged this pull request in 4f728c9.

Loading

ashishfarmer pushed a commit to ashishfarmer/pytorch that referenced this issue Apr 13, 2020
Summary:
Enabled constant folding for onnx::Shape
Pull Request resolved: pytorch#35386

Reviewed By: hl475

Differential Revision: D20682412

Pulled By: houseroad

fbshipit-source-id: 4559a35f174edfb7e6364c0fbf5bc1d55d0d26dc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants