Fixing ONNX export of logical ops to have correct output datatype #15185
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The failed check seems to be unrelated (test run timed out). |
|
Seeing the same issue again. There's a timeout reported. Also, an assert in blob_test is coming up, but I am not sure if that's the failing test and it seems unrelated. Could someone please take a look and advise? |
|
@spandantiwari no worries, I triggered the rebuild manually. |
|
@houseroad - thanks a lot! |
|
@houseroad - this one failed quite quickly. Looks like some network issue. |
|
Yeah, seems some problem with the docker image. No worries, I think we can ignore it. |
There was a problem hiding this comment.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Hi @houseroad, is this PR ok to merge? |
|
@ezyang it may break some internal pipeline, let me double check. Will send you the feedback soon. |
|
Looks like this broke a test: |
|
@ezyang interesting, the ci didn't catch it. @spandantiwari do you mind to update the expect file and resubmit the PR? Thanks. |
|
@ houseroad - yes, I can do that. I think the CI did not catch it because torch.ne support went in another PR very recently. So this branch might be stale. I can update it. |
|
No worries, feel free to open a new pr. Thanks |
|
OK will do. |
|
@houseroad - I have a new PR #15677 that updates the export of torch.ne and fixes the above test failure. Please take a look when you get a chance. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Jan 3, 2019
…utput datatype (#15677) Summary: This is the an updated version of the earlier PR #15185, since that one was closed. Currently PyTorch ONNX exporter exports the logical ops (lt, gt, le, ge, eq, ne) with output type in corresponding ONNX ops as type tensor(uint8). But ONNX spec allows for only tensor(bool), which is why models that have these ops fail to load properly. This issue is captured in #11339. Part of this issue, relating to the allowed input types, has been fixed in ONNX spec by houseroad. This PR fixes the other part pertaining to output type. Pull Request resolved: #15677 Reviewed By: dzhulgakov Differential Revision: D13568450 Pulled By: houseroad fbshipit-source-id: a6afbea1afdb4edad8f8b1bc492f50b14e5f2fce
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently PyTorch ONNX exporter exports the logical ops (
lt,gt,le,ge,eq) with output type in corresponding ONNX ops as typetensor(uint8). But ONNX spec allows for onlytensor(bool), which is why models that have these ops fail to load properly.This issue is captured in #11339. Part of this issue, relating to the allowed input types, has been fixed in ONNX spec by @houseroad. This PR fixes the other part pertaining to output type.