[ONNX] Add full checker mode in torch.onnx.export #83186

titaiwangms · 2022-08-10T16:54:27Z

Stack from ghstack (oldest at bottom):

Fix #82589
Why:

full_check works in onnx::checker::check_model function as it turns on strict_mode in onnx::shape_inference::InferShapes() which I think that was the intention of this part of code.
strict_mode catches failed shape type inference (invalid ONNX model from onnx perspective) and ONNXRUNTIME can't run these invalid models, as ONNXRUNTIME actually rely on ONNX shape type inference to optimize ONNX graph. Why we don't set it True for default? >>> some of existing users use other platform, such as caffe2 to run ONNX model which doesn't need valid ONNX model to run.
This PR doesn't change the original behavior of check_onnx_proto, but add a warning message for those models which can't pass strict shape type inference, saying the models would fail on onnxruntime.

cc @EikanWang @jgong5 @wenzhe-nrv @sanchitintel @ezyang @gchanan

[ghstack-poisoned]

ghstack-source-id: 0f4327f Pull Request resolved: #83186

facebook-github-bot · 2022-08-10T16:54:49Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/83186
✖️ Python docs build was skipped
✖️ C++ docs build was skipped
❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (0 Pending)

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

Expand to see more

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

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

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

Why: 1. Currently, the full_check mode in `_C._check_onnx_proto` does nothing, as `onnx::shape_inference::InferShapes(model)` is included in `onnx::checker::check_model(model)` 2. **full_check** works in `onnx::checker::check_model` function as it turns on **strict_mode** in `onnx::shape_inference::InferShapes()` which I think that was the intention of this part of code. 3. **strict_mode** catches failed shape type inference (invalid ONNX model from onnx perspective) and ONNXRUNTIME can't run these invalid models, as ONNXRUNTIME actually rely on ONNX shape type inference to optimize ONNX graph. Why we don't set it True for default? >>> some of existing users use other platform, such as caffe2 to run ONNX model which doesn't need valid ONNX model to run. [ghstack-poisoned]

ghstack-source-id: 076750e Pull Request resolved: #83186

Why: 1. Currently, the full_check mode in `_C._check_onnx_proto` does nothing, as `onnx::shape_inference::InferShapes(model)` is included in `onnx::checker::check_model(model)` 2. **full_check** works in `onnx::checker::check_model` function as it turns on **strict_mode** in `onnx::shape_inference::InferShapes()` which I think that was the intention of this part of code. 3. **strict_mode** catches failed shape type inference (invalid ONNX model from onnx perspective) and ONNXRUNTIME can't run these invalid models, as ONNXRUNTIME actually rely on ONNX shape type inference to optimize ONNX graph. Why we don't set it True for default? >>> some of existing users use other platform, such as caffe2 to run ONNX model which doesn't need valid ONNX model to run. CI should pass after #83201 [ghstack-poisoned]

Why: 1. Currently, the full_check mode in `_C._check_onnx_proto` does nothing, as `onnx::shape_inference::InferShapes(model)` is included in `onnx::checker::check_model(model)` 2. **full_check** works in `onnx::checker::check_model` function as it turns on **strict_mode** in `onnx::shape_inference::InferShapes()` which I think that was the intention of this part of code. 3. **strict_mode** catches failed shape type inference (invalid ONNX model from onnx perspective) and ONNXRUNTIME can't run these invalid models, as ONNXRUNTIME actually rely on ONNX shape type inference to optimize ONNX graph. Why we don't set it True for default? >>> some of existing users use other platform, such as caffe2 to run ONNX model which doesn't need valid ONNX model to run. [ghstack-poisoned]

Why: ONNX has mismatch checker usage between cpp and python and it's later fixed by onnx/onnx#4386. And since `torch.onnx.export` is using cpp checker for graph-level check,this improvement should be added. Also, this version bump enables #83186 [ghstack-poisoned]

Why: 1. Currently, the full_check mode in `_C._check_onnx_proto` does nothing, as `onnx::shape_inference::InferShapes(model)` is included in `onnx::checker::check_model(model)` 2. **full_check** works in `onnx::checker::check_model` function as it turns on **strict_mode** in `onnx::shape_inference::InferShapes()` which I think that was the intention of this part of code. 3. **strict_mode** catches failed shape type inference (invalid ONNX model from onnx perspective) and ONNXRUNTIME can't run these invalid models, as ONNXRUNTIME actually rely on ONNX shape type inference to optimize ONNX graph. Why we don't set it True for default? >>> some of existing users use other platform, such as caffe2 to run ONNX model which doesn't need valid ONNX model to run. [ghstack-poisoned]

Why: ONNX has mismatch checker usage between cpp and python and it's later fixed by onnx/onnx#4386. And since `torch.onnx.export` is using cpp checker for graph-level check,this improvement should be added. Also, this version bump enables #83186 [ghstack-poisoned]

Why: ONNX had mismatch checker usage between cpp and python and it's later fixed by onnx/onnx#4386. And since `torch.onnx.export` is using cpp checker for graph-level check with older version of ONNX,this improvement should be added. Also, this version bump enables #83186 Updated 12/5/2022: This PR includes ONNX 1.13.0 pre-release (https://github.com/onnx/onnx/tree/rel-1.13.0) cc VitalyFedyunin jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 gujinghui PenghuiCheng jianyuh min-jean-cho yanbing-j Guobing-Chen Xia-Weiwen mlazos soumith voznesenskym yanboliang penguinwu anijain2305 EikanWang chunyuan-w zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 desertfire [ghstack-poisoned]

Fix #82589 Depends: #83201 Why: 1. **full_check** works in `onnx::checker::check_model` function as it turns on **strict_mode** in `onnx::shape_inference::InferShapes()` which I think that was the intention of this part of code. 2. **strict_mode** catches failed shape type inference (invalid ONNX model from onnx perspective) and ONNXRUNTIME can't run these invalid models, as ONNXRUNTIME actually rely on ONNX shape type inference to optimize ONNX graph. Why we don't set it True for default? >>> some of existing users use other platform, such as caffe2 to run ONNX model which doesn't need valid ONNX model to run. 3. This PR doesn't change the original behavior of `check_onnx_proto`, but add a warning message for those models which can't pass strict shape type inference, saying the models would fail on onnxruntime. [ghstack-poisoned]

github-actions · 2023-02-05T03:34:32Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

Fix #82589 Why: 1. **full_check** works in `onnx::checker::check_model` function as it turns on **strict_mode** in `onnx::shape_inference::InferShapes()` which I think that was the intention of this part of code. 2. **strict_mode** catches failed shape type inference (invalid ONNX model from onnx perspective) and ONNXRUNTIME can't run these invalid models, as ONNXRUNTIME actually rely on ONNX shape type inference to optimize ONNX graph. Why we don't set it True for default? >>> some of existing users use other platform, such as caffe2 to run ONNX model which doesn't need valid ONNX model to run. 3. This PR doesn't change the original behavior of `check_onnx_proto`, but add a warning message for those models which can't pass strict shape type inference, saying the models would fail on onnxruntime. cc EikanWang jgong5 wenzhe-nrv sanchitintel [ghstack-poisoned]

Why: ONNX had mismatch checker usage between cpp and python and it's later fixed by onnx/onnx#4386. And since torch.onnx.export is using cpp checker for graph-level check with older version of ONNX,this improvement should be added. Also, this version bump enables #83186 Updated 12/5/2022: This PR includes ONNX 1.13.0 pre-release (https://github.com/onnx/onnx/tree/rel-1.13.0) [ghstack-poisoned]

ONNX had mismatch checker usage between cpp and python and it's later fixed by onnx/onnx#4386. And since `torch.onnx.export` is using cpp checker for graph-level check with older version of ONNX,this improvement should be added. Also, this version bump enables #83186 Updated 12/5/2022: This PR includes ONNX 1.13.0 release (https://github.com/onnx/onnx/tree/rel-1.13.0) For [CVE-2022-25882](https://nvd.nist.gov/vuln/detail/CVE-2022-25882) Pull Request resolved: #90332 Approved by: https://github.com/kit1980, https://github.com/malfet

Fix #82589 Why: 1. **full_check** works in `onnx::checker::check_model` function as it turns on **strict_mode** in `onnx::shape_inference::InferShapes()` which I think that was the intention of this part of code. 2. **strict_mode** catches failed shape type inference (invalid ONNX model from onnx perspective) and ONNXRUNTIME can't run these invalid models, as ONNXRUNTIME actually rely on ONNX shape type inference to optimize ONNX graph. Why we don't set it True for default? >>> some of existing users use other platform, such as caffe2 to run ONNX model which doesn't need valid ONNX model to run. 3. This PR doesn't change the original behavior of `check_onnx_proto`, but add a warning message for those models which can't pass strict shape type inference, saying the models would fail on onnxruntime. cc EikanWang jgong5 wenzhe-nrv sanchitintel [ghstack-poisoned]

titaiwangms · 2023-02-08T18:35:06Z

@pytorchbot merge -g

pytorchmergebot · 2023-02-08T18:36:55Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

pytorchmergebot · 2023-02-08T21:39:41Z

Merge failed

Reason: GraphQL query
fragment PRCheckSuites on CheckSuiteConnection {
edges {
node {
app {
name
databaseId
}
workflowRun {
workflow {
name
}
url
}
checkRuns(first: 50) {
nodes {
name
conclusion
detailsUrl
}
pageInfo {
endCursor
hasNextPage
}
}
conclusion
}
cursor
}
pageInfo {
hasNextPage
}
}

query ($owner: String!, $name: String!, $number: Int!, $cursor: String!) {
repository(name: $name, owner: $owner) {
pullRequest(number: $number) {
commits(last: 1) {
nodes {
commit {
oid
checkSuites(first: 10, after: $cursor) {
...PRCheckSuites
}
}
}
}
}
}
}
, args {'name': 'pytorch', 'owner': 'pytorch', 'number': 83186, 'cursor': 'Y3Vyc29yOnYyOpHPAAAAAoddcug='} failed: [{'message': 'Something went wrong while executing your query. Please include 07C3:7CCA:43256AB:88A6B31:63E4169C when reporting this issue.'}]

Details for Dev Infra team

Raised by workflow job

titaiwangms · 2023-02-08T21:52:39Z

Merge failed

Reason: GraphQL query fragment PRCheckSuites on CheckSuiteConnection { edges { node { app { name databaseId } workflowRun { workflow { name } url } checkRuns(first: 50) { nodes { name conclusion detailsUrl } pageInfo { endCursor hasNextPage } } conclusion } cursor } pageInfo { hasNextPage } }

query ($owner: String!, $name: String!, $number: Int!, $cursor: String!) { repository(name: $name, owner: $owner) { pullRequest(number: $number) { commits(last: 1) { nodes { commit { oid checkSuites(first: 10, after: $cursor) { ...PRCheckSuites } } } } } } } , args {'name': 'pytorch', 'owner': 'pytorch', 'number': 83186, 'cursor': 'Y3Vyc29yOnYyOpHPAAAAAoddcug='} failed: [{'message': 'Something went wrong while executing your query. Please include 07C3:7CCA:43256AB:88A6B31:63E4169C when reporting this issue.'}]

Details for Dev Infra team

Hi @kit1980,

Do you know what issue this might be?

kit1980 · 2023-02-08T21:54:24Z

Looks like GitHub hiccup?
Let's try again.

kit1980 · 2023-02-08T21:54:41Z

@pytorchbot merge

pytorchmergebot · 2023-02-08T21:56:31Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

kit1980 · 2023-02-08T22:01:46Z

torch/csrc/jit/serialization/export.cpp

 }

-void check_onnx_proto(const std::string& proto_string, bool full_check) {
+void check_onnx_proto(const std::string& proto_string) {


This changes public API, no?

I think it does. It's one of TORCH API. Is there anything else I should do?

You need to follow https://github.com/pytorch/pytorch/wiki/PyTorch's-Python-Frontend-Backward-and-Forward-Compatibility-Policy
It basically says first emit a warning for 2 releases, then can delete.
But for this function specifically I'm not sure if it's even used by anyone.
We may need to revert if there are complains.

In future please follow https://github.com/pytorch/pytorch/wiki/PyTorch's-Python-Frontend-Backward-and-Forward-Compatibility-Policy

Thanks! Users should most likely be using the onnx API from onnx repo.

I've added bc breaking labels.

add full checker mode

30025d0

[ghstack-poisoned]

titaiwangms requested review from BowenBao and shubhambhokare1 as code owners August 10, 2022 16:54

This was referenced Aug 10, 2022

[ONNX] Enable test_uninitialized_optional #83183

Closed

[ONNX] Add script/trace different flatten and move optional type tests to runtime #83184

Closed

[ONNX] Move OptionalHasElement tests to runtime #83185

Closed

titaiwangms added a commit that referenced this pull request Aug 10, 2022

add full checker mode

ba60809

ghstack-source-id: 0f4327f Pull Request resolved: #83186

facebook-github-bot added the cla signed label Aug 10, 2022

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Aug 10, 2022

titaiwangms marked this pull request as draft August 10, 2022 16:57

pytorchbot added the open source label Aug 10, 2022

titaiwangms changed the title ~~add full checker mode~~ [ONNX] Add full checker mode in torch.nn.export Aug 10, 2022

titaiwangms mentioned this pull request Aug 10, 2022

[ONNX] Update ONNX version #83201

Closed

titaiwangms added a commit that referenced this pull request Aug 10, 2022

add full checker mode

0ae82da

ghstack-source-id: 076750e Pull Request resolved: #83186

titaiwangms added module: onnx Related to torch.onnx release notes: onnx torch.onnx related changes that should show up in the release notes topic: improvements topic category labels Aug 11, 2022

titaiwangms mentioned this pull request Dec 6, 2022

Update ONNX version #90337

Closed

github-actions bot added the Stale label Feb 5, 2023

titaiwangms added no-stale and removed Stale labels Feb 5, 2023

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Feb 8, 2023

kit1980 reviewed Feb 8, 2023

View reviewed changes

pytorchmergebot added the Merged label Feb 8, 2023

pytorchmergebot closed this in b27ac6d Feb 8, 2023

kit1980 added module: bc-breaking Related to a BC-breaking change topic: bc breaking topic category and removed module: bc-breaking Related to a BC-breaking change labels Feb 8, 2023

pmeier mentioned this pull request Feb 14, 2023

ONNX test failures pytorch/vision#7247

Closed

facebook-github-bot deleted the gh/AllenTiTaiWang/4/head branch June 8, 2023 14:22

[ONNX] Add full checker mode in torch.onnx.export #83186

[ONNX] Add full checker mode in torch.onnx.export #83186

Uh oh!

Conversation

titaiwangms commented Aug 10, 2022 • edited by pytorch-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

✅ No Failures (0 Pending)

Uh oh!

github-actions bot commented Feb 5, 2023

Uh oh!

titaiwangms commented Feb 8, 2023

Uh oh!

pytorchmergebot commented Feb 8, 2023

Merge started

Uh oh!

pytorchmergebot commented Feb 8, 2023

Merge failed

Uh oh!

titaiwangms commented Feb 8, 2023

Merge failed

Uh oh!

kit1980 commented Feb 8, 2023

Uh oh!

kit1980 commented Feb 8, 2023

Uh oh!

pytorchmergebot commented Feb 8, 2023

Merge started

Uh oh!

kit1980 Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

titaiwangms Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

kit1980 Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

kit1980 Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

titaiwangms Feb 8, 2023 • 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!

kit1980 Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

titaiwangms commented Aug 10, 2022 •

edited by pytorch-bot bot

Loading

facebook-github-bot commented Aug 10, 2022 •

edited

Loading

titaiwangms Feb 8, 2023 •

edited

Loading